threads / patch / 9971

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

Subject: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

## tl;dr

45 messages between Sep 21, 2007 and Sep 25, 2007. Diffs are folded; open one to read it.

replies: 44people: 11as markdown or json

Eygene Ryabinkin· Sep 21, 2007, 21:43 UTC · lore
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.

Signed-off-by: Eygene Ryabinkin <rea-git@codelabs.ru>
---
 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(-)
Show changes to 16 files +16 −16

git-am.sh, git-clean.sh, git-commit.sh, git-fetch.sh, git-filter-branch.sh, git-instaweb.sh, git-ls-remote.sh, git-merge.sh, git-mergetool.sh, git-pull.sh, git-quiltimport.sh, git-rebase--interactive.sh, git-rebase.sh, git-repack.sh, git-reset.sh, git-submodule.sh

diff --git a/git-am.sh b/git-am.sh
index 6809aa0..0bd8d34 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -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=*)
diff --git a/git-clean.sh b/git-clean.sh
index a5cfd9f..1fac731 100755
--- a/git-clean.sh
+++ b/git-clean.sh
@@ -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)
diff --git a/git-commit.sh b/git-commit.sh
index bb113e8..5f298c1 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -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)
diff --git a/git-fetch.sh b/git-fetch.sh
index c3a2001..dac2d72 100755
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -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)
diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index a4b6577..02b567b 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -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
 	--)
diff --git a/git-instaweb.sh b/git-instaweb.sh
index b79c6b6..c85f8c0 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -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)
diff --git a/git-ls-remote.sh b/git-ls-remote.sh
index b7e5d04..4ef4341 100755
--- a/git-ls-remote.sh
+++ b/git-ls-remote.sh
@@ -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)
diff --git a/git-merge.sh b/git-merge.sh
index 3a01db0..94a50aa 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -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|\
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 47a8055..0e286dd 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -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*)
diff --git a/git-pull.sh b/git-pull.sh
index 5e96d1f..722ed4e 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -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|\
diff --git a/git-quiltimport.sh b/git-quiltimport.sh
index 9de54d1..4039617 100755
--- a/git-quiltimport.sh
+++ b/git-quiltimport.sh
@@ -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=*)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index abc2b1c..54e4299 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -317,7 +317,7 @@ do_rest () {
 	done
 }
 
-while case $# in 0) break ;; esac
+while case $# in 0) break ;; *) : ;; esac
 do
 	case "$1" in
 	--continue)
diff --git a/git-rebase.sh b/git-rebase.sh
index 3bd66b0..f7ae22c 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -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"
diff --git a/git-repack.sh b/git-repack.sh
index 156c5e8..aac771e 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -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 ;;
diff --git a/git-reset.sh b/git-reset.sh
index 1dc606f..eb92610 100755
--- a/git-reset.sh
+++ b/git-reset.sh
@@ -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)
diff --git a/git-submodule.sh b/git-submodule.sh
index 3320998..78a25ad 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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
Junio C Hamano· Sep 21, 2007, 23:52 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

Eygene Ryabinkin <rea-git@codelabs.ru> writes:
Show 17 quoted lines
> 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.

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.

David Kastrup· Sep 22, 2007, 00:05 UTC · re: Junio C Hamano · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

Junio C Hamano <gitster@pobox.com> writes:
> 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.
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
Junio C Hamano· Sep 22, 2007, 00:18 UTC · re: David Kastrup · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

David Kastrup <dak@gnu.org> writes:
> But doing a break inside of the while _condition_ rather than the body
> just feels wrong to me.

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.

Junio C Hamano· Sep 22, 2007, 00:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

Junio C Hamano <gitster@pobox.com> writes:
Show 8 quoted lines
> David Kastrup <dak@gnu.org> writes:
>
>> But doing a break inside of the while _condition_ rather than the body
>> just feels wrong to me.
>
> 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.

David Kastrup· Sep 22, 2007, 00:26 UTC · re: Junio C Hamano · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
> David Kastrup <dak@gnu.org> writes:
>
>> But doing a break inside of the while _condition_ rather than the body
>> just feels wrong to me.
>
> Sorry, but that is not the issue on the thread is about.
It is still relevant:
> BSD shell is failing the whole case statement when there is no
> matching case arm.

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 C Hamano· Sep 22, 2007, 02:33 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

