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

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

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

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

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