Re: [PATCH] git-prompt: show 'CONFLICT' indicator at command prompt

From: Justin Donnelly <justinrdonnelly@gmail.com>
Date: Jul 28, 2022, 02:17 UTC
Message-ID: <CAGTqyRzfeh4HLbXUCb3Zv=bWNoBmvBU5QB=N2g2d0=y+NEToag@mail.gmail.com>
In-Reply-To: <xmqq35ems49j.fsf@gitster.g>

Thanks for the feedback.

On Wed, Jul 27, 2022 at 4:13 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 24 quoted lines

>
> "Justin Donnelly via GitGitGadget" <gitgitgadget@gmail.com> 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.

Show 47 quoted lines

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