On Fri, Sep 4, 2015 at 10:15 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Fri, Sep 4, 2015 at 2:34 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Gábor Bernát <bernat@xxxxxxxxxxxxxx> 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.
Yes, the initial construct was different, willing to make this change.
>
>> * "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.
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.
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/276531/focus=276837
> [2]: https://www.gnu.org/software/autoconf/manual/autoconf.html#grep
Any other pain points, or this construction will satisfy everybody?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html