[Twisted-Python] non-merge commits to trunk & "review" keyword
Hello, GitHub apparently allows fast-forward merges to trunk. Here's an example of one: https://github.com/twisted/twisted/pull/730 This doesn't seem like a good thing. - The ticket is still open - There is no merge commit - There is no merge commit message - There are non-merge commits directly on trunk history (first parent) Anyone else have an opinion? Also, on the same PR, it seems like folks have trouble managing the two different ways to represent the review states: the "review" keyword in trac and the accepted/etc status on the GitHub PR. Maybe there shouldn't be two different ways to represent this? Jean-Paul
On Mar 6, 2017, at 6:03 PM, Jean-Paul Calderone <exarkun@twistedmatrix.com> wrote:
Hello,
GitHub apparently allows fast-forward merges to trunk. Here's an example of one:
https://github.com/twisted/twisted/pull/730 <https://github.com/twisted/twisted/pull/730>
This doesn't seem like a good thing. The ticket is still open There is no merge commit There is no merge commit message There are non-merge commits directly on trunk history (first parent) Anyone else have an opinion?
This is definitely bad, forbidden by existing policy, etc. In fact I remember adjusting the settings so that the 'merge' button would always create a merge commit; in fact, the configuration is still set that way. Craig, since you're the one who made this merge, can you explain what happened? Has github's 'merge' button stopped prompting for a commit message? Failing to wait for the removal of the 'review' keyword was obviously a mistake, but based on my previous experience with the 'merge' button this wouldn't even be a type of mistake that's _possible_ to make; understanding what you did to trigger it would therefore be very useful. I don't think this is bad enough to do any major VCS surgery to fix (similar mistakes exist in the history, and both commits to Twisted.Quotes and literally all of the imported SVN history are single-parent), but it is worth doing some work to avoid recurring.
Also, on the same PR, it seems like folks have trouble managing the two different ways to represent the review states: the "review" keyword in trac and the accepted/etc status on the GitHub PR. Maybe there shouldn't be two different ways to represent this?
Indeed there should not. I hope someone has the time to address this soon The only blocker is that I don't want to lose the notion of a "review queue". The random mess of works-in-progress, already-reviewed code awaiting feedback, and abandoned experiments that shows up on the default 'pull request' view is not a workable substitute for the relatively short and explicit "I have submitted code that wants a review but hasn't gotten one" list at https://twisted.reviews/ <https://twisted.reviews/>. However, some combination of Github's new-ish explicit review workflow, (possibly, if necessary) some kind of bot to perform the operations that are mysteriously forbidden to outside contributors, and a custom query could easily do the trick if someone can work it out and document it. -glyph
On Tue, 7 Mar 2017 at 06:38 Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
This is definitely bad, forbidden by existing policy, etc. In fact I remember adjusting the settings so that the 'merge' button would always create a merge commit; in fact, the configuration is still set that way.
Rebase merging was allowed when I checked earlier, so I disabled it (I guess I managed to do this before you looked at the configuration). I suspect that is what happened here, but note also that these settings only control what is possible by pressing the merge button on GitHub; you can construct commits locally however you like and push them, as long as you get green commit statuses for all the mandatory commit checks.
On Mar 6, 2017, at 9:02 PM, Tristan Seligmann <mithrandi@mithrandi.net> wrote:
On Tue, 7 Mar 2017 at 06:38 Glyph Lefkowitz <glyph@twistedmatrix.com <mailto:glyph@twistedmatrix.com>> wrote:
This is definitely bad, forbidden by existing policy, etc. In fact I remember adjusting the settings so that the 'merge' button would always create a merge commit; in fact, the configuration is still set that way.
Rebase merging was allowed when I checked earlier, so I disabled it (I guess I managed to do this before you looked at the configuration).
Perhaps I only did this for some other repos, then. Did I miss your announcement of this? Hint, hint? :)
I suspect that is what happened here, but note also that these settings only control what is possible by pressing the merge button on GitHub; you can construct commits locally however you like and push them, as long as you get green commit statuses for all the mandatory commit checks.
Is this true even for people with just repo:write? -glyph
On Tue, 7 Mar 2017 at 09:56 Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
On Mar 6, 2017, at 9:02 PM, Tristan Seligmann <mithrandi@mithrandi.net> wrote:
On Tue, 7 Mar 2017 at 06:38 Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
This is definitely bad, forbidden by existing policy, etc. In fact I remember adjusting the settings so that the 'merge' button would always create a merge commit; in fact, the configuration is still set that way.
Rebase merging was allowed when I checked earlier, so I disabled it (I guess I managed to do this before you looked at the configuration).
Perhaps I only did this for some other repos, then. Did I miss your announcement of this? Hint, hint? :)
By the time I got to my email client (which admittedly was several hours after I made the configuration change), your email had arrived, so my reply ended up being the "announcement" too. The last time I looked at these settings, rebasing was not allowed, so I guess somebody else turned it on (possibly by accident); the GitHub audit logs don't go far back enough for me to check who did it and when, but that means the change happened over several months ago.
I suspect that is what happened here, but note also that these settings only control what is possible by pressing the merge button on GitHub; you can construct commits locally however you like and push them, as long as you get green commit statuses for all the mandatory commit checks.
Is this true even for people with just repo:write?
I haven't actually tested this scenario, so I could be mistaken; inferring from other things it shouldn't matter what your permissions are (for a non-force push).
On Mon, Mar 6, 2017 at 8:37 PM, Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
Craig, since you're the one who made this merge, can you explain what happened? Has github's 'merge' button stopped prompting for a commit message? Failing to wait for the removal of the 'review' keyword was obviously a mistake, but based on my previous experience with the 'merge' button this wouldn't even be a type of mistake that's _possible_ to make; understanding what you did to trigger it would therefore be very useful.
Woops, sorry about that. I pressed the Merge button in git, but I guess I clicked on the wrong option after that. It is good that this is now disabled to prevent future mistakes like this. -- Craig
On Mar 7, 2017, at 10:40 AM, Craig Rodrigues <rodrigc@crodrigues.org> wrote:
On Mon, Mar 6, 2017 at 8:37 PM, Glyph Lefkowitz <glyph@twistedmatrix.com <mailto:glyph@twistedmatrix.com>> wrote:
Craig, since you're the one who made this merge, can you explain what happened? Has github's 'merge' button stopped prompting for a commit message? Failing to wait for the removal of the 'review' keyword was obviously a mistake, but based on my previous experience with the 'merge' button this wouldn't even be a type of mistake that's _possible_ to make; understanding what you did to trigger it would therefore be very useful.
Woops, sorry about that. I pressed the Merge button in git, but I guess I clicked on the wrong option after that. It is good that this is now disabled to prevent future mistakes like this.
OK, good to know; so this was a repo misconfiguration coupled with human error. Glad we have something in place to prevent it now :). Craig - that said, can you go back and fix up the relevant state (kill the 'review' keyword, close the ticket with a reference to the relevant commit, and so on)? -glyph
participants (4)
-
Craig Rodrigues -
Glyph Lefkowitz -
Jean-Paul Calderone -
Tristan Seligmann