Re: [PATCH v3 1/4] replace: add --graft option

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 6, 2014, 16:59 UTC
Message-ID: <xmqqegz2q8ih.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAP8UFD3k98_6Uh+noJgt4xqEooATVMAEf58FFkuy6rHBnP10zw@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 29 quoted lines

> On Thu, Jun 5, 2014 at 11:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Christian Couder <chriscool@tuxfamily.org> 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.