Patchwork record: exiting editor with non zero status should not stop recording session

login
register
mail settings
Submitter Laurent Charignon
Date June 5, 2015, 9:03 p.m.
Message ID <62cfbaf4ed6e086b3813.1433538197@lcharignon-mbp.local>
Download mbox | patch
Permalink /patch/9532/
State Superseded
Commit de1feed484cc126255743c574a4ee1baab59c21b
Delegated to: Matt Mackall
Headers show

Comments

Laurent Charignon - June 5, 2015, 9:03 p.m.
# HG changeset patch
# User Laurent Charignon <lcharignon@fb.com>
# Date 1433536278 25200
#      Fri Jun 05 13:31:18 2015 -0700
# Node ID 62cfbaf4ed6e086b38133a0f2fcd6e044f5f9392
# Parent  c39640d26a4c7546faef00b9e5c02af45ab8bf5e
record: exiting editor with non zero status should not stop recording session

Before this patch, exiting a hunk edition in record with a non-zero status lead
to the end of the recording session losing previously selected hunks to record.
This patch introduces the more desirable behavior of warning the user and
continuing the recording session.
Matt Mackall - June 5, 2015, 11:04 p.m.
On Fri, 2015-06-05 at 14:03 -0700, Laurent Charignon wrote:
> # HG changeset patch
> # User Laurent Charignon <lcharignon@fb.com>
> # Date 1433536278 25200
> #      Fri Jun 05 13:31:18 2015 -0700
> # Node ID 62cfbaf4ed6e086b38133a0f2fcd6e044f5f9392
> # Parent  c39640d26a4c7546faef00b9e5c02af45ab8bf5e
> record: exiting editor with non zero status should not stop recording session
> 
> Before this patch, exiting a hunk edition in record with a non-zero status lead
> to the end of the recording session losing previously selected hunks to record.
> This patch introduces the more desirable behavior of warning the user and
> continuing the recording session.
> 
> diff --git a/mercurial/patch.py b/mercurial/patch.py
> --- a/mercurial/patch.py
> +++ b/mercurial/patch.py
> @@ -1023,9 +1023,12 @@
>                      f.close()
>                      # Start the editor and wait for it to complete
>                      editor = ui.geteditor()
> -                    ui.system("%s \"%s\"" % (editor, patchfn),
> -                              environ={'HGUSER': ui.username()},
> -                              onerr=util.Abort, errprefix=_("edit failed"))
> +                    ret = ui.system("%s \"%s\"" % (editor, patchfn),
> +                                    environ={'HGUSER': ui.username()})
> +                    if ret != 0:
> +                        ui.write(_("editor exited with status"))
> +                        ui.write(" %d\n" % ret)

I think you're still confused about i18n strings

The bit inside the _() should be a string constant. This is good:

 ui.write(_("exit code %d\n") % ret)  <- variable outside
   ^^^^^^^^^^^^^^^^  <- constant

The string "exit code %d\n" gets extracted for translation. And then at
run-time _() looks up that precise string constant.

This is bad:

 ui.write(_("exit code " + ret + "\n"))
                           ^^^ variable inside

This is bad because _() gets called on "exit code 1", "exit code 2", ...
and we obviously don't want to translate every number. And who knows
what gets extracted for translation in the first place.

And this is just weird:

 ui.write(_("exit code "))
 ui.write(" %d\n" % ret)

We extract "exit code " for translation, but if the translator for some
reason wants to put the number first.. they can't. In fact, the
translator may not realize that a number comes next.

Also, if this is a warning, it should probably use ui.warn and indicate
that the hunk was skipped?

> +  $ printf 'exit 1' > editor.sh

Why printf instead of echo? You should probably just set the editor to
false.
Laurent Charignon - June 5, 2015, 11:13 p.m.
> On Jun 5, 2015, at 4:04 PM, Matt Mackall <mpm@selenic.com> wrote:
> 
> On Fri, 2015-06-05 at 14:03 -0700, Laurent Charignon wrote:
>> # HG changeset patch
>> # User Laurent Charignon <lcharignon@fb.com>
>> # Date 1433536278 25200
>> #      Fri Jun 05 13:31:18 2015 -0700
>> # Node ID 62cfbaf4ed6e086b38133a0f2fcd6e044f5f9392
>> # Parent  c39640d26a4c7546faef00b9e5c02af45ab8bf5e
>> record: exiting editor with non zero status should not stop recording session
>> 
>> Before this patch, exiting a hunk edition in record with a non-zero status lead
>> to the end of the recording session losing previously selected hunks to record.
>> This patch introduces the more desirable behavior of warning the user and
>> continuing the recording session.
>> 
>> diff --git a/mercurial/patch.py b/mercurial/patch.py
>> --- a/mercurial/patch.py
>> +++ b/mercurial/patch.py
>> @@ -1023,9 +1023,12 @@
>>                     f.close()
>>                     # Start the editor and wait for it to complete
>>                     editor = ui.geteditor()
>> -                    ui.system("%s \"%s\"" % (editor, patchfn),
>> -                              environ={'HGUSER': ui.username()},
>> -                              onerr=util.Abort, errprefix=_("edit failed"))
>> +                    ret = ui.system("%s \"%s\"" % (editor, patchfn),
>> +                                    environ={'HGUSER': ui.username()})
>> +                    if ret != 0:
>> +                        ui.write(_("editor exited with status"))
>> +                        ui.write(" %d\n" % ret)
> 
> I think you're still confused about i18n strings
> 
> The bit inside the _() should be a string constant. This is good:
> 
> ui.write(_("exit code %d\n") % ret)  <- variable outside
>   ^^^^^^^^^^^^^^^^  <- constant
> 
> The string "exit code %d\n" gets extracted for translation. And then at
> run-time _() looks up that precise string constant.
> 
> This is bad:
> 
> ui.write(_("exit code " + ret + "\n"))
>                           ^^^ variable inside
> 
> This is bad because _() gets called on "exit code 1", "exit code 2", ...
> and we obviously don't want to translate every number. And who knows
> what gets extracted for translation in the first place.
> 
> And this is just weird:
> 
> ui.write(_("exit code "))
> ui.write(" %d\n" % ret)
> 
> We extract "exit code " for translation, but if the translator for some
> reason wants to put the number first.. they can't. In fact, the
> translator may not realize that a number comes next.

Ok, I didn't realize there could be reordering of the number, I will do what you suggest for a V2.

> 
> Also, if this is a warning, it should probably use ui.warn and indicate
> that the hunk was skipped?
> 
>> +  $ printf 'exit 1' > editor.sh
> 
> Why printf instead of echo? You should probably just set the editor to
> false.

Ok

Thanks,

Laurent

> 
> -- 
> Mathematics is the supreme nostalgia of our time.
>

Patch

diff --git a/mercurial/patch.py b/mercurial/patch.py
--- a/mercurial/patch.py
+++ b/mercurial/patch.py
@@ -1023,9 +1023,12 @@ 
                     f.close()
                     # Start the editor and wait for it to complete
                     editor = ui.geteditor()
-                    ui.system("%s \"%s\"" % (editor, patchfn),
-                              environ={'HGUSER': ui.username()},
-                              onerr=util.Abort, errprefix=_("edit failed"))
+                    ret = ui.system("%s \"%s\"" % (editor, patchfn),
+                                    environ={'HGUSER': ui.username()})
+                    if ret != 0:
+                        ui.write(_("editor exited with status"))
+                        ui.write(" %d\n" % ret)
+                        continue
                     # Remove comment lines
                     patchfp = open(patchfn)
                     ncpatchfp = cStringIO.StringIO()
diff --git a/tests/test-commit-interactive.t b/tests/test-commit-interactive.t
--- a/tests/test-commit-interactive.t
+++ b/tests/test-commit-interactive.t
@@ -1291,6 +1291,34 @@ 
   abort: error parsing patch: unhandled transition: range -> range
   [255]
 
+Exiting editor with status 1, ignores the edit but does not stop the recording
+session
+
+  $ printf 'exit 1' > editor.sh
+  $ HGEDITOR="\"sh\" \"`pwd`/editor.sh\"" hg commit -i <<EOF
+  > y
+  > e
+  > n
+  > EOF
+  diff --git a/editedfile b/editedfile
+  1 hunks, 3 lines changed
+  examine changes to 'editedfile'? [Ynesfdaq?] y
+  
+  @@ -1,3 +1,3 @@
+  -This is the first line
+  -This change will be committed
+  -This is the third line
+  +This change will not be committed
+  +This is the second line
+  +This line has been added
+  record this change to 'editedfile'? [Ynesfdaq?] e
+  
+  editor exited with status 1
+  record this change to 'editedfile'? [Ynesfdaq?] n
+  
+  no changes to record
+
+
 random text in random positions is still an error
 
   $ cat > editor.sh << '__EOF__'