Shell script cleanups/style changes?

Shell script cleanups/style changes?

[David Kastrup]

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


Hi, I wanted to ask what the general stance towards shell script
cleanups and simplifications would be.  For example, I find the expr
usage quite inscrutable in commit, and there is no necessity of
putting "shift" in every case branch instead of once behind it, and a
lot of conditionals and other manipulations can be made much easier on
the eye by using parameter expansion patterns that are, as far as I
can see, available with every reasonable Bourne Shell and clones.

Here is an example context diff (in this case, I find it more readable
than unified) to illustrate (untested!, please don't apply without a
regular formatted git patch).

Should I bother doing such cleanups as I read up on code, or should I
just leave things alone?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 6998 bytes --]

diff --git a/git-commit.sh b/git-commit.sh
index d7e7028..bdf20be 100755
*** a/git-commit.sh
--- b/git-commit.sh
***************
*** 97,101 ****
  		no_edit=t
  		log_given=t$log_given
  		logfile="$1"
- 		shift
  		;;
--- 97,100 ----
***************
*** 102,107 ****
  	-F*|-f*)
  		no_edit=t
  		log_given=t$log_given
! 		logfile=`expr "z$1" : 'z-[Ff]\(.*\)'`
! 		shift
  		;;
--- 101,105 ----
  	-F*|-f*)
  		no_edit=t
  		log_given=t$log_given
! 		logfile="${1#-?}"
  		;;
***************
*** 108,113 ****
  	--F=*|--f=*|--fi=*|--fil=*|--file=*)
  		no_edit=t
  		log_given=t$log_given
! 		logfile=`expr "z$1" : 'z-[^=]*=\(.*\)'`
! 		shift
  		;;
--- 106,110 ----
  	--F=*|--f=*|--fi=*|--fil=*|--file=*)
  		no_edit=t
  		log_given=t$log_given
! 		logfile="${1#*=}"
  		;;
***************
*** 114,117 ****
  	-a|--a|--al|--all)
  		all=t
- 		shift
  		;;
--- 111,113 ----
***************
*** 118,127 ****
  	--au=*|--aut=*|--auth=*|--autho=*|--author=*)
! 		force_author=`expr "z$1" : 'z-[^=]*=\(.*\)'`
! 		shift
  		;;
  	--au|--aut|--auth|--autho|--author)
  		case "$#" in 1) usage ;; esac
  		shift
  		force_author="$1"
- 		shift
  		;;
--- 114,121 ----
  	--au=*|--aut=*|--auth=*|--autho=*|--author=*)
! 		force_author="${1#*=}"
  		;;
  	--au|--aut|--auth|--autho|--author)
  		case "$#" in 1) usage ;; esac
  		shift
  		force_author="$1"
  		;;
***************
*** 128,144 ****
  	-e|--e|--ed|--edi|--edit)
  		edit_flag=t
- 		shift
  		;;
  	-i|--i|--in|--inc|--incl|--inclu|--includ|--include)
  		also=t
- 		shift
  		;;
  	--int|--inte|--inter|--intera|--interac|--interact|--interacti|\
  	--interactiv|--interactive)
  		interactive=t
- 		shift
  		;;
  	-o|--o|--on|--onl|--only)
  		only=t
- 		shift
  		;;
--- 122,134 ----
***************
*** 145,159 ****
  	-m|--m|--me|--mes|--mess|--messa|--messag|--message)
  		case "$#" in 1) usage ;; esac
- 		shift
  		log_given=m$log_given
! 		if test "$log_message" = ''
! 		then
! 		    log_message="$1"
! 		else
! 		    log_message="$log_message
  
! $1"
! 		fi
  		no_edit=t
- 		shift
  		;;
--- 135,142 ----
  	-m|--m|--me|--mes|--mess|--messa|--messag|--message)
  		case "$#" in 1) usage ;; esac
  		log_given=m$log_given
! 		log_message="${log_message}${log_message:+
  
! }$1"
  		no_edit=t
  		;;
***************
*** 160,172 ****
  	-m*)
  		log_given=m$log_given
! 		if test "$log_message" = ''
! 		then
! 		    log_message=`expr "z$1" : 'z-m\(.*\)'`
! 		else
! 		    log_message="$log_message
  
! `expr "z$1" : 'z-m\(.*\)'`"
! 		fi
  		no_edit=t
- 		shift
  		;;
--- 143,149 ----
  	-m*)
  		log_given=m$log_given
! 		log_message="${log_message}${log_message:+
  
! }${1#-m}"
  		no_edit=t
  		;;
***************
*** 173,185 ****
  	--m=*|--me=*|--mes=*|--mess=*|--messa=*|--messag=*|--message=*)
  		log_given=m$log_given
! 		if test "$log_message" = ''
! 		then
! 		    log_message=`expr "z$1" : 'z-[^=]*=\(.*\)'`
! 		else
! 		    log_message="$log_message
  
! `expr "z$1" : 'zq-[^=]*=\(.*\)'`"
! 		fi
  		no_edit=t
- 		shift
  		;;
--- 150,156 ----
  	--m=*|--me=*|--mes=*|--mess=*|--messa=*|--messag=*|--message=*)
  		log_given=m$log_given
! 		log_message="${log_message}${log_message:+
  
! }${1#*=}"
  		no_edit=t
  		;;
***************
*** 186,197 ****
  	-n|--n|--no|--no-|--no-v|--no-ve|--no-ver|--no-veri|--no-verif|\
  	--no-verify)
  		verify=
- 		shift
  		;;
  	--a|--am|--ame|--amen|--amend)
  		amend=t
  		use_commit=HEAD
- 		shift
  		;;
  	-c)
  		case "$#" in 1) usage ;; esac
--- 157,166 ----
***************
*** 199,203 ****
  		log_given=t$log_given
  		use_commit="$1"
  		no_edit=
- 		shift
  		;;
--- 168,171 ----
***************
*** 204,213 ****
  	--ree=*|--reed=*|--reedi=*|--reedit=*|--reedit-=*|--reedit-m=*|\
  	--reedit-me=*|--reedit-mes=*|--reedit-mess=*|--reedit-messa=*|\
  	--reedit-messag=*|--reedit-message=*)
  		log_given=t$log_given
! 		use_commit=`expr "z$1" : 'z-[^=]*=\(.*\)'`
  		no_edit=
- 		shift
  		;;
  	--ree|--reed|--reedi|--reedit|--reedit-|--reedit-m|--reedit-me|\
  	--reedit-mes|--reedit-mess|--reedit-messa|--reedit-messag|\
--- 172,180 ----
  	--ree=*|--reed=*|--reedi=*|--reedit=*|--reedit-=*|--reedit-m=*|\
  	--reedit-me=*|--reedit-mes=*|--reedit-mess=*|--reedit-messa=*|\
  	--reedit-messag=*|--reedit-message=*)
  		log_given=t$log_given
! 		use_commit="${1#*=}"
  		no_edit=
  		;;
  	--ree|--reed|--reedi|--reedit|--reedit-|--reedit-m|--reedit-me|\
  	--reedit-mes|--reedit-mess|--reedit-messa|--reedit-messag|\
***************
*** 217,223 ****
  		log_given=t$log_given
  		use_commit="$1"
  		no_edit=
- 		shift
  		;;
  	-C)
  		case "$#" in 1) usage ;; esac
--- 184,189 ----
***************
*** 225,229 ****
  		log_given=t$log_given
  		use_commit="$1"
  		no_edit=t
- 		shift
  		;;
--- 191,194 ----
***************
*** 230,239 ****
  	--reu=*|--reus=*|--reuse=*|--reuse-=*|--reuse-m=*|--reuse-me=*|\
  	--reuse-mes=*|--reuse-mess=*|--reuse-messa=*|--reuse-messag=*|\
  	--reuse-message=*)
  		log_given=t$log_given
! 		use_commit=`expr "z$1" : 'z-[^=]*=\(.*\)'`
  		no_edit=t
- 		shift
  		;;
  	--reu|--reus|--reuse|--reuse-|--reuse-m|--reuse-me|--reuse-mes|\
  	--reuse-mess|--reuse-messa|--reuse-messag|--reuse-message)
--- 195,203 ----
  	--reu=*|--reus=*|--reuse=*|--reuse-=*|--reuse-m=*|--reuse-me=*|\
  	--reuse-mes=*|--reuse-mess=*|--reuse-messa=*|--reuse-messag=*|\
  	--reuse-message=*)
  		log_given=t$log_given
! 		use_commit="${1#*=}"
  		no_edit=t
  		;;
  	--reu|--reus|--reuse|--reuse-|--reuse-m|--reuse-me|--reuse-mes|\
  	--reuse-mess|--reuse-messa|--reuse-messag|--reuse-message)
***************
*** 242,273 ****
  		log_given=t$log_given
  		use_commit="$1"
  		no_edit=t
- 		shift
  		;;
  	-s|--s|--si|--sig|--sign|--signo|--signof|--signoff)
  		signoff=t
- 		shift
  		;;
  	-t|--t|--te|--tem|--temp|--templ|--templa|--templat|--template)
  		case "$#" in 1) usage ;; esac
  		shift
  		templatefile="$1"
  		no_edit=
- 		shift
  		;;
  	-q|--q|--qu|--qui|--quie|--quiet)
  		quiet=t
- 		shift
  		;;
  	-v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)
  		verbose=t
- 		shift
  		;;
  	-u|--u|--un|--unt|--untr|--untra|--untrac|--untrack|--untracke|\
  	--untracked|--untracked-|--untracked-f|--untracked-fi|--untracked-fil|\
  	--untracked-file|--untracked-files)
  		untracked_files=t
- 		shift
  		;;
  	--)
  		shift
--- 206,231 ----
***************
*** 280,285 ****
--- 238,244 ----
  		break
  		;;
  	esac
+ 	shift
  done
  case "$edit_flag" in t) no_edit= ;; esac
  
***************
*** 437,448 ****
  
  if test t = "$verify" && test -x "$GIT_DIR"/hooks/pre-commit
  then
! 	if test "$TMP_INDEX"
! 	then
! 		GIT_INDEX_FILE="$TMP_INDEX" "$GIT_DIR"/hooks/pre-commit
! 	else
! 		GIT_INDEX_FILE="$USE_INDEX" "$GIT_DIR"/hooks/pre-commit
! 	fi || exit
  fi
  
  if test "$log_message" != ''
