Allow shell scripts to run with non-Bash /bin/sh

Good day.

I had found that FreeBSD's /bin/sh refuses to work with git 1.5.3.2.
correctly: no flags are recognized. The details and fix are below. I don't currently know if the Bash's behaviour is POSIXly correct
or the 'case' statement semantics is not very well defined. But
the following patch fixes the things for the FreeBSD.

Here we go.

-----

Option parsing in the Git shell scripts uses the construct 'while case "$#" in 0) break ;; esac; do ... done'. This is neat, because it needs no external commands invocation. But in the case when /bin/sh is not GNU Bash (for example, on FreeBSD) this cycle will not be executed at all.

The fix is to add the case branch '*) : ;;'. It also needs no external commands invocation and it does its work, because ':' always returns zero.

---
 git-am.sh                  |    2 +-
 git-clean.sh               |    2 +-
 git-commit.sh              |    2 +-
 git-fetch.sh               |    2 +-
 git-filter-branch.sh       |    2 +-
 git-instaweb.sh            |    2 +-
 git-ls-remote.sh           |    2 +-
 git-merge.sh               |    2 +-
 git-mergetool.sh           |    2 +-
 git-pull.sh                |    2 +-
 git-quiltimport.sh         |    2 +-
 git-rebase--interactive.sh |    2 +-
 git-rebase.sh              |    2 +-
 git-repack.sh              |    2 +-
 git-reset.sh               |    2 +-
 git-submodule.sh           |    2 +-
 16 files changed, 16 insertions(+), 16 deletions(-)

index 6809aa0..0bd8d34 100755
@@ -109,7 +109,7 @@ dotest=.dotest sign= utf8=t keep= skip= interactive= resolved= binary=
 resolvemsg= resume=
 git_apply_opt=
 
-while case "$#" in 0) break;; esac
+while case "$#" in 0) break;; *) : ;; esac
 do
 	case "$1" in
 	-d=*|--d=*|--do=*|--dot=*|--dote=*|--dotes=*|--dotest=*)

index a5cfd9f..1fac731 100755
@@ -26,7 +26,7 @@ rmrf="rm -rf --"
 rm_refuse="echo Not removing"
 echo1="echo"
 
-while case "$#" in 0) break ;; esac
+while case "$#" in 0) break ;; *) : ;; esac
 do
 	case "$1" in
 	-d)

index bb113e8..5f298c1 100755
@@ -89,7 +89,7 @@ force_author=
 only_include_assumed=
 untracked_files=
 templatefile="`git config commit.template`"
-while case "$#" in 0) break;; esac
+while case "$#" in 0) break;; *) : ;; esac
 do
 	case "$1" in
 	-F|--F|-f|--f|--fi|--fil|--file)

index c3a2001..dac2d72 100755
@@ -27,7 +27,7 @@ shallow_depth=
 no_progress=
 test -t 1 || no_progress=--no-progress
 quiet=
-while case "$#" in 0) break ;; esac
+while case "$#" in 0) break ;; *) : ;; esac
 do
 	case "$1" in
 	-a|--a|--ap|--app|--appe|--appen|--append)

index a4b6577..02b567b 100755
@@ -105,7 +105,7 @@ filter_tag_name=
 filter_subdir=
 orig_namespace=refs/original/
 force=
-while case "$#" in 0) usage;; esac
+while case "$#" in 0) usage;; *) : ;; esac
 do
 	case "$1" in
 	--)

index b79c6b6..c85f8c0 100755
@@ -61,7 +61,7 @@ stop_httpd () {
 	test -f "$fqgitdir/pid" && kill `cat "$fqgitdir/pid"`
 }
 
-while case "$#" in 0) break ;; esac
+while case "$#" in 0) break ;; *) : ;; esac
 do
 	case "$1" in
 	--stop|stop)

index b7e5d04..4ef4341 100755
@@ -13,7 +13,7 @@ die () {
 }
 
 exec=
-while case "$#" in 0) break;; esac
+while case "$#" in 0) break;; *) : ;; esac
 do
   case "$1" in
   -h|--h|--he|--hea|--head|--heads)

