Re: [PATCH] Supplant the "while case ... break ;; esac" idiom

[Junio C Hamano <gitster@pobox.com>]

David Kastrup <dak@gnu.org> writes:

> I am somewhat taken aback that a commit message considered offensive
> (though I still have a problem understanding why and certainly did not
> intend this) has been committed into master without giving me a chance
> to amend it.

Heh, that's simple.  I changed my mind ;-)

When A and B test for preconditions, and C, D, and E are
operations with error reports as their side effects, we can
write our loop in these forms:

 (1) while A && B && C && D && E || false; do :; done
 (2) while A && B && C && D && E || break; do :; done
 (3) while A && B; do C && D && E || break; do :; done
 (4) while :; do A && B && C && D && E || break; done

and all of them are equivalent.

But obviously the only sane version is (3).

If your complaint were against things like (1) and (2), I would
have completely agreed with you.  If you want "effects", you do
so between do and done.  Although you can use break between do
and done if you need to conditionally break out of the loop
after causing some effect there, between while and do is where
you are only supposed to decide if you want to break out of the
loop without causing "effects".

But what you were complaining about was different.

If we were to ignore broken shells that do not return success
from a case statement with no matching pattern, the following
two are equivalent:

	while case "$sth" in foo) break ;; esac; do ...; done
	while case "$sth" in foo) false ;; esac; do ...; done

Their "case" are used to decide if you want to break out of the
loop; the former is (1) being a bit more explicit, and (2) used
to be a bit more efficient when false was not built-in.

Now the latter reason is mostly historical and it is not a valid
reason to choose the former over the latter anymore.  But that
does not make it any more confusing than the latter to a person
who knows what "break" means in a loop.  An explicit 'break' is
still more, eh,... explicit ;-)

But the "break" never was the issue.  Return value of "case"
was.

The reason I took your patch and proposed commit log message
(almost) as-is was because you rewrote "case" to "test".  That
IS an improvement, especially in the presense of a shell in the
field that does not implement case statement correctly, and you
talk about that in the later part of the commit log message.

The only "offending" part was "I consider...ugly", which is your
opinion but I think you as the patch author deserve to express
that.  I do not think it would not have helped the FreeBSD shell
a bit if you removed that "ugliness" by merely replacing "break"
with "false", so I think the comment was not just offending but
irrelevant, though.

All the rest of your commit message is correct.  The spec of
"case" might not be obvious to everybody that it ought to return
success when no pattern matched.  And I found your wording to
fold the bug decription of some BSD shells there amusing ;-)