Re: [PATCH 05/17] revert: Propogate errors upwards from do_pick_commit

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Ramkumar Ramachandra wrote:

> revert: Propogate errors upwards from do_pick_commit
>
> Currently, revert_or_cherry_pick can fail in two ways.  If it
> encounters a conflict, it returns a positive number indicating the
> intended exit status for the git wrapper to pass on; for all other
> errors, it calls die().  The latter behavior is inconsiderate towards
> callers, as it denies them the opportunity to do some post-processing
> or cleanup before exiting.  For instance, later in the series, callers
> will want to save some data about the current operation before
> exiting.

Thanks for explaining.  Why can't callers use set_die_routine() or
atexit()?  Is the last sentence true?

> Change this by replacing some of the calls to "die" with calls to
> "error", so that revert_or_cherry_pick can return negative values too.
> While postive return values indicate conflicts as before, negative
> ones indicate other errors.  This return status is propogated updwards
> from do_pick_commit, to be finally handled in cmd_cherry_pick and
> cmd_revert.
>
> In the same spirit, also introduce a new function error_dirty_index,
> based on die_dirty_index, which prints some hints and returns an error
> to its caller do_pick_commit.
>
> While the full benefits of this patch will only be seen once all the
> "die" calls are replaced with calls to "error", its immediate impact
> is to change some of the "fatal:" messages to "error:" messages and
> print a new "fatal: cherry-pick failed" message when the operation
> fails.

Starting to get long.  It's not immediately obvious to me why
error_dirty_index is a detail that needs to be singled out.  Maybe
these three paragraphs could be summarized by saying:

 After this patch, revert_or_cherry_pick will still return a positive
 return value to indicate an exit status for conflicts as before,
 while for other (fatal) errors it will print an error message and
 return -1 instead of die()-ing.  The cmd_revert and cmd_cherry_pick
 are adjusted to handle the fatal errors by die()-ing themselves.

 Thus for now, the only user-visible impact would be to change some
 "fatal:" messages to say "error:" and to add a new "fatal:
 cherry-pick failed" message at the end when the operation fails.

 Since no callers take advantage of the ability to recover from
 errors yet, it is possible and even likely that these functions do
 not completely clean up after themselves on (fatal) error but leave
 some state to be cleared away by exit().  Callers beware!

That last paragraph summarizes my worry.  Since this API change does
not seem to be used by the other patches, I would prefer not to
leave such a trap for unwary new callers, at least until the intended
legitimate use is a little clearer.

[...]
>> write_cache_as_tree() locks the index and does not always commit or
>> roll it back except on success.  Current callers aren't likely to try
>> to lock the index again (since they just die()), but presumably the
>> goal of returning error() here is to allow for callers that want to
>> stay alive and do something more.  How should they recover (i.e., what
>> is the intended API)?
>
> Hm, there was supposed to be a discard_cache() before this error as I
> recall -- fixed now.  Thanks for catching.

discard_index() does not unlock the index.

I had wondered what the discard_cache() stuff was about before; thanks
for explaining.

>> Similar questions probably apply to calls to other APIs that return -1
>> to mean "I failed; please print an appropriate message about that and
>> exit".  I haven't checked carefully, since the answer to this example
>> could help in knowing what to look for in the others.
>
> I think the others are fairly clear actually.

Roughly speaking, there are two kinds of functions that return error()
instead of die()-ing in the git codebase:

 1. Those that recover from their errors, leaving the process in a
    sane state that allows the caller (e.g., a long-lived interactive
    porcelain) to go on to do other things

 2. Those that do not have enough information to give a useful error
    message themselves, so delegate to the caller the responsibility
    to die() with an error message about where in the program the
    failure occured.

The commit message suggests that you are introducing a new category:

 3. Those that could die() without trouble, _except_ that some callers
    want to do cleanup of private state on exit and atexit() is not
    working for them for some reason

But from this description of it you can see that the category seems
questionable and unlikely to be necessary to me.

How do callers know which of the three categories a function being
called falls into?  That question is relevant because the caller must
respond accordingly:

 a. In case (1), the caller can go on with its work.
 b. In case (2), the caller writes a good error message and die()s ASAP.
 c. In the new case (3), the caller needs to exit() or die() ASAP,
    but there is no message that especially needs to be written.

I had thought the idea was to put pick_commits() in case (1) (usable
by long-lived porcelain like cgit that don't want to exit).  To
accomplish that, when you call a function of type (2), you would need
to audit its implementation to see what changes it needs to move to
type (1), if any.  (FWIW, I am not convinced you've tried to do this,
hence the comment above about it.)

It clearly is not part of case (2).  All the useful context has
already been written to stderr when pick_commits() returns.

If it is case (3), the reader is going to wonder why you didn't just
use atexit() or set_die_handler() and why it is worth the
complication.

Does that make sense?
Jonathan