Re: [msysGit] Pull request for msysGit patches

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 29, 2010, 02:23 UTC
Message-ID: <4CA2A32E.3050802@sunshineco.com>
In-Reply-To: <201009282252.25688.j6t@kdbg.org>

Hi Hannes,

On 9/28/2010 4:52 PM, Johannes Sixt wrote:

Show 15 quoted lines

> On Dienstag, 28. September 2010, Pat Thoyts wrote:
>> Junio,
>>
>> The msysGit tree currently tracks some 50+ patches on top of 'next'. I
>> have gathered 42 of these that look good to move upstream.
>
> I've browsed through the list, and I'll annotate my opinion below. When I
> say 'OK', then this means that the patch looks good, but it doesn't imply
> that I have tested it.
>
>>        Side-step line-ending corruption leading to t3032 failures.
>
> This one has non-portable 'export foo=bar', but it is in a MinGW specific
> path, so it should be fine. But why do we need to export GREP_OPTIONS, but
> not SED_OPTIONS?

I also normally avoid unportable 'export foo=bar'. In the particular case of GREP_OPTIONS, when commenting on my original patch submission, Dscho suggested 'test_have_prereq MINGW && export GREP_OPTIONS=foo' so that is the form which made it into the final patch. While considering whether this unportable usage was worthwhile, I took into consideration the fact that a much more senior project member preferred it, that the MinGW-specific build environment is Bash-based, and that a couple other test scripts (t1509, t5560) also employ this form. If preferred, the patches can be re-submitted to avoid the unportable usage.

Regarding exporting GREP_OPTIONS but not SED_OPTIONS: grep explicitly recognizes the GREP_OPTIONS environment variable. My original patch did not export GREP_OPTIONS, but instead referenced it as $GREP_OPTIONS in the grep invocation. Upon review, Dscho preferred the variable to be exported rather than referenced manually as $GREP_OPTIONS. sed, on the other hand, does not recognize any 'options' environment variable. It is necessary, therefore, to interpolate $SED_OPTIONS directly during the sed invocation. This variable is therefore local to the script and need not be exported.

-- ES