Hi Junio, On Mon, 28 Mar 2022, Junio C Hamano wrote: > Danny Lin <danny0838@xxxxxxxxx> writes: > > > Previous case does not correctly check the "p ..." pattern. > > > > Signed-off-by: Danny Lin <danny0838@xxxxxxxxx> > > --- > > contrib/completion/git-prompt.sh | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh > > index db7c0068fb..8ae341a306 100644 > > --- a/contrib/completion/git-prompt.sh > > +++ b/contrib/completion/git-prompt.sh > > @@ -315,7 +315,7 @@ __git_sequencer_status () > > elif __git_eread "$g/sequencer/todo" todo > > then > > case "$todo" in > > - p[\ \ ]|pick[\ \ ]*) > > + p[\ \ ]*|pick[\ \ ]*) > > r="|CHERRY-PICKING" > > return 0 > > ;; > > It is obvious that the original code is *not* prepared to see 'p' > followed by whitespace followed by other things, but I am not sure > how the code in sequencer.c::todo_list_write_to_file() can choose > to pass flags & TODO_LIST_ABBREVIATE_CMDS to todo_list_to_strbuf(). At first, I thought that if `rebase.abbreviateCommands` is set to `true`, we would write out todo lists with one-letter commands. But that's not true, it's only while we edit the file that that config setting affects how the todo list is written. I _guess_ it is during that time window that the prompt does not work as expected? > Danny, do you have a reproduction recipe, preferrably one you can > turn into a new test in t9903-bash-prompt.sh? Or was this found > merely by inspecting the code? I _guess_ there is a script at play which rewrites the `todo` file and which runs when a cherry-pick fails due to merge conflicts. Here is a patch that will exercise the code and verify the fix: -- snip -- diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh index bbd513bab0f..c5c3e3c83de 100755 --- a/t/t9903-bash-prompt.sh +++ b/t/t9903-bash-prompt.sh @@ -221,7 +221,10 @@ test_expect_success 'prompt - cherry-pick' ' git reset --merge && test_must_fail git rev-parse CHERRY_PICK_HEAD && __git_ps1 >"$actual" && - test_cmp expected "$actual" + test_cmp expected "$actual" && + echo "p HEAD" >.git/sequencer/todo && + __git_ps1 >"$actual".2 && + test_cmp expected "$actual".2 ' test_expect_success 'prompt - revert' ' -- snap -- > Dscho, as far as I can tell, builtin/rebase.c can set the bit in the > flags word when rebase.abbreviatecommands configuration is set, but > that configuration variable is about rebase and it shouldn't affect > how multi-step cherry-pick would work, should it? Indeed. In a multi-commit cherry-pick, we call `walk_revs_populate_todo()` which uses the long form of the `pick` command, always (look for `command_string`). > I am wondering if an uninitialized "flags" word, whose > TODO_LIST_ABBREVIATE_CMDS bit randomly was turned on, caused > todo_list_to_strbuf() to write an abbreviated insn in the todo file. I don't think that `todo_list_to_strbuf()` is called during a cherry-pick. Instead, `walk_revs_populate_todo()` is called in `git cherry-pick --continue`, and it does not modify the todo commands if run in non-rebase-i mode. > If so, the insn word being abbreviated or fully spelled out would not > affect the correctness, but the flags word affects other things that are > more crucial to correctness, so... It looks to me as if the abbreviated commands cannot be generated by Git (the `replay_opts` in `builtin/revert.c` are all initialized to `REPLAY_OPTS_INIT`, so there is not even any chance of uninitialized data there). Ciao, Dscho
