| Message ID | 20220325145301.3370-1-danny0838@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-prompt: fix sequencer/todo detection | expand |
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.
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.
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
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.
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 ;;
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(-)