[Geany-Devel] [RFC]: No force push policy on Github PRs

[Colomban Wendling]

Le 07/07/2015 08:41, Thomas Martitz a écrit :
> [...]
> 
> Am 07.07.2015 um 02:13 schrieb Matthew Brush:
>> Hi All,
>>
>> As anyone trying to follow Pull Requests on Github has probably
>> noticed, when you force push to your PR branch, Github deletes various
>> comments related to the PR, depending on what got clobbered (it seems).
> 
> Not always. I haven't found a consistent pattern, but it seems worse
> when commits are removed.

Just as your GH emails says, it's simply when people commented on a
*commit* that got removed from the PR by the rebase, no real mystery here.

The problem is that when the diff is relatively large it's a *lot*
easier sometimes to comment on the smaller commit-only changes than on
the whole set, so then it becomes annoying.
As it's like that, I'm trying more and more to comment only on the
overall diff, but it's sometimes just not handy either.

>> This makes it extremely difficult to keep track of and finally merge
>> PRs because issues that may have been raised are gone and it leaves
>> holes in the comments which are a useful way to make sure any issues,
>> notes, ideas, etc. have been dealt with before merging.
>>
>> In addition to the dropped comments, it makes it harder to follow the
>> changes made since it clobbers the Git history too, so you have to
>> basically review the entire changset by looking at the whole diff of
>> all files affected by the PR at once.
> 
> Well, assuming an updated PR only changes stuff which has  been
> commented on before or are otherwise explicitely noted in a new comment,
> you do not have to review the entire diff again. And you have to review
> those parts of the diff you commented on again, other parts should be
> fine since they received no comment at the first review, right?

No, you just cannot know what changed and what remained the same [1].
Of course the creator of the PR probably means well, but even then
sometimes small (or less small) mistakes get introduced by rebases (you
know it just as well as I do :), so it has to be reviewed again as a
whole.  Well, the whole always have to be reviewed at some point if we
ever rebase, but if it can be once in the end it's better.

[1] well, sometimes I pull the changes locally in versioned temp
branches so I can at least diff the various versions.

> So it boils down to lost comments are the main problem.
> 
>>
>> Another reason to avoid is because it makes it harder to test a PR if
>> the repos history keeps getting munged, it breaks your previous pulls.
>>
>> I propose that we disallow force pushing, rebasing, squashing, etc. on
>> any PR until it is fully ready for final merging. We could probably
>> use a label or milestone or something to signify that a PR has been
>> fully reviewed and is ready for merging. At that point it _might_ make
>> sense to fudge history and get rid of some noisy "fixup" commits[0].
>>
> 
> The more fixup commits the less likely that the post-review cleanup is
> actually going to happen. The largeish linkage-cleanup branch was almost
> merged as is, and I'm sure bisection of in the middle of those commits
> is harder or even impossible.

IMO that's not true.  You just need to be disciplined when you do your
fix commits and split them.  In such situation I generally create one
fixup commit per commit I to fix, and also insert the SHA1 of the commit
to fix in the fixup message so it's easy to track later.

>> Thoughts, opinions?
> 
> I prefer rebasing and rewriting commits, because that makes my life
> easier too. I can handle my stuff better if it has a clean history, and
> it helps me in making design decisions because I try to logically
> separate stuff in the commits.

Sure, but IMO here we're mostly talking about fixes that aren't complete
rewrites, so design decisions should be mostly dealt with.  And if they
aren't and aren't trivial, it might warrant a separate PR.

> And merging master into my PR when the PR
> should eventually be merged into master is not acceptable for, therefore
> I rebase.

Well, ok we have an history with you of letting your stuff rot for long
(sorry :-() so getting up-to-date with master is important, but in most
PRs that doesn't matter.


Regards,
Colomban