index 3a01db0..94a50aa 100755
@@ -122,7 +122,7 @@ merge_name () {
 case "$#" in 0) usage ;; esac
 
 have_message=
-while case "$#" in 0) break ;; esac
+while case "$#" in 0) break ;; *) : ;; esac
 do
 	case "$1" in
 	-n|--n|--no|--no-|--no-s|--no-su|--no-sum|--no-summ|\

index 47a8055..0e286dd 100755
@@ -268,7 +268,7 @@ merge_file () {
     cleanup_temp_files
 }
 
-while case $# in 0) break ;; esac
+while case $# in 0) break ;; *) : ;; esac
 do
     case "$1" in
 	-t|--tool*)

index 5e96d1f..722ed4e 100755
@@ -16,7 +16,7 @@ test -z "$(git ls-files -u)" ||
 	die "You are in the middle of a conflicted merge."
 
 strategy_args= no_summary= no_commit= squash=
-while case "$#,$1" in 0) break ;; *,-*) ;; *) break ;; esac
+while case "$#,$1" in 0) break ;; *,-*) : ;; *) break ;; esac
 do
 	case "$1" in
 	-n|--n|--no|--no-|--no-s|--no-su|--no-sum|--no-summ|\

index 9de54d1..4039617 100755
@@ -5,7 +5,7 @@ SUBDIRECTORY_ON=Yes
 
 dry_run=""
 quilt_author=""
-while case "$#" in 0) break;; esac
+while case "$#" in 0) break;; *) : ;; esac
 do
 	case "$1" in
 	--au=*|--aut=*|--auth=*|--autho=*|--author=*)

index abc2b1c..54e4299 100755
@@ -317,7 +317,7 @@ do_rest () {
 	done
 }
 
-while case $# in 0) break ;; esac
+while case $# in 0) break ;; *) : ;; esac
 do
 	case "$1" in
 	--continue)

index 3bd66b0..f7ae22c 100755
@@ -122,7 +122,7 @@ finish_rb_merge () {
 
 is_interactive () {
 	test -f "$dotest"/interactive ||
-	while case $#,"$1" in 0,|*,-i|*,--interactive) break ;; esac
+	while case $#,"$1" in 0,|*,-i|*,--interactive) break ;; *) : ;; esac
 	do
 		shift
 	done && test -n "$1"

index 156c5e8..aac771e 100755
@@ -9,7 +9,7 @@ SUBDIRECTORY_OK='Yes'
 
 no_update_info= all_into_one= remove_redundant=
 local= quiet= no_reuse= extra=
-while case "$#" in 0) break ;; esac
+while case "$#" in 0) break ;; *) : ;; esac
 do
 	case "$1" in
 	-n)	no_update_info=t ;;

index 1dc606f..eb92610 100755
@@ -11,7 +11,7 @@ require_work_tree
 update= reset_type=--mixed
 unset rev
 
-while case $# in 0) break ;; esac
+while case $# in 0) break ;; *) : ;; esac
 do
 	case "$1" in
 	--mixed | --soft | --hard)

index 3320998..78a25ad 100755
@@ -251,7 +251,7 @@ modules_list()
 	done
 }
 
-while case "$#" in 0) break ;; esac
+while case "$#" in 0) break ;; *) : ;; esac
 do
 	case "$1" in
 	add)

1.5.3.2
-- 
Eygene

I do not doubt that "while case $# in 0) break ;; esac" does not work for your shell. But I think the above comment is grossly misleading.

Don't mention bash there. You sound as if you are blaming bashism, but the thing is, your shell is simply broken.

You have other choices than bash on BSD don't you?

My quick test shows that ksh, pdksh and dash seem to work correctly. This idiom is what I picked up around late 80's from somebody, and kept using on many variants of Unices. I would find quite surprising that something that claims to be a shell does not work correctly. Even /bin/sh that comes with Solaris seems to work correctly, which should tell you something.

OpenBSD's /bin/sh seems to be Ok; I do not know whose shell they use, but it seems to be hard-linked to /bin/ksh which is pdksh.

Personally, I find this idiom distasteful.

I'd do either
while case $# in 0) false ;; *) true esac

or, more likely
while : do case $# in 0) break;; esac

