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

> 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
>
> The final format suggested[1] for this test was:
>
>     { echo $(date +%s) | grep -q '^[0-9][0-9]*$'; } 2>/dev/null &&
>         show_eta=t
>
>> A handful of points:
>>
>>  * "echo $(any-command)" is suspect, unless you are trying to let
>>    the shell munge output from any-command, which is not the case.
>
> Primarily my fault. I don't know what I was thinking when suggesting that.

>
>>  * "grep" without -E (or "egrep") takes BRE, which "+" (one or more)
>>    is not part of.
>
> This seems to have mutated from the suggested form.
>
>>  * That semicolon is a syntax error.  I think whoever suggested you
>>    to use it meant to squelch possible errors from "date" that does
>>    not understand the "%s" format.
>
> 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.

>
>>  * 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.