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

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

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