Christian Couder <christian.couder@xxxxxxxxx> writes:
> On Thu, Jun 5, 2014 at 11:49 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Christian Couder <chriscool@xxxxxxxxxxxxx> writes:
>>
>>> +static int create_graft(int argc, const char **argv, int force)
>>> +{
>>> + unsigned char old[20], new[20];
>>> + const char *old_ref = argv[0];
>>> + struct commit *commit;
>>> + struct strbuf buf = STRBUF_INIT;
>>> + struct strbuf new_parents = STRBUF_INIT;
>>> + const char *parent_start, *parent_end;
>>> + int i;
>>> +
>>> + if (get_sha1(old_ref, old) < 0)
>>> + die(_("Not a valid object name: '%s'"), old_ref);
>>> + commit = lookup_commit_or_die(old, old_ref);
>>
>> Not a problem with this patch, but the above sequence somehow makes
>> me wonder if lookup-commit-or-die is a good API for this sort of
>> thing. Wouldn't it be more natural if it took not "unsigned char
>> old[20]" but anything that would be understood by get_sha1()?
>>
>> It could be that this particular caller is peculiar and all the
>> existing callers are happy, though. I didn't "git grep" to spot
>> patterns in existing callers.
>
> lookup_commit_or_die() looked like a good API to me because I saw that
> it checked a lot of things and die in case of problems, which could
> make the patch shorter.
Perhaps I was vague. "find the commit object and die if that object
is not properly formed" is a good thing. I was referring to the
fact that you
- first had to do get-sha1 yourself to turn the extended sha1
you got from the user into a binary object name, and die with
your own error message if the user input was malformed.
- and then had to call lookup-commit-or-die to do the checking
and let it die.
It would have been simpler for *this* particular codepath if we had
another helper function you can use like so:
commit = lookup_commit_with_extended_sha1_or_die(old_ref);
which did the two-call sequence you handrolled above, and I was
wondering if other existing callers to lookup-commit-or-die wished
the same thing.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html