doc: add note about force pushes to contrib guide

[dbkinder]

Let folks know that using amend and force pushes for adding review
changes is the recommend method for contributing to the project, but it
can cause some unexpected behavior from GitHub.

Signed-off-by: David B. Kinder david.b.kinder@intel.com

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@e7107ba). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #14444   +/-   ##
=========================================
  Coverage          ?   51.99%           
=========================================
  Files             ?      308           
  Lines             ?    45572           
  Branches          ?    10553           
=========================================
  Hits              ?    23695           
  Misses            ?    17069           
  Partials          ?     4808

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7107ba...bf56dc9. Read the comment docs.

[marc-hb]

see below

[marc-hb]

Suggesting this:

While amending commits and force pushing is a common review model outside github and the one recommended by Zephyr, it's not the main model supported by github and causes unexpected behavior such as not being able to use "View Changes" buttons except the last one (with GitHub complaining it can't find older commits) and not always being able to compare the latest reviewed version with the latest submitted version. When rewriting history github guarantees access to the latest version only.

[dbkinder]

I took your suggested edit but broke up the long sentence...

[marc-hb]

Long sentences? Me? Naaaaahh...

[marc-hb]

Some references for the record:

• https://guides.github.com/introduction/flow/, fast-forward (pun intended) to slide number 4: "Discuss and review your code":

... If someone comments that you forgot to do something or if there is a bug in the code, you can fix it in your branch and push up the change. GitHub will show your new commits and any additional feedback...

• https://help.github.com/en/articles/about-pull-requests

... you can push follow-up commits if you need to update your proposed changes...

• https://help.github.com/en/articles/using-git-rebase-on-the-command-line#pushing-rebased-code-to-github

... Force pushing has serious implications,...

• https://help.github.com/en/articles/about-pull-request-merges#squash-and-merge-your-pull-request-commits

Also related:

• Mark pull request as depending on another isaacs/github#959 "Mark pull request as depending on another"
• https://graysonkoonce.com/stacked-pull-requests-keeping-github-diffs-small/
• How Gerrit filters out rebase noise: https://bugs.chromium.org/p/gerrit/issues/detail?id=217#c42

[marc-hb]

I would just like @dbkinder to reply to my suggestions before this gets merged.

[dbkinder]

@marc-hb I did a few seconds before your last comment :)

[marc-hb]

Long sentences? Me? Naaaaahh...

[marc-hb]

Found some other interesting side effects of force pushes:

• You can't reply to old threads from the "Files changed" tab, you must use the "Conversation" tab instead. This means one email per comment, you can't batch comments in a single "review" email.
• Once you started a review to batch several comments (on the latest code), then you can't reply to old threads at all. Must complete the review first.

[marc-hb]

[If you're not interested in github and force pushes please unsubscribe from this PR now. It's merged]

A couple real-world examples to make the description of this force push pain less theoretical.

zephyrproject-rtos/ci-tools#88

• None of the "View Changes" buttons work except the last one.

• On 16 Sep 2019 force-pushed the branch 4 times, most recently from f6dbeef to f63b19c: no way to see the versions in between

• There's no simple list of all the force pushes. Showing changes since your last review is not convenient: the SHA1s must be manually found and copied in the interface. Then review comments cannot be entered in that comparison screen which seems disconnected from the merge request.

• No way to see changes to commit messages. The best course is not to bother about git commit messages (they're just fixup commits) until the review is complete and use the PR description instead. Force pushes aside, the web interface has simply no equivalent to the git log command,

• When some code is changed multiple times in the PR (in different commits), review comments on the earlier commits disappear from the "Changes" tab. The only way to reply is in the "Discussion" tab where they are marked as "Outdated". Replying there causes one separate email to be sent for every comment. Of course not a problem in this small review but now switch to larger reviews like doc: New developer getting started guide #19123 instead.

• Consider the author or someone else wants to go through all review comments and not miss any. As they're gone from the Changes tab, it's not possible to process them by file or directory any more. They can only be scanned chronologically in the Conversation tab.

• Speaking of PR doc: New developer getting started guide #19123, pick any "Outdated" source code and try to see it in context, I mean larger context than 3 lines of code around it.

• Last but not least, intermediate force-pushes seem garbage-collected after a while. Try opening force-pushed commits in Zephyr fails to build with CPU load measurement enabled #16604

Both examples above are Merge Requests with a single commit. Force pushes with multiple commits add an entire new level of review complexity, try PR #16064. In fact the fixups & squash review model is not compatible with reviewing series of multiple commits because the same commits can not have two completely different purposes https://github.com/git-series/git-series

github's preferred review model is "fixup commits & final squash" and no force pushes - by several miles. It's very clear not just from all the above issues caused by force pushes but also in features like "Show changes since your last review" which shows a list of... fixup commits or in how the web editor creates fixup commits - good luck combining these with force pushes.

While gitlab supports force pushes better, some of the issues above apply to it too. "fixups & final squash" is also gitlab's preferred review model

"Force pushes" is Gerrit's main (single?) code review model (so "forcing" is not even needed). It even colors rebase noise differently: https://bugs.chromium.org/p/gerrit/issues/detail?id=217#c42
This doesn't mean reviewing series in Gerrit is easy - but it's an order of magnitude easier.

UPDATE: github's most common review model is apparently "fixup and NO squash"
isaacs/github#959 (comment)

[marc-hb]

I don't have a definitive proof this new discovery is related to the lack of support for force-pushes, but I strongly suspect it is: the commits tab may list commits in a "random" order. Maybe it sticks to the order of the very first push, never updated later? Thanks @crazoes for the report.

UPDATE: it's not "random" but based on the commit date. Amend as a workaround.

git fetch upstream pull/2711/head
git log --oneline FETCH_HEAD

dd29d2add903 .travis.yml: Fix incorrect usage of -j option
4778e578a44e scripts: xtensa-build-all.sh: Derive $ROOT from $HOST
9c07d0be1cb3 scripts: xtensa-build-all.sh: Use switch case
bac5718839cc scripts: xtensa-build-all.sh: Ignore set -e the right way
971834550495 scripts: xtensa-build-all.sh: Exit if unknown platform specified
ca8b2ed62681 scripts: xtensa-build-all.sh: Use getopts
e6b37a06ba4c scripts: xtensa-build-all.sh: Make PLATFORMS a proper array

but:

[marc-hb]

There is now a Google Summer of Code project idea to add git range-diff and solve some of these problems in... Gitlab: https://gitlab.com/gitlab-org/gitlab/-/issues/24096
Give it a thumb up if you have a gitlab.com account. Competition never hurts.