Add guidelines for keeping a clean git history

[lukpueh]

This is a first stab at creating a guideline document for keeping a clean git history in our lab code projects. At the moment the document purely reflects my personal opinion, albeit based on my experience in this lab. I'd love to hear other thoughts and ideas to make this better.

And here are some additional thoughts that I'd like to throw into the discussion:

• Can we make this more concise?
• Should we add command line examples?
• Can we better leverage existing guidelines and howtos?

[aaaaalbert]

Love it!

From my experience: git add -p for feature separation can be difficult because it's easy to overlook stuff, and you'd have to test the committed parts separately. So technically things are separated, but they'll not always work as intended.

Request: Let's mention squashing too. (On a similar note, I often do two branches where one is for feature development and the other is for cleaning up the commit history. This is a little more fine-grained than squash-merging a PR via the GitHub PR interface as you still get to choose and combine sub-sub-commits into sub-commits.)

[lukpueh]

Thanks for the love, @aaaaalbert! You are right that patch adding might not be the right tool for everyone, but I think it deserves a mention (I use it a lot).

Regarding squashing, I suggest that I expand the note about interactive rebasing, briefly mentioning all the possibilities, i.e. rewording commit messages, reordering commits, squashing, ... How does that sound?

Personally, I don't like squash merging a PR. I'm in favor of telling PR submitters to keep a clean branch history, so that it is also useful for the overall git history of the project.

Btw. for now the target audience of this document are contributors, who submit PRs. Should we also add a note for the maintainers, who merge PRs? Along the lines...

• make sure your contributors follow this guideline
• merge with --no-ff (default when using GitHub UI)

[lukpueh]

I'd love to hear some more opinions. @awwad, @SantiagoTorres does this align with your workflows? @michizhou, do you think this is clear enough for new students?

[aaaaalbert]

Didn't mean to flame add -p, it's a great tool and should definitely be mentioned.

Squashing and target audience: You're right, some things should go into a maintainer-flavored doc, not this.

[lukpueh]

@awwad provides some more related info in theupdateframework/python-tuf#804 (comment). Is it worth adding some of it to this document?

[alyptik]

@lukpueh said:

@awwad provides some more related info in theupdateframework/tuf#804 (comment). Is it worth adding some of it to this document?

In my opinion, it'd be useful to do so. Stuff like git rebase is a bit weird to get the hang of if you haven't used it much, and @awwad explained the process quite well. Once condensed a bit it'll fit in perfectly.

[awwad]

Love it. See suggested addition and comment on rebasing PRs.

[awwad]

While it's true that you should hesitate before rebasing once the PR is up, there are still a lot of cases where it's still better to rebase than not:

• history cleanup / squashing
• removing unneeded accidental additions
• even IMHO conflict resolution: I think that conflict resolution is better via rebasing than adding additional commits that won't make as much sense in the history once they're merged. (BTW, would be interested if people disagree. I do a lot of rebasing in PRs.)

[lukpueh]

I totally agree with you on above items. There are two reasons why I think rebasing on PR's that are actively being reviewed should be avoided:

• AFAIK review code comments are bound to commit ids, hence break on rebase. I do have the feeling that GitHub made some enhancements in that regard, but I haven't checked.
• If I write a comment on someone's Added foo commit, that says "please modify bar", then it's easier for me to re-review, if the person pushes a Modified bar in foo commit, instead of e.g. silently modifying bar and force pushing it to Added foo.

However, once all the "please modify bar" comments are addressed and green-lighted by the reviewer, I think the contributor can/should squash the review-addressing commits, and use rebase to fix conflicts or clean up the history in general.

[aaaaalbert]

AFAIK review code comments are bound to commit ids, hence break on rebase.

Fancy a quick test?

[lukpueh]

Will do

[lukpueh]

As per the insights from my quick and fancy (ah ... dirty) GitHub/rebase test below, I would conclude that GitHub does a pretty good job in tracking rebases. So you can probably disregard my first concern above. However, the second one is still relevant.

[SantiagoTorres]

I personally advice that in general we should keep git usage guidelines disconnected from github guidelines.

[awwad]

It sounds like we're roughly in the same place, then. I agree that generally a review-motivated/requested code fix should be provided in an additional commit.

[lukpueh]

GitHub and rebase test

