* [PATCH 1/3] t6050: use test_line_count instead of wc -l @ 2019-03-28 17:17 Christian Couder 2019-03-28 17:17 ` [PATCH 2/3] t6050: redirect expected error output to /dev/null Christian Couder 2019-03-28 17:17 ` [PATCH 3/3] replace: fix --graft when passing a tag Christian Couder 0 siblings, 2 replies; 6+ messages in thread From: Christian Couder @ 2019-03-28 17:17 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Andreas Schwab, Christian Couder This modernizes a test and makes it more portable. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] t6050: redirect expected error output to /dev/null 2019-03-28 17:17 [PATCH 1/3] t6050: use test_line_count instead of wc -l Christian Couder @ 2019-03-28 17:17 ` Christian Couder 2019-03-28 18:41 ` Eric Sunshine 2019-03-28 17:17 ` [PATCH 3/3] replace: fix --graft when passing a tag Christian Couder 1 sibling, 1 reply; 6+ messages in thread From: Christian Couder @ 2019-03-28 17:17 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Andreas Schwab, Christian Couder Otherwise the error from `git rev-parse` is uselessly polluting the debug output. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] t6050: redirect expected error output to /dev/null 2019-03-28 17:17 ` [PATCH 2/3] t6050: redirect expected error output to /dev/null Christian Couder @ 2019-03-28 18:41 ` Eric Sunshine 2019-03-28 20:22 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 6+ messages in thread From: Eric Sunshine @ 2019-03-28 18:41 UTC (permalink / raw) To: Christian Couder Cc: Git List, Junio C Hamano, Andreas Schwab, Christian Couder On Thu, Mar 28, 2019 at 1:17 PM Christian Couder <christian.couder@gmail.com> wrote: > Otherwise the error from `git rev-parse` is uselessly > polluting the debug output. > > Signed-off-by: Christian Couder <chriscool@tuxfamily.org> > --- > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] t6050: redirect expected error output to /dev/null 2019-03-28 18:41 ` Eric Sunshine @ 2019-03-28 20:22 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 6+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-03-28 20:22 UTC (permalink / raw) To: Eric Sunshine Cc: Christian Couder, Git List, Junio C Hamano, Andreas Schwab, Christian Couder On Thu, Mar 28 2019, Eric Sunshine wrote: > On Thu, Mar 28, 2019 at 1:17 PM Christian Couder > <christian.couder@gmail.com> wrote: >> Otherwise the error from `git rev-parse` is uselessly >> polluting the debug output. >> >> Signed-off-by: Christian Couder <chriscool@tuxfamily.org> >> --- >> 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] replace: fix --graft when passing a tag 2019-03-28 17:17 [PATCH 1/3] t6050: use test_line_count instead of wc -l Christian Couder 2019-03-28 17:17 ` [PATCH 2/3] t6050: redirect expected error output to /dev/null Christian Couder @ 2019-03-28 17:17 ` Christian Couder 2019-03-28 18:38 ` Taylor Blau 1 sibling, 1 reply; 6+ messages in thread From: Christian Couder @ 2019-03-28 17:17 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Andreas Schwab, Christian Couder 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(-) 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] replace: fix --graft when passing a tag 2019-03-28 17:17 ` [PATCH 3/3] replace: fix --graft when passing a tag Christian Couder @ 2019-03-28 18:38 ` Taylor Blau 0 siblings, 0 replies; 6+ messages in thread From: Taylor Blau @ 2019-03-28 18:38 UTC (permalink / raw) To: Christian Couder; +Cc: git, Junio C Hamano, Andreas Schwab, Christian Couder 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-28 20:22 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-28 17:17 [PATCH 1/3] t6050: use test_line_count instead of wc -l Christian Couder 2019-03-28 17:17 ` [PATCH 2/3] t6050: redirect expected error output to /dev/null Christian Couder 2019-03-28 18:41 ` Eric Sunshine 2019-03-28 20:22 ` Ævar Arnfjörð Bjarmason 2019-03-28 17:17 ` [PATCH 3/3] replace: fix --graft when passing a tag Christian Couder 2019-03-28 18:38 ` Taylor Blau
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).