[PATCH 0/5] some shell portability fixes

[PATCH 0/5] some shell portability fixes

[Ralf Wildenhues]

Here's a bunch of rather trivial patches against master to fix
portability issues, mostly shell ones.  FWIW I did not test the
patches other than ensuring the set of testsuite errors remains
the same, and that I only did on one system.

All of the issues are ones that are encountered on some real
system, most are documented either in POSIX or in the shell
portability section of the Autoconf manual.

Cheers,
Ralf

[PATCH 1/5] Avoid a few unportable, needlessly nested "...`...".

[Ralf Wildenhues]

Some shells wrongly interpret double-quote backquote double-quote
nesting.  OTOH, the right hand side of an assignment is never split
in words, so the outer double quotes are needless there.
---
 git-rebase--interactive.sh |    2 +-
 git-request-pull.sh        |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 76dc679..ecc6778 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -391,7 +391,7 @@ do
 	-s|--strategy)
 		case "$#,$1" in
 		*,*=*)
-			STRATEGY="-s `expr "z$1" : 'z-[^=]*=\(.*\)'`" ;;
+			STRATEGY="-s "`expr "z$1" : 'z-[^=]*=\(.*\)'` ;;
 		1,*)
 			usage ;;
 		*)
diff --git a/git-request-pull.sh b/git-request-pull.sh
index a992430..ec367d7 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -24,7 +24,7 @@ headrev=`git rev-parse --verify "$head"^0` || exit
 merge_base=`git merge-base $baserev $headrev` ||
 die "fatal: No commits in common between $base and $head"
 
-url="`get_remote_url "$url"`"
+url=`get_remote_url "$url"`
 branch=`git peek-remote "$url" \
 	| sed -n -e "/^$headrev	refs.heads./{
 		s/^.*	refs.heads.//
-- 
1.5.3.5.561.g140d

[PATCH 2/5] Fix sed script to work with AIX sed.

[Ralf Wildenhues]

\n is not portable in a s/// replacement string, only
in the regex part.  backslash-newline helps.
---
 git-bisect.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index b74f44d..1ed44e5 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -275,7 +275,8 @@ exit_if_skipped_commits () {
 	if expr "$_tried" : ".*[|].*" > /dev/null ; then
 		echo "There are only 'skip'ped commit left to test."
 		echo "The first bad commit could be any of:"
-		echo "$_tried" | sed -e 's/[|]/\n/g'
+		echo "$_tried" | sed -e 's/[|]/\
+/g'
 		echo "We cannot bisect more!"
 		exit 2
 	fi
-- 
1.5.3.5.561.g140d

Re: [PATCH 3/5] Replace $((...)) with expr invocations.

[Ralf Wildenhues]

---
 git-filter-branch.sh       |    4 ++--
 git-rebase--interactive.sh |    8 ++++----
 git-rebase.sh              |    8 ++++----
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index ffcc408..2d5c247 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -253,7 +253,7 @@ test $commits -eq 0 && die "Found nothing to rewrite"
 
 i=0
 while read commit parents; do
-	i=$(($i+1))
+	i=$(expr $i + 1)
 	printf "\rRewrite $commit ($i/$commits)"
 
 	case "$filter_subdir" in
@@ -374,7 +374,7 @@ do
 	;;
 	esac
 	git update-ref -m "filter-branch: backup" "$orig_namespace$ref" $sha1
-	count=$(($count+1))
+	count=$(expr $count + 1)
 done < "$tempdir"/heads
 
 # TODO: This should possibly go, with the semantics that all positive given
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index ecc6778..da48aaf 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -71,8 +71,8 @@ mark_action_done () {
 	sed -e 1q < "$TODO" >> "$DONE"
 	sed -e 1d < "$TODO" >> "$TODO".new
 	mv -f "$TODO".new "$TODO"
-	count=$(($(grep -ve '^$' -e '^#' < "$DONE" | wc -l)))
-	total=$(($count+$(grep -ve '^$' -e '^#' < "$TODO" | wc -l)))
+	count=$(grep -ve '^$' -e '^#' < "$DONE" | wc -l)
+	total=$(expr $count + $(grep -ve '^$' -e '^#' < "$TODO" | wc -l))
 	printf "Rebasing (%d/%d)\r" $count $total
 	test -z "$VERBOSE" || echo
 }
@@ -205,8 +205,8 @@ nth_string () {
 
 make_squash_message () {
 	if test -f "$SQUASH_MSG"; then
-		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
-			< "$SQUASH_MSG" | tail -n 1)+1))
+		COUNT=$(expr $(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
+			< "$SQUASH_MSG" | tail -n 1) + 1)
 		echo "# This is a combination of $COUNT commits."
 		sed -n "2,\$p" < "$SQUASH_MSG"
 	else
diff --git a/git-rebase.sh b/git-rebase.sh
index 224cca9..daa347a 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -79,7 +79,7 @@ continue_merge () {
 	echo "$prev_head" > "$dotest/prev_head"
 
 	# onto the next patch:
-	msgnum=$(($msgnum + 1))
+	msgnum=$(expr $msgnum + 1)
 	echo "$msgnum" >"$dotest/msgnum"
 }
 
@@ -90,7 +90,7 @@ call_merge () {
 	cmt_name=$(git symbolic-ref HEAD)
 	msgnum=$(cat "$dotest/msgnum")
 	end=$(cat "$dotest/end")
-	eval GITHEAD_$cmt='"${cmt_name##refs/heads/}~$(($end - $msgnum))"'
+	eval GITHEAD_$cmt='"${cmt_name##refs/heads/}~$(expr $end - $msgnum)"'
 	eval GITHEAD_$hd='$(cat "$dotest/onto_name")'
 	export GITHEAD_$cmt GITHEAD_$hd
 	git-merge-$strategy "$cmt^" -- "$hd" "$cmt"
@@ -163,7 +163,7 @@ do
 			prev_head=$(cat "$dotest/prev_head")
 			end=$(cat "$dotest/end")
 			msgnum=$(cat "$dotest/msgnum")
-			msgnum=$(($msgnum + 1))
+			msgnum=$(expr $msgnum + 1)
 			onto=$(cat "$dotest/onto")
 			while test "$msgnum" -le "$end"
 			do
@@ -349,7 +349,7 @@ echo "$prev_head" > "$dotest/prev_head"
 msgnum=0
 for cmt in `git rev-list --reverse --no-merges "$upstream"..ORIG_HEAD`
 do
-	msgnum=$(($msgnum + 1))
+	msgnum=$(expr $msgnum + 1)
 	echo "$cmt" > "$dotest/cmt.$msgnum"
 done
 
-- 
1.5.3.5.561.g140d

[PATCH 4/5] Fix sed string regex escaping in module_name.

[Ralf Wildenhues]

When escaping a string to be used as a sed regex, it is important
to only escape active characters.  Escaping other characters is
undefined according to POSIX, and may lead to problems with some
sed's extensions.
---
With this one, I'm not sure whether it is necessary, simply because I
don't know what kinds of characters can be part of a submodule path.
It anyway should be safe though.

 git-submodule.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 4aaaaab..5af28ec 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -73,7 +73,7 @@ resolve_relative_url ()
 module_name()
 {
 	# Do we have "submodule.<something>.path = $1" defined in .gitmodules file?
-	re=$(printf '%s' "$1" | sed -e 's/\([^a-zA-Z0-9_]\)/\\\1/g')
+	re=$(printf '%s' "$1" | sed -e 's/[].[^$\\*]/\\&/g')
 	name=$( GIT_CONFIG=.gitmodules \
 		git config --get-regexp '^submodule\..*\.path$' |
 		sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' )
-- 
1.5.3.5.561.g140d

[PATCH 5/5] Avoid "test -o" and "test -a" which are not POSIX, only XSI.

[Ralf Wildenhues]

---
 git-clone.sh        |    2 +-
 git-commit.sh       |    4 ++--
 git-merge-stupid.sh |    2 +-
 git-merge.sh        |    4 ++--
 git-mergetool.sh    |    2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/git-clone.sh b/git-clone.sh
index 3f00693..1ae137a 100755
--- a/git-clone.sh
+++ b/git-clone.sh
@@ -462,7 +462,7 @@ then
 
 	case "$no_checkout" in
 	'')
-		test "z$quiet" = z -a "z$no_progress" = z && v=-v || v=
+		test "z$quiet" = z && test "z$no_progress" = z && v=-v || v=
 		git read-tree -m -u $v HEAD HEAD
 	esac
 fi
diff --git a/git-commit.sh b/git-commit.sh
index fcb8443..78fe518 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -50,7 +50,7 @@ run_status () {
 		export GIT_INDEX_FILE
 	fi
 
-	if test "$status_only" = "t" -o "$use_status_color" = "t"; then
+	if test "$status_only" = "t" || test "$use_status_color" = "t"; then
 		color=
 	else
 		color=--nocolor
@@ -290,7 +290,7 @@ t,,[1-9]*)
 	die "No paths with -i does not make sense." ;;
 esac
 
-if test ! -z "$templatefile" -a -z "$log_given"
+if test ! -z "$templatefile" && test -z "$log_given"
 then
 	if test ! -f "$templatefile"
 	then
diff --git a/git-merge-stupid.sh b/git-merge-stupid.sh
index f612d47..95adbb3 100755
--- a/git-merge-stupid.sh
+++ b/git-merge-stupid.sh
@@ -44,7 +44,7 @@ case "$bases" in
 			 2>/dev/null ||	continue
 		# Count the paths that are unmerged.
 		cnt=`GIT_INDEX_FILE=$G git ls-files --unmerged | wc -l`
-		if test $best_cnt -le 0 -o $cnt -le $best_cnt
+		if test $best_cnt -le 0 || test $cnt -le $best_cnt
 		then
 			best=$c
 			best_cnt=$cnt
diff --git a/git-merge.sh b/git-merge.sh
index c2092a2..b6a35d4 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -124,7 +124,7 @@ merge_name () {
 		git show-ref -q --verify "refs/heads/$truname" 2>/dev/null
 	then
 		echo "$rh		branch '$truname' (early part) of ."
-	elif test "$remote" = "FETCH_HEAD" -a -r "$GIT_DIR/FETCH_HEAD"
+	elif test "$remote" = "FETCH_HEAD" && test -r "$GIT_DIR/FETCH_HEAD"
 	then
 		sed -e 's/	not-for-merge	/		/' -e 1q \
 			"$GIT_DIR/FETCH_HEAD"
@@ -478,7 +478,7 @@ do
 		git diff-files --name-only
 		git ls-files --unmerged
 	    } | wc -l`