But doing a break inside of the while _condition_ rather than the body just feels wrong to me.

David Kastrup

Sorry, but that is not the issue on the thread is about. BSD shell is failing the whole case statement when there is no matching case arm.

I did not mean "BSD shell" in general here. The shell Eygene uses on his (unspecified version of) FreeBSD box is failing.

It is still relevant:

Sure, that is a bug. But in my opinion the idiom as such is ugly enough not be worth keeping anyhow. The proposal of the patch submitter was making an already ugly idiom even uglier for the sake of his shell. I agree that this is not the way to go.

I was proposing replacing the idiom by something which I find cleaner. That it will most likely work with the original poster's shell is more or less a side effect. At least it is a side effect that might motivate somebody else rather than me to do the cleanup work (and have a buggy test shell to see whether he got everything indeed).

David Kastrup

Junio, good day.

OK, you're right. Especially if /bin/sh from Solaris and OpenBSD are working and they are not Bash. But I would not tell that the shell is broken now -- I had not seen the POSIX specification. Does it specifies how the shell should work in this case?

Did not understand the question, sorry. The thing is that FreeBSD has /bin/sh that is derived from the original Berkeley shell. And it is desirable to have it working with Git script, since I don't want to make bash (or whatever shell that is not /bin/sh) a dependency for the port.

OK, I think I need to find out why FreeBSD's /bin/sh behaves like this, because the test you propose on your next message works. See below.

By the way, my FreeBSD is 7-CURRENT, but I'll test on 6-STABLE and perhaps on 4-STABLE on Monday.

It says 'case returned ok', so I will try to understand why it works here and does not work in the 'while' construct.

Thanks for the pointer!

Eygene

I vaguely recall somebody else had exactly this issue and he concluded that the shell was busted. I do not recall the details of the story but interestingly, if he did something that accesses "$#" before the problematic "while case $# in ..." the shell behaved for him in his experiments.

Just to make sure you do not misunderstand me, I am not trying to be difficult. I am trying to assess (1) if it is sensible to support that broken shell, and (2) if so what the exact breakage is, especially because as the above shows the breakage does not look like what your "fix" literally suggests, and what is involved in working it around.

Also by my comment about "/bin/sh and bash not being the only shells available on FreeBSD", I did not mean that you should change your /bin/sh. You can build git with SHELL_PATH make varilable pointing at a non-broken shell, which does not have to be installed as /bin/sh.

That is what I did notice, just accessing $# fixed later uses of it.

If one's installing from the ports tree, then the port should depend on a non-broken shell and set SHELL_PATH. And as for installing by hand, just print out a warning that SHELL_PATH points to a broken shell and be done with it. This is a FreeBSD bug, not a git one.

I had been meaning to write up a bug about this using a small test case, but I couldn't reproduce it.

Adam

I have always been assuming it to be the case (this construct is not my invention but is an old school idiom I just inherited from my mentor) and never looked at the spec recently, but I re-read it just to make sure. The answer is yes.

Visit http://www.opengroup.org/onlinepubs/000095399/ and follow "Shell and Utilities volume (XCU)" and then "Case conditional construct".

Exit Status

The exit status of case shall be zero if no patterns are matched. Otherwise, the exit status shall be the exit status of the last command executed in the compound-list.

So, as David suggests, if

        false
        case Ultra in
        Super) false ;;
        Hyper) true ;;
        esac && echo case returned ok

does not say "case returned ok", then the shell has a bit of problem.

Junio, *, good day.

Yes, thanks for the pointer.

Correct: the current /bin/sh for FreeBSD does not set zero exit code if no case patterns were matched. So, I apologize for my quick decision on the non-brokenness of the /bin/sh -- it is broken.

I had fixed the shell and filed the problem report. May be the change will be incorporated into the future release of FreeBSD. Meanwhile, I had added workarounds to the other places Junio mentioned in his follow-up and will try to push this patch to the FreeBSD port of Git. The explanation had been changed too ;))

Thanks to all people who helped me to realize what is wrong and where!

Eygene

Independent of that: would you mind a patch replacing that idiom with

while : do case xxx) break; esac

