Re: [PATCH] git-prompt: fix sequencer/todo detection
Hi Junio,
On Mon, 28 Mar 2022, Junio C Hamano wrote:
Show 27 quoted lines
> 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 --
Show changes to t/t9903-bash-prompt.sh +4 −3
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
Danny Lin
