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

> Christian Couder <chriscool@tuxfamily.org> 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.

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