From: Justin Donnelly <justinrdonnelly@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Justin Donnelly via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Subject: Re: [PATCH] git-prompt: show 'CONFLICT' indicator at command prompt
Date: Wed, 27 Jul 2022 22:17:52 -0400 [thread overview]
Message-ID: <CAGTqyRzfeh4HLbXUCb3Zv=bWNoBmvBU5QB=N2g2d0=y+NEToag@mail.gmail.com> (raw)
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:
>
> "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.
> > @@ -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.
next prev parent reply other threads:[~2022-07-28 2:18 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-26 1:23 [PATCH] git-prompt: show 'CONFLICT' indicator at command prompt Justin Donnelly via GitGitGadget
2022-07-27 20:13 ` Junio C Hamano
2022-07-28 2:17 ` Justin Donnelly [this message]
2022-07-28 17:44 ` Junio C Hamano
2022-07-28 23:33 ` Justin Donnelly
2022-07-29 22:08 ` [PATCH v2] git-prompt: show presence of unresolved conflicts " Justin Donnelly via GitGitGadget
2022-08-14 21:06 ` Justin Donnelly
2022-08-15 4:22 ` Junio C Hamano
2022-08-15 12:50 ` Johannes Schindelin
2022-08-16 3:36 ` Justin Donnelly
2022-08-15 13:04 ` Phillip Wood
2022-08-15 16:00 ` Junio C Hamano
2022-08-16 4:20 ` Justin Donnelly
2022-08-16 23:32 ` Justin Donnelly
2022-08-17 0:18 ` [PATCH v3] " Justin Donnelly via GitGitGadget
2022-08-19 11:29 ` Johannes Schindelin
2022-08-19 17:57 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAGTqyRzfeh4HLbXUCb3Zv=bWNoBmvBU5QB=N2g2d0=y+NEToag@mail.gmail.com' \
--to=justinrdonnelly@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).