On Wed, Jul 9, 2014 at 4:59 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Christian Couder <chrisc...@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.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
