| Message ID | 20190328171722.9753-3-chriscool@tuxfamily.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] t6050: use test_line_count instead of wc -l | expand |
Hi Christian, On Thu, Mar 28, 2019 at 06:17:22PM +0100, Christian Couder wrote: > When passing a tag as a parent argument to `git replace --graft`, > it can be useful to accept it and use the underlying commit as a > parent. > > This already works for lightweight tags, but unfortunately > for annotated tags we have been using the hash of the tag object > instead of the hash of the underlying commit as a parent in the > replacement object we create. Ah. If I interpret your description correctly, the issue is that Git doesn't follow the annotated tag through to the commit it points to. Makes sense. > This created invalid objects, but the replace succeeded even if > it showed an error like: > > error: object A is a tag, not a commit > > This patch fixes that by using the hash of the underlying commit > when an annotated tag is passed. This seems like the straightforward fix. > While at it, let's also update an error message to make it > clearer. > > Signed-off-by: Christian Couder <chriscool@tuxfamily.org> > --- > > This doesn't fix issues when an annotated tag is passed as the > replaced object, that is when the tag is the first argument after > `git replace --graft`. But this can be done in subsequent patches > I already started to work on. > > builtin/replace.c | 9 ++++++--- > t/t6050-replace.sh | 11 +++++++++++ > 2 files changed, 17 insertions(+), 3 deletions(-) > > 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]); Good. After I read the commit message above, I was curious to see how this worked if you provided an annotated tag that pointed to a non-commit. 'lookup_commit_reference' should return NULL in that case, which you handle here appropriately. (And thanks for improving the error message, too ;-).) > } > - 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 */ > diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh > index 5cb8281bab..72075983ac 100755 > --- a/t/t6050-replace.sh > +++ b/t/t6050-replace.sh > @@ -405,6 +405,17 @@ test_expect_success '--graft with and without already replaced object' ' > git replace -d $HASH5 > ' > > +test_expect_success '--graft using a tag as the new parent' ' > + git tag new_parent $HASH5 && > + git replace --graft $HASH7 new_parent && > + commit_has_parents $HASH7 $HASH5 && > + git replace -d $HASH7 && > + git tag -a -m "annotated new parent tag" annotated_new_parent $HASH5 && > + git replace --graft $HASH7 annotated_new_parent && > + commit_has_parents $HASH7 $HASH5 && > + git replace -d $HASH7 Very nice. I would normally write at the beginning of the test instead: test_when_finished "git replace -d $HASH7" But I think your choice here is much better, since you need to delete the replacement a few lines above, too. I think it would be jarring to have both the inline `git replace -d`, and another from `test_when_finished`, so this makes much more sense. > +' > + > test_expect_success GPG 'set up a signed commit' ' > echo "line 17" >>hello && > echo "line 18" >>hello && > -- > 2.21.0.68.gd997bba285.dirty > The previous two look good as well. Reviewed-by: Taylor Blau <me@ttaylorr.com> Thanks, Taylor
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 */ diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh index 5cb8281bab..72075983ac 100755 --- a/t/t6050-replace.sh +++ b/t/t6050-replace.sh @@ -405,6 +405,17 @@ test_expect_success '--graft with and without already replaced object' ' git replace -d $HASH5 ' +test_expect_success '--graft using a tag as the new parent' ' + git tag new_parent $HASH5 && + git replace --graft $HASH7 new_parent && + commit_has_parents $HASH7 $HASH5 && + git replace -d $HASH7 && + git tag -a -m "annotated new parent tag" annotated_new_parent $HASH5 && + git replace --graft $HASH7 annotated_new_parent && + commit_has_parents $HASH7 $HASH5 && + git replace -d $HASH7 +' + test_expect_success GPG 'set up a signed commit' ' echo "line 17" >>hello && echo "line 18" >>hello &&
When passing a tag as a parent argument to `git replace --graft`, it can be useful to accept it and use the underlying commit as a parent. This already works for lightweight tags, but unfortunately for annotated tags we have been using the hash of the tag object instead of the hash of the underlying commit as a parent in the replacement object we create. This created invalid objects, but the replace succeeded even if it showed an error like: error: object A is a tag, not a commit This patch fixes that by using the hash of the underlying commit when an annotated tag is passed. While at it, let's also update an error message to make it clearer. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> --- This doesn't fix issues when an annotated tag is passed as the replaced object, that is when the tag is the first argument after `git replace --graft`. But this can be done in subsequent patches I already started to work on. builtin/replace.c | 9 ++++++--- t/t6050-replace.sh | 11 +++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-)