instead? I find breaking out of the condition rather than the body awkward, and I find a non-matching case statement, POSIX or not, quite unobvious in the place of a true while condition.

It is a bit too much of cleverness for my taste. Never mind that the current FreeBSD shell does not understand it due to being buggy: I find that this is not very readable to the human reader either without a double take.

David Kastrup, Kriemhildstr. 15, 44793 Bochum

I do not have any problem with your approach at all.

While I personally do not think it improves readability much, I do not think it hurts either. And it is a valid workaround for FBSD issue, so why not.

But on one condition, however. If it is done correctly with double semi-colons before "esac" ;-)

Thanks.

Too bad sh does... I need to write

while :; do case xxx) break; esac

instead which is slightly uglier concerning visual aesthetics.

My feeling about this is that double semi-colons before "esac" are redundant and similarly ugly as ";" before "end" in Pascal. However, looking at the existing git scripts it would seem that the redundant style is ubiquitous anyway, so it would be inconsistent to do this differently just within the idiom.

David Kastrup, Kriemhildstr. 15, 44793 Bochum

A lot of shell scripts contained stuff starting with

while case "$#" in 0) break ;; esac

and similar. I consider breaking out of the condition instead of the body od the loop ugly, and the implied "true" value of the non-matching case is not really obvious to humans at first glance. It happens not to be obvious to some BSD shells, either, but that's because they are not POSIX-compliant. In most cases, this has been replaced by a straight condition using "test". "case" has the advantage of being faster than "test" on vintage shells where "test" is not a builtin. Since none of them is likely to run the git scripts, anyway, the added readability should be worth the change.

A few loops have had their termination condition expressed differently.

---

Ok, this is not really what we have been talking about except in one case, but I think it is actually more of an improvement.

 contrib/examples/git-gc.sh         |    2 +-
 contrib/examples/git-reset.sh      |    2 +-
 contrib/examples/git-tag.sh        |    2 +-
 contrib/examples/git-verify-tag.sh |    2 +-
 git-am.sh                          |    2 +-
 git-clean.sh                       |    2 +-
 git-commit.sh                      |    2 +-
 git-fetch.sh                       |    2 +-
 git-filter-branch.sh               |    3 ++-
 git-instaweb.sh                    |    2 +-
 git-ls-remote.sh                   |    2 +-
 git-merge.sh                       |    2 +-
 git-mergetool.sh                   |    2 +-
 git-pull.sh                        |    6 +++---
 git-quiltimport.sh                 |    2 +-
 git-rebase--interactive.sh         |    2 +-
 git-rebase.sh                      |    5 ++---
 git-repack.sh                      |    2 +-
 git-submodule.sh                   |    2 +-
 19 files changed, 23 insertions(+), 23 deletions(-)

index 2ae235b..1597e9f 100755
@@ -9,7 +9,7 @@ SUBDIRECTORY_OK=Yes
 . git-sh-setup
 
 no_prune=:
-while case $# in 0) break ;; esac
+while test $# != 0
 do
 	case "$1" in
 	--prune)

index 1dc606f..bafeb52 100755
@@ -11,7 +11,7 @@ require_work_tree
 update= reset_type=--mixed
 unset rev
 
-while case $# in 0) break ;; esac
+while test $# != 0
 do
 	case "$1" in
 	--mixed | --soft | --hard)

index 5ee3f50..7bb7486 100755
@@ -14,7 +14,7 @@ username=
 list=
 verify=
 LINES=0
-while case "$#" in 0) break ;; esac
+while test "$#" != 0
 do
     case "$1" in
     -a)

index 37b0023..0902a5c 100755
@@ -5,7 +5,7 @@ SUBDIRECTORY_OK='Yes'
 . git-sh-setup
 
 verbose=
-while case $# in 0) break;; esac
+while test $# != 0
 do
 	case "$1" in
 	-v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)

index 6809aa0..d97b528 100755
@@ -109,7 +109,7 @@ dotest=.dotest sign= utf8=t keep= skip= interactive= resolved= binary=
 resolvemsg= resume=
 git_apply_opt=
 
