Re: [PATCH v6 02/10] replace: add --graft option

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jul 9, 2014 at 4:59 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Christian Couder <chriscool@xxxxxxxxxxxxx> writes:
>
>> +static void replace_parents(struct strbuf *buf, int argc, const char **argv)
>> +{
>> +     struct strbuf new_parents = STRBUF_INIT;
>> +     const char *parent_start, *parent_end;
>> +     int i;
>> +
>> +     /* find existing parents */
>> +     parent_start = buf->buf;
>> +     parent_start += 46; /* "tree " + "hex sha1" + "\n" */
>> +     parent_end = parent_start;
>> +
>> +     while (starts_with(parent_end, "parent "))
>> +             parent_end += 48; /* "parent " + "hex sha1" + "\n" */
>> +
>> +     /* prepare new parents */
>> +     for (i = 1; i < argc; i++) {
>
> It looks somewhat strange that both replace_parents() and
> create_graft() take familiar-looking <argc, argv> pair, but one
> ignores argv[0] and uses the remainder and the other uses argv[0].
> Shouldn't this function consume argv[] starting from [0] for
> consistency?  You'd obviously need to update the caller to adjust
> the arguments it gives to this function.

Ok, will do.

>> +static int create_graft(int argc, const char **argv, int force)
>> +{
>> +     unsigned char old[20], new[20];
>> +     const char *old_ref = argv[0];
>> +...
>> +
>> +     replace_parents(&buf, argc, argv);
>> +
>> +     if (write_sha1_file(buf.buf, buf.len, commit_type, new))
>> +             die(_("could not write replacement commit for: '%s'"), old_ref);
>> +
>> +     strbuf_release(&buf);
>> +
>> +     if (!hashcmp(old, new))
>> +             return error("new commit is the same as the old one: '%s'", sha1_to_hex(old));
>
> Is this really an error?  It may be a warning-worthy situation for a
> user or a script to end up doing a no-op graft, e.g.
>
>         git replace --graft HEAD HEAD^
>
> but I wonder if it is more convenient to signal an error (like this
> patch does) or just ignore the request and return without adding the
> replace ref.

As the user might expect that a new replace ref was created on success
(0 exit code), and as we should at least warn if we would create a
commit that is the same as an existing one, I think it is just simpler
to error out in this case.

Though maybe we could use a special exit code (for example 2) in this
case, so that the user might more easily ignore this error in a
script.

> Other than these two points, looks good to me.

Thanks,
Christian.
--
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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]