Re: [PATCH] completion: fix prompt with unset SHOWCONFLICTSTATE in nounset mode

From: Ville Skyttä <ville.skytta@iki.fi>
Date: Apr 1, 2024, 17:07 UTC
Message-ID: <CABr9L5A_zz6ZvBWUoX_Px6Upyiur3+SPp8U91uw3OXO0mXZgeg@mail.gmail.com>
In-Reply-To: <xmqqttklcd6d.fsf@gitster.g>

On Mon, 1 Apr 2024 at 15:31, Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines

>
> Ville Skyttä <ville.skytta@iki.fi> writes:
>
> > `GIT_PS1_SHOWCONFLICTSTATE` is a user variable that might not be set,
> > causing errors when the shell is in `nounset` mode.
> >
> > Take into account on access by falling back to an empty string.
> >
> > Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>
> > ---
>
> Obviously a good thing to do.
>
> A related tangent is that
>
>     $ git grep -e '$GIT_PS1' -e '${GIT_PS1_[A-Z0-9_]*}' contrib/completion/
>
> shows a hit for the line with SHOWCONFLICTSTATE, plus two lines with
> GIT_PS1_SHOWUPSTREAM that lack the "if unset then use this value".
> Do you want to do another patch to fix them, or are they good as-is
> for some reason?

I initially actually changed those very lines too when working on the fix for the issue I faced with GIT_PS1_SHOWCONFLICTSTATE. However, both occurrences are within __git_ps1_show_upstream, and the only call site for that function is protected by a check on the variable that does take possible unset state into account; the function will in the file's current form never be called with it unset. Additionally, the first occurrence is immediately following a line that sets the variable, so that one is "doubly protected".

Therefore, I decided to undo those changes and not include them here. I guess it's a matter of taste whether one finds it desirable to protect those accesses nevertheless, but it's not strictly necessary.

Ville