From: Junio C Hamano <gitster@pobox•com>
To: Christian Couder <christian.couder@gmail•com>
Cc: Christian Couder <chriscool@tuxfamily•org>,
git <git@vger•kernel.org>, Jeff King <peff@peff•net>,
Jakub Narebski <jnareb@gmail•com>,
Eric Sunshine <sunshine@sunshineco•com>
Subject: Re: [PATCH v3 1/4] replace: add --graft option
Date: Fri, 06 Jun 2014 09:59:18 -0700 [thread overview]
Message-ID: <xmqqegz2q8ih.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <CAP8UFD3k98_6Uh+noJgt4xqEooATVMAEf58FFkuy6rHBnP10zw@mail.gmail.com> (Christian Couder's message of "Fri, 6 Jun 2014 17:29:01 +0200")
Christian Couder <christian.couder@gmail•com> writes:
> 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.
next prev parent reply other threads:[~2014-06-06 16:59 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-04 19:43 [PATCH v3 0/4] Add --graft option to git replace Christian Couder
2014-06-04 19:43 ` [PATCH v3 1/4] replace: add --graft option Christian Couder
2014-06-05 21:49 ` Junio C Hamano
2014-06-06 15:29 ` Christian Couder
2014-06-06 15:44 ` Christian Couder
2014-06-08 6:49 ` Christian Couder
2014-06-08 11:23 ` Jeff King
2014-06-08 12:04 ` Jeff King
2014-06-08 12:09 ` Jeff King
2014-06-09 16:43 ` Junio C Hamano
[not found] ` <CAPc5daWBycdmKBZXGhhy4_649p_JFfGf7RQbqa08XA1hL9mFTg@mail.gmail.com>
2014-06-29 6:34 ` Christian Couder
2014-06-30 6:37 ` Junio C Hamano
2014-06-30 10:52 ` Christian Couder
2014-06-06 16:59 ` Junio C Hamano [this message]
2014-06-04 19:43 ` [PATCH v3 2/4] replace: add test for --graft Christian Couder
2014-06-04 19:43 ` [PATCH v3 3/4] Documentation: replace: add --graft option Christian Couder
2014-06-04 19:43 ` [PATCH v3 4/4] contrib: add convert-grafts-to-replace-refs.sh Christian Couder
2014-06-05 21:55 ` Junio C Hamano
2014-06-06 15:47 ` Christian Couder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqegz2q8ih.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=chriscool@tuxfamily$(echo .)org \
--cc=christian.couder@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jnareb@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox