Re: [PATCH] filter-branch: add passed/remaining seconds on progress

> On Fri, Sep 4, 2015 at 10:15 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Fri, Sep 4, 2015 at 2:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Gábor Bernát <bernat@primeranks.net> writes:
>>>> +echo $(date +%s) | grep -q '^[0-9]+$';  2>/dev/null && show_seconds=t
>>>
>>> That is very strange construct.  I think you meant to say something
>>> like
>>>
>>>         if date '+%s' 2>/dev/null | grep -q '^[0-9][0-9]*$'
>>>         then
>>>                 show_seconds=t
>>>         else
>>>                 show_seconds=
>>>         fi
>>
>> This also mutated. The suggested form wanted to suppress errors from
>> 'date' if it complained about "%s", and from 'grep'. In retrospect,
>> applying it to 'grep' is questionable. I was recalling this warning
>> from the Autoconf manual[2]:
>>
>>     Some of the options required by Posix are not portable in
>>     practice. Don't use ‘grep -q’ to suppress output, because many
>>     grep implementations (e.g., Solaris) do not support -q. Don't use
>>     ‘grep -s’ to suppress output either, because Posix says -s does
>>     not suppress output, only some error messages; also, the -s
>>     option of traditional grep behaved like -q does in most modern
>>     implementations. Instead, redirect the standard output and
>>     standard error (in case the file doesn't exist) of grep to
>>     /dev/null. Check the exit status of grep to determine whether it
>>     found a match.
>>
>> however, Git tests use 'grep -q' heavily, so perhaps we don't worry about that.
>
> So we should keep it as it is.

>>>  * I do not think you are clearing show_seconds to empty anywhere,
>>>    so an environment variable the user may have when s/he starts
>>>    filter-branch will seep through and confuse you.
>>
>> The empty assignment was implied in my example, but I should have been
>> more explicit and shown a more complete snippet:
>>
>>     show_eta=
>>     ...
>>     { echo $(date +%s) | grep -q '^[0-9][0-9]*$'; } 2>/dev/null &&
>>         show_eta=t
>>
>> The suggested 'if' form has the attribute of being clearer.
>
> My bad, sorry for that. Will amend.
>
> Any other pain points, or this construction will satisfy everybody?