Testing a review comment on a line that will change and links to lines that change and lines that don't change after a rebase and force push.

1. https://github.com/secure-systems-lab/lab-guidelines/pull/14/files#diff-b4039e218e46891be5b013b445f12c47R37
2. https://github.com/secure-systems-lab/lab-guidelines/blame/84b06d128cff13ecadc017793d7f41bb8001dcf8/git-history.md#L37
3. https://github.com/secure-systems-lab/lab-guidelines/blob/84b06d128cff13ecadc017793d7f41bb8001dcf8/git-history.md
4. https://github.com/secure-systems-lab/lab-guidelines/pull/14/files#diff-b4039e218e46891be5b013b445f12c47R36
5. https://github.com/secure-systems-lab/lab-guidelines/blame/84b06d128cff13ecadc017793d7f41bb8001dcf8/git-history.md#L36

[lukpueh]

GitHub and rebase test

Testing a review comment on a line that will not change and links to lines that change and lines that don't change after a rebase and force push.

1. https://github.com/secure-systems-lab/lab-guidelines/pull/14/files#diff-b4039e218e46891be5b013b445f12c47R37
2. https://github.com/secure-systems-lab/lab-guidelines/blame/84b06d128cff13ecadc017793d7f41bb8001dcf8/git-history.md#L37
3. https://github.com/secure-systems-lab/lab-guidelines/blob/84b06d128cff13ecadc017793d7f41bb8001dcf8/git-history.md
4. https://github.com/secure-systems-lab/lab-guidelines/pull/14/files#diff-b4039e218e46891be5b013b445f12c47R36
5. https://github.com/secure-systems-lab/lab-guidelines/blame/84b06d128cff13ecadc017793d7f41bb8001dcf8/git-history.md#L36

[lukpueh]

GitHub and rebase test insights (see #14 (comment))):

• GitHub show that there was a force push, see: lukpueh force-pushed the lukpueh:lab-git-history branch from 84b06d1 to d3e6463 a minute ago
• On the "Conversation" pane, the review comment on the changed line is labeled outdated and shows the old diff
• On the "Files changed" pane, that comment is gone.
• The review comment on the unchanged line stays as it was before in both panes
• GitHub keeps the old commit and it remains directly accessible (84b06d1)
• Links to the old commit that are not related to the Pull Request are still accessible:
  https://github.com/secure-systems-lab/lab-guidelines/blob/84b06d128cff13ecadc017793d7f41bb8001dcf8/git-history.md
  https://github.com/secure-systems-lab/lab-guidelines/blame/84b06d128cff13ecadc017793d7f41bb8001dcf8/git-history.md#L36
• Links to PR line diffs (changed or not changed) that don't contain a commit id, but a (GitHub?) diff id show the updated diff:
  https://github.com/secure-systems-lab/lab-guidelines/pull/14/files#diff-b4039e218e46891be5b013b445f12c47R37
  https://github.com/secure-systems-lab/lab-guidelines/pull/14/files#diff-b4039e218e46891be5b013b445f12c47R36
• Links to PR line diffs (changed or not changed) that contain the commit id lead to a page that says "Commit range not found". Click on "View changes" or the filename on top of my test comments:
  https://github.com/secure-systems-lab/lab-guidelines/pull/14/files/84b06d128cff13ecadc017793d7f41bb8001dcf8
  https://github.com/secure-systems-lab/lab-guidelines/pull/14/files/84b06d128cff13ecadc017793d7f41bb8001dcf8#diff-b4039e218e46891be5b013b445f12c47

[alyptik]

Just something that I realized recently when reviewing some PRs for unrelated personal repositories is that it's a lot easier if we have PR authors avoid force-pushing, rebasing, and other destructive git history operations until it comes time to merge. Otherwise, the squashed commit history makes it hard for the reviewers to tell if the PR author actually addressed their review comments as well as to figure out any other changes they might have made. I think it would be helpful to include this guideline somewhere in the guide.

[alyptik]

I should clarify that in my previous comment I am referring to self-rebasing such as git rebase -i @~5 to squash/reorder/drop commits. Rebasing on the parent branch because of new commits shouldn't cause any issues.

[lukpueh]

I made some minor modifications in the "rebasing on an open PR" paragraph, to integrate our recent discussions, and will merge now.