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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 11, 2011, 20:26 UTC
Message-ID: <7vvcykv8j7.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1302448317-32387-2-git-send-email-artagnon@gmail.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).

Show 7 quoted lines

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

Show 15 quoted lines

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