docs: Clarify when to rebase a non-conflicting PR and how

[UdjinM6]

Issue being fixed or feature implemented

Issues with rebasing non-conflicting pull requests on top of the updated target branch:

1. It's impossible annoying to run gfd on each rebase to verify that it was indeed a clean rebase if you did not pull the original/previous version (it is possible actually, must use full commit hash)
2. Github GUI is pretty much useless if a target branch update was huge

Because of (1) and (2) if a rebase was done in the middle of your review you have to basically start your review from scratch which is super annoying and should be avoided. Rebasing a conflicting PR or rebasing on top of the same HEAD as before is ok.

cc @kittywhiskers @vijaydasmp @knst

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[ogabrielides]

utACK

[knst]

100% ACK, re-reviewing is quite frustrating.

btw, even if PR has conflicts and has been rebased, I use some scripts to re-review rebased PR easier without re-reading everything.
@UdjinM6 @PastaPastaPasta @ogabrielides

Let take for example this PR: #5492

@UdjinM6 UdjinM6 force-pushed the assumeutxo4 branch from ca729a0 to 2bca58f

Need to run several commands:

# pull changes for both version to local branch
# NOTE: you should use FULL hash, not shortened version to fetch changes.
#       The fetch like "git fetch kitty ca729a0e" will fail as expected
git fetch kitty ca729a0ea3de4fe3cf7cc7fb4a8bd1a3d462e5bd #old version of PR 
git fetch kitty 2bca58f82b0cd748e8795f68078b9ccd62f261ea #new version of PR 

# check with git log what is the first commit that belong to PR
git log --oneline ca729a0ea3de4fe3cf7cc7fb4a8bd1a3d462e5bd
# it's 32d726cb06
git log --oneline 2bca58f82b0cd748e8795f68078b9ccd62f261ea 
# it's f3dd48fac7

# build 2 diffs - for old PR and for new PR
# notice that there're used 2 extra filters to make diff less noisy. 
git diff -r 32d726cb06~ ca729a0ea3de4fe3cf7cc7fb4a8bd1a3d462e5bd | grep -v '^index' > 5514-v1.diff
git diff -r f3dd48fac7~ 2bca58f82b0cd748e8795f68078b9ccd62f261ea | grep -v '^index' > 5514-v2.diff

# compare diffs (use your own faviour diff tool)
vimdiff 5514-v{1,2}.diff
# diffs are exactly same! 

Unfortunately, PR #5514 doesn't have any difference between force-push, so, can't show how look PR that has "not only rebase" but extra changes. Anyway, empty diff - is an even better case when you are reviewer. :)

[kwvg]

I believed that the process for reviewing rebases would be alleviated as the process to diff between two sets of changes (approved and new) was trivial but since that doesn't seem to be the case, this policy makes sense.

One clarification, usually when suggestions are made, I tend to rebase the branch on top of develop (usually to stay ahead of changes that might cause a conflict, I've found that GitHub would report a conflict which didn't require any actual conflict resolution, just a rebase) and then implement/incorporate changes.

Would this policy prevent me from doing a rebase at all unless GitHub reports a conflict (restraining the above usage) or restrains only rebases not accompanied by any changes.

[UdjinM6]

@kittywhiskers Rebasing to solve merge conflicts is ok. Rebasing only to keep up with the target branch is not. So if implementing suggestions/fixes would introduce a conflict when pushed to github, rebasing such PR is ok too.

[kwvg]

So I take it that unless the suggestions implemented/incorporated cause a conflict, there shouldn't be a rebase on top of develop?

[knst]

there shouldn't be a rebase on top of develop?

Since review is requested - yes

[kwvg]

Alright, then. Thanks for the clarification. Concept ACK.

[PastaPastaPasta]

ACK