Re: [PATCH] git p4 test: fix failure in 9814-git-p4-rename.sh Was: Re: Test failure in t9814-git-p4-rename.sh - my environment or bad test?
Christoph Bonitz writes: > Apart from your change and the word wrap adjustments suggested by > Pete, would the following also make sense, to fix the other flaw > Johannes pointed out? With regards to failing, git diff-tree should be > idempotent. I think those are the two occurrences in this file: As a band-aid, that might be OK, but I think these pipelines are unnecessarily and overly wasteful in the first place. All the "sed 1d" you see here is only because the upstream uses the one-tree form "diff-tree $commit"; by comparing two, i.e. "diff-tree $commit^ $commit", they can be dropped. All the "cut -f2" is to grab the pathname; we have "--name-only" these days. I.e. > src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) && should become src=$(git diff-tree --name-only -r -C --find-copies-harder HEAD^ HEAD) && I would think. Extracting C[0-9]* manually with sed is bad, and expecting that the score is within certain range is even worse, because there is no formal guarantee that the definition of similarity indices will not improve in the future. --diff-filter=C to limit the output to only copied paths, without looking at the similarity index, would be more appropriate, e.g. git diff-tree --name-only --diff-filter=C -r -C HEAD^ HEAD or something along those lines. Otherwise, run them outside $(), keep the result in a temporary file, and process the temporary file with the pipeline. That has an added benefit that lets you inspect the file when something goes wrong. I.e. git diff-tree ... >diff-tree-out && level=$( sed 1d http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git p4 test: fix failure in 9814-git-p4-rename.sh Was: Re: Test failure in t9814-git-p4-rename.sh - my environment or bad test?
On Fri, Jul 25, 2014 at 12:05 AM, Junio C Hamano wrote: > Johannes Sixt writes: >> I see a few other no-nos in the context of the changes, in particular, >> pipelines where git is not the last command; these would not catch >> failures in the git commands. But a fix for that is certainly outside >> the scope of this patch. > > Yuck. Thanks for spotting. > > Perhaps we should apply a preliminary clean-up before doing anything > else, perhaps? The change in 9814 is a post 2.0 regression. Apart from your change and the word wrap adjustments suggested by Pete, would the following also make sense, to fix the other flaw Johannes pointed out? With regards to failing, git diff-tree should be idempotent. I think those are the two occurrences in this file: diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh index 1fc1f5f..7815f9a 100755 --- a/t/t9814-git-p4-rename.sh +++ b/t/t9814-git-p4-rename.sh @@ -176,6 +176,7 @@ test_expect_success 'detect copies' ' git diff-tree -r -C --find-copies-harder HEAD && level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C test -n "$level" && test "$level" -gt 0 && test "$level" -lt 98 && + git diff-tree -r -C --find-copies-harder HEAD && src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) && test "$src" = file10 || test "$src" = file11 && git config git-p4.detectCopies $(($level + 2)) && @@ -190,6 +191,7 @@ test_expect_success 'detect copies' ' git diff-tree -r -C --find-copies-harder HEAD && level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C test -n "$level" && test "$level" -gt 2 && test "$level" -lt 100 && + git diff-tree -r -C --find-copies-harder HEAD && src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) && test "$src" = file10 || test "$src" = file11 || test "$src" = file12 && git config git-p4.detectCopies $(($level - 2)) && -- 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] git p4 test: fix failure in 9814-git-p4-rename.sh Was: Re: Test failure in t9814-git-p4-rename.sh - my environment or bad test?
On Thu, Jul 24, 2014 at 8:45 PM, Johannes Sixt wrote: > Am 23.07.2014 23:28, schrieb Christoph Bonitz: >> - test "$src" = file10 || test "$src" = file11 && >> + test "$src" = file2 || test "$src" = file10 || test "$src" = file11 && > > You can't test for alternatives in this way. It's already wrong in the > original line, which is from 795fcb0e (avoid "test -a/-o "), > and breaks the && chain. Thank you, I'm sorry I missed that! -- 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] git p4 test: fix failure in 9814-git-p4-rename.sh Was: Re: Test failure in t9814-git-p4-rename.sh - my environment or bad test?
Johannes Sixt writes:
>> @@ -177,7 +175,7 @@ test_expect_success 'detect copies' '
>> level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 |
>> cut -d" " -f5 | sed "s/C0*//") &&
>> test -n "$level" && test "$level" -gt 0 && test "$level" -lt 98 &&
>> src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
>> - test "$src" = file10 || test "$src" = file11 &&
>> + test "$src" = file2 || test "$src" = file10 || test "$src" = file11 &&
>
> You can't test for alternatives in this way. It's already wrong in the
> original line, which is from 795fcb0e (avoid "test -a/-o "),
> and breaks the && chain. You need braces like this:
>
> { test "$src" = file2 || test "$src" = file10 || test "$src" = file11; } &&
>
> or you wrap it up in a case statement.
> ...
>> )
>> '
>>
>
> I see a few other no-nos in the context of the changes, in particular,
> pipelines where git is not the last command; these would not catch
> failures in the git commands. But a fix for that is certainly outside
> the scope of this patch.
Yuck. Thanks for spotting.
Perhaps we should apply a preliminary clean-up before doing anything
else, perhaps? The change in 9814 is a post 2.0 regression.
t/t9814-git-p4-rename.sh | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index 1fc1f5f..95f4421 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -177,7 +177,10 @@ test_expect_success 'detect copies' '
level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d
| cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
test -n "$level" && test "$level" -gt 0 && test "$level" -lt 98
&&
src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d |
cut -f2) &&
- test "$src" = file10 || test "$src" = file11 &&
+ case "$src" in
+ file10 | file11) : ;; # happy
+ *) false ;; # not
+ &&
git config git-p4.detectCopies $(($level + 2)) &&
git p4 submit &&
p4 filelog //depot/file12 &&
@@ -191,7 +194,10 @@ test_expect_success 'detect copies' '
level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d
| cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
test -n "$level" && test "$level" -gt 2 && test "$level" -lt
100 &&
src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d |
cut -f2) &&
- test "$src" = file10 || test "$src" = file11 || test "$src" =
file12 &&
+ case "$src" in
+ file10 | file11 | file12) : ;; # happy
+ *) false ;; # not
+ &&
git config git-p4.detectCopies $(($level - 2)) &&
git p4 submit &&
p4 filelog //depot/file13 &&
--
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] git p4 test: fix failure in 9814-git-p4-rename.sh Was: Re: Test failure in t9814-git-p4-rename.sh - my environment or bad test?
Am 23.07.2014 23:28, schrieb Christoph Bonitz:
> The scenario in the rename test makes unnecessary assumptions about
> which file git file-tree will detect as a source for a copy-operations.
> Furthermore, copy detection is not tested by checking the resulting
> perforce revision history via p4 filelog, but via git diff-tree.
>
> This patch makes the test more robust by accepting each of the possible
> sources, and more rigorous by doing so via p4 filelog.
> ---
Please sign off your patch.
> t/t9814-git-p4-rename.sh | 12
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
> index 1fc1f5f..4068510 100755
> --- a/t/t9814-git-p4-rename.sh
> +++ b/t/t9814-git-p4-rename.sh
> @@ -156,18 +156,16 @@ test_expect_success 'detect copies' '
> git diff-tree -r -C HEAD &&
> git p4 submit &&
> p4 filelog //depot/file10 &&
> - p4 filelog //depot/file10 | grep -q "branch from //depot/file" &&
> + p4 filelog //depot/file10 | grep -q "branch from //depot/file2" &&
>
> cp file2 file11 &&
> git add file11 &&
> git commit -a -m "Copy file2 to file11" &&
> git diff-tree -r -C --find-copies-harder HEAD &&
> - src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
> - test "$src" = file10 &&
> git config git-p4.detectCopiesHarder true &&
> git p4 submit &&
> p4 filelog //depot/file11 &&
> - p4 filelog //depot/file11 | grep -q "branch from //depot/file" &&
> + p4 filelog //depot/file11 | grep -q -E "branch from //depot/file(2|10)" &&
>
> cp file2 file12 &&
> echo "some text" >>file12 &&
> @@ -177,7 +175,7 @@ test_expect_success 'detect copies' '
> level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 |
> cut -d" " -f5 | sed "s/C0*//") &&
> test -n "$level" && test "$level" -gt 0 && test "$level" -lt 98 &&
> src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
> - test "$src" = file10 || test "$src" = file11 &&
> + test "$src" = file2 || test "$src" = file10 || test "$src" = file11 &&
You can't test for alternatives in this way. It's already wrong in the
original line, which is from 795fcb0e (avoid "test -a/-o "),
and breaks the && chain. You need braces like this:
{ test "$src" = file2 || test "$src" = file10 || test "$src" = file11; } &&
or you wrap it up in a case statement.
> git config git-p4.detectCopies $(($level + 2)) &&
> git p4 submit &&
> p4 filelog //depot/file12 &&
> @@ -190,12 +188,10 @@ test_expect_success 'detect copies' '
> git diff-tree -r -C --find-copies-harder HEAD &&
> level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 |
> cut -d" " -f5 | sed "s/C0*//") &&
> test -n "$level" && test "$level" -gt 2 && test "$level" -lt 100 &&
> - src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
> - test "$src" = file10 || test "$src" = file11 || test "$src" = file12 &&
> git config git-p4.detectCopies $(($level - 2)) &&
> git p4 submit &&
> p4 filelog //depot/file13 &&
> - p4 filelog //depot/file13 | grep -q "branch from //depot/file"
> + p4 filelog //depot/file13 | grep -q -E "branch from
> //depot/file(2|10|11|12)"
> )
> '
>
I see a few other no-nos in the context of the changes, in particular,
pipelines where git is not the last command; these would not catch
failures in the git commands. But a fix for that is certainly outside
the scope of this patch.
-- Hannes
--
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] git p4 test: fix failure in 9814-git-p4-rename.sh Was: Re: Test failure in t9814-git-p4-rename.sh - my environment or bad test?
ml.christophbon...@gmail.com wrote on Wed, 23 Jul 2014 23:28 +0200: > The scenario in the rename test makes unnecessary assumptions about > which file git file-tree will detect as a source for a copy-operations. > Furthermore, copy detection is not tested by checking the resulting > perforce revision history via p4 filelog, but via git diff-tree. > > This patch makes the test more robust by accepting each of the possible > sources, and more rigorous by doing so via p4 filelog. I see what you're doing here, and it all looks good. The diff-tree checks were mainly debugging, and if "p4 filelog" shows the right "branch from", that means it works. You might be able to firm up the "branch from" lines for file8 and file9 too, but those aren't likely to fail. Indeed, as noted in the other thread, it would be better to make these not so flaky, but your change here fixes a problem, and doesn't do any harm. And gives you an opportunity to fix it more later. :) Be sure to fix the word-wrapping you have on two of the lines below. And be careful not to top post. Here's my ack for when you decide to send it back to the list, cc junio. Acked-by: Pete Wyckoff > --- > t/t9814-git-p4-rename.sh | 12 > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh > index 1fc1f5f..4068510 100755 > --- a/t/t9814-git-p4-rename.sh > +++ b/t/t9814-git-p4-rename.sh > @@ -156,18 +156,16 @@ test_expect_success 'detect copies' ' > git diff-tree -r -C HEAD && > git p4 submit && > p4 filelog //depot/file10 && > - p4 filelog //depot/file10 | grep -q "branch from //depot/file" && > + p4 filelog //depot/file10 | grep -q "branch from //depot/file2" && > > cp file2 file11 && > git add file11 && > git commit -a -m "Copy file2 to file11" && > git diff-tree -r -C --find-copies-harder HEAD && > - src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) && > - test "$src" = file10 && > git config git-p4.detectCopiesHarder true && > git p4 submit && > p4 filelog //depot/file11 && > - p4 filelog //depot/file11 | grep -q "branch from //depot/file" && > + p4 filelog //depot/file11 | grep -q -E "branch from //depot/file(2|10)" && > > cp file2 file12 && > echo "some text" >>file12 && > @@ -177,7 +175,7 @@ test_expect_success 'detect copies' ' > level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut > -f1 | cut -d" " -f5 | sed "s/C0*//") && > test -n "$level" && test "$level" -gt 0 && test "$level" -lt 98 && > src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) && > - test "$src" = file10 || test "$src" = file11 && > + test "$src" = file2 || test "$src" = file10 || test "$src" = file11 && > git config git-p4.detectCopies $(($level + 2)) && > git p4 submit && > p4 filelog //depot/file12 && > @@ -190,12 +188,10 @@ test_expect_success 'detect copies' ' > git diff-tree -r -C --find-copies-harder HEAD && > level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut > -f1 | cut -d" " -f5 | sed "s/C0*//") && > test -n "$level" && test "$level" -gt 2 && test "$level" -lt 100 && > - src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) && > - test "$src" = file10 || test "$src" = file11 || test "$src" = file12 && > git config git-p4.detectCopies $(($level - 2)) && > git p4 submit && > p4 filelog //depot/file13 && > - p4 filelog //depot/file13 | grep -q "branch from //depot/file" > + p4 filelog //depot/file13 | grep -q -E "branch from > //depot/file(2|10|11|12)" > ) > ' > > -- > 2.0.1 -- 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
