Re: [PATCH V2 1/2] devel: add release-checks.sh

Subject: Re: [PATCH V2 1/2] devel: add release-checks.sh

Date: Mon, 03 Sep 2012 17:36:11 +0200

To: Tomi Ollila, notmuch@notmuchmail.org

Cc:

From: Michal Nazarewicz


>> Tomi Ollila <tomi.ollila@iki.fi> writes:
>>> +for f in ./version debian/changelog NEWS "$PV_FILE"
>>> +do
>>> +	test -f $f || { append_emsg "File '$f' is missing"; continue; }
>>> +	test -r $f || { append_emsg "File '$f' is unreadable"; continue; }
>>> +	test -s $f ||   append_emsg "File '$f' is empty"

> On Mon, Sep 03 2012, Michal Nazarewicz <mina86@mina86.com> wrote:
>> if ! [ -f "$f" ]; then
>> 	append_emsg "File '$f' is missing"
>> elif ! [ -r "$f" ]; then
>> 	append_emsg "File '$f' is unreadable"
>> elif ! [ -s "$f" ]; then
>> 	append_emsg "File '$f' is empty"
>> fi

Tomi Ollila <tomi.ollila@iki.fi> writes:
> IMHO this short-circuited or (||) version works well due to this
> negation handling. The $f's should have been quoted ("$f"), though
> (in case some of the items contain whitespace the construct 
> would't fail). I am open to other opinions, though.

I personally don't like “||” or “&&” as a replacement for if as I find
it less readable.  Especially here as you use braces and “continue”.

>> if [ x"$deb_notmuch" = xnotmuch ]
>>
>> And so in the rest of the conditions below.
>
> builtin bash '[' seems to be robust in these cases(*) ... but I'm now breaking
> my own rule about not using most portable expressions; maybe it is just
> ugliness of the format in so many places. I'm not changing unless there is
> more desire for it :)

It's not even bash.  Newest POSIX requires that behaviour, but I always
use x anyway myself, hence the comment.

>>> +	set x $*

>> Uh?  Did you mean “set -- $*”?

> Nope, set in some shells don't know '--' (now I'm following the most
> portable code snippet principle) so with 'x' and just referencing
> positional parameters with number one larger than with '--' does the
> trick.

Interesting.  I must start doing that. :)

>>> +	manthdata `sed -n '/^[.]TH NOTMUCH/ { y/"/ /; p; }' "$mp"`

>> Alternatively “\.” instead of “[.]”.

> I prefer [.] as a more robust alternative -- backslashes may get 
> "expanded away" in some cases; using [.] just frees me from checking
> whether that may happen.

I guess it's a valid point.

> Thanks for the review, I'll probably send new version tomorrow...

Sure thing.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

Thread: