Re: [PATCH 01/11] revert: Avoid calling die; return error instead

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

> [Subject: revert: Avoid calling die; return error instead]
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

Presumably this is because the sequencer is going to pick up after the
error and clean up a little (why doesn't the change description say
so?).  Will it be resuming after that or just performing a little
cleanup before the exit?

Might make sense, but:

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -265,23 +265,23 @@ static struct tree *empty_tree(void)
>  	return tree;
>  }
>  
> -static NORETURN void die_dirty_index(const char *me)
> +static int error_dirty_index(const char *me)
>  {
>  	if (read_cache_unmerged()) {
>  		die_resolve_conflict(me);

Won't that exit?  

>  	} else {

This "else" could be removed (decreasing the indent of the rest by
one tab stop) since the "if" case has already returned or exited.
Not the subject of this patch, just an idea for earlier or later. ;-)

[...]
> @@ -331,7 +331,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>  	    (write_cache(index_fd, active_cache, active_nr) ||
>  	     commit_locked_index(&index_lock)))
>  		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
> -		die(_("%s: Unable to write new index file"), me);
> +		return error(_("%s: Unable to write new index file"), me);
>  	rollback_lock_file(&index_lock);

What happens to index_lock in the error case?

[...]
> @@ -533,34 +533,39 @@ static void prepare_revs(struct rev_info *revs)
>  		revs->reverse = 1;
>  
>  	argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
> -	if (argc > 1)
> -		usage(*revert_or_cherry_pick_usage());
> +	if (argc > 1) {
> +		fprintf(stderr, "usage: %s", _(*revert_or_cherry_pick_usage()));
> +		return 129;
> +	}

Yuck.  Maybe the error can be returned to the caller somehow, but
that seems somehow ambitious given that setup_revisions has all sorts
of ways to die anyway.

So you are bending the assumptions of many existing git functions (in
a good way).

I can think of at least three ways to go:

 1) Come up with a convention to give more information about the nature
    of returned errors in the functions you are touching.  For
    example, make sure errno is valid after the relevant functions, or
    use multiple negative values to express the nature of the error.

    So a caller could do:

	if (prepare_revs(...)) {
		if (errno == EINVAL)
			usage(*revert_or_cherry_pick_usage());
		die("BUG: unexpected error from prepare_revs");
	}

    Or:

 2) Use set_die_routine or sigchain_push + atexit to declare what cleanup
    has to happen before exiting.  Keep using die().

 3) Provide a new facility to register cleanup handlers that will free
    resources and otherwise return to a consistent state before
    unwinding the stack.  This way, you'd still have to audit die()
    calls to look for missing cleanup handlers, but they could stay as
    die() rather than changing to "return error" and the worried
    caller could use

	set_die_routine(longjmp_to_here);

    to keep git alive.  I don't suggest doing this.  It is a pain to
    get right and not obviously cleaner than "return error", and some
    errors really are unrecoverable (rather than just being a symptom
    of programmers to lazy to write error recovery code :)).

Okay, I'm stopping here.  Hope that helps.

Thanks,
Jonathan