Re: [PATCH] git-am: force egrep to use correct characters set

From: Christian Himpel <chressie@googlemail.com>
Date: Sep 28, 2009, 09:32 UTC
Message-ID: <20090928093216.GA31459@lamagra.informatik.uni-ulm.de>
In-Reply-To: <4AC06FFF.20008@viscovery.net>

On Mon, Sep 28, 2009 at 10:12:47AM +0200, Johannes Sixt wrote:

Show 37 quoted lines

> Jeff King schrieb:
> > On Fri, Sep 25, 2009 at 06:43:20PM +0200, Christian Himpel wrote:
> > 
> >> According to egrep(1) the US-ASCII table is used when LC_ALL=C is set.
> >> We do not rely here on the LC_ALL value we get from the environment.
> > 
> > Hmm. Probably makes sense here, as it is a wide enough range that it may
> > pick up other stray non-ascii characters in other charsets (though as
> > the manpage notes, the likely thing is to pick up A-Z along with a-z,
> > which is OK here as we encompass both in our range).
> > 
> > There are two other calls to egrep with brackets (both in
> > git-submodule.sh), but they are just [0-7], which is presumably OK in
> > just about any charset.
> > 
> > Do you happen to know a charset in which this is a problem, just for
> > reference?
> 
> It's not so much about charsets than about languages:
> 
>        Within a bracket expression, a range expression consists
>        of two characters separated by a hyphen.  It matches any
>        single character that sorts between the two  characters,
>        inclusive,  using  the  locale's  collating sequence and
>        character set.  For example, in the  default  C  locale,
>        [a-d]  is equivalent to [abcd].  Many locales sort char-
>        acters in dictionary order, and in these  locales  [a-d]
>        is  typically  not  equivalent  to  [abcd];  it might be
>        equivalent to [aBbCcDd], for  example.   To  obtain  the
>        traditional  interpretation  of bracket expressions, you
>        can use the C locale by setting the  LC_ALL  environment
>        variable to the value C.
> 
> For example, in locale de_DE.UTF-8, GNU grep '[a-z]' matches lowercase
> letters, uppercase letters (!), and umlauts (!!) because in dictionary
> order, 'A' and 'a' are equivalent and 'Ä' sorts after 'A'. (The input must
> be UTF-8, of course.)

Thanks for pointing this out. You are right. I must have read the "dictonary order" part over.

> Given that this applies not only to egrep, but to grep in general (and
> perhaps even to other tools that support ranges, like sed), it may be
> necessary to audit all range expressions.

After doing a quick:

LC_ALL=C find . -name '*.sh' -exec \
         egrep -Hne '(grep|awk|sed).*\[.*-.*\]' {} \;

As far as I can see, range expressions are used:

1. to replace or grep hexadecimal numbers (SHA1 sums).  This shouldn't
be a problem, if we can assume that these numbers are never malformed.

2. to replace or grep numbers (with digits).  This shouldn't be a
problem, since digits should be in dictionary order in every language
(?!).

3. in git-rebase--interactive.sh:742 to grep for a previously generated
string.  So it should be safe here.

> The case identified by Christian is certainly important because it is
> applied to a file whose contents can be anything, and the purpose of the
> check is to identify the text as an mbox file, whose header section can be
> only US-ASCII by definition. So, I think it has merit to apply the patch.

Yes. It seems that this is the only place where it is important to match just the ASCII printable characters.

Regards, chressie