Subject: Re: [PATCH v7 03/15] bugreport: add tool to generate debugging info

From: Emily Shaffer <[email-masked]>
Date: Wed, 19 Feb 2020 15:06:59 -0800

On Wed, Feb 19, 2020 at 02:09:48PM -0800, Junio C Hamano wrote:
> Emily Shaffer <[email-masked]> writes:
> 
> > launch_specified_editor() has a handful of exit points, of three kinds:
> >  1. return error(something)
> >  2. raise(sigsomething)
> >  3. return 0
> >     a. when the editor process closed happily, but the user supplied
> >        NULL instead of a buffer. That is, the user didn't want the
> >        contents of the editor given back to them in a strbuf.
> >     b. when the editor process closed happily and the user's supplied
> >        buffer was filled with the file's contents with no issue.
> >
> > So I think we can check "yes" here.
> 
> Heh.  If we raised a signal to kill ourselves, then we won't be
> returning a value from launch_editor() anyway.  That case won't
> affect the "between returning negation or !!, which is more
> appropriate?" discussion, I think.
> 
> >>  - we MUST NOT care to differenciate different error codes returned
> >>    from launch_editor().  IOW, we must be fine to give the invoker
> >>    of the program only 0 (success) or 1 (unspecified failure).
> 
> I actually think this holds for the codepath.  A failure from
> start_command() returns error(), and finish_command() that waits for
> the spawned editor process to complete yields the exit status from
> the editor, but unless we re-raise the signal that killed the editor
> process to ourselves, we just turn any non-zero exit into "return
> error()", so it is safe to say launch_editor() can return either 0
> or -1 and nothing else.  Would we later want to tell callers of
> launch_editor() how/why the editor session failed by returning
> something else?  I do not offhand think of any---we do not even
> differenciate between failure to start (e.g. misspelt command name
> for the editor) and other failures WITH the return value and
> consider it sufficient to tell the user with different error
> message right now.
> 
> So in practice returning -launch_editor() and !!launch_editor()
> would not make any difference, I would think.

Then, let's do the least surprising thing. I'll switch it to !! for the
next reroll.