Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Git email flow vs. GitHub flow

blog.brixit.nl

Post reply on HN
Print view View original

151–160 of 177 posts

Re: Git email flow vs. GitHub flow

#151

Post by brandmeyer »

mugsie wrote: post #119
brandmeyer wrote: post #111

Earlier quoted context omitted.

The other big downside is that enforcing the unit of review to be a single commit instead of a branch tends to encourage over-large commits. Rebasing a series of related commits for gerrit is a major pain.

yeah - that is a pain in the beginning. Personally (this is completely anecdotal, and not backed by any serious data), I have found it drives people to do multiple small commits, as they rebase a lot easier. using something like git review means that the entire chain is rebased by default when you summit a new patch on top, and I have the `git commit --am -a --no-edit && git fetch && git rebase` in muscle memory at t…

> the lack of ability to have PRs based off other open PRs is one of the biggest issues I have with the current ... pull request model

At least in bitbucket, you can just set a branch under review as the target of a pull request and the UI "just works" for that case.

Re: Git email flow vs. GitHub flow

#152

Post by brandmeyer »

mugsie wrote: post #149
u801e wrote: post #143

Earlier quoted context omitted.

How does Gerrit handle a group of commits in a branch? That it, if a feature takes several commits to implement and some of them depend on changes made in earlier commits in the branch, how do you ensure that the earlier commit is approved and applied to the main branch before the later commit?

it groups the branch into a chain of commits, and has dependancies between them, so you can approve a commit on top of the chain, but it will not merge until the ones it is based off merge.

My recollection was that you had to give each commit its own local branch name and both rebase and push them one-at-a-time with Gerrit. Are you saying that an entire branch of commits can be pushed and updated at one time?

Re: Git email flow vs. GitHub flow

#153

Post by mugsie »

brandmeyer wrote: post #151
mugsie wrote: post #119

Earlier quoted context omitted.

yeah - that is a pain in the beginning. Personally (this is completely anecdotal, and not backed by any serious data), I have found it drives people to do multiple small commits, as they rebase a lot easier. using something like git review means that the entire chain is rebased by default when you summit a new patch on top, and I have the `git commit --am -a --no-edit && git fetch && git rebase` in muscle memory at t…

> the lack of ability to have PRs based off other open PRs is one of the biggest issues I have with the current ... pull request model At least in bitbucket, you can just set a branch under review as the target of a pull request and the UI "just works" for that case.

yes - the same for github, but when you go to merge it, it messes up.

Re: Git email flow vs. GitHub flow

#154

Post by mugsie »

brandmeyer wrote: post #152
mugsie wrote: post #149

Earlier quoted context omitted.

it groups the branch into a chain of commits, and has dependancies between them, so you can approve a commit on top of the chain, but it will not merge until the ones it is based off merge.

My recollection was that you had to give each commit its own local branch name and both rebase and push them one-at-a-time with Gerrit. Are you saying that an entire branch of commits can be pushed and updated at one time?

yeah - at least with something like git-review [1] when you do git review on a branch with multiple commits, it creates a chain of commits in gerrit with one command.

1 - https://docs.opendev.org/opendev/git-review/latest/

Re: Git email flow vs. GitHub flow

#155

Post by brandmeyer »

mugsie wrote: post #153
brandmeyer wrote: post #151

Earlier quoted context omitted.

> the lack of ability to have PRs based off other open PRs is one of the biggest issues I have with the current ... pull request model At least in bitbucket, you can just set a branch under review as the target of a pull request and the UI "just works" for that case.

yes - the same for github, but when you go to merge it, it messes up.

> it messes up

Can you be more specific about the failure mode here? If we have a series master -> A_review -> B_review, and B_review is targeted to A_review in the PR tool, then a fast-forwarding merge of A_review to master just advances the master branch name and automatically updates B_review to now target master. No merge commits take place.

Re: Git email flow vs. GitHub flow

#156

Post by WorldMaker »

u801e wrote: post #145
WorldMaker wrote: post #125

Earlier quoted context omitted.

Merge commits for single-commit PRs helpfully record which PR # was merged if you need to review/audit the PR sometime later, if nothing else.

The original commit could be amended to include that information.

The point of using a "proper" merge commit would be to avoid amending/rebasing the original commit and allow the original commit to live as-developed in the final branch.

Re: Git email flow vs. GitHub flow

#157

Post by WorldMaker »

u801e wrote: post #141
WorldMaker wrote: post #118

Earlier quoted context omitted.

Even before Draft PRs existed, I would often encourage especially junior devs to open PRs early and just include a note (in the description or the first commit) with a "Work in Progress" or "Not Ready to Merge" type comment and then either delete that (if in the description) or add a new comment with "Ready to Merge" or similar when ready. It's great to see GitHub add this as a real tool now, but there's always been…

> evolve them rapidly with discussion That, in my experience makes it harder to review the change. To me, it's easier to do something like that while pair programming. Code review should be done once a final approach has been taken and only minor changes or factors that the implementation may not have taken into account should be addressed.

