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

[Ramkumar Ramachandra <artagnon@gmail.com>]

Hi,

Jonathan Nieder writes:
> 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?

Definitely a misguided commit message.
New commit message.

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 recover from errors and
do other things.

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 some other 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.

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 "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.

Very legitimate.  Thanks for explaining.  However, I don't think I've
introduced any new traps for callers except for the
write_cache_as_tree() thing which is fixed now -- so, I've omitted
your warning in the commit message because I'm not entirely sure it's
true.  Yes, we don't have callers that take advantage of this yet, but
the idea of the patch is to work in the right direction -- we have to
convert all the "die" calls to "error" calls and make a nice public
API out of it sometime in the future.

Also, should we worry about it so much at this stage? We haven't set
up a public API

>>> 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.

Ugh, sorry.  I realize now that you were talking about the lockfile,
which will only be cleaned up by exit().  I thought about this for a
while and decided to classify this as an unrecoverable error for the
moment -- we have no way to clean up the lockfile at this stage, so
might as well die() instead of setting a trap for callers.

>>> 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

Thanks for the excellent categorization -- this change falls mostly in
category (1), and a little bit in (2) too.  When we work towards a
general sequencer, do_pick_commit has no idea about the name of git
wrapper that invoked it ("git cherry-pick" or "git revert" in this
case).  Thinking about these things helped me write a better commit
message.

Thanks.

-- Ram