Contribution docs: Explain how to interact with review for your MR

Clearing the Path
,
1

Here’s something that easily happens to a beginner. First, this is a common way a MR begins:

  1. Jane opens an issue or picks an existing one
  2. Jane creates a branch in her fork, to resolve the issue
  3. Jane pushes her commit(s)
  4. Jane marks the MR as ready for review
  5. Jane gets review and comments about her work

But now, the following happens:

  1. Jane makes new tiny little commits on her MR as a reaction to comments
  2. While Jane works, master branch gets new commits, Jane merges them into her branch instead of rebasing, resulting with dummy merge commits in her branch, which later end up in Snowdrift master branch, cluttering the git log
  3. The MR eventually gets merged and now master branch contains many tiny commits all related to the same change, and it makes it much harder to have a big-picture look at the commit history

My suggestions:

  1. Jane does git commit --amend unless her change is really a big thing that really deserves its own commit; when unsure, the default is to amend
  2. Jane uses git push -f to update her fork, replacing the original commit with the amended one
  3. Jane git pulls master into her local master, and uses git rebase on her work branch to bring the changes into it, rather than let git make a merge and create that unnecessary dummy merge commit
  4. Be forgiving to beginners, it’s okay if people don’t get this right, perhaps the person who merges the MR can first squash its commits into one before merging into master? Just to have a nice clean history and give a good example of what a free software project looks like

Thoughts?

https://git.snowdrift.coop/sd/snowdrift/issues/100

2

I agree with the principles. Here are the suggestions I would make.

  1. For smallish contributions from beginners, we default to squash-merge. I would prefer to see a log of MR updates rather than see a constantly-amended set of commits. It’s less for them to think about, too.

  2. For intermediates and larger patches, we encourage amending MRs to a nice set of commits.

  3. We should strongly discourage merging from master. That, imho, is something that neither novice nor expert should ever do. If people find it “necessary”, the project has failed them. But it probably wasn’t necessary, anyway.

  4. Rebasing onto a new master is also something that rarely needs doing. We can encourage use of git merge-base and an understanding of dotted name notations to inspect differences between diverging branches.

  5. Gitlab (and GitHub) have really bad default names for merge commits. We should consider taking my git-accept, make it work with Gitlab, and use it for merging patches.

I don’t think I want to do rebase merges, and I also don’t want to squash-merge everything. We should also encourage use of git log --first-parent. Consider my log aliases for a bewildering, but logical, set of log commands.

3

Okay, cool. I’m just glad to raise the issue and make sure you have your ways to handle it.

By the way, merging master is needed more often than you think. As the repo master you just put stuff in master easily. But some contributor Jane makes a MR and while she handles the review of it, new stuff gets into master, and suddenly there’s a conflict preventing automatic merge because there are conflicts. Jane wants to bring the changes in master into her local copy and handle the conflicts and changes, but accidentally she does some git pull which git by default handles by merging. She needs to be aware that the clean right way to handle it is to pull remote master into local master, and then rebase and handle the conflicts, which doesn’t introduce any new commits.

Obviously it’s not nice and not fun to have to handle with conflicts, newcomers can get scared of that easily. Just saying, depending on the rate at which new MRs get reviews and merges, conflicts can easily happen.

It’s okay to decide you prefer to handle the conflicts even when it’s more than 5 minutes of work and not make the beginner have to, but then it can be nice to mention in the docs, just to handle the case of a motivated beginner who tries to merge/rebase with good intentions to give you a MR ready for merge.