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
_______________________________________________
Devel mailing list
Devel@lists.geany.org
https://lists.geany.org/cgi-bin/mailman/listinfo/devel
