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

[Jakub Narebski <jnareb@gmail.com>]

Junio C Hamano <gitster@pobox.com> writes:
> 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.

[taking the role of Advocatus Diaboli]

Well, this information (first three paragraphs) is maybe widely known to
the project, but do we have it explicitely documented somwehere, like
CodingGuidelines, or FAQ on some web site?

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

Right.

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

[taking the role of Advocatus Diaboli]

Maybe they are errors that caller cannot handle sanely in any way?
If I understand things correctly the problem is with parameter to the
--graft option...

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