--- 396,403 ----
  
  if test t = "$verify" && test -x "$GIT_DIR"/hooks/pre-commit
  then
!     GIT_INDEX_FILE="${TMP_INDEX:-${USE_INDEX}}" "$GIT_DIR"/hooks/pre-commit \
!     || exit
  fi
  
  if test "$log_message" != ''

[-- Attachment #3: Type: text/plain, Size: 20 bytes --]



-- 
David Kastrup

Re: Shell script cleanups/style changes?

[Robert Schiele]

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

On Thu, Aug 02, 2007 at 12:44:22PM +0200, David Kastrup wrote:
> ! 		logfile="${1#-?}"

You can't do something like that on /bin/sh on many systems (for instance
Solaris).

Robert

-- 
Robert Schiele
Dipl.-Wirtsch.informatiker	mailto:rschiele@gmail.com

"Quidquid latine dictum sit, altum sonatur."

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

Re: Shell script cleanups/style changes?

[David Kastrup]

Robert Schiele <rschiele@gmail.com> writes:

> On Thu, Aug 02, 2007 at 12:44:22PM +0200, David Kastrup wrote:
>> ! 		logfile="${1#-?}"
>
> You can't do something like that on /bin/sh on many systems (for
> instance Solaris).

Sigh.  It's in Posix.

I've seen a lot of "modern" constructs in the Shell scripts of git
(not least of all the eval hackery that is currently used instead of
this), so do you actually have positive knowledge that the existing
git stuff runs fine on such systems, and this wouldn't?

I don't have access to Solaris systems, so I have to take your word on
it, but I find it somewhat surprising that they would not follow Posix
here.

-- 
David Kastrup

Re: Shell script cleanups/style changes?

[Robert Schiele]

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

On Thu, Aug 02, 2007 at 04:20:44PM +0200, David Kastrup wrote:
> Sigh.  It's in Posix.

It is in latest POSIX but latest POSIX is not in Solaris.

> I've seen a lot of "modern" constructs in the Shell scripts of git
> (not least of all the eval hackery that is currently used instead of
> this), so do you actually have positive knowledge that the existing
> git stuff runs fine on such systems, and this wouldn't?

I can't say for sure for every corner case but for the most important stuff
the answer is "yes".  If you have specific doubts about some construct you may
ask me for that.  Then I would check.

> I don't have access to Solaris systems, so I have to take your word on
> it, but I find it somewhat surprising that they would not follow Posix
> here.

Nowadays you can download it for free if you like to test with it.

Robert

-- 
Robert Schiele
Dipl.-Wirtsch.informatiker	mailto:rschiele@gmail.com

"Quidquid latine dictum sit, altum sonatur."

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

Re: Shell script cleanups/style changes?

[Johannes Schindelin]

Hi,

On Thu, 2 Aug 2007, Robert Schiele wrote:

> On Thu, Aug 02, 2007 at 04:20:44PM +0200, David Kastrup wrote:
> > Sigh.  It's in Posix.
> 
> It is in latest POSIX but latest POSIX is not in Solaris.

It has been a really long standing tradition in git development to not 
care about POSIX if it disagrees with reality.  A good tradition.

Ciao,
Dscho

Re: Shell script cleanups/style changes?

[Robert Schiele]

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

On Thu, Aug 02, 2007 at 06:05:00PM +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Thu, 2 Aug 2007, Robert Schiele wrote:
> 
> > On Thu, Aug 02, 2007 at 04:20:44PM +0200, David Kastrup wrote:
> > > Sigh.  It's in Posix.
> > 
> > It is in latest POSIX but latest POSIX is not in Solaris.
> 
> It has been a really long standing tradition in git development to not 
> care about POSIX if it disagrees with reality.  A good tradition.

Sure.  I mean if Solaris was really a system nobody uses nowadays there would
be no reason to care about it but it actually still is one of the mayor
platforms availlable and thus not supporting it would be quite stupid.

It would actually draw away my interest (and most likely the one of many
others) from git since I need a system that works on _all_ systems we support.

Robert

-- 
Robert Schiele
Dipl.-Wirtsch.informatiker	mailto:rschiele@gmail.com

"Quidquid latine dictum sit, altum sonatur."

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

Re: Shell script cleanups/style changes?

[Uwe Kleine-König]

David Kastrup wrote:
> Robert Schiele <rschiele@gmail.com> writes:
> 
> > On Thu, Aug 02, 2007 at 12:44:22PM +0200, David Kastrup wrote:
> >> ! 		logfile="${1#-?}"
> >
> > You can't do something like that on /bin/sh on many systems (for
> > instance Solaris).
> 
> Sigh.  It's in Posix.
Well Solaris is (kind of) Posix compliant---you need some extra effort
to get it into Posix "mode":

	login@~ > uname -a
	SunOS login 5.10 Generic_125100-10 sun4u sparc
	login@~ > sh
	$ set tralala
	$ echo "${1#tra}"
	lala
	$ ^D

the "problem" here is, that my PATH includes /usr/xpg4/bin before
/usr/bin and that's non-standard (for Solaris).  That is
/usr/xpg4/bin/sh is Posix compliant and /usr/bin/sh is compatible to
former versions of Solaris/SunOS and this one doesn't support these
substitutions:

	login@~ > /usr/bin/sh
	$ set tralala
	$ echo "${1#tra}"
	bad substitution

Having 

	login@~ > ls -l /bin
	lrwxrwxrwx 1 root root 9 2006-07-03 09:29 /bin -> ./usr/bin

sadly the Solaris sh is used for shell scripts that use a shebang line
calling /bin/sh.

Best regards
Uwe

-- 
Uwe Kleine-König

http://www.google.com/search?q=5+choose+3

Re: Shell script cleanups/style changes?

[Bradford Smith]

On 8/2/07, David Kastrup <dak@gnu.org> wrote:
> Hi, I wanted to ask what the general stance towards shell script
> cleanups and simplifications would be.  For example, I find the expr
> usage quite inscrutable in commit, and there is no necessity of
> putting "shift" in every case branch instead of once behind it, and a
> lot of conditionals and other manipulations can be made much easier on
> the eye by using parameter expansion patterns that are, as far as I
> can see, available with every reasonable Bourne Shell and clones.
>
> Here is an example context diff (in this case, I find it more readable
> than unified) to illustrate (untested!, please don't apply without a
> regular formatted git patch).
>
> Should I bother doing such cleanups as I read up on code, or should I
> just leave things alone?

I have no authority over the git project, but please consider this argument:

Every time you submit a patch there are three costs:
1. The time you put into making the patch.
2. The time required for the maintainer to review the patch and
possibly merge it into the code base.
3. The risk that you may have accidentally broken something.

Obviously, you aren't too concerned about 1 (the cost to you), because
you're willing to do that work.  However, if I were Junio, I wouldn't
be willing to "spend" costs 2 and 3 on a patch that didn't either fix
a problem or provide a new feature.

So, I recommend you do the clean-up that you want to do on your own
local branch.  This will no doubt be fun and educational for you.  I
know I've learned a lot in the past by experimentally "cleaning up"
old ugly code on other projects, even though the result never made it
into the official code base.

Along the way, you will probably find real bugs.  When you do, submit
patches for them based on the current master branch.  You can probably
manage to sneak a bit of clean-up into those bug-fixing patches, as
long as you make sure it is all relevant to fixing the bugs and you
keep the patches readable.

Best Wishes,

Bradford C Smith

p.s. I should also point out that writing portable shell scripts is
far from trivial, so it is very difficult to be certain that what
works for you will work for someone with a different shell.

Re: Shell script cleanups/style changes?

[Sam Ravnborg]

> 
> Obviously, you aren't too concerned about 1 (the cost to you), because
> you're willing to do that work.  However, if I were Junio, I wouldn't
> be willing to "spend" costs 2 and 3 on a patch that didn't either fix
> a problem or provide a new feature.

For any decent codebase there is a need to keep the code clean.
Being part of the linux-kernel community we see clean-up patches 
each day and a lot are applied.
Even spelling errors in comments are sometimes applied.

Do not underestimate the value of a clean codebase.

	Sam

Re: Shell script cleanups/style changes?

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> Hi, I wanted to ask what the general stance towards shell script
> cleanups and simplifications would be.  For example, I find the expr
> usage quite inscrutable in commit, and there is no necessity of
> putting "shift" in every case branch instead of once behind it, and a
> lot of conditionals and other manipulations can be made much easier on
> the eye by using parameter expansion patterns that are, as far as I
> can see, available with every reasonable Bourne Shell and clones.

The shift in parameter parsing case arms were originally
generated by an automated tool.  If it bothers you, feel free to
move them at the end, I would not mind.  In fact, handcrafted
parameter parser in other scripts do use shift-at-the-end.

As to Bourne-ness of the shell script, please realize that your
maintainer is very old fashioned ;-), but is willing to be
taught new tricks within reason.

We try to limit ourselves to -, =, ?, + (and their colon "if
empty" variants when it really make sense) in parameter
expansion of shell variables.  We also use % and # (and their
"match largest" variants).

Non POSIX substitions such as ${parameter/pattern/string} and
${parameter:offset} are not to be used.  We do not want to
depend on bash.

We try to avoid [ ] and instead spell "test" explicitly; this is
just a personal taste, and not about portability but more about
readability.

After 1.5.3 git-commit.sh will hopefully become built-in, so I
would rather not touch the script.  Certainly, the kind of
change that is "intended to be style-only but somebody needs to
make sure it does not introduce regression to everybody's shell"
is very unwelcome at this point.

Re: Shell script cleanups/style changes?

[Nguyen Thai Ngoc Duy]

On 8/2/07, Junio C Hamano <gitster@pobox.com> wrote:
> Non POSIX substitions such as ${parameter/pattern/string} and
> ${parameter:offset} are not to be used.  We do not want to
> depend on bash.

There is in a test (t5300-pack-objects.sh) but I guess the
restrictions do not apply on tests.
-- 
Duy

Re: Shell script cleanups/style changes?

[David Kastrup]

"Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:

