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

On Wed, Jul 9, 2014 at 4:59 PM, Junio C Hamano [off-list ref] wrote:

Christian Couder [off-list ref] writes:

quoted

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

quoted

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