[FYI] How I compile on SunOS 5.7 with the SUNWspro compiler and ksh

[FYI] How I compile on SunOS 5.7 with the SUNWspro compiler and ksh

[Brandon Casey]

This email describes how I have successfully compiled git on SunOS 5.7 with
the native SUNWspro compiler version 6 update 2 C 5.3 2001/05/15 and
successfully run nearly all of the tests using the Korn shell.

A series of 8 patches will follow this email. The patches work around a
few issues with this c89 compiler, and the ancient build environment.

A build script is provided at the end of this email.

The GIT_SKIP_TESTS environment variable was used to skip tests which still fail.
The tests which still fail do so because of an ancient iconv, with two exceptions:
1) the t5000 tests which fail are due to the installed gnu tar being too old, and
2) the t6030 tests which fail do so because of the recent change which calls
   trap with signal 0 inside of a function. The installed korn shell (mis-?)handles
   this calling sequence.

Otherwise, I've been using this executable somewhat and have not experienced any
problems. Maybe it will be useful to others.

 Makefile                       |   14 +++++++++++++-
 builtin-cat-file.c             |    2 +-
 builtin-reset.c                |    2 +-
 dir.c                          |   11 ++++++-----
 t/annotate-tests.sh            |    2 +-
 t/lib-git-svn.sh               |    3 +++
 t/t1002-read-tree-m-u-2way.sh  |   10 ++++++----
 t/t4118-apply-empty-context.sh |    2 +-
 t/t4200-rerere.sh              |    5 +++--
 t/t9301-fast-export.sh         |    1 -
 t/t9700/test.pl                |   12 ++++++------
 t/test-lib.sh                  |    2 +-
 12 files changed, 42 insertions(+), 24 deletions(-)

-brandon


#!/bin/sh

GIT_SKIP_TESTS='
   t3900.1[0-289] t3900.2[023]
   t3901.*
   t5000.1[0-24-689] t5000.2[01]
   t5100.[56] t5100.1[02]
   t6030.1[2-9] t6030.2[0-9]
   t9301.4
'

GIT_TEST_CMP='cmp -s'

PATH="/usr/xpg4/bin:$PATH"

export PATH GIT_TEST_CMP GIT_SKIP_TESTS

exec gmake \
    CC=/opt/SUNWspro/bin/cc \
    INSTALL=ginstall \
    TAR=gtar \
    CFLAGS='-fast' \
    NO_CURL=1 \
    NO_OPENSSL=1 \
    NO_TCLTK=1 \
    NO_PERL_MAKEMAKER=1 \
    THREADED_DELTA_SEARCH=1 \
    "$@"

[PATCH] Makefile: configuration for SunOS 5.7

[Brandon Casey]

---
 Makefile |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 551bde9..1e4e66f 100644
--- a/Makefile
+++ b/Makefile
@@ -620,11 +620,23 @@ endif
 ifeq ($(uname_S),SunOS)
 	NEEDS_SOCKET = YesPlease
 	NEEDS_NSL = YesPlease
-	SHELL_PATH = /bin/bash
+	SHELL_PATH = /usr/xpg4/bin/sh
 	NO_STRCASESTR = YesPlease
 	NO_MEMMEM = YesPlease
 	NO_HSTRERROR = YesPlease
 	NO_MKDTEMP = YesPlease
+	ifeq ($(uname_R),5.7)
+		NO_IPV6 = YesPlease
+		NO_SOCKADDR_STORAGE = YesPlease
+		NO_UNSETENV = YesPlease
+		NO_SETENV = YesPlease
+		NO_STRLCPY = YesPlease
+		NO_INET_NTOP = YesPlease
+		NO_INET_PTON = YesPlease
+		NO_C99_FORMAT = YesPlease
+		NO_STRTOUMAX = YesPlease
+		OLD_ICONV = UnfortunatelyYes
+	endif
 	ifeq ($(uname_R),5.8)
 		NEEDS_LIBICONV = YesPlease
 		NO_UNSETENV = YesPlease
-- 
1.6.0.11.gecc7e

[PATCH] dir.c: avoid c99 array initialization

[Brandon Casey]

The following syntax:

        char foo[] = {
                [0] = 1,
                [7] = 2,
                [15] = 3
        };

is a c99 construct which some compilers do not support even though they
support other c99 constructs. Use an alternative.
---
 dir.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/dir.c b/dir.c