Eygene Ryabinkin <rea-git@codelabs.ru> writes:
> Good day.
>
> I had found that FreeBSD's /bin/sh refuses to work with git 1.5.3.2.
> correctly: no flags are recognized.
Show 7 quoted lines
> @@ -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

I am assuming that this works around _a_ bug in that /bin/sh; I would make sure I understand the nature of the bug. Is it Ok to understand that with that shell, after this construct runs:

	case <some word> in
        <case arm #1>)
        	something ;;
	<case arm #2>)
        	something else ;;
	esac

the status from the whole case statement is false, when <some word> does not match any of the glob patterns listed in any of the case arm?

That is, what does the shell say if you do this?
	case Ultra in
        Super)
        	false ;;
	Hyper)
        	true ;;
	esac &&
        echo case returned ok
The reason I ask is because
	while case $# in 0) ... esac
        do
        	...
	done

is not the only place the status from "case" itself matters in our scripts. There are places that do

	something &&
	case ... in
        ...
        esac &&
        something else

and we would need to add no-op match-everything arm to all of such case statements in our scripts.

Besides test scripts, there is one in git-ls-remote.sh which you seem to have missed.

Eygene Ryabinkin· Sep 22, 2007, 03:54 UTC · re: Junio C Hamano · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

Junio, good day.
Fri, Sep 21, 2007 at 04:52:52PM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> > 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.
> 
> 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.

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?

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

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.

Show 9 quoted lines
> 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.

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.

Fri, Sep 21, 2007 at 07:33:21PM -0700, Junio C Hamano wrote:
Show 23 quoted lines
> I am assuming that this works around _a_ bug in that /bin/sh; I
> would make sure I understand the nature of the bug.  Is it Ok to
> understand that with that shell, after this construct runs:
> 
> 	case <some word> in
>         <case arm #1>)
>         	something ;;
> 	<case arm #2>)
>         	something else ;;
> 	esac
> 
> the status from the whole case statement is false, when <some word>
> does not match any of the glob patterns listed in any of the case arm?
> 
> That is, what does the shell say if you do this?
> 
> 	case Ultra in
>         Super)
>         	false ;;
> 	Hyper)
>         	true ;;
> 	esac &&
>         echo case returned ok

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
David Kastrup· Sep 22, 2007, 04:06 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

Eygene Ryabinkin <rea-git@codelabs.ru> writes:
Show 12 quoted lines
>> That is, what does the shell say if you do this?
>> 
>> 	case Ultra in
>>         Super)
>>         	false ;;
>> 	Hyper)
>>         	true ;;
>> 	esac &&
>>         echo case returned ok
>
> It says 'case returned ok', so I will try to understand why it
> works here and does not work in the 'while' construct.
What you actually need to do is
false
case Ultra in
   Super)
   	false ;;
Hyper)
   	true ;;
esac && echo case returned ok
-- 
David Kastrup
Junio C Hamano· Sep 22, 2007, 06:58 UTC · re: David Kastrup · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

David Kastrup <dak@gnu.org> writes:
Show 24 quoted lines
> Eygene Ryabinkin <rea-git@codelabs.ru> writes:
>
>>> That is, what does the shell say if you do this?
>>> 
>>> 	case Ultra in
>>>         Super)
>>>         	false ;;
>>> 	Hyper)
>>>         	true ;;
>>> 	esac &&
>>>         echo case returned ok
>>
>> It says 'case returned ok', so I will try to understand why it
>> works here and does not work in the 'while' construct.
>
> What you actually need to do is
>
> false
> case Ultra in
>    Super)
>    	false ;;
> Hyper)
>    	true ;;
> esac && echo case returned ok
AHHHHHH.
Is "case" supposed to be transparent?
Vineet Kumar· Sep 22, 2007, 07:34 UTC · re: Junio C Hamano · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

