# [PATCH 1/3] t6050: use test_line_count instead of wc -l 6 messages from 2019-03-28 to 2019-03-28. Participants: Christian Couder, Taylor Blau, Eric Sunshine, Ævar Arnfjörð Bjarmason. Thread: https://gitlist.dev/t/50835 ## Christian Couder, 2019-03-28 17:17 Subject: [PATCH 1/3] t6050: use test_line_count instead of wc -l Message-ID: <20190328171722.9753-1-chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20190328171722.9753-1-chriscool%40tuxfamily.org ``` This modernizes a test and makes it more portable. Signed-off-by: Christian Couder --- t/t6050-replace.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh index d638119750..41b177936e 100755 --- a/t/t6050-replace.sh +++ b/t/t6050-replace.sh @@ -393,9 +393,11 @@ test_expect_success 'replace ref cleanup' ' ' test_expect_success '--graft with and without already replaced object' ' - test $(git log --oneline | wc -l) = 7 && + git log --oneline >log && + test_line_count = 7 log && git replace --graft $HASH5 && - test $(git log --oneline | wc -l) = 3 && + git log --oneline >log && + test_line_count = 3 log && commit_has_parents $HASH5 && test_must_fail git replace --graft $HASH5 $HASH4 $HASH3 && git replace --force -g $HASH5 $HASH4 $HASH3 && -- 2.21.0.68.gd997bba285.dirty ``` ## Christian Couder, 2019-03-28 17:17 Subject: [PATCH 2/3] t6050: redirect expected error output to /dev/null Message-ID: <20190328171722.9753-2-chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20190328171722.9753-2-chriscool%40tuxfamily.org In-Reply-To: <20190328171722.9753-1-chriscool@tuxfamily.org> ``` Otherwise the error from `git rev-parse` is uselessly polluting the debug output. Signed-off-by: Christian Couder --- t/t6050-replace.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh index 41b177936e..5cb8281bab 100755 --- a/t/t6050-replace.sh +++ b/t/t6050-replace.sh @@ -40,7 +40,7 @@ commit_peeling_shows_parents () test "$_found" = "$_parent" || return 1 _parent_number=$(( $_parent_number + 1 )) done && - test_must_fail git rev-parse --verify $_commit^$_parent_number + test_must_fail git rev-parse --verify $_commit^$_parent_number 2>/dev/null } commit_has_parents () -- 2.21.0.68.gd997bba285.dirty ``` ## Christian Couder, 2019-03-28 17:17 Subject: [PATCH 3/3] replace: fix --graft when passing a tag Message-ID: <20190328171722.9753-3-chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20190328171722.9753-3-chriscool%40tuxfamily.org In-Reply-To: <20190328171722.9753-1-chriscool@tuxfamily.org> ``` 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 --- 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]); } - 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 && -- 2.21.0.68.gd997bba285.dirty ``` ## Taylor Blau, 2019-03-28 18:38 Subject: Re: [PATCH 3/3] replace: fix --graft when passing a tag Message-ID: <20190328183812.GA24326@Taylors-MacBook-Pro.local> URL: https://gitlist.dev/e/20190328183812.GA24326%40Taylors-MacBook-Pro.local In-Reply-To: <20190328171722.9753-3-chriscool@tuxfamily.org> ``` 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 > --- > > 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 Thanks, Taylor ``` ## Eric Sunshine, 2019-03-28 18:41 Subject: Re: [PATCH 2/3] t6050: redirect expected error output to /dev/null Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSj5WZeGy4nwgafGtBbz7y921M8H57eFbBXwRy2Bm-C%2Bg%40mail.gmail.com In-Reply-To: <20190328171722.9753-2-chriscool@tuxfamily.org> ``` On Thu, Mar 28, 2019 at 1:17 PM Christian Couder wrote: > Otherwise the error from `git rev-parse` is uselessly > polluting the debug output. > > Signed-off-by: Christian Couder > --- > diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh > @@ -40,7 +40,7 @@ commit_peeling_shows_parents () > - test_must_fail git rev-parse --verify $_commit^$_parent_number > + test_must_fail git rev-parse --verify $_commit^$_parent_number 2>/dev/null When I'm debugging test failures, I find it very helpful to see that a command which was expected to fail did indeed fail with the expected error message, so I don't consider such a message as "uselessly polluting the debug output". Consequently, I'm rather negative on this change. ``` ## Ævar Arnfjörð Bjarmason, 2019-03-28 20:22 Subject: Re: [PATCH 2/3] t6050: redirect expected error output to /dev/null Message-ID: <87va02agvo.fsf@evledraar.gmail.com> URL: https://gitlist.dev/e/87va02agvo.fsf%40evledraar.gmail.com In-Reply-To: ``` On Thu, Mar 28 2019, Eric Sunshine wrote: > On Thu, Mar 28, 2019 at 1:17 PM Christian Couder > wrote: >> Otherwise the error from `git rev-parse` is uselessly >> polluting the debug output. >> >> Signed-off-by: Christian Couder >> --- >> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh >> @@ -40,7 +40,7 @@ commit_peeling_shows_parents () >> - test_must_fail git rev-parse --verify $_commit^$_parent_number >> + test_must_fail git rev-parse --verify $_commit^$_parent_number 2>/dev/null > > When I'm debugging test failures, I find it very helpful to see that a > command which was expected to fail did indeed fail with the expected > error message, so I don't consider such a message as "uselessly > polluting the debug output". Consequently, I'm rather negative on this > change. I believe this instead should make both of you happy: diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh index d638119750..2e3009f3cb 100755 --- a/t/t6050-replace.sh +++ b/t/t6050-replace.sh @@ -40,7 +40,8 @@ commit_peeling_shows_parents () test "$_found" = "$_parent" || return 1 _parent_number=$(( $_parent_number + 1 )) done && - test_must_fail git rev-parse --verify $_commit^$_parent_number + test_must_fail git rev-parse --verify $_commit^$_parent_number 2>err && + test_i18ngrep "Needed a single revision" err } commit_has_parents () Note that the file has existing 4-space-instead-of-tabs issues. ```