Shell script cleanups/style changes?

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?

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

Robert Schiele [off-list ref] writes:

On Thu, Aug 02, 2007 at 12:44:22PM +0200, David Kastrup wrote:

quoted

! 		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

On 8/2/07, David Kastrup [off-list ref] 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.

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

Hi,

On Thu, 2 Aug 2007, Robert Schiele wrote:

On Thu, Aug 02, 2007 at 04:20:44PM +0200, David Kastrup wrote:

quoted

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

On Thu, Aug 02, 2007 at 06:05:00PM +0100, Johannes Schindelin wrote:

Hi,

On Thu, 2 Aug 2007, Robert Schiele wrote:

quoted

On Thu, Aug 02, 2007 at 04:20:44PM +0200, David Kastrup wrote:

quoted

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

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

David Kastrup wrote:

Robert Schiele [off-list ref] writes:

quoted

On Thu, Aug 02, 2007 at 12:44:22PM +0200, David Kastrup wrote:

quoted

! 		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