* Junio C Hamano (gitster@pobox.com) [070921 23:58]:
Show 30 quoted lines
> David Kastrup <dak@gnu.org> writes:
> 
> > Eygene Ryabinkin <rea-git@codelabs.ru> writes:
> >
> >>> That is, what does the shell say if you do this?
> >>> 
> >>> 	case Ultra in
> >>>         Super)
> >>>         	false ;;
> >>> 	Hyper)
> >>>         	true ;;
> >>> 	esac &&
> >>>         echo case returned ok
> >>
> >> It says 'case returned ok', so I will try to understand why it
> >> works here and does not work in the 'while' construct.
> >
> > What you actually need to do is
> >
> > false
> > case Ultra in
> >    Super)
> >    	false ;;
> > Hyper)
> >    	true ;;
> > esac && echo case returned ok
> 
> 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/
David Kastrup· Sep 22, 2007, 11:07 UTC · re: Junio C Hamano · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

Junio C Hamano <gitster@pobox.com> writes:
Show 30 quoted lines
> David Kastrup <dak@gnu.org> writes:
>
>> Eygene Ryabinkin <rea-git@codelabs.ru> writes:
>>
>>>> That is, what does the shell say if you do this?
>>>> 
>>>> 	case Ultra in
>>>>         Super)
>>>>         	false ;;
>>>> 	Hyper)
>>>>         	true ;;
>>>> 	esac &&
>>>>         echo case returned ok
>>>
>>> It says 'case returned ok', so I will try to understand why it
>>> works here and does not work in the 'while' construct.
>>
>> What you actually need to do is
>>
>> false
>> case Ultra in
>>    Super)
>>    	false ;;
>> Hyper)
>>    	true ;;
>> esac && echo case returned ok
>
> AHHHHHH.
>
> Is "case" supposed to be transparent?
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
Junio C Hamano· Sep 22, 2007, 06:54 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

Eygene Ryabinkin <rea-git@codelabs.ru> writes:
Show 15 quoted lines
> By the way, my FreeBSD is 7-CURRENT, but I'll test on 6-STABLE
> and perhaps on 4-STABLE on Monday.
> ...
>> That is, what does the shell say if you do this?
>> 
>> 	case Ultra in
>>         Super)
>>         	false ;;
>> 	Hyper)
>>         	true ;;
>> 	esac &&
>>         echo case returned ok
>
> It says 'case returned ok', so I will try to understand why it
> works here and does not work in the 'while' construct.

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.

Adam Flott· Sep 22, 2007, 21:37 UTC · re: Junio C Hamano · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

On Fri, 21 Sep 2007, Junio C Hamano wrote:
Show 5 quoted lines
> 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.
That is what I did notice, just accessing $# fixed later uses of it.
Show 5 quoted lines
> 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.

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
Junio C Hamano· Sep 22, 2007, 08:32 UTC · re: Eygene Ryabinkin · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

Eygene Ryabinkin <rea-git@codelabs.ru> writes:
> 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?

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.

Eygene Ryabinkin· Sep 23, 2007, 08:31 UTC · re: Junio C Hamano · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

Junio, *, good day.
Sat, Sep 22, 2007 at 01:32:38AM -0700, Junio C Hamano wrote:
Show 13 quoted lines
> > 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?
> 
> 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".
Yes, thanks for the pointer.
Show 10 quoted lines
> 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.
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
David Kastrup· Sep 23, 2007, 08:59 UTC · re: Junio C Hamano · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> Eygene Ryabinkin <rea-git@codelabs.ru> writes:
>
>> 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?
>
> 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.
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
Junio C Hamano· Sep 23, 2007, 19:20 UTC · re: David Kastrup · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

David Kastrup <dak@gnu.org> writes:
Show 6 quoted lines
> 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,...
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.
David Kastrup· Sep 23, 2007, 19:33 UTC · re: Junio C Hamano · lore

Re: [PATCH] Allow shell scripts to run with non-Bash /bin/sh

Junio C Hamano <gitster@pobox.com> writes:
Show 10 quoted lines
> David Kastrup <dak@gnu.org> writes:
>
>> 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,...
>
> I do not have any problem with your approach at all.
Too bad sh does... I need to write
while :; do case xxx) break; esac
instead which is slightly uglier concerning visual aesthetics.
> But on one condition, however.  If it is done correctly with
> double semi-colons before "esac" ;-)

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
David Kastrup· Sep 23, 2007, 20:42 UTC · re: David Kastrup · lore

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

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.

Signed-off-by: David Kastrup <dak@gnu.org>
---

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(-)
Show changes to 19 files +23 −23