index 29d1d5b..14d2eea 100644
--- a/dir.c
+++ b/dir.c
@@ -680,13 +680,14 @@ static int cmp_name(const void *p1, const void *p2)
  */
 static int simple_length(const char *match)
 {
-	const char special[256] = {
-		[0] = 1, ['?'] = 1,
-		['\\'] = 1, ['*'] = 1,
-		['['] = 1
-	};
+	char special[256] = { 1, };
 	int len = -1;
 
+	special['?'] = 1;
+	special['\\'] = 1;
+	special['*'] = 1;
+	special['['] = 1;
+
 	for (;;) {
 		unsigned char c = *match++;
 		len++;
-- 
1.6.0.11.gecc7e

[PATCH FYI] reset,cat-file: remove const declaration from array

[Brandon Casey]

Silence compiler complaints about opt array initialized with local
variables which was declared const.
---
 builtin-cat-file.c |    2 +-
 builtin-reset.c    |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin-cat-file.c b/builtin-cat-file.c
index 7441a56..d954c09 100644
--- a/builtin-cat-file.c
+++ b/builtin-cat-file.c
@@ -212,7 +212,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 	int opt = 0, batch = 0;
 	const char *exp_type = NULL, *obj_name = NULL;
 
-	const struct option options[] = {
+	struct option options[] = {
 		OPT_GROUP("<type> can be one of: blob, tree, commit, tag"),
 		OPT_SET_INT('t', NULL, &opt, "show object type", 't'),
 		OPT_SET_INT('s', NULL, &opt, "show object size", 's'),
diff --git a/builtin-reset.c b/builtin-reset.c
index 4d246c3..28b633f 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -176,7 +176,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				*old_orig = NULL, sha1_old_orig[20];
 	struct commit *commit;
 	char *reflog_action, msg[1024];
-	const struct option options[] = {
+	struct option options[] = {
 		OPT_SET_INT(0, "mixed", &reset_type,
 						"reset HEAD and index", MIXED),
 		OPT_SET_INT(0, "soft", &reset_type, "reset only HEAD", SOFT),
-- 
1.6.0.11.gecc7e

[PATCH FYI] test-lib.sh: work around ksh's trap shortcomings

[Brandon Casey]

In ksh, if trap is called within a function with 0 or EXIT as its signal,
then the trap will be executed at the time the function returns. This
causes a problem in the test functions since 'trap - exit' is called
within the test_done function in order to remove the trap which calls
die() on exit. This means trap has to be called from the scripts top-level.
Do so using an alias.

Additionally, there is some strangeness with respect to aliases and
sourced script files; the alias hack doesn't work. So call 'trap - 0'
directly in lib-git-svn.sh before calling the test_done function.
---


This is the same patch I posted earlier with respect to compiling on IRIX
and Junio has suggested a more elegant solution.

-brandon


 t/lib-git-svn.sh |    3 +++
 t/test-lib.sh    |    2 +-
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index a841df2..e2e8cf3 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -3,6 +3,7 @@
 if test -n "$NO_SVN_TESTS"
 then
 	test_expect_success 'skipping git-svn tests, NO_SVN_TESTS defined' :
+	trap - exit
 	test_done
 	exit
 fi
@@ -15,6 +16,7 @@ svn >/dev/null 2>&1
 if test $? -ne 1
 then
     test_expect_success 'skipping git-svn tests, svn not found' :
+    trap - exit
     test_done
     exit
 fi
@@ -39,6 +41,7 @@ then
 		err='Perl SVN libraries not found or unusable, skipping test'
 	fi
 	test_expect_success "$err" :
+	trap - exit
 	test_done
 	exit
 fi
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11c0275..6a3fc93 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -415,7 +415,6 @@ test_create_repo () {
 }
 
 test_done () {
-	trap - exit
 	test_results_dir="$TEST_DIRECTORY/test-results"
 	mkdir -p "$test_results_dir"
 	test_results_path="$test_results_dir/${0%-*}-$$"
@@ -457,6 +456,7 @@ test_done () {
 
 	esac
 }
+alias test_done='trap - exit && test_done'
 
 # Test the binaries we have just built.  The tests are kept in
 # t/ subdirectory and are run in 'trash directory' subdirectory.
-- 
1.6.0.11.gecc7e

[PATCH FYI] t1002-read-tree-m-u-2way.sh: use 'git diff -U0' rather than 'diff -U0'

[Brandon Casey]

Some old platforms have an old diff which doesn't have the -U option.
'git diff' can be used in its place. Adjust the comparison function to
strip git's additional header lines to make this possible.
---


Same as the one posted in the IRIX thread.


 t/t1002-read-tree-m-u-2way.sh |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/t/t1002-read-tree-m-u-2way.sh b/t/t1002-read-tree-m-u-2way.sh
index aa9dd58..5e40cec 100755
--- a/t/t1002-read-tree-m-u-2way.sh
+++ b/t/t1002-read-tree-m-u-2way.sh
@@ -14,6 +14,8 @@ _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
 _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
 compare_change () {
 	sed >current \
+	    -e '1{/^diff --git /d;}' \
+	    -e '2{/^index /d;}' \
 	    -e '/^--- /d; /^+++ /d; /^@@ /d;' \
 	    -e 's/^\(.[0-7][0-7][0-7][0-7][0-7][0-7]\) '"$_x40"' /\1 X /' "$1"
 	test_cmp expected current
@@ -75,7 +77,7 @@ test_expect_success \
      git update-index --add yomin &&
      git read-tree -m -u $treeH $treeM &&
      git ls-files --stage >4.out || return 1
-     diff -U0 M.out 4.out >4diff.out
+     git diff -U0 --no-index M.out 4.out >4diff.out
      compare_change 4diff.out expected &&
      check_cache_at yomin clean &&
      sum bozbar frotz nitfol >actual4.sum &&
@@ -94,7 +96,7 @@ test_expect_success \
      echo yomin yomin >yomin &&
      git read-tree -m -u $treeH $treeM &&
      git ls-files --stage >5.out || return 1
-     diff -U0 M.out 5.out >5diff.out
+     git diff -U0 --no-index M.out 5.out >5diff.out
      compare_change 5diff.out expected &&
      check_cache_at yomin dirty &&
      sum bozbar frotz nitfol >actual5.sum &&
@@ -206,7 +208,7 @@ test_expect_success \
      git update-index --add nitfol &&
      git read-tree -m -u $treeH $treeM &&
      git ls-files --stage >14.out || return 1
-     diff -U0 M.out 14.out >14diff.out
+     git diff -U0 --no-index M.out 14.out >14diff.out
      compare_change 14diff.out expected &&
      sum bozbar frotz >actual14.sum &&
      grep -v nitfol M.sum > expected14.sum &&
@@ -227,7 +229,7 @@ test_expect_success \
      echo nitfol nitfol nitfol >nitfol &&
      git read-tree -m -u $treeH $treeM &&
      git ls-files --stage >15.out || return 1
-     diff -U0 M.out 15.out >15diff.out
+     git diff -U0 --no-index M.out 15.out >15diff.out
      compare_change 15diff.out expected &&
      check_cache_at nitfol dirty &&
      sum bozbar frotz >actual15.sum &&
-- 
1.6.0.11.gecc7e

[PATCH FYI] Work around sed issues

[Brandon Casey]

/usr/xpg4/bin/sed exits non-zero if the sed script is not newline
terminated. /bin/sed does not have this problem, so use it instead
where possible.

But, /bin/sed does not handle tab correctly. For this case in t4200,
rework the test so that the exit status of sed does not affect the
outcome of the test, and use /usr/xpg4/bin/sed (which is in the PATH).

---
 t/annotate-tests.sh            |    2 +-
 t/t4118-apply-empty-context.sh |    2 +-
 t/t4200-rerere.sh              |    5 +++--
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index cacb273..14ee531 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -114,7 +114,7 @@ test_expect_success \
 test_expect_success \
     'some edit' \
     'mv file file.orig &&
-    sed -e "s/^3A/99/" -e "/^1A/d" -e "/^incomplete/d" < file.orig > file &&
+    /bin/sed -e "s/^3A/99/" -e "/^1A/d" -e "/^incomplete/d" < file.orig > file &&
     echo "incomplete" | tr -d "\\012" >>file &&
     GIT_AUTHOR_NAME="D" git commit -a -m "edit"'
 
diff --git a/t/t4118-apply-empty-context.sh b/t/t4118-apply-empty-context.sh
index f92e259..c064f7b 100755
--- a/t/t4118-apply-empty-context.sh
+++ b/t/t4118-apply-empty-context.sh
@@ -23,7 +23,7 @@ test_expect_success setup '
 	cat file2 >file2.orig
 	git add file1 file2 &&
 	sed -e "/^B/d" <file1.orig >file1 &&
-	sed -e "/^[BQ]/d" <file2.orig >file2 &&
+	/bin/sed -e "/^[BQ]/d" <file2.orig >file2 &&
 	echo Q | tr -d "\\012" >>file2 &&
 	cat file1 >file1.mods &&
 	cat file2 >file2.mods &&
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index b68ab11..834f52f 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -189,8 +189,9 @@ test_expect_success 'file2 added differently in two branches' '
 	echo Bello > file2 &&
 	git add file2 &&
 	git commit -m version2 &&
-	test_must_fail git merge fourth &&
-	sha1=$(sed -e "s/	.*//" .git/MERGE_RR) &&
+	test_must_fail git merge fourth || return 1
+	sha1=$(sed -e "s/	.*//" .git/MERGE_RR)
+	test -n "$sha1" &&
 	rr=.git/rr-cache/$sha1 &&
 	echo Cello > file2 &&
 	git add file2 &&
-- 
1.6.0.11.gecc7e

[PATCH FYI] t9301-fast-export.sh: don't unset config variable while we're skipping test 4

[Brandon Casey]

---


Necessary if using my compile script which included t9301.4 in GIT_SKIP_TESTS
environment variable.

-brandon


 t/t9301-fast-export.sh |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/t/t9301-fast-export.sh b/t/t9301-fast-export.sh
index c19b4a2..475aadd 100755
--- a/t/t9301-fast-export.sh
+++ b/t/t9301-fast-export.sh
@@ -190,7 +190,6 @@ export GIT_COMMITTER_NAME='C O Mitter'
 
 test_expect_success 'setup copies' '
 
-	git config --unset i18n.commitencoding &&
 	git checkout -b copy rein &&
 	git mv file file3 &&
 	git commit -m move1 &&
-- 
1.6.0.11.gecc7e

Re: [FYI] How I compile on SunOS 5.7 with the SUNWspro compiler and ksh

[Jeff King]

On Mon, Aug 18, 2008 at 06:39:40PM -0500, Brandon Casey wrote:

> 1) the t5000 tests which fail are due to the installed gnu tar being
> too old, and

Hmm. I thought I had t5000 working on Solaris 5.7 a few months ago.
Unfortunately, the Solaris box I test on is down at the moment, so I
can't take a closer look. What is the problem?

-Peff

[PATCH FYI] t9700/test.pl: backwards compatibility improvements

[Brandon Casey]

Some versions of perl complain when 'STDERR' is used as the third argument
in the 3-argument form of open(). Convert to the 2-argument form which is
described for duping STDERR in my second edition camel book.

The object oriented version of File::Temp is a rather new incarnation it
seems. The File::Temp man page for v5.8.0 says "(NOT YET IMPLEMENTED)" in
the 'Objects' section. These can be converted to use File::Temp::tempfile().

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 t/t9700/test.pl |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index 4d23125..70f9836 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -38,7 +38,7 @@ is($r->get_color("color.test.slot1", "red"), $ansi_green, "get_color");
 # Failure cases for config:
 # Save and restore STDERR; we will probably extract this into a
 # "dies_ok" method and possibly move the STDERR handling to Git.pm.
-open our $tmpstderr, ">&", STDERR or die "cannot save STDERR"; close STDERR;
+open our $tmpstderr, ">&STDERR" or die "cannot save STDERR"; close STDERR;
 eval { $r->config("test.dupstring") };
 ok($@, "config: duplicate entry in scalar context fails");
 eval { $r->config_bool("test.boolother") };
@@ -69,18 +69,18 @@ is($r->ident_person("Name", "email", "123 +0000"), "Name <email>",
 
 # objects and hashes
 ok(our $file1hash = $r->command_oneline('rev-parse', "HEAD:file1"), "(get file hash)");
-our $tmpfile = File::Temp->new;
+our ($tmpfile, $tmpnam) = File::Temp::tempfile();
 is($r->cat_blob($file1hash, $tmpfile), 15, "cat_blob: size");
 our $blobcontents;
 { local $/; seek $tmpfile, 0, 0; $blobcontents = <$tmpfile>; }
 is($blobcontents, "changed file 1\n", "cat_blob: data");
 seek $tmpfile, 0, 0;
-is(Git::hash_object("blob", $tmpfile), $file1hash, "hash_object: roundtrip");
-$tmpfile = File::Temp->new();
+is(Git::hash_object("blob", $tmpnam), $file1hash, "hash_object: roundtrip");
+($tmpfile, $tmpnam) = File::Temp::tempfile();
 print $tmpfile my $test_text = "test blob, to be inserted\n";
-like(our $newhash = $r->hash_and_insert_object($tmpfile), qr/[0-9a-fA-F]{40}/,
+like(our $newhash = $r->hash_and_insert_object($tmpnam), qr/[0-9a-fA-F]{40}/,
      "hash_and_insert_object: returns hash");
-$tmpfile = File::Temp->new;
+$tmpfile = File::Temp::tempfile();
 is($r->cat_blob($newhash, $tmpfile), length $test_text, "cat_blob: roundtrip size");
 { local $/; seek $tmpfile, 0, 0; $blobcontents = <$tmpfile>; }
 is($blobcontents, $test_text, "cat_blob: roundtrip data");
-- 
1.6.0.11.gecc7e

Re: [FYI] How I compile on SunOS 5.7 with the SUNWspro compiler and ksh

[Brandon Casey]

Jeff King wrote:
> On Mon, Aug 18, 2008 at 06:39:40PM -0500, Brandon Casey wrote:
> 
>> 1) the t5000 tests which fail are due to the installed gnu tar being
>> too old, and
> 
> Hmm. I thought I had t5000 working on Solaris 5.7 a few months ago.
> Unfortunately, the Solaris box I test on is down at the moment, so I
> can't take a closer look. What is the problem?

Probably:
$ gtar --version
tar (GNU tar) 1.12


If I stop t5000-tar-tree.sh by inserting an exit before the 10th test
(which is the first one that fails):

    t5000-tar-tree.sh:
    ...
    test_expect_success \
       'git get-tar-commit-id' \
       'git get-tar-commit-id <b.tar >b.commitid &&
        diff .git/$(git symbolic-ref HEAD) b.commitid'

    exit

    test_expect_success \
       'extract tar archive' \
       '(cd b && "$TAR" xf -) <b.tar'
    ...

and then execute the test commands manually, I get:

$ cd t/trash\ directory
$ cd b
$ gtar xf - < ../b.tar
/apps/bin/gtar: Unknown file type 'g' for pax_global_header, extracted as normal file
/apps/bin/gtar: : Could not create directory: No such file or directory
/apps/bin/gtar: Error exit delayed from previous errors
$ find .
.
./a
./a/a
./a/l1
./a/substfile1
./a/long_path_to_a_file
./a/long_path_to_a_file/long_path_to_a_file
./a/long_path_to_a_file/long_path_to_a_file/long_path_to_a_file
./a/long_path_to_a_file/long_path_to_a_file/long_path_to_a_file/long_path_to_a_file
./a/bin
./a/bin/sh
./a/substfile2
./pax_global_header
./file_with_long_path


The native tar returns:
$ tar xf - < ../b.tar
tar: directory checksum error

-brandon

Re: [FYI] How I compile on SunOS 5.7 with the SUNWspro compiler and ksh

[Jeff King]

On Mon, Aug 18, 2008 at 07:47:39PM -0500, Brandon Casey wrote:

> > Hmm. I thought I had t5000 working on Solaris 5.7 a few months ago.
> > Unfortunately, the Solaris box I test on is down at the moment, so I
> > can't take a closer look. What is the problem?
> 
> Probably:
> $ gtar --version
> tar (GNU tar) 1.12

Ah, I found out the difference: my box is actually Solaris 5.8. The gtar
version there is 1.13 (which is also the version that ships with Solaris
5.9).

> $ gtar xf - < ../b.tar
> /apps/bin/gtar: Unknown file type 'g' for pax_global_header, extracted as normal file
> /apps/bin/gtar: : Could not create directory: No such file or directory
> /apps/bin/gtar: Error exit delayed from previous errors

I get the pax header warning, but nothing else. Judging from your find
results:

> ./a/long_path_to_a_file
> ./a/long_path_to_a_file/long_path_to_a_file
> ./a/long_path_to_a_file/long_path_to_a_file/long_path_to_a_file
> ./a/long_path_to_a_file/long_path_to_a_file/long_path_to_a_file/long_path_to_a_file

There should be another directory that is one level deeper, with
"file_with_long_path" in it...

> ./file_with_long_path

which seems to have been extracted in the root instead!

So perhaps you are hitting some length limitation in gtar.

-Peff

Re: [FYI] How I compile on SunOS 5.7 with the SUNWspro compiler and ksh

[Alex Riesen]

Brandon Casey, Tue, Aug 19, 2008 01:39:40 +0200:
> 
> GIT_SKIP_TESTS='
>    t3900.1[0-289] t3900.2[023]
>    t3901.*
>    t5000.1[0-24-689] t5000.2[01]
>    t5100.[56] t5100.1[02]
>    t6030.1[2-9] t6030.2[0-9]
>    t9301.4
> '
> 
> GIT_TEST_CMP='cmp -s'
> 
> PATH="/usr/xpg4/bin:$PATH"
> 
> export PATH GIT_TEST_CMP GIT_SKIP_TESTS
> 
> exec gmake \
>     CC=/opt/SUNWspro/bin/cc \
>     INSTALL=ginstall \
>     TAR=gtar \
>     CFLAGS='-fast' \
>     NO_CURL=1 \
>     NO_OPENSSL=1 \
>     NO_TCLTK=1 \
>     NO_PERL_MAKEMAKER=1 \
>     THREADED_DELTA_SEARCH=1 \
>     "$@"

You can put all of it in config.mak. It is exactly what it is there
for: build customizations.

Re: [FYI] How I compile on SunOS 5.7 with the SUNWspro compiler and ksh

[Brandon Casey]

Alex Riesen wrote:

> You can put all of it in config.mak. It is exactly what it is there
> for: build customizations.

Ah, I didn't know about config.mak. Thanks.

-brandon

Re: [PATCH] dir.c: avoid c99 array initialization

[David Kågedal]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> The following syntax:
>
>         char foo[] = {
>                 [0] = 1,
>                 [7] = 2,
>                 [15] = 3
>         };
>
> is a c99 construct which some compilers do not support even though they
> support other c99 constructs. Use an alternative.

But the alternative is much worse. So how important is it to support
non-C99 compilers?

> ---
>  dir.c |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index 29d1d5b..14d2eea 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -680,13 +680,14 @@ static int cmp_name(const void *p1, const void *p2)
>   */
>  static int simple_length(const char *match)
>  {
> -	const char special[256] = {
> -		[0] = 1, ['?'] = 1,
> -		['\\'] = 1, ['*'] = 1,
> -		['['] = 1
> -	};
> +	char special[256] = { 1, };
>  	int len = -1;
>  
> +	special['?'] = 1;
> +	special['\\'] = 1;
> +	special['*'] = 1;
> +	special['['] = 1;
> +
>  	for (;;) {
>  		unsigned char c = *match++;
>  		len++;

-- 
David Kågedal

Re: [PATCH] dir.c: avoid c99 array initialization

[Andreas Ericsson]

David Kågedal wrote:
> Brandon Casey <casey@nrlssc.navy.mil> writes:
> 
>> The following syntax:
>>
>>         char foo[] = {
>>                 [0] = 1,
>>                 [7] = 2,
>>                 [15] = 3
>>         };
>>
>> is a c99 construct which some compilers do not support even though they
>> support other c99 constructs. Use an alternative.
> 
> But the alternative is much worse. So how important is it to support
> non-C99 compilers?
> 

Fairly important. Lots of people have gone through lots of work to make
sure git works with legacy compilers.

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

Re: [PATCH] dir.c: avoid c99 array initialization

[Brandon Casey]

David Kågedal wrote:
> Brandon Casey <casey@nrlssc.navy.mil> writes:
> 
>> The following syntax:
>>
>>         char foo[] = {
>>                 [0] = 1,
>>                 [7] = 2,
>>                 [15] = 3
>>         };
>>
>> is a c99 construct which some compilers do not support even though they
>> support other c99 constructs. Use an alternative.
> 
> But the alternative is much worse.

_Much_ worse? In what way?

From an execution standpoint, I don't think any more work is performed.
Probably exactly the same amount of work.

From a readability standpoint, I think it is very nearly the same in
this case. The whole function is only 17 lines.

> So how important is it to support non-C99 compilers?

I think it is relative to the amount of effort it takes. If there is
a demonstrated need and a trivial work around, I think it is worth
it to support non-c99 compilers. Demonstrated need is required.

But, saying that, I posted the patch you replied to in a series that
was for informational purposes only (though I could have done a better
job labeling them). There were no comments from anyone who said that
the series solved any problems they were encountering. At some point
I will post an update.

-brandon

Re: [PATCH] dir.c: avoid c99 array initialization

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> David Kågedal wrote:
>> Brandon Casey <casey@nrlssc.navy.mil> writes:
>> 
>>> The following syntax:
>>>
>>>         char foo[] = {
>>>                 [0] = 1,
>>>                 [7] = 2,
>>>                 [15] = 3
>>>         };
>>>
>>> is a c99 construct which some compilers do not support even though they
>>> support other c99 constructs. Use an alternative.
>> 
>> But the alternative is much worse.
>
> _Much_ worse? In what way?
>
> From an execution standpoint, I don't think any more work is performed.
> Probably exactly the same amount of work.
>
> From a readability standpoint, I think it is very nearly the same in
> this case. The whole function is only 17 lines.

I do not think your patch deserves "much worse" comment.

>> So how important is it to support non-C99 compilers?
>
> I think it is relative to the amount of effort it takes. If there is
> a demonstrated need and a trivial work around, I think it is worth
> it to support non-c99 compilers.

I do not mind taking this patch.  While it would not hurt (because the
code is readable with or without the change) to convert the trivial ones
like this patch addresses, it would not help portability if there are more
nontrivial dependance to c99 constructs in other places in the code.  Are
there known ones?

Re: [PATCH] dir.c: avoid c99 array initialization

[Alex Riesen]

2008/8/19 Brandon Casey <casey@nrlssc.navy.mil>:
>  static int simple_length(const char *match)
>  {
> -       const char special[256] = {
> -               [0] = 1, ['?'] = 1,
> -               ['\\'] = 1, ['*'] = 1,
> -               ['['] = 1
> -       };
> +       char special[256] = { 1, };
>        int len = -1;
>
> +       special['?'] = 1;
> +       special['\\'] = 1;
> +       special['*'] = 1;
> +       special['['] = 1;

For just these 5 values it is likely more effective to just use
a conditional statement (less stack requested, less likely
some stupid compiler tries to optimize it wrongly).
And just as readable.

diff --git a/dir.c b/dir.c
index 92452eb..1cf5985 100644
--- a/dir.c
+++ b/dir.c
@@ -680,17 +680,12 @@ static int cmp_name(const void *p1, const void *p2)
  */
 static int simple_length(const char *match)
 {
-	const char special[256] = {
-		[0] = 1, ['?'] = 1,
-		['\\'] = 1, ['*'] = 1,
-		['['] = 1
-	};
 	int len = -1;

 	for (;;) {
 		unsigned char c = *match++;
 		len++;
-		if (special[c])
+		if (!c || '?' == c || '\\' == c || '*' == c || '[' == c)
 			return len;
 	}
 }

Re: [PATCH] dir.c: avoid c99 array initialization

[Shawn O. Pearce]

Alex Riesen <raa.lkml@gmail.com> wrote:
> 
> For just these 5 values it is likely more effective to just use
> a conditional statement (less stack requested, less likely
> some stupid compiler tries to optimize it wrongly).
> And just as readable.
> 
> diff --git a/dir.c b/dir.c
> index 92452eb..1cf5985 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -680,17 +680,12 @@ static int cmp_name(const void *p1, const void *p2)
>   */
>  static int simple_length(const char *match)
>  {
> -	const char special[256] = {
> -		[0] = 1, ['?'] = 1,
> -		['\\'] = 1, ['*'] = 1,
> -		['['] = 1
> -	};
>  	int len = -1;
> 
>  	for (;;) {
>  		unsigned char c = *match++;
>  		len++;
> -		if (special[c])
> +		if (!c || '?' == c || '\\' == c || '*' == c || '[' == c)

I am reminded of a year old thread with my patch to this:

  http://kerneltrap.org/mailarchive/git/2007/4/15/243541

The patch never applied.  I wonder why.  Was it just Dscho's comment?

-- 
Shawn.

Re: [PATCH] dir.c: avoid c99 array initialization

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Alex Riesen <raa.lkml@gmail.com> wrote:
>> 
>> For just these 5 values it is likely more effective to just use
>> a conditional statement (less stack requested, less likely
>> some stupid compiler tries to optimize it wrongly).
>> And just as readable.
>> 
>> diff --git a/dir.c b/dir.c
>> index 92452eb..1cf5985 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -680,17 +680,12 @@ static int cmp_name(const void *p1, const void *p2)
>>   */
>>  static int simple_length(const char *match)
>>  {
>> -	const char special[256] = {
>> -		[0] = 1, ['?'] = 1,
>> -		['\\'] = 1, ['*'] = 1,
>> -		['['] = 1
>> -	};
>>  	int len = -1;
>> 
>>  	for (;;) {
>>  		unsigned char c = *match++;
>>  		len++;
>> -		if (special[c])
>> +		if (!c || '?' == c || '\\' == c || '*' == c || '[' == c)
>
> I am reminded of a year old thread with my patch to this:
>
>   http://kerneltrap.org/mailarchive/git/2007/4/15/243541
>
> The patch never applied.  I wonder why.  Was it just Dscho's comment?

I think it was an unanswered question about stealing one bit from
ctype.c::sane_ctype[] that kept the discussion in limbo.

Re: [PATCH] dir.c: avoid c99 array initialization

[Alex Riesen]

2008/8/28 Shawn O. Pearce <spearce@spearce.org>:
> Alex Riesen <raa.lkml@gmail.com> wrote:
>>
>> For just these 5 values it is likely more effective to just use
>> a conditional statement (less stack requested, less likely
>> some stupid compiler tries to optimize it wrongly).
>> And just as readable.
>>
>> diff --git a/dir.c b/dir.c
>> index 92452eb..1cf5985 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -680,17 +680,12 @@ static int cmp_name(const void *p1, const void *p2)
>>   */
>>  static int simple_length(const char *match)
>>  {
>> -     const char special[256] = {
>> -             [0] = 1, ['?'] = 1,
>> -             ['\\'] = 1, ['*'] = 1,
>> -             ['['] = 1
>> -     };
>>       int len = -1;
>>
>>       for (;;) {
>>               unsigned char c = *match++;
>>               len++;
>> -             if (special[c])
>> +             if (!c || '?' == c || '\\' == c || '*' == c || '[' == c)
>
> I am reminded of a year old thread with my patch to this:
>
>  http://kerneltrap.org/mailarchive/git/2007/4/15/243541
>
> The patch never applied.  I wonder why.  Was it just Dscho's comment?
>

Likely. And that sane_ctype Junio mentioned, which still has 5 bits free

Re: [PATCH] dir.c: avoid c99 array initialization

[Brandon Casey]

Junio C Hamano wrote:

> I do not mind taking this patch.  While it would not hurt (because the
> code is readable with or without the change) to convert the trivial ones
> like this patch addresses, it would not help portability if there are more
> nontrivial dependance to c99 constructs in other places in the code.  Are
> there known ones?

Only one other "c99" issue that I have encountered (while using a c99 compiler
no less) and it is similarly trivial:

------->8-------
From: Brandon Casey <casey@nrlssc.navy.mil>
Date: Wed, 13 Aug 2008 11:09:33 -0700
Subject: [PATCH] unpack-trees.c: work around run-time array initialization flaw on IRIX 6.5

The c99 MIPSpro Compiler version 7.4.4m on IRIX 6.5 does not properly
initialize run-time initialized arrays. An array which is initialized with
fewer elements than the length of the array should have the uninitialized
elements initialized to zero. This compiler does perform proper
initialization when static initialization parameters are used. So, work
around this by using compile time initialization.
---
 unpack-trees.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index cba0aca..de7cb0b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -143,7 +143,8 @@ static inline int call_unpack_fn(struct cache_entry **src, struct unpack_trees_o

 static int unpack_index_entry(struct cache_entry *ce, struct unpack_trees_options *o)
 {
-	struct cache_entry *src[5] = { ce, };
+	struct cache_entry *src[5] = { NULL, };
+	src[0] = ce;

 	o->pos++;
 	if (ce_stage(ce)) {
-- 
1.6.0.1.119.gcb7f
------->8-------


The above patch, along with minor changes to git-compat-util.h and a new entry in
Makefile allow me to compile on IRIX 6.5. (note: the dir.c patch is not necessary
on IRIX 6.5).

The dir.c patch along with two removals of the const modifier from option
structures and a few tweaks in Makefile allow me to compile using the SUNWspro
compiler on Solaris 7. I'm not sure if it is a c99 issue, but I get the following
errors when compiling without the following patches (which follow the errors):

"builtin-cat-file.c", line 121: warning: non-constant initializer: op "NAME"
"builtin-cat-file.c", line 217: warning: non-constant initializer: op "U&"
"builtin-cat-file.c", line 217: left operand must be modifiable lvalue: op "="
"builtin-cat-file.c", line 218: left operand must be modifiable lvalue: op "="
"builtin-cat-file.c", line 219: left operand must be modifiable lvalue: op "="
"builtin-cat-file.c", line 221: left operand must be modifiable lvalue: op "="
"builtin-cat-file.c", line 222: left operand must be modifiable lvalue: op "="
"builtin-cat-file.c", line 224: left operand must be modifiable lvalue: op "="
cc: acomp failed for builtin-cat-file.c

and similar errors for builtin-reset.c.

----->8-----
diff --git a/builtin-cat-file.c b/builtin-cat-file.c
index 7441a56..d954c09 100644
--- a/builtin-cat-file.c
+++ b/builtin-cat-file.c
@@ -212,7 +212,7 @@ int cmd_cat_file(int argc, const char **argv, const char *pr
 	int opt = 0, batch = 0;
 	const char *exp_type = NULL, *obj_name = NULL;
 
-	const struct option options[] = {
+	struct option options[] = {
 		OPT_GROUP("<type> can be one of: blob, tree, commit, tag"),
 		OPT_SET_INT('t', NULL, &opt, "show object type", 't'),
 		OPT_SET_INT('s', NULL, &opt, "show object size", 's'),
diff --git a/builtin-reset.c b/builtin-reset.c
index 4d246c3..28b633f 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -176,7 +176,7 @@ int cmd_reset(int argc, const char **argv, const char *prefi
				*old_orig = NULL, sha1_old_orig[20];
 	struct commit *commit;
 	char *reflog_action, msg[1024];
-	const struct option options[] = {
+	struct option options[] = {
 		OPT_SET_INT(0, "mixed", &reset_type,
 						"reset HEAD and index", MIXED),
 		OPT_SET_INT(0, "soft", &reset_type, "reset only HEAD", SOFT),
 
----->8-----

Running the tests with ksh on these two platforms requires a work around for ksh's
unique handling of trap within a function (which I do not yet have a pretty solution
for).

-brandon

Re: [PATCH] dir.c: avoid c99 array initialization

[Alex Riesen]

2008/8/28 Brandon Casey <casey@nrlssc.navy.mil>:
> diff --git a/builtin-cat-file.c b/builtin-cat-file.c
> index 7441a56..d954c09 100644
> --- a/builtin-cat-file.c
> +++ b/builtin-cat-file.c
> @@ -212,7 +212,7 @@ int cmd_cat_file(int argc, const char **argv, const char *pr
>        int opt = 0, batch = 0;
>        const char *exp_type = NULL, *obj_name = NULL;
>
> -       const struct option options[] = {
> +       struct option options[] = {
>                OPT_GROUP("<type> can be one of: blob, tree, commit, tag"),

#ifdef HAVE_CONST
#define gitconst const
#else
#define gitconst
#endif

-	const struct option options[] = {
+	gitconst struct option options[] = {

?

Re: [PATCH] dir.c: avoid c99 array initialization

[Brandon Casey]

Alex Riesen wrote:
> 2008/8/28 Brandon Casey <casey@nrlssc.navy.mil>:
>> diff --git a/builtin-cat-file.c b/builtin-cat-file.c
>> index 7441a56..d954c09 100644
>> --- a/builtin-cat-file.c
>> +++ b/builtin-cat-file.c
>> @@ -212,7 +212,7 @@ int cmd_cat_file(int argc, const char **argv, const char *pr
>>        int opt = 0, batch = 0;
>>        const char *exp_type = NULL, *obj_name = NULL;
>>
>> -       const struct option options[] = {
>> +       struct option options[] = {
>>                OPT_GROUP("<type> can be one of: blob, tree, commit, tag"),
> 
> #ifdef HAVE_CONST
> #define gitconst const
> #else
> #define gitconst
> #endif
> 
> -	const struct option options[] = {
> +	gitconst struct option options[] = {
> 
> ?

The problem is not that the const modifier is unrecognized. You can see in
the above patch that the const modifier was not removed from exp_type or
obj_name.

I think the warning on line 217 is the most important where it complains
about a non-constant initializer, followed by complaints that the left
operand must be a modifiable lvalue.

I notice now that some functions have the 'struct option' and all of its
parameters declared static. I think this would satisfy the compiler, but
then the options would not be reinitialized if the function was called
again. Or are cmd_*() functions defined to only ever be called once?

-brandon

[PATCH v2] dir.c: Avoid c99 array initialization

[Brandon Casey]

The following syntax:

        char foo[] = {
                [0] = 1,
                [7] = 2,
                [15] = 3
        };

is a c99 construct which some compilers do not support even though they
support other c99 constructs. This construct can be avoided by folding
these 'special' test cases into the sane_ctype array and making use of
the related infrastructure.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---


Junio C Hamano wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:

>> > I am reminded of a year old thread with my patch to this:
>> >
>> >   http://kerneltrap.org/mailarchive/git/2007/4/15/243541
>> >
>> > The patch never applied.  I wonder why.  Was it just Dscho's comment?
> 
> I think it was an unanswered question about stealing one bit from
> ctype.c::sane_ctype[] that kept the discussion in limbo.

That seems easy enough. Anybody got a 5 character abbreviation for special?

-brandon


 ctype.c           |   10 ++++++----
 dir.c             |    7 +------
 git-compat-util.h |   16 +++++++++-------
 3 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/ctype.c b/ctype.c
index d2bd38e..acb5f0f 100644
--- a/ctype.c
+++ b/ctype.c
@@ -9,18 +9,20 @@
 #undef SS
 #undef AA
 #undef DD
+#undef GS
 
 #define SS GIT_SPACE
 #define AA GIT_ALPHA
 #define DD GIT_DIGIT
+#define GS GIT_SPECIAL  /* \0, *, ?, [, \\ */
 
 unsigned char sane_ctype[256] = {
-	 0,  0,  0,  0,  0,  0,  0,  0,  0, SS, SS,  0,  0, SS,  0,  0,		/* 0-15 */
+	GS,  0,  0,  0,  0,  0,  0,  0,  0, SS, SS,  0,  0, SS,  0,  0,		/* 0-15 */
 	 0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,		/* 16-15 */
-	SS,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,		/* 32-15 */
-	DD, DD, DD, DD, DD, DD, DD, DD, DD, DD,  0,  0,  0,  0,  0,  0,		/* 48-15 */
+	SS,  0,  0,  0,  0,  0,  0,  0,  0,  0, GS,  0,  0,  0,  0,  0,		/* 32-15 */
+	DD, DD, DD, DD, DD, DD, DD, DD, DD, DD,  0,  0,  0,  0,  0, GS,		/* 48-15 */
 	 0, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA,		/* 64-15 */
-	AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA,  0,  0,  0,  0,  0,		/* 80-15 */
+	AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, GS, GS,  0,  0,  0,		/* 80-15 */
 	 0, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA,		/* 96-15 */
 	AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA,  0,  0,  0,  0,  0,		/* 112-15 */
 	/* Nothing in the 128.. range */
diff --git a/dir.c b/dir.c
index 92452eb..e6a2556 100644
--- a/dir.c
+++ b/dir.c
@@ -680,17 +680,12 @@ static int cmp_name(const void *p1, const void *p2)
  */
 static int simple_length(const char *match)
 {
-	const char special[256] = {
-		[0] = 1, ['?'] = 1,
-		['\\'] = 1, ['*'] = 1,
-		['['] = 1
-	};
 	int len = -1;
 
 	for (;;) {
 		unsigned char c = *match++;
 		len++;
-		if (special[c])
+		if (isspecial(c))
 			return len;
 	}
 }
diff --git a/git-compat-util.h b/git-compat-util.h
index 6ee3255..ac22409 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -326,14 +326,16 @@ static inline int has_extension(const char *filename, const char *ext)
 #undef tolower
 #undef toupper
 extern unsigned char sane_ctype[256];
-#define GIT_SPACE 0x01
-#define GIT_DIGIT 0x02
-#define GIT_ALPHA 0x04
+#define GIT_SPACE   0x01
+#define GIT_DIGIT   0x02
+#define GIT_ALPHA   0x04
+#define GIT_SPECIAL 0x08
 #define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)
-#define isspace(x) sane_istest(x,GIT_SPACE)
-#define isdigit(x) sane_istest(x,GIT_DIGIT)
-#define isalpha(x) sane_istest(x,GIT_ALPHA)
-#define isalnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)
+#define isspace(x)   sane_istest(x,GIT_SPACE)
+#define isdigit(x)   sane_istest(x,GIT_DIGIT)
+#define isalpha(x)   sane_istest(x,GIT_ALPHA)
+#define isalnum(x)   sane_istest(x,GIT_ALPHA | GIT_DIGIT)
+#define isspecial(x) sane_istest(x,GIT_SPECIAL)
 #define tolower(x) sane_case((unsigned char)(x), 0x20)
 #define toupper(x) sane_case((unsigned char)(x), 0)
 
-- 
1.6.0.21.g35a2e

Re: [PATCH v2] dir.c: Avoid c99 array initialization

[Alex Riesen]

2008/8/29 Brandon Casey <casey@nrlssc.navy.mil>:
>>
>> I think it was an unanswered question about stealing one bit from
>> ctype.c::sane_ctype[] that kept the discussion in limbo.
>
> That seems easy enough. Anybody got a 5 character abbreviation for special?
>

SPECL :)

Re: [PATCH v2] dir.c: Avoid c99 array initialization

[Brandon Casey]

Brandon Casey wrote:
> The following syntax:
> 
>         char foo[] = {
>                 [0] = 1,
>                 [7] = 2,
>                 [15] = 3
>         };
> 
> is a c99 construct which some compilers do not support even though they
> support other c99 constructs. This construct can be avoided by folding
> these 'special' test cases into the sane_ctype array and making use of
> the related infrastructure.

You probably would want to squash this in:

diff --git a/dir.c b/dir.c
index e6a2556..acf1001 100644
--- a/dir.c
+++ b/dir.c
@@ -52,11 +52,6 @@ int common_prefix(const char **pathspec)
	return prefix;
 }
 
-static inline int special_char(unsigned char c1)
-{
-	return !c1 || c1 == '*' || c1 == '[' || c1 == '?' || c1 == '\\';
-}
-
 /*
  * Does 'match' matches the given name?
  * A match is found if
@@ -80,7 +75,7 @@ static int match_one(const char *match, const char *name, int 
 	for (;;) {
 		unsigned char c1 = *match;
 		unsigned char c2 = *name;
-		if (special_char(c1))
+		if (isspecial(c1))
 			break;
 		if (c1 != c2)
 			return 0;

Re: [PATCH] dir.c: avoid c99 array initialization

[Johannes Schindelin]

Hi,

On Thu, 28 Aug 2008, Shawn O. Pearce wrote:

> Alex Riesen <raa.lkml@gmail.com> wrote:
> > 
> > For just these 5 values it is likely more effective to just use
> > a conditional statement (less stack requested, less likely
> > some stupid compiler tries to optimize it wrongly).
> > And just as readable.
> > 
> > diff --git a/dir.c b/dir.c
> > index 92452eb..1cf5985 100644
> > --- a/dir.c
> > +++ b/dir.c
> > @@ -680,17 +680,12 @@ static int cmp_name(const void *p1, const void *p2)
> >   */
> >  static int simple_length(const char *match)
> >  {
> > -	const char special[256] = {
> > -		[0] = 1, ['?'] = 1,
> > -		['\\'] = 1, ['*'] = 1,
> > -		['['] = 1
> > -	};
> >  	int len = -1;
> > 
> >  	for (;;) {
> >  		unsigned char c = *match++;
> >  		len++;
> > -		if (special[c])
> > +		if (!c || '?' == c || '\\' == c || '*' == c || '[' == c)
> 
> I am reminded of a year old thread with my patch to this:
> 
>   http://kerneltrap.org/mailarchive/git/2007/4/15/243541
> 
> The patch never applied.  I wonder why.  Was it just Dscho's comment?

If it was, I am very sorry.

But I still think that a lookup for something that is called potentially a 
million times per second is better than a switch statement (except when 
there are less than, say, 4 cases).

Ciao,
Dscho

Re: [PATCH FYI] t9700/test.pl: backwards compatibility improvements

[Tom G. Christensen]

Brandon Casey wrote:
> Some versions of perl complain when 'STDERR' is used as the third argument
> in the 3-argument form of open(). Convert to the 2-argument form which is
> described for duping STDERR in my second edition camel book.
> 
> The object oriented version of File::Temp is a rather new incarnation it
> seems. The File::Temp man page for v5.8.0 says "(NOT YET IMPLEMENTED)" in
> the 'Objects' section. These can be converted to use File::Temp::tempfile().
> 
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
 >
Tested-by: Tom G. Christensen <tgc@statsbiblioteket.dk>

Without this patch the testsuite will fail with perl 5.8.0:
t9700$ ./test.pl
ok 1 - use Git;
Bareword "STDERR" not allowed while "strict subs" in use at ./test.pl 
line 41.
Execution of ./test.pl aborted due to compilation errors.
1..1
# Looks like your test died just after 1.


Could we please get this patch in?

-tgc

Re: [PATCH FYI] t9700/test.pl: backwards compatibility improvements

[Brandon Casey]

Tom G. Christensen wrote:
> Brandon Casey wrote:
>> Some versions of perl complain when 'STDERR' is used as the third
>> argument
>> in the 3-argument form of open(). Convert to the 2-argument form which is
>> described for duping STDERR in my second edition camel book.
>>
>> The object oriented version of File::Temp is a rather new incarnation it
>> seems. The File::Temp man page for v5.8.0 says "(NOT YET IMPLEMENTED)" in
>> the 'Objects' section. These can be converted to use
>> File::Temp::tempfile().
>>
>> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
>>
> Tested-by: Tom G. Christensen <tgc@statsbiblioteket.dk>
> 
> Without this patch the testsuite will fail with perl 5.8.0:
> t9700$ ./test.pl
> ok 1 - use Git;
> Bareword "STDERR" not allowed while "strict subs" in use at ./test.pl
> line 41.
> Execution of ./test.pl aborted due to compilation errors.
> 1..1
> # Looks like your test died just after 1.
> 
> 
> Could we please get this patch in?

Junio had asked me to follow up on that patch at a later time, so
thanks for prodding.

Here is an alternative which _removes_ the File::Temp requirement.

-brandon

[PATCH 1/3] t9700/test.pl: no longer requires File::Basename

[Brandon Casey]

Since ff30fff3 removed the call to basename(), we don't need to
'use File::Basename'.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 t/t9700/test.pl |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index 851cea4..732133e 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -8,7 +8,6 @@ use strict;
 use Test::More qw(no_plan);
 
 use Cwd;
-use File::Basename;
 use File::Temp;
 
 BEGIN { use_ok('Git') }
-- 
1.6.0.1.244.gdc19

[PATCH 2/3] t9700/test.pl: avoid bareword 'STDERR' in 3-argument open()

[Brandon Casey]

Some versions of perl complain when 'STDERR' is used as the third argument
in the 3-argument form of open(). Convert to the 2-argument form which is
described for duping STDERR in my second edition camel book.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 t/t9700/test.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index 732133e..f777b08 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -34,7 +34,7 @@ is($r->get_color("color.test.slot1", "red"), $ansi_green, "get_color");
 # Failure cases for config:
 # Save and restore STDERR; we will probably extract this into a
 # "dies_ok" method and possibly move the STDERR handling to Git.pm.
-open our $tmpstderr, ">&", STDERR or die "cannot save STDERR"; close STDERR;
+open our $tmpstderr, ">&STDERR" or die "cannot save STDERR"; close STDERR;
 eval { $r->config("test.dupstring") };
 ok($@, "config: duplicate entry in scalar context fails");
 eval { $r->config_bool("test.boolother") };
-- 
1.6.0.1.244.gdc19

[PATCH 3/3] t9700/test.pl: remove File::Temp requirement

[Brandon Casey]

The object oriented version of File::Temp is a rather new incarnation it
seems. The File::Temp man page for v5.8.0 says "(NOT YET IMPLEMENTED)" in
the 'Objects' section. Instead of creating a file with a unique name in
the system TMPDIR, we can create our own temporary file with a static
name and use that instead.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 t/t9700/test.pl |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index f777b08..9908109 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -8,7 +8,6 @@ use strict;
 use Test::More qw(no_plan);
 
 use Cwd;
-use File::Temp;
 
 BEGIN { use_ok('Git') }
 
@@ -65,21 +64,25 @@ is($r->ident_person("Name", "email", "123 +0000"), "Name <email>",
 
 # objects and hashes
 ok(our $file1hash = $r->command_oneline('rev-parse', "HEAD:file1"), "(get file hash)");
-our $tmpfile = File::Temp->new;
-is($r->cat_blob($file1hash, $tmpfile), 15, "cat_blob: size");
+my $tmpfile = "file.tmp";
+open TEMPFILE, "+>$tmpfile" or die "Can't open $tmpfile: $!";
+is($r->cat_blob($file1hash, \*TEMPFILE), 15, "cat_blob: size");
 our $blobcontents;
-{ local $/; seek $tmpfile, 0, 0; $blobcontents = <$tmpfile>; }
+{ local $/; seek TEMPFILE, 0, 0; $blobcontents = <TEMPFILE>; }
 is($blobcontents, "changed file 1\n", "cat_blob: data");
-seek $tmpfile, 0, 0;
+close TEMPFILE or die "Failed writing to $tmpfile: $!";
 is(Git::hash_object("blob", $tmpfile), $file1hash, "hash_object: roundtrip");
-$tmpfile = File::Temp->new();
-print $tmpfile my $test_text = "test blob, to be inserted\n";
+open TEMPFILE, ">$tmpfile" or die "Can't open $tmpfile: $!";
+print TEMPFILE my $test_text = "test blob, to be inserted\n";
+close TEMPFILE or die "Failed writing to $tmpfile: $!";
 like(our $newhash = $r->hash_and_insert_object($tmpfile), qr/[0-9a-fA-F]{40}/,
      "hash_and_insert_object: returns hash");
-$tmpfile = File::Temp->new;
-is($r->cat_blob($newhash, $tmpfile), length $test_text, "cat_blob: roundtrip size");
-{ local $/; seek $tmpfile, 0, 0; $blobcontents = <$tmpfile>; }
+open TEMPFILE, "+>$tmpfile" or die "Can't open $tmpfile: $!";
+is($r->cat_blob($newhash, \*TEMPFILE), length $test_text, "cat_blob: roundtrip size");
+{ local $/; seek TEMPFILE, 0, 0; $blobcontents = <TEMPFILE>; }
 is($blobcontents, $test_text, "cat_blob: roundtrip data");
+close TEMPFILE;
+unlink $tmpfile;
 
 # paths
 is($r->repo_path, "./.git", "repo_path");
-- 
1.6.0.1.244.gdc19

Re: [PATCH FYI] t9700/test.pl: backwards compatibility improvements

[Brandon Casey]

Brandon Casey wrote:
> Tom G. Christensen wrote:

>> Tested-by: Tom G. Christensen <tgc@statsbiblioteket.dk>

> Here is an alternative which _removes_ the File::Temp requirement.

A Tested-by for this new series would be appreciated.

-brandon

Re: [PATCH 3/3] t9700/test.pl: remove File::Temp requirement

[Tom G. Christensen]

Brandon Casey wrote:
> The object oriented version of File::Temp is a rather new incarnation it
> seems. The File::Temp man page for v5.8.0 says "(NOT YET IMPLEMENTED)" in
> the 'Objects' section. Instead of creating a file with a unique name in
> the system TMPDIR, we can create our own temporary file with a static
> name and use that instead.
> 
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
 >
Tested-by: Tom G. Christensen <tgc@statsbiblioteket.dk>

I applied all three patches and ran the testsuite with perl 5.8.0 
(RHEL3). It now passes t9700 and runs to completion as expected.

-tgc