{"thread":{"id":"57828","subject":"[PATCH] bisect--helper: warn if we are assuming an unlikely pathspec","startedAt":"2022-05-01T12:29:22Z","lastAt":"2022-05-03T18:51:49Z","messageCount":4,"participants":["Chris Down","Bagas Sanjaya","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"454697","messageId":"Ym59GmfWpCSV9Bqr@chrisdown.name","threadId":"57828","inReplyTo":null,"subject":"[PATCH] bisect--helper: warn if we are assuming an unlikely pathspec","fromName":"Chris Down","fromEmail":"chris@chrisdown.name","sentAt":"2022-05-01T12:29:14Z","receivedAt":"2022-05-01T12:29:22Z","isPatch":true,"sender":{"key":"chris@chrisdown.name","avatar":"https://avatars.githubusercontent.com/u/660663?v=4"},"body":"Commit 73c6de06aff8 (\"bisect: don't use invalid oid as rev when\nstarting\") changes the behaviour to consider invalid oids as pathspecs\nagain, as in the old shell implementation.\n\nWhile that behaviour may be desirable, it can also cause confusion. For\nexample, while bisecting in a particular repo I encountered this:\n\n    $ git bisect start d93ff48803f0 v6.3\n    $\n\n...which led to me sitting for a few moments, wondering why there's no\nprintout stating the first rev to check.\n\nIt turns out that the tag was actually \"6.3\", not \"v6.3\", and thus the\nbisect was still silently started with only a bad rev, because\nd93ff48803f0 was a valid oid and \"v6.3\" was silently considered to be a\npathspec.\n\nWhile this behaviour may be desirable, it can be confusing, especially\nwith different repo conventions either using or not using \"v\" before\nrelease names, or when a branch name or tag is simply misspelled on the\ncommand line.\n\nIn order to avoid this, emit a warning when we are assuming an argument\nis a pathspec, but no such path exists in the checkout and this doesn't\nlook like a pathspec according to looks_like_pathspec:\n\n    $ git bisect start d93ff48803f0 v6.3\n    warning: assuming 'v6.3' is a path\n    $\n\nIf the filename is after the double dash, assume the user knows what\nthey're doing, just like with other git commands.\n\nSigned-off-by: Chris Down <chris@chrisdown.name>\n---\n builtin/bisect--helper.c    |  4 ++++\n cache.h                     |  1 +\n setup.c                     |  2 +-\n t/t6030-bisect-porcelain.sh | 18 ++++++++++++++++++\n 4 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 8b2b259ff0..30a73e2a7d 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -682,6 +682,10 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\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\tif (!has_double_dash &&\n+\t\t\t    !looks_like_pathspec(arg) &&\n+\t\t\t    !check_filename(NULL, arg))\n+\t\t\t\twarning(_(\"assuming '%s' is a path\"), arg);\n \t\t\tbreak;\n \t\t}\n \t}\ndiff --git a/cache.h b/cache.h\nindex 6226f6a8a5..bae92859a3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -648,6 +648,7 @@ void verify_filename(const char *prefix,\n \t\t     int diagnose_misspelt_rev);\n void verify_non_filename(const char *prefix, const char *name);\n int path_inside_repo(const char *prefix, const char *path);\n+int looks_like_pathspec(const char *arg);\n \n #define INIT_DB_QUIET 0x0001\n #define INIT_DB_EXIST_OK 0x0002\ndiff --git a/setup.c b/setup.c\nindex a7b36f3ffb..bafbfb0d5e 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -208,7 +208,7 @@ static void NORETURN die_verify_filename(struct repository *r,\n  * but which look sufficiently like pathspecs that we'll consider\n  * them such for the purposes of rev/pathspec DWIM parsing.\n  */\n-static int looks_like_pathspec(const char *arg)\n+int looks_like_pathspec(const char *arg)\n {\n \tconst char *p;\n \tint escaped = 0;\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 5382e5d216..2ea50f4ba4 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -1025,4 +1025,22 @@ test_expect_success 'bisect visualize with a filename with dash and space' '\n \tgit bisect visualize -p -- \"-hello 2\"\n '\n \n+test_expect_success 'bisect warning on implicit enoent pathspec' '\n+\tgit bisect reset &&\n+\tgit bisect start \"$HASH4\" doesnotexist 2>output &&\n+\tgrep -F \"assuming '\\''doesnotexist'\\'' is a path\" output\n+'\n+\n+test_expect_success 'bisect fatal on implicit enoent pathspec with dash' '\n+\tgit bisect reset &&\n+\ttest_must_fail git bisect start \"$HASH4\" doesnotexist -- dne2 2>output &&\n+\tgrep -F \"'\\''doesnotexist'\\'' does not appear to be a valid revision\" output\n+'\n+\n+test_expect_success 'bisect no warning on explicit enoent pathspec' '\n+\tgit bisect reset &&\n+\tgit bisect start \"$HASH4\" -- doesnotexist 2>output &&\n+\t[ -z \"$(cat output)\" ]\n+'\n+\n test_done\n-- \n2.36.0\n\n"},{"id":"454706","messageId":"48cd2ce4-feac-b028-daad-2df6aa71e363@gmail.com","threadId":"57828","inReplyTo":"Ym59GmfWpCSV9Bqr@chrisdown.name","subject":"Re: [PATCH] bisect--helper: warn if we are assuming an unlikely pathspec","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2022-05-02T05:50:57Z","receivedAt":"2022-05-02T05:51:05Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 5/1/22 19:29, Chris Down wrote:\n> In order to avoid this, emit a warning when we are assuming an argument\n> is a pathspec, but no such path exists in the checkout and this doesn't\n> look like a pathspec according to looks_like_pathspec:\n> \n>     $ git bisect start d93ff48803f0 v6.3\n>     warning: assuming 'v6.3' is a path\n>     $\n> \n\nFor completeness, we can say 'If this is incorrect, abort the current\nbisection with \"git bisect reset\" and rerun with correct commit-ish.'.\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"454707","messageId":"xmqqv8uo1mk6.fsf@gitster.g","threadId":"57828","inReplyTo":"48cd2ce4-feac-b028-daad-2df6aa71e363@gmail.com","subject":"Re: [PATCH] bisect--helper: warn if we are assuming an unlikely pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-02T06:22:01Z","receivedAt":"2022-05-02T06:22:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bagas Sanjaya <bagasdotme@gmail.com> writes:\n\n> On 5/1/22 19:29, Chris Down wrote:\n>> In order to avoid this, emit a warning when we are assuming an argument\n>> is a pathspec, but no such path exists in the checkout and this doesn't\n>> look like a pathspec according to looks_like_pathspec:\n>> \n>>     $ git bisect start d93ff48803f0 v6.3\n>>     warning: assuming 'v6.3' is a path\n>>     $\n>> \n>\n> For completeness, we can say 'If this is incorrect, abort the current\n> bisection with \"git bisect reset\" and rerun with correct commit-ish.'.\n\nWe certainly can, but I am not sure how much it helps.\n\nEven though I understand that the original end-user confusion\nhappened to come from taking what the user thought was a revision as\npathspec, I think it is a mistake to build the \"warning\" UI around\nit.\n\nStepping back a bit, there are \"git bisect\" invocations that compute\nand check out the commit to be tested, and there are \"git bisect\"\ninvocations that only advance the internal state a bit but not yet\nbecome ready to offer the first commit to be tested.  The problem in\nthe current UI is that we are fairly chatty once we start bisecting,\nbut until we receive enough information to start bisecting, we are\nfairly silent.\n\nEven if the user did not use the lazy short-hand form of \"bisect\nstart\" with bad and good commits at all, after saying \"git bisect\ngood <X>\" (read: it is a state that made a bit of progress but still\nnot ready, because the command wants to see at least one bad commit,\ntoo), wouldn't it be nice if the user is told what state the command\nis in?  Perhaps we can give some feedback _before_ we are ready to\ncompute bisection?\n\nA model dialogue may go like this.\n\n    $ git bisect start -- bin/\n    info: bisect waiting for good and bad commits.\n    $ git bisect good master\n    info: bisect waiting for a bad commit, one good commit known.\n    $ git bisect good maint\n    info: bisect waiting for a bad commit, two good commits known.\n    $ git bisect bad next\n    Bisecting: ...\n\nThen the exchange for the lazy short-hand form of \"bisect start\"\nwould fall out quite naturally.\n\n    $ git bisect start d93ff48803f0 -- v6.3\n    info: bisect waiting for a good commit, one bad commit known.\n\nFor a bonus point, we may want to also say something on these\n\"info:\" lines that we were given a pathspec.\n\nIt would also be a good idea to add a new subcommand \"git bisect status\"\nto recompute the state (i.e. what it is waiting for and what it\nalready knows) when the user forgets, which can happen quite often.\n\nWith such a bonus feature, the exchange might go like this:\n\n    $ git bisect start seen\n    info: bisect waiting for a good commit, a bad commit known.\n    $ git reset --hard maint ;# choose an older point, hoping it is good.\n    $ make test\n    ... pages of output scrolls the \"info:\" out of window ...\n    $ git bisect status\n    info: bisect waiting for a good commit, a bad commit known.\n    $ git bisect bad maint\n    info: bisect waiting for a good commit, a bad commit known.\n    $ git reset --hard v1.0 ;# an even older point, hoping it is good.\n    $ make test\n    ... again pages of output ...\n    $ git bisect good v1.0\n    Bisecting ...\n\nHmm?\n"},{"id":"454797","messageId":"YnF5vpcBCJftFdH+@chrisdown.name","threadId":"57828","inReplyTo":"xmqqv8uo1mk6.fsf@gitster.g","subject":"Re: [PATCH] bisect--helper: warn if we are assuming an unlikely pathspec","fromName":"Chris Down","fromEmail":"chris@chrisdown.name","sentAt":"2022-05-03T18:51:42Z","receivedAt":"2022-05-03T18:51:49Z","isPatch":true,"sender":{"key":"chris@chrisdown.name","avatar":"https://avatars.githubusercontent.com/u/660663?v=4"},"body":"Junio C Hamano writes:\n>A model dialogue may go like this.\n>\n>    $ git bisect start -- bin/\n>    info: bisect waiting for good and bad commits.\n>    $ git bisect good master\n>    info: bisect waiting for a bad commit, one good commit known.\n>    $ git bisect good maint\n>    info: bisect waiting for a bad commit, two good commits known.\n>    $ git bisect bad next\n>    Bisecting: ...\n>\n>Then the exchange for the lazy short-hand form of \"bisect start\"\n>would fall out quite naturally.\n>\n>    $ git bisect start d93ff48803f0 -- v6.3\n>    info: bisect waiting for a good commit, one bad commit known.\n>\n>For a bonus point, we may want to also say something on these\n>\"info:\" lines that we were given a pathspec.\n>\n>It would also be a good idea to add a new subcommand \"git bisect status\"\n>to recompute the state (i.e. what it is waiting for and what it\n>already knows) when the user forgets, which can happen quite often.\n\nSounds good.\n\n>With such a bonus feature, the exchange might go like this:\n>\n>    $ git bisect start seen\n>    info: bisect waiting for a good commit, a bad commit known.\n>    $ git reset --hard maint ;# choose an older point, hoping it is good.\n>    $ make test\n>    ... pages of output scrolls the \"info:\" out of window ...\n>    $ git bisect status\n>    info: bisect waiting for a good commit, a bad commit known.\n>    $ git bisect bad maint\n>    info: bisect waiting for a good commit, a bad commit known.\n>    $ git reset --hard v1.0 ;# an even older point, hoping it is good.\n>    $ make test\n>    ... again pages of output ...\n>    $ git bisect good v1.0\n>    Bisecting ...\n>\n>Hmm?\n\nI'll wait a few days to see if anyone else has any other feedback, and then \nI'll send a patch to that effect.\n\nThanks!\n"}]}