* Re: [PATCH 7/7] revert: stop creating and removing sequencer-old directory
From: Ramkumar Ramachandra @ 2011-12-14 16:10 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Johannes Sixt, Junio C Hamano, git, Christian Couder,
Martin von Zweigbergk, Jay Soffian
In-Reply-To: <20111210130612.GH22035@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder wrote:
> That's the end.
Apart from the few minor details, I'm happy with the series overall.
> I hope the patches provided some amusement, and
> advice towards making them more useful would be welcome.
Oh, yes. There were quite a few :facepalm: moments in there for me ;)
Thanks for the enjoyable read.
-- Ram
^ permalink raw reply
* Re: [PATCH 6/7] Revert "reset: Make reset remove the sequencer state"
From: Ramkumar Ramachandra @ 2011-12-14 16:06 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Johannes Sixt, Junio C Hamano, git, Christian Couder,
Martin von Zweigbergk, Jay Soffian
In-Reply-To: <20111210130348.GG22035@elie.hsd1.il.comcast.net>
Hi again,
Jonathan Nieder wrote:
> This reverts commit 95eb88d8ee588d89b4f06d2753ed4d16ab13b39f, which
> was a UI experiment that did not reflect how "git reset" actually gets
> used. The reversion also fixes a test, indicated in the patch.
Looks good.
Thanks.
-- Ram
^ permalink raw reply
* Re: [RFC PATCH 3/4] Introduce a performance testing framework
From: Thomas Rast @ 2011-12-14 16:04 UTC (permalink / raw)
To: git; +Cc: Michael Haggerty
In-Reply-To: <91630874c45015e74cf72df03b6011856fd0fe4f.1323876121.git.trast@student.ethz.ch>
This patch actually needs the below fixup on top. Old files from
previous runs had fooled my testing...
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index ee6c083..f2a0b73 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -154,7 +154,7 @@ test_perf () {
then
base=$(basename "$0" .sh)
echo "$test_count" >>"$perf_results_dir"/$base.subtests
- echo "$1" >"$perf_results_dir"/$base.descr
+ echo "$1" >"$perf_results_dir"/$base.$test_count.descr
if test -z "$verbose"; then
echo -n "perf $test_count - $1:"
else
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply related
* Re: [PATCH 5/7] revert: do not remove state until sequence is finished
From: Ramkumar Ramachandra @ 2011-12-14 16:02 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Johannes Sixt, Junio C Hamano, git, Christian Couder,
Martin von Zweigbergk, Jay Soffian
In-Reply-To: <20111210130212.GF22035@elie.hsd1.il.comcast.net>
Hey,
Jonathan Nieder wrote:
> [...]
> After the recent patch "allow single-pick in the middle of cherry-pick
> sequence" we don't need that hack any more. In the new regime, a
> traditional "git cherry-pick <commit>" command never looks at
> .git/sequencer, so we do not need to cripple "git cherry-pick
> <commit>..<commit>" for it any more.
So this is why you needed that "relatively esoteric feature" :P
This approach competes with the approach I presented in "New sequencer
workflow!" [1], where I use a special case to side-step various
sequencer state files: this approach wins on the grounds of
simplicity, and I can't see any potential long-term issues with my
limited foresight. Do you have any other points of comparison?
[1]: https://github.com/artagnon/git sequencer #; 8a08d09b9
-- Ram
^ permalink raw reply
* Re: t0090-cache-tree fails due to wc whitespace
From: Andreas Schwab @ 2011-12-14 15:54 UTC (permalink / raw)
To: Thomas Rast; +Cc: Brian Gernhardt, Git List
In-Reply-To: <201112141643.06656.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> (Oddly, according to 'man 1p wc' here, the POSIXly correct format in
> the absence of options is
>
> "%d %d %d %s\n", <newlines>, <words>, <bytes>, <file>
>
> Taking it literally would mean no padding/alignment whatsoever.
> Neither GNU wc on my Linux exactly conforms to this.)
A space in the format string stands for one or more <blank>s. If only a
single <space> is allowed the standard uses 𝚫.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Re: [PATCH 4/7] revert: allow single-pick in the middle of cherry-pick sequence
From: Ramkumar Ramachandra @ 2011-12-14 15:48 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Johannes Sixt, Junio C Hamano, git, Christian Couder,
Martin von Zweigbergk, Jay Soffian
In-Reply-To: <20111210125948.GE22035@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder wrote:
> When I messed up a difficult conflict in the middle of a cherry-pick
> sequence, it can be useful to be able to 'git checkout HEAD . && git
> cherry-pick that-one-commit' to restart the conflict resolution.
I was about to complain about the commit message until I noticed that
Junio already fixed it in `next`:
revert: allow single-pick in the middle of cherry-pick sequence
After messing up a difficult conflict resolution in the middle of a
cherry-pick sequence, it can be useful to be able to
git checkout HEAD . && git cherry-pick that-one-commit
to restart the conflict resolution. The current code however errors out
saying that another cherry-pick is already in progress.
Interesting concept; let's see how it's implemented.
> Suggested-by: Johannes Sixt <j6t@kdbg.org>
Could you link to the corresponding thread with Johannes?
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 71570357..dcb69904 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -1072,6 +1072,12 @@ static int sequencer_continue(struct replay_opts *opts)
> return pick_commits(todo_list, opts);
> }
>
> +static int single_pick(struct commit *cmit, struct replay_opts *opts)
> +{
> + setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
> + return do_pick_commit(cmit, opts);
> +}
single_pick as opposed to the continue_single_pick introduced in 2/7.
> static int pick_revisions(struct replay_opts *opts)
> {
> struct commit_list *todo_list = NULL;
> @@ -1097,6 +1103,26 @@ static int pick_revisions(struct replay_opts *opts)
> return sequencer_continue(opts);
>
> /*
> + * If we were called as "git cherry-pick <commit>", just
> + * cherry-pick/revert it, set CHERRY_PICK_HEAD /
> + * REVERT_HEAD, and don't touch the sequencer state.
> + * This means it is possible to cherry-pick in the middle
> + * of a cherry-pick sequence.
> + */
Conceptually all very good. What I'm really interested in seeing is
how you persist opts for "cherry-pick --continue" when a single-commit
pick fails: in other words, how you manage to get " --continue of
single-pick respects -x" to pass.
> + if (opts->revs->cmdline.nr == 1 &&
> + opts->revs->cmdline.rev->whence == REV_CMD_REV &&
> + opts->revs->no_walk &&
> + !opts->revs->cmdline.rev->flags) {
Yuck, seriously.
1. I'd have expected you to check opts->revs->commits, not
opts->revs->cmdline.nr. Okay, you're using the cmdline because the
revision walk hasn't happened yet.
2. Why are you using opts->revs->cmdline.rev->whence as opposed to
opts->action? Why do you want to expose the underlying revision
walking mechanism?
3. When will the opts->revs->no_walk condition not be satisfied? Only
when you explicitly set it to 0 or NULL, right -- where is this
happening in revert.c?
4. Why are you checking flags? When is this condition not going to be
satisfied?
Since 3 and 4 indicate that you're being overly defensive, consistency
requires you to guarantee that this code will work no matter what the
rev_info struct is filled up with prior to this segment.
Is this true?
> + struct commit *cmit;
> + if (prepare_revision_walk(opts->revs))
> + die(_("revision walk setup failed"));
> + cmit = get_revision(opts->revs);
> + if (!cmit || get_revision(opts->revs))
> + die("BUG: expected exactly one commit from walk");
> + return single_pick(cmit, opts);
> + }
I'd have expected you to reuse prepare_revs().
> + /*
> * Start a new cherry-pick/ revert sequence; but
> * first, make sure that an existing one isn't in
> * progress
Since all your new code is a special case of "Start a new cherry-pick/
revert sequence", you don't check the sequencer state in the first
place.
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index 56c95ec1..98a27a23 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -50,6 +50,18 @@ test_expect_success 'cherry-pick persists data on failure' '
> test_path_is_file .git/sequencer/opts
> '
>
> +test_expect_success 'cherry-pick mid-cherry-pick-sequence' '
> + pristine_detach initial &&
> + test_must_fail git cherry-pick base..anotherpick &&
> + test_cmp_rev picked CHERRY_PICK_HEAD &&
> + # "oops, I forgot that these patches rely on the change from base"
> + git checkout HEAD foo &&
> + git cherry-pick base &&
> + git cherry-pick picked &&
> + git cherry-pick --continue &&
> + git diff --exit-code anotherpick
> +'
Cute feature, although I don't ever recall needing it personally. Why
does this relatively esoteric "feature" belong along with the other
"maintenance patches" in jn/maint-sequencer-fixes?
-- Ram
^ permalink raw reply
* Re: t0090-cache-tree fails due to wc whitespace
From: Thomas Rast @ 2011-12-14 15:43 UTC (permalink / raw)
To: Brian Gernhardt; +Cc: Git List
In-Reply-To: <7F1792D2-8ED4-4546-8ED4-52B95E0AE9FC@silverinsanity.com>
Brian Gernhardt wrote:
>
> It's time for my periodic complaint: People assuming `wc -l` outputs
> just a number. wc on OS X (and perhaps other BSD-like systems)
> always aligns the output in columns, even with the -l flag.
Oops.
> Generally this results in a quick patch from me to remove some
> unneeded quotes. However, this time it's used in a more complex
> manner:
[...]
> - "($(git ls-files|wc -l) entries, 0 subtrees)" >expect &&
> + "($(git ls-files|wc -l|sed -e 's/^ *//') entries, 0 subtrees)" >expect &&
I'm tempted to say we should define
test_wc_l () {
test $# = 0 || error "bug in test script: passing arguments to wc -l is not portable"
wc -l | tr -d -c 0-9
}
just to avoid issues if any wc comes across and prints a tab for
padding or says "hi, the number of lines you wanted to know is: 42".
(Oddly, according to 'man 1p wc' here, the POSIXly correct format in
the absence of options is
"%d %d %d %s\n", <newlines>, <words>, <bytes>, <file>
Taking it literally would mean no padding/alignment whatsoever.
Neither GNU wc on my Linux exactly conforms to this.)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: t0090-cache-tree fails due to wc whitespace
From: Johannes Sixt @ 2011-12-14 15:41 UTC (permalink / raw)
To: Brian Gernhardt; +Cc: Git List, Thomas Rast
In-Reply-To: <7F1792D2-8ED4-4546-8ED4-52B95E0AE9FC@silverinsanity.com>
Am 12/14/2011 15:35, schrieb Brian Gernhardt:
> It's time for my periodic complaint: People assuming `wc -l` outputs
> just a number. wc on OS X (and perhaps other BSD-like systems) always
> aligns the output in columns, even with the -l flag. Generally this
> results in a quick patch from me to remove some unneeded quotes.
> However, this time it's used in a more complex manner:
>
> echo "SHA " \
> "($(git ls-files|wc -l) entries, 0 subtrees)" >expect &&
> cmp_cache_tree expect
I'd solve it by moving the command substitution outside the quoted string:
printf "SHA (%d entries, 0 subtrees)\n" \
$(git ls-files | wc -l) >expect &&
Other proposed solutions add another process. I don't like that on Windows ;)
-- Hannes
^ permalink raw reply
* [RFC PATCH 4/4] Add a performance test for git-grep
From: Thomas Rast @ 2011-12-14 15:23 UTC (permalink / raw)
To: git; +Cc: Michael Haggerty
In-Reply-To: <cover.1323876121.git.trast@student.ethz.ch>
The only catch is that we don't really know what our repo contains, so
we have to ignore any possible "not found" status from git-grep.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
t/perf/p7810-grep.sh | 23 +++++++++++++++++++++++
1 files changed, 23 insertions(+), 0 deletions(-)
create mode 100755 t/perf/p7810-grep.sh
diff --git a/t/perf/p7810-grep.sh b/t/perf/p7810-grep.sh
new file mode 100755
index 0000000..9f4ade6
--- /dev/null
+++ b/t/perf/p7810-grep.sh
@@ -0,0 +1,23 @@
+#!/bin/sh
+
+test_description="git-grep performance in various modes"
+
+. ./perf-lib.sh
+
+test_perf_large_repo
+test_checkout_worktree
+
+test_perf 'grep worktree, cheap regex' '
+ git grep some_nonexistent_string || :
+'
+test_perf 'grep worktree, expensive regex' '
+ git grep "^.* *some_nonexistent_string$" || :
+'
+test_perf 'grep --cached, cheap regex' '
+ git grep --cached some_nonexistent_string || :
+'
+test_perf 'grep --cached, expensive regex' '
+ git grep --cached "^.* *some_nonexistent_string$" || :
+'
+
+test_done
--
1.7.8.304.ge42e4
^ permalink raw reply related
* [RFC PATCH 3/4] Introduce a performance testing framework
From: Thomas Rast @ 2011-12-14 15:23 UTC (permalink / raw)
To: git; +Cc: Michael Haggerty
In-Reply-To: <cover.1323876121.git.trast@student.ethz.ch>
This introduces a performance testing framework under t/perf/. It
tries to be as close to the test-lib.sh infrastructure as possible,
and thus should be easy to get used to for git developers.
The following points were considered for the implementation:
1. You usually want to compare arbitrary revisions/build trees against
each other. They may not have the performance test under
consideration, or even the perf-lib.sh infrastructure.
To cope with this, the 'run' script lets you specify arbitrary
build dirs and revisions. It even automatically builds the revisions
if it doesn't have them at hand yet.
2. Usually you would not want to run all tests. It would take too
long anyway. The 'run' script lets you specify which tests to run;
or you can also do it manually. There is a Makefile for
discoverability and 'make clean', but it is not meant for
real-world use.
3. Creating test repos from scratch in every test is extremely
time-consuming, and shipping or downloading such large/weird repos
is out of the question.
We leave this decision to the user. Two different sizes of test
repos can be configured, and the scripts just copy one or more of
those (using hardlinks for the object store). By default it tries
to use the build tree's git.git repository.
This is fairly fast and versatile. Using a copy instead of a clone
preserves many properties that the user may want to test for, such
as lots of loose objects, unpacked refs, etc.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
Makefile | 19 ++++-
t/Makefile | 5 +-
t/perf/.gitignore | 2 +
t/perf/Makefile | 15 +++
t/perf/README | 146 ++++++++++++++++++++++++++++
t/perf/aggregate.perl | 166 ++++++++++++++++++++++++++++++++
t/perf/min_time.perl | 21 ++++
t/perf/p0000-perf-lib-sanity.sh | 41 ++++++++
t/perf/p0001-rev-list.sh | 17 ++++
t/perf/perf-lib.sh | 199 +++++++++++++++++++++++++++++++++++++++
t/perf/run | 82 ++++++++++++++++
t/test-lib.sh | 12 ++-
12 files changed, 722 insertions(+), 3 deletions(-)
create mode 100644 t/perf/.gitignore
create mode 100644 t/perf/Makefile
create mode 100644 t/perf/README
create mode 100755 t/perf/aggregate.perl
create mode 100755 t/perf/min_time.perl
create mode 100755 t/perf/p0000-perf-lib-sanity.sh
create mode 100755 t/perf/p0001-rev-list.sh
create mode 100644 t/perf/perf-lib.sh
create mode 100755 t/perf/run
diff --git a/Makefile b/Makefile
index 52506bf..c9e0a17 100644
--- a/Makefile
+++ b/Makefile
@@ -1769,7 +1769,7 @@ export DIFF TAR INSTALL DESTDIR SHELL_PATH
SHELL = $(SHELL_PATH)
-all:: shell_compatibility_test $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
+all:: shell_compatibility_test $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS GIT-TEST-OPTIONS
ifneq (,$X)
$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
endif
@@ -2228,6 +2228,18 @@ ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT
@echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@
endif
@echo GETTEXT_POISON=\''$(subst ','\'',$(subst ','\'',$(GETTEXT_POISON)))'\' >>$@
+ifdef GIT_PERF_REPEAT_COUNT
+ @echo GIT_PERF_REPEAT_COUNT=\''$(subst ','\'',$(subst ','\'',$(GIT_PERF_REPEAT_COUNT)))'\' >>$@
+endif
+ifdef GIT_PERF_REPO
+ @echo GIT_PERF_REPO=\''$(subst ','\'',$(subst ','\'',$(GIT_PERF_REPO)))'\' >>$@
+endif
+ifdef GIT_PERF_LARGE_REPO
+ @echo GIT_PERF_LARGE_REPO=\''$(subst ','\'',$(subst ','\'',$(GIT_PERF_LARGE_REPO)))'\' >>$@
+endif
+ifdef GIT_PERF_MAKE_OPTS
+ @echo GIT_PERF_MAKE_OPTS=\''$(subst ','\'',$(subst ','\'',$(GIT_PERF_MAKE_OPTS)))'\' >>$@
+endif
### Detect Tck/Tk interpreter path changes
ifndef NO_TCLTK
@@ -2263,6 +2275,11 @@ export NO_SVN_TESTS
test: all
$(MAKE) -C t/ all
+perf: all
+ $(MAKE) -C t/perf/ all
+
+.PHONY: test perf
+
test-ctype$X: ctype.o
test-date$X: date.o ctype.o
diff --git a/t/Makefile b/t/Makefile
index 9046ec9..a481ab3 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -73,6 +73,9 @@ gitweb-test:
valgrind:
$(MAKE) GIT_TEST_OPTS="$(GIT_TEST_OPTS) --valgrind"
+perf:
+ $(MAKE) -C perf/ all
+
# Smoke testing targets
-include ../GIT-VERSION-FILE
uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo unknown')
@@ -111,4 +114,4 @@ smoke_report: smoke
http://smoke.git.nix.is/app/projects/process_add_report/1 \
| grep -v ^Redirecting
-.PHONY: pre-clean $(T) aggregate-results clean valgrind smoke smoke_report
+.PHONY: pre-clean $(T) aggregate-results clean valgrind smoke smoke_report perf
diff --git a/t/perf/.gitignore b/t/perf/.gitignore
new file mode 100644
index 0000000..50f5cc1
--- /dev/null
+++ b/t/perf/.gitignore
@@ -0,0 +1,2 @@
+build/
+test-results/
diff --git a/t/perf/Makefile b/t/perf/Makefile
new file mode 100644
index 0000000..8c47155
--- /dev/null
+++ b/t/perf/Makefile
@@ -0,0 +1,15 @@
+-include ../../config.mak
+export GIT_TEST_OPTIONS
+
+all: perf
+
+perf: pre-clean
+ ./run
+
+pre-clean:
+ rm -rf test-results
+
+clean:
+ rm -rf build "trash directory".* test-results
+
+.PHONY: all perf pre-clean clean
diff --git a/t/perf/README b/t/perf/README
new file mode 100644
index 0000000..fdf974a
--- /dev/null
+++ b/t/perf/README
@@ -0,0 +1,146 @@
+Git performance tests
+=====================
+
+This directory holds performance testing scripts for git tools. The
+first part of this document describes the various ways in which you
+can run them.
+
+When fixing the tools or adding enhancements, you are strongly
+encouraged to add tests in this directory to cover what you are
+trying to fix or enhance. The later part of this short document
+describes how your test scripts should be organized.
+
+
+Running Tests
+-------------
+
+The easiest way to run tests is to say "make". This runs all
+the tests on the current git repository.
+
+ === Running 2 tests in this tree ===
+ [...]
+ Test this tree
+ ---------------------------------------------------------
+ 0001.1: rev-list --all 0.54(0.51+0.02)
+ 0001.2: rev-list --all --objects 6.14(5.99+0.11)
+ 7810.1: grep worktree, cheap regex 0.16(0.16+0.35)
+ 7810.2: grep worktree, expensive regex 7.90(29.75+0.37)
+ 7810.3: grep --cached, cheap regex 3.07(3.02+0.25)
+ 7810.4: grep --cached, expensive regex 9.39(30.57+0.24)
+
+You can compare multiple repositories and even git revisions with the
+'run' script:
+
+ $ ./run . origin/next /path/to/git-tree p0001-rev-list.sh
+
+where . stands for the current git tree. The full invocation is
+
+ ./run [<revision|directory>...] [--] [<test-script>...]
+
+A '.' argument is implied if you do not pass any other
+revisions/directories.
+
+You can also manually test this or another git build tree, and then
+call the aggregation script to summarize the results:
+
+ $ ./p0001-rev-list.sh
+ [...]
+ $ GIT_BUILD_DIR=/path/to/other/git ./p0001-rev-list.sh
+ [...]
+ $ ./aggregate.perl . /path/to/other/git ./p0001-rev-list.sh
+
+aggregate.perl has the same invocation as 'run', it just does not run
+anything beforehand.
+
+You can set the following variables (also in your config.mak):
+
+ GIT_PERF_REPEAT_COUNT
+ Number of times a test should be repeated for best-of-N
+ measurements. Defaults to 5.
+
+ GIT_PERF_MAKE_OPTS
+ Options to use when automatically building a git tree for
+ performance testing. E.g., -j6 would be useful.
+
+ GIT_PERF_REPO
+ GIT_PERF_LARGE_REPO
+ Repositories to copy for the performance tests. The normal
+ repo should be at least git.git size. The large repo should
+ probably be about linux-2.6.git size for optimal results.
+ Both default to the git.git you are running from.
+
+You can also pass the options taken by ordinary git tests; the most
+useful one is:
+
+--root=<directory>::
+ Create "trash" directories used to store all temporary data during
+ testing under <directory>, instead of the t/ directory.
+ Using this option with a RAM-based filesystem (such as tmpfs)
+ can massively speed up the test suite.
+
+
+Naming Tests
+------------
+
+The performance test files are named as:
+
+ pNNNN-commandname-details.sh
+
+where N is a decimal digit. The same conventions for choosing NNNN as
+for normal tests apply.
+
+
+Writing Tests
+-------------
+
+The perf script starts much like a normal test script, except it
+sources perf-lib.sh:
+
+ #!/bin/sh
+ #
+ # Copyright (c) 2005 Junio C Hamano
+ #
+
+ test_description='xxx performance test'
+ . ./perf-lib.sh
+
+After that you will want to use some of the following:
+
+ test_perf_default_repo # sets up a "normal" repository
+ test_perf_large_repo # sets up a "large" repository
+
+ test_perf_default_repo sub # ditto, in a subdir "sub"
+
+ test_checkout_worktree # if you need the worktree too
+
+At least one of the first two is required!
+
+You can use test_expect_success as usual. For actual performance
+tests, use
+
+ test_perf 'descriptive string' '
+ command1 &&
+ command2
+ '
+
+test_perf spawns a subshell, for lack of better options. This means
+that
+
+* you _must_ export all variables that you need in the subshell
+
+* you _must_ flag all variables that you want to persist from the
+ subshell with 'test_export':
+
+ test_perf 'descriptive string' '
+ foo=$(git rev-parse HEAD) &&
+ test_export foo
+ '
+
+ The so-exported variables are automatically marked for export in the
+ shell executing the perf test. For your convenience, test_export is
+ the same as export in the main shell.
+
+ This feature relies on a bit of magic using 'set' and 'source'.
+ While we have tried to make sure that it can cope with embedded
+ whitespace and other special characters, it will not work with
+ multi-line data.
diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
new file mode 100755
index 0000000..15f7fc1
--- /dev/null
+++ b/t/perf/aggregate.perl
@@ -0,0 +1,166 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+use Git;
+
+sub get_times {
+ my $name = shift;
+ open my $fh, "<", $name or return undef;
+ my $line = <$fh>;
+ return undef if not defined $line;
+ close $fh or die "cannot close $name: $!";
+ $line =~ /^(?:(\d+):)?(\d+):(\d+(?:\.\d+)?) (\d+(?:\.\d+)?) (\d+(?:\.\d+)?)$/
+ or die "bad input line: $line";
+ my $rt = ((defined $1 ? $1 : 0.0)*60+$2)*60+$3;
+ return ($rt, $4, $5);
+}
+
+sub format_times {
+ my ($r, $u, $s, $firstr) = @_;
+ if (!defined $r) {
+ return "<missing>";
+ }
+ my $out = sprintf "%.2f(%.2f+%.2f)", $r, $u, $s;
+ if (defined $firstr) {
+ if ($firstr > 0) {
+ $out .= sprintf " %+.1f%%", 100.0*($r-$firstr)/$firstr;
+ } elsif ($r == 0) {
+ $out .= " =";
+ } else {
+ $out .= " +inf";
+ }
+ }
+ return $out;
+}
+
+my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests);
+while (scalar @ARGV) {
+ my $arg = $ARGV[0];
+ my $dir;
+ last if -f $arg or $arg eq "--";
+ if (! -d $arg) {
+ my $rev = Git::command_oneline(qw(rev-parse --verify), $arg);
+ $dir = "build/".$rev;
+ } else {
+ $arg =~ s{/*$}{};
+ $dir = $arg;
+ $dirabbrevs{$dir} = $dir;
+ }
+ push @dirs, $dir;
+ $dirnames{$dir} = $arg;
+ my $prefix = $dir;
+ $prefix =~ tr/^a-zA-Z0-9/_/c;
+ $prefixes{$dir} = $prefix . '.';
+ shift @ARGV;
+}
+
+if (not @dirs) {
+ @dirs = ('.');
+}
+$dirnames{'.'} = $dirabbrevs{'.'} = "this tree";
+$prefixes{'.'} = '';
+
+shift @ARGV if scalar @ARGV and $ARGV[0] eq "--";
+
+@tests = @ARGV;
+if (not @tests) {
+ @tests = glob "p????-*.sh";
+}
+
+my @subtests;
+my %shorttests;
+for my $t (@tests) {
+ $t =~ s{(?:.*/)?(p(\d+)-[^/]+)\.sh$}{$1} or die "bad test name: $t";
+ my $n = $2;
+ my $fname = "test-results/$t.subtests";
+ open my $fp, "<", $fname or die "cannot open $fname: $!";
+ for (<$fp>) {
+ chomp;
+ /^(\d+)$/ or die "malformed subtest line: $_";
+ push @subtests, "$t.$1";
+ $shorttests{"$t.$1"} = "$n.$1";
+ }
+ close $fp or die "cannot close $fname: $!";
+}
+
+sub read_descr {
+ my $name = shift;
+ open my $fh, "<", $name or return "<error reading description>";
+ my $line = <$fh>;
+ close $fh or die "cannot close $name";
+ chomp $line;
+ return $line;
+}
+
+my %descrs;
+my $descrlen = 4; # "Test"
+for my $t (@subtests) {
+ $descrs{$t} = $shorttests{$t}.": ".read_descr("test-results/$t.descr");
+ $descrlen = length $descrs{$t} if length $descrs{$t}>$descrlen;
+}
+
+sub have_duplicate {
+ my %seen;
+ for (@_) {
+ return 1 if exists $seen{$_};
+ $seen{$_} = 1;
+ }
+ return 0;
+}
+sub have_slash {
+ for (@_) {
+ return 1 if m{/};
+ }
+ return 0;
+}
+
+my %newdirabbrevs = %dirabbrevs;
+while (!have_duplicate(values %newdirabbrevs)) {
+ %dirabbrevs = %newdirabbrevs;
+ last if !have_slash(values %dirabbrevs);
+ %newdirabbrevs = %dirabbrevs;
+ for (values %newdirabbrevs) {
+ s{^[^/]*/}{};
+ }
+}
+
+my %times;
+my @colwidth = ((0)x@dirs);
+for my $i (0..$#dirs) {
+ my $d = $dirs[$i];
+ my $w = length (exists $dirabbrevs{$d} ? $dirabbrevs{$d} : $dirnames{$d});
+ $colwidth[$i] = $w if $w > $colwidth[$i];
+}
+for my $t (@subtests) {
+ my $firstr;
+ for my $i (0..$#dirs) {
+ my $d = $dirs[$i];
+ $times{$prefixes{$d}.$t} = [get_times("test-results/$prefixes{$d}$t.times")];
+ my ($r,$u,$s) = @{$times{$prefixes{$d}.$t}};
+ my $w = length format_times($r,$u,$s,$firstr);
+ $colwidth[$i] = $w if $w > $colwidth[$i];
+ $firstr = $r unless defined $firstr;
+ }
+}
+my $totalwidth = 3*@dirs+$descrlen;
+$totalwidth += $_ for (@colwidth);
+
+printf "%-${descrlen}s", "Test";
+for my $i (0..$#dirs) {
+ my $d = $dirs[$i];
+ printf " %-$colwidth[$i]s", (exists $dirabbrevs{$d} ? $dirabbrevs{$d} : $dirnames{$d});
+}
+print "\n";
+print "-"x$totalwidth, "\n";
+for my $t (@subtests) {
+ printf "%-${descrlen}s", $descrs{$t};
+ my $firstr;
+ for my $i (0..$#dirs) {
+ my $d = $dirs[$i];
+ my ($r,$u,$s) = @{$times{$prefixes{$d}.$t}};
+ printf " %-$colwidth[$i]s", format_times($r,$u,$s,$firstr);
+ $firstr = $r unless defined $firstr;
+ }
+ print "\n";
+}
diff --git a/t/perf/min_time.perl b/t/perf/min_time.perl
new file mode 100755
index 0000000..c1a2717
--- /dev/null
+++ b/t/perf/min_time.perl
@@ -0,0 +1,21 @@
+#!/usr/bin/perl
+
+my $minrt = 1e100;
+my $min;
+
+while (<>) {
+ # [h:]m:s.xx U.xx S.xx
+ /^(?:(\d+):)?(\d+):(\d+(?:\.\d+)?) (\d+(?:\.\d+)?) (\d+(?:\.\d+)?)$/
+ or die "bad input line: $_";
+ my $rt = ((defined $1 ? $1 : 0.0)*60+$2)*60+$3;
+ if ($rt < $minrt) {
+ $min = $_;
+ $minrt = $rt;
+ }
+}
+
+if (!defined $min) {
+ die "no input found";
+}
+
+print $min;
diff --git a/t/perf/p0000-perf-lib-sanity.sh b/t/perf/p0000-perf-lib-sanity.sh
new file mode 100755
index 0000000..2ca4aac
--- /dev/null
+++ b/t/perf/p0000-perf-lib-sanity.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+test_description='Tests whether perf-lib facilities work'
+. ./perf-lib.sh
+
+test_perf_default_repo
+
+test_perf 'test_perf_default_repo works' '
+ foo=$(git rev-parse HEAD) &&
+ test_export foo
+'
+
+test_checkout_worktree
+
+test_perf 'test_checkout_worktree works' '
+ wt=$(find . | wc -l) &&
+ idx=$(git ls-files | wc -l) &&
+ test $wt -gt $idx
+'
+
+baz=baz
+test_export baz
+
+test_expect_success 'test_export works' '
+ echo "$foo" &&
+ test "$foo" = "$(git rev-parse HEAD)" &&
+ echo "$baz" &&
+ test "$baz" = baz
+'
+
+test_perf 'export a weird var' '
+ bar="weird # variable" &&
+ test_export bar
+'
+
+test_expect_success 'test_export works with weird vars' '
+ echo "$bar" &&
+ test "$bar" = "weird # variable"
+'
+
+test_done
diff --git a/t/perf/p0001-rev-list.sh b/t/perf/p0001-rev-list.sh
new file mode 100755
index 0000000..4f71a63
--- /dev/null
+++ b/t/perf/p0001-rev-list.sh
@@ -0,0 +1,17 @@
+#!/bin/sh
+
+test_description="Tests history walking performance"
+
+. ./perf-lib.sh
+
+test_perf_default_repo
+
+test_perf 'rev-list --all' '
+ git rev-list --all >/dev/null
+'
+
+test_perf 'rev-list --all --objects' '
+ git rev-list --all --objects >/dev/null
+'
+
+test_done
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
new file mode 100644
index 0000000..ee6c083
--- /dev/null
+++ b/t/perf/perf-lib.sh
@@ -0,0 +1,199 @@
+#!/bin/bash
+#
+# Copyright (c) 2011 Thomas Rast
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see http://www.gnu.org/licenses/ .
+
+# do the --tee work early; it otherwise confuses our careful
+# GIT_BUILD_DIR mangling
+case "$GIT_TEST_TEE_STARTED, $* " in
+done,*)
+ # do not redirect again
+ ;;
+*' --tee '*|*' --va'*)
+ mkdir -p test-results
+ BASE=test-results/$(basename "$0" .sh)
+ (GIT_TEST_TEE_STARTED=done ${SHELL-sh} "$0" "$@" 2>&1;
+ echo $? > $BASE.exit) | tee $BASE.out
+ test "$(cat $BASE.exit)" = 0
+ exit
+ ;;
+esac
+
+TEST_DIRECTORY=$(pwd)/..
+TEST_OUTPUT_DIRECTORY=$(pwd)
+if test -z "$GIT_BUILD_DIR"; then
+ GIT_BUILD_DIR=$TEST_DIRECTORY/..
+ perf_results_prefix=
+else
+ perf_results_prefix=$(printf "%s" "$GIT_BUILD_DIR" | tr -c "[a-zA-Z0-9]" "[_*]")"."
+ # make the build dir absolute
+ GIT_BUILD_DIR=$(cd "$GIT_BUILD_DIR" && pwd)
+fi
+
+TEST_NO_CREATE_REPO=t
+
+. ../test-lib.sh
+
+perf_results_dir=$TEST_OUTPUT_DIRECTORY/test-results
+mkdir -p "$perf_results_dir"
+rm -f "$perf_results_dir"/$(basename "$0" .sh).subtests
+
+if test -z "$GIT_PERF_REPEAT_COUNT"; then
+ GIT_PERF_REPEAT_COUNT=3
+fi
+die_if_build_dir_not_repo () {
+ if ! ( cd "$TEST_DIRECTORY/.." &&
+ git rev-parse --build-dir >/dev/null 2>&1 ); then
+ error "No $1 defined, and your build directory is not a repo"
+ fi
+}
+
+if test -z "$GIT_PERF_REPO"; then
+ die_if_build_dir_not_repo '$GIT_PERF_REPO'
+ GIT_PERF_REPO=$TEST_DIRECTORY/..
+fi
+if test -z "$GIT_PERF_LARGE_REPO"; then
+ die_if_build_dir_not_repo '$GIT_PERF_LARGE_REPO'
+ GIT_PERF_LARGE_REPO=$TEST_DIRECTORY/..
+fi
+
+test_perf_create_repo_from () {
+ test "$#" = 2 ||
+ error "bug in the test script: not 2 parameters to test-create-repo"
+ repo="$1"
+ source="$2"
+ source_git=$source/$(cd "$source" && git rev-parse --git-dir)
+ mkdir -p "$repo/.git"
+ (
+ cd "$repo/.git" &&
+ { cp -Rl "$source_git/objects" . 2>/dev/null ||
+ cp -R "$source_git/objects" .; } &&
+ for stuff in "$source_git"/*; do
+ case "$stuff" in
+ */objects|*/hooks|*/config)
+ ;;
+ *)
+ cp -R "$stuff" . || break
+ ;;
+ esac
+ done &&
+ cd .. &&
+ git init -q &&
+ mv .git/hooks .git/hooks-disabled 2>/dev/null
+ ) || error "failed to copy repository '$source' to '$repo'"
+}
+
+# call at least one of these to establish an appropriately-sized repository
+test_perf_default_repo () {
+ test_perf_create_repo_from "${1:-$TRASH_DIRECTORY}" "$GIT_PERF_REPO"
+}
+test_perf_large_repo () {
+ if test "$GIT_PERF_LARGE_REPO" = "$GIT_BUILD_DIR"; then
+ echo "warning: \$GIT_PERF_LARGE_REPO is \$GIT_BUILD_DIR." >&2
+ echo "warning: This will work, but may not be a sufficiently large repo" >&2
+ echo "warning: for representative measurements." >&2
+ fi
+ test_perf_create_repo_from "${1:-$TRASH_DIRECTORY}" "$GIT_PERF_LARGE_REPO"
+}
+test_checkout_worktree () {
+ git checkout-index -u -a ||
+ error "git checkout-index failed"
+}
+
+# Performance tests should never fail. If they do, stop immediately
+immediate=t
+
+test_run_perf_ () {
+ test_cleanup=:
+ test_export_="test_cleanup"
+ export test_cleanup test_export_
+ /usr/bin/time -f "%E %U %S" -o test_time.$i "$SHELL" -c '
+. '"$TEST_DIRECTORY"/../test-lib-functions.sh'
+test_export () {
+ [ $# != 0 ] || return 0
+ test_export_="$test_export_\\|$1"
+ shift
+ test_export "$@"
+}
+'"$1"'
+ret=$?
+set | sed -n "s'"/'/'\\\\''/g"';s/^\\($test_export_\\)/export '"'&'"'/p" >test_vars
+exit $ret' >&3 2>&4
+ eval_ret=$?
+
+ if test $eval_ret = 0 || test -n "$expecting_failure"
+ then
+ test_eval_ "$test_cleanup"
+ source ./test_vars || error "failed to load updated environment"
+ fi
+ if test "$verbose" = "t" && test -n "$HARNESS_ACTIVE"; then
+ echo ""
+ fi
+ return "$eval_ret"
+}
+
+
+test_perf () {
+ test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
+ test "$#" = 2 ||
+ error "bug in the test script: not 2 or 3 parameters to test-expect-success"
+ export test_prereq
+ if ! test_skip "$@"
+ then
+ base=$(basename "$0" .sh)
+ echo "$test_count" >>"$perf_results_dir"/$base.subtests
+ echo "$1" >"$perf_results_dir"/$base.descr
+ if test -z "$verbose"; then
+ echo -n "perf $test_count - $1:"
+ else
+ echo "perf $test_count - $1:"
+ fi
+ for i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do
+ say >&3 "running: $2"
+ if test_run_perf_ "$2"
+ then
+ if test -z "$verbose"; then
+ echo -n " $i"
+ else
+ echo "* timing run $i/$GIT_PERF_REPEAT_COUNT:"
+ fi
+ else
+ test -z "$verbose" && echo
+ test_failure_ "$@"
+ break
+ fi
+ done
+ if test -z "$verbose"; then
+ echo " ok"
+ else
+ test_ok_ "$1"
+ fi
+ base="$perf_results_dir"/"$perf_results_prefix$(basename "$0" .sh)"."$test_count"
+ "$TEST_DIRECTORY"/perf/min_time.perl test_time.* >"$base".times
+ fi
+ echo >&3 ""
+}
+
+# We extend test_done to print timings at the end (./run disables this
+# and does it after running everything)
+test_at_end_hook_ () {
+ if test -z "$GIT_PERF_AGGREGATING_LATER"; then
+ ( cd "$TEST_DIRECTORY"/perf && ./aggregate.perl $(basename "$0") )
+ fi
+}
+
+test_export () {
+ export "$@"
+}
diff --git a/t/perf/run b/t/perf/run
new file mode 100755
index 0000000..85ead88
--- /dev/null
+++ b/t/perf/run
@@ -0,0 +1,82 @@
+#!/bin/sh
+
+case "$1" in
+ --help)
+ echo "usage: $0 [other_git_tree...] [--] [test_scripts]"
+ exit 0
+ ;;
+esac
+
+die () {
+ echo >&2 "error: $*"
+ exit 1
+}
+
+run_one_dir () {
+ if test $# -eq 0; then
+ set -- p????-*.sh
+ fi
+ echo "=== Running $# tests in ${GIT_BUILD_DIR:-this tree} ==="
+ for t in "$@"; do
+ ./$t $GIT_TEST_OPTS
+ done
+}
+
+unpack_git_rev () {
+ rev=$1
+ mkdir -p build/$rev
+ (cd "$(git rev-parse --show-cdup)" && git archive --format=tar $rev) |
+ (cd build/$rev && tar x)
+}
+build_git_rev () {
+ rev=$1
+ cp ../../config.mak build/$rev/config.mak
+ (cd build/$rev && make $GIT_PERF_MAKE_OPTS) ||
+ die "failed to build revision '$mydir'"
+}
+
+run_dirs_helper () {
+ mydir=${1%/}
+ shift
+ while test $# -gt 0 -a "$1" != -- -a ! -f "$1"; do
+ shift
+ done
+ if test $# -gt 0 -a "$1" == --; then
+ shift
+ fi
+ if [ ! -d "$mydir" ]; then
+ rev=$(git rev-parse --verify "$mydir" 2>/dev/null) ||
+ die "'$mydir' is neither a directory nor a valid revision"
+ if [ ! -d build/$rev ]; then
+ unpack_git_rev $rev
+ fi
+ build_git_rev $rev
+ mydir=build/$rev
+ fi
+ if test "$mydir" = .; then
+ unset GIT_BUILD_DIR
+ else
+ GIT_BUILD_DIR="$mydir"
+ export GIT_BUILD_DIR
+ fi
+ run_one_dir "$@"
+}
+
+run_dirs () {
+ while test $# -gt 0 -a "$1" != -- -a ! -f "$1"; do
+ run_dirs_helper "$@"
+ shift
+ done
+}
+
+GIT_PERF_AGGREGATING_LATER=t
+export GIT_PERF_AGGREGATING_LATER
+
+cd "$(dirname $0)"
+. ../../GIT-TEST-OPTIONS
+
+if test $# = 0 -o "$1" = -- -o -f "$1"; then
+ set -- . "$@"
+fi
+run_dirs "$@"
+./aggregate.perl "$@"
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 06f8c96..83bb7b3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -55,6 +55,7 @@ unset $(perl -e '
PROVE
VALGRIND
BUILD_DIR
+ PERF_AGGREGATING_LATER
));
my @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);
print join("\n", @vars);
@@ -313,6 +314,9 @@ test_skip () {
esac
}
+# stub; perf-lib overrides it
+test_at_end_hook_ () {
+ :
}
test_done () {
@@ -358,6 +362,8 @@ test_done () {
cd "$(dirname "$remove_trash")" &&
rm -rf "$(basename "$remove_trash")"
+ test_at_end_hook_
+
exit 0 ;;
*)
@@ -539,7 +545,11 @@ rm -fr "$test" || {
HOME="$TRASH_DIRECTORY"
export HOME
-test_create_repo "$test"
+if test -z "$TEST_NO_CREATE_REPO"; then
+ test_create_repo "$test"
+else
+ mkdir -p "$test"
+fi
# Use -P to resolve symlinks in our working directory so that the cwd
# in subprocesses like git equals our $PWD (for pathname comparisons).
cd -P "$test" || exit 1
--
1.7.8.304.ge42e4
^ permalink raw reply related
* [RFC PATCH 2/4] test-lib: allow testing another git build tree
From: Thomas Rast @ 2011-12-14 15:23 UTC (permalink / raw)
To: git; +Cc: Michael Haggerty
In-Reply-To: <cover.1323876121.git.trast@student.ethz.ch>
The perf-lib work wants this feature, so we may as well do it for
test-lib in general. You can now say
GIT_BUILD_DIR=/another/git/tree
make test # or any other test
and it will run the tests of the current tree against the binaries of
the other tree.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
Makefile | 7 ++++++-
t/test-lib.sh | 19 ++++++++++++++++---
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/Makefile b/Makefile
index 2127c1b..52506bf 100644
--- a/Makefile
+++ b/Makefile
@@ -2215,6 +2215,12 @@ GIT-BUILD-OPTIONS: FORCE
@echo USE_LIBPCRE=\''$(subst ','\'',$(subst ','\'',$(USE_LIBPCRE)))'\' >>$@
@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@
@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@
+ @echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@
+
+GIT-TEST-OPTIONS: FORCE
+ifdef GIT_TEST_OPTS
+ @echo GIT_TEST_OPTS=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_OPTS)))'\' >$@
+endif
ifdef GIT_TEST_CMP
@echo GIT_TEST_CMP=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_CMP)))'\' >>$@
endif
@@ -2222,7 +2228,6 @@ ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT
@echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@
endif
@echo GETTEXT_POISON=\''$(subst ','\'',$(subst ','\'',$(GETTEXT_POISON)))'\' >>$@
- @echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@
### Detect Tck/Tk interpreter path changes
ifndef NO_TCLTK
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 368e48c..06f8c96 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -54,6 +54,7 @@ unset $(perl -e '
.*_TEST
PROVE
VALGRIND
+ BUILD_DIR
));
my @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);
print join("\n", @vars);
@@ -318,7 +319,7 @@ test_done () {
GIT_EXIT_OK=t
if test -z "$HARNESS_ACTIVE"; then
- test_results_dir="$TEST_DIRECTORY/test-results"
+ test_results_dir="$TEST_OUTPUT_DIRECTORY/test-results"
mkdir -p "$test_results_dir"
test_results_path="$test_results_dir/${0%.sh}-$$.counts"
@@ -379,7 +380,18 @@ then
# itself.
TEST_DIRECTORY=$(pwd)
fi
-GIT_BUILD_DIR="$TEST_DIRECTORY"/..
+if test -z "$TEST_OUTPUT_DIRECTORY"
+then
+ # Similarly, override this to store the test-results subdir
+ # elsewhere
+ TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
+fi
+if test -z "$GIT_BUILD_DIR"
+then
+ # Similarly, override this to test a different git build tree
+ # than the one you are running the tests from
+ GIT_BUILD_DIR="$TEST_DIRECTORY"/..
+fi
if test -n "$valgrind"
then
@@ -477,6 +489,7 @@ GIT_ATTR_NOSYSTEM=1
export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_ATTR_NOSYSTEM
. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+. "$TEST_DIRECTORY"/../GIT-TEST-OPTIONS
if test -z "$GIT_TEST_CMP"
then
@@ -514,7 +527,7 @@ test="trash directory.$(basename "$0" .sh)"
test -n "$root" && test="$root/$test"
case "$test" in
/*) TRASH_DIRECTORY="$test" ;;
- *) TRASH_DIRECTORY="$TEST_DIRECTORY/$test" ;;
+ *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$test" ;;
esac
test ! -z "$debug" || remove_trash=$TRASH_DIRECTORY
rm -fr "$test" || {
--
1.7.8.304.ge42e4
^ permalink raw reply related
* [RFC PATCH 1/4] Move the user-facing test library to test-lib-functions.sh
From: Thomas Rast @ 2011-12-14 15:23 UTC (permalink / raw)
To: git; +Cc: Michael Haggerty
In-Reply-To: <cover.1323876121.git.trast@student.ethz.ch>
This just moves all the user-facing functions to a separate file and
sources that instead.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
t/test-lib-functions.sh | 528 ++++++++++++++++++++++++++++++++++++++++++++++
t/test-lib.sh | 530 +----------------------------------------------
2 files changed, 533 insertions(+), 525 deletions(-)
create mode 100644 t/test-lib-functions.sh
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
new file mode 100644
index 0000000..103c97e
--- /dev/null
+++ b/t/test-lib-functions.sh
@@ -0,0 +1,528 @@
+# The semantics of the editor variables are that of invoking
+# sh -c "$EDITOR \"$@\"" files ...
+#
+# If our trash directory contains shell metacharacters, they will be
+# interpreted if we just set $EDITOR directly, so do a little dance with
+# environment variables to work around this.
+#
+# In particular, quoting isn't enough, as the path may contain the same quote
+# that we're using.
+test_set_editor () {
+ FAKE_EDITOR="$1"
+ export FAKE_EDITOR
+ EDITOR='"$FAKE_EDITOR"'
+ export EDITOR
+}
+
+test_decode_color () {
+ awk '
+ function name(n) {
+ if (n == 0) return "RESET";
+ if (n == 1) return "BOLD";
+ if (n == 30) return "BLACK";
+ if (n == 31) return "RED";
+ if (n == 32) return "GREEN";
+ if (n == 33) return "YELLOW";
+ if (n == 34) return "BLUE";
+ if (n == 35) return "MAGENTA";
+ if (n == 36) return "CYAN";
+ if (n == 37) return "WHITE";
+ if (n == 40) return "BLACK";
+ if (n == 41) return "BRED";
+ if (n == 42) return "BGREEN";
+ if (n == 43) return "BYELLOW";
+ if (n == 44) return "BBLUE";
+ if (n == 45) return "BMAGENTA";
+ if (n == 46) return "BCYAN";
+ if (n == 47) return "BWHITE";
+ }
+ {
+ while (match($0, /\033\[[0-9;]*m/) != 0) {
+ printf "%s<", substr($0, 1, RSTART-1);
+ codes = substr($0, RSTART+2, RLENGTH-3);
+ if (length(codes) == 0)
+ printf "%s", name(0)
+ else {
+ n = split(codes, ary, ";");
+ sep = "";
+ for (i = 1; i <= n; i++) {
+ printf "%s%s", sep, name(ary[i]);
+ sep = ";"
+ }
+ }
+ printf ">";
+ $0 = substr($0, RSTART + RLENGTH, length($0) - RSTART - RLENGTH + 1);
+ }
+ print
+ }
+ '
+}
+
+nul_to_q () {
+ perl -pe 'y/\000/Q/'
+}
+
+q_to_nul () {
+ perl -pe 'y/Q/\000/'
+}
+
+q_to_cr () {
+ tr Q '\015'
+}
+
+q_to_tab () {
+ tr Q '\011'
+}
+
+append_cr () {
+ sed -e 's/$/Q/' | tr Q '\015'
+}
+
+remove_cr () {
+ tr '\015' Q | sed -e 's/Q$//'
+}
+
+# In some bourne shell implementations, the "unset" builtin returns
+# nonzero status when a variable to be unset was not set in the first
+# place.
+#
+# Use sane_unset when that should not be considered an error.
+
+sane_unset () {
+ unset "$@"
+ return 0
+}
+
+test_tick () {
+ if test -z "${test_tick+set}"
+ then
+ test_tick=1112911993
+ else
+ test_tick=$(($test_tick + 60))
+ fi
+ GIT_COMMITTER_DATE="$test_tick -0700"
+ GIT_AUTHOR_DATE="$test_tick -0700"
+ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
+}
+
+# Call test_commit with the arguments "<message> [<file> [<contents>]]"
+#
+# This will commit a file with the given contents and the given commit
+# message. It will also add a tag with <message> as name.
+#
+# Both <file> and <contents> default to <message>.
+
+test_commit () {
+ file=${2:-"$1.t"}
+ echo "${3-$1}" > "$file" &&
+ git add "$file" &&
+ test_tick &&
+ git commit -m "$1" &&
+ git tag "$1"
+}
+
+# Call test_merge with the arguments "<message> <commit>", where <commit>
+# can be a tag pointing to the commit-to-merge.
+
+test_merge () {
+ test_tick &&
+ git merge -m "$1" "$2" &&
+ git tag "$1"
+}
+
+# This function helps systems where core.filemode=false is set.
+# Use it instead of plain 'chmod +x' to set or unset the executable bit
+# of a file in the working directory and add it to the index.
+
+test_chmod () {
+ chmod "$@" &&
+ git update-index --add "--chmod=$@"
+}
+
+# Unset a configuration variable, but don't fail if it doesn't exist.
+test_unconfig () {
+ git config --unset-all "$@"
+ config_status=$?
+ case "$config_status" in
+ 5) # ok, nothing to unset
+ config_status=0
+ ;;
+ esac
+ return $config_status
+}
+
+# Set git config, automatically unsetting it after the test is over.
+test_config () {
+ test_when_finished "test_unconfig '$1'" &&
+ git config "$@"
+}
+
+test_config_global () {
+ test_when_finished "test_unconfig --global '$1'" &&
+ git config --global "$@"
+}
+
+# Use test_set_prereq to tell that a particular prerequisite is available.
+# The prerequisite can later be checked for in two ways:
+#
+# - Explicitly using test_have_prereq.
+#
+# - Implicitly by specifying the prerequisite tag in the calls to
+# test_expect_{success,failure,code}.
+#
+# The single parameter is the prerequisite tag (a simple word, in all
+# capital letters by convention).
+
+test_set_prereq () {
+ satisfied="$satisfied$1 "
+}
+satisfied=" "
+
+test_have_prereq () {
+ # prerequisites can be concatenated with ','
+ save_IFS=$IFS
+ IFS=,
+ set -- $*
+ IFS=$save_IFS
+
+ total_prereq=0
+ ok_prereq=0
+ missing_prereq=
+
+ for prerequisite
+ do
+ total_prereq=$(($total_prereq + 1))
+ case $satisfied in
+ *" $prerequisite "*)
+ ok_prereq=$(($ok_prereq + 1))
+ ;;
+ *)
+ # Keep a list of missing prerequisites
+ if test -z "$missing_prereq"
+ then
+ missing_prereq=$prerequisite
+ else
+ missing_prereq="$prerequisite,$missing_prereq"
+ fi
+ esac
+ done
+
+ test $total_prereq = $ok_prereq
+}
+
+test_declared_prereq () {
+ case ",$test_prereq," in
+ *,$1,*)
+ return 0
+ ;;
+ esac
+ return 1
+}
+
+test_expect_failure () {
+ test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
+ test "$#" = 2 ||
+ error "bug in the test script: not 2 or 3 parameters to test-expect-failure"
+ export test_prereq
+ if ! test_skip "$@"
+ then
+ say >&3 "checking known breakage: $2"
+ if test_run_ "$2" expecting_failure
+ then
+ test_known_broken_ok_ "$1"
+ else
+ test_known_broken_failure_ "$1"
+ fi
+ fi
+ echo >&3 ""
+}
+
+test_expect_success () {
+ test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
+ test "$#" = 2 ||
+ error "bug in the test script: not 2 or 3 parameters to test-expect-success"
+ export test_prereq
+ if ! test_skip "$@"
+ then
+ say >&3 "expecting success: $2"
+ if test_run_ "$2"
+ then
+ test_ok_ "$1"
+ else
+ test_failure_ "$@"
+ fi
+ fi
+ echo >&3 ""
+}
+
+# test_external runs external test scripts that provide continuous
+# test output about their progress, and succeeds/fails on
+# zero/non-zero exit code. It outputs the test output on stdout even
+# in non-verbose mode, and announces the external script with "# run
+# <n>: ..." before running it. When providing relative paths, keep in
+# mind that all scripts run in "trash directory".
+# Usage: test_external description command arguments...
+# Example: test_external 'Perl API' perl ../path/to/test.pl
+test_external () {
+ test "$#" = 4 && { test_prereq=$1; shift; } || test_prereq=
+ test "$#" = 3 ||
+ error >&5 "bug in the test script: not 3 or 4 parameters to test_external"
+ descr="$1"
+ shift
+ export test_prereq
+ if ! test_skip "$descr" "$@"
+ then
+ # Announce the script to reduce confusion about the
+ # test output that follows.
+ say_color "" "# run $test_count: $descr ($*)"
+ # Export TEST_DIRECTORY, TRASH_DIRECTORY and GIT_TEST_LONG
+ # to be able to use them in script
+ export TEST_DIRECTORY TRASH_DIRECTORY GIT_TEST_LONG
+ # Run command; redirect its stderr to &4 as in
+ # test_run_, but keep its stdout on our stdout even in
+ # non-verbose mode.
+ "$@" 2>&4
+ if [ "$?" = 0 ]
+ then
+ if test $test_external_has_tap -eq 0; then
+ test_ok_ "$descr"
+ else
+ say_color "" "# test_external test $descr was ok"
+ test_success=$(($test_success + 1))
+ fi
+ else
+ if test $test_external_has_tap -eq 0; then
+ test_failure_ "$descr" "$@"
+ else
+ say_color error "# test_external test $descr failed: $@"
+ test_failure=$(($test_failure + 1))
+ fi
+ fi
+ fi
+}
+
+# Like test_external, but in addition tests that the command generated
+# no output on stderr.
+test_external_without_stderr () {
+ # The temporary file has no (and must have no) security
+ # implications.
+ tmp=${TMPDIR:-/tmp}
+ stderr="$tmp/git-external-stderr.$$.tmp"
+ test_external "$@" 4> "$stderr"
+ [ -f "$stderr" ] || error "Internal error: $stderr disappeared."
+ descr="no stderr: $1"
+ shift
+ say >&3 "# expecting no stderr from previous command"
+ if [ ! -s "$stderr" ]; then
+ rm "$stderr"
+
+ if test $test_external_has_tap -eq 0; then
+ test_ok_ "$descr"
+ else
+ say_color "" "# test_external_without_stderr test $descr was ok"
+ test_success=$(($test_success + 1))
+ fi
+ else
+ if [ "$verbose" = t ]; then
+ output=`echo; echo "# Stderr is:"; cat "$stderr"`
+ else
+ output=
+ fi
+ # rm first in case test_failure exits.
+ rm "$stderr"
+ if test $test_external_has_tap -eq 0; then
+ test_failure_ "$descr" "$@" "$output"
+ else
+ say_color error "# test_external_without_stderr test $descr failed: $@: $output"
+ test_failure=$(($test_failure + 1))
+ fi
+ fi
+}
+
+# debugging-friendly alternatives to "test [-f|-d|-e]"
+# The commands test the existence or non-existence of $1. $2 can be
+# given to provide a more precise diagnosis.
+test_path_is_file () {
+ if ! [ -f "$1" ]
+ then
+ echo "File $1 doesn't exist. $*"
+ false
+ fi
+}
+
+test_path_is_dir () {
+ if ! [ -d "$1" ]
+ then
+ echo "Directory $1 doesn't exist. $*"
+ false
+ fi
+}
+
+test_path_is_missing () {
+ if [ -e "$1" ]
+ then
+ echo "Path exists:"
+ ls -ld "$1"
+ if [ $# -ge 1 ]; then
+ echo "$*"
+ fi
+ false
+ fi
+}
+
+# test_line_count checks that a file has the number of lines it
+# ought to. For example:
+#
+# test_expect_success 'produce exactly one line of output' '
+# do something >output &&
+# test_line_count = 1 output
+# '
+#
+# is like "test $(wc -l <output) = 1" except that it passes the
+# output through when the number of lines is wrong.
+
+test_line_count () {
+ if test $# != 3
+ then
+ error "bug in the test script: not 3 parameters to test_line_count"
+ elif ! test $(wc -l <"$3") "$1" "$2"
+ then
+ echo "test_line_count: line count for $3 !$1 $2"
+ cat "$3"
+ return 1
+ fi
+}
+
+# This is not among top-level (test_expect_success | test_expect_failure)
+# but is a prefix that can be used in the test script, like:
+#
+# test_expect_success 'complain and die' '
+# do something &&
+# do something else &&
+# test_must_fail git checkout ../outerspace
+# '
+#
+# Writing this as "! git checkout ../outerspace" is wrong, because
+# the failure could be due to a segv. We want a controlled failure.
+
+test_must_fail () {
+ "$@"
+ exit_code=$?
+ if test $exit_code = 0; then
+ echo >&2 "test_must_fail: command succeeded: $*"
+ return 1
+ elif test $exit_code -gt 129 -a $exit_code -le 192; then
+ echo >&2 "test_must_fail: died by signal: $*"
+ return 1
+ elif test $exit_code = 127; then
+ echo >&2 "test_must_fail: command not found: $*"
+ return 1
+ fi
+ return 0
+}
+
+# Similar to test_must_fail, but tolerates success, too. This is
+# meant to be used in contexts like:
+#
+# test_expect_success 'some command works without configuration' '
+# test_might_fail git config --unset all.configuration &&
+# do something
+# '
+#
+# Writing "git config --unset all.configuration || :" would be wrong,
+# because we want to notice if it fails due to segv.
+
+test_might_fail () {
+ "$@"
+ exit_code=$?
+ if test $exit_code -gt 129 -a $exit_code -le 192; then
+ echo >&2 "test_might_fail: died by signal: $*"
+ return 1
+ elif test $exit_code = 127; then
+ echo >&2 "test_might_fail: command not found: $*"
+ return 1
+ fi
+ return 0
+}
+
+# Similar to test_must_fail and test_might_fail, but check that a
+# given command exited with a given exit code. Meant to be used as:
+#
+# test_expect_success 'Merge with d/f conflicts' '
+# test_expect_code 1 git merge "merge msg" B master
+# '
+
+test_expect_code () {
+ want_code=$1
+ shift
+ "$@"
+ exit_code=$?
+ if test $exit_code = $want_code
+ then
+ return 0
+ fi
+
+ echo >&2 "test_expect_code: command exited with $exit_code, we wanted $want_code $*"
+ return 1
+}
+
+# test_cmp is a helper function to compare actual and expected output.
+# You can use it like:
+#
+# test_expect_success 'foo works' '
+# echo expected >expected &&
+# foo >actual &&
+# test_cmp expected actual
+# '
+#
+# This could be written as either "cmp" or "diff -u", but:
+# - cmp's output is not nearly as easy to read as diff -u
+# - not all diff versions understand "-u"
+
+test_cmp() {
+ $GIT_TEST_CMP "$@"
+}
+
+# This function can be used to schedule some commands to be run
+# unconditionally at the end of the test to restore sanity:
+#
+# test_expect_success 'test core.capslock' '
+# git config core.capslock true &&
+# test_when_finished "git config --unset core.capslock" &&
+# hello world
+# '
+#
+# That would be roughly equivalent to
+#
+# test_expect_success 'test core.capslock' '
+# git config core.capslock true &&
+# hello world
+# git config --unset core.capslock
+# '
+#
+# except that the greeting and config --unset must both succeed for
+# the test to pass.
+#
+# Note that under --immediate mode, no clean-up is done to help diagnose
+# what went wrong.
+
+test_when_finished () {
+ test_cleanup="{ $*
+ } && (exit \"\$eval_ret\"); eval_ret=\$?; $test_cleanup"
+}
+
+# Most tests can use the created repository, but some may need to create more.
+# Usage: test_create_repo <directory>
+test_create_repo () {
+ test "$#" = 1 ||
+ error "bug in the test script: not 1 parameter to test-create-repo"
+ repo="$1"
+ mkdir -p "$repo"
+ (
+ cd "$repo" || error "Cannot setup test environment"
+ "$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
+ error "cannot run git init -- have you built things yet?"
+ mv .git/hooks .git/hooks-disabled
+ ) || exit
+}
+
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 160479b..368e48c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -96,6 +96,8 @@ _z40=0000000000000000000000000000000000000000
LF='
'
+export _x05 _x40 _z40 LF
+
# Each test should start with something like this, after copyright notices:
#
# test_description='Description of this test...
@@ -220,226 +222,9 @@ die () {
GIT_EXIT_OK=
trap 'die' EXIT
-# The semantics of the editor variables are that of invoking
-# sh -c "$EDITOR \"$@\"" files ...
-#
-# If our trash directory contains shell metacharacters, they will be
-# interpreted if we just set $EDITOR directly, so do a little dance with
-# environment variables to work around this.
-#
-# In particular, quoting isn't enough, as the path may contain the same quote
-# that we're using.
-test_set_editor () {
- FAKE_EDITOR="$1"
- export FAKE_EDITOR
- EDITOR='"$FAKE_EDITOR"'
- export EDITOR
-}
-
-test_decode_color () {
- awk '
- function name(n) {
- if (n == 0) return "RESET";
- if (n == 1) return "BOLD";
- if (n == 30) return "BLACK";
- if (n == 31) return "RED";
- if (n == 32) return "GREEN";
- if (n == 33) return "YELLOW";
- if (n == 34) return "BLUE";
- if (n == 35) return "MAGENTA";
- if (n == 36) return "CYAN";
- if (n == 37) return "WHITE";
- if (n == 40) return "BLACK";
- if (n == 41) return "BRED";
- if (n == 42) return "BGREEN";
- if (n == 43) return "BYELLOW";
- if (n == 44) return "BBLUE";
- if (n == 45) return "BMAGENTA";
- if (n == 46) return "BCYAN";
- if (n == 47) return "BWHITE";
- }
- {
- while (match($0, /\033\[[0-9;]*m/) != 0) {
- printf "%s<", substr($0, 1, RSTART-1);
- codes = substr($0, RSTART+2, RLENGTH-3);
- if (length(codes) == 0)
- printf "%s", name(0)
- else {
- n = split(codes, ary, ";");
- sep = "";
- for (i = 1; i <= n; i++) {
- printf "%s%s", sep, name(ary[i]);
- sep = ";"
- }
- }
- printf ">";
- $0 = substr($0, RSTART + RLENGTH, length($0) - RSTART - RLENGTH + 1);
- }
- print
- }
- '
-}
-
-nul_to_q () {
- perl -pe 'y/\000/Q/'
-}
-
-q_to_nul () {
- perl -pe 'y/Q/\000/'
-}
-
-q_to_cr () {
- tr Q '\015'
-}
-
-q_to_tab () {
- tr Q '\011'
-}
-
-append_cr () {
- sed -e 's/$/Q/' | tr Q '\015'
-}
-
-remove_cr () {
- tr '\015' Q | sed -e 's/Q$//'
-}
-
-# In some bourne shell implementations, the "unset" builtin returns
-# nonzero status when a variable to be unset was not set in the first
-# place.
-#
-# Use sane_unset when that should not be considered an error.
-
-sane_unset () {
- unset "$@"
- return 0
-}
-
-test_tick () {
- if test -z "${test_tick+set}"
- then
- test_tick=1112911993
- else
- test_tick=$(($test_tick + 60))
- fi
- GIT_COMMITTER_DATE="$test_tick -0700"
- GIT_AUTHOR_DATE="$test_tick -0700"
- export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
-}
-
-# Call test_commit with the arguments "<message> [<file> [<contents>]]"
-#
-# This will commit a file with the given contents and the given commit
-# message. It will also add a tag with <message> as name.
-#
-# Both <file> and <contents> default to <message>.
-
-test_commit () {
- file=${2:-"$1.t"}
- echo "${3-$1}" > "$file" &&
- git add "$file" &&
- test_tick &&
- git commit -m "$1" &&
- git tag "$1"
-}
-
-# Call test_merge with the arguments "<message> <commit>", where <commit>
-# can be a tag pointing to the commit-to-merge.
-
-test_merge () {
- test_tick &&
- git merge -m "$1" "$2" &&
- git tag "$1"
-}
-
-# This function helps systems where core.filemode=false is set.
-# Use it instead of plain 'chmod +x' to set or unset the executable bit
-# of a file in the working directory and add it to the index.
-
-test_chmod () {
- chmod "$@" &&
- git update-index --add "--chmod=$@"
-}
-
-# Unset a configuration variable, but don't fail if it doesn't exist.
-test_unconfig () {
- git config --unset-all "$@"
- config_status=$?
- case "$config_status" in
- 5) # ok, nothing to unset
- config_status=0
- ;;
- esac
- return $config_status
-}
-
-# Set git config, automatically unsetting it after the test is over.
-test_config () {
- test_when_finished "test_unconfig '$1'" &&
- git config "$@"
-}
-
-test_config_global () {
- test_when_finished "test_unconfig --global '$1'" &&
- git config --global "$@"
-}
-
-# Use test_set_prereq to tell that a particular prerequisite is available.
-# The prerequisite can later be checked for in two ways:
-#
-# - Explicitly using test_have_prereq.
-#
-# - Implicitly by specifying the prerequisite tag in the calls to
-# test_expect_{success,failure,code}.
-#
-# The single parameter is the prerequisite tag (a simple word, in all
-# capital letters by convention).
-
-test_set_prereq () {
- satisfied="$satisfied$1 "
-}
-satisfied=" "
-
-test_have_prereq () {
- # prerequisites can be concatenated with ','
- save_IFS=$IFS
- IFS=,
- set -- $*
- IFS=$save_IFS
-
- total_prereq=0
- ok_prereq=0
- missing_prereq=
-
- for prerequisite
- do
- total_prereq=$(($total_prereq + 1))
- case $satisfied in
- *" $prerequisite "*)
- ok_prereq=$(($ok_prereq + 1))
- ;;
- *)
- # Keep a list of missing prerequisites
- if test -z "$missing_prereq"
- then
- missing_prereq=$prerequisite
- else
- missing_prereq="$prerequisite,$missing_prereq"
- fi
- esac
- done
-
- test $total_prereq = $ok_prereq
-}
-
-test_declared_prereq () {
- case ",$test_prereq," in
- *,$1,*)
- return 0
- ;;
- esac
- return 1
-}
+# The user-facing functions are loaded from a separate file so that
+# test_perf subshells can have them too
+. "${TEST_DIRECTORY:-.}"/test-lib-functions.sh
# You are not expected to call test_ok_ and test_failure_ directly, use
# the text_expect_* functions instead.
@@ -527,311 +312,6 @@ test_skip () {
esac
}
-test_expect_failure () {
- test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
- test "$#" = 2 ||
- error "bug in the test script: not 2 or 3 parameters to test-expect-failure"
- export test_prereq
- if ! test_skip "$@"
- then
- say >&3 "checking known breakage: $2"
- if test_run_ "$2" expecting_failure
- then
- test_known_broken_ok_ "$1"
- else
- test_known_broken_failure_ "$1"
- fi
- fi
- echo >&3 ""
-}
-
-test_expect_success () {
- test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
- test "$#" = 2 ||
- error "bug in the test script: not 2 or 3 parameters to test-expect-success"
- export test_prereq
- if ! test_skip "$@"
- then
- say >&3 "expecting success: $2"
- if test_run_ "$2"
- then
- test_ok_ "$1"
- else
- test_failure_ "$@"
- fi
- fi
- echo >&3 ""
-}
-
-# test_external runs external test scripts that provide continuous
-# test output about their progress, and succeeds/fails on
-# zero/non-zero exit code. It outputs the test output on stdout even
-# in non-verbose mode, and announces the external script with "# run
-# <n>: ..." before running it. When providing relative paths, keep in
-# mind that all scripts run in "trash directory".
-# Usage: test_external description command arguments...
-# Example: test_external 'Perl API' perl ../path/to/test.pl
-test_external () {
- test "$#" = 4 && { test_prereq=$1; shift; } || test_prereq=
- test "$#" = 3 ||
- error >&5 "bug in the test script: not 3 or 4 parameters to test_external"
- descr="$1"
- shift
- export test_prereq
- if ! test_skip "$descr" "$@"
- then
- # Announce the script to reduce confusion about the
- # test output that follows.
- say_color "" "# run $test_count: $descr ($*)"
- # Export TEST_DIRECTORY, TRASH_DIRECTORY and GIT_TEST_LONG
- # to be able to use them in script
- export TEST_DIRECTORY TRASH_DIRECTORY GIT_TEST_LONG
- # Run command; redirect its stderr to &4 as in
- # test_run_, but keep its stdout on our stdout even in
- # non-verbose mode.
- "$@" 2>&4
- if [ "$?" = 0 ]
- then
- if test $test_external_has_tap -eq 0; then
- test_ok_ "$descr"
- else
- say_color "" "# test_external test $descr was ok"
- test_success=$(($test_success + 1))
- fi
- else
- if test $test_external_has_tap -eq 0; then
- test_failure_ "$descr" "$@"
- else
- say_color error "# test_external test $descr failed: $@"
- test_failure=$(($test_failure + 1))
- fi
- fi
- fi
-}
-
-# Like test_external, but in addition tests that the command generated
-# no output on stderr.
-test_external_without_stderr () {
- # The temporary file has no (and must have no) security
- # implications.
- tmp=${TMPDIR:-/tmp}
- stderr="$tmp/git-external-stderr.$$.tmp"
- test_external "$@" 4> "$stderr"
- [ -f "$stderr" ] || error "Internal error: $stderr disappeared."
- descr="no stderr: $1"
- shift
- say >&3 "# expecting no stderr from previous command"
- if [ ! -s "$stderr" ]; then
- rm "$stderr"
-
- if test $test_external_has_tap -eq 0; then
- test_ok_ "$descr"
- else
- say_color "" "# test_external_without_stderr test $descr was ok"
- test_success=$(($test_success + 1))
- fi
- else
- if [ "$verbose" = t ]; then
- output=`echo; echo "# Stderr is:"; cat "$stderr"`
- else
- output=
- fi
- # rm first in case test_failure exits.
- rm "$stderr"
- if test $test_external_has_tap -eq 0; then
- test_failure_ "$descr" "$@" "$output"
- else
- say_color error "# test_external_without_stderr test $descr failed: $@: $output"
- test_failure=$(($test_failure + 1))
- fi
- fi
-}
-
-# debugging-friendly alternatives to "test [-f|-d|-e]"
-# The commands test the existence or non-existence of $1. $2 can be
-# given to provide a more precise diagnosis.
-test_path_is_file () {
- if ! [ -f "$1" ]
- then
- echo "File $1 doesn't exist. $*"
- false
- fi
-}
-
-test_path_is_dir () {
- if ! [ -d "$1" ]
- then
- echo "Directory $1 doesn't exist. $*"
- false
- fi
-}
-
-test_path_is_missing () {
- if [ -e "$1" ]
- then
- echo "Path exists:"
- ls -ld "$1"
- if [ $# -ge 1 ]; then
- echo "$*"
- fi
- false
- fi
-}
-
-# test_line_count checks that a file has the number of lines it
-# ought to. For example:
-#
-# test_expect_success 'produce exactly one line of output' '
-# do something >output &&
-# test_line_count = 1 output
-# '
-#
-# is like "test $(wc -l <output) = 1" except that it passes the
-# output through when the number of lines is wrong.
-
-test_line_count () {
- if test $# != 3
- then
- error "bug in the test script: not 3 parameters to test_line_count"
- elif ! test $(wc -l <"$3") "$1" "$2"
- then
- echo "test_line_count: line count for $3 !$1 $2"
- cat "$3"
- return 1
- fi
-}
-
-# This is not among top-level (test_expect_success | test_expect_failure)
-# but is a prefix that can be used in the test script, like:
-#
-# test_expect_success 'complain and die' '
-# do something &&
-# do something else &&
-# test_must_fail git checkout ../outerspace
-# '
-#
-# Writing this as "! git checkout ../outerspace" is wrong, because
-# the failure could be due to a segv. We want a controlled failure.
-
-test_must_fail () {
- "$@"
- exit_code=$?
- if test $exit_code = 0; then
- echo >&2 "test_must_fail: command succeeded: $*"
- return 1
- elif test $exit_code -gt 129 -a $exit_code -le 192; then
- echo >&2 "test_must_fail: died by signal: $*"
- return 1
- elif test $exit_code = 127; then
- echo >&2 "test_must_fail: command not found: $*"
- return 1
- fi
- return 0
-}
-
-# Similar to test_must_fail, but tolerates success, too. This is
-# meant to be used in contexts like:
-#
-# test_expect_success 'some command works without configuration' '
-# test_might_fail git config --unset all.configuration &&
-# do something
-# '
-#
-# Writing "git config --unset all.configuration || :" would be wrong,
-# because we want to notice if it fails due to segv.
-
-test_might_fail () {
- "$@"
- exit_code=$?
- if test $exit_code -gt 129 -a $exit_code -le 192; then
- echo >&2 "test_might_fail: died by signal: $*"
- return 1
- elif test $exit_code = 127; then
- echo >&2 "test_might_fail: command not found: $*"
- return 1
- fi
- return 0
-}
-
-# Similar to test_must_fail and test_might_fail, but check that a
-# given command exited with a given exit code. Meant to be used as:
-#
-# test_expect_success 'Merge with d/f conflicts' '
-# test_expect_code 1 git merge "merge msg" B master
-# '
-
-test_expect_code () {
- want_code=$1
- shift
- "$@"
- exit_code=$?
- if test $exit_code = $want_code
- then
- return 0
- fi
-
- echo >&2 "test_expect_code: command exited with $exit_code, we wanted $want_code $*"
- return 1
-}
-
-# test_cmp is a helper function to compare actual and expected output.
-# You can use it like:
-#
-# test_expect_success 'foo works' '
-# echo expected >expected &&
-# foo >actual &&
-# test_cmp expected actual
-# '
-#
-# This could be written as either "cmp" or "diff -u", but:
-# - cmp's output is not nearly as easy to read as diff -u
-# - not all diff versions understand "-u"
-
-test_cmp() {
- $GIT_TEST_CMP "$@"
-}
-
-# This function can be used to schedule some commands to be run
-# unconditionally at the end of the test to restore sanity:
-#
-# test_expect_success 'test core.capslock' '
-# git config core.capslock true &&
-# test_when_finished "git config --unset core.capslock" &&
-# hello world
-# '
-#
-# That would be roughly equivalent to
-#
-# test_expect_success 'test core.capslock' '
-# git config core.capslock true &&
-# hello world
-# git config --unset core.capslock
-# '
-#
-# except that the greeting and config --unset must both succeed for
-# the test to pass.
-#
-# Note that under --immediate mode, no clean-up is done to help diagnose
-# what went wrong.
-
-test_when_finished () {
- test_cleanup="{ $*
- } && (exit \"\$eval_ret\"); eval_ret=\$?; $test_cleanup"
-}
-
-# Most tests can use the created repository, but some may need to create more.
-# Usage: test_create_repo <directory>
-test_create_repo () {
- test "$#" = 1 ||
- error "bug in the test script: not 1 parameter to test-create-repo"
- repo="$1"
- mkdir -p "$repo"
- (
- cd "$repo" || error "Cannot setup test environment"
- "$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
- error "cannot run git init -- have you built things yet?"
- mv .git/hooks .git/hooks-disabled
- ) || exit
}
test_done () {
--
1.7.8.304.ge42e4
^ permalink raw reply related
* [RFC PATCH 0/4] Performance test framework
From: Thomas Rast @ 2011-12-14 15:23 UTC (permalink / raw)
To: git; +Cc: Michael Haggerty
Hi,
This is the first shot at a usable performance test suite. It's
another angle than the refperf work of Michael Haggerty, however he
helped shape this in #git-devel (thanks!).
There's a big README, but if you just want to dive in, here's me
comparing the grep performance of my POC "pack reading in parallel"
branch against Junio's next:
$ cd t/perf
$ ./run origin/next t/sha1_file-parallel p7810-grep.sh
[snip no-op compilation]
=== Running 1 tests in build/7a6d658aa3c9016dd04ff3515cbf15edca6562a4 ===
perf 1 - grep worktree, cheap regex: 1 2 3 4 5 ok
perf 2 - grep worktree, expensive regex: 1 2 3 4 5 ok
perf 3 - grep --cached, cheap regex: 1 2 3 4 5 ok
perf 4 - grep --cached, expensive regex: 1 2 3 4 5 ok
# passed all 4 test(s)
1..4
GIT_VERSION = 1.7.8.GIT
* new build flags or prefix
* new link flags
GEN common-cmds.h
CC hex.o
CC kwset.o
[snip rest of compilation]
=== Running 1 tests in build/dd2bf650b382f5aca727b7d93a48598fb1a2f7d9 ===
perf 1 - grep worktree, cheap regex: 1 2 3 4 5 ok
perf 2 - grep worktree, expensive regex: 1 2 3 4 5 ok
perf 3 - grep --cached, cheap regex: 1 2 3 4 5 ok
perf 4 - grep --cached, expensive regex: 1 2 3 4 5 ok
# passed all 4 test(s)
1..4
Test origin/next t/sha1_file-parallel
----------------------------------------------------------------------------------
7810.1: grep worktree, cheap regex 0.16(0.16+0.35) 0.16(0.15+0.36) +0.0%
7810.2: grep worktree, expensive regex 7.83(29.68+0.39) 7.95(29.98+0.39) +1.5%
7810.3: grep --cached, cheap regex 3.12(3.11+0.24) 1.11(3.46+0.18) -64.4%
7810.4: grep --cached, expensive regex 9.43(30.53+0.28) 8.89(32.99+0.22) -5.7%
Note in particular that neither of the two branches contains the perf
work.
Have fun!
Thomas Rast (4):
Move the user-facing test library to test-lib-functions.sh
test-lib: allow testing another git build tree
Introduce a performance testing framework
Add a performance test for git-grep
Makefile | 26 ++-
t/Makefile | 5 +-
t/perf/.gitignore | 2 +
t/perf/Makefile | 15 +
t/perf/README | 146 ++++++++++
t/perf/aggregate.perl | 166 ++++++++++++
t/perf/min_time.perl | 21 ++
t/perf/p0000-perf-lib-sanity.sh | 41 +++
t/perf/p0001-rev-list.sh | 17 ++
t/perf/p7810-grep.sh | 23 ++
t/perf/perf-lib.sh | 199 ++++++++++++++
t/perf/run | 82 ++++++
t/test-lib-functions.sh | 528 ++++++++++++++++++++++++++++++++++++
t/test-lib.sh | 561 +++------------------------------------
14 files changed, 1300 insertions(+), 532 deletions(-)
create mode 100644 t/perf/.gitignore
create mode 100644 t/perf/Makefile
create mode 100644 t/perf/README
create mode 100755 t/perf/aggregate.perl
create mode 100755 t/perf/min_time.perl
create mode 100755 t/perf/p0000-perf-lib-sanity.sh
create mode 100755 t/perf/p0001-rev-list.sh
create mode 100755 t/perf/p7810-grep.sh
create mode 100644 t/perf/perf-lib.sh
create mode 100755 t/perf/run
create mode 100644 t/test-lib-functions.sh
--
1.7.8.304.ge42e4
^ permalink raw reply
* Re: Auto update submodules after merge and reset
From: Marc Branchaud @ 2011-12-14 15:16 UTC (permalink / raw)
To: Andreas T.Auer; +Cc: Jens Lehmann, Phil Hord, git
In-Reply-To: <4EE7DA2C.7000300@ursus.ath.cx>
On 11-12-13 06:05 PM, Andreas T.Auer wrote:
>
> On 13.12.2011 22:44 Jens Lehmann wrote:
>>
>> If you follow a tip there won't be any new SHA-1s recorded during
>> that following so you could not do a bisect and expect the submodule
>> to be what the developer had when doing the commits, no?
>
> If you never commit something to the superproject, you wouldn't get SHA1s
> recorded, that's right. But when you commit something to the superproject,
> why shouldn't the current submodule SHA1 be stored? Floating is about
> _ignoring_ the recorded SHA1 in _some_ cases, not about disabling the
> recording. So you can bisect to the bad superproject commit. If you suspect a
> bad submodule commit causing the problem then you could still bisect the
> submodule commits between the recorded SHA1s.
To me this question is one of the more problematic aspects of floating
submodules: When to commit submodule SHA1s.
Andreas suggests always committing them whenever a submodule's tip is moved.
I don't think this is practical because commits end up with extra changes
that likely have nothing to do with the commit's topic. This makes it
difficult to merge (or even cherry-pick) a commit elsewhere without also
picking up an unwanted submodule update.
Jens suggests never committing submodule SHA1s. This makes it difficult to
restore the submodules to the state they were in when a super-repo commit was
made. To me this is unacceptable -- if commits don't encompass the entire
state of the repo (including its submodules) then they're pretty much
useless. Well, maybe commits themselves don't need to do that, but I sure
need some way to restore the entire state of the repo.
I'm not sure there's a good or easy answer here. I suspect that it should
always be up to the user whether or not submodule changes are included in a
commit. The way git currently works, that means that a floating checkout
would always have a modified status when in fact it isn't really dirty but
merely smudged.
Wish I could propose a solution, but all I have are questions!
M.
^ permalink raw reply
* Re: t0090-cache-tree fails due to wc whitespace
From: Hallvard Breien Furuseth @ 2011-12-14 15:09 UTC (permalink / raw)
To: Brian Gernhardt; +Cc: Git List, Thomas Rast
In-Reply-To: <7F1792D2-8ED4-4546-8ED4-52B95E0AE9FC@silverinsanity.com>
Brian Gernhardt writes:
> I was able to fix this by adding a sed command to remove leading spaces:
>
> - "($(git ls-files|wc -l) entries, 0 subtrees)" >expect &&
> + "($(git ls-files|wc -l|sed -e 's/^ *//') entries, 0 subtrees)" >expect &&
>
> But I'm not sure if this is the best way to solve the issue.
Well, tr -d ' ' saves all of 7 characters from sed -e 's/^ *//'.
--
Hallvard
^ permalink raw reply
* Re: t0090-cache-tree fails due to wc whitespace
From: Stefano Lattarini @ 2011-12-14 14:57 UTC (permalink / raw)
To: Brian Gernhardt; +Cc: Git List, Thomas Rast
In-Reply-To: <7F1792D2-8ED4-4546-8ED4-52B95E0AE9FC@silverinsanity.com>
On Wednesday 14 December 2011, Brian Gernhardt wrote:
> It's time for my periodic complaint: People assuming `wc -l`
> outputs just a number. wc on OS X (and perhaps other BSD-like
> systems) always aligns the output in columns, even with the -l
> flag.
>
It surely does so on Solaris 10 as well:
$ echo x | wc -l
1
$ for i in {1..1000}; do echo x; done | wc -l
1000
Regards,
Stefano
^ permalink raw reply
* Re: [PATCH 3/7] revert: pass around rev-list args in already-parsed form
From: Ramkumar Ramachandra @ 2011-12-14 14:51 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Johannes Sixt, Junio C Hamano, git, Christian Couder,
Martin von Zweigbergk, Jay Soffian
In-Reply-To: <20111210125825.GD22035@elie.hsd1.il.comcast.net>
Hey,
Jonathan Nieder wrote:
> [...]
> Original patch by Jonathan, tweaks and test from Ram.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Improved-by: Ramkumar Ramachandra <artagnon@gmail.com>
I've already seen this, so let's see how it fits into the rest of the series.
-- Ram
^ permalink raw reply
* t0090-cache-tree fails due to wc whitespace
From: Brian Gernhardt @ 2011-12-14 14:35 UTC (permalink / raw)
To: Git List; +Cc: Thomas Rast
It's time for my periodic complaint: People assuming `wc -l` outputs just a number. wc on OS X (and perhaps other BSD-like systems) always aligns the output in columns, even with the -l flag. Generally this results in a quick patch from me to remove some unneeded quotes. However, this time it's used in a more complex manner:
echo "SHA " \
"($(git ls-files|wc -l) entries, 0 subtrees)" >expect &&
cmp_cache_tree expect
This results in errors like:
--- expect 2011-12-14 14:26:26.000000000 +0000
+++ filtered 2011-12-14 14:26:26.000000000 +0000
@@ -1 +1 @@
-SHA ( 1 entries, 0 subtrees)
+SHA (1 entries, 0 subtrees)
I was able to fix this by adding a sed command to remove leading spaces:
- "($(git ls-files|wc -l) entries, 0 subtrees)" >expect &&
+ "($(git ls-files|wc -l|sed -e 's/^ *//') entries, 0 subtrees)" >expect &&
But I'm not sure if this is the best way to solve the issue.
~~ Brian Gernhardt
^ permalink raw reply
* Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES
From: Frans Klaver @ 2011-12-14 14:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vliqguwhq.fsf@alter.siamese.dyndns.org>
On Tue, Dec 13, 2011 at 8:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> - That "have_..._ions()" is too long and ugly.
I half expected that one and I agree. I vaguely remember typing it,
deleting it and typing it again when I started on that one.
>
> - The only thing you care about this callsite is if you have enough
> permission to execute the "cmd".
>
> In fact, you should not unconditionally require read permissions here.
>
> $ chmod a-r $(type --path git) && /bin/ls -l $(type --path git)
> --wx--x--x 109 junio junio 5126580 Dec 13 09:47 /home/junio/git-active/bin/git
> $ /home/junio/git-active/bin/git --version
> git version 1.7.8.249.gb1b73
>
> You may need read permission when the file is a script (i.e. not binary
> executable).
[...]
> When checking if you can run "foo/bar/baz", directories "foo/" and "foo/bar/"
> do not have to be readable. They only have to have executable bit to allow
> descending into them, and typically this is called "searchable" (see man chmod).
>
> $ mkdir -p /var/tmp/a/b && cp $(type --path git) /var/tmp/a/b/git
> $ chmod 111 /var/tmp/a /var/tmp/a/b
> $ /var/tmp/a/b/git --version
> git version 1.7.8.249.gb1b73
>
> I'd suggest having two helper functions, instead of the single one with
> overlong "have...ions" name.
>
> - can_search_directory() checks with access(X_OK);
>
> - can_execute_file() checks with access(X_OK|R_OK), even though R_OK is
> not always needed.
On the whole I like the suggestion. We should probably take it a bit
further. Since the x and r bits basically have nothing to do with each
other, and we need +rx only on scripts, I could just rely on fopen()
for the +r check. I will still add the can_execute_file() and
can_search_dir() helpers to support readability, as access(path, X_OK)
means different things in the different contexts. I would then
probably go for is_searchable() and is_executable() as function names.
is_executable then means "is file and has executable flag set",
is_searchable means "is directory and has executable flag set".
Basically files won't be searchable and directories won't be
executable. If execvp fails on a command that is executable, but not
readable, it is definitely a script and we can generate an error in
that case. 1/2 would then probably use access(path, R_OK), while 2/2
would start using fopen.
Since fopen() uses the effective uid/gid, it then makes sense to use
eaccess(3) instead of access(2) if available. It would be stupid to
have bugs arise just because of a mismatch between the [ug]ids used by
the two access checks. I'm aware of the fact that eaccess isn't a
standard function, so a #define HAVE... fallback to at least access()
would probably be required.
>
> Use the former here where you check the directory that contains the
> command, and use the latter up above where you check the command that is
> supposed to be executable, and also down below after you checked sb.buf is
> a path to a file that may be the command that is supposed to be
> executable.
>
> Then patch 2/2 can extend can_execute() to enhance its support for scripts
> by reading the hash-bang line and validating it, etc.
I'd rather keep the hash-bang check outside of that function and use
can_execute/is_executable for checking the interpreter as well, if
only for keeping the possibility of easily promoting them into an API.
I'd rather move check_interpreter into where it's called now, but pull
out the logic to find the interpreter. This will keep the error text
generation in diagnose_execvp_eacces. I think the code will make more
sense this way. There's tons of more errors that can be caused by a
faulty interpreter, and it'll be easier to cover more cases this way
in the future.
Thanks for the insightful reviews so far.
Let me know what you think,
Frans
^ permalink raw reply
* Re: [PATCH 2/7] revert: allow cherry-pick --continue to commit before resuming
From: Ramkumar Ramachandra @ 2011-12-14 14:26 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Johannes Sixt, Junio C Hamano, git, Christian Couder,
Martin von Zweigbergk, Jay Soffian
In-Reply-To: <20111210124925.GC22035@elie.hsd1.il.comcast.net>
Hi again,
Jonathan Nieder wrote:
> When "git cherry-pick ..bar" encounters conflicts, permit the operator
> to use cherry-pick --continue after resolving them as a shortcut for
> "git commit && git cherry-pick --continue" to record the resolution
> and carry on with the rest of the sequence.
Sounds good. I remember my implementation being quite complicated;
let's see how you've done this.
> Example: after encountering a conflict from running "git cherry-pick
> foo bar baz":
>
> CONFLICT (content): Merge conflict in main.c
> error: could not apply f78a8d98c... bar!
> hint: after resolving the conflicts, mark the corrected paths
> hint: with 'git add <paths>' or 'git rm <paths>'
> hint: and commit the result with 'git commit'
>
> We edit main.c to resolve the conflict, mark it acceptable with "git
> add main.c", and can run "cherry-pick --continue" to resume the
> sequence.
>
> $ git cherry-pick --continue
> [editor opens to confirm commit message]
> [master 78c8a8c98] bar!
> 1 files changed, 1 insertions(+), 1 deletions(-)
> [master 87ca8798c] baz!
> 1 files changed, 1 insertions(+), 1 deletions(-)
I like the presentation of this example: much clearer than my examples.
> builtin/revert.c | 23 ++++++-
> t/t3510-cherry-pick-sequence.sh | 139 ++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 156 insertions(+), 6 deletions(-)
Oh, good -- lots of new tests :)
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 9f6c85c1..a43b4d85 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -1038,18 +1038,35 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
> return 0;
> }
>
> +static int continue_single_pick(void)
> +{
> + const char *argv[] = { "commit", NULL };
> +
> + if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
> + !file_exists(git_path("REVERT_HEAD")))
> + return error(_("no cherry-pick or revert in progress"));
> + return run_command_v_opt(argv, RUN_GIT_CMD);
> +}
Very nice! I can see how the introduction of REVERT_HEAD simplifies things :)
I'm totally embarrassed by the horribly convoluted logic in the "New
sequencer workflow!" I posted earlier.
Note to self: don't capitalize error() messages.
> static int sequencer_continue(struct replay_opts *opts)
> {
> struct commit_list *todo_list = NULL;
>
> if (!file_exists(git_path(SEQ_TODO_FILE)))
> - return error(_("No %s in progress"), action_name(opts));
> + return continue_single_pick();
> read_populate_opts(&opts);
> read_populate_todo(&todo_list, opts);
>
> /* Verify that the conflict has been resolved */
> - if (!index_differs_from("HEAD", 0))
> - todo_list = todo_list->next;
> + if (file_exists(git_path("CHERRY_PICK_HEAD")) ||
> + file_exists(git_path("REVERT_HEAD"))) {
> + int ret = continue_single_pick();
> + if (ret)
> + return ret;
> + }
> + if (index_differs_from("HEAD", 0))
> + return error_dirty_index(opts);
> + todo_list = todo_list->next;
> return pick_commits(todo_list, opts);
> }
Very nicely done. I can see why 1/7 makes so much sense now: it
helps think of different operations independently.
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index 2c4c1c85..4d1883b7 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -2,6 +2,7 @@
>
> test_description='Test cherry-pick continuation features
>
> + + conflicting: rewrites unrelated to conflicting
> + yetanotherpick: rewrites foo to e
> + anotherpick: rewrites foo to d
> + picked: rewrites foo to c
Note to self: this list of commits is becoming quite unwieldy. We
should probably refactor these sometime.
> @@ -27,6 +28,7 @@ test_cmp_rev () {
> }
>
> test_expect_success setup '
> + git config advice.detachedhead false
> echo unrelated >unrelated &&
> git add unrelated &&
> test_commit initial foo a &&
Huh, why are you moving this line up? Oh, right: there are
"test_commit" statements in the setup- good catch. This is unrelated
to your patch and should be a separate commit though.
> @@ -35,8 +37,8 @@ test_expect_success setup '
> test_commit picked foo c &&
> test_commit anotherpick foo d &&
> test_commit yetanotherpick foo e &&
> - git config advice.detachedhead false
> -
> + pristine_detach initial &&
> + test_commit conflicting unrelated
> '
Looks fishy- I wonder why you're doing this. Let's read ahead and find out.
> @@ -243,7 +245,66 @@ test_expect_success '--continue complains when there are unresolved conflicts' '
> test_must_fail git cherry-pick --continue
> '
>
> -test_expect_success '--continue continues after conflicts are resolved' '
> +test_expect_success '--continue of single cherry-pick' '
> + pristine_detach initial &&
> + echo c >expect &&
> + test_must_fail git cherry-pick picked &&
> + echo c >foo &&
> + git add foo &&
> + git cherry-pick --continue &&
> +
> + test_cmp expect foo &&
> + test_cmp_rev initial HEAD^ &&
> + git diff --exit-code HEAD &&
> + test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
> +'
Beautiful. The tests I wrote are ugly in comparison :\
> +test_expect_success '--continue of single revert' '
> + pristine_detach initial &&
> + echo resolved >expect &&
> + echo "Revert \"picked\"" >expect.msg &&
> + test_must_fail git revert picked &&
> + echo resolved >foo &&
> + git add foo &&
> + git cherry-pick --continue &&
Huh? You're continuing a "git revert" with a a "git cherry-pick
--continue"? The current 'master' still uses a commit_list, and
doesn't allow mixed "pick" and "revert" instructions yet.
> + git diff --exit-code HEAD &&
> + test_cmp expect foo &&
> + test_cmp_rev initial HEAD^ &&
> + git diff-tree -s --pretty=tformat:%s HEAD >msg &&
> + test_cmp expect.msg msg &&
> + test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
> + test_must_fail git rev-parse --verify REVERT_HEAD
> +'
A couple of notes:
1. I haven't used the "-s" flag of "git diff-tree" before, so I opened
up the documentation to find this:
By default, 'git diff-tree --stdin' shows differences,
either in machine-readable form (without '-p') or in patch
form (with '-p'). This output can be suppressed. It is
only useful with '-v' flag.
Very misleading. TODO: Fix this.
2. Why did you use "diff-tree" to get the commit message? Isn't
"cat-file commit" much more straightforward?
> +test_expect_success '--continue after resolving conflicts' '
> + pristine_detach initial &&
> + echo d >expect &&
> + cat >expect.log <<-\EOF &&
> + OBJID
> + :100644 100644 OBJID OBJID M foo
> + OBJID
> + :100644 100644 OBJID OBJID M foo
> + OBJID
> + :100644 100644 OBJID OBJID M unrelated
> + OBJID
> + :000000 100644 OBJID OBJID A foo
> + :000000 100644 OBJID OBJID A unrelated
> + EOF
> + test_must_fail git cherry-pick base..anotherpick &&
> + echo c >foo &&
> + git add foo &&
> + git cherry-pick --continue &&
> + {
> + git rev-list HEAD |
> + git diff-tree --root --stdin |
> + sed "s/$_x40/OBJID/g"
> + } >actual.log &&
> + test_cmp expect foo &&
> + test_cmp expect.log actual.log
> +'
Unchanged from the original: I suspect you've moved the generation of
expectation messages up to produce a clean diff.
> +test_expect_success '--continue after resolving conflicts and committing' '
> pristine_detach initial &&
> test_must_fail git cherry-pick base..anotherpick &&
> echo "c" >foo &&
Okay, the diff isn't all that clean :P
> +test_expect_success '--continue asks for help after resolving patch to nil' '
> + pristine_detach conflicting &&
> + test_must_fail git cherry-pick initial..picked &&
> +
> + test_cmp_rev unrelatedpick CHERRY_PICK_HEAD &&
> + git checkout HEAD -- unrelated &&
> + test_must_fail git cherry-pick --continue 2>msg &&
> + test_i18ngrep "The previous cherry-pick is now empty" msg
> +'
I thought it was a bad idea to grep for specific output messages,
because of their volatile nature? Remind me what this test has to do
with the rest of your patch?
> +test_expect_failure 'follow advice and skip nil patch' '
> + pristine_detach conflicting &&
> + test_must_fail git cherry-pick initial..picked &&
> +
> + git checkout HEAD -- unrelated &&
> + test_must_fail git cherry-pick --continue &&
> + git reset &&
> + git cherry-pick --continue &&
> +
> + git rev-list initial..HEAD >commits &&
> + test_line_count = 3 commits
> +'
Again, what does this test have to do with the rest of your patch?
> test_expect_success '--continue respects opts' '
> pristine_detach initial &&
> test_must_fail git cherry-pick -x base..anotherpick &&
> @@ -288,6 +372,29 @@ test_expect_success '--continue respects opts' '
> grep "cherry picked from" anotherpick_msg
> '
>
> +test_expect_success '--continue of single-pick respects -x' '
> + pristine_detach initial &&
> + test_must_fail git cherry-pick -x picked &&
> + echo c >foo &&
> + git add foo &&
> + git cherry-pick --continue &&
> + test_path_is_missing .git/sequencer &&
> + git cat-file commit HEAD >msg &&
> + grep "cherry picked from" msg
> +'
I'd have liked s/respects -x/respects opts/ here for symmetry with the
previous test.
> +test_expect_success '--continue respects -x in first commit in multi-pick' '
> + pristine_detach initial &&
> + test_must_fail git cherry-pick -x picked anotherpick &&
> + echo c >foo &&
> + git add foo &&
> + git cherry-pick --continue &&
> + test_path_is_missing .git/sequencer &&
> + git cat-file commit HEAD^ >msg &&
> + picked=$(git rev-parse --verify picked) &&
> + grep "cherry picked from.*$picked" msg
> +'
Can you explain why "first commit in a multi-pick" is a special case?
> @@ -306,6 +413,32 @@ test_expect_success '--signoff is not automatically propagated to resolved confl
> grep "Signed-off-by:" anotherpick_msg
> '
>
> +test_expect_success '--signoff dropped for implicit commit of resolution, multi-pick case' '
> + pristine_detach initial &&
> + test_must_fail git cherry-pick -s picked anotherpick &&
> + echo c >foo &&
> + git add foo &&
> + git cherry-pick --continue &&
> +
> + git diff --exit-code HEAD &&
> + test_cmp_rev initial HEAD^^ &&
> + git cat-file commit HEAD^ >msg &&
> + ! grep Signed-off-by: msg
> +'
Unrelated.
> +test_expect_success 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '
> + pristine_detach initial &&
> + test_must_fail git cherry-pick -s picked &&
> + echo c >foo &&
> + git add foo &&
> + git cherry-pick --continue &&
> +
> + git diff --exit-code HEAD &&
> + test_cmp_rev initial HEAD^ &&
> + git cat-file commit HEAD >msg &&
> + ! grep Signed-off-by: msg
> +'
If the previous test were a separate patch preceding this one, this'd
belong here.
Thanks for working on this.
-- Ram
^ permalink raw reply
* [PATCH 3/3] Do not create commits whose message contains NUL
From: Nguyễn Thái Ngọc Duy @ 2011-12-14 14:08 UTC (permalink / raw)
To: git; +Cc: Jeff King, Miles Bader, Nguyễn Thái Ngọc Duy
In-Reply-To: <1323871699-8839-1-git-send-email-pclouds@gmail.com>
We assume that the commit log messages are uninterpreted sequences of
non-NUL bytes (see Documentation/i18n.txt). However the assumption
does not really stand out and it's quite easy to set an editor to save
in a NUL-included encoding. Currently we silently cut at the first NUL
we see.
Make it more obvious that NUL is not welcome by refusing to create
such commits. Those who deliberately want to create them can still do
with hash-object.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/config.txt | 4 ++++
advice.c | 2 ++
advice.h | 1 +
commit.c | 9 +++++++++
t/t3900-i18n-commit.sh | 6 ++++++
t/t3900/UTF-16.txt | Bin 0 -> 32 bytes
6 files changed, 22 insertions(+), 0 deletions(-)
create mode 100644 t/t3900/UTF-16.txt
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5a841da..daf57c2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -144,6 +144,10 @@ advice.*::
Advice shown when you used linkgit::git-checkout[1] to
move to the detach HEAD state, to instruct how to create
a local branch after the fact. Default: true.
+ commitWideEncoding::
+ Advice shown when linkgit::git-commit[1] refuses to
+ proceed because there are NULs in commit message.
+ Default: true.
--
core.fileMode::
diff --git a/advice.c b/advice.c
index e02e632..130949e 100644
--- a/advice.c
+++ b/advice.c
@@ -6,6 +6,7 @@ int advice_commit_before_merge = 1;
int advice_resolve_conflict = 1;
int advice_implicit_identity = 1;
int advice_detached_head = 1;
+int advice_commmit_wide_encoding = 1;
static struct {
const char *name;
@@ -17,6 +18,7 @@ static struct {
{ "resolveconflict", &advice_resolve_conflict },
{ "implicitidentity", &advice_implicit_identity },
{ "detachedhead", &advice_detached_head },
+ { "commitwideencoding", &advice_commmit_wide_encoding },
};
void advise(const char *advice, ...)
diff --git a/advice.h b/advice.h
index e5d0af7..d913bdb 100644
--- a/advice.h
+++ b/advice.h
@@ -9,6 +9,7 @@ extern int advice_commit_before_merge;
extern int advice_resolve_conflict;
extern int advice_implicit_identity;
extern int advice_detached_head;
+extern int advice_commmit_wide_encoding;
int git_default_advice_config(const char *var, const char *value);
void advise(const char *advice, ...);
diff --git a/commit.c b/commit.c
index d67b8c7..59e5bce 100644
--- a/commit.c
+++ b/commit.c
@@ -855,6 +855,15 @@ int commit_tree(const char *msg, size_t msg_len, unsigned char *tree,
assert_sha1_type(tree, OBJ_TREE);
+ if (memchr(msg, '\0', msg_len)) {
+ error(_("your commit message contains NUL characters."));
+ if (advice_commmit_wide_encoding) {
+ advise(_("This is often caused by using wide encodings such as"));
+ advise(_("UTF-16. Please check your editor settings."));
+ }
+ return -1;
+ }
+
/* Not having i18n.commitencoding is the same as having utf-8 */
encoding_is_utf8 = is_encoding_utf8(git_commit_encoding);
diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index 1f62c15..d48a7c0 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -34,6 +34,12 @@ test_expect_success 'no encoding header for base case' '
test z = "z$E"
'
+test_expect_failure 'UTF-16 refused because of NULs' '
+ echo UTF-16 >F &&
+ git commit -a -F "$TEST_DIRECTORY"/t3900/UTF-16.txt
+'
+
+
for H in ISO8859-1 eucJP ISO-2022-JP
do
test_expect_success "$H setup" '
diff --git a/t/t3900/UTF-16.txt b/t/t3900/UTF-16.txt
new file mode 100644
index 0000000000000000000000000000000000000000..53296be684253f40964c0604be7fa7ff12e200cb
GIT binary patch
literal 32
mcmezOpWz6@X@-jo=NYasZ~@^#h9rjP3@HpR7}6Nh8Mpw;r3yp<
literal 0
HcmV?d00001
--
1.7.8.36.g69ee2
^ permalink raw reply related
* [PATCH 2/3] merge: abort if fails to commit
From: Nguyễn Thái Ngọc Duy @ 2011-12-14 14:08 UTC (permalink / raw)
To: git; +Cc: Jeff King, Miles Bader, Nguyễn Thái Ngọc Duy
In-Reply-To: <1323871699-8839-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/merge.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index df4548a..e57eefa 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -913,7 +913,9 @@ static int merge_trivial(struct commit *head)
parent->next->item = remoteheads->item;
parent->next->next = NULL;
prepare_to_commit();
- commit_tree(merge_msg.buf, merge_msg.len, result_tree, parent, result_commit, NULL);
+ if (commit_tree(merge_msg.buf, merge_msg.len,
+ result_tree, parent, result_commit, NULL))
+ die(_("failed to write commit object"));
finish(head, result_commit, "In-index merge");
drop_save();
return 0;
@@ -944,7 +946,9 @@ static int finish_automerge(struct commit *head,
strbuf_addch(&merge_msg, '\n');
prepare_to_commit();
free_commit_list(remoteheads);
- commit_tree(merge_msg.buf, merge_msg.len, result_tree, parents, result_commit, NULL);
+ if (commit_tree(merge_msg.buf, merge_msg.len,
+ result_tree, parents, result_commit, NULL))
+ die(_("failed to write commit object"));
strbuf_addf(&buf, "Merge made by the '%s' strategy.", wt_strategy);
finish(head, result_commit, buf.buf);
strbuf_release(&buf);
--
1.7.8.36.g69ee2
^ permalink raw reply related
* [PATCH 1/3] Make commit_tree() take message length in addition to the commit message
From: Nguyễn Thái Ngọc Duy @ 2011-12-14 14:08 UTC (permalink / raw)
To: git; +Cc: Jeff King, Miles Bader, Nguyễn Thái Ngọc Duy
In-Reply-To: <1323871699-8839-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/commit-tree.c | 2 +-
builtin/commit.c | 2 +-
builtin/merge.c | 4 ++--
builtin/notes.c | 2 +-
commit.c | 4 ++--
commit.h | 2 +-
notes-cache.c | 2 +-
notes-merge.c | 8 ++++----
notes-merge.h | 2 +-
9 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index d083795..8fa384f 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -56,7 +56,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
if (strbuf_read(&buffer, 0, 0) < 0)
die_errno("git commit-tree: failed to read");
- if (commit_tree(buffer.buf, tree_sha1, parents, commit_sha1, NULL)) {
+ if (commit_tree(buffer.buf, buffer.len, tree_sha1, parents, commit_sha1, NULL)) {
strbuf_release(&buffer);
return 1;
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 8f2bebe..ce0e47f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1483,7 +1483,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
exit(1);
}
- if (commit_tree(sb.buf, active_cache_tree->sha1, parents, sha1,
+ if (commit_tree(sb.buf, sb.len, active_cache_tree->sha1, parents, sha1,
author_ident.buf)) {
rollback_index_files();
die(_("failed to write commit object"));
diff --git a/builtin/merge.c b/builtin/merge.c
index 2870a6a..df4548a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -913,7 +913,7 @@ static int merge_trivial(struct commit *head)
parent->next->item = remoteheads->item;
parent->next->next = NULL;
prepare_to_commit();
- commit_tree(merge_msg.buf, result_tree, parent, result_commit, NULL);
+ commit_tree(merge_msg.buf, merge_msg.len, result_tree, parent, result_commit, NULL);
finish(head, result_commit, "In-index merge");
drop_save();
return 0;
@@ -944,7 +944,7 @@ static int finish_automerge(struct commit *head,
strbuf_addch(&merge_msg, '\n');
prepare_to_commit();
free_commit_list(remoteheads);
- commit_tree(merge_msg.buf, result_tree, parents, result_commit, NULL);
+ commit_tree(merge_msg.buf, merge_msg.len, result_tree, parents, result_commit, NULL);
strbuf_addf(&buf, "Merge made by the '%s' strategy.", wt_strategy);
finish(head, result_commit, buf.buf);
strbuf_release(&buf);
diff --git a/builtin/notes.c b/builtin/notes.c
index f8e437d..d665459 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -306,7 +306,7 @@ void commit_notes(struct notes_tree *t, const char *msg)
if (buf.buf[buf.len - 1] != '\n')
strbuf_addch(&buf, '\n'); /* Make sure msg ends with newline */
- create_notes_commit(t, NULL, buf.buf + 7, commit_sha1);
+ create_notes_commit(t, NULL, buf.buf + 7, buf.len - 7, commit_sha1);
update_ref(buf.buf, t->ref, commit_sha1, NULL, 0, DIE_ON_ERR);
strbuf_release(&buf);
diff --git a/commit.c b/commit.c
index 73b7e00..d67b8c7 100644
--- a/commit.c
+++ b/commit.c
@@ -845,7 +845,7 @@ static const char commit_utf8_warn[] =
"You may want to amend it after fixing the message, or set the config\n"
"variable i18n.commitencoding to the encoding your project uses.\n";
-int commit_tree(const char *msg, unsigned char *tree,
+int commit_tree(const char *msg, size_t msg_len, unsigned char *tree,
struct commit_list *parents, unsigned char *ret,
const char *author)
{
@@ -884,7 +884,7 @@ int commit_tree(const char *msg, unsigned char *tree,
strbuf_addch(&buffer, '\n');
/* And add the comment */
- strbuf_addstr(&buffer, msg);
+ strbuf_add(&buffer, msg, msg_len);
/* And check the encoding */
if (encoding_is_utf8 && !is_utf8(buffer.buf))
diff --git a/commit.h b/commit.h
index 009b113..1acaf53 100644
--- a/commit.h
+++ b/commit.h
@@ -181,7 +181,7 @@ static inline int single_parent(struct commit *commit)
struct commit_list *reduce_heads(struct commit_list *heads);
-extern int commit_tree(const char *msg, unsigned char *tree,
+extern int commit_tree(const char *msg, size_t msg_len, unsigned char *tree,
struct commit_list *parents, unsigned char *ret,
const char *author);
diff --git a/notes-cache.c b/notes-cache.c
index 4c8984e..04a5698 100644
--- a/notes-cache.c
+++ b/notes-cache.c
@@ -56,7 +56,7 @@ int notes_cache_write(struct notes_cache *c)
if (write_notes_tree(&c->tree, tree_sha1))
return -1;
- if (commit_tree(c->validity, tree_sha1, NULL, commit_sha1, NULL) < 0)
+ if (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL, commit_sha1, NULL) < 0)
return -1;
if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL,
0, QUIET_ON_ERR) < 0)
diff --git a/notes-merge.c b/notes-merge.c
index ce10aac..b3baaf4 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -530,7 +530,7 @@ static int merge_from_diffs(struct notes_merge_options *o,
}
void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
- const char *msg, unsigned char *result_sha1)
+ const char *msg, size_t msg_len, unsigned char *result_sha1)
{
unsigned char tree_sha1[20];
@@ -551,7 +551,7 @@ void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
/* else: t->ref points to nothing, assume root/orphan commit */
}
- if (commit_tree(msg, tree_sha1, parents, result_sha1, NULL))
+ if (commit_tree(msg, msg_len, tree_sha1, parents, result_sha1, NULL))
die("Failed to commit notes tree to database");
}
@@ -669,7 +669,7 @@ int notes_merge(struct notes_merge_options *o,
commit_list_insert(remote, &parents); /* LIFO order */
commit_list_insert(local, &parents);
create_notes_commit(local_tree, parents, o->commit_msg.buf,
- result_sha1);
+ o->commit_msg.len, result_sha1);
}
found_result:
@@ -734,7 +734,7 @@ int notes_merge_commit(struct notes_merge_options *o,
}
create_notes_commit(partial_tree, partial_commit->parents, msg,
- result_sha1);
+ strlen(msg), result_sha1);
if (o->verbosity >= 4)
printf("Finalized notes merge commit: %s\n",
sha1_to_hex(result_sha1));
diff --git a/notes-merge.h b/notes-merge.h
index 168a672..fd52988 100644
--- a/notes-merge.h
+++ b/notes-merge.h
@@ -37,7 +37,7 @@ void init_notes_merge_options(struct notes_merge_options *o);
* The resulting commit SHA1 is stored in result_sha1.
*/
void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
- const char *msg, unsigned char *result_sha1);
+ const char *msg, size_t msg_len, unsigned char *result_sha1);
/*
* Merge notes from o->remote_ref into o->local_ref
--
1.7.8.36.g69ee2
^ permalink raw reply related
* [PATCH 0/3] git-commit rejects messages with NULs
From: Nguyễn Thái Ngọc Duy @ 2011-12-14 14:08 UTC (permalink / raw)
To: git; +Cc: Jeff King, Miles Bader, Nguyễn Thái Ngọc Duy
In-Reply-To: <1323777368-19697-1-git-send-email-pclouds@gmail.com>
Hi,
I'm sorry I forgot the patch that makes commit_tree() take message
length. This version rewords the error message and use memchr()
instead.
Nguyễn Thái Ngọc Duy (3):
Make commit_tree() take message length in addition to the commit
message
merge: abort if fails to commit
Do not create commits whose message contains NUL
Documentation/config.txt | 4 ++++
advice.c | 2 ++
advice.h | 1 +
builtin/commit-tree.c | 2 +-
builtin/commit.c | 2 +-
builtin/merge.c | 8 ++++++--
builtin/notes.c | 2 +-
commit.c | 13 +++++++++++--
commit.h | 2 +-
notes-cache.c | 2 +-
notes-merge.c | 8 ++++----
notes-merge.h | 2 +-
t/t3900-i18n-commit.sh | 6 ++++++
t/t3900/UTF-16.txt | Bin 0 -> 32 bytes
14 files changed, 40 insertions(+), 14 deletions(-)
create mode 100644 t/t3900/UTF-16.txt
--
1.7.8.36.g69ee2
^ permalink raw reply
* [PATCH v2 4/4] use wrapper for unchecked setenv/putenv calls
From: Erik Faye-Lund @ 2011-12-14 14:07 UTC (permalink / raw)
To: git; +Cc: peff, gitster, schwab
In-Reply-To: <1323871631-2872-1-git-send-email-kusmabite@gmail.com>
This avoids us from accidentally dropping state, possibly leading
to unexpected behaviour.
This is especially important on Windows, where the maximum size of
the environment is 32 kB.
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
builtin/clone.c | 2 +-
builtin/commit.c | 6 +++---
builtin/help.c | 4 ++--
builtin/init-db.c | 2 +-
builtin/merge.c | 4 ++--
builtin/notes.c | 2 +-
builtin/remote-ext.c | 4 ++--
builtin/revert.c | 2 +-
config.c | 2 +-
exec_cmd.c | 4 ++--
git.c | 18 +++++++++---------
| 2 +-
run-command.c | 2 +-
setup.c | 6 +++---
14 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index efe8b6c..8d81c29 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -566,7 +566,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
atexit(remove_junk);
sigchain_push_common(remove_junk_on_signal);
- setenv(CONFIG_ENVIRONMENT, mkpath("%s/config", git_dir), 1);
+ xsetenv(CONFIG_ENVIRONMENT, mkpath("%s/config", git_dir), 1);
if (safe_create_leading_directories_const(git_dir) < 0)
die(_("could not create leading directories of '%s'"), git_dir);
diff --git a/builtin/commit.c b/builtin/commit.c
index e36e9ad..2b87da9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -361,13 +361,13 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
die(_("unable to create temporary index"));
old_index_env = getenv(INDEX_ENVIRONMENT);
- setenv(INDEX_ENVIRONMENT, index_lock.filename, 1);
+ xsetenv(INDEX_ENVIRONMENT, index_lock.filename, 1);
if (interactive_add(argc, argv, prefix, patch_interactive) != 0)
die(_("interactive add failed"));
if (old_index_env && *old_index_env)
- setenv(INDEX_ENVIRONMENT, old_index_env, 1);
+ xsetenv(INDEX_ENVIRONMENT, old_index_env, 1);
else
unsetenv(INDEX_ENVIRONMENT);
@@ -1023,7 +1023,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
if (edit_flag)
use_editor = 1;
if (!use_editor)
- setenv("GIT_EDITOR", ":", 1);
+ xsetenv("GIT_EDITOR", ":", 1);
/* Sanity check options */
if (amend && !current_head)
diff --git a/builtin/help.c b/builtin/help.c
index 61ff798..e7dc15b 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -330,7 +330,7 @@ static void setup_man_path(void)
if (old_path)
strbuf_addstr(&new_path, old_path);
- setenv("MANPATH", new_path.buf, 1);
+ xsetenv("MANPATH", new_path.buf, 1);
strbuf_release(&new_path);
}
@@ -371,7 +371,7 @@ static void show_man_page(const char *git_cmd)
static void show_info_page(const char *git_cmd)
{
const char *page = cmd_to_page(git_cmd);
- setenv("INFOPATH", system_path(GIT_INFO_PATH), 1);
+ xsetenv("INFOPATH", system_path(GIT_INFO_PATH), 1);
execlp("info", "info", "gitman", page, (char *)NULL);
die("no info viewer handled the request");
}
diff --git a/builtin/init-db.c b/builtin/init-db.c
index d07554c..21ff09e 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -537,7 +537,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
if (is_bare_repository_cfg == 1) {
static char git_dir[PATH_MAX+1];
- setenv(GIT_DIR_ENVIRONMENT,
+ xsetenv(GIT_DIR_ENVIRONMENT,
getcwd(git_dir, sizeof(git_dir)), argc > 0);
}
diff --git a/builtin/merge.c b/builtin/merge.c
index a1c8534..a4ae473 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1257,7 +1257,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
strbuf_addstr(&buf, "merge");
for (i = 0; i < argc; i++)
strbuf_addf(&buf, " %s", argv[i]);
- setenv("GIT_REFLOG_ACTION", buf.buf, 0);
+ xsetenv("GIT_REFLOG_ACTION", buf.buf, 0);
strbuf_reset(&buf);
for (i = 0; i < argc; i++) {
@@ -1267,7 +1267,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
remotes = &commit_list_insert(commit, remotes)->next;
strbuf_addf(&buf, "GITHEAD_%s",
sha1_to_hex(commit->object.sha1));
- setenv(buf.buf, argv[i], 1);
+ xsetenv(buf.buf, argv[i], 1);
strbuf_reset(&buf);
if (merge_remote_util(commit) &&
merge_remote_util(commit)->obj &&
diff --git a/builtin/notes.c b/builtin/notes.c
index 10b8bc7..7b53c69 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -1076,7 +1076,7 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
struct strbuf sb = STRBUF_INIT;
strbuf_addstr(&sb, override_notes_ref);
expand_notes_ref(&sb);
- setenv("GIT_NOTES_REF", sb.buf, 1);
+ xsetenv("GIT_NOTES_REF", sb.buf, 1);
strbuf_release(&sb);
}
diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c
index 692c834..0b2169a 100644
--- a/builtin/remote-ext.c
+++ b/builtin/remote-ext.c
@@ -38,8 +38,8 @@ static char *strip_escapes(const char *str, const char *service,
psoff = 4;
/* Pass the service to command. */
- setenv("GIT_EXT_SERVICE", service, 1);
- setenv("GIT_EXT_SERVICE_NOPREFIX", service + psoff, 1);
+ xsetenv("GIT_EXT_SERVICE", service, 1);
+ xsetenv("GIT_EXT_SERVICE_NOPREFIX", service + psoff, 1);
/* Scan the length of argument. */
while (str[rpos] && (escape || str[rpos] != ' ')) {
diff --git a/builtin/revert.c b/builtin/revert.c
index 1ea525c..955a99f 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -1007,7 +1007,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
struct commit_list *cur;
int res;
- setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
+ xsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);
if (opts->allow_ff)
assert(!(opts->signoff || opts->no_commit ||
opts->record_origin || opts->edit));
diff --git a/config.c b/config.c
index 5ea101f..f461a62 100644
--- a/config.c
+++ b/config.c
@@ -43,7 +43,7 @@ void git_config_push_parameter(const char *text)
strbuf_addch(&env, ' ');
}
sq_quote_buf(&env, text);
- setenv(CONFIG_DATA_ENVIRONMENT, env.buf, 1);
+ xsetenv(CONFIG_DATA_ENVIRONMENT, env.buf, 1);
strbuf_release(&env);
}
diff --git a/exec_cmd.c b/exec_cmd.c
index 171e841..a5a92dd 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -63,7 +63,7 @@ void git_set_argv_exec_path(const char *exec_path)
/*
* Propagate this setting to external programs.
*/
- setenv(EXEC_PATH_ENVIRONMENT, exec_path, 1);
+ xsetenv(EXEC_PATH_ENVIRONMENT, exec_path, 1);
}
@@ -108,7 +108,7 @@ void setup_path(void)
else
strbuf_addstr(&new_path, _PATH_DEFPATH);
- setenv("PATH", new_path.buf, 1);
+ xsetenv("PATH", new_path.buf, 1);
strbuf_release(&new_path);
}
diff --git a/git.c b/git.c
index 8e34903..cb80de2 100644
--- a/git.c
+++ b/git.c
@@ -54,7 +54,7 @@ int check_pager_config(const char *cmd)
static void commit_pager_choice(void) {
switch (use_pager) {
case 0:
- setenv("GIT_PAGER", "cat", 1);
+ xsetenv("GIT_PAGER", "cat", 1);
break;
case 1:
setup_pager();
@@ -109,7 +109,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--no-replace-objects")) {
read_replace_refs = 0;
- setenv(NO_REPLACE_OBJECTS_ENVIRONMENT, "1", 1);
+ xsetenv(NO_REPLACE_OBJECTS_ENVIRONMENT, "1", 1);
if (envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--git-dir")) {
@@ -117,13 +117,13 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
fprintf(stderr, "No directory given for --git-dir.\n" );
usage(git_usage_string);
}
- setenv(GIT_DIR_ENVIRONMENT, (*argv)[1], 1);
+ xsetenv(GIT_DIR_ENVIRONMENT, (*argv)[1], 1);
if (envchanged)
*envchanged = 1;
(*argv)++;
(*argc)--;
} else if (!prefixcmp(cmd, "--git-dir=")) {
- setenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);
+ xsetenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);
if (envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--namespace")) {
@@ -131,13 +131,13 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
fprintf(stderr, "No namespace given for --namespace.\n" );
usage(git_usage_string);
}
- setenv(GIT_NAMESPACE_ENVIRONMENT, (*argv)[1], 1);
+ xsetenv(GIT_NAMESPACE_ENVIRONMENT, (*argv)[1], 1);
if (envchanged)
*envchanged = 1;
(*argv)++;
(*argc)--;
} else if (!prefixcmp(cmd, "--namespace=")) {
- setenv(GIT_NAMESPACE_ENVIRONMENT, cmd + 12, 1);
+ xsetenv(GIT_NAMESPACE_ENVIRONMENT, cmd + 12, 1);
if (envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--work-tree")) {
@@ -145,19 +145,19 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
fprintf(stderr, "No directory given for --work-tree.\n" );
usage(git_usage_string);
}
- setenv(GIT_WORK_TREE_ENVIRONMENT, (*argv)[1], 1);
+ xsetenv(GIT_WORK_TREE_ENVIRONMENT, (*argv)[1], 1);
if (envchanged)
*envchanged = 1;
(*argv)++;
(*argc)--;
} else if (!prefixcmp(cmd, "--work-tree=")) {
- setenv(GIT_WORK_TREE_ENVIRONMENT, cmd + 12, 1);
+ xsetenv(GIT_WORK_TREE_ENVIRONMENT, cmd + 12, 1);
if (envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--bare")) {
static char git_dir[PATH_MAX+1];
is_bare_repository_cfg = 1;
- setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);
+ xsetenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);
if (envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "-c")) {
--git a/pager.c b/pager.c
index 975955b..d3a1299 100644
--- a/pager.c
+++ b/pager.c
@@ -76,7 +76,7 @@ void setup_pager(void)
if (!pager)
return;
- setenv("GIT_PAGER_IN_USE", "true", 1);
+ xsetenv("GIT_PAGER_IN_USE", "true", 1);
/* spawn the pager */
pager_argv[0] = pager;
diff --git a/run-command.c b/run-command.c
index 1c51043..37abbde 100644
--- a/run-command.c
+++ b/run-command.c
@@ -258,7 +258,7 @@ fail_pipe:
if (cmd->env) {
for (; *cmd->env; cmd->env++) {
if (strchr(*cmd->env, '='))
- putenv((char *)*cmd->env);
+ xputenv((char *)*cmd->env);
else
unsetenv(*cmd->env);
}
diff --git a/setup.c b/setup.c
index 61c22e6..06f38d0 100644
--- a/setup.c
+++ b/setup.c
@@ -309,7 +309,7 @@ void setup_work_tree(void)
* if $GIT_WORK_TREE is set relative
*/
if (getenv(GIT_WORK_TREE_ENVIRONMENT))
- setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1);
+ xsetenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1);
set_git_dir(relative_path(git_dir, work_tree));
initialized = 1;
@@ -683,9 +683,9 @@ const char *setup_git_directory_gently(int *nongit_ok)
prefix = setup_git_directory_gently_1(nongit_ok);
if (prefix)
- setenv("GIT_PREFIX", prefix, 1);
+ xsetenv("GIT_PREFIX", prefix, 1);
else
- setenv("GIT_PREFIX", "", 1);
+ xsetenv("GIT_PREFIX", "", 1);
if (startup_info) {
startup_info->have_repository = !nongit_ok || !*nongit_ok;
--
1.7.7.1.msysgit.0.272.g9e47e
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox