[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