[Twisted-Python] non-merge commits to trunk & "review" keyword

[Jean-Paul Calderone]

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

[Glyph Lefkowitz]

Post by Jean-Paul Calderone
Hello,
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.

Post by Jean-Paul Calderone
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

[Tristan Seligmann]

Post by Glyph Lefkowitz
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.

[Glyph Lefkowitz]

Post by Glyph Lefkowitz
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? :)

Post by Glyph Lefkowitz
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

[Tristan Seligmann]

Post by Glyph Lefkowitz
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.

Post by Glyph Lefkowitz
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).

[Craig Rodrigues]

Post by Glyph Lefkowitz
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

[Glyph Lefkowitz]

Post by Glyph Lefkowitz
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