contrib/examples/git-gc.sh, contrib/examples/git-reset.sh, contrib/examples/git-tag.sh, contrib/examples/git-verify-tag.sh, git-am.sh, git-clean.sh, git-commit.sh, git-fetch.sh, git-filter-branch.sh, git-instaweb.sh, git-ls-remote.sh, git-merge.sh, git-mergetool.sh, git-pull.sh, git-quiltimport.sh, git-rebase--interactive.sh, git-rebase.sh, git-repack.sh, git-submodule.sh

diff --git a/contrib/examples/git-gc.sh b/contrib/examples/git-gc.sh
index 2ae235b..1597e9f 100755
--- a/contrib/examples/git-gc.sh
+++ b/contrib/examples/git-gc.sh
@@ -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)
diff --git a/contrib/examples/git-reset.sh b/contrib/examples/git-reset.sh
index 1dc606f..bafeb52 100755
--- a/contrib/examples/git-reset.sh
+++ b/contrib/examples/git-reset.sh
@@ -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)
diff --git a/contrib/examples/git-tag.sh b/contrib/examples/git-tag.sh
index 5ee3f50..7bb7486 100755
--- a/contrib/examples/git-tag.sh
+++ b/contrib/examples/git-tag.sh
@@ -14,7 +14,7 @@ username=
 list=
 verify=
 LINES=0
-while case "$#" in 0) break ;; esac
+while test "$#" != 0
 do
     case "$1" in
     -a)
diff --git a/contrib/examples/git-verify-tag.sh b/contrib/examples/git-verify-tag.sh
index 37b0023..0902a5c 100755
--- a/contrib/examples/git-verify-tag.sh
+++ b/contrib/examples/git-verify-tag.sh
@@ -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)
diff --git a/git-am.sh b/git-am.sh
index 6809aa0..d97b528 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -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=*)
diff --git a/git-clean.sh b/git-clean.sh
index a5cfd9f..e847ae2 100755
--- a/git-clean.sh
+++ b/git-clean.sh
@@ -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)
diff --git a/git-commit.sh b/git-commit.sh
index 3e46dbb..dcdd49b 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -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)
diff --git a/git-fetch.sh b/git-fetch.sh
index c3a2001..dc6483f 100755
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -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)
diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index a4b6577..b7b2ef7 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -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
diff --git a/git-instaweb.sh b/git-instaweb.sh
index b79c6b6..364cf93 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -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)
diff --git a/git-ls-remote.sh b/git-ls-remote.sh
index b7e5d04..e6e9760 100755
--- a/git-ls-remote.sh
+++ b/git-ls-remote.sh
@@ -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)
diff --git a/git-merge.sh b/git-merge.sh
index 3a01db0..e4116a8 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -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|\
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 47a8055..a0e44f7 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -268,7 +268,7 @@ merge_file () {
     cleanup_temp_files
 }
 
-while case $# in 0) break ;; esac
+while test $# != 0
 do
     case "$1" in
 	-t|--tool*)
diff --git a/git-pull.sh b/git-pull.sh
index 5e96d1f..c3f05f5 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -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
diff --git a/git-quiltimport.sh b/git-quiltimport.sh
index 9de54d1..1ad9291 100755
--- a/git-quiltimport.sh
+++ b/git-quiltimport.sh
@@ -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=*)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index abc2b1c..2fa53fd 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -317,7 +317,7 @@ do_rest () {
 	done
 }
 
-while case $# in 0) break ;; esac
+while test $# != 0
 do
 	case "$1" in
 	--continue)
diff --git a/git-rebase.sh b/git-rebase.sh
index c9942f2..9779d34 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -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)
diff --git a/git-repack.sh b/git-repack.sh
index 156c5e8..77126cd 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -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 ;;
diff --git a/git-submodule.sh b/git-submodule.sh
index 3320998..a7180ad 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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
Junio C Hamano· Sep 23, 2007, 22:20 UTC · re: David Kastrup · lore

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

David Kastrup <dak@gnu.org> writes:
Show 22 quoted lines
> 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.
>
> Signed-off-by: David Kastrup <dak@gnu.org>
> ---
>
> 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.
Gaah, didn't I say I do NOT think it is an improvement?
> I consider breaking out of the condition instead of the
> body od the loop ugly,

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

> and the implied "true" value of the
> non-matching case is not really obvious to humans at first
> glance.
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 ;-).

