From: Eric Sunshine Date: Sun, 06 Sep 2015 10:05:53 GMT Subject: Re: [PATCH] filter-branch: add passed/remaining seconds on progress Message-ID: In-Reply-To: On Sun, Sep 6, 2015 at 5:49 AM, Gabor Bernat wrote: > On Fri, Sep 4, 2015 at 10:15 PM, Eric Sunshine wrote: >> On Fri, Sep 4, 2015 at 2:34 PM, Junio C Hamano wrote: >>> Gábor Bernát 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.