git replace --graft does error checking too late
When running `git replace --graft A B' where B is a non-commit (eg. a tag) it displays an error, but creates the replace ref anyway. I think it should verify that B names a commit object before creating the ref.
Andreas.
Signature · 3 lines
Andreas Schwab, SUSE Labs, [email protected] GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Quoted 3 lines · Andreas Schwab
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.
Quoted 2 lines
but creates the replace ref anyway. I think
it should verify that B names a commit object before creating the ref.
Accepting a tag and using the commit the tag points to could be useful.
For example someone could look at the commit graph, then decide to tag the commit(s) that should be used when replacing, and then use git replace --graft A <tag>...
using the created tag(s).
(I checked the code in builtin/replace.c and it seems that we use lookup_commit_reference() on each of the new parents, so we should be safe in case one of the given new parents cannot be peeled into a commit.)
So it seems to me that the issue is that it shows a wrong error when it shouldn't show anything, or perhaps only a warning.
Quoted 11 lines · Christian Couder
Quoted 3 lines · Andreas Schwab
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.
Quoted 15 lines · Christian Couder
Quoted 11 lines · Christian Couder
Quoted 3 lines · Andreas Schwab
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.
Quoted 8 lines
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.
builtin/replace.c +0 -0
@@ -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 */