On 2015-07-06 11:41 PM, Thomas Martitz wrote:
Hello,
first of all, I think github should fix this problem, instead of
enforcing a suboptimal workflow on us.
I reported this problem to github, let's see if they respond.
I think I reported it before already. IIRC they said something to the
effect of "ya we know it's a problem, we'll add it to our list of things
to think about fixing" (paraphrasing).
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.
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?
IMO, it's easier to review a small list of commits rather than a mega
commit, even if there's a few little nuisance commits in it. In all
cases if you merge something you have to review it completely, it's just
easier, IMO, when you can kind of follow the comments interspersed with
the commits in the order they were added. Also, if something you tested
yesterday worked, you know that when the contributor added commits
today, it didn't invalidate everything you had previously
reviewed/tested, and have to now start all over.
Added to that, re-writing the history makes it a PITA for more than one
person to contribute changes to a PR at once. IMO, we should be
promoting that kind of thing, not making it harder.
So it boils down to lost comments are the main problem.
Well it's not the only one, but definitively the one that incited this
email about changing our (never really discussed IIRC) policy about
re-writing history mid-PR.
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.
IIRC it actually showed the real history of how the changes evolved and
I assume each commit was at least moderately tested. I'd have to look
closer to see if there were many noisy commits.
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.
You could still do that locally as you hack away on stuff, it's only the
stuff you pushed to your PR branch that people are pulling from where
you wouldn't re-write the history from underneath them (and Github).
And merging master into my PR when the PR
should eventually be merged into master is not acceptable for, therefore
I rebase.
Why? IMO there's no big deal merging master into your work branch if
it's long-lived enough to warrant it.
Continuously rewriting history is common in the patch distribution via
mailing lists so it's successfully performed elsewhere. It's really just
github being bad at maintaining comments and that should be fixed on
their side.
Mostly, but also it makes certain stuff easier for everyone. One of the
hassles of patches is because you have to keep creating branches to
apply them on or keep backing the changes out and applying new patches,
etc (basically the same thing as when someone force pushes to their PR
branch).
PS: Lost comments should still be in your mailbox as a last resort,
since github sends notifications for each.
The problem is it doesn't send you copies of your own messages :(
Cheers,
Matthew Brush
_______________________________________________
Devel mailing list
Devel@lists.geany.org
https://lists.geany.org/cgi-bin/mailman/listinfo/devel
