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 https://github.com/theupdateframework/tuf/pull/804#issuecomment-440017361. 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.

[lukpueh]

GitHub and rebase test insights (see https://github.com/secure-systems-lab/lab-guidelines/pull/14#discussion_r235344697)):

• 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.