Re: [PATCH 6/8] git-prompt: add fallback for shells without $'...'

avih
2024-07-24T02:09:02Z

On Tuesday, July 23, 2024 at 11:25:29 PM GMT+3, Junio C Hamano <gitster@pobox.com> wrote:

> > $'...' is new in POSIX (2024), and some shells support it in recent
> > versions, while others have had it for decades (bash, zsh, ksh93).

> I will not look at this series futher during the current development
> cycle that is about to conclude, but ...

Thanks. I'm happy to continue whenever others have the bandwidth.

> > +__git_SOH=$'\1' __git_STX=$'\2' __git_ESC=$'\33'
> > +__git_LF=$'\n' __git_CRLF=$'\r\n'
> > +
> > +if [ $'\101' != A ]; then  # fallback for shells without $'...'
> > +  __git_CRLF=$(printf "\r\n\1\2\33")  # CR LF SOH STX ESC
> > +  __git_ESC=${__git_CRLF#????}; __git_CRLF=${__git_CRLF%?}
> > +  __git_STX=${__git_CRLF#???};  __git_CRLF=${__git_CRLF%?}
> > +  __git_SOH=${__git_CRLF#??};  __git_CRLF=${__git_CRLF%?}
> > +  __git_LF=${__git_CRLF#?}
> > +fi

> ... I would prefer to see it done without any "fallback".

That's fair. I'll change it.

> $(printf '\r') would work with bash, zsh and ksh93, too, and one
> time assignment to these variables is not going to be performance
> critical.

Generally true, and also mostly non-generally as far as performace
goes, though personally I'd prefer to avoid command substitution in
this context if possible, as even a single one can have non-negligible
impact in (very) hot scripts.

But it would still be very small, and doesn't matter with this file.

> Just forbid use of $'\octal' notation in the coding
> guidelines document, and implement just one variant.

Agreed, and the CodingGuidelines patch LGTM.

However, off the top of my head I wouldn't know how this variant
should look like. This one printf and splitting it later is a bit
meh to be used in every script which needs control literals, but
I also don't have anything cleaner off the top of my head.

> Perhaps we should spell more things out that you wrote in some of
> your proposed log messages more explicitly.  I think these have been
> rules we have followed (grep for them in *.sh files outside
> contrib/) but I did not find mention in the guidelines document.

If I can help with this, let me know how.

> Thanks.

My pleasure.