I have now run every test script <= t9001 using the current 'master' on
the SunOS 5.8 box available to me. It _mostly_ passed. Following are
three patches:
1/3: git_config_*: don't assume we are parsing a config file
This is an actual minor bug which was covered by most platforms'
ability to print (null) for NULL. I posted this earlier, but with
a typo.
2/3: t3404: use configured shell instead of /bin/sh
This is a problem purely in the test script, not git itself.
3/3: diff: fix java funcname pattern for solaris
This is a Solaris-specific bug in git.
With these fixes, everything passes for me with a few exceptions:
- Sun's diff doesn't understand "-u". I was able to use GNU diff.
Since comparing actual and expected output is so common, we could
potentially abstract this with a "test_cmp()" function and use
something platform specific. It's probably not worth the trouble, as
it impacts only the test suite, and only on systems with a totally
broken diff.
- t4020 fails without gnu grep, as it requires "grep -a"
- t4024 fails unless /usr/xpg4/bin is in your path, as the regular
version of 'expr' is unhappy with us (I didn't investigate too
closely)
- t4118, t4200, t8001, and t8002 fail unless you have GNU sed. The
xpg4 sed barfs if there's no newline at the end of input (and this
is specifically part of these tests)
- I couldn't personally run t5502, t5505, t5512, t5520, or t9001, all
of which require the Git.pm module; the version of perl on this
machine is old (5.005_02) and complained about the 'our' keyword
- t7505 uses a hook which hard-codes /bin/sh but uses syntax that
the Solaris /bin/sh doesn't like. Maybe hooks should get their
$SHELL value replaced?
- t3900 mostly passes, but fails to correctly convert between EUCJP
and ISO-2022-JP (in either direction). I didn't investigate very
far.
- git-filter-branch invokes "sh" for the ...It is unfair to call diff without -u "totally broken". It is You've already did that in your [PATCH 2/3], haven't you? So why not? -
Fair enough (and you are right that it is not even POSIX). Is it something we want to work around? We "diff -u" quite a bit in the test Because I didn't look very closely, and I assumed it was one of the regular hooks. But yes, it is defined in the script and could get the same treatment. -Peff -
The Solaris regex library doesn't like having the '$' anchor
inside capture parentheses. It rejects the match, causing
t4018 to fail.
Signed-off-by: Jeff King <peff@peff.net>
---
diff.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/diff.c b/diff.c
index 58fe775..0f18ee9 100644
--- a/diff.c
+++ b/diff.c
@@ -1199,7 +1199,7 @@ static struct builtin_funcname_pattern {
"new\\|return\\|switch\\|throw\\|while\\)\n"
"^[ ]*\\(\\([ ]*"
"[A-Za-z_][A-Za-z_0-9]*\\)\\{2,\\}"
- "[ ]*([^;]*$\\)" },
+ "[ ]*([^;]*\\)$" },
{ "tex", "^\\(\\\\\\(sub\\)*section{.*\\)$" },
};
--
1.5.4.2.247.g107bd
-The fake-editor shell script invoked /bin/sh; normally this is fine, unless the /bin/sh doesn't meet our compatibility requirements, as is the case with Solaris. Specifically, the $() syntax used by fake-editor is not understood. Signed-off-by: Jeff King <peff@peff.net> --- t/t3404-rebase-interactive.sh | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index e5ed745..62e65d7 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -61,8 +61,8 @@ test_expect_success 'setup' ' git tag I ' -cat > fake-editor.sh <<\EOF -#!/bin/sh +echo "#!$SHELL" >fake-editor +cat >> fake-editor.sh <<\EOF case "$1" in */COMMIT_EDITMSG) test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1" -- 1.5.4.2.247.g107bd -
-- Hannes -
Gah, yes. Not sure how I was even able to run the test with that error. Junio, your $SHELL_PATH fix looks sensible, too. -Peff -
Simple. fake-editor.sh does not begin with #! anymore and exec causes /bin/sh to run, and your system happens to have a sane shell there. If you used $SHELL not $SHELL_PATH and wrote to fake-editor.sh, you would have noticed that using $SHELL there was wrong as it would have said "#!bash" for you ;-) -
I thought I tested it on the Solaris box with the broken shell, but Actually, my $SHELL is "/usr/local/bin/bash" on the box in question. -Peff -
Hmm. Actually, we just set SHELL_PATH to SHELL in the Makefile. So I really don't see the point of SHELL_PATH at all, unless there are some setups where it is already set outside of git. -Peff -
Well, on one of my machines, I seem to be getting SHELL=bash (not /bin/bash). Honestly, I do not think it matters getting the value from Makefile for this particular case, but I'd rather get people into habits to run "make tXXXX-name", not "sh ./tXXXX-name.sh", unless they know what they are doing. -
My point is that nothing ever turns that into /bin/bash, unless there is something totally outside of git setting up SHELL_PATH (which is not set up on any of my systems). I would think to handle the case you're talking about, you would want to change SHELL_PATH ?= $(SHELL) to something like Why? -Peff -
SHELL can be just "bash" or "dash" or whatever. Makefile sets and exports SHELL_PATH so use it when telling which shell to use to fake-editor.sh t/t3404-rebase-interactive.sh | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 62e65d7..049aa37 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -61,7 +61,7 @@ test_expect_success 'setup' ' git tag I ' -echo "#!$SHELL" >fake-editor +echo "#!$SHELL_PATH" >fake-editor.sh cat >> fake-editor.sh <<\EOF case "$1" in */COMMIT_EDITMSG) -
Hm, when we run the test outside make, like $ sh t3404-rebase-interactive.sh then the fake-editor.sh script begins with this line: #! Isn't this a trap similar to what Jeff wanted to avoid? -- Hannes -
Well, the trap I wanted to avoid is that "/bin/sh" specifically is broken. But yes, I think losing the ability to run the tests from the commandline is bad. Ideally we figured out a sane shell for our shell scripts in the main build process, and we want to always use that here. I think "git rev-parse --sane-shell-path" is probably overkill. Maybe test-lib.sh can source a file of build-time options? The "diff -u" vs "cmp" thing could go there, as well. -Peff -
We never had it. "make t3404-rebase-interactive" is the way to do so. -
Well, that is a surprise to me, since I have been using it for almost two years with no problems. And I know I am not alone because others have posted snippets on the list invoking a test via "sh -x", including both Dscho and you. Is there a good reason not to do so? -Peff -
I think our messages are crossing, but (1) currently we do not export much from t/Makefile and the only people who can get affected are on platforms that do need custom configuration, and the difference being subtle and rare makes it more surprising and harder to diagnose when the difference does matter, (2) I'd like to place some stuff in t/Makefile in such a way that no tests that runs a server that listens to a network port is not run by default, among other things, which means the difference between "sh tXXXX-name.sh" and "make tXXXX-name" will get bigger not smaller. -
OK, thanks for explaining (point 2 was what I was looking for). Personally, I think we are better off putting such configuration into a file that gets sourced by test-lib.sh, but I don't overly care. I _do_ find "sh -x tXXXX-name.sh" useful from time to time, but mainly only while debugging the test scripts themselves. -Peff -
I do too. I think the basic goal should be to come up with a way to make sure that "cd t/ && make" and "cd t/ && sh -x tXXXX-name.sh" would get an environment as close as possible to what happens when you run "make test" at the toplevel. -
I was thinking of something like this:
---
diff --git a/Makefile b/Makefile
index d33a556..9046b43 100644
--- a/Makefile
+++ b/Makefile
@@ -808,7 +808,7 @@ export TAR INSTALL DESTDIR SHELL_PATH
### Build rules
-all:: $(ALL_PROGRAMS) $(BUILT_INS) $(OTHER_PROGRAMS)
+all:: $(ALL_PROGRAMS) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
ifneq (,$X)
$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), $(RM) '$p';)
endif
@@ -1010,6 +1010,9 @@ GIT-CFLAGS: .FORCE-GIT-CFLAGS
echo "$$FLAGS" >GIT-CFLAGS; \
fi
+GIT-BUILD-OPTIONS: .FORCE-GIT-BUILD-OPTIONS
+ @echo SHELL_PATH='$(SHELL_PATH_SQ)' >$@
+
### Detect Tck/Tk interpreter path changes
ifndef NO_TCLTK
TRACK_VARS = $(subst ','\'',-DTCLTK_PATH='$(TCLTK_PATH_SQ)')
@@ -1169,6 +1172,7 @@ endif
.PHONY: all install clean strip
.PHONY: .FORCE-GIT-VERSION-FILE TAGS tags cscope .FORCE-GIT-CFLAGS
+.PHONY: .FORCE-GIT-BUILD-OPTIONS
### Check documentation
#
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 62e65d7..049aa37 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -61,7 +61,7 @@ test_expect_success 'setup' '
git tag I
'
-echo "#!$SHELL" >fake-editor
+echo "#!$SHELL_PATH" >fake-editor.sh
cat >> fake-editor.sh <<\EOF
case "$1" in
*/COMMIT_EDITMSG)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 83889c4..3c4e21f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -342,6 +342,8 @@ if ! test -x ../test-chmtime; then
exit 1
fi
+. ../GIT-BUILD-OPTIONS
+
# Test repository
test=trash
rm -fr "$test"
-Sounds sane to me. -
OK, here it is cleaned up a little (adding some missing quotation marks
and the proper gitignore) and with a commit message.
Possibly t/Makefile should also be getting this information to actually
run the test scripts. That would require either:
- making the GIT-BUILD-OPTIONS both make and shell parseable, meaning
changing SHELL_PATH='$(SHELL_PATH_SQ)' into SHELL_PATH=$(SHELL_PATH)
and hoping nobody uses metacharacters
- writing two separate files, one for shell and one for make
- working some Makefile magic to set variables from the shell; this is
not possible in ordinary make, but I suspect there are some GNU-isms
we could use
-- >8 --
use build-time SHELL_PATH in test scripts
The top-level Makefile now creates a GIT-BUILD-OPTIONS file
which stores any options selected by the make process that
may be of use to further parts of the build process.
Specifically, we store the SHELL_PATH so that it can be used
by tests to construct shell scripts on the fly.
The format of the GIT-BUILD-OPTIONS file is Bourne shell,
and it is sourced by test-lib.sh; all tests can rely on just
having $SHELL_PATH correctly set in the environment.
The GIT-BUILD-OPTIONS file is written every time the
toplevel 'make' is invoked. Since the only users right now
are the test scripts, there's no drawback to updating its
timestamp. If something build-related depends on this, we
can do a trick similar to the one used by GIT-CFLAGS.
Signed-off-by: Jeff King <peff@peff.net>
---
.gitignore | 1 +
Makefile | 6 +++++-
t/t3404-rebase-interactive.sh | 2 +-
t/test-lib.sh | 2 ++
4 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/.gitignore b/.gitignore
index 165b256..4ff2fec 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,3 +1,4 @@
+GIT-BUILD-OPTIONS
GIT-CFLAGS
GIT-GUI-VARS
GIT-VERSION-FILE
diff --git a/Makefile b/Makefile
index d33a556..ba1aaf8 100644
--- a/Makefile
+++ b/Makefil...Good eyes ;-) -
These functions get called by other code, including parsing
config options from the command line. In that case,
config_file_name is NULL, leading to an ugly message or even
a segfault on some implementations of printf.
Signed-off-by: Jeff King <peff@peff.net>
---
config.c | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index 8064cae..cba2bcf 100644
--- a/config.c
+++ b/config.c
@@ -280,11 +280,18 @@ int git_parse_ulong(const char *value, unsigned long *ret)
return 0;
}
+static void die_bad_config(const char *name)
+{
+ if (config_file_name)
+ die("bad config value for '%s' in %s", name, config_file_name);
+ die("bad config value for '%s'", name);
+}
+
int git_config_int(const char *name, const char *value)
{
long ret;
if (!git_parse_long(value, &ret))
- die("bad config value for '%s' in %s", name, config_file_name);
+ die_bad_config(name);
return ret;
}
@@ -292,7 +299,7 @@ unsigned long git_config_ulong(const char *name, const char *value)
{
unsigned long ret;
if (!git_parse_ulong(value, &ret))
- die("bad config value for '%s' in %s", name, config_file_name);
+ die_bad_config(name);
return ret;
}
--
1.5.4.2.247.g107bd
-
| Pardo | Re: pthread_create() slow for many threads; also time to revisit 64b context switc... |
| Paul Jackson | Inquiry: Should we remove "isolcpus= kernel boot option? (may have realtime uses) |
| Srivatsa Vaddagiri | Re: [PATCH, RFC] reimplement flush_workqueue() |
| Peter Zijlstra | Re: Btrfs v0.16 released |
git: | |
| Giuseppe Bilotta | Re: gitweb and remote branches |
| Miklos Vajna | [rfc] git submodules howto |
| JD Guzman | C# Git Implementation |
| Junio C Hamano | Re: [PATCH] fix parallel make problem |
| Richard Stallman | Real men don't attack straw men |
| Steve B | SSH brute force attacks no longer being caught by PF rule |
| GVG GVG | ssh_exchange_identification: Connection closed by remote host |
| Marius ROMAN | 1440x900 resolution problem |
| Tomasz Grobelny | [PATCH 0/5] [DCCP]: Queuing policies |
| Dushan Tcholich | Re: ksoftirqd high cpu load on kernels 2.6.24 to 2.6.27-rc1-mm1 |
| John Heffner | Re: A Linux TCP SACK Question |
| Denys Fedoryshchenko | Re: Could you make vconfig less stupid? |
