Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
On Mon, Aug 27, 2018 at 07:04:52PM -0400, Jeff King wrote: > On Mon, Aug 27, 2018 at 10:22:46AM +, Kirill Smelkov wrote: > > > A minor comment from outside observer: running tests under something > > like > > > > -e and -o pipefail > > > > would automatically catch the mistake in the first place. Maybe `-o > > pipefail` is bashism (I had not checked), but `git grep " | " t/` shows > > there are a lot of pipelines being used, and thus similar errors might be > > silently resting there. Something like -o pipefail would catch all such > > problems automatically. > > Yes, "pipefail" is a bash-ism that we can't rely on. > > I will say that I have been bitten before by "set -e -o pipefail" and > its subtle handling of SIGPIPE. Try this: > > set -e -o pipefail > yes | head Thanks for the information. Oh well...
Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
On Mon, Aug 27, 2018 at 10:22:46AM +, Kirill Smelkov wrote: > A minor comment from outside observer: running tests under something > like > > -e and -o pipefail > > would automatically catch the mistake in the first place. Maybe `-o > pipefail` is bashism (I had not checked), but `git grep " | " t/` shows > there are a lot of pipelines being used, and thus similar errors might be > silently resting there. Something like -o pipefail would catch all such > problems automatically. Yes, "pipefail" is a bash-ism that we can't rely on. I will say that I have been bitten before by "set -e -o pipefail" and its subtle handling of SIGPIPE. Try this: set -e -o pipefail yes | head -Peff
Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
On Tue, Aug 14, 2018 at 01:47:21PM +0200, SZEDER Gábor wrote:
> The test 'pack-objects to file can use bitmap' added in 645c432d61
> (pack-objects: use reachability bitmap index when generating
> non-stdout pack, 2016-09-10) is silently buggy and doesn't check what
> it's supposed to.
>
> In 't5310-pack-bitmaps.sh', the 'list_packed_objects' helper function
> does what its name implies by running:
>
> git show-index <"$1" | cut -d' ' -f2
>
> The test in question invokes this function like this:
>
> list_packed_objects packa.objects &&
> list_packed_objects packb.objects &&
> test_cmp packa.objects packb.objects
>
> Note how these two callsites don't specify the name of the pack index
> file as the function's parameter, but redirect the function's standard
> input from it. This triggers an error message from the shell, as it
> has no filename to redirect from in the function, but this error is
> ignored, because it happens upstream of a pipe. Consequently, both
> invocations produce empty 'pack{a,b}.objects' files, and the
> subsequent 'test_cmp' happily finds those two empty files identical.
>
> Fix these two 'list_packed_objects' invocations by specifying the pack
> index files as parameters. Furthermore, eliminate the pipe in that
> function by replacing it with an &&-chained pair of commands using an
> intermediate file, so a failure of 'git show-index' or the shell
> redirection will fail the test.
>
> Signed-off-by: SZEDER Gábor
> ---
> t/t5310-pack-bitmaps.sh | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index 6ee4d3f2d9..557bd0d0c0 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -9,7 +9,8 @@ objpath () {
>
> # show objects present in pack ($1 should be associated *.idx)
> list_packed_objects () {
> - git show-index <"$1" | cut -d' ' -f2
> + git show-index <"$1" >object-list &&
> + cut -d' ' -f2 object-list
> }
>
> # has_any pattern-file content-file
> @@ -204,8 +205,8 @@ test_expect_success 'pack-objects to file can use bitmap'
> '
> # verify equivalent packs are generated with/without using bitmap index
> packasha1=$(git pack-objects --no-use-bitmap-index --all packa
>packbsha1=$(git pack-objects --use-bitmap-index --all packb &&
> - list_packed_objects packa.objects &&
> - list_packed_objects packb.objects &&
> + list_packed_objects packa-$packasha1.idx >packa.objects &&
> + list_packed_objects packb-$packbsha1.idx >packb.objects &&
> test_cmp packa.objects packb.objects
> '
Thanks for catching and correcting this.
A minor comment from outside observer: running tests under something
like
-e and -o pipefail
would automatically catch the mistake in the first place. Maybe `-o
pipefail` is bashism (I had not checked), but `git grep " | " t/` shows
there are a lot of pipelines being used, and thus similar errors might be
silently resting there. Something like -o pipefail would catch all such
problems automatically.
Kirill
Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
I would encourage use of an existing function to check for emptiness, but require a particular argument for it to be considered "the right way:" test_cmp /dev/null actual This means less vocabulary to memorize for test writers. It's usually a code smell to have special logic for a specific value for a specific argument - a sign that a separate function ought to be created - but since we want to add an error or warning in test_cmp anyway when is empty, I think this special logic is OK. As for comparing against a file that *might* be empty, like a utility function, might I suggest requiring the file name be formatted in a specific way if it may be empty? Like require a certain substring. Then the syntax for comparison would be: # If the emptiness is unconditional test_cmp /dev/null actual # If the emptiness is unknown ahead of time test_cmp maybe_empty_expected actual Then, issue an error for something like: > expected && test_cmp expected actual which says: " Use test_cmp /dev/null to verify a file is empty. If the file may or may not be empty (as in a utility function), include the string "maybe_empty" in the file name. "
Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
Jeff King writes: >> > If we assume that "expect" is first (which is our convention but not >> > necessarily guaranteed), then I think the best logic is something like: >> > >> > if $1 is empty; then >> > bug in the test script >> > elif test_cmp_allow_empty "$@" >> > test failure >> > >> > We do not need to check $2 at all. An empty one is either irrelevant (if >> > the expectation is empty), or a test failure (because it would not match >> > the non-empty $1). >> >> ... this is indeed a better solution. I written out the cases for >> updated test_cmp to straighten out my thinking: > > I'd be OK pursuing either this line, or what you showed originally. As I do find [1] to be a real concern, I'd prefer not to flag empty input to test_cmp as special. But if we _were_ to do something, I agree that "$2 can be anything---that is the output from the potentially buggy program we are testing" is the right attitude to take. [Footnote] *1* https://public-inbox.org/git/cam0vkjkt7fbjrie_3f4b13bht9hp9mxrhux5r1sogh2x7kq...@mail.gmail.com/
Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
On Sun, Aug 19, 2018 at 11:37:59PM +0200, Andrei Rybak wrote:
> On 19/08/18 22:32, Jeff King wrote:
> > On Sun, Aug 19, 2018 at 07:50:42PM +0200, Andrei Rybak wrote:
> >
> >> 1. Check both files at the same time (combination with Gábor's
> >> function):
> >>
> >>test_cmp () {
> >>if test "$1" != - &&
> >> test "$2" != - &&
> >> ! test -s "$1" &&
> >> ! test -s "$2"
> >>then
> >>error "bug in test script: using test_cmp to check
> >> empty file; use test_must_be_empty instead"
> >>fi
> >>test_cmp_allow_empty "$@"
> >>}
> >>
> >> This will still be reporting to the developer clearly, but
> >> will only catch cases exactly like the bogus test in t5310.
> >
> > Doesn't that have the opposite issue? If we expect non-empty output but
> > the command produces empty output, we'd say "bug in the test script".
> > But that is not true at all; it's a failed test.
>
> No. Only when both "$1" and "$2" are empty files will the function above
> report "bug in test script". From patch's commit message:
Oh, you're right. Somewhere between the screen and my brain the "&&"
became an "||".
I agree that works, and has the advantage that the arguments are treated
symmetrically. We _might_ say "test failure" instead of "bug in test"
when the expectation is empty and the generated output is not (because
we do not know which is which), but I think that would be uncommon (and
the most important thing is that we do not silently consider it a pass).
> > If we assume that "expect" is first (which is our convention but not
> > necessarily guaranteed), then I think the best logic is something like:
> >
> > if $1 is empty; then
> > bug in the test script
> > elif test_cmp_allow_empty "$@"
> > test failure
> >
> > We do not need to check $2 at all. An empty one is either irrelevant (if
> > the expectation is empty), or a test failure (because it would not match
> > the non-empty $1).
>
> ... this is indeed a better solution. I written out the cases for
> updated test_cmp to straighten out my thinking:
I'd be OK pursuing either this line, or what you showed originally.
-Peff
Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
On 19/08/18 22:32, Jeff King wrote:
> On Sun, Aug 19, 2018 at 07:50:42PM +0200, Andrei Rybak wrote:
>
>> 1. Check both files at the same time (combination with Gábor's
>> function):
>>
>> test_cmp () {
>> if test "$1" != - &&
>> test "$2" != - &&
>> ! test -s "$1" &&
>> ! test -s "$2"
>> then
>> error "bug in test script: using test_cmp to check
>> empty file; use test_must_be_empty instead"
>> fi
>> test_cmp_allow_empty "$@"
>> }
>>
>> This will still be reporting to the developer clearly, but
>> will only catch cases exactly like the bogus test in t5310.
>
> Doesn't that have the opposite issue? If we expect non-empty output but
> the command produces empty output, we'd say "bug in the test script".
> But that is not true at all; it's a failed test.
No. Only when both "$1" and "$2" are empty files will the function above
report "bug in test script". From patch's commit message:
... both invocations produce empty 'pack{a,b}.objects' files, and the
subsequent 'test_cmp' happily finds those two empty files identical.
That's what I meant by "will only catch cases exactly like the bogus
test in t5310".
However ...
> If we assume that "expect" is first (which is our convention but not
> necessarily guaranteed), then I think the best logic is something like:
>
> if $1 is empty; then
> bug in the test script
> elif test_cmp_allow_empty "$@"
> test failure
>
> We do not need to check $2 at all. An empty one is either irrelevant (if
> the expectation is empty), or a test failure (because it would not match
> the non-empty $1).
... this is indeed a better solution. I written out the cases for
updated test_cmp to straighten out my thinking:
* both $1 and $2 are empty:
bogus test:
needs either fixing generation of both expect and actual
or switching to test_must_be_empty
OR
bogus helper function, as Gábor described above:
needs to switch to test_cmp_allow_empty
* $1 is non-empty && $2 is empty
proceeding with test
test failure from GIT_TEST_CMP
* $1 is empty && $2 is non-empty
bogus test - needs either switching to test_must_be_empty
(and after that test_must_be_empty will report failure)
or fixing generation of expect (and after that test result
depends on contents).
* both $1 and $2 are non-empty
proceeding with test
result depends on contents
Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
On Sun, Aug 19, 2018 at 07:50:42PM +0200, Andrei Rybak wrote:
> > I actually think the above gives way too confusing output, when the
> > actual output is empty and we are expecting some output.
> >
> > The tester wants to hear from test_cmp "your 'git cmd' produced some
> > output when we are expecting none" as the primary message. We are
> > trying to find bugs in "git" under development, and diagnosing iffy
> > tests is secondary. But with your change, the first thing that is
> > checked is if 'expect' is an empty file and that is what we get
> > complaints about, without even looking at what is in 'actual'.
>
> I came up with two solutions for this issue:
>
> 1. Check both files at the same time (combination with Gábor's
> function):
>
> test_cmp () {
> if test "$1" != - &&
> test "$2" != - &&
> ! test -s "$1" &&
> ! test -s "$2"
> then
> error "bug in test script: using test_cmp to check
> empty file; use test_must_be_empty instead"
> fi
> test_cmp_allow_empty "$@"
> }
>
> This will still be reporting to the developer clearly, but
> will only catch cases exactly like the bogus test in t5310.
Doesn't that have the opposite issue? If we expect non-empty output but
the command produces empty output, we'd say "bug in the test script".
But that is not true at all; it's a failed test.
If we assume that "expect" is first (which is our convention but not
necessarily guaranteed), then I think the best logic is something like:
if $1 is empty; then
bug in the test script
elif test_cmp_allow_empty "$@"
test failure
We do not need to check $2 at all. An empty one is either irrelevant (if
the expectation is empty), or a test failure (because it would not match
the non-empty $1).
If we go that route, we should make sure that test_cmp's documentation
is updated to mention that the two sides are not symmetric (and possibly
update some callers, though I'd be OK leaving ones that aren't broken by
this, and just clean them up over time).
-Peff
Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
On 17/08/18 22:09, Junio C Hamano wrote:
> Andrei Rybak writes:
>>
>> I'll try something like the following on the weekend:
>>
>> test_cmp () {
>> if test "$1" != - && ! test -s "$1"
>> then
>> echo >&4 "error: trying to compare empty file '$1'"
>> return 1
>> fi
>> if test "$2" != - && ! test -s "$2"
>> then
>> echo >&4 "error: trying to compare empty file '$2'"
>> return 1
>> fi
>> test_cmp_allow_empty "$@"
>> }
>
> I actually think the above gives way too confusing output, when the
> actual output is empty and we are expecting some output.
>
> The tester wants to hear from test_cmp "your 'git cmd' produced some
> output when we are expecting none" as the primary message. We are
> trying to find bugs in "git" under development, and diagnosing iffy
> tests is secondary. But with your change, the first thing that is
> checked is if 'expect' is an empty file and that is what we get
> complaints about, without even looking at what is in 'actual'.
I came up with two solutions for this issue:
1. Check both files at the same time (combination with Gábor's
function):
test_cmp () {
if test "$1" != - &&
test "$2" != - &&
! test -s "$1" &&
! test -s "$2"
then
error "bug in test script: using test_cmp to check
empty file; use test_must_be_empty instead"
fi
test_cmp_allow_empty "$@"
}
This will still be reporting to the developer clearly, but
will only catch cases exactly like the bogus test in t5310.
2. Enable this check via variable, smth like EMPTY_CMP_LINT=1
Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
On Fri, Aug 17, 2018 at 9:27 PM Andrei Rybak wrote:
>
> On 17/08/18 19:39, SZEDER Gábor wrote:
> >
> > See, we have quite a few tests that extract repetitive common tasks
> > into helper functions, which sometimes includes preparing the expected
> > results and running 'test_cmp', e.g. something like this
> > (oversimplified) example:
> >
> > check_cmd () {
> > git cmd $1 >actual &&
> > echo "$2" >expect &&
> > test_cmp expect actual
> > }
> >
> > check_cmd --fooFOO
> > check_cmd --no-foo ""
>
> I've only had time to look into this from t0001 up to t0008-ignores.sh, where
> test_check_ignore does this. If these helper functions need to allow comparing
> empty files -- how about adding special variation of cmp functions for cases
> like this: test_cmp_allow_empty and test_i18ncmp_allow_empty?
>
> I think it would be a good trade-off to allow these helper functions to skip
> checking emptiness of arguments for test_cmp. Such patch will require only
> s/test_cmp/&_allow_empty/ for these helper functions and it will help catch
> cases as bogus test in t5310.
>
> I'll try something like the following on the weekend:
>
> test_cmp() {
> if test "$1" != - && ! test -s "$1"
> then
> echo >&4 "error: trying to compare empty file '$1'"
> return 1
> fi
> if test "$2" != - && ! test -s "$2"
> then
> echo >&4 "error: trying to compare empty file '$2'"
> return 1
> fi
Yeah, these conditions are what I instrumented 'test_cmp' with (except
I used 'error "bug in test script ..."') to find callsites that could
be converted to 'test_must_be_empty', that's how I found the bug fixed
in this patch. However, it resulted in a lot of errors from the cases
mentioned in my previous email. Then I reached out to Bash and tried
this:
test_cmp() {
if test -n "$BASH_VERSION" &&
test "${FUNCNAME[1]}" = "test_eval_inner_" &&
test "$1" != "-" &&
test ! -s "$1"
then
error "bug in test script: using test_cmp to check
empty file; use test_must_be_empty instead"
fi
$GIT_TEST_CMP "$@"
}
i.e. to limit the check callsites where 'test_cmp' is called directly
from within a test_expect_{success,failure} block. This is better,
almost all errors could be converted to 'test_must_be_empty' stright
away; I have the patches almost ready. There are, however, a few
parametric cases that choke on this: where we run 'test_cmp' in a
loop, e.g. 'cvs update (-p)' in t9400, and I think there was a case
where the 'test_expect_success' block is within a function.
> test_cmp_allow_empty "$@"
> }
>
> test_cmp_allow_empty() {
> $GIT_TEST_CMP "$@"
> }
>
> (I'm not sure about redirections in test lib functions. The two if's would
> probably be in a separate function to be re-used by test_i18ncmp.)
Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
Andrei Rybak writes:
> I think it would be a good trade-off to allow these helper functions to skip
> checking emptiness of arguments for test_cmp. Such patch will require only
> s/test_cmp/&_allow_empty/ for these helper functions and it will help catch
> cases as bogus test in t5310.
>
> I'll try something like the following on the weekend:
>
> test_cmp() {
Style: SP before and after ().
> if test "$1" != - && ! test -s "$1"
> then
> echo >&4 "error: trying to compare empty file '$1'"
> return 1
> fi
> if test "$2" != - && ! test -s "$2"
> then
> echo >&4 "error: trying to compare empty file '$2'"
> return 1
> fi
> test_cmp_allow_empty "$@"
> }
I actually think the above gives way too confusing output, when the
actual output is empty and we are expecting some output. I.e.
: >expect &&
git cmd >actual &&
test_cmp expect actual
The tester wants to hear from test_cmp "your 'git cmd' produced some
output when we are expecting none" as the primary message. We are
trying to find bugs in "git" under development, and diagnosing iffy
tests is secondary. But with your change, the first thing that is
checked is if 'expect' is an empty file and that is what we get
complaints about, without even looking at what is in 'actual'.
It's the same story, and it is even worse than the above, when we
are expecting some output but the command produced no output, i.e.
echo Everything up-to-date. >expect &&
git cmd >actual &&
test_cmp expect actual
We should get a complaint from test_cmp that 'actual' does not match
the string we were expecting (and even better, we show how they are
different by running them thru "diff -u"), not that 'actual' is an
empty file.
> test_cmp_allow_empty() {
> $GIT_TEST_CMP "$@"
> }
>
> (I'm not sure about redirections in test lib functions. The two if's would
> probably be in a separate function to be re-used by test_i18ncmp.)
Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
On 17/08/18 19:39, SZEDER Gábor wrote:
>
> See, we have quite a few tests that extract repetitive common tasks
> into helper functions, which sometimes includes preparing the expected
> results and running 'test_cmp', e.g. something like this
> (oversimplified) example:
>
> check_cmd () {
> git cmd $1 >actual &&
> echo "$2" >expect &&
> test_cmp expect actual
> }
>
> check_cmd --fooFOO
> check_cmd --no-foo ""
I've only had time to look into this from t0001 up to t0008-ignores.sh, where
test_check_ignore does this. If these helper functions need to allow comparing
empty files -- how about adding special variation of cmp functions for cases
like this: test_cmp_allow_empty and test_i18ncmp_allow_empty?
I think it would be a good trade-off to allow these helper functions to skip
checking emptiness of arguments for test_cmp. Such patch will require only
s/test_cmp/&_allow_empty/ for these helper functions and it will help catch
cases as bogus test in t5310.
I'll try something like the following on the weekend:
test_cmp() {
if test "$1" != - && ! test -s "$1"
then
echo >&4 "error: trying to compare empty file '$1'"
return 1
fi
if test "$2" != - && ! test -s "$2"
then
echo >&4 "error: trying to compare empty file '$2'"
return 1
fi
test_cmp_allow_empty "$@"
}
test_cmp_allow_empty() {
$GIT_TEST_CMP "$@"
}
(I'm not sure about redirections in test lib functions. The two if's would
probably be in a separate function to be re-used by test_i18ncmp.)
Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
On Fri, Aug 17, 2018 at 12:36 AM Junio C Hamano wrote:
>
> Andrei Rybak writes:
>
> > On 14/08/18 13:47, SZEDER Gábor wrote:
> >> ... both
> >> invocations produce empty 'pack{a,b}.objects' files, and the
> >> subsequent 'test_cmp' happily finds those two empty files identical.
> >
> > Is test_cmp ever used for empty files? Would it make sense for
> > test_cmp to issue warning when an empty file is being compared?
>
> Typically test_cmp is used to compare the actual output from a
> dubious command being tested with the expected output from a
> procedure that is known not to be broken (e.g. a run of 'echo', or a
> 'cat' of here-doc), so at least one side would not be empty.
>
> The test done here is an odd case---it compares output from two
> equally dubious processes that are being tested and sees if their
> results match.
>
> That said, since we have test_must_be_empty, we could forbid feeding
> empty files to test_cmp, after telling everybody that a test that
> expects an empty output must use test_must_be_empty. I do not think
> it is a terrible idea.
I thought so as well, and I've looked into it; in fact this patch was
one of the skeletons that fell out of our test suite "while at it".
However, I had to change my mind about it, and now I don't think we
should go all the way and forbid that.
See, we have quite a few tests that extract repetitive common tasks
into helper functions, which sometimes includes preparing the expected
results and running 'test_cmp', e.g. something like this
(oversimplified) example:
check_cmd () {
git cmd $1 >actual &&
echo "$2" >expect &&
test_cmp expect actual
}
check_cmd --fooFOO
check_cmd --no-foo ""
Completely forbidding feeding empty files to 'test_cmp' would require
us to add a bit of logic to cases like this to call 'test_cmp' or
'test_must_be_empty' based on the content of the second parameter.
Arguably it's only a tiny bit of logic, as only a single if statement
is needed, but following our coding style it would take 7 lines
instead of only 2:
if test -n "$2"
then
echo "$2" >expect &&
test_cmp expect actual
else
test_must_be_empty actual
fi
I don't think it's worth it in cases like this.
Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
Andrei Rybak writes:
> On 14/08/18 13:47, SZEDER Gábor wrote:
>> ... both
>> invocations produce empty 'pack{a,b}.objects' files, and the
>> subsequent 'test_cmp' happily finds those two empty files identical.
>
> Is test_cmp ever used for empty files? Would it make sense for
> test_cmp to issue warning when an empty file is being compared?
Typically test_cmp is used to compare the actual output from a
dubious command being tested with the expected output from a
procedure that is known not to be broken (e.g. a run of 'echo', or a
'cat' of here-doc), so at least one side would not be empty.
The test done here is an odd case---it compares output from two
equally dubious processes that are being tested and sees if their
results match.
That said, since we have test_must_be_empty, we could forbid feeding
empty files to test_cmp, after telling everybody that a test that
expects an empty output must use test_must_be_empty. I do not think
it is a terrible idea.
Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
On 14/08/18 13:47, SZEDER Gábor wrote:
> ... both
> invocations produce empty 'pack{a,b}.objects' files, and the
> subsequent 'test_cmp' happily finds those two empty files identical.
Is test_cmp ever used for empty files? Would it make sense for
test_cmp to issue warning when an empty file is being compared?
> ---
> t/t5310-pack-bitmaps.sh | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index 6ee4d3f2d9..557bd0d0c0 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -9,7 +9,8 @@ objpath () {
>
> # show objects present in pack ($1 should be associated *.idx)
> list_packed_objects () {
> - git show-index <"$1" | cut -d' ' -f2
> + git show-index <"$1" >object-list &&
> + cut -d' ' -f2 object-list
> }
>
> # has_any pattern-file content-file
> @@ -204,8 +205,8 @@ test_expect_success 'pack-objects to file can use bitmap'
> '
> # verify equivalent packs are generated with/without using bitmap index
> packasha1=$(git pack-objects --no-use-bitmap-index --all packa
>packbsha1=$(git pack-objects --use-bitmap-index --all packb &&
> - list_packed_objects packa.objects &&
> - list_packed_objects packb.objects &&
> + list_packed_objects packa-$packasha1.idx >packa.objects &&
> + list_packed_objects packb-$packbsha1.idx >packb.objects &&
> test_cmp packa.objects packb.objects
> '
>
>
Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
On Tue, Aug 14, 2018 at 01:47:21PM +0200, SZEDER Gábor wrote:
> The test 'pack-objects to file can use bitmap' added in 645c432d61
> (pack-objects: use reachability bitmap index when generating
> non-stdout pack, 2016-09-10) is silently buggy and doesn't check what
> it's supposed to.
>
> In 't5310-pack-bitmaps.sh', the 'list_packed_objects' helper function
> does what its name implies by running:
>
> git show-index <"$1" | cut -d' ' -f2
>
> The test in question invokes this function like this:
>
> list_packed_objects packa.objects &&
> list_packed_objects packb.objects &&
> test_cmp packa.objects packb.objects
>
> Note how these two callsites don't specify the name of the pack index
> file as the function's parameter, but redirect the function's standard
> input from it. This triggers an error message from the shell, as it
> has no filename to redirect from in the function, but this error is
> ignored, because it happens upstream of a pipe. Consequently, both
> invocations produce empty 'pack{a,b}.objects' files, and the
> subsequent 'test_cmp' happily finds those two empty files identical.
>
> Fix these two 'list_packed_objects' invocations by specifying the pack
> index files as parameters. Furthermore, eliminate the pipe in that
> function by replacing it with an &&-chained pair of commands using an
> intermediate file, so a failure of 'git show-index' or the shell
> redirection will fail the test.
Good catch, and nicely explained. The patch itself looks obviously
correct.
-Peff
