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

[Laurent Charignon]

> On Jun 5, 2015, at 4:04 PM, Matt Mackall <mpm at selenic.com> wrote:
> 
> On Fri, 2015-06-05 at 14:03 -0700, Laurent Charignon wrote:
>> # HG changeset patch
>> # User Laurent Charignon <lcharignon at 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.
>