Thanks for the feedback.
On Wed, Jul 27, 2022 at 4:13 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Justin Donnelly via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> > index 1435548e004..2d30eb5c17e 100644
> > --- a/contrib/completion/git-prompt.sh
> > +++ b/contrib/completion/git-prompt.sh
> > @@ -84,6 +84,9 @@
> > # single '?' character by setting GIT_PS1_COMPRESSSPARSESTATE, or omitted
> > # by setting GIT_PS1_OMITSPARSESTATE.
> > #
> > +# When there is a conflict, the prompt will include "|CONFLICT". This can
> > +# be omitted by setting GIT_PS1_OMITCONFLICTSTATE.
> > +#
>
> It is unusual to subject our unsuspecting users to new features in a
> way that is done by this patch. A more usual practice, I think, is
> to tell the users that they can set GIT_PS1_INCLUDECONFLICTSTATE to
> "yes" if they want to opt in, and trigger the new feature only to
> them. Later, we may decide that the feature is useful and widely
> apprlicable enough, at which time it may be turned on by default and
> tell the users to set GIT_PS1_INCLUDECONFLICTSTATE to "no" if they
> do not want to see it. But one step at a time.
>
I see that most of the state indicators are disabled by default, so it
makes sense to be consistent. Should I make a variable with 'yes'/'no'
values, or set/unset? 'yes'/'no' has the benefit that if the default
is later changed, existing setups will continue to work. Set/unset
makes it harder to change the default later (I guess you could just
use a new variable with a different name), but is more consistent with
the other state indicators. I'm leaning towards set/unset, but it's
not a strong preference. Let me know if you disagree.
> > @@ -508,6 +511,12 @@ __git_ps1 ()
> > r="$r $step/$total"
> > fi
> >
> > + local conflict="" # state indicator for unresolved conflicts
> > + if [[ -z "${GIT_PS1_OMITCONFLICTSTATE-}" ]] &&
>
> And flipping the polarity and disabling it by default would be a
> simple change, I would imagine, that can be made here.
>
> > + [[ $(git ls-files --unmerged 2>/dev/null) ]]; then
> > + conflict="|CONFLICT"
> > + fi
> > +
> > local w=""
> > local i=""
> > local s=""
> > @@ -572,7 +581,7 @@ __git_ps1 ()
> > fi
> >
> > local f="$h$w$i$s$u$p"
> > - local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"
> > + local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}${conflict}"
> >
> > if [ $pcmode = yes ]; then
> > if [ "${__git_printf_supports_v-}" != yes ]; then
> > diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> > index 6a30f5719c3..9314776211c 100755
> > --- a/t/t9903-bash-prompt.sh
> > +++ b/t/t9903-bash-prompt.sh
> > @@ -183,7 +183,7 @@ test_expect_success 'prompt - interactive rebase' '
> > '
> >
> > test_expect_success 'prompt - rebase merge' '
>
> Of course, this needs to be tweaked if we did so. Running the test
> in the default state will not need this change, but then ...
>
> > - printf " (b2|REBASE 1/3)" >expected &&
> > + printf " (b2|REBASE 1/3|CONFLICT)" >expected &&
> > git checkout b2 &&
> > test_when_finished "git checkout main" &&
> > test_must_fail git rebase --merge b1 b2 &&
>
> ... a new test that runs the same thing but with the new feature
> asked by setting the environment variable would show the new |CONFLICT
> marker.
I'll add these.
>
> Thanks.
