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

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

[Danny Lin]

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
 		;;
-- 
2.35.1.windows.2

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

[Junio C Hamano]

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
>  		;;

The original obviously is broken ;-)  Will queue.  Thanks.

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

[Junio C Hamano]

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().

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?

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?  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.  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...

Thanks.

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > 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 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).

Good.  Then the right "fix" would be to drop the misleading "We
would also accept the abbreviated" side of the case arm, instead of
fixing "if we were generating the abbreviated one, here is how we
might support it" code, I guess.

Thanks for sanity-checking my digging in the sequencer.c code.