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

Matthew Brush mbrush at xxxxx
Wed Jul 8 03:47:16 UTC 2015


On 2015-07-07 06:07 AM, Colomban Wendling wrote:
> Le 07/07/2015 02:13, Matthew Brush a écrit :
>> [snip]
>
>> 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.
>

It kind of makes sense that `git push +fackyou` does what it says 
(regardless if you prefer the -f/--fack-you syntax or not), it still 
says "Dear Git, break this sh1t".

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

It's a good workaround to be sure, but still a workaround.

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

Thomas, since you're one of the most frequent contributors, are you OK 
if we try this? I think you often rebase when you think it's expected 
and to just get the damn changes finally merged ... so if we lower the 
expectation, will it hamper your Geany-Fu a lot?

>> Thoughts, opinions?
>>

Everyone else? (Silence = "don't care")

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

Yeah, I can understand this point. The main advantage I see is that when 
a PR is merged, it has the same set of commits that were proven (via our 
ad-hoc testing and review procedures) to be OK, as opposed to <insert 
undefined stuff that happened during fixing of merge/rebase conflicts 
and adjustments of history>.

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

I think we all agree here, just a bit differing on the spectrum of 
"incremental" vs "oh shyte, oops!" commits (both being expected in a PR, 
naturally).

Cheers,
Matthew Brush



More information about the Devel mailing list