Most PR systems have multiple ways to "slice" the changes coming in to specific updates and giving you "bookmarks" for what you've reviewed prior and what is new in the latest update.

Just because it may not be your preferred workflow doesn't mean it isn't a useful workflow or that the tools don't already exist to make it a manageable workflow.

Re: Git email flow vs. GitHub flow

#158

Post by WorldMaker »

u801e wrote: post #146
WorldMaker wrote: post #121

Earlier quoted context omitted.

Yes, most of the issues this article has with PR branches and needing to force push them stems from the self-imposed "requirement" that they don't like merge commits and don't allow them. Obviously that will make working with PRs much harder than the merge-based workflows that PRs were originally built for and still tend to be best optimized for.

The merge button merges the feature branch into the base branch. The merge commit provides information regarding what commits are in the branch (commits from the first to second parent). Merging the base branch into the feature branch introduces a merge commit that just shows changes to the base branch and conflicts that were addressed. That information could just as well not be there if you created the feature branc…

Every merge is a risk of merge conflicts, of bad auto-resolutions (don't get me started on the circle of pain where you need to understand git rerere and its consequences). Just because there are auto-resolutions and they mostly work (most of the time) automatically doesn't mean that all merges are safe. (Fast-forward only merges being an exception, of course.) Rebasing hides the history of those merges, their conflicts, their resolutions. Merge commits do provide all sorts of useful information in the cases where merges go wrong. That information is entirely lost in rebases, depending on the user that did the rebase and how they do the rebase.

I understand many people consider merge commits noise. I think rebases are destructive and dangerous. I'd rather have an extremely "noisy" git graph that I can manage with UI tools and filters like --first-parent than a "clean" git graph with no way to research and/or fix a bad merge after the fact.

I realize those are often very opposed viewpoints, I'm just offering mine in a thread full of people who don't shudder every time they hear a junior developer attempted a rebase or amended a commit in a branch they shouldn't have.

Re: Git email flow vs. GitHub flow

#159

Post by WorldMaker »

u801e wrote: post #144
WorldMaker wrote: post #123

Earlier quoted context omitted.

> un-bisect-able fixup commits in the final merged master/main branch If you require PRs to create merge commits you get the nice world where git bisect --first-parent bisects at the PR level, you don't have to worry about the individual commits inside the PR/below the PR level when bisecting, but you still have that commit history "as-is" for deep archeological dives when you need it. (And you can use --first-parent…

And those commits rarely provide useful information because they're of the variety where people fix syntax errors, add missing files, remove changes they didn't mean to commit, etc.

There's plenty of information in all those types of commits even if you personally don't find that information "useful". I've had to do the sort of archeology digs to figure out "what syntax errors did a build tool miss", "why is this type of file often missed to be added, and how often do we miss it", "what was still TODO in this feature effort that got removed at the last minute", etc. All of which needs information from those sorts of "low level" commits.

Re: Git email flow vs. GitHub flow

#160

Post by u801e »

WorldMaker wrote: post #158
u801e wrote: post #146

Earlier quoted context omitted.

The merge button merges the feature branch into the base branch. The merge commit provides information regarding what commits are in the branch (commits from the first to second parent). Merging the base branch into the feature branch introduces a merge commit that just shows changes to the base branch and conflicts that were addressed. That information could just as well not be there if you created the feature branc…

Every merge is a risk of merge conflicts, of bad auto-resolutions (don't get me started on the circle of pain where you need to understand git rerere and its consequences). Just because there are auto-resolutions and they mostly work (most of the time) automatically doesn't mean that all merges are safe. (Fast-forward only merges being an exception, of course.) Rebasing hides the history of those merges, their confli…

If you imagine a VCS where merges didn't exist, then what would you do when the code is updated after you started on a change you were making? You probably would get a copy of the most up to date version of the code and try to apply your changes to it and make sure it still works.

That's essentially what rebasing does.

What merges do is basically try to have the VCS apply the code changes you made based off an older version of the code and apply it to the newer version of the code. Conflicts are changes you made to your code or the base code in order to get your code to work.

So, it really comes down to the following:

1. Would a reviewer want to view the changes based on the most up to date version of the code as a set of one or more changes?

or

2. Do they want what's in item 1 and then one or more commits that are autogenerated with a mix of automatically applied changes as determined by the VCS and manual changes that are a mix of changes to the base code and code that's part of the new feature?

I assert that it's easier to deal with option 1 because we can clearly see, in a set of organized commits, what changes were made based on the latest version of the code. With option 2, you mix what's in option 1 with automatically generated commits mixed in with changes that update lines of code that may have been made by multiple commits previously applied to the branch.

> a junior developer attempted a rebase or amended a commit in a branch they shouldn't have.

Developers, in general, should look at the commits in their branch by running:

  git log -p origin/master..
and read through the commit messages and diffs and see if things make sense. They can also test each change by running:

  git rebase --exec "test_command" origin/master..
in order to verify that the changes introduced by each commit didn't break any of the tests.
Post reply on HN