{"thread":{"id":"55331","subject":"git bisect fails to handle annotated tags","startedAt":"2021-03-16T13:06:39Z","lastAt":"2021-03-17T18:25:07Z","messageCount":5,"participants":["Andreas Schwab","Jeff King","Bagas Sanjaya","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"419299","messageId":"878s6nz1sg.fsf@igel.home","threadId":"55331","inReplyTo":null,"subject":"git bisect fails to handle annotated tags","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2021-03-16T13:05:51Z","receivedAt":"2021-03-16T13:06:39Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"$ git --version\ngit version 2.31.0\n$ git bisect start\n$ git bisect good v2.30.0\n$ git bisect bad v2.31.0\n3e90d4b58f3819cfd58ac61cb8668e83d3ea0563 was both good and bad\n$ git bisect log\ngit bisect start\n# good: [71ca53e8125e36efbda17293c50027d31681a41f] Git 2.30\ngit bisect good 2d9685d47a7e516281aa093bf0cddc8aafa72448\n# bad: [a5828ae6b52137b913b978e16cd2334482eb4c1f] Git 2.31\ngit bisect bad 3e90d4b58f3819cfd58ac61cb8668e83d3ea0563\n$ git bisect start\nAlready on 'master'\nYour branch is up to date with 'origin/master'.\n$ git bisect good v2.30.0^{}\n$ git bisect bad v2.31.0^{}\nBisecting: 453 revisions left to test after this (roughly 9 steps)\n[41abfe15d95ede4c2a047180a6062eac23d8f7d6] maintenance: add pack-refs task\n$ git bisect log\ngit bisect start\n# good: [71ca53e8125e36efbda17293c50027d31681a41f] Git 2.30\ngit bisect good 71ca53e8125e36efbda17293c50027d31681a41f\n# bad: [a5828ae6b52137b913b978e16cd2334482eb4c1f] Git 2.31\ngit bisect bad a5828ae6b52137b913b978e16cd2334482eb4c1f\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1\n\"And now for something completely different.\"\n"},{"id":"419305","messageId":"YFDGX4EsrvHqZgPF@coredump.intra.peff.net","threadId":"55331","inReplyTo":"878s6nz1sg.fsf@igel.home","subject":"Re: git bisect fails to handle annotated tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-16T14:53:19Z","receivedAt":"2021-03-16T14:53:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 16, 2021 at 02:05:51PM +0100, Andreas Schwab wrote:\n\n> $ git --version\n> git version 2.31.0\n> $ git bisect start\n> $ git bisect good v2.30.0\n> $ git bisect bad v2.31.0\n> 3e90d4b58f3819cfd58ac61cb8668e83d3ea0563 was both good and bad\n\nLooks like it bisects to 27257bc466 (bisect--helper: reimplement\n`bisect_state` & `bisect_head` shell functions in C, 2020-10-15), which\nisn't too surprising. So it broke in v2.30, but nobody seems to have\nnoticed during the last cycle.\n\nI'd guess it's just missing a call to peel the input oid.\n\n-Peff\n"},{"id":"419307","messageId":"YFDLq9mLbJtLqKea@coredump.intra.peff.net","threadId":"55331","inReplyTo":"YFDGX4EsrvHqZgPF@coredump.intra.peff.net","subject":"[PATCH] bisect: peel annotated tags to commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-16T15:15:55Z","receivedAt":"2021-03-16T15:18:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 16, 2021 at 10:53:19AM -0400, Jeff King wrote:\n\n> On Tue, Mar 16, 2021 at 02:05:51PM +0100, Andreas Schwab wrote:\n> \n> > $ git --version\n> > git version 2.31.0\n> > $ git bisect start\n> > $ git bisect good v2.30.0\n> > $ git bisect bad v2.31.0\n> > 3e90d4b58f3819cfd58ac61cb8668e83d3ea0563 was both good and bad\n> \n> Looks like it bisects to 27257bc466 (bisect--helper: reimplement\n> `bisect_state` & `bisect_head` shell functions in C, 2020-10-15), which\n> isn't too surprising. So it broke in v2.30, but nobody seems to have\n> noticed during the last cycle.\n> \n> I'd guess it's just missing a call to peel the input oid.\n\nYep. Here's a fix. Again, not new in v2.31, so we don't have to worry\nabout a brown-bag fix for yesterday's release. But I do think it's worth\ntrying to get onto a maint release. I prepared this patch on top of\nmr/bisect-in-c-3.\n\n-- >8 --\nSubject: [PATCH] bisect: peel annotated tags to commits\n\nThis patch fixes a bug where git-bisect doesn't handle receiving\nannotated tags as \"git bisect good <tag>\", etc. It's a regression in\n27257bc466 (bisect--helper: reimplement `bisect_state` & `bisect_head`\nshell functions in C, 2020-10-15).\n\nThe original shell code called:\n\n  sha=$(git rev-parse --verify \"$rev^{commit}\") ||\n          die \"$(eval_gettext \"Bad rev input: \\$rev\")\"\n\nwhich will peel the input to a commit (or complain if that's not\npossible). But the C code just calls get_oid(), which will yield the oid\nof the tag.\n\nThe fix is to peel to a commit. The error message here is a little\nnon-idiomatic for Git (since it starts with a capital). I've mostly left\nit, as it matches the other converted messages (like the \"Bad rev input\"\nwe print when get_oid() fails), though I did add an indication that it\nwas the peeling that was the problem. It might be worth taking a pass\nthrough this converted code to modernize some of the error messages.\n\nNote also that the test does a bare \"grep\" (not i18ngrep) on the\nexpected \"X is the first bad commit\" output message. This matches the\nrest of the test script.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/bisect--helper.c    |  9 ++++++++-\n t/t6030-bisect-porcelain.sh | 12 ++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex fc6ca257a4..f0eeb4a2f0 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -876,12 +876,19 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, const char **a\n \t */\n \n \tfor (; argc; argc--, argv++) {\n+\t\tstruct commit *commit;\n+\n \t\tif (get_oid(*argv, &oid)){\n \t\t\terror(_(\"Bad rev input: %s\"), *argv);\n \t\t\toid_array_clear(&revs);\n \t\t\treturn BISECT_FAILED;\n \t\t}\n-\t\toid_array_append(&revs, &oid);\n+\n+\t\tcommit = lookup_commit_reference(the_repository, &oid);\n+\t\tif (!commit)\n+\t\t\tdie(_(\"Bad rev input (not a commit): %s\"), *argv);\n+\n+\t\toid_array_append(&revs, &commit->object.oid);\n \t}\n \n \tif (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex b886529e59..9c389553a7 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -929,4 +929,16 @@ test_expect_success 'git bisect reset cleans bisection state properly' '\n \ttest_path_is_missing \"$GIT_DIR/BISECT_START\"\n '\n \n+test_expect_success 'bisect handles annotated tags' '\n+\ttest_commit commit-one &&\n+\tgit tag -m foo tag-one &&\n+\ttest_commit commit-two &&\n+\tgit tag -m foo tag-two &&\n+\tgit bisect start &&\n+\tgit bisect good tag-one &&\n+\tgit bisect bad tag-two >output &&\n+\tbad=$(git rev-parse --verify tag-two^{commit}) &&\n+\tgrep \"$bad is the first bad commit\" output\n+'\n+\n test_done\n-- \n2.31.0.559.g509d4a088b\n\n"},{"id":"419449","messageId":"5c41383f-55b5-a68e-4e35-8b8dc4694375@gmail.com","threadId":"55331","inReplyTo":"YFDLq9mLbJtLqKea@coredump.intra.peff.net","subject":"Re: [PATCH] bisect: peel annotated tags to commits","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-03-17T08:23:13Z","receivedAt":"2021-03-17T08:24:16Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"I can reproduce this issue with v2.31.0 (as you mentioned).\n\nApplying the patch, bisecting between annotated tags now worked\njust before git bisect is rewritten in C.\n\nThanks.\n\nTested-by: Bagas Sanjaya <bagasdotme@gmail.com>\n\nOn 16/03/21 22.15, Jeff King wrote:\n> On Tue, Mar 16, 2021 at 10:53:19AM -0400, Jeff King wrote:\n> \n>> On Tue, Mar 16, 2021 at 02:05:51PM +0100, Andreas Schwab wrote:\n>>\n>>> $ git --version\n>>> git version 2.31.0\n>>> $ git bisect start\n>>> $ git bisect good v2.30.0\n>>> $ git bisect bad v2.31.0\n>>> 3e90d4b58f3819cfd58ac61cb8668e83d3ea0563 was both good and bad\n>>\n>> Looks like it bisects to 27257bc466 (bisect--helper: reimplement\n>> `bisect_state` & `bisect_head` shell functions in C, 2020-10-15), which\n>> isn't too surprising. So it broke in v2.30, but nobody seems to have\n>> noticed during the last cycle.\n>>\n>> I'd guess it's just missing a call to peel the input oid.\n> \n> Yep. Here's a fix. Again, not new in v2.31, so we don't have to worry\n> about a brown-bag fix for yesterday's release. But I do think it's worth\n> trying to get onto a maint release. I prepared this patch on top of\n> mr/bisect-in-c-3.\n> \n> -- >8 --\n> Subject: [PATCH] bisect: peel annotated tags to commits\n> \n> This patch fixes a bug where git-bisect doesn't handle receiving\n> annotated tags as \"git bisect good <tag>\", etc. It's a regression in\n> 27257bc466 (bisect--helper: reimplement `bisect_state` & `bisect_head`\n> shell functions in C, 2020-10-15).\n> \n> The original shell code called:\n> \n>    sha=$(git rev-parse --verify \"$rev^{commit}\") ||\n>            die \"$(eval_gettext \"Bad rev input: \\$rev\")\"\n> \n> which will peel the input to a commit (or complain if that's not\n> possible). But the C code just calls get_oid(), which will yield the oid\n> of the tag.\n> \n> The fix is to peel to a commit. The error message here is a little\n> non-idiomatic for Git (since it starts with a capital). I've mostly left\n> it, as it matches the other converted messages (like the \"Bad rev input\"\n> we print when get_oid() fails), though I did add an indication that it\n> was the peeling that was the problem. It might be worth taking a pass\n> through this converted code to modernize some of the error messages.\n> \n> Note also that the test does a bare \"grep\" (not i18ngrep) on the\n> expected \"X is the first bad commit\" output message. This matches the\n> rest of the test script.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>   builtin/bisect--helper.c    |  9 ++++++++-\n>   t/t6030-bisect-porcelain.sh | 12 ++++++++++++\n>   2 files changed, 20 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index fc6ca257a4..f0eeb4a2f0 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -876,12 +876,19 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, const char **a\n>   \t */\n>   \n>   \tfor (; argc; argc--, argv++) {\n> +\t\tstruct commit *commit;\n> +\n>   \t\tif (get_oid(*argv, &oid)){\n>   \t\t\terror(_(\"Bad rev input: %s\"), *argv);\n>   \t\t\toid_array_clear(&revs);\n>   \t\t\treturn BISECT_FAILED;\n>   \t\t}\n> -\t\toid_array_append(&revs, &oid);\n> +\n> +\t\tcommit = lookup_commit_reference(the_repository, &oid);\n> +\t\tif (!commit)\n> +\t\t\tdie(_(\"Bad rev input (not a commit): %s\"), *argv);\n> +\n> +\t\toid_array_append(&revs, &commit->object.oid);\n>   \t}\n>   \n>   \tif (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||\n> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\n> index b886529e59..9c389553a7 100755\n> --- a/t/t6030-bisect-porcelain.sh\n> +++ b/t/t6030-bisect-porcelain.sh\n> @@ -929,4 +929,16 @@ test_expect_success 'git bisect reset cleans bisection state properly' '\n>   \ttest_path_is_missing \"$GIT_DIR/BISECT_START\"\n>   '\n>   \n> +test_expect_success 'bisect handles annotated tags' '\n> +\ttest_commit commit-one &&\n> +\tgit tag -m foo tag-one &&\n> +\ttest_commit commit-two &&\n> +\tgit tag -m foo tag-two &&\n> +\tgit bisect start &&\n> +\tgit bisect good tag-one &&\n> +\tgit bisect bad tag-two >output &&\n> +\tbad=$(git rev-parse --verify tag-two^{commit}) &&\n> +\tgrep \"$bad is the first bad commit\" output\n> +'\n> +\n>   test_done\n> \n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"419522","messageId":"xmqqim5pwse4.fsf@gitster.g","threadId":"55331","inReplyTo":"YFDLq9mLbJtLqKea@coredump.intra.peff.net","subject":"Re: [PATCH] bisect: peel annotated tags to commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-17T18:24:03Z","receivedAt":"2021-03-17T18:25:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> Looks like it bisects to 27257bc466 (bisect--helper: reimplement\n>> `bisect_state` & `bisect_head` shell functions in C, 2020-10-15), which\n>> isn't too surprising. So it broke in v2.30, but nobody seems to have\n>> noticed during the last cycle.\n>> \n>> I'd guess it's just missing a call to peel the input oid.\n>\n> Yep. Here's a fix. Again, not new in v2.31, so we don't have to worry\n> about a brown-bag fix for yesterday's release. But I do think it's worth\n> trying to get onto a maint release. I prepared this patch on top of\n> mr/bisect-in-c-3.\n\nThanks.  Yes, if we ever do another update to 2.30.x, this probably\nshould be in it, as it is expected that people will start with tags\nand not with individual commits.\n\n"}]}