Re: [msysGit] Pull request for msysGit patches

[Eric Sunshine <sunshine@sunshineco.com>]

Hi Hannes,

On 9/28/2010 4:52 PM, Johannes Sixt wrote:
> 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