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

[Junio C Hamano <gitster@pobox.com>]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

You would need to write a lot more than that to justify why this is a good
change and does not regress the existing codepaths.  The above Subject:
implies as if all you did was to replace "die()" with "return error()",
but I am sure that you would also have audited all the existing callers of
the affected codepaths and either they already handled an error return
correctly by dying or exiting with non-zero status, or you adjusted them
to expect an error return and exit with 129 in this patch.

Also we know from the context of this post that you are planning to add
new callsites to some of the functions that are converted to give an error
return with this patch, but it is nevertheless a good idea to briefly
mention that (just "the codepath to implement new nitfol feature will be
making calls to xyzzy and frotz and it does not want these to die; rather
it wants to handle error cases itself" would do).

> @@ -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);

Do the callers rollback the lockfile in their error return codepaths now?
Should they?  If not why not?

One acceptable answer is "the only thing the current callers do in their
error codepaths is to exit(129), and that will roll it back for us", but
then that might mean this patch made the API more error prone to use when
the next callsite you add wants to do more than just exitting.

> @@ -397,18 +397,18 @@ static int do_pick_commit(void)
>  		 * to work on.
>  		 */
>  		if (write_cache_as_tree(head, 0, NULL))
> -			die (_("Your index file is unmerged."));
> +			return error(_("Your index file is unmerged."));
>  	} else {
>  		if (get_sha1("HEAD", head))
> -			die (_("You do not have a valid HEAD"));
> +			return error(_("You do not have a valid HEAD"));
>  		if (index_differs_from("HEAD", 0))
> -			die_dirty_index(me);
> +			return error_dirty_index(me);
>  	}
>  	discard_cache();

Likewise for this "discard-cache".  Should it be the responsibility to the
caller to discard the in-core cache when they handle an error return and
possibly take an alternative action, or should this function be the one to
do so for them?