-while case "$#" in 0) break;; esac
+while test "$#" != 0
 do
 	case "$1" in
 	-d=*|--d=*|--do=*|--dot=*|--dote=*|--dotes=*|--dotest=*)

index a5cfd9f..e847ae2 100755
@@ -26,7 +26,7 @@ rmrf="rm -rf --"
 rm_refuse="echo Not removing"
 echo1="echo"
 
-while case "$#" in 0) break ;; esac
+while test "$#" != 0
 do
 	case "$1" in
 	-d)

index 3e46dbb..dcdd49b 100755
@@ -89,7 +89,7 @@ force_author=
 only_include_assumed=
 untracked_files=
 templatefile="`git config commit.template`"
-while case "$#" in 0) break;; esac
+while test "$#" != 0
 do
 	case "$1" in
 	-F|--F|-f|--f|--fi|--fil|--file)

index c3a2001..dc6483f 100755
@@ -27,7 +27,7 @@ shallow_depth=
 no_progress=
 test -t 1 || no_progress=--no-progress
 quiet=
-while case "$#" in 0) break ;; esac
+while test "$#" != 0
 do
 	case "$1" in
 	-a|--a|--ap|--app|--appe|--appen|--append)

index a4b6577..b7b2ef7 100755
@@ -105,8 +105,9 @@ filter_tag_name=
 filter_subdir=
 orig_namespace=refs/original/
 force=
-while case "$#" in 0) usage;; esac
+while :
 do
+	test "$#" = 0 && usage
 	case "$1" in
 	--)
 		shift

index b79c6b6..364cf93 100755
@@ -61,7 +61,7 @@ stop_httpd () {
 	test -f "$fqgitdir/pid" && kill `cat "$fqgitdir/pid"`
 }
 
-while case "$#" in 0) break ;; esac
+while test "$#" != 0
 do
 	case "$1" in
 	--stop|stop)

index b7e5d04..e6e9760 100755
@@ -13,7 +13,7 @@ die () {
 }
 
 exec=
-while case "$#" in 0) break;; esac
+while test "$#" != 0
 do
   case "$1" in
   -h|--h|--he|--hea|--head|--heads)

index 3a01db0..e4116a8 100755
@@ -122,7 +122,7 @@ merge_name () {
 case "$#" in 0) usage ;; esac
 
 have_message=
-while case "$#" in 0) break ;; esac
+while test "$#" != 0
 do
 	case "$1" in
 	-n|--n|--no|--no-|--no-s|--no-su|--no-sum|--no-summ|\

index 47a8055..a0e44f7 100755
@@ -268,7 +268,7 @@ merge_file () {
     cleanup_temp_files
 }
 
-while case $# in 0) break ;; esac
+while test $# != 0
 do
     case "$1" in
 	-t|--tool*)

index 5e96d1f..c3f05f5 100755
@@ -16,7 +16,7 @@ test -z "$(git ls-files -u)" ||
 	die "You are in the middle of a conflicted merge."
 
 strategy_args= no_summary= no_commit= squash=
-while case "$#,$1" in 0) break ;; *,-*) ;; *) break ;; esac
+while :
 do
 	case "$1" in
 	-n|--n|--no|--no-|--no-s|--no-su|--no-sum|--no-summ|\

@@ -46,8 +46,8 @@ do
 	-h|--h|--he|--hel|--help)
 		usage
 		;;
-	-*)
-		# Pass thru anything that is meant for fetch.
+	*)
+		# Pass thru anything that may be meant for fetch.
 		break
 		;;
 	esac

index 9de54d1..1ad9291 100755
@@ -5,7 +5,7 @@ SUBDIRECTORY_ON=Yes
 
 dry_run=""
 quilt_author=""
-while case "$#" in 0) break;; esac
+while test "$#" != 0
 do
 	case "$1" in
 	--au=*|--aut=*|--auth=*|--autho=*|--author=*)

index abc2b1c..2fa53fd 100755
@@ -317,7 +317,7 @@ do_rest () {
 	done
 }
 
-while case $# in 0) break ;; esac
+while test $# != 0
 do
 	case "$1" in
 	--continue)

index c9942f2..9779d34 100755
@@ -122,15 +122,14 @@ finish_rb_merge () {
 
 is_interactive () {
 	test -f "$dotest"/interactive ||
-	while case $#,"$1" in 0,|*,-i|*,--interactive) break ;; esac
-	do
+	while :; do case $#,"$1" in 0,|*,-i|*,--interactive) break ;; esac
 		shift
 	done && test -n "$1"
 }
 
 is_interactive "$@" && exec git-rebase--interactive "$@"
 
-while case "$#" in 0) break ;; esac
+while test "$#" != 0
 do
 	case "$1" in
 	--continue)

index 156c5e8..77126cd 100755
@@ -9,7 +9,7 @@ SUBDIRECTORY_OK='Yes'
 
 no_update_info= all_into_one= remove_redundant=
 local= quiet= no_reuse= extra=
-while case "$#" in 0) break ;; esac
+while test "$#" != 0
 do
 	case "$1" in
 	-n)	no_update_info=t ;;

index 3320998..a7180ad 100755
@@ -251,7 +251,7 @@ modules_list()
 	done
 }
 
-while case "$#" in 0) break ;; esac
+while test "$#" != 0
 do
 	case "$1" in
 	add)

1.5.3.1.96.g4568

Why?

David Kastrup, Kriemhildstr. 15, 44793 Bochum

Because -ne does a numeric comparison, != does a string comparison, and it's a numeric comparison happening, semantically speaking.

Dave.

I don't see the point in converting $# and 0 into numbers before comparing them. "!=" is quite more readable, and the old code also compared the strings.

David Kastrup

Fwiw $# already is a number. Hence test $# -ne 0 is definitely a
better test.

$# != 0 would yield sth like (strcmp(sprintf("%d", argc), "0")) $# -ne 0 would yield sth like (argc != atoi("0")).

Not that it matters much, but the latter looks better to me.

·O·  Pierre Habouzit
··O                                                [email protected]
OOO                                                http://www.madism.org

Of course this holds only for shell where test/[ is a builtin, which
is the at least the case for zsh, bash, and dash (but not posh).

·O·  Pierre Habouzit
··O                                                [email protected]
OOO                                                http://www.madism.org

Hi,

The reason we used "case" is that this has always been a builtin (has to be, because it changes workflow).

Therefore I am somewhat uneasy that the patch went in so easily, especially given a message that flies in the face of our endeavours to make git less dependent on any given shell (as long as it is not broken to begin with).

Ciao,
Dscho

It didn't yet.

"test" is not actually a shell dependency since it is available as an external when not available as builtin. And if you really want to prefer "case" over "test" because the latter is not a built-in in a small number of shells, then it should be done consistently everywhere and not just in code I touch.

David Kastrup

The comment "... holds only for a shell where [ is a builtin" doesn't make any sense to me though: "-ne" is a standard operator even if "[" isn't a builtin. It's useful if you are testing numbers in potentially non-canonical form, e.g., with leading zeroes or something, and AFAIK is quite portable.

-Miles

Everywhere is walking distance if you have the time.  -- Steven Wright

Miles,

The 'while case ...' construct does not invoke any external commands. The 'while test ...' too, but only when 'test' is builtin. When 'test' is the external binary you get one additional fork/exec per each cycle.

I believe that this trick comes from the old days where people were generally much more eager to save CPU cycles than now ;)) But if you tried to run Cygwin on the moderately new machines (like PIII), you still can notice that commands like 'man' are a bit slow just because they require a bunch of preprocessors (and processes in pipe) to run. So, "save the process, kill the fork/exec" ;))

In general -- yes, it is portable and good for leading zeros. But in the case of $#, there will hardly be leading zeros or something else. And using '!=' typically saves you one atoi() + some other checks, since it typically translates to a bare strcmp().

Eygene

In practice that's not an issue though -- every reasonable shell has test as a builtin these days, so the "works when test is not a builtin" criteria is really important only for robustness.

Yes. I still occasionally find myself using "case" where if+test might be more natural, but I think it's basically an anachronism these days, and causes more harm by reducing readability than good.

-Miles

The car has become... an article of dress without which we feel uncertain,
unclad, and incomplete.  [Marshall McLuhan, Understanding Media, 1964]