Show 5 quoted lines
> -while case "$#" in 0) break ;; esac
> +while test "$#" != 0
>  do
>      case "$1" in
>      -a)
And let's not quote "$#".
David Kastrup· Sep 24, 2007, 06:22 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
> David Kastrup <dak@gnu.org> writes:
>
>> 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.
>
> Gaah, didn't I say I do NOT think it is an improvement?

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

Show 5 quoted lines
>> I consider breaking out of the condition instead of the
>> body od the loop ugly,
>
> Well, as we all know that we disagree on this point, stating
> what you consider one-sidedly here is quite inappropriate.

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."?
>> and the implied "true" value of the non-matching case is not really
>> obvious to humans at first glance.
>
> It is more like "if you do not know shell".
It is more to take in.  Believe me, I do 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 ;-).

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?

Show 7 quoted lines
>> -while case "$#" in 0) break ;; esac
>> +while test "$#" != 0
>>  do
>>      case "$1" in
>>      -a)
>
> And let's not quote "$#".

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
David Kastrup· Sep 24, 2007, 14:24 UTC · re: David Kastrup · lore

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

David Kastrup <dak@gnu.org> writes:
Show 15 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Well, as we all know that we disagree on this point, stating what
>> you consider one-sidedly here is quite inappropriate.
>
> 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."?
[...]
Show 20 quoted lines
>> In other words, I am somewhat disgusted with the first part of
>> your proposed commit log message, although I like what the patch
>> does ;-).
>
> 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?
>
>>> -while case "$#" in 0) break ;; esac
>>> +while test "$#" != 0
>>>  do
>>>      case "$1" in
>>>      -a)
>>
>> And let's not quote "$#".
>
> 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.

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
Junio C Hamano· Sep 24, 2007, 23:31 UTC · re: David Kastrup · lore

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

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

David Kastrup· Sep 25, 2007, 06:13 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 21 quoted lines
> 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).
Uh, it is the only version with a syntax error.
Show 9 quoted lines
> 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.
Basically
while A && B || break; do C && D && E || break; done
Show 10 quoted lines
> 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.

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.

Show 7 quoted lines
> 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.

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.

> The reason I took your patch and proposed commit log message
> (almost) as-is was because you rewrote "case" to "test".

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
Junio C Hamano· Sep 25, 2007, 06:29 UTC · re: David Kastrup · lore

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

David Kastrup <dak@gnu.org> writes:
> 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.

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

Johannes Schindelin· Sep 25, 2007, 10:33 UTC · re: Junio C Hamano · lore

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

Hi,
On Mon, 24 Sep 2007, Junio C Hamano wrote:
Show 9 quoted lines
> David Kastrup <dak@gnu.org> writes:
> 
> > 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.
> 
> Ah, I like that ;-)  It is obvious when you think about it, and it is so 
> true but in a very twisted way...
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

Avi Kivity· Sep 25, 2007, 10:46 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 13 quoted lines
> David Kastrup <dak@gnu.org> writes:
>
>   
>> 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.
>>     
>
> 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.
Mike Hommey· Sep 24, 2007, 06:05 UTC · re: David Kastrup · lore

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

On Sun, Sep 23, 2007 at 10:42:08PM +0200, David Kastrup wrote:
> -while case $# in 0) break ;; esac
> +while test $# != 0
Wouldn't -ne be better ?
Mike
David Kastrup· Sep 24, 2007, 06:26 UTC · re: Mike Hommey · lore

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

Mike Hommey <mh@glandium.org> writes:
Show 5 quoted lines
> On Sun, Sep 23, 2007 at 10:42:08PM +0200, David Kastrup wrote:
>> -while case $# in 0) break ;; esac
>> +while test $# != 0
>
> Wouldn't -ne be better ?
Why?
-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum
David Symonds· Sep 24, 2007, 06:30 UTC · re: David Kastrup · lore

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

On 24/09/2007, David Kastrup <dak@gnu.org> wrote:
Show 9 quoted lines
> Mike Hommey <mh@glandium.org> writes:
>
> > On Sun, Sep 23, 2007 at 10:42:08PM +0200, David Kastrup wrote:
> >> -while case $# in 0) break ;; esac
> >> +while test $# != 0
> >
> > Wouldn't -ne be better ?
>
> Why?

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