> On 8/2/07, Junio C Hamano <gitster@pobox.com> wrote:
>> Non POSIX substitions such as ${parameter/pattern/string} and
>> ${parameter:offset} are not to be used.  We do not want to
>> depend on bash.
>
> There is in a test (t5300-pack-objects.sh) but I guess the
> restrictions do not apply on tests.

Oh, but they definitely should.  Precisely on those platforms with
shells not generally in use by the developers it becomes _most_
important to reliably be able to trace failed tests to problems with
the _commands_, not problems with the tests.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Shell script cleanups/style changes?

[Junio C Hamano]

"Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:

> On 8/2/07, Junio C Hamano <gitster@pobox.com> wrote:
>> Non POSIX substitions such as ${parameter/pattern/string} and
>> ${parameter:offset} are not to be used.  We do not want to
>> depend on bash.
>
> There is in a test (t5300-pack-objects.sh) but I guess the
> restrictions do not apply on tests.

That would have been an earlier mistake.  Keeping tests portable
is important, perhaps it might be of lessor importance but
still.

Re: Shell script cleanups/style changes?

[David Kastrup]

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

> David Kastrup <dak@gnu.org> writes:
>
>> Hi, I wanted to ask what the general stance towards shell script
>> cleanups and simplifications would be.  For example, I find the
>> expr usage quite inscrutable in commit, and there is no necessity
>> of putting "shift" in every case branch instead of once behind it,
>> and a lot of conditionals and other manipulations can be made much
>> easier on the eye by using parameter expansion patterns that are,
>> as far as I can see, available with every reasonable Bourne Shell
>> and clones.
>
> As to Bourne-ness of the shell script, please realize that your
> maintainer is very old fashioned ;-), but is willing to be taught
> new tricks within reason.

Most of the "new tricks" I try on bash, dash and ash.

> We try to limit ourselves to -, =, ?, + (and their colon "if
> empty" variants when it really make sense) in parameter
> expansion of shell variables.  We also use % and # (and their
> "match largest" variants).

You do?

Indeed:

-*- mode: grep; default-directory: "/home/tmp/git/" -*-
Grep started at Thu Aug  2 22:47:30

