Re: [PATCH] git-prompt: fix sequencer/todo detection

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Mon, 28 Mar 2022, Junio C Hamano wrote:

> Danny Lin <danny0838@gmail.com> writes:
>
> > Previous case does not correctly check the "p ..." pattern.
> >
> > Signed-off-by: Danny Lin <danny0838@gmail.com>
> > ---
> >  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