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

login
register
mail settings
Submitter Laurent Charignon
Date June 5, 2015, 11:20 p.m.
Message ID <08bb462f727b88812a46.1433546428@lcharignon-mbp.local>
Download mbox | patch
Permalink /patch/9536/
State Accepted
Headers show

Comments

Laurent Charignon - June 5, 2015, 11:20 p.m.
# HG changeset patch
# User Laurent Charignon <lcharignon@fb.com>
# Date 1433536278 25200
#      Fri Jun 05 13:31:18 2015 -0700
# Node ID 08bb462f727b88812a46570b80c3a467c1ea23e9
# 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.
Augie Fackler - June 8, 2015, 2:45 p.m.
On Fri, Jun 05, 2015 at 04:20:28PM -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 08bb462f727b88812a46570b80c3a467c1ea23e9
> # Parent  c39640d26a4c7546faef00b9e5c02af45ab8bf5e
> record: exiting editor with non zero status should not stop recording session

queued, thanks

I've got one note below that might be worth doing in a followup -
maybe ask other facebook folks how they feel about it?

>
> 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,11 @@
>                      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.warn(_("editor exited with exit code %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,33 @@
>    abort: error parsing patch: unhandled transition: range -> range
>    [255]
>
> +Exiting editor with status 1, ignores the edit but does not stop the recording
> +session
> +
> +  $ HGEDITOR=false 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 exit code 1
> +  record this change to 'editedfile'? [Ynesfdaq?] n
> +
> +  no changes to record

it occurs to me that the editor might leave a bunch of spew on the
terminal and we should redisplay the diff?

> +
> +
>  random text in random positions is still an error
>
>    $ cat > editor.sh << '__EOF__'
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Laurent Charignon - June 12, 2015, 8:38 p.m.
> On Jun 8, 2015, at 7:45 AM, Augie Fackler <raf@durin42.com> wrote:
> 
> On Fri, Jun 05, 2015 at 04:20:28PM -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 08bb462f727b88812a46570b80c3a467c1ea23e9
>> # Parent  c39640d26a4c7546faef00b9e5c02af45ab8bf5e
>> record: exiting editor with non zero status should not stop recording session
> 
> queued, thanks
> 
> I've got one note below that might be worth doing in a followup -
> maybe ask other facebook folks how they feel about it?
> 
>> 
>> 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,11 @@
>>                     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.warn(_("editor exited with exit code %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,33 @@
>>   abort: error parsing patch: unhandled transition: range -> range
>>   [255]
>> 
>> +Exiting editor with status 1, ignores the edit but does not stop the recording
>> +session
>> +
>> +  $ HGEDITOR=false 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 exit code 1
>> +  record this change to 'editedfile'? [Ynesfdaq?] n
>> +
>> +  no changes to record
> 
> it occurs to me that the editor might leave a bunch of spew on the
> terminal and we should redisplay the diff?

That's a great idea, I will put that in my queue
> 
>> +
>> +
>> random text in random positions is still an error
>> 
>>   $ cat > editor.sh << '__EOF__'
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> https://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/patch.py b/mercurial/patch.py
--- a/mercurial/patch.py
+++ b/mercurial/patch.py
@@ -1023,9 +1023,11 @@ 
                     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.warn(_("editor exited with exit code %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,33 @@ 
   abort: error parsing patch: unhandled transition: range -> range
   [255]
 
+Exiting editor with status 1, ignores the edit but does not stop the recording
+session
+
+  $ HGEDITOR=false 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 exit code 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__'