grep -nH -e '\${[a-zA-Z0-9_]*[#%]' *.sh
git-am.sh:146:	resolvemsg=${1#--resolvemsg=}; shift ;;
git-clone.sh:358:			destname="refs/$branch_top/${name#refs/heads/}" ;;
git-clone.sh:360:			destname="refs/$tag_top/${name#refs/tags/}" ;;
git-filter-branch.sh:361:		ref="${ref#refs/tags/}"
git-pull.sh:98:	curr_branch=${curr_branch#refs/heads/}
git-rebase.sh:93:	eval GITHEAD_$cmt='"${cmt_name##refs/heads/}~$(($end - $msgnum))"'
git-stash.sh:52:		branch=${branch#refs/heads/}

Grep finished (matches found) at Thu Aug  2 22:47:31

I am confused now: a different poster adamantly stated that /bin/sh on
Solaris did not support those constructs, and that every functionality
of git was working fine for him.

> Non POSIX substitions such as ${parameter/pattern/string} and
> ${parameter:offset} are not to be used.  We do not want to
> depend on bash.

Sure.  What about the git-rebase line using $(($end - $msgnum)) ?
That's even more risque than ##.

> After 1.5.3 git-commit.sh will hopefully become built-in, so I would
> rather not touch the script.

Too bad: this should mean that $EDITOR can get called from C...  I've
been glad to see that so far this could be avoided.

> Certainly, the kind of change that is "intended to be style-only but
> somebody needs to make sure it does not introduce regression to
> everybody's shell" is very unwelcome at this point.

Understood.  But using ${...#...} and ${...:+...} does not exactly
seem to be news in the git code base.  Even though we have the claim
that Solaris' sh won't deal with the former.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Shell script cleanups/style changes?

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> Sure.  What about the git-rebase line using $(($end - $msgnum)) ?
> That's even more risque than ##.

Is it really risque?  I do not think we have heard trouble with
the arith expansion from anybody.  A few mistakes in the past
made that said things like:

	$((end - 1))	    ;# wrong... say "$end" if you mean variable
	$((cd ...; pwd))    ;# wrong... say $( (...)) if command substitution
                             # that involves subshell

but I think we fixed them.

> Understood.  But using ${...#...} and ${...:+...} does not exactly
> seem to be news in the git code base.  Even though we have the claim
> that Solaris' sh won't deal with the former.

I do not think we have trouble with ${parameter#word}.  Much
less with ${parameter+word}; it has been in /bin/sh forever.

Re: Shell script cleanups/style changes?

[Junio C Hamano]

You might find this thread amusing.

    http://thread.gmane.org/gmane.comp.version-control.git/7116/focus=7136

Historically, I have even avoided accepting ${var#word}, ${var%word},
and arithmetic expansions.

I would still reject shell arrays.

Re: Shell script cleanups/style changes?

[David Kastrup]

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

> You might find this thread amusing.
>
>     http://thread.gmane.org/gmane.comp.version-control.git/7116/focus=7136
>
> Historically, I have even avoided accepting ${var#word}, ${var%word},
> and arithmetic expansions.

I learnt Unix with a Banaham/Rutter primer in the early eighties.  I
got hit so often by the "this is now supposed to work in Bourne
shells?" surprise it wasn't funny.

The first time I saw "for ((i=0; i<$NR; i++)) ..."  I thought the
author had been smoking too much C and got things confused.  It still
creeps me out.  I am more comfortable doing arithmetic with dc rather
than sh.

Employing the existing globbing machinery for # and %, on the other
hand, seems quite bournesque (still-bourne sounds so ugly) to me.  And
it is certainly quite more readable than the regexp/expr stuff.

If it works.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Shell script cleanups/style changes?

[Robert Schiele]

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

On Thu, Aug 02, 2007 at 10:57:31PM +0200, David Kastrup wrote:
> Most of the "new tricks" I try on bash, dash and ash.

Well, those are not really the most challenging one.  Thus you should either
test on more or just believe those people that have other shells that it does
not work.

> I am confused now: a different poster adamantly stated that /bin/sh on
> Solaris did not support those constructs, and that every functionality
> of git was working fine for him.

No, you should read the mails you are refering to.  I said that the most
important stuff does work.  Apparently this did not yet hurt me on the
platform.  Thus we have to decide whether we want some textbook example code
and thus break this platform completely or whether we want to fix the issues
you have listed and thus have a more portable application.

> Sure.  What about the git-rebase line using $(($end - $msgnum)) ?

Bad on Solaris:

$ uname -a
SunOS solaris10-x64 5.10 Generic i86pc i386 i86pc
$ end=1
$ msgnum=5
$ echo $(($end - $msgnum))
syntax error: `(' unexpected
$ 

> Too bad: this should mean that $EDITOR can get called from C...  I've
> been glad to see that so far this could be avoided.

Why is it bad to call the editor from C?

On Thu, Aug 02, 2007 at 02:21:01PM -0700, Junio C Hamano wrote:
> David Kastrup <dak@gnu.org> writes:
> 
> > Sure.  What about the git-rebase line using $(($end - $msgnum)) ?
> > That's even more risque than ##.
> 
> Is it really risque?  I do not think we have heard trouble with
> the arith expansion from anybody.  A few mistakes in the past

See above.

> I do not think we have trouble with ${parameter#word}.  Much

$ uname -a
SunOS solaris10-x64 5.10 Generic i86pc i386 i86pc
$ parameter=bla
$ echo ${parameter#word}
bad substitution
$ 

> less with ${parameter+word}; it has been in /bin/sh forever.

That one is ok for Solaris.

Robert

-- 
Robert Schiele
Dipl.-Wirtsch.informatiker	mailto:rschiele@gmail.com

"Quidquid latine dictum sit, altum sonatur."

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

Re: Shell script cleanups/style changes?

[David Kastrup]

Robert Schiele <rschiele@gmail.com> writes:

> No, you should read the mails you are refering to.  I said that the
> most important stuff does work.  Apparently this did not yet hurt me
> on the platform.

A non-working rebase would seem rather tough.

> Thus we have to decide whether we want some textbook example code
> and thus break this platform completely or whether we want to fix
> the issues you have listed and thus have a more portable
> application.

The "issues" are with Solaris, apparently.  There is always a price
for portability.  If Solaris users can fix their problems with a
global search and replace of the first line in *.sh, the question is
whether it is worth the hassle of having unreadable but "portable"
code.  After all, it has to be read also by humans.

>> Sure.  What about the git-rebase line using $(($end - $msgnum)) ?
>
> Bad on Solaris:
>
> $ uname -a
> SunOS solaris10-x64 5.10 Generic i86pc i386 i86pc
> $ end=1
> $ msgnum=5
> $ echo $(($end - $msgnum))
> syntax error: `(' unexpected
> $

You are missing the line
$ echo $0
which is probably the most interesting one...  we don't need to be
compatible with everything having a "$ " prompt, just with everything
called "/bin/sh".

>> Too bad: this should mean that $EDITOR can get called from C...  I've
>> been glad to see that so far this could be avoided.
>
> Why is it bad to call the editor from C?

See the rationale in my recently posted patch for implementing
EDITOR/VISUAL support.  One needs to shell-quote stuff properly, and
the shell is better at shell-quote magic than C is.

>> I do not think we have trouble with ${parameter#word}.  Much
>
> $ uname -a
> SunOS solaris10-x64 5.10 Generic i86pc i386 i86pc
> $ parameter=bla
> $ echo ${parameter#word}
> bad substitution
> $ 
>
>> less with ${parameter+word}; it has been in /bin/sh forever.
>
> That one is ok for Solaris.

If you prepare a patch replacing all existing ${parameter#word} uses
and get it accepted, I will not push for inclusion of my cleanup.

But you _really_ should go for it _now_.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Shell script cleanups/style changes?

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> Robert Schiele <rschiele@gmail.com> writes:
> ...
>> Thus we have to decide whether we want some textbook example code
>> and thus break this platform completely or whether we want to fix
>> the issues you have listed and thus have a more portable
>> application.
>
> The "issues" are with Solaris, apparently.  There is always a price
> for portability.  If Solaris users can fix their problems with a
> global search and replace of the first line in *.sh, the question is
> whether it is worth the hassle of having unreadable but "portable"
> code.  After all, it has to be read also by humans.

I am in the camp of avoiding "it is even in POSIX so it's your
fault if your shell does not support it".  We do not take POSIX
too seriously in that way, although we do say "let's not use it,
it is not even in POSIX".  In other words, I've been trying to
be, and as a result of that we are, fairly conservative.

However, there is a line we need to draw when bending bacwards
for compatibility, and I think a system that does not have a
working command substitution $( ... )  is on the other side of
that line.

Re: Shell script cleanups/style changes?

[David Kastrup]

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

> However, there is a line we need to draw when bending bacwards for
> compatibility, and I think a system that does not have a working
> command substitution $( ... )  is on the other side of that line.

Not an issue.  But apparently, ${parameter#word} is for Solaris.  I'd
still like to get confirmation that it is indeed /bin/sh, but if it
is, the current code is not good for Solaris.

So the line of compatibility is much more interesting for this one.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Shell script cleanups/style changes?

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> However, there is a line we need to draw when bending bacwards for
>> compatibility, and I think a system that does not have a working
>> command substitution $( ... )  is on the other side of that line.
>
> Not an issue.  But apparently, ${parameter#word} is for Solaris.  I'd
> still like to get confirmation that it is indeed /bin/sh, but if it
> is, the current code is not good for Solaris.

I happen to feel ${parameter#word} is more esoteric than $(cmd).
If a system does not even do the latter, then avoiding the
former to help such a system is a futile effort.

And /bin/sh my Solaris box does not understand $(cmd) and wants
you to say `cmd`.  Of course % and # parameter substition do not
work there.

People could probably say SHELL_PATH=/usr/xpg4/bin/sh there to
get a saner shell, even if they do not like bash, though.

Re: Shell script cleanups/style changes?

[David Kastrup]

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

> David Kastrup <dak@gnu.org> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> However, there is a line we need to draw when bending bacwards for
>>> compatibility, and I think a system that does not have a working
>>> command substitution $( ... )  is on the other side of that line.
>>
>> Not an issue.  But apparently, ${parameter#word} is for Solaris.  I'd
>> still like to get confirmation that it is indeed /bin/sh, but if it
>> is, the current code is not good for Solaris.
>
> I happen to feel ${parameter#word} is more esoteric than $(cmd).
> If a system does not even do the latter, then avoiding the
> former to help such a system is a futile effort.

The situation is that we currently don't avoid the former.  Robert
said that he had prepared a patch that would do so.

It would make sense to either encourage him to present his patch
(though we probably don't know for sure that there are indeed shells
for which the former works worse than the latter), or permit further
use of ${parameter#word} where it makes things more readable.

But "only a little bit of ${parameter#word}, please" seems pointless.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Shell script cleanups/style changes?

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

>> I happen to feel ${parameter#word} is more esoteric than $(cmd).
>> If a system does not even do the latter, then avoiding the
>> former to help such a system is a futile effort.
>
> The situation is that we currently don't avoid the former.  Robert
> said that he had prepared a patch that would do so.
> ...
> But "only a little bit of ${parameter#word}, please" seems pointless.

Absolutely.  And we started to adopt #/% substititions some time
ago.  Undoing them just feels going backwards, and we need to
judge what the merits of going backwards are.

For that discussion, /bin/sh on Solaris does not count.  There
are huge downside of rewriting scripts to work with stock
Solaris /bin/sh:

 (1) that shell does not even grok $(cmd) substitution.

     I won't accept a half-baked patch that replaces "$(" with a
     backtick and matching ")" with another backtick.  You need
     to at least make sure your interpolated variables within
     the backtick pair work sensibly, and you haven't broken
     existing nesting of command interpolations, if any.  I do
     not even want to inspect, comment on and reject that kind
     of changes.  Quite frankly, it's not worth my time.

 (2) Rewriting $(cmd) to `cmd`, and ${parameter#word} with sed
     or expr would reduce readability, at least to other people.

     Remember, I was the one who originally avoided modern
     ${parameter#word} substitutions, and older scripts had many
     more invocations of expr than we currently have.  Reading
     such a backward rewrite would not be too much of a problem
     for *me*, but other people also need to read and understand
     scripts, if only to be able to rewrite them in C.

     There may still be many old parts of the scripts that could
     be made more readable and efficient using ${parameter#word}
     substitutions. If we were to rewrite scripts, more use of
     them could be a good thing, not the other way around.

Besides, on that platform there are more reasonable shells
available via SHELL_PATH, and it is not limited to going to
bash.

Re: Shell script cleanups/style changes?

[David Kastrup]

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

> David Kastrup <dak@gnu.org> writes:
>
>>> I happen to feel ${parameter#word} is more esoteric than $(cmd).
>>> If a system does not even do the latter, then avoiding the
>>> former to help such a system is a futile effort.
>>
>> The situation is that we currently don't avoid the former.  Robert
>> said that he had prepared a patch that would do so.
>> ...
>> But "only a little bit of ${parameter#word}, please" seems pointless.
>
> Absolutely.  And we started to adopt #/% substititions some time
> ago.  Undoing them just feels going backwards, and we need to judge
> what the merits of going backwards are.

Ok, seems like the sort of cleanups I proposed would not clash with
current git policies.  I'll readily agree that the timing of their
adoption might not really fit with a rc4, but posting them for the
queue does not seem outrageous.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Shell script cleanups/style changes?

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> Ok, seems like the sort of cleanups I proposed would not clash with
> current git policies.  I'll readily agree that the timing of their
> adoption might not really fit with a rc4, but posting them for the
> queue does not seem outrageous.

Yeah, except that Kristian's C-rewrite of git-commit.sh may well
jump the queue before such a patch would touch the file it
intends to replace...

Re: Shell script cleanups/style changes?

[David Kastrup]

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

> David Kastrup <dak@gnu.org> writes:
>
>> Ok, seems like the sort of cleanups I proposed would not clash with
>> current git policies.  I'll readily agree that the timing of their
>> adoption might not really fit with a rc4, but posting them for the
>> queue does not seem outrageous.
>
> Yeah, except that Kristian's C-rewrite of git-commit.sh may well
> jump the queue before such a patch would touch the file it
> intends to replace...

Well, since the work has already been done, I guess I might as well
post it.

With regard to C rewrites: I would hazard a guess that git's
performance might actually be improved by splitting some primitives
into even smaller C building blocks and tying them all together with
pipes (which makes shell scripts a natural container).  As long as one
designs the C chunks carefully enough that no bulk processing is done
in the scripts themselves, this could actually lead to better
schedulable pieces of software.  I think that most index processing
can be done in a list-merge style on sorted lists.  That implies pipes
and files on input and output, and a small memory footprint.  With a
good scheduler (the current Linux scheduler sucks at exploiting the
asynchronicity of pipes; this should be better with CFS), this should
make things work rather efficiently, be flexible for extension, and
make good use of multi-core systems.

We have seen a recent example on this list: hand-chaining git-ls-files
and a few other tools into a pipeline beat the pants off
builtin-add.c.

Given that portability goes down the drain if we want to use similarly
or more efficient constellations in C (multithreading and asynchronous
I/O come to mind), I would not replace shell scripts (and the
associated flexibility in extending functionality) lightly right now.
As long as the main data flow is only managed rather than processed by
the scripts, I think we would have more to gain by restructuring into
pipelineable pieces.  It will still be possible to ultimately tie
those together in a single process image (with multiple threads
presumably).  But that immediately takes away a lot of flexibility.

-- 
David Kastrup

Re: Shell script cleanups/style changes?

[David Kastrup]

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

>  (1) that shell does not even grok $(cmd) substitution.
>
>      I won't accept a half-baked patch that replaces "$(" with a
>      backtick and matching ")" with another backtick.  You need
>      to at least make sure your interpolated variables within
>      the backtick pair work sensibly, and you haven't broken
>      existing nesting of command interpolations, if any.  I do
>      not even want to inspect, comment on and reject that kind
>      of changes.  Quite frankly, it's not worth my time.

And that's actually not even _half_ of the deal: we are talking about
pandering to legacy shells here, and the amount of variance of just
what level of quoting/backslashing is needed on the inside of `...` in
order to get stuff through with just the right level of quoting is
actually stunning.

I've had my fair share of bad surprises with portable scripts.
Getting a backquote mechanism running on one shell does not mean it
will work on another.  Basically, you have to forego nesting stuff and
split it out into small units in separate commands.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Shell script cleanups/style changes?

[Robert Schiele]

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

On Fri, Aug 03, 2007 at 02:24:38AM +0200, David Kastrup wrote:
> The situation is that we currently don't avoid the former.  Robert
> said that he had prepared a patch that would do so.
> 
> It would make sense to either encourage him to present his patch

Well, if you want to see it, just tell me.  Do you want to have it with or
without the arithmetic replacements I did as well?

Robert

-- 
Robert Schiele
Dipl.-Wirtsch.informatiker	mailto:rschiele@gmail.com

"Quidquid latine dictum sit, altum sonatur."

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

Re: Shell script cleanups/style changes?

[David Kastrup]

Robert Schiele <rschiele@gmail.com> writes:

> On Fri, Aug 03, 2007 at 02:24:38AM +0200, David Kastrup wrote:
>> The situation is that we currently don't avoid the former.  Robert
>> said that he had prepared a patch that would do so.
>> 
>> It would make sense to either encourage him to present his patch
>
> Well, if you want to see it, just tell me.  Do you want to have it with or
> without the arithmetic replacements I did as well?

>From Junio's answer, I gather that indeed this would seem pointless.
$(...) is not going away anytime soon, and I have seen no evidence
that there is a shell in widespread use that supports it, but doesn't
support ${...#...}.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Shell script cleanups/style changes?

[Robert Schiele]

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

On Thu, Aug 02, 2007 at 04:05:46PM -0700, Junio C Hamano wrote:
> However, there is a line we need to draw when bending bacwards
> for compatibility, and I think a system that does not have a
> working command substitution $( ... )  is on the other side of
> that line.

Well, I have now a patch ready for submission that would replace all
occurences of ${PARAMETER#WORD}, ${PARAMETER%WORD}, and $(( EXPRESSION )).
But if you say that you won't accept replacement of $( ... ) then this is not
worth the effort since this one isn't accepted as well.

Robert

-- 
Robert Schiele
Dipl.-Wirtsch.informatiker	mailto:rschiele@gmail.com

"Quidquid latine dictum sit, altum sonatur."

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

Re: Shell script cleanups/style changes?

[David Kastrup]

Robert Schiele <rschiele@gmail.com> writes:

> On Thu, Aug 02, 2007 at 04:05:46PM -0700, Junio C Hamano wrote:
>> However, there is a line we need to draw when bending bacwards
>> for compatibility, and I think a system that does not have a
>> working command substitution $( ... )  is on the other side of
>> that line.
>
> Well, I have now a patch ready for submission that would replace all
> occurences of ${PARAMETER#WORD}, ${PARAMETER%WORD}, and $((
> EXPRESSION )).  But if you say that you won't accept replacement of
> $( ... ) then this is not worth the effort since this one isn't
> accepted as well.

I absolutely can't understand your claim that most things work for
you, then.  Are you _really_, _really_ sure you are talking about
/bin/sh here?

Look:

-*- mode: grep; default-directory: "/home/tmp/git/" -*-
Grep started at Fri Aug  3 01:29:41

fgrep -nH -e '$(' *.sh
check-builtins.sh:6:	$(foreach b,$(BUILT_INS),echo XXX $b YYY;)
git-am.sh:24:    cmdline=$(basename $0)
git-am.sh:83:    his_tree=$(GIT_INDEX_FILE="$dotest/patch-merge-index" git write-tree) &&
git-am.sh:84:    orig_tree=$(cat "$dotest/patch-merge-base") &&
git-am.sh:160:   last=$(cat "$dotest/last") &&
git-am.sh:161:   next=$(cat "$dotest/next") &&
git-am.sh:216:	files=$(git diff-index --cached --name-only HEAD) || exit
git-am.sh:223:if test "$(cat "$dotest/binary")" = t
git-am.sh:227:if test "$(cat "$dotest/utf8")" = t
git-am.sh:233:if test "$(cat "$dotest/keep")" = t
git-am.sh:238:if test "$(cat "$dotest/sign")" = t
git-am.sh:301:	GIT_AUTHOR_NAME="$(sed -n '/^Author/ s/Author: //p' "$dotest/info")"
git-am.sh:302:	GIT_AUTHOR_EMAIL="$(sed -n '/^Email/ s/Email: //p' "$dotest/info")"
git-am.sh:303:	GIT_AUTHOR_DATE="$(sed -n '/^Date/ s/Date: //p' "$dotest/info")"
git-am.sh:313:	SUBJECT="$(sed -n '/^Subject/ s/Subject: //p' "$dotest/info")"
git-am.sh:415:		unmerged=$(git ls-files -u)
git-am.sh:453:	tree=$(git write-tree) &&
git-am.sh:455:	parent=$(git rev-parse --verify HEAD) &&
git-am.sh:456:	commit=$(git commit-tree $tree -p $parent <"$dotest/final-commit") &&
git-bisect.sh:59:	head=$(GIT_DIR="$GIT_DIR" git symbolic-ref HEAD) ||
git-bisect.sh:92:	orig_args=$(sq "$@")
git-bisect.sh:102:	        rev=$(git rev-parse --verify "$arg^{commit}" 2>/dev/null) || {
git-bisect.sh:127:		rev=$(git rev-parse --verify HEAD) ;;
git-bisect.sh:129:		rev=$(git rev-parse --verify "$1^{commit}") ;;
git-bisect.sh:141:	echo "# bad: "$(git show-branch $rev) >>"$GIT_DIR/BISECT_LOG"
git-bisect.sh:147:	0)    revs=$(git rev-parse --verify HEAD) || exit ;;
git-bisect.sh:148:	*)    revs=$(git rev-parse --revs-only --no-flags "$@") &&
git-bisect.sh:153:		rev=$(git rev-parse --verify "$rev^{commit}") || exit
git-bisect.sh:164:	echo "# good: "$(git show-branch $rev) >>"$GIT_DIR/BISECT_LOG"
git-bisect.sh:170:	test -n "$(git for-each-ref "refs/bisect/good-*")" || missing_good=t
git-bisect.sh:187:			case "$(read yesno)" in [Nn]*) exit 1 ;; esac
git-bisect.sh:214:	bad=$(git rev-parse --verify refs/bisect/bad) &&
git-bisect.sh:215:	good=$(git for-each-ref --format='^%(objectname)' \
git-bisect.sh:218:	eval="$eval $(cat "$GIT_DIR/BISECT_NAMES")" &&
git-bisect.sh:219:	eval=$(eval "$eval") &&
git-bisect.sh:243:	eval gitk bisect/bad --not $not -- $(cat "$GIT_DIR/BISECT_NAMES")
git-bisect.sh:291:			echo "# good: "$(git show-branch $rev) >>"$GIT_DIR/BISECT_LOG"
git-bisect.sh:296:			echo "# bad: "$(git show-branch $rev) >>"$GIT_DIR/BISECT_LOG"
git-checkout.sh:9:old=$(git rev-parse --verify $old_name 2>/dev/null)
git-checkout.sh:10:oldbranch=$(git symbolic-ref $old_name 2>/dev/null)
git-checkout.sh:60:		if rev=$(git rev-parse --verify "$arg^0" 2>/dev/null)
git-checkout.sh:69:				rev=$(git rev-parse --verify "refs/heads/$arg^0")
git-checkout.sh:73:		elif rev=$(git rev-parse --verify "$arg^{tree}" 2>/dev/null)
git-checkout.sh:206:    merge_error=$(git read-tree -m -u --exclude-per-directory=.gitignore $old $new 2>&1) || (
git-clone.sh:147:		upload_pack=--upload-pack=$(expr "z$1" : 'z-[^=]*=\(.*\)') ;;
git-clone.sh:181:if base=$(get_repo_base "$repo"); then
git-clone.sh:188:[ -z "$dir" ] && dir=$(echo "$repo" | sed -e 's|/$||' -e 's|:*/*\.git$||' -e 's|.*[/:]||g')
git-clone.sh:205:mkdir -p "$dir" && D=$(cd "$dir" && pwd) || usage
git-clone.sh:207:W=$(cd "$GIT_WORK_TREE" && pwd) && export GIT_WORK_TREE="$W"
git-clone.sh:231:		ref_git=$(cd "$ref_git" && pwd)
git-clone.sh:260:	    sample_file=$(cd "$repo" && \
git-clone.sh:391:	head_points_at=$(
git-commit.sh:463:	encoding=$(git config i18n.commitencoding || echo UTF-8)
git-commit.sh:479:	sign=$(git-var GIT_COMMITTER_IDENT | sed -e '
git-commit.sh:506:	eval "$(get_author_ident_from_commit "$use_commit")"
git-commit.sh:528:		PARENTS=$(git cat-file commit HEAD |
git-commit.sh:531:	current="$(git rev-parse --verify HEAD)"
git-commit.sh:533:	if [ -z "$(git ls-files)" ]; then
git-commit.sh:625:		tree=$(GIT_INDEX_FILE="$USE_INDEX" git write-tree)
git-commit.sh:627:		tree=$(GIT_INDEX_FILE="$TMP_INDEX" git write-tree) &&
git-commit.sh:630:	commit=$(git commit-tree $tree $PARENTS <"$GIT_DIR/COMMIT_MSG") &&
git-commit.sh:631:	rlogm=$(sed -e 1q "$GIT_DIR"/COMMIT_MSG) &&
git-fetch.sh:43:		exec=--upload-pack=$(expr "z$1" : 'z-[^=]*=\(.*\)')
git-fetch.sh:88:	origin=$(get_default_remote)
git-fetch.sh:89:	test -n "$(get_remote_url ${origin})" ||
git-fetch.sh:97:	exec="--upload-pack=$(get_uploadpack $1)"
git-fetch.sh:101:remote=$(get_remote_url "$@")
git-fetch.sh:112:ls_remote_result=$(git ls-remote $exec "$remote") ||
git-fetch.sh:125:if test -z "$update_head_ok" && test $(is_bare_repository) = false
git-fetch.sh:127:	orig_head=$(git rev-parse --verify HEAD 2>/dev/null)
git-fetch.sh:133:	case "$(git config --get "remote.$1.tagopt")" in
git-fetch.sh:144:reflist=$(get_remote_refs_for_fetch "$@")
git-fetch.sh:166:  eval=$(echo "$1" | git fetch--tool parse-reflist "-")
git-fetch.sh:192:			theirs=$(echo "$ls_remote_result" | \
git-fetch.sh:242:	  ref=$(expr "z$ref" : 'z\.\(.*\)')
git-fetch.sh:249:	  ref=$(expr "z$ref" : 'z+\(.*\)')
git-fetch.sh:253:      remote_name=$(expr "z$ref" : 'z\([^:]*\):')
git-fetch.sh:254:      local_name=$(expr "z$ref" : 'z[^:]*:\(.*\)')
git-fetch.sh:273:	  head=$(echo "$ls_remote_result" | \
git-fetch.sh:286:	  head=$(git rev-parse --verify TMP_HEAD)
git-fetch.sh:343:		taglist=$(IFS='	' &&
git-fetch.sh:368:	curr_head=$(git rev-parse --verify HEAD 2>/dev/null)
git-filter-branch.sh:43:	lid="$(echo "$1" | tr "A-Z" "a-z")"
git-filter-branch.sh:44:	uid="$(echo "$1" | tr "a-z" "A-Z")"
git-filter-branch.sh:154:tempdir="$(cd "$tempdir"; pwd)" &&
git-filter-branch.sh:156:workdir="$(pwd)" ||
git-filter-branch.sh:177:	GIT_DIR="$(pwd)/../../$GIT_DIR"
git-filter-branch.sh:190:		ref="$(git symbolic-ref "$ref")"
git-filter-branch.sh:195:		ref="$(git for-each-ref --format='%(refname)' |
git-filter-branch.sh:205:export GIT_INDEX_FILE="$(pwd)/../index"
git-filter-branch.sh:222:commits=$(wc -l <../revs | tr -d " ")
git-filter-branch.sh:230:	i=$(($i+1))
git-filter-branch.sh:245:	eval "$(set_ident AUTHOR <../commit)" ||
git-filter-branch.sh:247:	eval "$(set_ident COMMITTER <../commit)" ||
git-filter-branch.sh:272:		for reparent in $(map "$parent"); do
git-filter-branch.sh:277:		parentstr="$(echo "$parentstr" | eval "$filter_parent")" ||
git-filter-branch.sh:285:		$(git write-tree) $parentstr < ../message > ../map/$commit
git-filter-branch.sh:296:	sha1=$(git rev-parse "$ref"^0)
git-filter-branch.sh:301:	for p in $( (cd "$workdir"/../map; ls | sed "s/^/^/") |
git-filter-branch.sh:320:	sha1=$(git rev-parse "$ref"^0)
git-filter-branch.sh:321:	rewritten=$(map $sha1)
git-filter-branch.sh:344:		rewritten=$(echo "$rewritten" | head -n 1)
git-filter-branch.sh:351:	count=$(($count+1))
git-filter-branch.sh:370:			sha1="$(git rev-parse "$sha1"^{commit} 2>/dev/null)" || continue
git-filter-branch.sh:374:		new_sha1="$(cat "../map/$sha1")"
git-filter-branch.sh:376:		new_ref="$(echo "$ref" | eval "$filter_tag_name")" ||
git-ls-remote.sh:30:	exec=--upload-pack=$(expr "z$1" : 'z-[^=]*=\(.*\)')
git-ls-remote.sh:48:peek_repo="$(get_remote_url "$@")"
git-ls-remote.sh:75:	head=$(cat "$tmpdir/HEAD") &&
git-ls-remote.sh:78:		head=$(expr "z$head" : 'zref: \(.*\)') &&
git-ls-remote.sh:79:		head=$(cat "$tmpdir/$head") || exit
git-merge-octopus.sh:48:MRT=$(git write-tree)
git-merge-octopus.sh:64:	common=$(git merge-base --all $MRC $SHA1) ||
git-merge-octopus.sh:86:		MRC=$SHA1 MRT=$(git write-tree)
git-merge-octopus.sh:94:	next=$(git write-tree 2>/dev/null)
git-merge-octopus.sh:100:		next=$(git write-tree 2>/dev/null)
git-merge-one-file.sh:36:		rmdir -p "$(expr "z$4" : 'z\(.*\)/')" 2>/dev/null || :
git-merge-resolve.sh:43:if result_tree=$(git write-tree  2>/dev/null)
git-merge.sh:13:test -z "$(git ls-files -u)" ||
git-merge.sh:102:	rh=$(git rev-parse --verify "$remote^0" 2>/dev/null) || return
git-merge.sh:103:	bh=$(git show-ref -s --verify "refs/heads/$remote" 2>/dev/null)
git-merge.sh:107:	elif truname=$(expr "$remote" : '\(.*\)~[1-9][0-9]*$') &&
git-merge.sh:173:    test "$(git config --bool merge.diffstat)" = false && show_diffstat=false
git-merge.sh:184:	second_token=$(git rev-parse --verify "$2^0" 2>/dev/null) &&
git-merge.sh:185:	head_commit=$(git rev-parse --verify "HEAD" 2>/dev/null) &&
git-merge.sh:203:	rh=$(git rev-parse --verify "$1^0") ||
git-merge.sh:219:	merge_name=$(for remote
git-merge.sh:226:head=$(git rev-parse --verify "$head_arg"^0) || usage
git-merge.sh:235:	remotehead=$(git rev-parse --verify "$remote"^0 2>/dev/null) ||
git-merge.sh:281:	common=$(git merge-base --all $head "$@")
git-merge.sh:284:	common=$(git show-branch --merge-base $head "$@")
git-merge.sh:304:	echo "Updating $(git rev-parse --short $head)..$(git rev-parse --short $1)"
git-merge.sh:311:	new_head=$(git rev-parse --verify "$1^0") &&
git-merge.sh:334:		   result_tree=$(git write-tree)
git-merge.sh:337:			result_commit=$(
git-merge.sh:353:		common_one=$(git merge-base --all $head $remote)
git-merge.sh:435:    result_tree=$(git write-tree) && break
git-merge.sh:442:    parents=$(git show-branch --independent "$head" "$@" | sed -e 's/^/-p /')
git-merge.sh:443:    result_commit=$(printf '%s\n' "$merge_msg" | git commit-tree $result_tree $parents) || exit
git-merge-stupid.sh:69:if result_tree=$(git write-tree  2>/dev/null)
git-mergetool.sh:51:	echo "a symbolic link -> '$(cat "$file")'"
git-mergetool.sh:266:		eval "${eval_candidate}" --eval '"$(emacs-string "(ediff-merge-files-with-ancestor" @ "$LOCAL" @ "$REMOTE" @ "$BASE" "(lambda()(setq ediff-keep-variants nil))" @ "$path" ")")"'
git-mergetool.sh:268:		eval "${eval_candidate}" --eval '"$(emacs-string "(ediff-merge-files" @ "$LOCAL" @ "$REMOTE" "(lambda()(setq ediff-keep-variants nil))" @ "$path" ")")"'
git-mergetool.sh:314:	for i in "$GIT_EDITOR" "$(git config core.editor)" "$VISUAL" "$EDITOR"
git-parse-remote.sh:5:GIT_DIR=$(git rev-parse --git-dir 2>/dev/null) || :;
git-parse-remote.sh:16:		if test "$(git config --get "remote.$1.url")"
git-parse-remote.sh:32:	data_source=$(get_data_source "$1")
git-parse-remote.sh:58:	curr_branch=$(git symbolic-ref -q HEAD | sed -e 's|^refs/heads/||')
git-parse-remote.sh:59:	origin=$(git config --get "branch.$curr_branch.remote")
git-parse-remote.sh:64:	data_source=$(get_data_source "$1")
git-parse-remote.sh:105:		set $(expand_refs_wildcard "$remote" "$@")
git-parse-remote.sh:108:		if test "$remote" = "$(get_default_remote)"
git-parse-remote.sh:110:			curr_branch=$(git symbolic-ref -q HEAD | \
git-parse-remote.sh:112:			merge_branches=$(git config \
git-parse-remote.sh:125:			ref=$(expr "z$ref" : 'z+\(.*\)')
git-parse-remote.sh:130:		remote=$(expr "z$ref" : 'z\([^:]*\):')
git-parse-remote.sh:131:		local=$(expr "z$ref" : 'z[^:]*:\(.*\)')
git-parse-remote.sh:157:		if local_ref_name=$(expr "z$local" : 'zrefs/\(.*\)')
git-parse-remote.sh:168:	data_source=$(get_data_source "$1")
git-parse-remote.sh:174:			$(git for-each-ref --format='%(refname):')
git-parse-remote.sh:178:			$(git config --get-all "remote.$1.fetch") ;;
git-parse-remote.sh:180:		remote_branch=$(sed -ne '/#/s/.*#//p' "$GIT_DIR/branches/$1")
git-parse-remote.sh:185:		canon_refs_list_for_fetch -d "$1" $(sed -ne '/^Pull: */{
git-parse-remote.sh:254:	data_source=$(get_data_source "$1")
git-parse-remote.sh:257:		uplp=$(git config --get "remote.$1.uploadpack")
git-pull.sh:15:test -z "$(git ls-files -u)" ||
git-pull.sh:57:orig_head=$(git rev-parse --verify HEAD 2>/dev/null)
git-pull.sh:60:curr_head=$(git rev-parse --verify HEAD 2>/dev/null)
git-pull.sh:83:merge_head=$(sed -e '/	not-for-merge	/d' \
git-pull.sh:89:	curr_branch=$(git symbolic-ref -q HEAD)
git-pull.sh:121:merge_name=$(git fmt-merge-msg <"$GIT_DIR/FETCH_HEAD") || exit
git-quiltimport.sh:12:		quilt_author=$(expr "z$1" : 'z-[^=]*\(.*\)')
git-quiltimport.sh:29:		QUILT_PATCHES=$(expr "z$1" : 'z-[^=]*\(.*\)')
git-quiltimport.sh:48:	quilt_author_name=$(expr "z$quilt_author" : 'z\(.*[^ ]\) *<.*') &&
git-quiltimport.sh:49:	quilt_author_email=$(expr "z$quilt_author" : '.*<\([^>]*\)') &&
git-quiltimport.sh:70:commit=$(git rev-parse HEAD)
git-quiltimport.sh:73:for patch_name in $(grep -v '^#' < "$QUILT_PATCHES/series" ); do
git-quiltimport.sh:83:	export GIT_AUTHOR_NAME=$(sed -ne 's/Author: //p' "$tmp_info")
git-quiltimport.sh:84:	export GIT_AUTHOR_EMAIL=$(sed -ne 's/Email: //p' "$tmp_info")
git-quiltimport.sh:102:			patch_author_name=$(expr "z$patch_author" : 'z\(.*[^ ]\) *<.*') &&
git-quiltimport.sh:103:			patch_author_email=$(expr "z$patch_author" : '.*<\([^>]*\)') &&
git-quiltimport.sh:110:	export GIT_AUTHOR_DATE=$(sed -ne 's/Date: //p' "$tmp_info")
git-quiltimport.sh:111:	export SUBJECT=$(sed -ne 's/Subject: //p' "$tmp_info")
git-quiltimport.sh:113:		SUBJECT=$(echo $patch_name | sed -e 's/.patch$//')
git-quiltimport.sh:118:		tree=$(git write-tree) &&
git-quiltimport.sh:119:		commit=$( (echo "$SUBJECT"; echo; cat "$tmp_msg") | git commit-tree $tree -p $commit) &&
git-rebase--interactive.sh:29:test -f "$DOTEST"/strategy && STRATEGY="$(cat "$DOTEST"/strategy)"
git-rebase--interactive.sh:73:	count=$(($(wc -l < "$DONE")))
git-rebase--interactive.sh:74:	total=$(($count+$(wc -l < "$TODO")))
git-rebase--interactive.sh:80:	parent_sha1=$(git rev-parse --verify "$1"^ 2> /dev/null)
git-rebase--interactive.sh:103:	parent_sha1=$(git rev-parse --verify $sha1^ 2>/dev/null)
git-rebase--interactive.sh:104:	current_sha1=$(git rev-parse --verify HEAD)
git-rebase--interactive.sh:108:		sha1=$(git rev-parse --short $sha1)
git-rebase--interactive.sh:117:	sha1=$(git rev-parse $sha1)
git-rebase--interactive.sh:121:		current_commit=$(cat "$DOTEST"/current-commit) &&
git-rebase--interactive.sh:131:	for p in $(git rev-list --parents -1 $sha1 | cut -d\  -f2-)
git-rebase--interactive.sh:136:			new_p=$(cat "$REWRITTEN"/$p)
git-rebase--interactive.sh:154:		first_parent=$(expr "$new_parents" : " \([^ ]*\)")
git-rebase--interactive.sh:163:			author_script=$(get_author_ident_from_commit $sha1)
git-rebase--interactive.sh:165:			msg="$(git cat-file commit $sha1 | \
git-rebase--interactive.sh:192:		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([0-9]\+\).*/\1/p" \
git-rebase--interactive.sh:204:	echo "# This is the $(nth_string $COUNT) commit message:"
git-rebase--interactive.sh:245:		test -z "$(grep -ve '^$' -e '^#' < $DONE)" &&
git-rebase--interactive.sh:250:		case "$(peek_next_command)" in
git-rebase--interactive.sh:265:		author_script=$(get_author_ident_from_commit $sha1)
git-rebase--interactive.sh:288:	HEADNAME=$(cat "$DOTEST"/head-name) &&
git-rebase--interactive.sh:289:	OLDHEAD=$(cat "$DOTEST"/head) &&
git-rebase--interactive.sh:290:	SHORTONTO=$(git rev-parse --short $(cat "$DOTEST"/onto)) &&
git-rebase--interactive.sh:294:			current_commit=$(cat "$DOTEST"/current-commit) &&
git-rebase--interactive.sh:296:		NEWHEAD=$(cat "$REWRITTEN"/$OLDHEAD)
git-rebase--interactive.sh:298:		NEWHEAD=$(git rev-parse HEAD)
git-rebase--interactive.sh:304:			git diff --stat $(cat "$DOTEST"/head)..HEAD
git-rebase--interactive.sh:344:		HEADNAME=$(cat "$DOTEST"/head-name)
git-rebase--interactive.sh:345:		HEAD=$(cat "$DOTEST"/head)
git-rebase--interactive.sh:400:			ONTO=$(git rev-parse --verify "$2") ||
git-rebase--interactive.sh:417:		HEAD=$(git rev-parse --verify HEAD) || die "No HEAD?"
git-rebase--interactive.sh:418:		UPSTREAM=$(git rev-parse --verify "$1") || die "Invalid base"
git-rebase--interactive.sh:441:			for c in $(git merge-base --all $HEAD $UPSTREAM)
git-rebase--interactive.sh:451:		SHORTUPSTREAM=$(git rev-parse --short $UPSTREAM)
git-rebase--interactive.sh:452:		SHORTHEAD=$(git rev-parse --short $HEAD)
git-rebase--interactive.sh:453:		SHORTONTO=$(git rev-parse --short $ONTO)
git-rebase--interactive.sh:470:		test -z "$(grep -ve '^$' -e '^#' < $TODO)" &&
git-rebase--interactive.sh:477:		test -z "$(grep -ve '^$' -e '^#' < $TODO)" &&
git-rebase.sh:54:	unmerged=$(git ls-files -u)
git-rebase.sh:82:	msgnum=$(($msgnum + 1))
git-rebase.sh:87:	cmt="$(cat $dotest/cmt.$1)"
git-rebase.sh:89:	hd=$(git rev-parse --verify HEAD)
git-rebase.sh:90:	cmt_name=$(git symbolic-ref HEAD)
git-rebase.sh:91:	msgnum=$(cat $dotest/msgnum)
git-rebase.sh:92:	end=$(cat $dotest/end)
git-rebase.sh:93:	eval GITHEAD_$cmt='"${cmt_name##refs/heads/}~$(($end - $msgnum))"'
git-rebase.sh:94:	eval GITHEAD_$hd='"$(cat $dotest/onto_name)"'
git-rebase.sh:167:			msgnum=$(($msgnum + 1))
git-rebase.sh:257:diff=$(git diff-index --cached --name-status -r HEAD)
git-rebase.sh:272:onto=$(git rev-parse --verify "${onto_name}^0") || exit
git-rebase.sh:298:branch=$(git rev-parse --verify "${branch_name}^0") || exit
git-rebase.sh:304:mb=$(git merge-base "$onto" "$branch")
git-rebase.sh:351:	msgnum=$(($msgnum + 1))
git-repack.sh:67:names=$(git pack-objects --non-empty --all --reflog $args </dev/null "$PACKTMP") ||
git-reset.sh:27:		rev=$(git rev-parse --verify "$1") || exit
git-reset.sh:36:rev=$(git rev-parse --verify $rev^0) || exit
git-reset.sh:69:	   test "" != "$(git ls-files --unmerged)"
git-reset.sh:78:if orig=$(git rev-parse --verify HEAD 2>/dev/null)
git-sh-setup.sh:32:	: "${GIT_EDITOR:=$(git config core.editor)}"
git-sh-setup.sh:52:	cdup=$(git rev-parse --show-cdup)
git-sh-setup.sh:63:	test $(git rev-parse --is-inside-work-tree) = true ||
git-sh-setup.sh:89:	encoding=$(git config i18n.commitencoding || echo UTF-8)
git-sh-setup.sh:113:	GIT_DIR=$(GIT_DIR="$GIT_DIR" git rev-parse --git-dir) || {
git-sh-setup.sh:119:	GIT_DIR=$(git rev-parse --git-dir) || exit
git-stash.sh:22:	if current=$(git rev-parse --verify $ref_stash 2>/dev/null)
git-stash.sh:43:	if b_commit=$(git rev-parse --verify HEAD)
git-stash.sh:45:		head=$(git log --abbrev-commit --pretty=oneline -n 1 HEAD)
git-stash.sh:50:	if branch=$(git symbolic-ref -q HEAD)
git-stash.sh:56:	msg=$(printf '%s: %s' "$branch" "$head")
git-stash.sh:59:	i_tree=$(git write-tree) &&
git-stash.sh:60:	i_commit=$(printf 'index on %s' "$msg" |
git-stash.sh:65:	w_tree=$( (
git-stash.sh:80:		stash_msg=$(printf 'WIP on %s' "$msg")
git-stash.sh:82:		stash_msg=$(printf 'On %s: %s' "$branch" "$stash_msg")
git-stash.sh:84:	w_commit=$(printf '%s\n' "$stash_msg" |
git-stash.sh:104:	flags=$(git rev-parse --no-revs --flags "$@")
git-stash.sh:109:	s=$(git rev-parse --revs-only --no-flags --default $ref_stash "$@")
git-stash.sh:111:	w_commit=$(git rev-parse --verify "$s") &&
git-stash.sh:112:	b_commit=$(git rev-parse --verify "$s^") &&
git-stash.sh:128:	c_tree=$(git write-tree) ||
git-stash.sh:133:	s=$(git rev-parse --revs-only --no-flags --default $ref_stash "$@") &&
git-stash.sh:134:	w_tree=$(git rev-parse --verify "$s:") &&
git-stash.sh:135:	b_tree=$(git rev-parse --verify "$s^1:") &&
git-stash.sh:136:	i_tree=$(git rev-parse --verify "$s^2:") ||
git-stash.sh:145:		unstashed_index_tree=$(git-write-tree) ||
git-submodule.sh:50:	re=$(printf '%s' "$1" | sed -e 's/\([^a-zA-Z0-9_]\)/\\\1/g')
git-submodule.sh:51:	name=$( GIT_CONFIG=.gitmodules \
git-submodule.sh:108:	if base=$(get_repo_base "$repo"); then
git-submodule.sh:114:		path=$(echo "$repo" | sed -e 's|/*$||' -e 's|:*/*\.git$||' -e 's|.*[/:]||g')
git-submodule.sh:116:		path=$(echo "$path" | sed -e 's|/*$||')
git-submodule.sh:148:		name=$(module_name "$path") || exit
git-submodule.sh:149:		url=$(git config submodule."$name".url)
git-submodule.sh:152:		url=$(GIT_CONFIG=.gitmodules git config submodule."$name".url)
git-submodule.sh:173:		name=$(module_name "$path") || exit
git-submodule.sh:174:		url=$(git config submodule."$name".url)
git-submodule.sh:189:			subsha1=$(unset GIT_DIR && cd "$path" &&
git-submodule.sh:206:	revname=$( (
git-submodule.sh:232:		name=$(module_name "$path") || exit
git-submodule.sh:233:		url=$(git config submodule."$name".url)
git-submodule.sh:246:				sha1=$(unset GIT_DIR && cd "$path" && git rev-parse --verify HEAD)
git-tag.sh:39:		LINES=$(expr "$1" : '\([0-9]*\)')
git-tag.sh:64:		OBJTYPE=$(git cat-file -t "$TAG")
git-tag.sh:67:			ANNOTATION=$(git cat-file tag "$TAG" |
git-tag.sh:100:	    message="$(cat "$1")"
git-tag.sh:121:		cur=$(git show-ref --verify --hash -- "refs/tags/$tag") || {
git-tag.sh:137:	tag=$(git show-ref --verify --hash -- "refs/tags/$tag_name") ||
git-tag.sh:165:object=$(git rev-parse --verify --default HEAD "$@") || exit 1
git-tag.sh:166:type=$(git cat-file -t $object) || exit 1
git-tag.sh:167:tagger=$(git-var GIT_COMMITTER_IDENT) || exit 1
git-tag.sh:170:	username=$(git repo-config user.signingkey) ||
git-tag.sh:171:	username=$(expr "z$tagger" : 'z\(.*>\)')
git-tag.sh:202:    object=$(git-mktag < "$GIT_DIR"/TAG_TMP)
git-verify-tag.sh:24:type="$(git cat-file -t "$1" 2>/dev/null)" ||

Grep finished (matches found) at Fri Aug  3 01:29:41


-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Shell script cleanups/style changes?

[Robert Schiele]

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

On Fri, Aug 03, 2007 at 01:32:07AM +0200, David Kastrup wrote:
> Robert Schiele <rschiele@gmail.com> writes:
> > Well, I have now a patch ready for submission that would replace all
> > occurences of ${PARAMETER#WORD}, ${PARAMETER%WORD}, and $((
> > EXPRESSION )).  But if you say that you won't accept replacement of
> > $( ... ) then this is not worth the effort since this one isn't
> > accepted as well.
> 
> I absolutely can't understand your claim that most things work for
> you, then.  Are you _really_, _really_ sure you are talking about
> /bin/sh here?

I started wondering myself and it turned out that we just didn't look in the
right place.  Actually we _have_ an infrastructure in place to replace the
shell.  (SHELL_PATH in the Makefile)  In that case I would not consider this
an issue and you might go on with the cleanup from my point of view.

Robert

-- 
Robert Schiele
Dipl.-Wirtsch.informatiker	mailto:rschiele@gmail.com

"Quidquid latine dictum sit, altum sonatur."

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

Re: Shell script cleanups/style changes?

[David Kastrup]

Robert Schiele <rschiele@gmail.com> writes:

> On Fri, Aug 03, 2007 at 01:32:07AM +0200, David Kastrup wrote:
>> Robert Schiele <rschiele@gmail.com> writes:
>> > Well, I have now a patch ready for submission that would replace all
>> > occurences of ${PARAMETER#WORD}, ${PARAMETER%WORD}, and $((
>> > EXPRESSION )).  But if you say that you won't accept replacement of
>> > $( ... ) then this is not worth the effort since this one isn't
>> > accepted as well.
>> 
>> I absolutely can't understand your claim that most things work for
>> you, then.  Are you _really_, _really_ sure you are talking about
>> /bin/sh here?
>
> I started wondering myself and it turned out that we just didn't
> look in the right place.  Actually we _have_ an infrastructure in
> place to replace the shell.  (SHELL_PATH in the Makefile) In that
> case I would not consider this an issue and you might go on with the
> cleanup from my point of view.

I was starting to doubt my sanity here.  Sorry that this thread
resulted in wasted work for you.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Shell script cleanups/style changes?

[Robert Schiele]

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

On Fri, Aug 03, 2007 at 01:52:32AM +0200, David Kastrup wrote:
> I was starting to doubt my sanity here.  Sorry that this thread
> resulted in wasted work for you.

That's ok.  I mean I didn't spot the SHELL_PATH thing in the first place
although I already had seen that one before (what I remember now).

At least in that case I don't have to worry any longer since I always can put
some compliant shell at some random place and use that one.

But at least you have learned now that not everything in the real world looks
like as it is written in some standard books. ;-)

Robert

-- 
Robert Schiele
Dipl.-Wirtsch.informatiker	mailto:rschiele@gmail.com

"Quidquid latine dictum sit, altum sonatur."

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

Re: Shell script cleanups/style changes?

[David Kastrup]

Robert Schiele <rschiele@gmail.com> writes:

> On Fri, Aug 03, 2007 at 01:52:32AM +0200, David Kastrup wrote:
>> I was starting to doubt my sanity here.  Sorry that this thread
>> resulted in wasted work for you.
>
> That's ok.  I mean I didn't spot the SHELL_PATH thing in the first
> place although I already had seen that one before (what I remember
> now).
>
> At least in that case I don't have to worry any longer since I
> always can put some compliant shell at some random place and use
> that one.
>
> But at least you have learned now that not everything in the real
> world looks like as it is written in some standard books. ;-)

You are aware that I am the maintainer of AUCTeX?  Which has an
autoconf-based setup that works under AIX, Solaris, MinGW, Cygwin,
HP/UX and a few other oddities, all with the respective native tools?

I know more about stupid shells than I really want to.  But that does
not change that I could not imagine a shell such as yours to work
"mostly" with the current git code base.

For autoconf, it is fine to call sed (and you would probably not
believe how small the portable language subset for sed is) all the
time.  Performance is not an issue.  For normal user commands, this is
different.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Shell script cleanups/style changes?

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> ... For autoconf, it is fine to call sed (and you would probably not
> believe how small the portable language subset for sed is) all the
> time.  Performance is not an issue.  For normal user commands, this is
> different.

Wow, you made me recall a painful past for myself.  AIX's sed
was so bad that I had to send in a few bugfixes to autoconf to
work it around.  I do not know if I should feel happy to find
somebody to commiserate with...

Re: Shell script cleanups/style changes?

[Robert Schiele]

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

On Fri, Aug 03, 2007 at 12:14:13AM +0200, David Kastrup wrote:
> A non-working rebase would seem rather tough.

It depends on how you work.  I for example always just push any change to a
linux machine where I do the actual integration work.  Thus I don't need much
commands on the Solaris machine.

> The "issues" are with Solaris, apparently.  There is always a price
> for portability.  If Solaris users can fix their problems with a
> global search and replace of the first line in *.sh, the question is
> whether it is worth the hassle of having unreadable but "portable"
> code.  After all, it has to be read also by humans.

Ok, but then we needed an infrastructure to replace the shell with a
configuration parameter.

> You are missing the line
> $ echo $0
> which is probably the most interesting one...  we don't need to be
> compatible with everything having a "$ " prompt, just with everything
> called "/bin/sh".

Your way of telling people that you are considering everybody besides you to
be a moron is somehow insulting.  You might wish to change that.

Sure I tested in on /bin/sh!

> See the rationale in my recently posted patch for implementing
> EDITOR/VISUAL support.  One needs to shell-quote stuff properly, and
> the shell is better at shell-quote magic than C is.

Ok, will look up that later.

> If you prepare a patch replacing all existing ${parameter#word} uses
> and get it accepted, I will not push for inclusion of my cleanup.
> 
> But you _really_ should go for it _now_.

I have it ready now.  Just waiting for the answer of Junio to my last mail.

Robert

-- 
Robert Schiele
Dipl.-Wirtsch.informatiker	mailto:rschiele@gmail.com

"Quidquid latine dictum sit, altum sonatur."

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

Re: Shell script cleanups/style changes?

[Florian Weimer]

* Robert Schiele:

>> Sure.  What about the git-rebase line using $(($end - $msgnum)) ?
>
> Bad on Solaris:
>
> $ uname -a
> SunOS solaris10-x64 5.10 Generic i86pc i386 i86pc
> $ end=1
> $ msgnum=5
> $ echo $(($end - $msgnum))
> syntax error: `(' unexpected
> $ 

Is this with /usr/xpg4/bin/sh or /bin/sh?  The latter is not POSIX and
should not be used by GIT, IMHO, otherwise there will be endless
issues in less-well-tested code paths.  Is rewriting the shebang lines
to use the POSIX shell an option for GIT?

Re: Shell script cleanups/style changes?

[Robert Schiele]

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

On Sat, Aug 04, 2007 at 09:10:00AM +0200, Florian Weimer wrote:
> * Robert Schiele:
> 
> >> Sure.  What about the git-rebase line using $(($end - $msgnum)) ?
> >
> > Bad on Solaris:
> >
> > $ uname -a
> > SunOS solaris10-x64 5.10 Generic i86pc i386 i86pc
> > $ end=1
> > $ msgnum=5
> > $ echo $(($end - $msgnum))
> > syntax error: `(' unexpected
> > $ 
> 
> Is this with /usr/xpg4/bin/sh or /bin/sh?  The latter is not POSIX and
> should not be used by GIT, IMHO, otherwise there will be endless
> issues in less-well-tested code paths.  Is rewriting the shebang lines
> to use the POSIX shell an option for GIT?

Hi Florian,

I recommend you read the other mails in this thread.  This issue is already
completely resolved.

Robert

-- 
Robert Schiele
Dipl.-Wirtsch.informatiker	mailto:rschiele@gmail.com

"Quidquid latine dictum sit, altum sonatur."

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

Re: Shell script cleanups/style changes?

[Junio C Hamano]

Robert Schiele <rschiele@gmail.com> writes:

> Hi Florian,
>
> I recommend you read the other mails in this thread.  This issue is already
> completely resolved.

To be a bit more helpful, a short summary is:

 * SHELL_PATH in Makefile lets you munge installed scripts;

 * /bin/ksh is usable on Solaris and xpg4 is fine too;

 * We need to draw a line somewhere, and the current rule is
   that we cannot afford to support a shell that does not
   understand $(cmd) substitution, as we haven't seen a shell
   that supports ${parameter#word} but not $(cmd).

Re: Shell script cleanups/style changes?

[David Kastrup]

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

> David Kastrup <dak@gnu.org> writes:
>
>> Understood.  But using ${...#...} and ${...:+...} does not exactly
>> seem to be news in the git code base.  Even though we have the
>> claim that Solaris' sh won't deal with the former.
>
> I do not think we have trouble with ${parameter#word}.  Much less
> with ${parameter+word}; it has been in /bin/sh forever.

Basically this should mean that the proposed cleanups (apart from a
forgotten shift I had to add) are tenable.

Given that another poster claimed that Solaris /bin/sh does not
support ${parameter#word}, making the suggested changes to git-commit
might actually be a good idea: ${parameter#word} is used in half a
dozen other (likely less used) utilities in various other places.  If
this is an overlooked regression, we want to make it non-overlookable
while we are still in testing, and git-commit would appear to be the
perfect candidate for that...

Depending on the feedback, we can either replace _all_ uses
everywhere, or accept it for good.

While I would think it perfectly understandable if you wanted to avoid
making an infamous "breaks all of Solaris release", _if_ ${...#...}
would indeed be fishy (and I somewhat doubt it), we are already there.
I have this cleaned-up version of git-commit.sh on a computer I can't
access right now.  I'll post the patch tomorrow.  Whether you want to
apply it to git.git remains at your discretion.  I would, however,
strongly urge Solaris and potentially other POSIXly impaired users to
aplly and test this patch: if it breaks (and it will do so pretty
obviously, pretty much being unable to parse any option), then this is
_quite_ alarming with regard to existing uses of ${...#...} and would
need to get addressed _very_ soon.

Frankly, I doubt that this would have escaped notice so far, however.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum