Author Topic: Merging pull requests in Git  (Read 2612 times)

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1013
Merging pull requests in Git
« on: April 22, 2018, 07:45:10 AM »
I've been practicing pull requests and merging them in OP2Utility. When merging a pull request up until now, I've used the default option of merge request which adds a merge commit to the master branch in addition to each commit contained in the branch.

The other two options are squash and merge or rebase and merge. Both of these options so not add a merge commit and can simplify the master tree a little.

Squash and merge squashes all commits into a single commit and removes the separate merge commit. This is nice when you introduce a new feature that has several intermediate commits that do not add value to the master branch after merging.

Rebase and merge also eliminates the merge commit but adds all the individual commits from the branch into the master. This is useful if you want to retain each commit but not muddy the master branch with the merge commit.

The catch is you cannot use squash and merge or rebase and merge if there are merge conflicts. There are some other subtleties mentioned in the GitHub documentation, but this is the overall gist.

In the future I'm going to try using these options to reduce the number of merge commits unless I am merging a substantial branch into master or there are known merge conflicts.

I still have a lot to learn about Git...

-Brett

Offline leeor_net

  • Administrator
  • Hero Member
  • *****
  • Posts: 2350
  • OPHD Lead Developer
    • LairWorks Entertainment
Re: Merging pull requests in Git
« Reply #1 on: April 22, 2018, 08:03:30 AM »
If it makes you feel better, I'm in the same boat of being almost clueless with Git. It seems overly complicated with too many ways to use it but with the 'advanced' users having 'the one true way'. It's almost religious in nature (fairly typical of the community from which Git arose in the first place).

Anyway, I use Git similarly to the way I used SVN -- I just commit after each change set then push after I've completed a feature. Merge commits are fine by me as I'd personally want to preserve that history, that the change came from outside the main development branch from a fork or similar.

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: Merging pull requests in Git
« Reply #2 on: April 27, 2018, 11:22:18 AM »
I usually rebase, since I like having linear history. Perhaps this stems from my early years of using SVN.

Technically, the merge commits preserve more info, and are a more accurate representation of how development actually happened. Some people are pretty vocal about that, and I can see why. A merge commit can give you enough info to determine what commits belonged to the branch after the merge. Without it, you lose branch info, so it's not obvious where the branch boundary lay. For big branch changes, involving multiple independent but related commits, this might be useful info. For smaller changes that fit within one commit, not so much.

I'm not such a fan of squash commits. They can take a series of logical changes and merge them into one big unwieldy commit that is hard to audit. Though I might be a fan if I ran into programmers that made a lot of experimental commits that didn't work. I figure those should be cleaned up before sharing. It's basically a sign of someone committing untested code, which makes me wonder if they should have even made the commit in the first place.

At any rate, accidents do happen, and sometimes a bad commit is made. But, if you catch it before pushing, you can rewrite history using an interactive Git rebase to fix the mistake. That's what I would probably do.


Quote
In the future I'm going to try using these options to reduce the number of merge commits unless I am merging a substantial branch into master or there are known merge conflicts.

I think that's exactly what I would do. Rebase on master for simple changes involving very independent commits, and merge commit for bigger more complicated branches.

As a side note, you can run a rebase just before creating a pull request. That would let you fix any merge conflicts ahead of time.

I'd also be curious as to your experiences if you've ever tried to rebase after creating a pull request. At that point, you are technically rewriting history that is now public to the pull request.

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1013
Re: Merging pull requests in Git
« Reply #3 on: April 28, 2018, 09:21:05 AM »
Hooman,

Good call on performing a rebase before the merge request. I had been branching the code and not attempting to reconcile it with the main branch until after the merge commit was completed. Since I've been averaging about 2-3 branches open at once and pushing occasional simple commits directly on the repo, the branches become stale before the merge commit.

I really like the rebase feature that allows me to move my branch into the future from where it's original base commit. Reconciling more often makes the branch easier to work in. I could see this being really helpful if working on a team where a lot of people are merging in branches and you want to check your current work against their changes often.

I'll try using the rebase command more often now.

Quote
I'd also be curious as to your experiences if you've ever tried to rebase after creating a pull request. At that point, you are technically rewriting history that is now public to the pull request.

