On Sun, Sep 6, 2015 at 5:49 AM, Gabor Bernat <bernat@xxxxxxxxxxxxxx> wrote:
> 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
>>
>> 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.
Use of 'grep -q' seems to be fine, however, Junio's comment was about
the errant semicolon, which should not be kept.
>>> * 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?
Junio's proposed if/then/else construct should be satisfactory.
--
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