Since the external test is pretty standardized, "works when test is one of various builtins" is actually more important for robustness...

David Kastrup

Hi,

AAAAAAAAAAAAAARRRRRGGGHHHHHHHHHHHH!

_Exactly_ the same reasoning can be said about the old code: _every_ reasonable shell can grok the code that used to be there!

<rhetoric-question>
So what exactly was your point again?
</rhetoric-question>

Ciao,
Dscho

The points are:

(1) The code used to be there is known to cause trouble with a deployed shell on FreeBSD of some vintage. It may be true that the shell is broken, but it does not matter much to the end user on such systems if breakage is in shell or in scripts --- the end result is that the user cannot benefit from git, and we already happen to know the workaround, which does not make the scripts less readable nor less portable;

(2) It's not like people who work on git scripts share the exact same style and tradition. While I do not personally think there is much readability improvements between the old code and the new code, if more people find the latter easier to work with, it's better to switch to the new code especially because there is no downside.

(2-a) Nobody finds the latter less readable nor impossible
to work with. Even I am not saying that; I only said I do not think it improves.

(2-b) git is not an educational project. It can be done
elsewhere in a UNIX history class, not here, to teach people that "case ... esac" used to be much more preferred over "test" because often the latter was not built-in and slower.

Regarding "$# != 0" vs "$# -ne 0", I agree with the patch by David. If the variable were "$something_else", then it might have been better to use the explicitly numeric form, but I think any seasoned shell people is much more used to see $# and $? (or $status after an earlier "status=$?") compared with numeric string with "=" or "!=".

There are no known shells that would not grok the proposed code, and the BSD shells don't grok the "code that used to be there".

So what point is there in preserving compatibility with some mystical non-specified shell while breaking compatibility with actually existing shells?

And by using a less human-readable idiom, to boot?

David Kastrup

As has been stated, that's not true. Some "real" shells don't handle the old code correctly (and the old code is less readable as well).

AFAIK, the new code works all cases, it's merely very slightly slower in unusual circumstances.

-Miles

/\ /\
(^.^)
(")")
*This is the cute kitty virus, please copy this into your sig so it can spread.

It isn't.

/* TEST/[ builtin. */
int
test_builtin (list)

     WORD_LIST *list;

{
char **argv;
int argc, result;

/* We let Matthew Bradburn and Kevin Braunsdorf's code do the
actual test command. So turn the list of args into an array of strings, since that is what their code wants. */
if (list == 0)

    {
      if (this_command_name[0] == '[' && !this_command_name[1])
	{
	  builtin_error ("missing `]'");
	  return (EX_BADUSAGE);
	}

      return (EXECUTION_FAILURE);
    }

  argv = make_builtin_argv  (list, &argc);
  result = test_command (argc, argv);
  free ((char *)argv);

return (result);
}

Not to me. The code does not support your argument, and all $x expansions certainly are strings, according to manual and usage. I will rework the patch this evening in order to get a commit message more placable to Junio, and I will at his request remove all of the (redundant) quoting. But removing quoting from $# does not turn it into a number: it remains the same string '0'. If someone else feels he should replace all "=" and "!=" tests for "numeric" comparisons with the unreadable numeric tests, he can go ahead proposing a separate patch that should not just cover $#.

You can have bash declare numeric variables, but even they are strings (they just auto-evaluate on assignment):

declare -i nonsense
nonsense="3+$(echo 4)"
echo "$nonsense"

gives 7, even though everything has been "strings" here.

David Kastrup

Gaah, didn't I say I do NOT think it is an improvement?

Well, as we all know that we disagree on this point, stating what you consider one-sidedly here is quite inappropriate.

It is more like "if you do not know shell".

In other words, I am somewhat disgusted with the first part of your proposed commit log message, although I like what the patch does ;-).

Ah, but I am not presuming to speak for you in my commit message and postings.

Hm. If I create a patch after you basically said "go ahead, I don't mind, but I consider it unimportant", how am I going to put the motivation for the patch in the commit message while expressing _your_ opinion? I thought that using "I" to make clear that it is my personal view would be doing that.

So what am I supposed to write instead?

"There is no good reason for this patch, but we might as well do it."?

