Can't terminate arc diff workflow by quitting without saving in editor

Bug
#1

Switching from svn to git, a user noticed that they could no longer :q! in vim when editing the diff message to terminate the workflow as they were used to doing in svn. The expectation was that this would result in Usage Exception: Template not edited, but instead it just moved forward with submitting the diff. Maybe this is intentional because the template was edited by arc, but the user hadn’t edited anything, so from their perspective it was considered to be a bug.

I believe this is because using arc in a git repo populates $fields, leaving $template_is_default false and preventing the check for $template == $new_template later. I fixed it with this diff:

--- src/workflow/ArcanistDiffWorkflow.php.orig  2020-05-14 16:16:36 UTC
+++ src/workflow/ArcanistDiffWorkflow.php
@@ -1679,6 +1679,10 @@ EOTEXT
       if ($first && $this->getArgument('verbatim') && !$template_is_default) {
         $new_template = $template;
       } else {
+        // Now that the user is being presented the editor, assume the values
+        // presented are the default template. This allows quit without save
+        // to terminate the workflow.
+        $template_is_default = true;
         $new_template = $this->newInteractiveEditor($template)
           ->setName('new-commit')
           ->editInteractively();

Phabricator/Arcanist Version
arc version b76b9c4

#2

I may be missing some piece of nuance here, but it’s expected that users will sometimes be presented with the correct template message and quit without saving with the intent of continuing the workflow.

This most often happens when you manually use git commit with a local commit template to provide a completely valid commit message prior to invoking arc. The modern documentation doesn’t guide users in this direction, but this was common at Facebook and remains a supported workflow.

You can also get a valid local commit message in other ways, like using arc diff --create to resubmit a previously-abandoned change.

In this case, you’re presented with the correct commit message you already typed. If we didn’t continue the workflow, you’d be forced to make a trivial change to the message in order to continue, which would be unintuitive and confusing.

If you have a valid message but want to abort the workflow, you can “^C” after exiting the editor or intentionally break the message (e.g., by adding an invalid reviewer).

If you don’t have a valid message, the workflow should abort a few seconds later during validation anyway (e.g., “you haven’t specified any reviewers” or “no summary” or “you’re missing a test plan”).

So I think this is an intentional difference between the SVN and Git workflows: in SVN, there’s no way to have a local commit with a valid message already primed in your working copy when you run arc diff – but in Git, there is, and this workflow is supported.

Maybe a helpful question is: if I git commit my changes, type a complete, correct message (with “Summary”, “Test Plan”, and “Reviewers” – with or without the aid of a template), and then run arc diff and am presented with an editor that already has exactly the fields I want filled out properly, what action should arc require me to take to continue if not “quit without saving changes”?

#3

All excellent points. The intent of my diff was to make quit without saving terminate the workflow, but I also made quit with saving terminate the workflow when no changes were made. I would’ve realized the problem next time I tried to reuse a saved commit message after aborting a workflow :slight_smile:

I like your helpful question. If it helps, we could also directly ask the question that arose here: If I git commit my changes and run arc diff without supplying a template what action should I take to terminate the workflow? Just as having to make a trivial change to the message in order to continue is not intuitive, neither is having to enter intentionally incorrect data (although I’ll admit it’s slightly more intuitive than the former). The practice that a few of us have used is the ^C option, but for a change that doesn’t have any linting or unit tests, there might not be much time to sneak that in.

As a user myself, I think it would be most intuitive if quitting the editor without saving aborted the workflow and saving then quitting proceeded with submitting diff. In both cases it should be irrelevant if I modified the initial text or not. Does that make sense or is there a workflow that I’m not thinking of (again)?

#4

As a user myself, I think it would be most intuitive if quitting the editor without saving aborted the workflow and saving then quitting proceeded with submitting diff.

I don’t think we can cleanly distinguish between these cases: in general, editors exit 0 in both cases and the file on disk has no content changes.

The best test I can come up with is that we could test that the modification time of the file has changed, i.e. that the user has explicitly forced a write of the file after waiting at least one second. vi and nano do both provide ways to do this, but I think there is no guarantee that an arbitrary editor does (like mate, notepad.exe, etc – it seems reasonable for a modern GUI editor not to provide a “write this exact same file to disk to update the modification time” operation) or that the workflow is at all convenient or intuitive.

The practice that a few of us have used is the ^C option, but for a change that doesn’t have any linting or unit tests, there might not be much time to sneak that in.

This is what I use too, and I can’t remember ever missing a ^C, even in test repositories with no lint/unit tests and a local Phabricator install. Can you remember ever missing it?

If you or other users are missing it in practice, I think it would plausibly be reasonable to guarantee some minimum delay after the editor exits before any changes are written to durable storage. This isn’t great, but feels less unintuitive/weird to me than testing file modification times.

With modern prompting that allows users to save a response, a possible approach is to introduce an explicit prompt, and then anyone who is satisfied that they’re fast enough on the trigger with ^C can “y!” it to always continue automatically.

Another approach might be to explicitly add a command like “!abort” and help text like:

# To abort this workflow: delete the entire message, then save and exit;
# or type "!abort" at the beginning of any line, then save and exit; or
# intentionally make the message invalid, then save and exit; or exit,
# then press ^C quickly.

But this feels like a tremendous amount of complexity to address what I believe is a non-problem (I think everyone just presses ^C quickly and misses are very rare or nonexistent).

#5

This is what I use too, and I can’t remember ever missing a ^C, even in test repositories with no lint/unit tests and a local Phabricator install. Can you remember ever missing it?

I personally haven’t missed a ^C, but I mostly work on code that goes through several linters that take a while. I was theorizing there might be a problem for those that don’t have this delay, but if you have experience with this directly and it isn’t a problem, that’s good enough for me. I agree the complexity isn’t worth it at this point.

Thanks for the quick response and for saving me the trouble of my “fix” showing up later as a bug.