{"thread":{"id":"57617","subject":"[PATCH] git-prompt: fix sequencer/todo detection","startedAt":"2022-03-25T14:54:01Z","lastAt":"2022-04-07T22:43:18Z","messageCount":5,"participants":["Danny Lin","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"452271","messageId":"20220325145301.3370-1-danny0838@gmail.com","threadId":"57617","inReplyTo":null,"subject":"[PATCH] git-prompt: fix sequencer/todo detection","fromName":"Danny Lin","fromEmail":"danny0838@gmail.com","sentAt":"2022-03-25T14:53:01Z","receivedAt":"2022-03-25T14:54:01Z","isPatch":true,"sender":{"key":"danny0838@gmail.com","avatar":"https://avatars.githubusercontent.com/u/531417?v=4"},"body":"Previous case does not correctly check the \"p ...\" pattern.\n\nSigned-off-by: Danny Lin <danny0838@gmail.com>\n---\n contrib/completion/git-prompt.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex db7c0068fb..8ae341a306 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -315,7 +315,7 @@ __git_sequencer_status ()\n \telif __git_eread \"$g/sequencer/todo\" todo\n \tthen\n \t\tcase \"$todo\" in\n-\t\tp[\\ \\\t]|pick[\\ \\\t]*)\n+\t\tp[\\ \\\t]*|pick[\\ \\\t]*)\n \t\t\tr=\"|CHERRY-PICKING\"\n \t\t\treturn 0\n \t\t;;\n-- \n2.35.1.windows.2\n\n"},{"id":"452395","messageId":"xmqqtubl7eng.fsf@gitster.g","threadId":"57617","inReplyTo":"20220325145301.3370-1-danny0838@gmail.com","subject":"Re: [PATCH] git-prompt: fix sequencer/todo detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-26T00:15:15Z","receivedAt":"2022-03-26T00:15:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Danny Lin <danny0838@gmail.com> writes:\n\n> Previous case does not correctly check the \"p ...\" pattern.\n>\n> Signed-off-by: Danny Lin <danny0838@gmail.com>\n> ---\n>  contrib/completion/git-prompt.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n> index db7c0068fb..8ae341a306 100644\n> --- a/contrib/completion/git-prompt.sh\n> +++ b/contrib/completion/git-prompt.sh\n> @@ -315,7 +315,7 @@ __git_sequencer_status ()\n>  \telif __git_eread \"$g/sequencer/todo\" todo\n>  \tthen\n>  \t\tcase \"$todo\" in\n> -\t\tp[\\ \\\t]|pick[\\ \\\t]*)\n> +\t\tp[\\ \\\t]*|pick[\\ \\\t]*)\n>  \t\t\tr=\"|CHERRY-PICKING\"\n>  \t\t\treturn 0\n>  \t\t;;\n\nThe original obviously is broken ;-)  Will queue.  Thanks.\n"},{"id":"452516","messageId":"xmqqwngdzque.fsf@gitster.g","threadId":"57617","inReplyTo":"20220325145301.3370-1-danny0838@gmail.com","subject":"Re: [PATCH] git-prompt: fix sequencer/todo detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-28T21:53:29Z","receivedAt":"2022-03-28T22:13:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Danny Lin <danny0838@gmail.com> writes:\n\n> Previous case does not correctly check the \"p ...\" pattern.\n>\n> Signed-off-by: Danny Lin <danny0838@gmail.com>\n> ---\n>  contrib/completion/git-prompt.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n> index db7c0068fb..8ae341a306 100644\n> --- a/contrib/completion/git-prompt.sh\n> +++ b/contrib/completion/git-prompt.sh\n> @@ -315,7 +315,7 @@ __git_sequencer_status ()\n>  \telif __git_eread \"$g/sequencer/todo\" todo\n>  \tthen\n>  \t\tcase \"$todo\" in\n> -\t\tp[\\ \\\t]|pick[\\ \\\t]*)\n> +\t\tp[\\ \\\t]*|pick[\\ \\\t]*)\n>  \t\t\tr=\"|CHERRY-PICKING\"\n>  \t\t\treturn 0\n>  \t\t;;\n\nIt is obvious that the original code is *not* prepared to see 'p'\nfollowed by whitespace followed by other things, but I am not sure\nhow the code in sequencer.c::todo_list_write_to_file() can choose\nto pass flags & TODO_LIST_ABBREVIATE_CMDS to todo_list_to_strbuf().\n\nDanny, do you have a reproduction recipe, preferrably one you can\nturn into a new test in t9903-bash-prompt.sh?  Or was this found\nmerely by inspecting the code?\n\nDscho, as far as I can tell, builtin/rebase.c can set the bit in the\nflags word when rebase.abbreviatecommands configuration is set, but\nthat configuration variable is about rebase and it shouldn't affect\nhow multi-step cherry-pick would work, should it?  I am wondering if\nan uninitialized \"flags\" word, whose TODO_LIST_ABBREVIATE_CMDS bit\nrandomly was turned on, caused todo_list_to_strbuf() to write an\nabbreviated insn in the todo file.  If so, the insn word being\nabbreviated or fully spelled out would not affect the correctness,\nbut the flags word affects other things that are more crucial to\ncorrectness, so...\n\nThanks.\n\n\n"},{"id":"453294","messageId":"nycvar.QRO.7.76.6.2204072315330.347@tvgsbejvaqbjf.bet","threadId":"57617","inReplyTo":"xmqqwngdzque.fsf@gitster.g","subject":"Re: [PATCH] git-prompt: fix sequencer/todo detection","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-04-07T21:45:20Z","receivedAt":"2022-04-07T21:45:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 28 Mar 2022, Junio C Hamano wrote:\n\n> Danny Lin <danny0838@gmail.com> writes:\n>\n> > Previous case does not correctly check the \"p ...\" pattern.\n> >\n> > Signed-off-by: Danny Lin <danny0838@gmail.com>\n> > ---\n> >  contrib/completion/git-prompt.sh | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n> > index db7c0068fb..8ae341a306 100644\n> > --- a/contrib/completion/git-prompt.sh\n> > +++ b/contrib/completion/git-prompt.sh\n> > @@ -315,7 +315,7 @@ __git_sequencer_status ()\n> >  \telif __git_eread \"$g/sequencer/todo\" todo\n> >  \tthen\n> >  \t\tcase \"$todo\" in\n> > -\t\tp[\\ \\\t]|pick[\\ \\\t]*)\n> > +\t\tp[\\ \\\t]*|pick[\\ \\\t]*)\n> >  \t\t\tr=\"|CHERRY-PICKING\"\n> >  \t\t\treturn 0\n> >  \t\t;;\n>\n> It is obvious that the original code is *not* prepared to see 'p'\n> followed by whitespace followed by other things, but I am not sure\n> how the code in sequencer.c::todo_list_write_to_file() can choose\n> to pass flags & TODO_LIST_ABBREVIATE_CMDS to todo_list_to_strbuf().\n\nAt first, I thought that if `rebase.abbreviateCommands` is set to `true`,\nwe would write out todo lists with one-letter commands.\n\nBut that's not true, it's only while we edit the file that that config\nsetting affects how the todo list is written.\n\nI _guess_ it is during that time window that the prompt does not work as\nexpected?\n\n> Danny, do you have a reproduction recipe, preferrably one you can\n> turn into a new test in t9903-bash-prompt.sh?  Or was this found\n> merely by inspecting the code?\n\nI _guess_ there is a script at play which rewrites the `todo` file and\nwhich runs when a cherry-pick fails due to merge conflicts. Here is a\npatch that will exercise the code and verify the fix:\n\n-- snip --\ndiff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh\nindex bbd513bab0f..c5c3e3c83de 100755\n--- a/t/t9903-bash-prompt.sh\n+++ b/t/t9903-bash-prompt.sh\n@@ -221,7 +221,10 @@ test_expect_success 'prompt - cherry-pick' '\n \tgit reset --merge &&\n \ttest_must_fail git rev-parse CHERRY_PICK_HEAD &&\n \t__git_ps1 >\"$actual\" &&\n-\ttest_cmp expected \"$actual\"\n+\ttest_cmp expected \"$actual\" &&\n+\techo \"p HEAD\" >.git/sequencer/todo &&\n+\t__git_ps1 >\"$actual\".2 &&\n+\ttest_cmp expected \"$actual\".2\n '\n\n test_expect_success 'prompt - revert' '\n-- snap --\n\n> Dscho, as far as I can tell, builtin/rebase.c can set the bit in the\n> flags word when rebase.abbreviatecommands configuration is set, but\n> that configuration variable is about rebase and it shouldn't affect\n> how multi-step cherry-pick would work, should it?\n\nIndeed. In a multi-commit cherry-pick, we call `walk_revs_populate_todo()`\nwhich uses the long form of the `pick` command, always (look for\n`command_string`).\n\n> I am wondering if an uninitialized \"flags\" word, whose\n> TODO_LIST_ABBREVIATE_CMDS bit randomly was turned on, caused\n> todo_list_to_strbuf() to write an abbreviated insn in the todo file.\n\nI don't think that `todo_list_to_strbuf()` is called during a cherry-pick.\nInstead, `walk_revs_populate_todo()` is called in `git cherry-pick\n--continue`, and it does not modify the todo commands if run in\nnon-rebase-i mode.\n\n> If so, the insn word being abbreviated or fully spelled out would not\n> affect the correctness, but the flags word affects other things that are\n> more crucial to correctness, so...\n\nIt looks to me as if the abbreviated commands cannot be generated by Git\n(the `replay_opts` in `builtin/revert.c` are all initialized to\n`REPLAY_OPTS_INIT`, so there is not even any chance of uninitialized data\nthere).\n\nCiao,\nDscho\n"},{"id":"453305","messageId":"xmqq7d80qzuq.fsf@gitster.g","threadId":"57617","inReplyTo":"nycvar.QRO.7.76.6.2204072315330.347@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] git-prompt: fix sequencer/todo detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-07T22:43:09Z","receivedAt":"2022-04-07T22:43:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> > diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n>> > index db7c0068fb..8ae341a306 100644\n>> > --- a/contrib/completion/git-prompt.sh\n>> > +++ b/contrib/completion/git-prompt.sh\n>> > @@ -315,7 +315,7 @@ __git_sequencer_status ()\n>> >  \telif __git_eread \"$g/sequencer/todo\" todo\n>> >  \tthen\n>> >  \t\tcase \"$todo\" in\n>> > -\t\tp[\\ \\\t]|pick[\\ \\\t]*)\n>> > +\t\tp[\\ \\\t]*|pick[\\ \\\t]*)\n>> >  \t\t\tr=\"|CHERRY-PICKING\"\n>> >  \t\t\treturn 0\n>> >  \t\t;;\n>> ...\n>\n> It looks to me as if the abbreviated commands cannot be generated by Git\n> (the `replay_opts` in `builtin/revert.c` are all initialized to\n> `REPLAY_OPTS_INIT`, so there is not even any chance of uninitialized data\n> there).\n\nGood.  Then the right \"fix\" would be to drop the misleading \"We\nwould also accept the abbreviated\" side of the case arm, instead of\nfixing \"if we were generating the abbreviated one, here is how we\nmight support it\" code, I guess.\n\nThanks for sanity-checking my digging in the sequencer.c code.\n\n\n"}]}