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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 4, 2015, 20:15 UTC
Message-ID: <CAPig+cRh-7BDOoumLxyh6_tNspL3ANq_wCE5f_VoQt6UwUFckQ@mail.gmail.com>
In-Reply-To: <xmqqk2s6f2zj.fsf@gitster.mtv.corp.google.com>

On Fri, Sep 4, 2015 at 2:34 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

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

[1]: http://thread.gmane.org/gmane.comp.version-control.git/276531/focus=276837 [2]: https://www.gnu.org/software/autoconf/manual/autoconf.html#grep