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

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

[Eygene Ryabinkin]

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

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

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

[Junio C Hamano]

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

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

[David Kastrup]

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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> 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

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

[Junio C Hamano]

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.

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

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

[Eygene Ryabinkin]

Junio, good day.

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

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

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

[David Kastrup]

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


-- 
David Kastrup

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

[Junio C Hamano]

Eygene Ryabinkin <rea-git@codelabs.ru> writes:

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

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

[Junio C Hamano]

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?

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

[Vineet Kumar]

* Junio C Hamano (gitster@pobox.com) [070921 23:58]:
> 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/

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

[Junio C Hamano]

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.

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

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> 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

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

[Adam Flott]

On Fri, 21 Sep 2007, Junio C Hamano wrote:

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

> 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

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

[Eygene Ryabinkin]

Junio, *, good day.

Sat, Sep 22, 2007 at 01:32:38AM -0700, Junio C Hamano wrote:
> > 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.

> 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

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

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> 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

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

[Junio C Hamano]

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.

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.

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

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> 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

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

[David Kastrup]

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

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

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

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

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

> -while case "$#" in 0) break ;; esac
> +while test "$#" != 0
>  do
>      case "$1" in
>      -a)

And let's not quote "$#".

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

[Mike Hommey]

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

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

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

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

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

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

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

[David Kastrup]

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?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

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

[David Symonds]

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.


Dave.

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

[David Kastrup]

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

-- 
David Kastrup

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

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

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

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1382 bytes --]

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

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[David Kastrup]

Pierre Habouzit <madcoder@debian.org> writes:

> 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

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

[Johannes Schindelin]

Hi,

On Mon, 24 Sep 2007, Pierre Habouzit wrote:

> 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

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

[Miles Bader]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> >   $# != 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

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

[Eygene Ryabinkin]

Miles,

Mon, Sep 24, 2007 at 08:21:42PM +0900, Miles Bader wrote:
> > 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

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

[David Kastrup]

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

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

[Miles Bader]

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.

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

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

[David Kastrup]

Miles Bader <miles@gnu.org> writes:

> 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

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

[Johannes Schindelin]

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!

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

Ciao,
Dscho

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

[David Kastrup]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 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

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

[David Kastrup]

David Kastrup <dak@gnu.org> writes:

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

[...]

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

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

[Miles Bader]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 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.

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 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 "!=".

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

[Junio C Hamano]

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

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

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

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

> 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

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

> 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

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

[Junio C Hamano]

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

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

[Johannes Schindelin]

Hi,

On Mon, 24 Sep 2007, Junio C Hamano wrote:

> 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

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

[Avi Kivity]

Junio C Hamano wrote:
> 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.