Dave.
David Kastrup· Sep 24, 2007, 07:57 UTC · re: David Symonds · lore

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

"David Symonds" <dsymonds@gmail.com> writes:
Show 13 quoted lines
> On 24/09/2007, David Kastrup <dak@gnu.org> wrote:
>> Mike Hommey <mh@glandium.org> writes:
>>
>> > On Sun, Sep 23, 2007 at 10:42:08PM +0200, David Kastrup wrote:
>> >> -while case $# in 0) break ;; esac
>> >> +while test $# != 0
>> >
>> > Wouldn't -ne be better ?
>>
>> Why?
>
> Because -ne does a numeric comparison, != does a string comparison,
> and it's a numeric comparison happening, semantically speaking.

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
Pierre Habouzit· Sep 24, 2007, 08:01 UTC · re: David Kastrup · lore

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

On Mon, Sep 24, 2007 at 07:57:31AM +0000, David Kastrup wrote:
Show 19 quoted lines
> "David Symonds" <dsymonds@gmail.com> writes:
> 
> > On 24/09/2007, David Kastrup <dak@gnu.org> wrote:
> >> Mike Hommey <mh@glandium.org> writes:
> >>
> >> > On Sun, Sep 23, 2007 at 10:42:08PM +0200, David Kastrup wrote:
> >> >> -while case $# in 0) break ;; esac
> >> >> +while test $# != 0
> >> >
> >> > Wouldn't -ne be better ?
> >>
> >> Why?
> >
> > Because -ne does a numeric comparison, != does a string comparison,
> > and it's a numeric comparison happening, semantically speaking.
> 
> 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.
  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                                                madcoder@debian.org
OOO                                                http://www.madism.org
Pierre Habouzit· Sep 24, 2007, 08:04 UTC · re: Pierre Habouzit · lore

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

On Mon, Sep 24, 2007 at 08:01:34AM +0000, Pierre Habouzit wrote:
Show 26 quoted lines
> On Mon, Sep 24, 2007 at 07:57:31AM +0000, David Kastrup wrote:
> > "David Symonds" <dsymonds@gmail.com> writes:
> > 
> > > On 24/09/2007, David Kastrup <dak@gnu.org> wrote:
> > >> Mike Hommey <mh@glandium.org> writes:
> > >>
> > >> > On Sun, Sep 23, 2007 at 10:42:08PM +0200, David Kastrup wrote:
> > >> >> -while case $# in 0) break ;; esac
> > >> >> +while test $# != 0
> > >> >
> > >> > Wouldn't -ne be better ?
> > >>
> > >> Why?
> > >
> > > Because -ne does a numeric comparison, != does a string comparison,
> > > and it's a numeric comparison happening, semantically speaking.
> > 
> > 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.
> 
>   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")).
  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                                                madcoder@debian.org
OOO                                                http://www.madism.org
Johannes Schindelin· Sep 24, 2007, 10:33 UTC · re: Pierre Habouzit · lore

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

Hi,
On Mon, 24 Sep 2007, Pierre Habouzit wrote:
Show 30 quoted lines
> On Mon, Sep 24, 2007 at 08:01:34AM +0000, Pierre Habouzit wrote:
> > On Mon, Sep 24, 2007 at 07:57:31AM +0000, David Kastrup wrote:
> > > "David Symonds" <dsymonds@gmail.com> writes:
> > > 
> > > > On 24/09/2007, David Kastrup <dak@gnu.org> wrote:
> > > >> Mike Hommey <mh@glandium.org> writes:
> > > >>
> > > >> > On Sun, Sep 23, 2007 at 10:42:08PM +0200, David Kastrup wrote:
> > > >> >> -while case $# in 0) break ;; esac
> > > >> >> +while test $# != 0
> > > >> >
> > > >> > Wouldn't -ne be better ?
> > > >>
> > > >> Why?
> > > >
> > > > Because -ne does a numeric comparison, != does a string comparison,
> > > > and it's a numeric comparison happening, semantically speaking.
> > > 
> > > 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.
> > 
> >   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")).
> 
>   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).

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

