Re: [PATCH v2 1/7] replace: "libify" create_graft()

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 20, 2018, 00:25 UTC
Message-ID: <xmqqh8o6d8rg.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <1f1c64314088383570dd2232e08a88c8df3298f5.1524125760.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 20 quoted lines

> It is quite convenient to simply die() in builtins, in the absence of
> proper exception handling, because it allows us to just go belly up
> without having to implement error handling chains.
>
> Of course, for reusable library functions, this is a big no-no, so we
> (try to) restrict the usage of die() to one-shot commands, i.e. places
> where we know that the caller does not want to, say, give the user a
> useful high-level error message, i.e. a message that the function calling
> die() could not possibly know.
>
> The problem with this reasoning is that sooner or later, pretty much all
> useful functions will *need* to be libified: the more useful a function,
> the more likely it is to be called from a call chain where the outer
> function implements a high-level operation that needs to provide
> additional advice to the user in case of failure.
>
> This is the case here: the create_graft() function is useful enough to be
> called in a loop, say, in the upcoming patch to convert a graft file in
> one fell swoop. Therefore, this function must not be allowed to die(), nor
> any of its callees.

All of the first three paragraphs are already widely known to the project, and I do not think you need to verbosely repeat it, if brevity demands it.

One thing that the proposed log message for this step would be far more useful to say is that the current callers of create_graft() is fine with the behaviour change (i.e. prepared to act on an error return).

Also it may want to say why the other die() we see in this function in the pre-context is OK. I actually think it is not OK and should return error(not a valid object name) the same way (I suspect that this is mere omission/mistake, and you did not intend to leave it dying). As long as the caller is prepared to deal with an error return, that is, which was the previous point.

FWIW, I fully agree with the goal of making this (or other pieces that would be useful if they were reusable) reusable.

Show 20 quoted lines

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/replace.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 935647be6bd..43264f0998e 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -395,7 +395,9 @@ static int create_graft(int argc, const char **argv, int force)
>  
>  	if (get_oid(old_ref, &old_oid) < 0)
>  		die(_("Not a valid object name: '%s'"), old_ref);
> -	commit = lookup_commit_or_die(&old_oid, old_ref);
> +	commit = lookup_commit_reference(&old_oid);
> +	if (!commit)
> +		return error(_("could not parse %s"), old_ref);
>  
>  	buffer = get_commit_buffer(commit, &size);
>  	strbuf_add(&buf, buffer, size);