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:

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

Robert

Robert Schiele <rschiele@gmail.com> writes:

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.

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

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

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.

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

Robert

Hi,

On Thu, 2 Aug 2007, Robert Schiele wrote:

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:

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

David Kastrup wrote:

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

On 8/2/07, David Kastrup <dak@gnu.org> wrote:

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.

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

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.

On 8/2/07, Junio C Hamano <gitster@pobox.com> wrote:

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

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

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.

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

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

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

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

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.

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

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.

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

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.

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

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.

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

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.

On Thu, Aug 02, 2007 at 10:57:31PM +0200, David Kastrup wrote:

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.

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.

Bad on Solaris:

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

Why is it bad to call the editor from C?

On Thu, Aug 02, 2007 at 02:21:01PM -0700, Junio C Hamano wrote:

See above.

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

That one is ok for Solaris.

Robert

Robert Schiele <rschiele@gmail.com> writes:

A non-working rebase would seem rather tough.

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.

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

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.

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

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.

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

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

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.

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

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

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.

Junio C Hamano <gitster@pobox.com> 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.

David Kastrup <dak@gnu.org> writes:

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

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

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.

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

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.

On Fri, Aug 03, 2007 at 02:24:38AM +0200, David Kastrup wrote:

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 <rschiele@gmail.com> writes:

$(...) 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 ${...#...}.

On Thu, Aug 02, 2007 at 04:05:46PM -0700, Junio C Hamano wrote:

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 <rschiele@gmail.com> writes:

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

On Fri, Aug 03, 2007 at 01:32:07AM +0200, David Kastrup wrote:

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 <rschiele@gmail.com> writes:

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

On Fri, Aug 03, 2007 at 01:52:32AM +0200, David Kastrup wrote:

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 <rschiele@gmail.com> writes:

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

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

On Fri, Aug 03, 2007 at 12:14:13AM +0200, David Kastrup wrote:

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.

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

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!

Ok, will look up that later.

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

Robert

* Robert Schiele:

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?

On Sat, Aug 04, 2007 at 09:10:00AM +0200, Florian Weimer wrote:

Hi Florian,

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

Robert

Robert Schiele <rschiele@gmail.com> writes:

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

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

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.