Re: git replace --graft does error checking too late

From: Christian Couder <christian.couder@gmail.com>
Date: Mar 28, 2019, 07:35 UTC
Message-ID: <CAP8UFD2yMOLKOdcXnPsCvAzymjLJX2A4U2DHfq1JjvZnQj9pLQ@mail.gmail.com>
In-Reply-To: <CAP8UFD111-zqa7Fhr1KnmWYAKpBN4ofqn0AvD_nP3s_WBFzLqw@mail.gmail.com>

On Wed, Mar 27, 2019 at 2:21 PM Christian Couder <christian.couder@gmail.com> wrote:

Show 23 quoted lines

>
> On Wed, Mar 27, 2019 at 2:11 PM Christian Couder
> <christian.couder@gmail.com> wrote:
> >
> > On Wed, Mar 27, 2019 at 11:24 AM Andreas Schwab <schwab@suse.de> wrote:
> > >
> > > When running `git replace --graft A B' where B is a non-commit (eg. a
> > > tag) it displays an error,
> >
> > Yeah, it seems that when A is a commit and B a tag I get:
> >
> > "error: object A is a tag, not a commit"
> >
> > which is wrong as A is a commit.
>
> Actually I get the above only if A is a commit but there is a tag
> pointing to it. If there is no tag pointing to it I get:
>
> error: object C is a tag, not a commit
>
> where C is the hash of the tag object B (C=$(git rev-parse B))
>
> So yeah, this is weird.

I think I understand what happens. It seems that we put the SHA-1 of the tag in the new object we create instead of the SHA-1 of the underlying commit and that's what generates the error message and other issues later.

The following patch (that unfortunately Gmail is likely to muck) should fix at least part of the issue. I will send a real patch with tests later.

diff --git a/builtin/replace.c b/builtin/replace.c
index f5701629a8..b0a9227f9a 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -370,16 +370,19 @@ static int replace_parents(struct strbuf *buf,
int argc, const char **argv)
        /* prepare new parents */
        for (i = 0; i < argc; i++) {
                struct object_id oid;
+               struct commit *commit;
+
                if (get_oid(argv[i], &oid) < 0) {
                        strbuf_release(&new_parents);
                        return error(_("not a valid object name: '%s'"),
                                     argv[i]);
                }
-               if (!lookup_commit_reference(the_repository, &oid)) {
+               commit = lookup_commit_reference(the_repository, &oid);
+               if (!commit) {
                        strbuf_release(&new_parents);
-                       return error(_("could not parse %s"), argv[i]);
+                       return error(_("could not parse %s as a
commit"), argv[i]);
                }
-               strbuf_addf(&new_parents, "parent %s\n", oid_to_hex(&oid));
+               strbuf_addf(&new_parents, "parent %s\n",
oid_to_hex(&commit->object.oid));
        }

        /* replace existing parents with new ones */