Re: [PATCH v3 2/3] perf: make the tests work in worktrees
On Fri, May 13, 2016 at 03:25:58PM +0200, Johannes Schindelin wrote: > This patch makes perf-lib.sh more robust so that it can run correctly > even inside a worktree. For example, it assumed that $GIT_DIR/objects is > the objects directory (which is not the case for worktrees) and it used > the commondir file verbatim, even if it contained a relative path. > > Furthermore, the setup code expected `git rev-parse --git-dir` to spit > out a relative path, which is also not true for worktrees. Let's just > change the code to accept both relative and absolute paths, by avoiding > the `cd` into the copied working directory. I was trying to run the perf scripts today and got bit by this patch. The problem is this line: > + objects_dir="$(git -C "$source" rev-parse --git-path objects)" When the perf script is running, the "git" in its PATH is the version of git being perf-tested, not the most recent one. And --git-path wasn't introduced until v2.5.0. So if you try to compare results against an older git, it fails: $ cd t/perf $ ./run v2.4.0 v2.9.0 -- p-perf-lib-sanity.sh [...] === Running 1 tests in build/67308bd628c6235dbc1bad60c9ad1f2d27d576cc/bin-wrappers === fatal: ambiguous argument 'objects': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git [...] -- [...]' cp: unrecognized option '--git-path objects' Try 'cp --help' for more information. error: failed to copy repository '/home/peff/compile/git/t/..' to '/var/ram/git-tests/trash directory.p-perf-lib-sanity' [...] Test v2.4.0 v2.9.0 -- .1: test_perf_default_repo works 0.00(0.00+0.00) .2: test_checkout_worktree works 0.01(0.00+0.00) .4: export a weird var 0.00(0.00+0.00) .6: important variables available in subshells 0.00(0.00+0.00) .7: test-lib-functions correctly loaded in subshells 0.00(0.00+0.00) I know that running modern perf tests against older versions isn't guaranteed to work (the tests might rely on newer options, for example), but it at least generally worked up until this point. IMHO the problem is in the design of t/perf, though. It should always use your modern git for the harness setup, and only use the git-to-test inside test_expect and test_perf blocks. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] perf: make the tests work in worktrees
René Scharfe writes: >>> I fear that interacts badly with the `cd "$repo"` I introduced later >>> (replacing a `cd ..`)... > > Oh, right, it does if $repo is a relative path. > >> What do you want to do then? For now before -rc1 we can revert the >> whole thing so that we can get a tested thing that works in both >> layouts. > > Put it in its own subshell, e.g. like this? OK. I've squashed it and merged the result to 'next'. We'll see if anybody screams and merge it to 'master' before -rc2. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] perf: make the tests work in worktrees
Am 30.05.2016 um 20:03 schrieb Junio C Hamano:
> Johannes Schindelin writes:
>
>>> This breaks perf for the non-worktree case:
>>
>> Oh drats!
>>
>>> lsr@debian:~/src/git/t/perf$ make
>>> rm -rf test-results
>>> ./run
>>> === Running 12 tests in this tree ===
>>> cp: cannot stat '.git/objects': No such file or directory
>>> error: failed to copy repository '/home/lsr/src/git/t/..' to '/tmp/trash
>>> directory.p-perf-lib-sanity'
>>> cp: cannot stat '.git/objects': No such file or directory
>>> error: failed to copy repository '/home/lsr/src/git/t/..' to '/tmp/trash
>>> directory.p0001-rev-list'
>>> ...
>>>
>>> Here's a fix:
>>>
>>> -- >8 --
>>> Subject: perf: make the tests work without a worktree
>>>
>>> In regular repositories $source_git and $objects_dir contain relative
>>> paths based on $source. Go there to allow cp to resolve them.
>>>
>>> Signed-off-by: Rene Scharfe
>>> ---
>>> t/perf/perf-lib.sh | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
>>> index 5ef1744..1888790 100644
>>> --- a/t/perf/perf-lib.sh
>>> +++ b/t/perf/perf-lib.sh
>>> @@ -84,6 +84,7 @@ test_perf_create_repo_from () {
>>> objects_dir="$(git -C "$source" rev-parse --git-path objects)"
>>> mkdir -p "$repo/.git"
>>> (
>>> + cd "$source" &&
>>
>> I fear that interacts badly with the `cd "$repo"` I introduced later
>> (replacing a `cd ..`)...
Oh, right, it does if $repo is a relative path.
> What do you want to do then? For now before -rc1 we can revert the
> whole thing so that we can get a tested thing that works in both
> layouts.
Put it in its own subshell, e.g. like this?
---
t/perf/perf-lib.sh | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 5ef1744..18c363e 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -84,6 +84,7 @@ test_perf_create_repo_from () {
objects_dir="$(git -C "$source" rev-parse --git-path objects)"
mkdir -p "$repo/.git"
(
+ cd "$source" &&
{ cp -Rl "$objects_dir" "$repo/.git/" 2>/dev/null ||
cp -R "$objects_dir" "$repo/.git/"; } &&
for stuff in "$source_git"/*; do
@@ -94,7 +95,9 @@ test_perf_create_repo_from () {
cp -R "$stuff" "$repo/.git/" || exit 1
;;
esac
- done &&
+ done
+ ) &&
+ (
cd "$repo" &&
git init -q && {
test_have_prereq SYMLINKS ||
--
2.8.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] perf: make the tests work in worktrees
Johannes Schindelin writes:
>> This breaks perf for the non-worktree case:
>
> Oh drats!
>
>> lsr@debian:~/src/git/t/perf$ make
>> rm -rf test-results
>> ./run
>> === Running 12 tests in this tree ===
>> cp: cannot stat '.git/objects': No such file or directory
>> error: failed to copy repository '/home/lsr/src/git/t/..' to '/tmp/trash
>> directory.p-perf-lib-sanity'
>> cp: cannot stat '.git/objects': No such file or directory
>> error: failed to copy repository '/home/lsr/src/git/t/..' to '/tmp/trash
>> directory.p0001-rev-list'
>> ...
>>
>> Here's a fix:
>>
>> -- >8 --
>> Subject: perf: make the tests work without a worktree
>>
>> In regular repositories $source_git and $objects_dir contain relative
>> paths based on $source. Go there to allow cp to resolve them.
>>
>> Signed-off-by: Rene Scharfe
>> ---
>> t/perf/perf-lib.sh | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
>> index 5ef1744..1888790 100644
>> --- a/t/perf/perf-lib.sh
>> +++ b/t/perf/perf-lib.sh
>> @@ -84,6 +84,7 @@ test_perf_create_repo_from () {
>> objects_dir="$(git -C "$source" rev-parse --git-path objects)"
>> mkdir -p "$repo/.git"
>> (
>> +cd "$source" &&
>
> I fear that interacts badly with the `cd "$repo"` I introduced later
> (replacing a `cd ..`)...
What do you want to do then? For now before -rc1 we can revert the
whole thing so that we can get a tested thing that works in both
layouts.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] perf: make the tests work in worktrees
Hi René,
On Sun, 29 May 2016, René Scharfe wrote:
> Am 13.05.2016 um 15:25 schrieb Johannes Schindelin:
> > This patch makes perf-lib.sh more robust so that it can run correctly
> > even inside a worktree. For example, it assumed that $GIT_DIR/objects is
> > the objects directory (which is not the case for worktrees) and it used
> > the commondir file verbatim, even if it contained a relative path.
> >
> > Furthermore, the setup code expected `git rev-parse --git-dir` to spit
> > out a relative path, which is also not true for worktrees. Let's just
> > change the code to accept both relative and absolute paths, by avoiding
> > the `cd` into the copied working directory.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > t/perf/perf-lib.sh | 14 +++---
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> > index 9fa0706..5ef1744 100644
> > --- a/t/perf/perf-lib.sh
> > +++ b/t/perf/perf-lib.sh
> > @@ -80,22 +80,22 @@ test_perf_create_repo_from () {
> > 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)
> > + source_git="$(git -C "$source" rev-parse --git-dir)"
> > + objects_dir="$(git -C "$source" rev-parse --git-path objects)"
> > mkdir -p "$repo/.git"
> > (
> > - cd "$repo/.git" &&
> > - { cp -Rl "$source_git/objects" . 2>/dev/null ||
> > - cp -R "$source_git/objects" .; } &&
> > + { cp -Rl "$objects_dir" "$repo/.git/" 2>/dev/null ||
> > + cp -R "$objects_dir" "$repo/.git/"; } &&
> > for stuff in "$source_git"/*; do
> > case "$stuff" in
> > - */objects|*/hooks|*/config)
> > + */objects|*/hooks|*/config|*/commondir)
> > ;;
> > *)
> > - cp -R "$stuff" . || exit 1
> > + cp -R "$stuff" "$repo/.git/" || exit 1
> > ;;
> > esac
> > done &&
> > - cd .. &&
> > + cd "$repo" &&
> > git init -q && {
> > test_have_prereq SYMLINKS ||
> > git config core.symlinks false
> >
>
> This breaks perf for the non-worktree case:
Oh drats!
> lsr@debian:~/src/git/t/perf$ make
> rm -rf test-results
> ./run
> === Running 12 tests in this tree ===
> cp: cannot stat '.git/objects': No such file or directory
> error: failed to copy repository '/home/lsr/src/git/t/..' to '/tmp/trash
> directory.p-perf-lib-sanity'
> cp: cannot stat '.git/objects': No such file or directory
> error: failed to copy repository '/home/lsr/src/git/t/..' to '/tmp/trash
> directory.p0001-rev-list'
> ...
>
> Here's a fix:
>
> -- >8 --
> Subject: perf: make the tests work without a worktree
>
> In regular repositories $source_git and $objects_dir contain relative
> paths based on $source. Go there to allow cp to resolve them.
>
> Signed-off-by: Rene Scharfe
> ---
> t/perf/perf-lib.sh | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index 5ef1744..1888790 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -84,6 +84,7 @@ test_perf_create_repo_from () {
> objects_dir="$(git -C "$source" rev-parse --git-path objects)"
> mkdir -p "$repo/.git"
> (
> + cd "$source" &&
I fear that interacts badly with the `cd "$repo"` I introduced later
(replacing a `cd ..`)...
Ciao,
Dscho
Re: [PATCH v3 2/3] perf: make the tests work in worktrees
Am 13.05.2016 um 15:25 schrieb Johannes Schindelin:
> This patch makes perf-lib.sh more robust so that it can run correctly
> even inside a worktree. For example, it assumed that $GIT_DIR/objects is
> the objects directory (which is not the case for worktrees) and it used
> the commondir file verbatim, even if it contained a relative path.
>
> Furthermore, the setup code expected `git rev-parse --git-dir` to spit
> out a relative path, which is also not true for worktrees. Let's just
> change the code to accept both relative and absolute paths, by avoiding
> the `cd` into the copied working directory.
>
> Signed-off-by: Johannes Schindelin
> ---
> t/perf/perf-lib.sh | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index 9fa0706..5ef1744 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -80,22 +80,22 @@ test_perf_create_repo_from () {
> 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)
> + source_git="$(git -C "$source" rev-parse --git-dir)"
> + objects_dir="$(git -C "$source" rev-parse --git-path objects)"
> mkdir -p "$repo/.git"
> (
> - cd "$repo/.git" &&
> - { cp -Rl "$source_git/objects" . 2>/dev/null ||
> - cp -R "$source_git/objects" .; } &&
> + { cp -Rl "$objects_dir" "$repo/.git/" 2>/dev/null ||
> + cp -R "$objects_dir" "$repo/.git/"; } &&
> for stuff in "$source_git"/*; do
> case "$stuff" in
> - */objects|*/hooks|*/config)
> + */objects|*/hooks|*/config|*/commondir)
> ;;
> *)
> - cp -R "$stuff" . || exit 1
> + cp -R "$stuff" "$repo/.git/" || exit 1
> ;;
> esac
> done &&
> - cd .. &&
> + cd "$repo" &&
> git init -q && {
> test_have_prereq SYMLINKS ||
> git config core.symlinks false
>
This breaks perf for the non-worktree case:
lsr@debian:~/src/git/t/perf$ make
rm -rf test-results
./run
=== Running 12 tests in this tree ===
cp: cannot stat '.git/objects': No such file or directory
error: failed to copy repository '/home/lsr/src/git/t/..' to '/tmp/trash
directory.p-perf-lib-sanity'
cp: cannot stat '.git/objects': No such file or directory
error: failed to copy repository '/home/lsr/src/git/t/..' to '/tmp/trash
directory.p0001-rev-list'
...
Here's a fix:
-- >8 --
Subject: perf: make the tests work without a worktree
In regular repositories $source_git and $objects_dir contain relative
paths based on $source. Go there to allow cp to resolve them.
Signed-off-by: Rene Scharfe
---
t/perf/perf-lib.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 5ef1744..1888790 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -84,6 +84,7 @@ test_perf_create_repo_from () {
objects_dir="$(git -C "$source" rev-parse --git-path objects)"
mkdir -p "$repo/.git"
(
+ cd "$source" &&
{ cp -Rl "$objects_dir" "$repo/.git/" 2>/dev/null ||
cp -R "$objects_dir" "$repo/.git/"; } &&
for stuff in "$source_git"/*; do
--
2.8.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/3] perf: make the tests work in worktrees
This patch makes perf-lib.sh more robust so that it can run correctly
even inside a worktree. For example, it assumed that $GIT_DIR/objects is
the objects directory (which is not the case for worktrees) and it used
the commondir file verbatim, even if it contained a relative path.
Furthermore, the setup code expected `git rev-parse --git-dir` to spit
out a relative path, which is also not true for worktrees. Let's just
change the code to accept both relative and absolute paths, by avoiding
the `cd` into the copied working directory.
Signed-off-by: Johannes Schindelin
---
t/perf/perf-lib.sh | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 9fa0706..5ef1744 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -80,22 +80,22 @@ test_perf_create_repo_from () {
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)
+ source_git="$(git -C "$source" rev-parse --git-dir)"
+ objects_dir="$(git -C "$source" rev-parse --git-path objects)"
mkdir -p "$repo/.git"
(
- cd "$repo/.git" &&
- { cp -Rl "$source_git/objects" . 2>/dev/null ||
- cp -R "$source_git/objects" .; } &&
+ { cp -Rl "$objects_dir" "$repo/.git/" 2>/dev/null ||
+ cp -R "$objects_dir" "$repo/.git/"; } &&
for stuff in "$source_git"/*; do
case "$stuff" in
- */objects|*/hooks|*/config)
+ */objects|*/hooks|*/config|*/commondir)
;;
*)
- cp -R "$stuff" . || exit 1
+ cp -R "$stuff" "$repo/.git/" || exit 1
;;
esac
done &&
- cd .. &&
+ cd "$repo" &&
git init -q && {
test_have_prereq SYMLINKS ||
git config core.symlinks false
--
2.8.2.465.gb077790
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