-	    if test $best_cnt -le 0 -o $cnt -le $best_cnt
+	    if test $best_cnt -le 0 || test $cnt -le $best_cnt
 	    then
 		best_strategy=$strategy
 		best_cnt=$cnt
diff --git a/git-mergetool.sh b/git-mergetool.sh
index a68b403..9f7386b 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -167,7 +167,7 @@ merge_file () {
     local_present  && git cat-file blob ":2:$prefix$path" >"$LOCAL" 2>/dev/null
     remote_present && git cat-file blob ":3:$prefix$path" >"$REMOTE" 2>/dev/null
 
-    if test -z "$local_mode" -o -z "$remote_mode"; then
+    if test -z "$local_mode" || test -z "$remote_mode"; then
 	echo "Deleted merge conflict for '$path':"
 	describe_file "$local_mode" "local" "$LOCAL"
 	describe_file "$remote_mode" "remote" "$REMOTE"
-- 
1.5.3.5.561.g140d

Re: [PATCH 3/5] Replace $((...)) with expr invocations.

[Ralf Wildenhues]

* Ralf Wildenhues wrote on Tue, Nov 06, 2007 at 09:18:09PM CET:
> ---
>  git-filter-branch.sh       |    4 ++--
>  git-rebase--interactive.sh |    8 ++++----
>  git-rebase.sh              |    8 ++++----
>  3 files changed, 10 insertions(+), 10 deletions(-)

Hmm, maybe this one is overkill.  $((...)) is POSIX, I temporarily
forgot (thanks Benoît!).

I'm unsure whether git targets non-POSIX Bourne shells like Solaris
/bin/sh.  That would however mean replacing stuff like $(cmd) with
`cmd` as well, and from grepping the source it looks like you'd rather
avoid that.

Cheers,
Ralf

Re: [PATCH 0/5] some shell portability fixes

[Junio C Hamano]

All missing Signed-off-by: lines.

[1/5] In addition to take advantage of the fact that the RHS of
      assignment is not split, I'd prefer replacing `` with $()
      with these cases.	 Much easier to read if your shell
      supports it (and all the modern ones do).

[2/5] Gaah, AIX sed X-<.  I am not opposed to this patch but
      would want to get Yays from people with non GNU sed.  Is
      busybox sed good enough to grok our scripts these days?
      Please ask help and collect Acks at least from folks on
      Solaris, MacOS, FBSD, and OBSD.

[3/5] Arithmetic expansion.  Have you caught _all_ of them, or
      is this patch about only the ones you noticed?

      We used to have expr all over the place as I was one of
      the primary authors of our shell scripts, and I am
      "80-ish" old fashioned.  There was a long discussion on
      scripts in the past and we ruled that $(( ... )) is easier
      to read and supported widely enough to be acceptable.

      This patch goes backwards.  Will drop, unless you can
      demonstrate that an implementation does not support it and
      convince people that the implementation is important.
      /bin/sh on Solaris does not count as you can configure
      SHELL_PATH to point at xpg4 shell or ksh on that platform.

[4/5] I wonder if use of fgrep would be easier to read and more
      portable with this one:

	name=$( GIT_CONFIG=.gitmodules \
		git config --get-regexp '^submodule\..*\.path$' |
		fgrep "submodule.$1.path" |
		sed -e 's/^submodule\.\(.*\)\.path$/\1/'
	)

[5/5] Again, have you covered all of them?  I am not opposed to
      this one, although I am a bit curious who lacks -a/-o in
      practice.

Re: [PATCH 0/5] some shell portability fixes

[Mike Hommey]

On Tue, Nov 06, 2007 at 12:46:35PM -0800, Junio C Hamano wrote:
> [5/5] Again, have you covered all of them?  I am not opposed to
>       this one, although I am a bit curious who lacks -a/-o in
>       practice.

Solaris's /bin/sh, but it already doesn't support $() and other stuff
used all over the place in git, so it's not like it's changing anything.

Maybe some other obscure old crappy shell ?

Mike

Re: [PATCH 3/5] Replace $((...)) with expr invocations.

[Junio C Hamano]

Ralf Wildenhues <Ralf.Wildenhues@gmx.de> writes:

> * Ralf Wildenhues wrote on Tue, Nov 06, 2007 at 09:18:09PM CET:
>> ---
>>  git-filter-branch.sh       |    4 ++--
>>  git-rebase--interactive.sh |    8 ++++----
>>  git-rebase.sh              |    8 ++++----
>>  3 files changed, 10 insertions(+), 10 deletions(-)
>
> Hmm, maybe this one is overkill.  $((...)) is POSIX, I temporarily
> forgot (thanks Benoît!).
>
> I'm unsure whether git targets non-POSIX Bourne shells like Solaris
> /bin/sh.  That would however mean replacing stuff like $(cmd) with
> `cmd` as well, and from grepping the source it looks like you'd rather
> avoid that.

For git, two rough rules are:

 - Most importantly, we never say "It's in POSIX; we'll happily
   screw your system that does not conform."  We live in the
   real world.

 - However, we often say "Let's stay away from that construct,
   it's not even in POSIX".

For shell scripts specifically (not exhaustive):

 - We prefer $( ... ) for command substitution; unlike ``, it
   properly nests.  It should have been the way Bourne spelled
   it from day one, but unfortunately isn't.

 - We use ${parameter-word} and its [-=?+] siblings, and their
   colon'ed "unset or null" form.

 - We use ${parameter#word} and its [#%] siblings, and their
   doubled "longest matching" form.

 - We use Arithmetic Expansion $(( ... )).

 - No "Substring Expansion" ${parameter:offset:length}.

 - No shell arrays.

 - No strlen ${#parameter}.

 - No regexp ${parameter/pattern/string}.

 - We do not use Process Substitution <(list) or >(list).

 - We prefer "test" over "[ ... ]".

 - We do not write noiseword "function" in front of shell
   functions.

Re: [PATCH 0/5] some shell portability fixes

[Ralf Wildenhues]

Hello Junio,

* Junio C Hamano wrote on Tue, Nov 06, 2007 at 09:46:35PM CET:
> All missing Signed-off-by: lines.

Oops.  Sorry.

> [1/5] In addition to take advantage of the fact that the RHS of
>       assignment is not split, I'd prefer replacing `` with $()
>       with these cases.	 Much easier to read if your shell
>       supports it (and all the modern ones do).

OK.

> [2/5] Gaah, AIX sed X-<.  I am not opposed to this patch but
>       would want to get Yays from people with non GNU sed.  Is
>       busybox sed good enough to grok our scripts these days?
>       Please ask help and collect Acks at least from folks on
>       Solaris, MacOS, FBSD, and OBSD.

FWIW, I have little experience with busybox sed, but for the others here
you go:  With

echo axbyc | sed 's,x,\n,; s,y,\
,'

I get on OpenBSD, FreeBSD, Solaris, and Darwin (minus indentation):
  anb
  c

GNU sed gives
  a
  b
  c

> [3/5] Arithmetic expansion.  Have you caught _all_ of them, or
>       is this patch about only the ones you noticed?

I have grepped *.sh.  But let's drop that, I see that it goes backwards.

> [4/5] I wonder if use of fgrep would be easier to read and more
>       portable with this one:
> 
> 	name=$( GIT_CONFIG=.gitmodules \
> 		git config --get-regexp '^submodule\..*\.path$' |
> 		fgrep "submodule.$1.path" |
> 		sed -e 's/^submodule\.\(.*\)\.path$/\1/'
> 	)

Certainly easier to read.  But fgrep itself is not portable (it could be
grep -F).  Also, isn't the $1 to be matched at the end, after a "="
here?  FWIW the pattern I posted has survived a few years in Automake,
so there is some hope that it works.

> [5/5] Again, have you covered all of them?

No, oops again.  As I searched for `test.*-[oa]' I have missed line
wraps and [ ... -o ... ].

>       I am not opposed to
>       this one, although I am a bit curious who lacks -a/-o in
>       practice.

Hmm, good question.  I actually don't know whether there is a shell
that isn't ruled out by $() anyway.  Let's drop that one, too, then.

Cheers,
Ralf

[PATCH] Add Documentation/CodingStyle

[Johannes Schindelin]

Even if our code is quite a good documentation for our coding style,
some people seem to prefer a document describing it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	I long resisted in adding this, as I really believe our code
	base is a very clean one, and a good description of what we
	prefer.

	But it seems that not everybody has the time to study our
	code in depth, beautiful as it is ;-)

	BTW the first to catch the allusion to a certain movie 
	wins a drink with me.

 Documentation/CodingStyle |   87 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 87 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/CodingStyle

diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
new file mode 100644
index 0000000..622b80b
--- /dev/null
+++ b/Documentation/CodingStyle
@@ -0,0 +1,87 @@
+As a popular project, we also have some guidelines to keep to the
+code.  For git in general, two rough rules are:
+
+ - Most importantly, we never say "It's in POSIX; we'll happily
+   screw your system that does not conform."  We live in the
+   real world.
+
+ - However, we often say "Let's stay away from that construct,
+   it's not even in POSIX".
+
+As for more concrete guidelines, just imitate the existing code
+(this is a good guideline, no matter which project you are contributing
+to...).  But if you must have some list of rules, here they are.
+
+For shell scripts specifically (not exhaustive):
+
+ - We prefer $( ... ) for command substitution; unlike ``, it
+   properly nests.  It should have been the way Bourne spelled
+   it from day one, but unfortunately isn't.
+
+ - We use ${parameter-word} and its [-=?+] siblings, and their
+   colon'ed "unset or null" form.
+
+ - We use ${parameter#word} and its [#%] siblings, and their
+   doubled "longest matching" form.
+
+ - We use Arithmetic Expansion $(( ... )).
+
+ - No "Substring Expansion" ${parameter:offset:length}.
+
+ - No shell arrays.
+
+ - No strlen ${#parameter}.
+
+ - No regexp ${parameter/pattern/string}.
+
+ - We do not use Process Substitution <(list) or >(list).
+
+ - We prefer "test" over "[ ... ]".
+
+ - We do not write noiseword "function" in front of shell
+   functions.
+
+For C programs:
+
+ - Use tabs to increment, and interpret tabs as taking up to 8 spaces
+
+ - Try to keep to 80 characters per line
+
+ - When declaring pointers, the star sides with the variable name, i.e.
+   "char *string", not "char* string" or "char * string".  This makes
+   it easier to understand "char *string, c;"
+
+ - Do not use curly brackets unnecessarily.  I.e.
+
+	if (bla) {
+		x = 1;
+	}
+
+   is frowned upon.  A gray area is when the statement extends over a
+   few lines, and/or you have a lengthy comment atop of it.
+
+ - Try to make your code understandable.  You may put comments in, but
+   comments invariably tend to stale out when the code they were
+   describing changes.  Often splitting a function into two makes the
+   intention of the code much clearer.
+
+   Double negation is often harder to understand than no negation at
+   all.
+
+   Some clever tricks, like using the !! operator with arithmetic
+   constructs, can be extremely confusing to others.  Avoid them,
+   unless there is a compelling reason to use them.
+
+ - Use the API.  No, really.  We have a strbuf (variable length string),
+   several arrays with the ALLOC_GROW() macro, a path_list for sorted
+   string lists, a hash map (mapping struct objects) named
+   "struct decorate", amongst other things.
+
+ - #include system headers in git-compat-util.h.  Some headers on some
+   systems show subtle breakages when you change the order, so it is
+   best to keep them in one place.
+
+ - if you are planning a new command, consider writing it in shell or
+   perl first, so that changes in semantics can be easily changed and
+   discussed.  Many git commands started out like that, and a few are
+   still scripts.
-- 
1.5.3.5.1597.g7191

Re: [PATCH 0/5] some shell portability fixes

[Johannes Schindelin]

Hi,

On Tue, 6 Nov 2007, Mike Hommey wrote:

> On Tue, Nov 06, 2007 at 12:46:35PM -0800, Junio C Hamano wrote:
> > [5/5] Again, have you covered all of them?  I am not opposed to
> >       this one, although I am a bit curious who lacks -a/-o in
> >       practice.
> 
> Solaris's /bin/sh, but it already doesn't support $() and other stuff
> used all over the place in git, so it's not like it's changing anything.
> 
> Maybe some other obscure old crappy shell ?

As Junio commented in the part you did not quote, there are better shells 
in Solaris.  Use those.

Ciao,
Dscho

Re: [PATCH] Add Documentation/CodingStyle

[Andreas Ericsson]

Johannes Schindelin wrote:
> Even if our code is quite a good documentation for our coding style,
> some people seem to prefer a document describing it.
> 

Sweet. You just saved me 40 minutes of writing one just like that for
company use. I owe you a drink, and then you can tell me what movie
you alluded to ;-)

> +
> + - if you are planning a new command, consider writing it in shell or
> +   perl first, so that changes in semantics can be easily changed and
> +   discussed.  Many git commands started out like that, and a few are
> +   still scripts.

I'd skip this part though and just add a pointer to contrib/examples/,
saying something along the lines of

- if you're planning a new command, sneak a peak in contrib/examples/
  for ample study-material of retired git commands implemented in perl
  and shell.

Possibly with s/retired // on that paragraph.

There's nothing particular wrong with writing in C from the start after
all.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] Add Documentation/CodingStyle

[Junio C Hamano]

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

> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> new file mode 100644
> index 0000000..622b80b
> --- /dev/null
> +++ b/Documentation/CodingStyle
> @@ -0,0 +1,87 @@
> +As a popular project, we also have some guidelines to keep to the
> +code.  For git in general, two rough rules are:
> +
> + - Most importantly, we never say "It's in POSIX; we'll happily
> +   screw your system that does not conform."  We live in the
> +   real world.
> +
> + - However, we often say "Let's stay away from that construct,
> +   it's not even in POSIX".
> +

I am not sure if we want to have CodingStyle document, but the
above are not CodingStyle issues.

If we are to write this down, I'd like to have the more
important third rule, which is:

 - In spite of the above two rules, we sometimes say "Although
   this is not in POSIX, it (is so convenient | makes the code
   much more readable | has other good characteristics) and
   practically all the platforms we care about support it, so
   let's use it".  Again, we live in the real world, and it is
   sometimes a judgement call, decided based more on real world
   constraints people face than what the paper standard says.

> +For C programs:
> +
> + - Use tabs to increment, and interpret tabs as taking up to 8 spaces

What's the character for decrement?  DEL? ;-)

> +   Double negation is often harder to understand than no negation at
> +   all.
> +
> +   Some clever tricks, like using the !! operator with arithmetic
> +   constructs, can be extremely confusing to others.  Avoid them,
> +   unless there is a compelling reason to use them.

I actually think (!!var) idiom is already established in our
codebase.

> + - Use the API.  No, really.  We have a strbuf (variable length string),
> +   several arrays with the ALLOC_GROW() macro, a path_list for sorted
> +   string lists, a hash map (mapping struct objects) named
> +   "struct decorate", amongst other things.

 - When you come up with an API, document it.

> + - if you are planning a new command, consider writing it in shell or
> +   perl first, so that changes in semantics can be easily changed and
> +   discussed.  Many git commands started out like that, and a few are
> +   still scripts.

No Python allowed?

Re: [PATCH] Add Documentation/CodingStyle

[Wincent Colaiuta]

El 7/11/2007, a las 0:17, Johannes Schindelin escribió:

> Even if our code is quite a good documentation for our coding style,
> some people seem to prefer a document describing it.

Great idea, Johannes, especially your nice concise summary of  
conventions for shell-scripts (which is an area where we most often  
see list traffic on which way to write things).

Cheers,
Wincent

Re: [PATCH] Add Documentation/CodingStyle

[Andreas Ericsson]

Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
>> new file mode 100644
>> index 0000000..622b80b
>> --- /dev/null
>> +++ b/Documentation/CodingStyle
>> @@ -0,0 +1,87 @@
>> +As a popular project, we also have some guidelines to keep to the
>> +code.  For git in general, two rough rules are:
>> +
>> + - Most importantly, we never say "It's in POSIX; we'll happily
>> +   screw your system that does not conform."  We live in the
>> +   real world.
>> +
>> + - However, we often say "Let's stay away from that construct,
>> +   it's not even in POSIX".
>> +
> 
> I am not sure if we want to have CodingStyle document, but the
> above are not CodingStyle issues.
> 
> If we are to write this down, I'd like to have the more
> important third rule, which is:
> 
>  - In spite of the above two rules, we sometimes say "Although
>    this is not in POSIX, it (is so convenient | makes the code
>    much more readable | has other good characteristics) and
>    practically all the platforms we care about support it, so
>    let's use it".  Again, we live in the real world, and it is
>    sometimes a judgement call, decided based more on real world
>    constraints people face than what the paper standard says.
> 
>> +For C programs:
>> +
>> + - Use tabs to increment, and interpret tabs as taking up to 8 spaces
> 
> What's the character for decrement?  DEL? ;-)
> 
>> +   Double negation is often harder to understand than no negation at
>> +   all.
>> +
>> +   Some clever tricks, like using the !! operator with arithmetic
>> +   constructs, can be extremely confusing to others.  Avoid them,
>> +   unless there is a compelling reason to use them.
> 
> I actually think (!!var) idiom is already established in our
> codebase.
> 

Not very widely for arithmetic expressions though. I believe this
alludes to the (!!a + !!b + !!c) idiom used earlier, where it's not
exactly clear *why* a, b and c can be > 1 if != 0 is the only thing
we care about.

>> + - Use the API.  No, really.  We have a strbuf (variable length string),
>> +   several arrays with the ALLOC_GROW() macro, a path_list for sorted
>> +   string lists, a hash map (mapping struct objects) named
>> +   "struct decorate", amongst other things.
> 
>  - When you come up with an API, document it.
> 
>> + - if you are planning a new command, consider writing it in shell or
>> +   perl first, so that changes in semantics can be easily changed and
>> +   discussed.  Many git commands started out like that, and a few are
>> +   still scripts.
> 
> No Python allowed?


Perhaps with this addendum?

- Think very, very hard before introducing a new dependency into git. This
  means you should stay away from scripting languages not already used in
  the git core command set unless your command is clearly separate from it,
  such as an importer to convert random-scm-X repositories to git.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] Add Documentation/CodingStyle

[Andreas Ericsson]

Wincent Colaiuta wrote:
> El 7/11/2007, a las 0:17, Johannes Schindelin escribió:
> 
>> Even if our code is quite a good documentation for our coding style,
>> some people seem to prefer a document describing it.
> 
> Great idea, Johannes, especially your nice concise summary of 
> conventions for shell-scripts (which is an area where we most often see 
> list traffic on which way to write things).
> 

That was ripped from a mail Junio sent to the list though ;-)

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH 0/5] some shell portability fixes

[Mike Ralphson]

Junio C Hamano wrote on Tue, Nov 06, 2007 at 09:46:35PM CET:
> [2/5] Gaah, AIX sed X-<.  I am not opposed to this patch but
>       would want to get Yays from people with non GNU sed.  Is
>       busybox sed good enough to grok our scripts these days?
>       Please ask help and collect Acks at least from folks on
>       Solaris, MacOS, FBSD, and OBSD.

On Nov 6, 2007 11:25 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> As Junio commented in the part you did not quote, there are better shells
> in Solaris.  Use those.

Equally GNU sed is available as a drop-in rpm for AIX. I wonder if it
would be worth adding
Makefile support for a PATH prefix for the git scripts, so they could
prepend (in this case)
something like /opt/freeware/bin or /usr/linux/bin ?

In our AIX environment many GNU tools are installed but I can't
guarantee they come first
in the paths of the git users.

I'm willing to work up a patch if there's any interest.

Mike

Re: [PATCH 0/5] some shell portability fixes

[Johannes Schindelin]

Hi,

On Wed, 7 Nov 2007, Mike Ralphson wrote:

> Junio C Hamano wrote on Tue, Nov 06, 2007 at 09:46:35PM CET:
> > [2/5] Gaah, AIX sed X-<.  I am not opposed to this patch but
> >       would want to get Yays from people with non GNU sed.  Is
> >       busybox sed good enough to grok our scripts these days?
> >       Please ask help and collect Acks at least from folks on
> >       Solaris, MacOS, FBSD, and OBSD.
> 
> On Nov 6, 2007 11:25 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > As Junio commented in the part you did not quote, there are better shells
> > in Solaris.  Use those.
> 
> Equally GNU sed is available as a drop-in rpm for AIX. I wonder if it
> would be worth adding
> Makefile support for a PATH prefix for the git scripts, so they could
> prepend (in this case)
> something like /opt/freeware/bin or /usr/linux/bin ?
> 
> In our AIX environment many GNU tools are installed but I can't
> guarantee they come first
> in the paths of the git users.
> 
> I'm willing to work up a patch if there's any interest.

Would that be a task for configure?  Because I am not sure if the GNU 
tools are installed in the same place on all AIX boxen...

Ciao,
Dscho

Re: [PATCH] Add Documentation/CodingStyle

[Johannes Schindelin]

Hi,

On Tue, 6 Nov 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> > new file mode 100644
> > index 0000000..622b80b
> > --- /dev/null
> > +++ b/Documentation/CodingStyle
> > @@ -0,0 +1,87 @@
> > +As a popular project, we also have some guidelines to keep to the
> > +code.  For git in general, two rough rules are:
> > +
> > + - Most importantly, we never say "It's in POSIX; we'll happily
> > +   screw your system that does not conform."  We live in the
> > +   real world.
> > +
> > + - However, we often say "Let's stay away from that construct,
> > +   it's not even in POSIX".
> > +
> 
> I am not sure if we want to have CodingStyle document, but the
> above are not CodingStyle issues.

Would you like to call it CodingConventions?

> If we are to write this down, I'd like to have the more
> important third rule, which is:
> 
>  - In spite of the above two rules, we sometimes say "Although
>    this is not in POSIX, it (is so convenient | makes the code
>    much more readable | has other good characteristics) and
>    practically all the platforms we care about support it, so
>    let's use it".  Again, we live in the real world, and it is
>    sometimes a judgement call, decided based more on real world
>    constraints people face than what the paper standard says.

Okay, will add.

> > +For C programs:
> > +
> > + - Use tabs to increment, and interpret tabs as taking up to 8 spaces
> 
> What's the character for decrement?  DEL? ;-)

Hehe.  As you undoubtedly guessed, I meant "indent"...

> > +   Double negation is often harder to understand than no negation at
> > +   all.
> > +
> > +   Some clever tricks, like using the !! operator with arithmetic
> > +   constructs, can be extremely confusing to others.  Avoid them,
> > +   unless there is a compelling reason to use them.
> 
> I actually think (!!var) idiom is already established in our
> codebase.

Yep, but when there are easier variants, AFAICT they were preferred.

> > + - Use the API.  No, really.  We have a strbuf (variable length string),
> > +   several arrays with the ALLOC_GROW() macro, a path_list for sorted
> > +   string lists, a hash map (mapping struct objects) named
> > +   "struct decorate", amongst other things.
> 
>  - When you come up with an API, document it.

Okay.

> > + - if you are planning a new command, consider writing it in shell or
> > +   perl first, so that changes in semantics can be easily changed and
> > +   discussed.  Many git commands started out like that, and a few are
> > +   still scripts.
> 
> No Python allowed?

Well, maybe we should allow that again.  Although I hoped we are over that 
now, as it would complicate the msysGit efforts tremendously.

Ciao,
Dscho

[PATCH v2] Add Documentation/CodingStyle

[Johannes Schindelin]

Even if our code is quite a good documentation for our coding style,
some people seem to prefer a document describing it.

The part about the shell scripts is clearly just copied from one of
Junio's helpful mails, and some parts were added from comments by
Junio and Andreas Ericsson.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Wed, 7 Nov 2007, Andreas Ericsson wrote:

	> Perhaps with this addendum?
	> 
	> - Think very, very hard before introducing a new dependency into git. This
	>  means you should stay away from scripting languages not already used in
	>  the git core command set unless your command is clearly separate from it,
	>  such as an importer to convert random-scm-X repositories to git.

	I edited it minimally.

 Documentation/CodingStyle |  103 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 103 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/CodingStyle

diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
new file mode 100644
index 0000000..38a3d9f
--- /dev/null
+++ b/Documentation/CodingStyle
@@ -0,0 +1,103 @@
+As a popular project, we also have some guidelines to keep to the
+code.  For git in general, two rough rules are:
+
+ - Most importantly, we never say "It's in POSIX; we'll happily
+   screw your system that does not conform."  We live in the
+   real world.
+
+ - However, we often say "Let's stay away from that construct,
+   it's not even in POSIX".
+
+ - In spite of the above two rules, we sometimes say "Although
+   this is not in POSIX, it (is so convenient | makes the code
+   much more readable | has other good characteristics) and
+   practically all the platforms we care about support it, so
+   let's use it".  Again, we live in the real world, and it is
+   sometimes a judgement call, decided based more on real world
+   constraints people face than what the paper standard says.
+
+
+As for more concrete guidelines, just imitate the existing code
+(this is a good guideline, no matter which project you are contributing
+to...).  But if you must have some list of rules, here they are.
+
+For shell scripts specifically (not exhaustive):
+
+ - We prefer $( ... ) for command substitution; unlike ``, it
+   properly nests.  It should have been the way Bourne spelled
+   it from day one, but unfortunately isn't.
+
+ - We use ${parameter-word} and its [-=?+] siblings, and their
+   colon'ed "unset or null" form.
+
+ - We use ${parameter#word} and its [#%] siblings, and their
+   doubled "longest matching" form.
+
+ - We use Arithmetic Expansion $(( ... )).
+
+ - No "Substring Expansion" ${parameter:offset:length}.
+
+ - No shell arrays.
+
+ - No strlen ${#parameter}.
+
+ - No regexp ${parameter/pattern/string}.
+
+ - We do not use Process Substitution <(list) or >(list).
+
+ - We prefer "test" over "[ ... ]".
+
+ - We do not write noiseword "function" in front of shell
+   functions.
+
+For C programs:
+
+ - Use tabs to indent, and interpret tabs as taking up to 8 spaces
+
+ - Try to keep to 80 characters per line
+
+ - When declaring pointers, the star sides with the variable name, i.e.
+   "char *string", not "char* string" or "char * string".  This makes
+   it easier to understand "char *string, c;"
+
+ - Do not use curly brackets unnecessarily.  I.e.
+
+	if (bla) {
+		x = 1;
+	}
+
+   is frowned upon.  A gray area is when the statement extends over a
+   few lines, and/or you have a lengthy comment atop of it.
+
+ - Try to make your code understandable.  You may put comments in, but
+   comments invariably tend to stale out when the code they were
+   describing changes.  Often splitting a function into two makes the
+   intention of the code much clearer.
+
+   Double negation is often harder to understand than no negation at
+   all.
+
+   Some clever tricks, like using the !! operator with arithmetic
+   constructs, can be extremely confusing to others.  Avoid them,
+   unless there is a compelling reason to use them.
+
+ - Use the API.  No, really.  We have a strbuf (variable length string),
+   several arrays with the ALLOC_GROW() macro, a path_list for sorted
+   string lists, a hash map (mapping struct objects) named
+   "struct decorate", amongst other things.
+
+ - When you come up with an API, document it.
+
+ - #include system headers in git-compat-util.h.  Some headers on some
+   systems show subtle breakages when you change the order, so it is
+   best to keep them in one place.
+
+ - if you are planning a new command, consider writing it in shell or
+   perl first, so that changes in semantics can be easily changed and
+   discussed.  Many git commands started out like that, and a few are
+   still scripts.
+
+ - Avoid introducing a new dependency into git. This means you usually
+   should stay away from scripting languages not already used in the git
+   core command set (unless your command is clearly separate from it,
+   such as an importer to convert random-scm-X repositories to git).
-- 
1.5.3.5.1597.g7191

Re: [PATCH 0/5] some shell portability fixes

[Mike Ralphson]

On Nov 7, 2007 2:47 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > Equally GNU sed is available as a drop-in rpm for AIX. I wonder if it
> > would be worth adding Makefile support for a PATH prefix for the git
> > scripts, so they could prepend (in this case) something like
> > /opt/freeware/bin or /usr/linux/bin ?
> >
> > In our AIX environment many GNU tools are installed but I can't
> > guarantee they come first in the paths of the git users.
> >
> > I'm willing to work up a patch if there's any interest.
>
> Would that be a task for configure?  Because I am not sure if the GNU
> tools are installed in the same place on all AIX boxen...

Well let's say the patch would arrive earlier if it was based on the
shipped Makefile rather than the unholy abomination that is
autoconf... If the GNU tools have been installed via the IBM AIX
Toolbox for Linux Applications[1] then they'll be installed in
/opt/freeware/bin and /usr/linux/bin will be a set of symlinks to
them.

That said, there may be 32/64bit differences and of course anyone
could have rolled their own sed, awk, diff, patch, grep, sort etc in
/usr/local/bin or anywhere else, and I'd guess this might be useful
for Solaris / HPUX users etc.

I was thinking along the lines of the existing $SHELL_PATH, i.e. a
build-time manually-set Makefile/environment variable. I'd also like
to be able to override gitexecdir in the same way without having my
builds marked dirty.

Cheers, Mike

[1] http://www-03.ibm.com/systems/p/os/aix/linux/download.html

Re: [PATCH 0/5] some shell portability fixes

[Johannes Schindelin]

Hi,

On Wed, 7 Nov 2007, Mike Ralphson wrote:

> On Nov 7, 2007 2:47 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> > I am not sure if the GNU tools are installed in the same place on all 
> > AIX boxen...
> 
> Well let's say the patch would arrive earlier if it was based on the
> shipped Makefile rather than the unholy abomination that is
> autoconf... If the GNU tools have been installed via the IBM AIX
> Toolbox for Linux Applications[1] then they'll be installed in
> /opt/freeware/bin and /usr/linux/bin will be a set of symlinks to
> them.

I guess Makefile is the better place, then.  You are the expert on AIX.

Ciao,
Dscho

Re: [PATCH 0/5] some shell portability fixes

[Nguyen Thai Ngoc Duy]

On 11/7/07, Junio C Hamano <gitster@pobox.com> wrote:
> [2/5] Gaah, AIX sed X-<.  I am not opposed to this patch but
>       would want to get Yays from people with non GNU sed.  Is
>       busybox sed good enough to grok our scripts these days?
>       Please ask help and collect Acks at least from folks on
>       Solaris, MacOS, FBSD, and OBSD.

I haven't extensively used all the scripts. There seems to be no
sed-related failure from git testsuite results in my git-box branch.
So I would say for now it's good enough.
-- 
Duy

Re: [PATCH 0/5] some shell portability fixes

[Nguyen Thai Ngoc Duy]

On 11/7/07, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> On 11/7/07, Junio C Hamano <gitster@pobox.com> wrote:
> > [2/5] Gaah, AIX sed X-<.  I am not opposed to this patch but
> >       would want to get Yays from people with non GNU sed.  Is
> >       busybox sed good enough to grok our scripts these days?
> >       Please ask help and collect Acks at least from folks on
> >       Solaris, MacOS, FBSD, and OBSD.
>
> I haven't extensively used all the scripts. There seems to be no
> sed-related failure from git testsuite results in my git-box branch.
> So I would say for now it's good enough.

Argh, should have made it clear, busybox sed is good enough.

-- 
Duy

Re: [PATCH] Add Documentation/CodingStyle

[Jon Loeliger]

On Tue, 2007-11-06 at 17:17, Johannes Schindelin wrote:

> +
> + - Do not use curly brackets unnecessarily.  I.e.
> +
> +	if (bla) {
> +		x = 1;
> +	}

In my opinion, I think this is a bad guideline.

> +   is frowned upon.  A gray area is when the statement extends over a
> +   few lines, and/or you have a lengthy comment atop of it.

Or if it is some macro, or any number of vague problem areas.

Again, in my opinion, one should always take the safer
defensive programming tactic and always use braces.
Having them really never produces errors, while omitting
them is often error prone.

Yes, I know that is not a popular opinion by example,
but I'm still allowed to state it. :-)
Feel free to ignore me as well. :-)

jdl

Re: [PATCH] Add Documentation/CodingStyle

[Johannes Schindelin]

Hi,

On Wed, 7 Nov 2007, Jon Loeliger wrote:

> On Tue, 2007-11-06 at 17:17, Johannes Schindelin wrote:
> 
> > +
> > + - Do not use curly brackets unnecessarily.  I.e.
> > +
> > +	if (bla) {
> > +		x = 1;
> > +	}
> 
> In my opinion, I think this is a bad guideline.

In my opinion, this is a good guideline.

So now what? Let's have another pointless flamewar?

Ciao,
Dscho

Re: [PATCH 0/5] some shell portability fixes

[Junio C Hamano]

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

> On 11/7/07, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
>> On 11/7/07, Junio C Hamano <gitster@pobox.com> wrote:
>> > [2/5] Gaah, AIX sed X-<.  I am not opposed to this patch but
>> >       would want to get Yays from people with non GNU sed.  Is
>> >       busybox sed good enough to grok our scripts these days?
>> >       Please ask help and collect Acks at least from folks on
>> >       Solaris, MacOS, FBSD, and OBSD.
>>
>> I haven't extensively used all the scripts. There seems to be no
>> sed-related failure from git testsuite results in my git-box branch.
>> So I would say for now it's good enough.
>
> Argh, should have made it clear, busybox sed is good enough.

Thanks.  And you can also happy grok Ralf's rewritten construct,
right?

That is, existing

        $ sed -e 's/foo/\n/' file

will be rewritten by the patch [2/5] to

        $ sed -e 's/foo/\
        /' file

Re: [PATCH v2] Add Documentation/CodingStyle

[Robin Rosenberg]

onsdag 07 november 2007 skrev Johannes Schindelin:
> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> new file mode 100644
> index 0000000..38a3d9f
> --- /dev/null
> +++ b/Documentation/CodingStyle
> @@ -0,0 +1,103 @@
> +As a popular project, we also have some guidelines to keep to the
> +code.  For git in general, two rough rules are:
> +
> + - Most importantly, we never say "It's in POSIX; we'll happily
> +   screw your system that does not conform."  We live in the
> +   real world.

Can we use less offensive wording in documentation than we do on the list or IRC?
I'm not hurt by it but it doesn't look serious.

> + - Try to keep to 80 characters per line
less than?

> +
> + - When declaring pointers, the star sides with the variable name, i.e.
> +   "char *string", not "char* string" or "char * string".  This makes
> +   it easier to understand "char *string, c;"

Rationale: The C syntax is defined the way it is. C programmers should
understand it.

I'd amend: Don't mix different types in declarations.

Write:
	char *string;
	char c;

Rather than:

	char *string,c;

> + - Do not use curly brackets unnecessarily.  I.e.
> +
> +	if (bla) {
> +		x = 1;
> +	}
> +
> +   is frowned upon.  A gray area is when the statement extends over a
> +   few lines, and/or you have a lengthy comment atop of it.

Avoid unnecessary curly brackets but use them if unsure. (This is less 
strict than "do not". There are cases where we do want "unnecessary"
brackets.

-- robin

[PATCH v3] Add Documentation/CodingGuidelines

[Johannes Schindelin]

Even if our code is quite a good documentation for our coding style,
some people seem to prefer a document describing it.

The part about the shell scripts is clearly just copied from one of
Junio's helpful mails, and some parts were added from comments by
Junio, Andreas Ericsson and Robin Rosenberg.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	I think I owe this list an apology for starting yet another
	thread which seems to instigate comments by not many
	code contributors.

	My intention was to make things simpler, but it appears
	that the human brain was created such that it needs complexity
	and creates it when it is absent.

	For example, when I wrote "guidelines" I fully expected that
	it was understood that these are no natural laws which you
	cannot break, should the circumstances afford it.

	Given the discussion, I am half convinced that even this
	patch is a bad idea, and am quite willing to just stop sending
	updates to it.

 Documentation/CodingGuidelines |  106 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 106 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/CodingGuidelines

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
new file mode 100644
index 0000000..2d8656f
--- /dev/null
+++ b/Documentation/CodingGuidelines
@@ -0,0 +1,106 @@
+As a popular project, we also have some guidelines to keep to the
+code.  For git in general, two rough rules are:
+
+ - Most importantly, we never say "It's in POSIX; we'll happily
+   ignore your needs should your system that does not conform."
+   We live in the real world.
+
+ - However, we often say "Let's stay away from that construct,
+   it's not even in POSIX".
+
+ - In spite of the above two rules, we sometimes say "Although
+   this is not in POSIX, it (is so convenient | makes the code
+   much more readable | has other good characteristics) and
+   practically all the platforms we care about support it, so
+   let's use it".  Again, we live in the real world, and it is
+   sometimes a judgement call, decided based more on real world
+   constraints people face than what the paper standard says.
+
+
+As for more concrete guidelines, just imitate the existing code
+(this is a good guideline, no matter which project you are contributing
+to...).  But if you must have some list of rules, here they are.
+
+For shell scripts specifically (not exhaustive):
+
+ - We prefer $( ... ) for command substitution; unlike ``, it
+   properly nests.  It should have been the way Bourne spelled
+   it from day one, but unfortunately isn't.
+
+ - We use ${parameter-word} and its [-=?+] siblings, and their
+   colon'ed "unset or null" form.
+
+ - We use ${parameter#word} and its [#%] siblings, and their
+   doubled "longest matching" form.
+
+ - We use Arithmetic Expansion $(( ... )).
+
+ - No "Substring Expansion" ${parameter:offset:length}.
+
+ - No shell arrays.
+
+ - No strlen ${#parameter}.
+
+ - No regexp ${parameter/pattern/string}.
+
+ - We do not use Process Substitution <(list) or >(list).
+
+ - We prefer "test" over "[ ... ]".
+
+ - We do not write noiseword "function" in front of shell
+   functions.
+
+For C programs:
+
+ - Use tabs to indent, and interpret tabs as taking up to 8 spaces
+
+ - Try to keep to at most 80 characters per line
+
+ - When declaring pointers, the star sides with the variable name, i.e.
+   "char *string", not "char* string" or "char * string".  This makes
+   it easier to understand "char *string, c;"
+
+ - Do not use curly brackets unnecessarily.  I.e.
+
+	if (bla) {
+		x = 1;
+	}
+
+   is frowned upon.  A gray area is when the statement extends over a
+   few lines, and/or you have a lengthy comment atop of it.  Also,
+   like in the Linux kernel, if there is a long list of "else if"
+   statements, it can make sense to add curly brackets to single
+   line blocks.
+
+ - Try to make your code understandable.  You may put comments in, but
+   comments invariably tend to stale out when the code they were
+   describing changes.  Often splitting a function into two makes the
+   intention of the code much clearer.
+
+   Double negation is often harder to understand than no negation at
+   all.
+
+   Some clever tricks, like using the !! operator with arithmetic
+   constructs, can be extremely confusing to others.  Avoid them,
+   unless there is a compelling reason to use them.
+
+ - Use the API.  No, really.  We have a strbuf (variable length string),
+   several arrays with the ALLOC_GROW() macro, a path_list for sorted
+   string lists, a hash map (mapping struct objects) named
+   "struct decorate", amongst other things.
+
+ - When you come up with an API, document it.
+
+ - #include system headers in git-compat-util.h.  Some headers on some
+   systems show subtle breakages when you change the order, so it is
+   best to keep them in one place.
+
+ - if you are planning a new command, consider writing it in shell or
+   perl first, so that changes in semantics can be easily changed and
+   discussed.  Many git commands started out like that, and a few are
+   still scripts.
+
+ - Avoid introducing a new dependency into git. This means you usually
+   should stay away from scripting languages not already used in the git
+   core command set (unless your command is clearly separate from it,
+   such as an importer to convert random-scm-X repositories to git).
-- 
1.5.3.5.1597.g7191

Re: [PATCH v3] Add Documentation/CodingGuidelines

[Junio C Hamano]

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

> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> new file mode 100644
> index 0000000..2d8656f
> --- /dev/null
> +++ b/Documentation/CodingGuidelines
> @@ -0,0 +1,106 @@
> +As a popular project, we also have some guidelines to keep to the
> +code.  For git in general, two rough rules are:

I'd rather not to say "As a popular project" here.  That is
something for others to decide, not for us to advertise.

Also we now have three rules here ;-).

> +
> + - Most importantly, we never say "It's in POSIX; we'll happily
> +   ignore your needs should your system that does not conform."

"should your system not conform"?  "if your system does not
conform"?

> +As for more concrete guidelines, just imitate the existing code
> +(this is a good guideline, no matter which project you are contributing
> +to...).  But if you must have some list of rules, here they are.

s/\.\.\.//;

> +For C programs:
> +
> + - Use tabs to indent, and interpret tabs as taking up to 8 spaces
> +
> + - Try to keep to at most 80 characters per line
> +
> + - When declaring pointers, the star sides with the variable name, i.e.
> +   "char *string", not "char* string" or "char * string".  This makes
> +   it easier to understand "char *string, c;"

End each sentence with a full stop "." for consistency.

> +
> + - Do not use curly brackets unnecessarily.  I.e.
> +
> +	if (bla) {
> +		x = 1;
> +	}
> +
> +   is frowned upon.  A gray area is when the statement extends over a
> +   few lines, and/or you have a lengthy comment atop of it.  Also,
> +   like in the Linux kernel, if there is a long list of "else if"
> +   statements, it can make sense to add curly brackets to single
> +   line blocks.

As Robin suggests, s/Do not use/Avoid using/ feels more in line
with the spirit with the "A gray area is..." description.

I think the official name for {} are "braces", by the way.

> +   Double negation is often harder to understand than no negation at
> +   all.
> +
> +   Some clever tricks, like using the !! operator with arithmetic
> +   constructs, can be extremely confusing to others.  Avoid them,
> +   unless there is a compelling reason to use them.

Need a bullet point "-" before "Double" (but not "Some").

> + - #include system headers in git-compat-util.h.  Some headers on some
> +   systems show subtle breakages when you change the order, so it is
> +   best to keep them in one place.
> +

The first #include in C files except platform specific compat/
implementation should be git-compat-util.h or another header
file that includes it, such as cache.h and builtin.h.

> + - if you are planning a new command, consider writing it in shell or
> +   perl first, so that changes in semantics can be easily changed and
> +   discussed.  Many git commands started out like that, and a few are
> +   still scripts.

Begin a statement with a Capital letter, for consistency.

[PATCH v4] Add Documentation/CodingGuidelines

[Johannes Schindelin]

Even if our code is quite a good documentation for our coding style,
some people seem to prefer a document describing it.

The part about the shell scripts is clearly just copied from one of
Junio's helpful mails, and some parts were added from comments by
Junio, Andreas Ericsson and Robin Rosenberg.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/CodingGuidelines |  112 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 112 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/CodingGuidelines

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
new file mode 100644
index 0000000..3b042db
--- /dev/null
+++ b/Documentation/CodingGuidelines
@@ -0,0 +1,112 @@
+Like other projects, we also have some guidelines to keep to the
+code.  For git in general, three rough rules are:
+
+ - Most importantly, we never say "It's in POSIX; we'll happily
+   ignore your needs should your system not conform to it."
+   We live in the real world.
+
+ - However, we often say "Let's stay away from that construct,
+   it's not even in POSIX".
+
+ - In spite of the above two rules, we sometimes say "Although
+   this is not in POSIX, it (is so convenient | makes the code
+   much more readable | has other good characteristics) and
+   practically all the platforms we care about support it, so
+   let's use it".
+
+   Again, we live in the real world, and it is sometimes a
+   judgement call, the decision based more on real world
+   constraints people face than what the paper standard says.
+
+
+As for more concrete guidelines, just imitate the existing code
+(this is a good guideline, no matter which project you are
+contributing to).  But if you must have a list of rules,
+here they are.
+
+For shell scripts specifically (not exhaustive):
+
+ - We prefer $( ... ) for command substitution; unlike ``, it
+   properly nests.  It should have been the way Bourne spelled
+   it from day one, but unfortunately isn't.
+
+ - We use ${parameter-word} and its [-=?+] siblings, and their
+   colon'ed "unset or null" form.
+
+ - We use ${parameter#word} and its [#%] siblings, and their
+   doubled "longest matching" form.
+
+ - We use Arithmetic Expansion $(( ... )).
+
+ - No "Substring Expansion" ${parameter:offset:length}.
+
+ - No shell arrays.
+
+ - No strlen ${#parameter}.
+
+ - No regexp ${parameter/pattern/string}.
+
+ - We do not use Process Substitution <(list) or >(list).
+
+ - We prefer "test" over "[ ... ]".
+
+ - We do not write the noiseword "function" in front of shell
+   functions.
+
+For C programs:
+
+ - We use tabs to indent, and interpret tabs as taking up to
+   8 spaces.
+
+ - We try to keep to at most 80 characters per line.
+
+ - When declaring pointers, the star sides with the variable
+   name, i.e. "char *string", not "char* string" or
+   "char * string".  This makes it easier to understand code
+   like "char *string, c;".
+
+ - We avoid using braces unnecessarily.  I.e.
+
+	if (bla) {
+		x = 1;
+	}
+
+   is frowned upon.  A gray area is when the statement extends
+   over a few lines, and/or you have a lengthy comment atop of
+   it.  Also, like in the Linux kernel, if there is a long list
+   of "else if" statements, it can make sense to add braces to
+   single line blocks.
+
+ - Try to make your code understandable.  You may put comments
+   in, but comments invariably tend to stale out when the code
+   they were describing changes.  Often splitting a function
+   into two makes the intention of the code much clearer.
+
+ - Double negation is often harder to understand than no negation
+   at all.
+
+ - Some clever tricks, like using the !! operator with arithmetic
+   constructs, can be extremely confusing to others.  Avoid them,
+   unless there is a compelling reason to use them.
+
+ - Use the API.  No, really.  We have a strbuf (variable length
+   string), several arrays with the ALLOC_GROW() macro, a
+   path_list for sorted string lists, a hash map (mapping struct
+   objects) named "struct decorate", amongst other things.
+
+ - When you come up with an API, document it.
+
+ - The first #include in C files, except in platform specific
+   compat/ implementations, should be git-compat-util.h or another
+   header file that includes it, such as cache.h or builtin.h.
+
+ - If you are planning a new command, consider writing it in shell
+   or perl first, so that changes in semantics can be easily
+   changed and discussed.  Many git commands started out like
+   that, and a few are still scripts.
+
+ - Avoid introducing a new dependency into git. This means you
+   usually should stay away from scripting languages not already
+   used in the git core command set (unless your command is clearly
+   separate from it, such as an importer to convert random-scm-X
+   repositories to git).
-- 
1.5.3.5.1597.g7191

Re: [PATCH v4] Add Documentation/CodingGuidelines

[Junio C Hamano]

Ok, will queue this for 'maint' ;-).

Re: [PATCH 0/5] some shell portability fixes

[Ralf Wildenhues]

* Junio C Hamano wrote on Wed, Nov 07, 2007 at 09:42:41PM CET:
> "Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:
> >
> > Argh, should have made it clear, busybox sed is good enough.
> 
> Thanks.  And you can also happy grok Ralf's rewritten construct,
> right?
> 
> That is, existing
> 
>         $ sed -e 's/foo/\n/' file
> 
> will be rewritten by the patch [2/5] to
> 
>         $ sed -e 's/foo/\
>         /' file

The original was something like
  sed 's/[|]/\n/g'

Using instead
  tr '|' '\n'

should work for the original construct, and AFAIK only /usr/ucb/tr on
Solaris fails to understand \n correctly.  Would that be better for you?
Or even
  tr '|' '\012'

which fails only on EBCDIC, which I don't think git targets.

I'll resend the patches tonight.

Cheers,
Ralf

Re: [PATCH] Add Documentation/CodingStyle

[Mike Ralphson]

On Nov 6, 2007 11:17 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> Even if our code is quite a good documentation for our coding style,
> some people seem to prefer a document describing it.

Might this file be a place to document exactly which licenses we
accept code under? I.e. is it GPLv2 only or any license compatible
with GPLv2 ?

Currently I see only GPLv2 in core git, but I have a series of patches
which give me a speed-up of two orders of magnitude for some cases of
at least git-status (on my platform, with my repos etc) and so far my
two approaches involve bringing in code which is either public domain
with restrictions (which may be too onerous for us to carry in
mainline) or 3-clause BSD.

Cheers, Mike

Re: [PATCH 0/5] some shell portability fixes

[Miles Bader]

Junio C Hamano <gitster@pobox.com> writes:
>       /bin/sh on Solaris does not count as you can configure
>       SHELL_PATH to point at xpg4 shell or ksh on that platform.

That's likely to be a rather confusing point for many people trying to
install however (solaris is rather common), unless git somehow arranges
for the right thing to happen automatically, or at least spits out a big
noticeable warning message at install time...

-Miles
-- 
Saa, shall we dance?  (from a dance-class advertisement)

Re: [PATCH 0/5] some shell portability fixes

[Nguyen Thai Ngoc Duy]

On Nov 8, 2007 3:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> "Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:
>
> > On 11/7/07, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> >> On 11/7/07, Junio C Hamano <gitster@pobox.com> wrote:
> >> > [2/5] Gaah, AIX sed X-<.  I am not opposed to this patch but
> >> >       would want to get Yays from people with non GNU sed.  Is
> >> >       busybox sed good enough to grok our scripts these days?
> >> >       Please ask help and collect Acks at least from folks on
> >> >       Solaris, MacOS, FBSD, and OBSD.
> >>
> >> I haven't extensively used all the scripts. There seems to be no
> >> sed-related failure from git testsuite results in my git-box branch.
> >> So I would say for now it's good enough.
> >
> > Argh, should have made it clear, busybox sed is good enough.
>
> Thanks.  And you can also happy grok Ralf's rewritten construct,
> right?
>
> That is, existing
>
>         $ sed -e 's/foo/\n/' file
>
> will be rewritten by the patch [2/5] to
>
>         $ sed -e 's/foo/\
>         /' file
>
>

Yes it worked well

-- 
Duy