{"thread":{"id":"50835","subject":"[PATCH 1/3] t6050: use test_line_count instead of wc -l","startedAt":"2019-03-28T17:17:50Z","lastAt":"2019-03-28T20:22:41Z","messageCount":6,"participants":["Christian Couder","Taylor Blau","Eric Sunshine","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"372634","messageId":"20190328171722.9753-1-chriscool@tuxfamily.org","threadId":"50835","inReplyTo":null,"subject":"[PATCH 1/3] t6050: use test_line_count instead of wc -l","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-28T17:17:20Z","receivedAt":"2019-03-28T17:17:50Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This modernizes a test and makes it more portable.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n t/t6050-replace.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex d638119750..41b177936e 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -393,9 +393,11 @@ test_expect_success 'replace ref cleanup' '\n '\n \n test_expect_success '--graft with and without already replaced object' '\n-\ttest $(git log --oneline | wc -l) = 7 &&\n+\tgit log --oneline >log &&\n+\ttest_line_count = 7 log &&\n \tgit replace --graft $HASH5 &&\n-\ttest $(git log --oneline | wc -l) = 3 &&\n+\tgit log --oneline >log &&\n+\ttest_line_count = 3 log &&\n \tcommit_has_parents $HASH5 &&\n \ttest_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&\n \tgit replace --force -g $HASH5 $HASH4 $HASH3 &&\n-- \n2.21.0.68.gd997bba285.dirty\n\n"},{"id":"372635","messageId":"20190328171722.9753-2-chriscool@tuxfamily.org","threadId":"50835","inReplyTo":"20190328171722.9753-1-chriscool@tuxfamily.org","subject":"[PATCH 2/3] t6050: redirect expected error output to /dev/null","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-28T17:17:21Z","receivedAt":"2019-03-28T17:17:53Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Otherwise the error from `git rev-parse` is uselessly\npolluting the debug output.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n t/t6050-replace.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex 41b177936e..5cb8281bab 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -40,7 +40,7 @@ commit_peeling_shows_parents ()\n \ttest \"$_found\" = \"$_parent\" || return 1\n \t_parent_number=$(( $_parent_number + 1 ))\n     done &&\n-    test_must_fail git rev-parse --verify $_commit^$_parent_number\n+    test_must_fail git rev-parse --verify $_commit^$_parent_number 2>/dev/null\n }\n \n commit_has_parents ()\n-- \n2.21.0.68.gd997bba285.dirty\n\n"},{"id":"372636","messageId":"20190328171722.9753-3-chriscool@tuxfamily.org","threadId":"50835","inReplyTo":"20190328171722.9753-1-chriscool@tuxfamily.org","subject":"[PATCH 3/3] replace: fix --graft when passing a tag","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-28T17:17:22Z","receivedAt":"2019-03-28T17:18:35Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"When passing a tag as a parent argument to `git replace --graft`,\nit can be useful to accept it and use the underlying commit as a\nparent.\n\nThis already works for lightweight tags, but unfortunately\nfor annotated tags we have been using the hash of the tag object\ninstead of the hash of the underlying commit as a parent in the\nreplacement object we create.\n\nThis created invalid objects, but the replace succeeded even if\nit showed an error like:\n\nerror: object A is a tag, not a commit\n\nThis patch fixes that by using the hash of the underlying commit\nwhen an annotated tag is passed.\n\nWhile at it, let's also update an error message to make it\nclearer.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n\nThis doesn't fix issues when an annotated tag is passed as the\nreplaced object, that is when the tag is the first argument after\n`git replace --graft`. But this can be done in subsequent patches\nI already started to work on. \n\n builtin/replace.c  |  9 ++++++---\n t/t6050-replace.sh | 11 +++++++++++\n 2 files changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex f5701629a8..b0a9227f9a 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -370,16 +370,19 @@ static int replace_parents(struct strbuf *buf, int argc, const char **argv)\n \t/* prepare new parents */\n \tfor (i = 0; i < argc; i++) {\n \t\tstruct object_id oid;\n+\t\tstruct commit *commit;\n+\n \t\tif (get_oid(argv[i], &oid) < 0) {\n \t\t\tstrbuf_release(&new_parents);\n \t\t\treturn error(_(\"not a valid object name: '%s'\"),\n \t\t\t\t     argv[i]);\n \t\t}\n-\t\tif (!lookup_commit_reference(the_repository, &oid)) {\n+\t\tcommit = lookup_commit_reference(the_repository, &oid);\n+\t\tif (!commit) {\n \t\t\tstrbuf_release(&new_parents);\n-\t\t\treturn error(_(\"could not parse %s\"), argv[i]);\n+\t\t\treturn error(_(\"could not parse %s as a commit\"), argv[i]);\n \t\t}\n-\t\tstrbuf_addf(&new_parents, \"parent %s\\n\", oid_to_hex(&oid));\n+\t\tstrbuf_addf(&new_parents, \"parent %s\\n\", oid_to_hex(&commit->object.oid));\n \t}\n \n \t/* replace existing parents with new ones */\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex 5cb8281bab..72075983ac 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -405,6 +405,17 @@ test_expect_success '--graft with and without already replaced object' '\n \tgit replace -d $HASH5\n '\n \n+test_expect_success '--graft using a tag as the new parent' '\n+\tgit tag new_parent $HASH5 &&\n+\tgit replace --graft $HASH7 new_parent &&\n+\tcommit_has_parents $HASH7 $HASH5 &&\n+\tgit replace -d $HASH7 &&\n+\tgit tag -a -m \"annotated new parent tag\" annotated_new_parent $HASH5 &&\n+\tgit replace --graft $HASH7 annotated_new_parent &&\n+\tcommit_has_parents $HASH7 $HASH5 &&\n+\tgit replace -d $HASH7\n+'\n+\n test_expect_success GPG 'set up a signed commit' '\n \techo \"line 17\" >>hello &&\n \techo \"line 18\" >>hello &&\n-- \n2.21.0.68.gd997bba285.dirty\n\n"},{"id":"372640","messageId":"20190328183812.GA24326@Taylors-MacBook-Pro.local","threadId":"50835","inReplyTo":"20190328171722.9753-3-chriscool@tuxfamily.org","subject":"Re: [PATCH 3/3] replace: fix --graft when passing a tag","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-03-28T18:38:12Z","receivedAt":"2019-03-28T18:38:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Christian,\n\nOn Thu, Mar 28, 2019 at 06:17:22PM +0100, Christian Couder wrote:\n> When passing a tag as a parent argument to `git replace --graft`,\n> it can be useful to accept it and use the underlying commit as a\n> parent.\n>\n> This already works for lightweight tags, but unfortunately\n> for annotated tags we have been using the hash of the tag object\n> instead of the hash of the underlying commit as a parent in the\n> replacement object we create.\n\nAh. If I interpret your description correctly, the issue is that Git\ndoesn't follow the annotated tag through to the commit it points to.\nMakes sense.\n\n> This created invalid objects, but the replace succeeded even if\n> it showed an error like:\n>\n> error: object A is a tag, not a commit\n>\n> This patch fixes that by using the hash of the underlying commit\n> when an annotated tag is passed.\n\nThis seems like the straightforward fix.\n\n> While at it, let's also update an error message to make it\n> clearer.\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>\n> This doesn't fix issues when an annotated tag is passed as the\n> replaced object, that is when the tag is the first argument after\n> `git replace --graft`. But this can be done in subsequent patches\n> I already started to work on.\n>\n>  builtin/replace.c  |  9 ++++++---\n>  t/t6050-replace.sh | 11 +++++++++++\n>  2 files changed, 17 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index f5701629a8..b0a9227f9a 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -370,16 +370,19 @@ static int replace_parents(struct strbuf *buf, int argc, const char **argv)\n>  \t/* prepare new parents */\n>  \tfor (i = 0; i < argc; i++) {\n>  \t\tstruct object_id oid;\n> +\t\tstruct commit *commit;\n> +\n>  \t\tif (get_oid(argv[i], &oid) < 0) {\n>  \t\t\tstrbuf_release(&new_parents);\n>  \t\t\treturn error(_(\"not a valid object name: '%s'\"),\n>  \t\t\t\t     argv[i]);\n>  \t\t}\n> -\t\tif (!lookup_commit_reference(the_repository, &oid)) {\n> +\t\tcommit = lookup_commit_reference(the_repository, &oid);\n> +\t\tif (!commit) {\n>  \t\t\tstrbuf_release(&new_parents);\n> -\t\t\treturn error(_(\"could not parse %s\"), argv[i]);\n> +\t\t\treturn error(_(\"could not parse %s as a commit\"), argv[i]);\n\nGood. After I read the commit message above, I was curious to see how\nthis worked if you provided an annotated tag that pointed to a\nnon-commit.\n\n'lookup_commit_reference' should return NULL in that case, which you\nhandle here appropriately. (And thanks for improving the error message,\ntoo ;-).)\n\n>  \t\t}\n> -\t\tstrbuf_addf(&new_parents, \"parent %s\\n\", oid_to_hex(&oid));\n> +\t\tstrbuf_addf(&new_parents, \"parent %s\\n\", oid_to_hex(&commit->object.oid));\n>  \t}\n>\n>  \t/* replace existing parents with new ones */\n> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\n> index 5cb8281bab..72075983ac 100755\n> --- a/t/t6050-replace.sh\n> +++ b/t/t6050-replace.sh\n> @@ -405,6 +405,17 @@ test_expect_success '--graft with and without already replaced object' '\n>  \tgit replace -d $HASH5\n>  '\n>\n> +test_expect_success '--graft using a tag as the new parent' '\n> +\tgit tag new_parent $HASH5 &&\n> +\tgit replace --graft $HASH7 new_parent &&\n> +\tcommit_has_parents $HASH7 $HASH5 &&\n> +\tgit replace -d $HASH7 &&\n> +\tgit tag -a -m \"annotated new parent tag\" annotated_new_parent $HASH5 &&\n> +\tgit replace --graft $HASH7 annotated_new_parent &&\n> +\tcommit_has_parents $HASH7 $HASH5 &&\n> +\tgit replace -d $HASH7\n\nVery nice. I would normally write at the beginning of the test instead:\n\n  test_when_finished \"git replace -d $HASH7\"\n\nBut I think your choice here is much better, since you need to delete\nthe replacement a few lines above, too. I think it would be jarring to\nhave both the inline `git replace -d`, and another from\n`test_when_finished`, so this makes much more sense.\n\n> +'\n> +\n>  test_expect_success GPG 'set up a signed commit' '\n>  \techo \"line 17\" >>hello &&\n>  \techo \"line 18\" >>hello &&\n> --\n> 2.21.0.68.gd997bba285.dirty\n>\n\nThe previous two look good as well.\n\nReviewed-by: Taylor Blau <me@ttaylorr.com>\n\nThanks,\nTaylor\n"},{"id":"372641","messageId":"CAPig+cSj5WZeGy4nwgafGtBbz7y921M8H57eFbBXwRy2Bm-C+g@mail.gmail.com","threadId":"50835","inReplyTo":"20190328171722.9753-2-chriscool@tuxfamily.org","subject":"Re: [PATCH 2/3] t6050: redirect expected error output to /dev/null","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-28T18:41:08Z","receivedAt":"2019-03-28T18:41:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 28, 2019 at 1:17 PM Christian Couder\n<christian.couder@gmail.com> wrote:\n> Otherwise the error from `git rev-parse` is uselessly\n> polluting the debug output.\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\n> @@ -40,7 +40,7 @@ commit_peeling_shows_parents ()\n> -    test_must_fail git rev-parse --verify $_commit^$_parent_number\n> +    test_must_fail git rev-parse --verify $_commit^$_parent_number 2>/dev/null\n\nWhen I'm debugging test failures, I find it very helpful to see that a\ncommand which was expected to fail did indeed fail with the expected\nerror message, so I don't consider such a message as \"uselessly\npolluting the debug output\". Consequently, I'm rather negative on this\nchange.\n"},{"id":"372645","messageId":"87va02agvo.fsf@evledraar.gmail.com","threadId":"50835","inReplyTo":"CAPig+cSj5WZeGy4nwgafGtBbz7y921M8H57eFbBXwRy2Bm-C+g@mail.gmail.com","subject":"Re: [PATCH 2/3] t6050: redirect expected error output to /dev/null","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-28T20:22:35Z","receivedAt":"2019-03-28T20:22:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Mar 28 2019, Eric Sunshine wrote:\n\n> On Thu, Mar 28, 2019 at 1:17 PM Christian Couder\n> <christian.couder@gmail.com> wrote:\n>> Otherwise the error from `git rev-parse` is uselessly\n>> polluting the debug output.\n>>\n>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n>> ---\n>> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\n>> @@ -40,7 +40,7 @@ commit_peeling_shows_parents ()\n>> -    test_must_fail git rev-parse --verify $_commit^$_parent_number\n>> +    test_must_fail git rev-parse --verify $_commit^$_parent_number 2>/dev/null\n>\n> When I'm debugging test failures, I find it very helpful to see that a\n> command which was expected to fail did indeed fail with the expected\n> error message, so I don't consider such a message as \"uselessly\n> polluting the debug output\". Consequently, I'm rather negative on this\n> change.\n\nI believe this instead should make both of you happy:\n\n    diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\n    index d638119750..2e3009f3cb 100755\n    --- a/t/t6050-replace.sh\n    +++ b/t/t6050-replace.sh\n    @@ -40,7 +40,8 @@ commit_peeling_shows_parents ()\n            test \"$_found\" = \"$_parent\" || return 1\n            _parent_number=$(( $_parent_number + 1 ))\n         done &&\n    -    test_must_fail git rev-parse --verify $_commit^$_parent_number\n    +    test_must_fail git rev-parse --verify $_commit^$_parent_number 2>err &&\n    +    test_i18ngrep \"Needed a single revision\" err\n     }\n\n     commit_has_parents ()\n\nNote that the file has existing 4-space-instead-of-tabs issues.\n"}]}