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).
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.
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].
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.
[1]: Although force pushing is generally considered bad-practice on public branches, so I doubt too many people would do it without being asked.
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.
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?
So it boils down to lost comments are the main problem.
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.
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. And merging master into my PR when the PR should eventually be merged into master is not acceptable for, therefore I rebase.
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.
PS: Lost comments should still be in your mailbox as a last resort, since github sends notifications for each.
Best regards.
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
Le 07/07/2015 08:41, Thomas Martitz a écrit :
[...]
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.
Just as your GH emails says, it's simply when people commented on a *commit* that got removed from the PR by the rebase, no real mystery here.
The problem is that when the diff is relatively large it's a *lot* easier sometimes to comment on the smaller commit-only changes than on the whole set, so then it becomes annoying. As it's like that, I'm trying more and more to comment only on the overall diff, but it's sometimes just not handy either.
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?
No, you just cannot know what changed and what remained the same [1]. Of course the creator of the PR probably means well, but even then sometimes small (or less small) mistakes get introduced by rebases (you know it just as well as I do :), so it has to be reviewed again as a whole. Well, the whole always have to be reviewed at some point if we ever rebase, but if it can be once in the end it's better.
[1] well, sometimes I pull the changes locally in versioned temp branches so I can at least diff the various versions.
So it boils down to lost comments are the main problem.
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.
IMO that's not true. You just need to be disciplined when you do your fix commits and split them. In such situation I generally create one fixup commit per commit I to fix, and also insert the SHA1 of the commit to fix in the fixup message so it's easy to track later.
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.
Sure, but IMO here we're mostly talking about fixes that aren't complete rewrites, so design decisions should be mostly dealt with. And if they aren't and aren't trivial, it might warrant a separate PR.
And merging master into my PR when the PR should eventually be merged into master is not acceptable for, therefore I rebase.
Well, ok we have an history with you of letting your stuff rot for long (sorry :-() so getting up-to-date with master is important, but in most PRs that doesn't matter.
Regards, Colomban
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.
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.
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.
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.
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.
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.
Regards, Colomban
On 7 July 2015 at 23:07, Colomban Wendling lists.ban@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>/v3etc. 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@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
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
On Wed, Jul 8, 2015 at 5:47 AM, Matthew Brush mbrush@codebrainz.ca wrote:
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].
The question is how to detect the "fully ready for final merging" moment. For my patches the workflow typically looks like
1. I submit a patch 2. Colomban reviews it 3. I repush the patch with fixes 4. Colomban merges it
I kind of implicitly assume that after (3) the patch will be ready for merging (and it typically is) so I do the force push but of course there may be further comments.
For bigger patch sets one should choose what seems to be most practical. For instance for
https://github.com/geany/geany/pull/488
where there were many commits and also review comments I decided to create a separate pull request containing the fixes
https://github.com/geany/geany/pull/505
to preserve the comments in the original pull request. In this case adding fix commits would make things too messy.
So personally I wouldn't carve any rules in stone and would decide case to case. For bigger patches with many review comments it's probably best to ask the reviewer which way he prefers to have the fixes committed.
Cheers,
Jiri
On 2015-07-08 01:03 PM, Jiří Techet wrote:
On Wed, Jul 8, 2015 at 5:47 AM, Matthew Brush <mbrush@codebrainz.ca [snip] So personally I wouldn't carve any rules in stone and would decide case to case. For bigger patches with many review comments it's probably best to ask the reviewer which way he prefers to have the fixes committed.
Yeah, of course. I didn't mean, despite how the subject might sound, that we could/should _never_ re-write a PR, just that we shouldn't do so casually/by default, and that if rebasing is desired, that it should be held off until the end when whoever's going to merge it is ready and asks the contributor to do it or does it themselves.
Cheers, Matthew Brush
Am 08.07.2015 um 05:47 schrieb Matthew Brush:
Everyone else? (Silence = "don't care")
I just don't care much to be honest. ;) However, I'd prefer as less as possible commits at the end if they are still atomic. Losing of outdated comments ... well... ok. Most of them can be dismissed as these are quiet often just working comments.
Cheers, Frank