Hub: UX: quitting the editor without editing the default commit message template creates a pull request anyway

[mgedmin]

I was creating a pull request for Ansible by running hub pull-request. It downloaded Ansible's PULL_REQUEST_TEMPLATE.md very helpfully and spawned Vim for me. I remembered about issue #1377 and decide I didn't want hub to create a malformed PR for me, I'd rather create one by hand on the web. So I quit vim with :q! without editing the blank message template.

hub created a pull request anyway.

[mislav]

Thanks for reporting! This is indeed unwanted behavior.

[elliott-beach]

We can probably fix this by checking the modified time of the file before and after the editor executes.

I don't believe git does that for any of it's commit message editing, so this is a little different than how most ,or all, git commands work. For example, git commit -c HEAD still executes if you quit the editor.

[elliott-beach]

I implemented this at e-beach/hub@b76efdf, but this causes the pull-request feature suite to fail unpredictably, as the spoofed vim may modify files faster than the clock can detect. Potentially, a text editor could do the same, if the user is somehow automating or scripting the editor, so this may not be a robust method.

Otherwise, there is no way to detect the difference between :wq and :q!.

[mislav]

Otherwise, there is no way to detect the difference between :wq and :q!.

To avoid having to check timestamps, we could compare the _contents_ of the file before and after editing. We would have to compensate for the fact that the editor might have made whitespace changes, e.g. changed the line ending style or added an empty line at the end of the file.

[mgedmin]

Potentially, a text editor could do the same, if the user is somehow automating or scripting the editor, so this may not be a robust method.

Changing the file's mtime to a couple of seconds before now ought to fix this race.

[elliott-beach]

I like @mgedmin's suggestion because it will allow users to use the default pull request message if they so choose, which matches the current behavior, rather than forcing them to edit the message.

[mislav]

I like @mgedmin's suggestion because it will allow users to use the default pull request message if they so choose, which matches the current behavior, rather than forcing them to edit the message.

I also like this suggestion, if we have a reliable, cross-platform way to set a file's mtime into the past.

However, I thought that the whole point of this discussion was basically about _forcing_ users to edit the message, at least if a pull request template was used? I can't imagine any case in which sending the body of the pull request template would be desireable to the receiver of the PR.

[elliott-beach]

I would consider go's os.Chtimes a reliable, cross-platform method; there are no caveats to as what time can be set, whether in the past or future. We also know that setting the modified time into the past is possible without irregularities on macOS, Linux, and Windows).

Checking modified times is reliable if other programs are not modifying our own files, which is as safe to assume and is also an assumption when checking file contents.

[mlewand]

From other perspective, why would you prefer creation time check over actual content check? I think that relying on time might lead to cases where this will still be an issue. I just tested VSCode (Win) with a dummy file, and as I'm pressing save hotkey, it overwrites modification time 😢 Just curious.

[saveriomiroddi]

My 0.02 hexadecimal dollars.

I used to develop a program similar to Hub, and at the time I opted for content comparison - this type of mistake is something that definitely happens in real world (well, to me at least :smile:).

I think there is no ultimate solution, as edge cases can't be really controlled, however, I think content comparison has a few advantages:

1. it's visible to the user; in other words, whatever the user does in the editor, he knows exactly which modifications are performed, while the timestamp is opaque;
2. timestamp checking is single-dimensional, as the only test that can be performed is quantitative; content instead can have sophisticated checks, ranging from none to user-customizable in case a hypothetical plugin system is implemented in Hub (so a user with "heavy autoscripting" can also write the plugin to rollback aborted PRs :wink:).

Another advantage, related to point 2., is that it can be iteratively released. Starting with no sophisticated checks (newlines, whitespace...) is way better than not having this feature at all :smile:

Of course, this assumes that the following holds true:

I can't imagine any case in which sending the body of the pull request template would be desireable to the receiver of the PR.

I can't see any case where one would want to send the unmodified template, as well. Content checking is potentially extensible, anyway :wink:.

[bakert]

Is there a workaround here if you're in the editor and you realize what's about to happen? Empty the file completely and save or something?

[trapier]

Is there a workaround here if you're in the editor and you realize what's about to happen? Empty the file completely and save or something?

If I'm in vim I usually press escape to get into to normal mode then Ctrl-Z to suspend hub, then kill it with kill -9 %%.

[mgedmin]

vim has :cq, which means "exit with a non-zero status code". I've finally learned about it and started using it after a couple of accidents with git and also this very issue.

[bakert]

I think part of the difficulty with implementing anything at all here is that are two distinct scenarios where you don't change the file:

(1) The default message is exactly what you want.
(2) You realize you don't want to send a PR.

They have opposite desirable outcomes. So for as long as we're going to allow pre-populating of the PR message with something it's going to be annoying one way or the other. The way hub works (defaulting to latest commit message if only one commit in the PR) actually makes scenario (1) pretty common. And we know from the existence of this ticket that scenario (2) exists.

Is there any behavior that we can pick up on to decide which scenario were are in? Look for common placeholder text? Only assume no-change means "go" if we're in the case where we are defaulting to last commit's message? Default to not creating the PR if the PR message is only whitespace?

If anyone can think of a way to differentiate the two cases I'm game to have a go at content-comparison implementation. For now I'm sticking with :cg per mgedmin.