Miles Bader· Sep 24, 2007, 11:21 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 13 quoted lines
>> >   $# != 0 would yield sth like (strcmp(sprintf("%d", argc), "0"))
>> >   $# -ne 0 would yield sth like (argc != atoi("0")).
>> 
>>   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).
>
> 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).

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
Eygene Ryabinkin· Sep 24, 2007, 11:35 UTC · re: Miles Bader · lore

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

Miles,
Mon, Sep 24, 2007 at 08:21:42PM +0900, Miles Bader wrote:
Show 10 quoted lines
> > 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).
> 
> The comment "... holds only for a shell where [ is a builtin" doesn't
> make any sense to me

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

> 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.

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
Miles Bader· Sep 24, 2007, 13:45 UTC · re: Eygene Ryabinkin · lore

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

Eygene Ryabinkin <rea-git@codelabs.ru> writes:
Show 7 quoted lines
>> The comment "... holds only for a shell where [ is a builtin" doesn't
>> make any sense to me
>
> 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.

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.

> I believe that this trick comes from the old days where people were
> generally much more eager to save CPU cycles than now ;))

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]
David Kastrup· Sep 24, 2007, 13:58 UTC · re: Miles Bader · lore

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

Miles Bader <miles@gnu.org> writes:
Show 12 quoted lines
> Eygene Ryabinkin <rea-git@codelabs.ru> writes:
>>> The comment "... holds only for a shell where [ is a builtin" doesn't
>>> make any sense to me
>>
>> 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.
>
> 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.

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

-- 
David Kastrup
Johannes Schindelin· Sep 24, 2007, 14:04 UTC · re: Miles Bader · lore

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

Hi,
On Mon, 24 Sep 2007, Miles Bader wrote:
Show 12 quoted lines
> Eygene Ryabinkin <rea-git@codelabs.ru> writes:
> >> The comment "... holds only for a shell where [ is a builtin" doesn't
> >> make any sense to me
> >
> > 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.
> 
> 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.
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

David Kastrup· Sep 24, 2007, 14:10 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 21 quoted lines
> Hi,
>
> On Mon, 24 Sep 2007, Miles Bader wrote:
>
>> Eygene Ryabinkin <rea-git@codelabs.ru> writes:
>> >> The comment "... holds only for a shell where [ is a builtin" doesn't
>> >> make any sense to me
>> >
>> > 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.
>> 
>> 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.
>
> AAAAAAAAAAAAAARRRRRGGGHHHHHHHHHHHH!
>
> _Exactly_ the same reasoning can be said about the old code: _every_ 
> reasonable shell can grok the code that used to be there!

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
Miles Bader· Sep 24, 2007, 14:58 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 8 quoted lines
>> 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.
>
> AAAAAAAAAAAAAARRRRRGGGHHHHHHHHHHHH!
>
> _Exactly_ the same reasoning can be said about the old code: _every_ 
> reasonable shell can grok the code that used to be there!

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.
Junio C Hamano· Sep 24, 2007, 16:24 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 14 quoted lines
> On Mon, 24 Sep 2007, Miles Bader wrote:
>> ...
>> 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.
>
> 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>
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 "!=".

David Kastrup· Sep 24, 2007, 11:39 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 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,
It didn't yet.
> 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).

"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
David Kastrup· Sep 24, 2007, 08:29 UTC · re: Pierre Habouzit · lore

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

Pierre Habouzit <madcoder@debian.org> writes:
Show 22 quoted lines
> On Mon, Sep 24, 2007 at 07:57:31AM +0000, David Kastrup wrote:
>> "David Symonds" <dsymonds@gmail.com> writes:
>> 
>> > On 24/09/2007, David Kastrup <dak@gnu.org> wrote:
>> >> Mike Hommey <mh@glandium.org> writes:
>> >>
>> >> > On Sun, Sep 23, 2007 at 10:42:08PM +0200, David Kastrup wrote:
>> >> >> -while case $# in 0) break ;; esac
>> >> >> +while test $# != 0
>> >> >
>> >> > Wouldn't -ne be better ?
>> >>
>> >> Why?
>> >
>> > Because -ne does a numeric comparison, != does a string comparison,
>> > and it's a numeric comparison happening, semantically speaking.
>> 
>> 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.
>
>   Fwiw $# already is a number.
It isn't.
> Hence test $# -ne 0 is definitely a better test.
/* 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);
}
>   $# != 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.

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

← back to recent threads