3 ms·

Step 3 is problematic. When you push change requests you also need to rebase i.e. put the fixing commits right after the affected commits. Otherwise, if you on

Step 3 is problematic. When you push change requests you also need to rebase i.e. put the fixing commits right after the affected commits. Otherwise, if you only rebase once the PR is complete, then you are likely to stumble upon conflicts due to the order of affected files. It's a waste of the developer's time either way, not to mention that rebasing i.e. changing history on an (upstream) PR hurts the review experience e.g. comments on lost commits.

seba_dos1 4y ago
> rebasing i.e. changing history on an (upstream) PR hurts the review experience e.g. comments on lost commits. GitLab handles it well.
CuriousCosmic 4y ago
I generally agree that it's not ideal. When I say rebasing, mostly I mean going back to clean up commit texts/titles and occassionally to squash/fixup commits that at the end of the dev process don't really make sense anymore and would probably only confuse people looking back at it (like squashing/fixuping down "do a, do b, revert a, do c, revert b, do d" into "do c, do d"). I make the distinction from step 2 because while I forgot to mention it (and in my head was considering it a part of step 3), I'll generally rebase right before I open the PR which cleans up most of the noise (such as "merge master even though no relevant files were touched" commits which don't add anything meaningful). Then the rebase at the end of the PR is only really squashing down a lot of the back and forth on the code review into meaningful chunks. On Github this does kinda mess with the code snippet/suggestion stuff but since it's right before merge, all of those should be closed as resolved/implemented/etc anyways. As for it being a waste of time or not, I'd disagree. I spend maybe 5 minutes at absolute most rebasing and often only about a minute (really just git rebase -i, mark the relevant commits e/s/f, let it do it's thing poking it where necessary, resigning w/ gpg, and pushing). If the rebase is throwing nasty conflicts while working on fairly linear single-dev feature branches, odds are you are rebasing wrong. And I find this saves future me a lot of time since things generally "just work" and I have all the important history still in git.
b3morales 4y ago
> changing history on an (upstream) PR hurts the review experience e.g. comments on lost commits. It does, but this is specifically a weakness of GitHub's UI, not of the process itself. It's handled better by other tools.

seba_dos1 4y ago

> rebasing i.e. changing history on an (upstream) PR hurts the review experience e.g. comments on lost commits. GitLab handles it well.

CuriousCosmic 4y ago

I generally agree that it's not ideal. When I say rebasing, mostly I mean going back to clean up commit texts/titles and occassionally to squash/fixup commits that at the end of the dev process don't really make sense anymore and would probably only confuse people looking back at it (like squashing/fixuping down "do a, do b, revert a, do c, revert b, do d" into "do c, do d"). I make the distinction from step 2 because while I forgot to mention it (and in my head was considering it a part of step 3), I'll generally rebase right before I open the PR which cleans up most of the noise (such as "merge master even though no relevant files were touched" commits which don't add anything meaningful). Then the rebase at the end of the PR is only really squashing down a lot of the back and forth on the code review into meaningful chunks. On Github this does kinda mess with the code snippet/suggestion stuff but since it's right before merge, all of those should be closed as resolved/implemented/etc anyways. As for it being a waste of time or not, I'd disagree. I spend maybe 5 minutes at absolute most rebasing and often only about a minute (really just git rebase -i, mark the relevant commits e/s/f, let it do it's thing poking it where necessary, resigning w/ gpg, and pushing). If the rebase is throwing nasty conflicts while working on fairly linear single-dev feature branches, odds are you are rebasing wrong. And I find this saves future me a lot of time since things generally "just work" and I have all the important history still in git.

b3morales 4y ago

> changing history on an (upstream) PR hurts the review experience e.g. comments on lost commits. It does, but this is specifically a weakness of GitHub's UI, not of the process itself. It's handled better by other tools.