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

[Lex Trotman]

On 7 July 2015 at 23:07, Colomban Wendling <lists.ban at herbesfolles.org> wrote:
> Le 07/07/2015 02:13, Matthew Brush a écrit :
>> 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, […]
>
> Yeah, that annoyed the hell out of me more than a few times.

Agreed

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

Agreed

>
>> 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.
>
> Also true.  For that part they could provide a diff between the previous
> and current state, so at least we could see what changed.  But well,
> it's not (yet) the case.

But if a commit is changes to previous changes its better to look at
the final result, which is what the files tab is for, so agree that
there is no need to rebase.

>
>> 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.
>
> Well, true but you can also just create "versioned" local branches -- so
> you can even diff them.  I generally do that with largish stuff, like:
>
>         <user>/<pr>/v1
>         <user>/<pr>/v2
>         <user>/<pr>/v3
>
> etc.  Of course it requires manual intervention, but it's not very hard.

But why add work when St Linus designed the git workflow to be easy :)

>
>> I propose that we disallow force pushing, rebasing, squashing, etc.
>> on any PR until it is fully ready for final merging. […] ready for
>> merging. At that point it _might_ make sense to fudge history and get
>> rid of some noisy "fixup" commits[0].
>
> Agreed, this should be the default behavior.  We should at least try and
> see how life gets easier.

Agree it should be the general situation.  Nothing should be an
absolute (and after all in an open source project you can't force such
things anyway) but a strong request from the project maintainer/devs
would be good.

>
>> Thoughts, opinions?
>>
>> If it sounds like a good idea, I could probably try and update any
>> relevant documentation (HACKING comes to mind) to make a note of
>> this[1].
>>
>> Cheers, Matthew Brush
>>
>> [0]: I personally don't think it's a big deal leaving the history to
>> match real life, but I can see how "Fix some typo" type of commits
>> aren't very useful to keep around.
>
> Well, IMO it doesn't make sense to keep endless fixup commits in the
> final marge.  Not only it clobbers history, but it also makes it a lot
> harder to bisect.
>
> Of course, this applies to *fixups*, not incremental development.  If
> changes were made incrementally in the PR it probably makes sense (or
> may not, depending on the case) to keep the incremental history too.

Bisectable means that every commit must compile and preferably run, so
some commits that broke the build may need hiding just before merge.
Though for the Geany project bisecting is less common than some due to
Colombans relentless reviewing regime.

Cheers
Lex

>
> Regards,
> Colomban
> _______________________________________________
> Devel mailing list
> Devel at lists.geany.org
> https://lists.geany.org/cgi-bin/mailman/listinfo/devel