{"thread":{"id":"54283","subject":"[PATCH] bisect: don't use invalid oid as rev when starting","startedAt":"2020-09-23T17:09:33Z","lastAt":"2020-09-25T16:54:28Z","messageCount":23,"participants":["Christian Couder","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"406203","messageId":"20200923170915.21748-1-chriscool@tuxfamily.org","threadId":"54283","inReplyTo":null,"subject":"[PATCH] bisect: don't use invalid oid as rev when starting","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-09-23T17:09:15Z","receivedAt":"2020-09-23T17:09:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"In 06f5608c14 (bisect--helper: `bisect_start` shell function\npartially in C, 2019-01-02), we changed the following shell\ncode:\n\n- rev=$(git rev-parse -q --verify \"$arg^{commit}\") || {\n- test $has_double_dash -eq 1 &&\n- die \"$(eval_gettext \"'\\$arg' does not appear to be a valid revision\")\"\n- break\n- }\n- revs=\"$revs $rev\"\n\ninto:\n\n+ char *commit_id = xstrfmt(\"%s^{commit}\", arg);\n+ if (get_oid(commit_id, &oid) && has_double_dash)\n+ die(_(\"'%s' does not appear to be a valid \"\n+ \"revision\"), arg);\n+\n+ string_list_append(&revs, oid_to_hex(&oid));\n+ free(commit_id);\n\nIn case of an invalid \"arg\" when \"has_double_dash\" is false, the old\ncode would \"break\" out of the argument loop.\n\nIn the new C code though, `oid_to_hex(&oid)` is unconditonally\nappended to \"revs\". This is wrong first because \"oid\" is junk as\n`get_oid(commit_id, &oid)` failed and second because it doesn't break\nout of the argument loop.\n\nNot breaking out of the argument loop means that \"arg\" is then not\ntreated as a path restriction (which is wrong).\n\nSigned-off-by: Christian Couder \n---\nThis is a bug fix for the bug Miriam talks about in:\n\nhttps://lore.kernel.org/git/20200923072740.20772-1-mirucam@gmail.com/\n\nand:\n\nhttps://lore.kernel.org/git/CAN7CjDDVp_i7dhpbAq5zrGW69nE6+SfivJQ-dembmu+WyqKiQQ@mail.gmail.com/\n\n builtin/bisect--helper.c | 14 +++++++++-----\n t/t6030-bisect-porcelain.sh | 7 +++++++\n 2 files changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 7dcc1b5188..538fa6f16b 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -486,12 +486,16 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\n \t\t\treturn error(_(\"unrecognized option: '%s'\"), arg);\n \t\t} else {\n \t\t\tchar *commit_id = xstrfmt(\"%s^{commit}\", arg);\n-\t\t\tif (get_oid(commit_id, &oid) && has_double_dash)\n-\t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n-\t\t\t\t \"revision\"), arg);\n-\n-\t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n+\t\t\tint res = get_oid(commit_id, &oid);\n \t\t\tfree(commit_id);\n+\t\t\tif (res) {\n+\t\t\t\tif (has_double_dash)\n+\t\t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n+\t\t\t\t\t \"revision\"), arg);\n+\t\t\t\tbreak;\n+\t\t\t} else {\n+\t\t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n+\t\t\t}\n \t\t}\n \t}\n \tpathspec_pos = i;\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex b886529e59..cb645cf8c8 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -82,6 +82,13 @@ test_expect_success 'bisect fails if given any junk instead of revs' '\n \tgit bisect bad $HASH4\n '\n \n+test_expect_success 'bisect start without -- uses unknown stuff as path restriction' '\n+\tgit bisect reset &&\n+\tgit bisect start foo bar &&\n+\tgrep foo \".git/BISECT_NAMES\" &&\n+\tgrep bar \".git/BISECT_NAMES\"\n+'\n+\n test_expect_success 'bisect reset: back in the master branch' '\n \tgit bisect reset &&\n \techo \"* master\" > branch.expect &&\n-- \n2.28.0.587.g1c7fdf1d8b\n\n"},{"id":"406205","messageId":"xmqqwo0kietp.fsf@gitster.c.googlers.com","threadId":"54283","inReplyTo":"20200923170915.21748-1-chriscool@tuxfamily.org","subject":"Re: [PATCH] bisect: don't use invalid oid as rev when starting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-23T17:27:14Z","receivedAt":"2020-09-23T17:27:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> In 06f5608c14 (bisect--helper: `bisect_start` shell function\n> partially in C, 2019-01-02), we changed the following shell\n> code:\n\nWow, that's an ancient breakage (goes back to 2.21 or something).\n\nThanks; will queue.\n\n>\n> - rev=$(git rev-parse -q --verify \"$arg^{commit}\") || {\n> - test $has_double_dash -eq 1 &&\n> - die \"$(eval_gettext \"'\\$arg' does not appear to be a valid revision\")\"\n> - break\n> - }\n> - revs=\"$revs $rev\"\n>\n> into:\n>\n> + char *commit_id = xstrfmt(\"%s^{commit}\", arg);\n> + if (get_oid(commit_id, &oid) && has_double_dash)\n> + die(_(\"'%s' does not appear to be a valid \"\n> + \"revision\"), arg);\n> +\n> + string_list_append(&revs, oid_to_hex(&oid));\n> + free(commit_id);\n>\n> In case of an invalid \"arg\" when \"has_double_dash\" is false, the old\n> code would \"break\" out of the argument loop.\n>\n> In the new C code though, `oid_to_hex(&oid)` is unconditonally\n> appended to \"revs\". This is wrong first because \"oid\" is junk as\n> `get_oid(commit_id, &oid)` failed and second because it doesn't break\n> out of the argument loop.\n>\n> Not breaking out of the argument loop means that \"arg\" is then not\n> treated as a path restriction (which is wrong).\n>\n> Signed-off-by: Christian Couder \n> ---\n> This is a bug fix for the bug Miriam talks about in:\n>\n> https://lore.kernel.org/git/20200923072740.20772-1-mirucam@gmail.com/\n>\n> and:\n>\n> https://lore.kernel.org/git/CAN7CjDDVp_i7dhpbAq5zrGW69nE6+SfivJQ-dembmu+WyqKiQQ@mail.gmail.com/\n>\n> builtin/bisect--helper.c | 14 +++++++++-----\n> t/t6030-bisect-porcelain.sh | 7 +++++++\n> 2 files changed, 16 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 7dcc1b5188..538fa6f16b 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -486,12 +486,16 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\n> \t\t\treturn error(_(\"unrecognized option: '%s'\"), arg);\n> \t\t} else {\n> \t\t\tchar *commit_id = xstrfmt(\"%s^{commit}\", arg);\n> -\t\t\tif (get_oid(commit_id, &oid) && has_double_dash)\n> -\t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n> -\t\t\t\t \"revision\"), arg);\n> -\n> -\t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n> +\t\t\tint res = get_oid(commit_id, &oid);\n> \t\t\tfree(commit_id);\n> +\t\t\tif (res) {\n> +\t\t\t\tif (has_double_dash)\n> +\t\t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n> +\t\t\t\t\t \"revision\"), arg);\n> +\t\t\t\tbreak;\n> +\t\t\t} else {\n> +\t\t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n> +\t\t\t}\n> \t\t}\n> \t}\n> \tpathspec_pos = i;\n> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\n> index b886529e59..cb645cf8c8 100755\n> --- a/t/t6030-bisect-porcelain.sh\n> +++ b/t/t6030-bisect-porcelain.sh\n> @@ -82,6 +82,13 @@ test_expect_success 'bisect fails if given any junk instead of revs' '\n> \tgit bisect bad $HASH4\n> '\n> \n> +test_expect_success 'bisect start without -- uses unknown stuff as path restriction' '\n> +\tgit bisect reset &&\n> +\tgit bisect start foo bar &&\n> +\tgrep foo \".git/BISECT_NAMES\" &&\n> +\tgrep bar \".git/BISECT_NAMES\"\n> +'\n> +\n> test_expect_success 'bisect reset: back in the master branch' '\n> \tgit bisect reset &&\n> \techo \"* master\" > branch.expect &&\n"},{"id":"406218","messageId":"nycvar.QRO.7.76.6.2009232229000.5061@tvgsbejvaqbjf.bet","threadId":"54283","inReplyTo":"20200923170915.21748-1-chriscool@tuxfamily.org","subject":"Re: [PATCH] bisect: don't use invalid oid as rev when starting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-23T20:37:46Z","receivedAt":"2020-09-23T20:38:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Christian,\n\nOn Wed, 23 Sep 2020, Christian Couder wrote:\n\n> In 06f5608c14 (bisect--helper: `bisect_start` shell function\n> partially in C, 2019-01-02), we changed the following shell\n> code:\n>\n> - rev=$(git rev-parse -q --verify \"$arg^{commit}\") || {\n> - test $has_double_dash -eq 1 &&\n> - die \"$(eval_gettext \"'\\$arg' does not appear to be a valid revision\")\"\n> - break\n> - }\n> - revs=\"$revs $rev\"\n>\n> into:\n>\n> + char *commit_id = xstrfmt(\"%s^{commit}\", arg);\n> + if (get_oid(commit_id, &oid) && has_double_dash)\n> + die(_(\"'%s' does not appear to be a valid \"\n> + \"revision\"), arg);\n> +\n> + string_list_append(&revs, oid_to_hex(&oid));\n> + free(commit_id);\n>\n> In case of an invalid \"arg\" when \"has_double_dash\" is false, the old\n> code would \"break\" out of the argument loop.\n>\n> In the new C code though, `oid_to_hex(&oid)` is unconditonally\n> appended to \"revs\". This is wrong first because \"oid\" is junk as\n> `get_oid(commit_id, &oid)` failed and second because it doesn't break\n> out of the argument loop.\n>\n> Not breaking out of the argument loop means that \"arg\" is then not\n> treated as a path restriction (which is wrong).\n\nGood catch!\n\n> This is a bug fix for the bug Miriam talks about in:\n>\n> https://lore.kernel.org/git/20200923072740.20772-1-mirucam@gmail.com/\n>\n> and:\n>\n> https://lore.kernel.org/git/CAN7CjDDVp_i7dhpbAq5zrGW69nE6+SfivJQ-dembmu+WyqKiQQ@mail.gmail.com/\n\nThank you for clarifying.\n\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 7dcc1b5188..538fa6f16b 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -486,12 +486,16 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\n> \t\t\treturn error(_(\"unrecognized option: '%s'\"), arg);\n> \t\t} else {\n> \t\t\tchar *commit_id = xstrfmt(\"%s^{commit}\", arg);\n> -\t\t\tif (get_oid(commit_id, &oid) && has_double_dash)\n> -\t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n> -\t\t\t\t \"revision\"), arg);\n> -\n> -\t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n> +\t\t\tint res = get_oid(commit_id, &oid);\n> \t\t\tfree(commit_id);\n> +\t\t\tif (res) {\n> +\t\t\t\tif (has_double_dash)\n> +\t\t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n> +\t\t\t\t\t \"revision\"), arg);\n> +\t\t\t\tbreak;\n> +\t\t\t} else {\n> +\t\t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n> +\t\t\t}\n\nI would find that a lot easier to read if it was reordered thusly:\n\n\t\t\tif (!get_oidf(&oid, \"%s^{commit}\", arg))\n\t\t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n\t\t\telse if (!has_double_dash)\n\t\t\t\tbreak;\n\t\t\telse\n\t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n\t\t\t\t revision\"), arg);\n\nAnd it would actually probably make sense to replace the `get_oid()` by\n`get_oid_committish()` in the first place.\n\n> \t\t}\n> \t}\n> \tpathspec_pos = i;\n> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\n> index b886529e59..cb645cf8c8 100755\n> --- a/t/t6030-bisect-porcelain.sh\n> +++ b/t/t6030-bisect-porcelain.sh\n> @@ -82,6 +82,13 @@ test_expect_success 'bisect fails if given any junk instead of revs' '\n> \tgit bisect bad $HASH4\n> '\n>\n> +test_expect_success 'bisect start without -- uses unknown stuff as path restriction' '\n\ns/stuff/arg/ maybe?\n\nThe rest of the patch looks good to me.\n\nThank you,\nDscho\n\n> +\tgit bisect reset &&\n> +\tgit bisect start foo bar &&\n> +\tgrep foo \".git/BISECT_NAMES\" &&\n> +\tgrep bar \".git/BISECT_NAMES\"\n> +'\n> +\n> test_expect_success 'bisect reset: back in the master branch' '\n> \tgit bisect reset &&\n> \techo \"* master\" > branch.expect &&\n> --\n> 2.28.0.587.g1c7fdf1d8b\n>\n>\n"},{"id":"406222","messageId":"nycvar.QRO.7.76.6.2009232302050.5061@tvgsbejvaqbjf.bet","threadId":"54283","inReplyTo":"nycvar.QRO.7.76.6.2009232229000.5061@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] bisect: don't use invalid oid as rev when starting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-23T21:05:20Z","receivedAt":"2020-09-23T21:05:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Christian,\n\nOn Wed, 23 Sep 2020, Johannes Schindelin wrote:\n\n> On Wed, 23 Sep 2020, Christian Couder wrote:\n>\n> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> > index 7dcc1b5188..538fa6f16b 100644\n> > --- a/builtin/bisect--helper.c\n> > +++ b/builtin/bisect--helper.c\n> > @@ -486,12 +486,16 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\n> > \t\t\treturn error(_(\"unrecognized option: '%s'\"), arg);\n> > \t\t} else {\n> > \t\t\tchar *commit_id = xstrfmt(\"%s^{commit}\", arg);\n> > -\t\t\tif (get_oid(commit_id, &oid) && has_double_dash)\n> > -\t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n> > -\t\t\t\t \"revision\"), arg);\n> > -\n> > -\t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n> > +\t\t\tint res = get_oid(commit_id, &oid);\n> > \t\t\tfree(commit_id);\n> > +\t\t\tif (res) {\n> > +\t\t\t\tif (has_double_dash)\n> > +\t\t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n> > +\t\t\t\t\t \"revision\"), arg);\n> > +\t\t\t\tbreak;\n> > +\t\t\t} else {\n> > +\t\t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n> > +\t\t\t}\n>\n> I would find that a lot easier to read if it was reordered thusly:\n>\n> \t\t\tif (!get_oidf(&oid, \"%s^{commit}\", arg))\n> \t\t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n> \t\t\telse if (!has_double_dash)\n> \t\t\t\tbreak;\n> \t\t\telse\n> \t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n> \t\t\t\t revision\"), arg);\n>\n> And it would actually probably make sense to replace the `get_oid()` by\n> `get_oid_committish()` in the first place.\n\nI verified that this actually works, and figured out that we can save yet\nanother indentation level (as well as avoid awfully long lines):\n\n-- snipsnap --\nFrom f673cea53e046774847be918f4023430e56bf6cb Mon Sep 17 00:00:00 2001\nFrom: Christian Couder \nDate: Wed, 23 Sep 2020 19:09:15 +0200\nSubject: [PATCH] bisect: don't use invalid oid as rev when starting\n\nIn 06f5608c14 (bisect--helper: `bisect_start` shell function\npartially in C, 2019-01-02), we changed the following shell\ncode:\n\n\trev=$(git rev-parse -q --verify \"$arg^{commit}\") || {\n\t\ttest $has_double_dash -eq 1 &&\n\t\tdie \"$(eval_gettext \"'\\$arg' does not appear to be a valid revision\")\"\n\t\tbreak\n\t}\n\trevs=\"$revs $rev\"\n\ninto:\n\n\tchar *commit_id = xstrfmt(\"%s^{commit}\", arg);\n\tif (get_oid(commit_id, &oid) && has_double_dash)\n\t\tdie(_(\"'%s' does not appear to be a valid \"\n\t\t \"revision\"), arg);\n\n\tstring_list_append(&revs, oid_to_hex(&oid));\n\tfree(commit_id);\n\nIn case of an invalid \"arg\" when \"has_double_dash\" is false, the old\ncode would \"break\" out of the argument loop.\n\nIn the new C code though, `oid_to_hex(&oid)` is unconditonally\nappended to \"revs\". This is wrong first because \"oid\" is junk as\n`get_oid(commit_id, &oid)` failed and second because it doesn't break\nout of the argument loop.\n\nNot breaking out of the argument loop means that \"arg\" is then not\ntreated as a path restriction (which is wrong).\n\nSigned-off-by: Christian Couder \nSigned-off-by: Johannes Schindelin \n---\n builtin/bisect--helper.c | 17 ++++++-----------\n t/t6030-bisect-porcelain.sh | 7 +++++++\n 2 files changed, 13 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 93e855271b9..d11d4c9bbb5 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -660,18 +660,13 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\n \t\t\tterms->term_bad = xstrdup(arg);\n \t\t} else if (starts_with(arg, \"--\")) {\n \t\t\treturn error(_(\"unrecognized option: '%s'\"), arg);\n-\t\t} else {\n-\t\t\tchar *commit_id = xstrfmt(\"%s^{commit}\", arg);\n-\t\t\tif (get_oid(commit_id, &oid) && has_double_dash) {\n-\t\t\t\terror(_(\"'%s' does not appear to be a valid \"\n-\t\t\t\t\t\"revision\"), arg);\n-\t\t\t\tfree(commit_id);\n-\t\t\t\treturn BISECT_FAILED;\n-\t\t\t}\n-\n+\t\t} else if (!get_oid_committish(arg, &oid))\n \t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n-\t\t\tfree(commit_id);\n-\t\t}\n+\t\telse if (has_double_dash)\n+\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n+\t\t\t \"revision\"), arg);\n+\t\telse\n+\t\t\tbreak;\n \t}\n \tpathspec_pos = i;\n\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex b886529e596..d6b4bca482a 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -82,6 +82,13 @@ test_expect_success 'bisect fails if given any junk instead of revs' '\n \tgit bisect bad $HASH4\n '\n\n+test_expect_success 'bisect start without -- uses unknown arg as pathspec' '\n+\tgit bisect reset &&\n+\tgit bisect start foo bar &&\n+\tgrep foo \".git/BISECT_NAMES\" &&\n+\tgrep bar \".git/BISECT_NAMES\"\n+'\n+\n test_expect_success 'bisect reset: back in the master branch' '\n \tgit bisect reset &&\n \techo \"* master\" > branch.expect &&\n--\n2.28.0.windows.1.18.g5300e52e185\n\n"},{"id":"406224","messageId":"xmqqa6xgi363.fsf@gitster.c.googlers.com","threadId":"54283","inReplyTo":"nycvar.QRO.7.76.6.2009232302050.5061@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] bisect: don't use invalid oid as rev when starting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-23T21:39:00Z","receivedAt":"2020-09-23T21:39:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Christian,\n> ...\n> -- snipsnap --\n> From f673cea53e046774847be918f4023430e56bf6cb Mon Sep 17 00:00:00 2001\n> From: Christian Couder \n> Date: Wed, 23 Sep 2020 19:09:15 +0200\n> Subject: [PATCH] bisect: don't use invalid oid as rev when starting\n> ...\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 93e855271b9..d11d4c9bbb5 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n\nUnfortunately this does not apply to the broken commit or 'master'\nor anywhere else, it seems (no such blob as 93e855271b9 found at the\npath).\n\nIt is better to make it applicable at least to 'master'. Making it\nalso apply to 'maint' is optional, I would say, as the bug it fixes\nis not so critical.\n\nThanks.\n\n"},{"id":"406234","messageId":"20200924060344.15541-1-chriscool@tuxfamily.org","threadId":"54283","inReplyTo":"20200923170915.21748-1-chriscool@tuxfamily.org","subject":"[PATCH v2] bisect: don't use invalid oid as rev when starting","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-09-24T06:03:44Z","receivedAt":"2020-09-24T06:04:06Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"In 06f5608c14 (bisect--helper: `bisect_start` shell function\npartially in C, 2019-01-02), we changed the following shell\ncode:\n\n- rev=$(git rev-parse -q --verify \"$arg^{commit}\") || {\n- test $has_double_dash -eq 1 &&\n- die \"$(eval_gettext \"'\\$arg' does not appear to be a valid revision\")\"\n- break\n- }\n- revs=\"$revs $rev\"\n\ninto:\n\n+ char *commit_id = xstrfmt(\"%s^{commit}\", arg);\n+ if (get_oid(commit_id, &oid) && has_double_dash)\n+ die(_(\"'%s' does not appear to be a valid \"\n+ \"revision\"), arg);\n+\n+ string_list_append(&revs, oid_to_hex(&oid));\n+ free(commit_id);\n\nIn case of an invalid \"arg\" when \"has_double_dash\" is false, the old\ncode would \"break\" out of the argument loop.\n\nIn the new C code though, `oid_to_hex(&oid)` is unconditonally\nappended to \"revs\". This is wrong first because \"oid\" is junk as\n`get_oid(commit_id, &oid)` failed and second because it doesn't break\nout of the argument loop.\n\nNot breaking out of the argument loop means that \"arg\" is then not\ntreated as a path restriction.\n\nSigned-off-by: Christian Couder \nSigned-off-by: Johannes Schindelin \n---\nThis patch is made on top of e1cfff6765 (Sixteenth batch, 2020-09-22)\nand incorporates Dscho's suggestions.\n\nThanks to Junio and Dscho for reviewing the first version.\n\n builtin/bisect--helper.c | 14 ++++++--------\n t/t6030-bisect-porcelain.sh | 7 +++++++\n 2 files changed, 13 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 7dcc1b5188..f4762e1774 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -484,15 +484,13 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\n \t\t\tterms->term_bad = xstrdup(arg);\n \t\t} else if (starts_with(arg, \"--\")) {\n \t\t\treturn error(_(\"unrecognized option: '%s'\"), arg);\n-\t\t} else {\n-\t\t\tchar *commit_id = xstrfmt(\"%s^{commit}\", arg);\n-\t\t\tif (get_oid(commit_id, &oid) && has_double_dash)\n-\t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n-\t\t\t\t \"revision\"), arg);\n-\n+\t\t} else if (!get_oid_committish(arg, &oid))\n \t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n-\t\t\tfree(commit_id);\n-\t\t}\n+\t\telse if (has_double_dash)\n+\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n+\t\t\t \"revision\"), arg);\n+\t\telse\n+\t\t\tbreak;\n \t}\n \tpathspec_pos = i;\n \ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex b886529e59..70c39a9459 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -82,6 +82,13 @@ test_expect_success 'bisect fails if given any junk instead of revs' '\n \tgit bisect bad $HASH4\n '\n \n+test_expect_success 'bisect start without -- uses unknown arg as path restriction' '\n+\tgit bisect reset &&\n+\tgit bisect start foo bar &&\n+\tgrep foo \".git/BISECT_NAMES\" &&\n+\tgrep bar \".git/BISECT_NAMES\"\n+'\n+\n test_expect_success 'bisect reset: back in the master branch' '\n \tgit bisect reset &&\n \techo \"* master\" > branch.expect &&\n-- \n2.28.0.585.ge1cfff6765\n\n"},{"id":"406235","messageId":"CAP8UFD3mfPgedqGk0ZFp3K3r1gDA3-stbSxoEomRh4T0_qH+tg@mail.gmail.com","threadId":"54283","inReplyTo":"xmqqa6xgi363.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] bisect: don't use invalid oid as rev when starting","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-09-24T06:10:31Z","receivedAt":"2020-09-24T06:10:45Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Sep 23, 2020 at 11:39 PM Junio C Hamano wrote:\n>\n> Johannes Schindelin writes:\n>\n> > From f673cea53e046774847be918f4023430e56bf6cb Mon Sep 17 00:00:00 2001\n> > From: Christian Couder \n> > Date: Wed, 23 Sep 2020 19:09:15 +0200\n> > Subject: [PATCH] bisect: don't use invalid oid as rev when starting\n> > ...\n> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> > index 93e855271b9..d11d4c9bbb5 100644\n> > --- a/builtin/bisect--helper.c\n> > +++ b/builtin/bisect--helper.c\n>\n> Unfortunately this does not apply to the broken commit or 'master'\n> or anywhere else, it seems (no such blob as 93e855271b9 found at the\n> path).\n>\n> It is better to make it applicable at least to 'master'. Making it\n> also apply to 'maint' is optional, I would say, as the bug it fixes\n> is not so critical.\n\nSorry, I don't know what happened. It seemed to me that my branch was\nbased on master, but maybe I did something wrong.\n\nHopefully the V2 I just sent will be better anyway.\n"},{"id":"406239","messageId":"xmqqsgb7hdpw.fsf@gitster.c.googlers.com","threadId":"54283","inReplyTo":"CAP8UFD3mfPgedqGk0ZFp3K3r1gDA3-stbSxoEomRh4T0_qH+tg@mail.gmail.com","subject":"Re: [PATCH] bisect: don't use invalid oid as rev when starting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-24T06:48:43Z","receivedAt":"2020-09-24T06:48:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Wed, Sep 23, 2020 at 11:39 PM Junio C Hamano wrote:\n>>\n>> Johannes Schindelin writes:\n>>\n>> > From f673cea53e046774847be918f4023430e56bf6cb Mon Sep 17 00:00:00 2001\n>> > From: Christian Couder \n>> > Date: Wed, 23 Sep 2020 19:09:15 +0200\n>> > Subject: [PATCH] bisect: don't use invalid oid as rev when starting\n>> > ...\n>> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n>> > index 93e855271b9..d11d4c9bbb5 100644\n>> > --- a/builtin/bisect--helper.c\n>> > +++ b/builtin/bisect--helper.c\n>>\n>> Unfortunately this does not apply to the broken commit or 'master'\n>> or anywhere else, it seems (no such blob as 93e855271b9 found at the\n>> path).\n>>\n>> It is better to make it applicable at least to 'master'. Making it\n>> also apply to 'maint' is optional, I would say, as the bug it fixes\n>> is not so critical.\n>\n> Sorry, I don't know what happened. It seemed to me that my branch was\n> based on master, but maybe I did something wrong.\n\nOh, you didn't do anything wrong. What was queued was your patch\nwhich applied cleanly to maint, master and the old version that\nbrought the breakage into the codebase. I think I queued it on\nmaint-2.25 or something sufficiently old, but as I said, a patch\nthat applies to 'master' would be good enough.\n\nWhat I had trouble applying to see how it improves upon yours was\nthe one from Dscho.\n\nThanks.\n"},{"id":"406247","messageId":"nycvar.QRO.7.76.6.2009240947080.5061@tvgsbejvaqbjf.bet","threadId":"54283","inReplyTo":"20200924060344.15541-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v2] bisect: don't use invalid oid as rev when starting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-24T07:49:43Z","receivedAt":"2020-09-24T10:00:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Christian,\n\nOn Thu, 24 Sep 2020, Christian Couder wrote:\n\n> In 06f5608c14 (bisect--helper: `bisect_start` shell function\n> partially in C, 2019-01-02), we changed the following shell\n> code:\n>\n> - rev=$(git rev-parse -q --verify \"$arg^{commit}\") || {\n> - test $has_double_dash -eq 1 &&\n> - die \"$(eval_gettext \"'\\$arg' does not appear to be a valid revision\")\"\n> - break\n> - }\n> - revs=\"$revs $rev\"\n\nThese are awfully long lines. The reason is that you kept the indentation\nof the diff. But that's actually not necessary, because we do not need to\napply a diff here; This code snippet is intended purely for human\nconsumption.\n\nWhat I suggested in my adaptation of your patch was to lose the diff\nmarkers and to decrease the insane amount of indentation to just one (and\ntwo) horizontal tabs.\n\n> into:\n>\n> + char *commit_id = xstrfmt(\"%s^{commit}\", arg);\n> + if (get_oid(commit_id, &oid) && has_double_dash)\n> + die(_(\"'%s' does not appear to be a valid \"\n> + \"revision\"), arg);\n> +\n> + string_list_append(&revs, oid_to_hex(&oid));\n> + free(commit_id);\n>\n> In case of an invalid \"arg\" when \"has_double_dash\" is false, the old\n> code would \"break\" out of the argument loop.\n>\n> In the new C code though, `oid_to_hex(&oid)` is unconditonally\n> appended to \"revs\". This is wrong first because \"oid\" is junk as\n> `get_oid(commit_id, &oid)` failed and second because it doesn't break\n> out of the argument loop.\n>\n> Not breaking out of the argument loop means that \"arg\" is then not\n> treated as a path restriction.\n>\n> Signed-off-by: Christian Couder \n> Signed-off-by: Johannes Schindelin \n> ---\n> This patch is made on top of e1cfff6765 (Sixteenth batch, 2020-09-22)\n> and incorporates Dscho's suggestions.\n>\n> Thanks to Junio and Dscho for reviewing the first version.\n>\n> builtin/bisect--helper.c | 14 ++++++--------\n> t/t6030-bisect-porcelain.sh | 7 +++++++\n> 2 files changed, 13 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 7dcc1b5188..f4762e1774 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -484,15 +484,13 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\n> \t\t\tterms->term_bad = xstrdup(arg);\n> \t\t} else if (starts_with(arg, \"--\")) {\n> \t\t\treturn error(_(\"unrecognized option: '%s'\"), arg);\n> -\t\t} else {\n> -\t\t\tchar *commit_id = xstrfmt(\"%s^{commit}\", arg);\n> -\t\t\tif (get_oid(commit_id, &oid) && has_double_dash)\n> -\t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n> -\t\t\t\t \"revision\"), arg);\n> -\n> +\t\t} else if (!get_oid_committish(arg, &oid))\n> \t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n> -\t\t\tfree(commit_id);\n> -\t\t}\n> +\t\telse if (has_double_dash)\n> +\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n> +\t\t\t \"revision\"), arg);\n> +\t\telse\n> +\t\t\tbreak;\n\nThank you!\n\n> \t}\n> \tpathspec_pos = i;\n>\n> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\n> index b886529e59..70c39a9459 100755\n> --- a/t/t6030-bisect-porcelain.sh\n> +++ b/t/t6030-bisect-porcelain.sh\n> @@ -82,6 +82,13 @@ test_expect_success 'bisect fails if given any junk instead of revs' '\n> \tgit bisect bad $HASH4\n> '\n>\n> +test_expect_success 'bisect start without -- uses unknown arg as path restriction' '\n\nTo avoid the overly long line (and also to re-use existing naming\nconventions), I replaced \"path restrictions\" by \"pathspecs\" here. What do\nyou think?\n\nCiao,\nDscho\n\n> +\tgit bisect reset &&\n> +\tgit bisect start foo bar &&\n> +\tgrep foo \".git/BISECT_NAMES\" &&\n> +\tgrep bar \".git/BISECT_NAMES\"\n> +'\n> +\n> test_expect_success 'bisect reset: back in the master branch' '\n> \tgit bisect reset &&\n> \techo \"* master\" > branch.expect &&\n> --\n> 2.28.0.585.ge1cfff6765\n>\n>\n"},{"id":"406248","messageId":"nycvar.QRO.7.76.6.2009240950010.5061@tvgsbejvaqbjf.bet","threadId":"54283","inReplyTo":"CAP8UFD3mfPgedqGk0ZFp3K3r1gDA3-stbSxoEomRh4T0_qH+tg@mail.gmail.com","subject":"Re: [PATCH] bisect: don't use invalid oid as rev when starting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-24T07:51:17Z","receivedAt":"2020-09-24T10:01:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Christian & Junio,\n\nOn Thu, 24 Sep 2020, Christian Couder wrote:\n\n> On Wed, Sep 23, 2020 at 11:39 PM Junio C Hamano wrote:\n> >\n> > Johannes Schindelin writes:\n> >\n> > > From f673cea53e046774847be918f4023430e56bf6cb Mon Sep 17 00:00:00 2001\n> > > From: Christian Couder \n> > > Date: Wed, 23 Sep 2020 19:09:15 +0200\n> > > Subject: [PATCH] bisect: don't use invalid oid as rev when starting\n> > > ...\n> > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> > > index 93e855271b9..d11d4c9bbb5 100644\n> > > --- a/builtin/bisect--helper.c\n> > > +++ b/builtin/bisect--helper.c\n> >\n> > Unfortunately this does not apply to the broken commit or 'master'\n> > or anywhere else, it seems (no such blob as 93e855271b9 found at the\n> > path).\n> >\n> > It is better to make it applicable at least to 'master'. Making it\n> > also apply to 'maint' is optional, I would say, as the bug it fixes\n> > is not so critical.\n>\n> Sorry, I don't know what happened. It seemed to me that my branch was\n> based on master, but maybe I did something wrong.\n>\n> Hopefully the V2 I just sent will be better anyway.\n\nFWIW I was working off of Miriam's `git-bisect-work-part2-v8` branch at\nhttps://gitlab.com/mirucam/git.git.\n\nI'm happy with Christian's v2 (with or without the indentation fixes I\nsuggested).\n\nCiao,\nDscho\n"},{"id":"406253","messageId":"CAP8UFD3HtiAj2yiHBZOeqm-=VxVeV7mpwvFRV_mV8y-vsxgQUQ@mail.gmail.com","threadId":"54283","inReplyTo":"nycvar.QRO.7.76.6.2009240947080.5061@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2] bisect: don't use invalid oid as rev when starting","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-09-24T11:08:01Z","receivedAt":"2020-09-24T11:08:15Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Dscho,\n\nOn Thu, Sep 24, 2020 at 11:59 AM Johannes Schindelin\n wrote:\n\n> On Thu, 24 Sep 2020, Christian Couder wrote:\n\n> > - rev=$(git rev-parse -q --verify \"$arg^{commit}\") || {\n> > - test $has_double_dash -eq 1 &&\n> > - die \"$(eval_gettext \"'\\$arg' does not appear to be a valid revision\")\"\n> > - break\n> > - }\n> > - revs=\"$revs $rev\"\n>\n> These are awfully long lines. The reason is that you kept the indentation\n> of the diff. But that's actually not necessary, because we do not need to\n> apply a diff here; This code snippet is intended purely for human\n> consumption.\n>\n> What I suggested in my adaptation of your patch was to lose the diff\n> markers and to decrease the insane amount of indentation to just one (and\n> two) horizontal tabs.\n\nYeah, I didn't realize that.\n\nWhen I am sent some code or patch like this, I often hesitate between:\n\n- using it verbatim, which can create issues as it makes me more\nlikely to overlook something in the case the sender didn't fully check\neverything\n- looking at the differences with the existing code/patch and applying\nthem one by one, which has the risk of missing or forgetting a\ndifference\n\nI guess the best would be to do both and then check the differences\nbetween the 2 results, but it feels like twice the amount of work for\nthis step.\n\n> > diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\n> > index b886529e59..70c39a9459 100755\n> > --- a/t/t6030-bisect-porcelain.sh\n> > +++ b/t/t6030-bisect-porcelain.sh\n> > @@ -82,6 +82,13 @@ test_expect_success 'bisect fails if given any junk instead of revs' '\n> > git bisect bad $HASH4\n> > '\n> >\n> > +test_expect_success 'bisect start without -- uses unknown arg as path restriction' '\n>\n> To avoid the overly long line (and also to re-use existing naming\n> conventions), I replaced \"path restrictions\" by \"pathspecs\" here. What do\n> you think?\n\nIt's not a huge issue, but I tend to prefer using \"restrictions\"\nbecause the tests that check that these arguments are used properly\nare called \"restricting bisection on one dir\" and \"restricting\nbisection on one dir and a file\". So I feel that it keeps test names\nmore coherent.\n\nBest,\nChristian.\n"},{"id":"406269","messageId":"xmqqo8lvgmdw.fsf@gitster.c.googlers.com","threadId":"54283","inReplyTo":"nycvar.QRO.7.76.6.2009240950010.5061@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] bisect: don't use invalid oid as rev when starting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-24T16:39:07Z","receivedAt":"2020-09-24T16:39:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Hopefully the V2 I just sent will be better anyway.\n>\n> FWIW I was working off of Miriam's `git-bisect-work-part2-v8` branch at\n> https://gitlab.com/mirucam/git.git.\n>\n> I'm happy with Christian's v2 (with or without the indentation fixes I\n> suggested).\n\nThanks, both of you. The updated one does look good.\n\n\n"},{"id":"406270","messageId":"xmqqk0wjgm51.fsf@gitster.c.googlers.com","threadId":"54283","inReplyTo":"CAP8UFD3HtiAj2yiHBZOeqm-=VxVeV7mpwvFRV_mV8y-vsxgQUQ@mail.gmail.com","subject":"Re: [PATCH v2] bisect: don't use invalid oid as rev when starting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-24T16:44:26Z","receivedAt":"2020-09-24T16:44:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n>> > +test_expect_success 'bisect start without -- uses unknown arg as path restriction' '\n>>\n>> To avoid the overly long line (and also to re-use existing naming\n>> conventions), I replaced \"path restrictions\" by \"pathspecs\" here. What do\n>> you think?\n>\n> It's not a huge issue, but I tend to prefer using \"restrictions\"\n> because the tests that check that these arguments are used properly\n> are called \"restricting bisection on one dir\" and \"restricting\n> bisection on one dir and a file\". So I feel that it keeps test names\n> more coherent.\n\nHmph, but in the context of a sentence \"use an arg as X\", we should\ntry to pick a well-known word to describe various Git arguments for\nX, no? The one you are using, in order to filter the set of commits\ninvolved in the operation to those that touch specific set of paths,\nalready has its own established name and the word is \"pathspec\".\n\nSo...?\n\n\n\n"},{"id":"406276","messageId":"xmqqwo0jm356.fsf@gitster.c.googlers.com","threadId":"54283","inReplyTo":"xmqqo8lvgmdw.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] bisect: don't use invalid oid as rev when starting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-24T18:38:13Z","receivedAt":"2020-09-24T18:38:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>>> Hopefully the V2 I just sent will be better anyway.\n>>\n>> FWIW I was working off of Miriam's `git-bisect-work-part2-v8` branch at\n>> https://gitlab.com/mirucam/git.git.\n>>\n>> I'm happy with Christian's v2 (with or without the indentation fixes I\n>> suggested).\n>\n> Thanks, both of you. The updated one does look good.\n\nOops, do you mean s/path restriction/pathspec/ fix? v2 looks OK nesting-wise\nand I think your indentex-fix suggestion was for the previous one.\n\nJust making sure...\n\nThanks.\n"},{"id":"406277","messageId":"xmqqsgb7m2bq.fsf@gitster.c.googlers.com","threadId":"54283","inReplyTo":"20200924060344.15541-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v2] bisect: don't use invalid oid as rev when starting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-24T18:55:53Z","receivedAt":"2020-09-24T18:56:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> -\t\t} else {\n> -\t\t\tchar *commit_id = xstrfmt(\"%s^{commit}\", arg);\n> -\t\t\tif (get_oid(commit_id, &oid) && has_double_dash)\n> -\t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n> -\t\t\t\t \"revision\"), arg);\n> -\n> +\t\t} else if (!get_oid_committish(arg, &oid))\n\nThis is wrong, I think. The \"_committish\" only applies to the fact\nthat the disambiguation includes only the objects that are\ncommittishes, and the result are not peeled---you'll get an\nannotated tag back in oid, if you give it an annotated tag that\npoints at a commit.\n\nThis is not what ^{commit} does.\n\nIt may happen to work if the downstream consumers peel objects\n(which are appended to the 'revs' here) down to commit when they are\nused, and if somebody verified that is indeed the case, it would be\nOK, but if this patch is presented as \"unlike the previous broken\none, this is the faithful conversion of the original in shell, so\nthere is no need to verify anything outside of the patch context\",\nthat is a problem.\n\nWe may need to audit recent additions of get_oid_committish() calls\nin our codebase. I suspect there may be other instances of the same\nmistake.\n\nOther than that, the code structure does look more straight-forward\ncompared to the previous round. A fix on this version would involve\npeeling what is in oid down to commit, and you need to handle errors\nduring that process, so it may not stay pretty with a fix, though.\nI dunno.\n\nThanks.\n\n> \t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n> -\t\t\tfree(commit_id);\n> -\t\t}\n> +\t\telse if (has_double_dash)\n> +\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n> +\t\t\t \"revision\"), arg);\n> +\t\telse\n> +\t\t\tbreak;\n> \t}\n> \tpathspec_pos = i;\n> \n> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\n> index b886529e59..70c39a9459 100755\n> --- a/t/t6030-bisect-porcelain.sh\n> +++ b/t/t6030-bisect-porcelain.sh\n> @@ -82,6 +82,13 @@ test_expect_success 'bisect fails if given any junk instead of revs' '\n> \tgit bisect bad $HASH4\n> '\n> \n> +test_expect_success 'bisect start without -- uses unknown arg as path restriction' '\n> +\tgit bisect reset &&\n> +\tgit bisect start foo bar &&\n> +\tgrep foo \".git/BISECT_NAMES\" &&\n> +\tgrep bar \".git/BISECT_NAMES\"\n> +'\n> +\n> test_expect_success 'bisect reset: back in the master branch' '\n> \tgit bisect reset &&\n> \techo \"* master\" > branch.expect &&\n"},{"id":"406281","messageId":"xmqqo8lvm0y3.fsf@gitster.c.googlers.com","threadId":"54283","inReplyTo":"xmqqsgb7m2bq.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] bisect: don't use invalid oid as rev when starting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-24T19:25:40Z","receivedAt":"2020-09-24T19:25:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> We may need to audit recent additions of get_oid_committish() calls\n> in our codebase. I suspect there may be other instances of the same\n> mistake.\n>\n> Other than that, the code structure does look more straight-forward\n> compared to the previous round. A fix on this version would involve\n> peeling what is in oid down to commit, and you need to handle errors\n> during that process, so it may not stay pretty with a fix, though.\n> I dunno.\n\nI'll queue it with this band-aid on top for now.\n\nThanks.\n\n builtin/bisect--helper.c | 7 ++++---\n t/t6030-bisect-porcelain.sh | 2 +-\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex a1f97e3f6c..2fcc023a3b 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -474,13 +474,14 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t} else if (starts_with(arg, \"--\") &&\n \t\t\t !one_of(arg, \"--term-good\", \"--term-bad\", NULL)) {\n \t\t\treturn error(_(\"unrecognized option: '%s'\"), arg);\n-\t\t} else if (!get_oid_committish(arg, &oid))\n+\t\t} else if (!get_oidf(&oid, \"%s^{commit}\", arg)) {\n \t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n-\t\telse if (has_double_dash)\n+\t\t} else if (has_double_dash) {\n \t\t\tdie(_(\"'%s' does not appear to be a valid \"\n \t\t\t \"revision\"), arg);\n-\t\telse\n+\t\t} else {\n \t\t\tbreak;\n+\t\t}\n \t}\n \tpathspec_pos = i;\n \ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 94179b6acf..4dbfa63ca1 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -82,7 +82,7 @@ test_expect_success 'bisect fails if given any junk instead of revs' '\n \tgit bisect bad $HASH4\n '\n \n-test_expect_success 'bisect start without -- uses unknown arg as path restriction' '\n+test_expect_success 'bisect start without -- takes unknown arg as pathspec' '\n \tgit bisect reset &&\n \tgit bisect start foo bar &&\n \tgrep foo \".git/BISECT_NAMES\" &&\n"},{"id":"406282","messageId":"xmqqk0wjlzj8.fsf@gitster.c.googlers.com","threadId":"54283","inReplyTo":"xmqqsgb7m2bq.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] bisect: don't use invalid oid as rev when starting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-24T19:56:11Z","receivedAt":"2020-09-24T19:56:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> We may need to audit recent additions of get_oid_committish() calls\n> in our codebase. I suspect there may be other instances of the same\n> mistake.\n\nInterim progress report. I've only looked at files that use\nget_oid_treeish() but audited all uses of get_oid_*ish() in them.\n\nThe results are as follows.\n\n * builtin/reset.c::parse_args() makes get_oid_committish() and\n get_oid_treeish() only to discard the object name, because it\n wants to ensure the args can be peeled down to such. OK.\n\n * builtin/reset.c::cmd_reset() applies get_oid_committish() and\n get_oid_treeish() to the result taken from the above, but then\n uses lookup_commit_reference() and parse_tree_indirect() to peel\n the result to the desired type. OK.\n\n * notes.c::init_notes() uses get_oid_treeish() to validate that the\n notes ref can be read as a tree, and then uses get_tree_entry()\n on it, which in turn uses read_object_with_reference() for\n tree_type so it tolerates a commit object. OK.\n\n\nI didn't audit the following hits of get_oid_committish(). There\nmight be a similar mistake as you made in v2, or there may not be.\n\nI am undecided if I should just move on, marking them as\nleft-over-bits ;-)\n\n\n\nbuiltin/blame.c:\t\tif (get_oid_committish(i->string, &oid))\nbuiltin/checkout.c:\t\trepo_get_oid_committish(the_repository, branch->name, &branch->oid);\nbuiltin/rev-parse.c:\tif (!get_oid_committish(start, &start_oid) && !get_oid_committish(end, &end_oid)) {\nbuiltin/rev-parse.c:\tif (get_oid_committish(arg, &oid) ||\ncommit.c:\tif (get_oid_committish(name, &oid))\nrevision.c:\tif (get_oid_committish(arg, &oid))\nsequencer.c:\t\t !get_oid_committish(buf.buf, &oid))\nsha1-name.c:\t\tst = repo_get_oid_committish(r, sb.buf, &oid_tmp);\nsha1-name.c:\tif (repo_get_oid_committish(r, dots[3] ? (dots + 3) : \"HEAD\", &oid_tmp))\nsha1-name.c:int repo_get_oid_committish(struct repository *r,\nt/helper/test-reach.c:\t\tif (get_oid_committish(buf.buf + 2, &oid))\n\n"},{"id":"406283","messageId":"xmqqd02anbfl.fsf@gitster.c.googlers.com","threadId":"54283","inReplyTo":"xmqqk0wjlzj8.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] bisect: don't use invalid oid as rev when starting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-24T20:53:50Z","receivedAt":"2020-09-24T20:53:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I didn't audit the following hits of get_oid_committish(). There\n> might be a similar mistake as you made in v2, or there may not be.\n>\n> I am undecided if I should just move on, marking them as\n> left-over-bits ;-)\n>\n>\n>\n> builtin/blame.c:\t\tif (get_oid_committish(i->string, &oid))\n\nThis one throws the object name of revs to be skipped to a list, and\nbecause revision traversal works on commit objects, if the user\ngives an annotated tag and expects the underlying commit is ignored,\nit may appear as a bug. But in the same function a list of revs to\nbe ignored is read from file using oidset_parse_file() that in turn\nuses parse_oid_hex() without even validating if the named object\nexists, I would say it is OK---after all, if it hurts, the user can\nrefrain from doing so ;-)\n\nBut it would be nice to fix all issues around this caller. After\ncollecting the object names to an oidset, somebody should go through\nthe list, peel them down to commit and make sure they exist, or\nsomething like that. A possible #leftoverbits.\n\n> builtin/checkout.c:\t\trepo_get_oid_committish(the_repository, branch->name, &branch->oid);\n\nThis one is probably OK as branch refs are supposed to point at\ncommits and not annotated tags that point at commits.\n\n> builtin/rev-parse.c:\tif (!get_oid_committish(start, &start_oid) && !get_oid_committish(end, &end_oid)) {\n\nThis one handles \"rev-parse v1.0..v2.0\" which gives \"^v1.0 v2.0\" but\nusing the (unpeeled) object name. It is fine and should not be\nchanged to auto-peel.\n\n> builtin/rev-parse.c:\tif (get_oid_committish(arg, &oid) ||\n\nThis is immediately followed by lookup_commit_reference() to peel as\nneeded. OK.\n\n> commit.c:\tif (get_oid_committish(name, &oid))\n\nThis is part of lookup_commit_reference_by_name(), which peels and\nparses it down to an in-core commit object instance. OK.\n\n\n> revision.c:\tif (get_oid_committish(arg, &oid))\n\nThis is followed by a loop to peel it as needed. OK.\n\n> sequencer.c:\t\t !get_oid_committish(buf.buf, &oid))\n\nThis feeds the contents of rebase-merge/stopped-sha file. I presume\nthat the contents of this file (which is not directly shown to the\nend users) is always a commit object name, so this is OK. Use of\n_committish() may probably be overkill for this internal bookkeeping\nfile. If we stop make_patch() from shortening then probably we can\nchange it to parse_oid_hex() to expect and read the full object\nname.\n\n> sha1-name.c:\t\tst = repo_get_oid_committish(r, sb.buf, &oid_tmp);\n> sha1-name.c:\tif (repo_get_oid_committish(r, dots[3] ? (dots + 3) : \"HEAD\", &oid_tmp))\n\nSince I know those who wrote this old part of the codebase knew what\nthey were doing, I do not have to comment, but these are fine. They\nare all peeled to commit as appropriate by calling\nlookup_commit_reference_gently() before feeding the result to\nget_merge_bases().\n\n> sha1-name.c:int repo_get_oid_committish(struct repository *r,\n\nThis is the implementation ;-)\n\n> t/helper/test-reach.c:\t\tif (get_oid_committish(buf.buf + 2, &oid))\n\nThis peels afterwards, so it is OK.\n\n\nThe true reason I went through all the callers was to see if _all_\nthe callers want to either ignore the resulting object name (i.e.\nthey want to make sure that the arg given can be peeled down to an\nappropriate type) or wants the object name to be peeled to the type.\nIf that were the case (and from the above, it clearly isn't), we\ncould change the semantics of get_oid_*ish() so that the resulting\noid is already peeled down to the wanted type and that could simplify\nthe current callers that are peeling the result themselves.\n\nBut because some callers do not want to see the result peeled, we\nshouldn't touch what get_oid_*ish() functions do.\n\n\n"},{"id":"406320","messageId":"nycvar.QRO.7.76.6.2009250910200.5061@tvgsbejvaqbjf.bet","threadId":"54283","inReplyTo":"xmqqwo0jm356.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] bisect: don't use invalid oid as rev when starting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-25T07:13:44Z","receivedAt":"2020-09-25T11:21:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 24 Sep 2020, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n> > Johannes Schindelin writes:\n> >\n> >>> Hopefully the V2 I just sent will be better anyway.\n> >>\n> >> FWIW I was working off of Miriam's `git-bisect-work-part2-v8` branch at\n> >> https://gitlab.com/mirucam/git.git.\n> >>\n> >> I'm happy with Christian's v2 (with or without the indentation fixes I\n> >> suggested).\n> >\n> > Thanks, both of you. The updated one does look good.\n>\n> Oops, do you mean s/path restriction/pathspec/ fix? v2 looks OK nesting-wise\n> and I think your indentex-fix suggestion was for the previous one.\n\nI was referring to the indentation of the -/+ lines in the commit message.\nYour current `SQUASH???` does not include the line-shortening, but that's\nokay. I slightly prefer the version where single conditional statements\nlose the curly brackets, but that's just nit-picking.\n\nA slightly bigger question is whether `git_oid_committish()` would be okay\nenough, or whether we do need `get_oidf(&oid, \"%s^{commit}\", arg)` (as\nyour `SQUASH???` does).\n\nI'm not quite sure: aren't those two calls idempotent, with the latter\ngoing through some unnecessary string processing dances?\n\nCiao,\nDscho\n"},{"id":"406321","messageId":"nycvar.QRO.7.76.6.2009250914140.5061@tvgsbejvaqbjf.bet","threadId":"54283","inReplyTo":"nycvar.QRO.7.76.6.2009250910200.5061@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] bisect: don't use invalid oid as rev when starting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-25T07:14:35Z","receivedAt":"2020-09-25T11:22:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 25 Sep 2020, Johannes Schindelin wrote:\n\n> On Thu, 24 Sep 2020, Junio C Hamano wrote:\n>\n> > Junio C Hamano writes:\n> >\n> > > Johannes Schindelin writes:\n> > >\n> > >>> Hopefully the V2 I just sent will be better anyway.\n> > >>\n> > >> FWIW I was working off of Miriam's `git-bisect-work-part2-v8` branch at\n> > >> https://gitlab.com/mirucam/git.git.\n> > >>\n> > >> I'm happy with Christian's v2 (with or without the indentation fixes I\n> > >> suggested).\n> > >\n> > > Thanks, both of you. The updated one does look good.\n> >\n> > Oops, do you mean s/path restriction/pathspec/ fix? v2 looks OK nesting-wise\n> > and I think your indentex-fix suggestion was for the previous one.\n>\n> I was referring to the indentation of the -/+ lines in the commit message.\n> Your current `SQUASH???` does not include the line-shortening, but that's\n> okay. I slightly prefer the version where single conditional statements\n> lose the curly brackets, but that's just nit-picking.\n>\n> A slightly bigger question is whether `git_oid_committish()` would be okay\n> enough, or whether we do need `get_oidf(&oid, \"%s^{commit}\", arg)` (as\n> your `SQUASH???` does).\n>\n> I'm not quite sure: aren't those two calls idempotent, with the latter\n> going through some unnecessary string processing dances?\n\nWhoops. You explained that elsewhere in the thread. My bad. Ignore me.\n\nCiao,\nDscho\n"},{"id":"406331","messageId":"20200925130128.5589-1-chriscool@tuxfamily.org","threadId":"54283","inReplyTo":"20200924060344.15541-1-chriscool@tuxfamily.org","subject":"[PATCH v3] bisect: don't use invalid oid as rev when starting","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-09-25T13:01:28Z","receivedAt":"2020-09-25T13:01:55Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"In 06f5608c14 (bisect--helper: `bisect_start` shell function\npartially in C, 2019-01-02), we changed the following shell\ncode:\n\n- rev=$(git rev-parse -q --verify \"$arg^{commit}\") || {\n- test $has_double_dash -eq 1 &&\n- die \"$(eval_gettext \"'\\$arg' does not appear to be a valid revision\")\"\n- break\n- }\n- revs=\"$revs $rev\"\n\ninto:\n\n+ char *commit_id = xstrfmt(\"%s^{commit}\", arg);\n+ if (get_oid(commit_id, &oid) && has_double_dash)\n+ die(_(\"'%s' does not appear to be a valid \"\n+ \"revision\"), arg);\n+\n+ string_list_append(&revs, oid_to_hex(&oid));\n+ free(commit_id);\n\nIn case of an invalid \"arg\" when \"has_double_dash\" is false, the old\ncode would \"break\" out of the argument loop.\n\nIn the new C code though, `oid_to_hex(&oid)` is unconditonally\nappended to \"revs\". This is wrong first because \"oid\" is junk as\n`get_oid(commit_id, &oid)` failed and second because it doesn't break\nout of the argument loop.\n\nNot breaking out of the argument loop means that \"arg\" is then not\ntreated as a path restriction (which is wrong).\n\nSigned-off-by: Christian Couder \nSigned-off-by: Johannes Schindelin \n---\nCompared to V2, this applies Junio's `SQUASH???` commit and improves\nindentation in the commit message.\n\n builtin/bisect--helper.c | 13 ++++++-------\n t/t6030-bisect-porcelain.sh | 7 +++++++\n 2 files changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 7dcc1b5188..2f8ef0ea30 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -484,14 +484,13 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\n \t\t\tterms->term_bad = xstrdup(arg);\n \t\t} else if (starts_with(arg, \"--\")) {\n \t\t\treturn error(_(\"unrecognized option: '%s'\"), arg);\n-\t\t} else {\n-\t\t\tchar *commit_id = xstrfmt(\"%s^{commit}\", arg);\n-\t\t\tif (get_oid(commit_id, &oid) && has_double_dash)\n-\t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n-\t\t\t\t \"revision\"), arg);\n-\n+\t\t} else if (!get_oidf(&oid, \"%s^{commit}\", arg)) {\n \t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n-\t\t\tfree(commit_id);\n+\t\t} else if (has_double_dash) {\n+\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n+\t\t\t \"revision\"), arg);\n+\t\t} else {\n+\t\t\tbreak;\n \t\t}\n \t}\n \tpathspec_pos = i;\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex b886529e59..aa226381be 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -82,6 +82,13 @@ test_expect_success 'bisect fails if given any junk instead of revs' '\n \tgit bisect bad $HASH4\n '\n \n+test_expect_success 'bisect start without -- takes unknown arg as pathspec' '\n+\tgit bisect reset &&\n+\tgit bisect start foo bar &&\n+\tgrep foo \".git/BISECT_NAMES\" &&\n+\tgrep bar \".git/BISECT_NAMES\"\n+'\n+\n test_expect_success 'bisect reset: back in the master branch' '\n \tgit bisect reset &&\n \techo \"* master\" > branch.expect &&\n-- \n2.28.0.585.ge1cfff6765\n\n"},{"id":"406332","messageId":"CAP8UFD1zLeeWsmPvwrs-ypZkwirxcEmasqW5mSxbFeqF6-Fntw@mail.gmail.com","threadId":"54283","inReplyTo":"xmqqsgb7m2bq.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] bisect: don't use invalid oid as rev when starting","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-09-25T13:09:23Z","receivedAt":"2020-09-25T13:09:39Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Sep 24, 2020 at 8:55 PM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > - } else {\n> > - char *commit_id = xstrfmt(\"%s^{commit}\", arg);\n> > - if (get_oid(commit_id, &oid) && has_double_dash)\n> > - die(_(\"'%s' does not appear to be a valid \"\n> > - \"revision\"), arg);\n> > -\n> > + } else if (!get_oid_committish(arg, &oid))\n>\n> This is wrong, I think. The \"_committish\" only applies to the fact\n> that the disambiguation includes only the objects that are\n> committishes, and the result are not peeled---you'll get an\n> annotated tag back in oid, if you give it an annotated tag that\n> points at a commit.\n>\n> This is not what ^{commit} does.\n\nThanks for finding this.\n\n> It may happen to work if the downstream consumers peel objects\n> (which are appended to the 'revs' here) down to commit when they are\n> used, and if somebody verified that is indeed the case, it would be\n> OK, but if this patch is presented as \"unlike the previous broken\n> one, this is the faithful conversion of the original in shell, so\n> there is no need to verify anything outside of the patch context\",\n> that is a problem.\n\nI agree that it's better if there is no need to verify anything\noutside of the patch context. So I agree with your fix.\n\nI am also ok with using \"pathspec\" in the test description of the new test.\n\nThanks,\nChristian.\n"},{"id":"406349","messageId":"xmqqwo0hlrus.fsf@gitster.c.googlers.com","threadId":"54283","inReplyTo":"nycvar.QRO.7.76.6.2009250910200.5061@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] bisect: don't use invalid oid as rev when starting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-25T16:54:19Z","receivedAt":"2020-09-25T16:54:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> A slightly bigger question is whether `git_oid_committish()` would be okay\n> enough, or whether we do need `get_oidf(&oid, \"%s^{commit}\", arg)` (as\n> your `SQUASH???` does).\n>\n> I'm not quite sure: aren't those two calls idempotent, with the latter\n> going through some unnecessary string processing dances?\n\nI tend to agree that get_oidf() is not quite satisfactory but it is\na handy short-hand that is readily available to us to parse a string\ninto the name of the object of type commit the string peels into.\n\nWhile get_oid_X_ish() makes sure the result can be peeled to type X,\nit gives the outer object back to the caller to allow it to behave\ndifferently and it is up to the caller to peel the tag down to\ncommit.\n\nI audited all uses of get_oid_X_ish(), in the hope that perhaps we\nhave enough callers that want to get peeled object back, and more\nimportantly, no caller that wants the original. Then we could\nchange these to peel for the callers, and we may be able to lose a\nfew lines from the callers.\n\nBut unfortunately that was not the case. A new helper\n\n\tint oid_of_type(struct object_id *result_oid,\n\t\t\tconst char *name_text,\n\t\t\tenum object_type peel_to);\n\nis a possibility, but I have a suspicion that the API in question\nencourages to manipulate and swap objects at their hash level for\ntoo long with little upside. If we were considering to clean the\ncallers up, we would want to encourage them to work with in-core\nobjects longer.\n\nIOW, repo_peel_to_type() might be a better fit for some callers that\nuse get_oid_X_ish() and then peel the result to obtain an object of\ndesired type.\n\nThanks.\n"}]}