On Mon, 1 Apr 2024 at 15:31, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Ville Skyttä <ville.skytta@xxxxxx> 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@xxxxxx>
> > ---
>
> 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
