Re: [PATCH] git-am: force egrep to use correct characters set
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
Christian Himpel