Well, not much to say here. I have used GitHub to perform both Squash and Merge & Rebase and merge. I just click the button and it works. For a squash and merge, GitHub allows you to modify the commit message just like you would when squashing commits locally. The GitHub documentation states that you can disable Squash and merge and/or Rebase and merge on a repository. I suspect they allow disabling because as you stated someone is re-writing history on a public branch to perform either action. Also, there is the blurb below, which I'm not sure means anything to me at my current knowledge of Git. (Shrugs)

From https://help.github.com/articles/about-pull-request-merges/
Quote
The rebase and merge behavior on GitHub deviates slightly from git rebase. Rebase and merge on GitHub will always update the committer information and create new commit SHAs, whereas git rebase outside of GitHub does not change the committer information when the rebase happens on top of an ancestor commit.




On a slightly different note, I've been using the Git Stash command lately. Once when I started working on a new feature but forgot to switch out of a branch before starting work. Git wouldn't let me switch out of the branch without recording the changes. I just stashed them, created the new branch, and then used stash pop to move the changes into the new branch.

One of the times I used stash / stash pop, I caused merge conflicts, but I was able to resolve them pretty much just like I would resolve them on a merge commit.

I've found TortoiseGit's tool for resolving merge conflicts very good. I like it more than using KDiff, which is what I use with TortoiseHG. KDiff is a fine tool, especially for being free, so I'm not bashing it by any means.

I would be really curious which workflow was easier/faster for someone that mastered both, using the command line for Git or using TortoiseGit. Perhaps someday I'll try switching to the command line but for now TortoiseGit has been able to perform everything I've needed.

Thanks,
Brett

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: Merging pull requests in Git
« Reply #4 on: April 29, 2018, 08:20:45 AM »
I suspect GitHub allows you to disable Squash and Merge, and Rebase and Merge so you can force people to use merge commits, which preserve branch tracking information. It may also be related to the inherent dishonesty of rewriting history. ;)

As for GitHub's note on the small difference, there is a distinction between an author and a committer. This would be relevant in a code review setting where the reviewer commits the code to the repository, but the code was written by a different author. This happened during the SVN->Git conversion, where the author was listed as the SVN user ID, while the Git account used by the uploader is listed as the committer. You'll notice a small user icon overlayed on top of a larger user icon for those commits.

Git stash is indeed quite useful. That's one reason why I don't like partial untested commits. You can just stash changes instead if you need to run off and do something else for a bit.


I've never much used TortoiseGit, but I did use TortoiseSVN quite a bit on Windows. I always thought TortoiseSVN was quite slick and polished. On Linux I used command line SVN. It's kind of the same, just I have to lookup options a little more often. I'd say I preferred TortoiseSVN, though the difference isn't that big. I imagine the experience with Git is similar, though Git is also a little more powerful and complicated, and my limited experience with TortoiseGit (many years ago) suggested it was slightly less polished.

Offline leeor_net

  • Administrator
  • Hero Member
  • *****
  • Posts: 2350
  • OPHD Lead Developer
    • LairWorks Entertainment
Re: Merging pull requests in Git
« Reply #5 on: April 29, 2018, 01:59:33 PM »
I'm not such a fan of squash commits. They can take a series of logical changes and merge them into one big unwieldy commit that is hard to audit. Though I might be a fan if I ran into programmers that made a lot of experimental commits that didn't work. I figure those should be cleaned up before sharing. It's basically a sign of someone committing untested code, which makes me wonder if they should have even made the commit in the first place.

I'm in agreement here. I see no point in eliminating history and if you're committing untested or broken code you're doing it wrong.

Part of this comes from my work with SVN. I always finished what I was working on before I made a commit. I've been doing the same with Git though I'll make a series of commits before I push. E.g., as I make changes I check said changes then commit. Once the 'feature' is complete I push everything in one shot.

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: Merging pull requests in Git
« Reply #6 on: May 01, 2018, 02:42:31 AM »
Right, that sounds like the proper way to use Git. Plus, by delaying the push, you have a chance to fix bad commits in case you later find an error that you didn't notice when making the commit. I love that about Git.