It is more to take in. Believe me, I do know shell.

Could you propose a commit message that would be acceptable to you, yet not make it appear like a mistake to actually commit the patch?

I kept this as it was originally. Some authors prefer to quote every shell variable as a rule in order to avoid stupid syntactic things happening. Of course, $# never needs quoting, but I did not want to change the personal style of the respective authors. I can make this consistent if you want to.

David Kastrup, Kriemhildstr. 15, 44793 Bochum

[...]

It seems like the window of opportunity to fix the objectable commit message has closed for me, as well as doing the work of removing the "$#" (which you did already): I find that the patch has already made it into upstream.

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.

Unfortunately, the ensuing discussion around the _technical_ merits is somewhat lopsided since Dscho keeps me in his killfile, and so the commit message in the repository is all he'll ever be able to see from me concerning this matter.

Which makes it more unfortunate that I have not been able to amend it.

Too bad.

David Kastrup

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 ;-)

Uh, it is the only version with a syntax error.

Basically

while A && B || break; do C && D && E || break; done

As a completely irrelevant side note: the autoconf documentation mentions that "false" is more portable than "true" since calling it returns a non-zero exit status even when it is not installed or built-in.

I guess this has been a misunderstanding: for me, personally, the break was the issue: I don't like breaking out of a condition, since breaking for me is an action. I just used the fact that the BSD shells happen not to grok the constructs (and actually through a somewhat similar confusion between condition and action) to leverage my dislike of this construct and propose a patch.

Uhm, ok. It was a case of realizing "hm, this does not really look much nicer" before I chose to switch to "test". In fact, there is one case statement remaining which I rewrote in the previously discussed manner, and it did not strike me as being much prettier. So maybe I somewhat misjudged the core of my offended sense of aesthetics, but the impetus of the discussion still carried into the commit message.

Alea iacta est ("The SHA-1 has been established").

David Kastrup, Kriemhildstr. 15, 44793 Bochum

Ah, I like that ;-) It is obvious when you think about it, and it is so true but in a very twisted way...

You mean, it is not false but in a twisted way, don't you?

Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Hi,

But would you not have to redirect stderr to /dev/null, then?

In the same vein, we could replace "true" by "! false".

That's such a good idea that I'll go and make a patch.

Ciao,
Dscho

What you actually need to do is

false
case Ultra in
Super)
false ;;
Hyper)
true ;;
esac && echo case returned ok

David Kastrup

AHHHHHH.

Is "case" supposed to be transparent?

That doesn't seem to be the case (no pun intended) on either bash or dash. Here's what I tested on bash (apologies for the long lines; these are verbatim pastes from my shell):

vineet@sprocket:~$ false
vineet@sprocket:~$ case Super in Super) echo super ; false ;; Hyper) echo hyper ; true ;; esac && echo case returned ok
super
vineet@sprocket:~$ false
vineet@sprocket:~$ case Hyper in Super) echo super ; false ;; Hyper) echo hyper ; true ;; esac && echo case returned ok
hyper
case returned ok
vineet@sprocket:~$ false
vineet@sprocket:~$ case Ultra in Super) echo super ; false ;; Hyper) echo hyper ; true ;; esac && echo case returned ok
case returned ok
vineet@sprocket:~$ 

and on dash:

vineet@sprocket:~$ dash
$ false
$ case Super in Super) echo super ; false ;; Hyper) echo hyper ; true ;; esac && echo case returned ok
super
$ false
$ case Hyper in Super) echo super ; false ;; Hyper) echo hyper ; true ;; esac && echo case returned ok
hyper
case returned ok
$ false
$ case Ultra in Super) echo super ; false ;; Hyper) echo hyper ; true ;; esac && echo case returned ok
case returned ok
$ 

So it seems like a "case" statement isn't special; it returns a status like any other statement.

Vineet

http://www.doorstop.net/

Not that I would know. It is basically a revival of the

false
if false then : ; fi || echo "this fails!?!"

bug that probably has been fixed by now. For obvious reasons, conditionals without a taken branch are considered to have an exit code of 0.

David Kastrup, Kriemhildstr. 15, 44793 Bochum