[PATCH] test-lib-functions: fix test_subcommand_inexact

[PATCH] test-lib-functions: fix test_subcommand_inexact

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

The implementation of test_subcommand_inexact() was originally
introduced in e4d0c11c0 (repack: respect kept objects with '--write-midx
-b', 2021-12-20) with the intention to allow finding a subcommand based
on an initial set of arguments. The inexactness was intended as a way to
allow flexible options beyond that initial set, as opposed to
test_subcommand() which requires that the full list of options is
provided in its entirety.

The implementation began by copying test_subcommand() and replaced the
repeated argument 'printf' statement to append ".*" instead of "," to
each argument. This has a few drawbacks:

1. Most importantly, this repeats the use of ".*" within 'expr', so the
   inexact match is even more flexible than expected. It allows the list
   of arguments to exist as a subsequence (with any other items included
   between those arguments).

2. The line 'expr="$(expr%,}"' that previously removed a trailing comma
   now no longer does anything, since the string ends with ".*".

Both of these issues are fixed by keeping the addition of the comma in
the printf statement, then adding ".*" after stripping out the trailing
comma.

All existing tests continue to pass with this change, since none of them
were taking advantage of this unintended flexibility.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
    test-lib-functions: fix test_subcommand_inexact
    
    This fixes a concern discovered by Junio in [1].
    
    [1] https://lore.kernel.org/git/xmqq4k41vdwe.fsf@gitster.g/
    
    Thanks, -Stolee

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1185%2Fderrickstolee%2Ftest-subcommand-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1185/derrickstolee/test-subcommand-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1185

 t/test-lib-functions.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 0f439c99d61..8f0e5da8727 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1811,8 +1811,8 @@ test_subcommand_inexact () {
 		shift
 	fi
 
-	local expr=$(printf '"%s".*' "$@")
-	expr="${expr%,}"
+	local expr=$(printf '"%s",' "$@")
+	expr="${expr%,}.*"
 
 	if test -n "$negate"
 	then

base-commit: 74cc1aa55f30ed76424a0e7226ab519aa6265061
-- 
gitgitgadget

Re: [PATCH] test-lib-functions: fix test_subcommand_inexact

[Derrick Stolee]

On 3/21/22 4:34 PM, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
> 
> The implementation of test_subcommand_inexact() was originally
> introduced in e4d0c11c0 (repack: respect kept objects with '--write-midx
> -b', 2021-12-20) with the intention to allow finding a subcommand based
> on an initial set of arguments. The inexactness was intended as a way to
> allow flexible options beyond that initial set, as opposed to
> test_subcommand() which requires that the full list of options is
> provided in its entirety.
> 
> The implementation began by copying test_subcommand() and replaced the
> repeated argument 'printf' statement to append ".*" instead of "," to
> each argument. This has a few drawbacks:
> 
> 1. Most importantly, this repeats the use of ".*" within 'expr', so the
>    inexact match is even more flexible than expected. It allows the list
>    of arguments to exist as a subsequence (with any other items included
>    between those arguments).
> 
> 2. The line 'expr="$(expr%,}"' that previously removed a trailing comma
>    now no longer does anything, since the string ends with ".*".
> 
> Both of these issues are fixed by keeping the addition of the comma in
> the printf statement, then adding ".*" after stripping out the trailing
> comma.
> 
> All existing tests continue to pass with this change, since none of them
> were taking advantage of this unintended flexibility.

For some reason, I thought I had run all tests on my machine to
passing, but I was wrong.

Instead, t7700-repack.sh fails because of this test:

test_expect_success '--write-midx -b packs non-kept objects' '
	GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
		git repack --write-midx -a -b &&
	test_subcommand_inexact git pack-objects --honor-pack-keep <trace.txt
'

Specifically, "git pack-objects" has several options before
"--honor-pack-keep" including the temporary pack name and
a "--keep-true-parents" flag.

So, this patch is incorrect about keeping things working. The
options are:

1. Keep the repeated ".*" and be clear about the expectations.
   (This could drop the "remove trailing comma" line.)

2. Find another way to test this --write-midx behavior while
   keeping the stricter test_subcommand_inexact helper.

3. Something else???

Thanks,
-Stolee

Re: [PATCH] test-lib-functions: fix test_subcommand_inexact

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> So, this patch is incorrect about keeping things working. The
> options are:
>
> 1. Keep the repeated ".*" and be clear about the expectations.
>    (This could drop the "remove trailing comma" line.)
>
> 2. Find another way to test this --write-midx behavior while
>    keeping the stricter test_subcommand_inexact helper.
>
> 3. Something else???

The result of doing #1 is still "inexact" but at that point it is
unclear if we are being way too inexact to be useful.  If the
looseness bothers us too much, we may decide that #1 is not worth
doing.  But obviously the looseness did not bother us that much
until last week, so probably an obvious #3, do nothing, letting the
sleeping dog lie, might be what we want to do?

If we were to pursue #2, then, would we tightening the test for the
write-midx using the "stricter" helper, or would the stricter one be
too strict to be useful for that case?  If we are rewriting the
write-midx test by not using the "stricter" helper, then we would be
creating a stricter one nobody uses, which sounds quite wasteful.

It seems that the only case that could result in a result that is
better than "do nothing" is if we can use a different pattern with
the "stricter" helper to express what "write-midx" test wanted to
do, but because what we need to fuzzily match on the command line in
that test includes a generated temporary filename, I do not think
it is likely to be easily doable.

So, perhaps #3 ;-)?

Re: [PATCH] test-lib-functions: fix test_subcommand_inexact

[Derrick Stolee]

On 3/23/2022 10:53 AM, Junio C Hamano wrote:
> Derrick Stolee <derrickstolee@github.com> writes:
> 
>> So, this patch is incorrect about keeping things working. The
>> options are:
>>
>> 1. Keep the repeated ".*" and be clear about the expectations.
>>    (This could drop the "remove trailing comma" line.)
>>
>> 2. Find another way to test this --write-midx behavior while
>>    keeping the stricter test_subcommand_inexact helper.
>>
>> 3. Something else???
> 
> The result of doing #1 is still "inexact" but at that point it is
> unclear if we are being way too inexact to be useful.  If the
> looseness bothers us too much, we may decide that #1 is not worth
> doing.  But obviously the looseness did not bother us that much
> until last week, so probably an obvious #3, do nothing, letting the
> sleeping dog lie, might be what we want to do?
> 
> If we were to pursue #2, then, would we tightening the test for the
> write-midx using the "stricter" helper, or would the stricter one be
> too strict to be useful for that case?  If we are rewriting the
> write-midx test by not using the "stricter" helper, then we would be
> creating a stricter one nobody uses, which sounds quite wasteful.
> 
> It seems that the only case that could result in a result that is
> better than "do nothing" is if we can use a different pattern with
> the "stricter" helper to express what "write-midx" test wanted to
> do, but because what we need to fuzzily match on the command line in
> that test includes a generated temporary filename, I do not think
> it is likely to be easily doable.
> 
> So, perhaps #3 ;-)?

I'll default to #3 (do nothing), but if this shows up again
I'll plan on adding a comment to the helper to be clear on
how "inexact" the helper really is.

Thanks,
-Stolee

Re: [PATCH] test-lib-functions: fix test_subcommand_inexact

[Taylor Blau]

On Wed, Mar 23, 2022 at 10:55:37AM -0400, Derrick Stolee wrote:
> > So, perhaps #3 ;-)?
>
> I'll default to #3 (do nothing), but if this shows up again
> I'll plan on adding a comment to the helper to be clear on
> how "inexact" the helper really is.

I wonder if we could sidestep the whole issue with
test_subcommand_inexact by testing this behavior by looking at the
contents of the packs themselves.

If we have a kept pack, and then add some new objects, and run "git
repack --write-midx -adb", the new pack should not contain any of the
objects found in the old (kept) pack. And that's the case after this
patch, but was broken before it.

Here's a test which constructs that scenario and then asserts that there
isn't any overlap between the newly created pack and the old, kept pack.

--- 8< ---

diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 5922fb5bdd..96812fa226 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -370,9 +370,31 @@ test_expect_success '--write-midx with preferred bitmap tips' '
 '

 test_expect_success '--write-midx -b packs non-kept objects' '
-	GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
-		git repack --write-midx -a -b &&
-	test_subcommand_inexact git pack-objects --honor-pack-keep <trace.txt
+	git init repo &&
+	test_when_finished "rm -fr repo" &&
+	(
+		cd repo &&
+
+		test_commit base &&
+		git repack -ad &&
+
+		find $objdir/pack -name "*.idx" >before &&
+		>$objdir/pack/$(basename $(cat before) .idx).keep &&
+
+		test_commit other &&
+		git repack --write-midx -a -b -d &&
+
+		find $objdir/pack -name "*.idx" | sort >after &&
+
+		git show-index <$(cat before) >old.raw &&
+		git show-index <$(comm -13 before after) >new.raw &&
+
+		cut -d" " -f2 old.raw | sort >old.objects &&
+		cut -d" " -f2 new.raw | sort >new.objects &&
+
+		comm -12 old.objects new.objects >shared.objects &&
+		test_must_be_empty shared.objects
+	)
 '

 test_expect_success TTY '--quiet disables progress' '

--- >8 ---

It does seem a little word-y to me, but I think you could clean it up a
little bit, too. If you want to take that patch, I think we could
reasonably use the above diff as a first patch, and then remove the
declaration of test_subcommand_inexact in a second patch.

(Feel free to forge my s-o-b if you do want to pick that up).

Thanks,
Taylor

Re: [PATCH] test-lib-functions: fix test_subcommand_inexact

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> On Wed, Mar 23, 2022 at 10:55:37AM -0400, Derrick Stolee wrote:
>> > So, perhaps #3 ;-)?
>>
>> I'll default to #3 (do nothing), but if this shows up again
>> I'll plan on adding a comment to the helper to be clear on
>> how "inexact" the helper really is.
>
> I wonder if we could sidestep the whole issue with
> test_subcommand_inexact by testing this behavior by looking at the
> contents of the packs themselves.
>
> If we have a kept pack, and then add some new objects, and run "git
> repack --write-midx -adb", the new pack should not contain any of the
> objects found in the old (kept) pack. And that's the case after this
> patch, but was broken before it.

Sounds quite sensible.

Instead of saying "we are happy as long as we internally run this
command, as that _should_ give us the desired outcome", we check the
resulting packs ourselves, and we do not really care how the
"repack" command gave us that desired outcome.

Re: [PATCH] test-lib-functions: fix test_subcommand_inexact

[Derrick Stolee]

On 3/23/2022 7:10 PM, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> 
>> On Wed, Mar 23, 2022 at 10:55:37AM -0400, Derrick Stolee wrote:
>>>> So, perhaps #3 ;-)?
>>>
>>> I'll default to #3 (do nothing), but if this shows up again
>>> I'll plan on adding a comment to the helper to be clear on
>>> how "inexact" the helper really is.
>>
>> I wonder if we could sidestep the whole issue with
>> test_subcommand_inexact by testing this behavior by looking at the
>> contents of the packs themselves.
>>
>> If we have a kept pack, and then add some new objects, and run "git
>> repack --write-midx -adb", the new pack should not contain any of the
>> objects found in the old (kept) pack. And that's the case after this
>> patch, but was broken before it.
> 
> Sounds quite sensible.
> 
> Instead of saying "we are happy as long as we internally run this
> command, as that _should_ give us the desired outcome", we check the
> resulting packs ourselves, and we do not really care how the
> "repack" command gave us that desired outcome.

Sounds good. It's all about a balance: using test_subcommand[_inexact]
gives us a way to check "Did we trigger this other command that we
trust works correctly from other tests?" without the more complicated
work of doing a full post-condition check. It's a bit more of a unit-
level check than most Git tests.

The full post-condition check requires more test code, but that's not
really a problem. The problem comes in if that test is now too rigid
to future changes in that subcommand. What if the post-conditions
change in a subtle way because of the subcommand does something
differently, but in a way that is not of importance to the top
command?

In this specific case, the test name says that it "packs non-kept
objects", so we can do more here to validate that post-condition
that we care about.

As I'm looking at Taylor's test case example, the one thing I notice
is that there is only one pack-file before the repack. It would be
good to have a non-kept packfile get repacked in the process, not
just the loose objects added by the test_commit. I'll take a look at
what can be done here.

Thanks,
-Stolee

Re: [PATCH] test-lib-functions: fix test_subcommand_inexact

[Taylor Blau]

On Thu, Mar 24, 2022 at 11:42:44AM -0400, Derrick Stolee wrote:
> As I'm looking at Taylor's test case example, the one thing I notice
> is that there is only one pack-file before the repack. It would be
> good to have a non-kept packfile get repacked in the process, not
> just the loose objects added by the test_commit. I'll take a look at
> what can be done here.

I think you are too good at nerd-sniping me ;-). Here's a more robust
test, that I think reads a little cleaner than the previous round. Let
me know what you think:

--- 8< ---

diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 5922fb5bdd..1ed9a98a36 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -369,10 +369,36 @@ test_expect_success '--write-midx with preferred bitmap tips' '
 	)
 '

+packdir=$objdir/pack
+
 test_expect_success '--write-midx -b packs non-kept objects' '
-	GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
-		git repack --write-midx -a -b &&
-	test_subcommand_inexact git pack-objects --honor-pack-keep <trace.txt
+	git init repo &&
+	test_when_finished "rm -fr repo" &&
+	(
+		cd repo &&
+
+		test_commit kept &&
+		git repack -ad &&
+
+		>$packdir/$(basename $packdir/pack-*.pack .pack).keep &&
+
+		test_commit unkept &&
+		git repack -d &&
+
+		test_commit new &&
+
+		find $packdir -type f -name "pack-*.idx" | sort >before &&
+		git repack --write-midx -a -b -d &&
+		find $packdir -type f -name "pack-*.idx" | sort >after &&
+
+		git rev-list --objects --no-object-names kept.. >expect.raw &&
+		sort expect.raw >expect &&
+
+		git show-index <$(comm -13 before after) >actual.raw &&
+		cut -d" " -f2 actual.raw >actual &&
+
+		test_cmp expect actual
+	)
 '

 test_expect_success TTY '--quiet disables progress' '

--- >8 ---

Thanks,
Taylor

Re: [PATCH] test-lib-functions: fix test_subcommand_inexact

[Derrick Stolee]

On 3/24/2022 12:02 PM, Taylor Blau wrote:
> On Thu, Mar 24, 2022 at 11:42:44AM -0400, Derrick Stolee wrote:
>> As I'm looking at Taylor's test case example, the one thing I notice
>> is that there is only one pack-file before the repack. It would be
>> good to have a non-kept packfile get repacked in the process, not
>> just the loose objects added by the test_commit. I'll take a look at
>> what can be done here.
> 
> I think you are too good at nerd-sniping me ;-). Here's a more robust
> test, that I think reads a little cleaner than the previous round. Let
> me know what you think:

Finally, you found my most redeeming quality!

> +		test_commit kept &&
> +		git repack -ad &&
> +
> +		>$packdir/$(basename $packdir/pack-*.pack .pack).keep &&
> +
> +		test_commit unkept &&
> +		git repack -d &&
> +
> +		test_commit new &&
> +
> +		find $packdir -type f -name "pack-*.idx" | sort >before &&
> +		git repack --write-midx -a -b -d &&
> +		find $packdir -type f -name "pack-*.idx" | sort >after &&
> +
> +		git rev-list --objects --no-object-names kept.. >expect.raw &&
> +		sort expect.raw >expect &&

This is an interesting way to get this set of objects without storing
the original pack name. It might be good to keep consistent with the
way we get the new objects, though.

> +
> +		git show-index <$(comm -13 before after) >actual.raw &&
> +		cut -d" " -f2 actual.raw >actual &&
> +
> +		test_cmp expect actual
> +	)

I've got a modification of your original design prepared in my GGG PR
and will send a v2 including it after it passes all of the builds.

Thanks,
-Stolee

Re: [PATCH] test-lib-functions: fix test_subcommand_inexact

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> The full post-condition check requires more test code, but that's not
> really a problem. The problem comes in if that test is now too rigid
> to future changes in that subcommand. What if the post-conditions
> change in a subtle way because of the subcommand does something
> differently, but in a way that is not of importance to the top
> command?
>
> In this specific case, the test name says that it "packs non-kept
> objects", so we can do more here to validate that post-condition
> that we care about.

Thanks for clearly laying out the way to think about the issue.  I
agree with all of it, of course ;-)

Re: [PATCH] test-lib-functions: fix test_subcommand_inexact

[Abhradeep Chakraborty]

Junio C Hamano <gitster@pobox.com> wrote:

> The result of doing #1 is still "inexact" but at that point it is
> unclear if we are being way too inexact to be useful.  If the
> looseness bothers us too much, we may decide that #1 is not worth
> doing.  But obviously the looseness did not bother us that much
> until last week, so probably an obvious #3, do nothing, letting the
> sleeping dog lie, might be what we want to do?

Personally, I would prefer #3 i.e. do nothing (even in the future; unless
it is removed all together). I also think that the current behaviour is not
"too inexact". Rather it would be too strict for `test_subcommand_inexact`
if we remove the ".*" thing here.

Inexact means that the line needs not to be exactly same - there may be
some words in between the desired words (in this case, any flags that come
between the desired sub-commands). The current behaviour (i.e. 
`local expr=$(printf '"%s".*' "$@")`) is justifying the name of the function.
Replacing ".*" with "," will therefore not work as the name of the function
suggests - it will rather work as `test_subcommand_starts_with`.

Only the `expr=${expr%,}.*" line needs to be changed, I think.

Thanks :)

Re: [PATCH] test-lib-functions: fix test_subcommand_inexact

[Junio C Hamano]

Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> writes:

> Inexact means that the line needs not to be exactly same - there may be
> some words in between the desired words (in this case, any flags that come
> between the desired sub-commands). The current behaviour (i.e. 
> `local expr=$(printf '"%s".*' "$@")`) is justifying the name of the function.

The current name may justify what it does, but so does "declare
match randomly" would.  How much fuzziness we tolerate is something
that must be made clear to help developers who may potentially want
to use it, and the phrase "inexact" alone would not help us.

Re: [PATCH] test-lib-functions: fix test_subcommand_inexact

[Abhradeep Chakraborty]

Junio C Hamano <gitster@pobox.com> wrote:

> ... How much fuzziness we tolerate is something
> that must be made clear to help developers who may potentially want
> to use it, and the phrase "inexact" alone would not help us.

Yep! I agree.

Thanks :)

[PATCH v2 0/2] test-lib-functions: fix test_subcommand_inexact

[Derrick Stolee via GitGitGadget]

Junio discovered in [1] that test_subcommand_inexact is more flexible than
initially intended.

[1] https://lore.kernel.org/git/xmqq4k41vdwe.fsf@gitster.g/

The intention was that we do not need to specify the remaining arguments for
a subcommand, but instead the current behavior is to allow the given
arguments to appear as any subsequence within the command (except that the
first "git" instance must be the first argument).

This changes the helper to be more rigid.


Changes in v2
=============

 * After noticing that t7700 fails with the helper change, Taylor wrote a
   modification of the test to check the post-conditions directly and avoid
   the helper. This is included as Patch 1 with some modifications to be
   easier to read.

 * Patch 2 is the v1 patch with some change to the commit message about why
   the tests pass after patch 1.

Thanks, -Stolee

Derrick Stolee (2):
  t7700: check post-condition in kept-pack test
  test-lib-functions: fix test_subcommand_inexact

 t/t7700-repack.sh       | 52 ++++++++++++++++++++++++++++++++++++++---
 t/test-lib-functions.sh |  4 ++--
 2 files changed, 51 insertions(+), 5 deletions(-)


base-commit: a68dfadae5e95c7f255cf38c9efdcbc2e36d1931
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1185%2Fderrickstolee%2Ftest-subcommand-fix-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1185/derrickstolee/test-subcommand-fix-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1185

Range-diff vs v1:

 -:  ----------- > 1:  f2f8d12929b t7700: check post-condition in kept-pack test
 1:  50176722bbf ! 2:  ed67b748971 test-lib-functions: fix test_subcommand_inexact
     @@ Commit message
          the printf statement, then adding ".*" after stripping out the trailing
          comma.
      
     -    All existing tests continue to pass with this change, since none of them
     -    were taking advantage of this unintended flexibility.
     +    All existing tests continue to pass with this change. There was one
     +    instance from t7700-repack.sh that was taking advantage of this
     +    flexibility, but it was removed in the previous change.
      
     +    Helped-by: Taylor Blau <me@ttaylorr.com>
          Signed-off-by: Derrick Stolee <derrickstolee@github.com>
      
       ## t/test-lib-functions.sh ##

-- 
gitgitgadget

[PATCH v2 1/2] t7700: check post-condition in kept-pack test

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

The '--write-midx -b packs non-kept objects' test in t7700-repack.sh
uses test_subcommand_inexact to check that 'git repack' properly adds
the '--honor-pack-keep' flag to the 'git pack-objects' subcommand.
However, the test_subcommand_inexact helper is more flexible than
initially designed, and this instance is the only one that makes use of
it: there are additional arguments between 'git pack-objects' and the
'--honor-pack-keep' flag. In order to make test_subcommand_inexact more
strict, we need to fix this instance.

This test checks that 'git repack --write-midx -a -b -d' will create a
new pack-file that does not contain the objects within the kept pack.
This behavior is possible because of the multi-pack-index bitmap that
will bitmap objects against multiple packs. Without --write-midx, the
objects in the kept pack would be duplicated so the resulting pack is
closed under reachability and bitmaps can be created against it. This is
discussed in more detail in e4d0c11c0 (repack: respect kept objects with
'--write-midx -b', 2021-12-20) which also introduced this instance of
test_subcommand_inexact.

To better verify the intended post-conditions while also removing this
instance of test_subcommand_inexact, rewrite the test to check the list
of packed objects in the kept pack and the list of the objects in the
newly-repacked pack-file _other_ than the kept pack. These lists should
be disjoint.

Be sure to include a non-kept pack-file and loose objects to be extra
careful that this is properly behaving with kept packs and not just
avoiding repacking all pack-files.

Co-authored-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 t/t7700-repack.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 3 deletions(-)

diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 770d1432046..73452e23896 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -369,10 +369,56 @@ test_expect_success '--write-midx with preferred bitmap tips' '
 	)
 '
 
+get_sorted_objects_from_packs () {
+	git show-index <$(cat) >raw &&
+	cut -d" " -f2 raw | sort
+}
+
 test_expect_success '--write-midx -b packs non-kept objects' '
-	GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
-		git repack --write-midx -a -b &&
-	test_subcommand_inexact git pack-objects --honor-pack-keep <trace.txt
+	git init repo &&
+	test_when_finished "rm -fr repo" &&
+	(
+		cd repo &&
+
+		# Create a kept pack-file
+		test_commit base &&
+		git repack -ad &&
+		find $objdir/pack -name "*.idx" >before &&
+		>$objdir/pack/$(basename $(cat before) .idx).keep &&
+
+		# Create a non-kept pack-file
+		test_commit other &&
+		git repack &&
+
+		# Create loose objects
+		test_commit loose &&
+
+		# Repack everything
+		git repack --write-midx -a -b -d &&
+
+		# There should be two pack-files now, the
+		# old, kept pack and the new, non-kept pack.
+		find $objdir/pack -name "*.idx" | sort >after &&
+		test_line_count = 2 after &&
+		find $objdir/pack -name "*.keep" >kept &&
+		test_line_count = 1 kept &&
+
+		# Get object list from the kept pack.
+		get_sorted_objects_from_packs \
+			<before \
+			>old.objects &&
+
+		# Get object list from the one non-kept pack-file
+		comm -13 before after >new-pack &&
+		get_sorted_objects_from_packs \
+			<new-pack \
+			>new.objects &&
+
+		# None of the objects in the new pack should
+		# exist within the kept pack.
+		comm -12 old.objects new.objects >shared.objects &&
+		test_must_be_empty shared.objects
+	)
 '
 
 test_expect_success TTY '--quiet disables progress' '
-- 
gitgitgadget

Re: [PATCH v2 1/2] t7700: check post-condition in kept-pack test

[Taylor Blau]

On Thu, Mar 24, 2022 at 06:34:56PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
>
> The '--write-midx -b packs non-kept objects' test in t7700-repack.sh
> uses test_subcommand_inexact to check that 'git repack' properly adds
> the '--honor-pack-keep' flag to the 'git pack-objects' subcommand.
> However, the test_subcommand_inexact helper is more flexible than
> initially designed, and this instance is the only one that makes use of
> it: there are additional arguments between 'git pack-objects' and the
> '--honor-pack-keep' flag. In order to make test_subcommand_inexact more
> strict, we need to fix this instance.
>
> This test checks that 'git repack --write-midx -a -b -d' will create a
> new pack-file that does not contain the objects within the kept pack.
> This behavior is possible because of the multi-pack-index bitmap that
> will bitmap objects against multiple packs. Without --write-midx, the
> objects in the kept pack would be duplicated so the resulting pack is
> closed under reachability and bitmaps can be created against it. This is
> discussed in more detail in e4d0c11c0 (repack: respect kept objects with
> '--write-midx -b', 2021-12-20) which also introduced this instance of
> test_subcommand_inexact.
>
> To better verify the intended post-conditions while also removing this
> instance of test_subcommand_inexact, rewrite the test to check the list
> of packed objects in the kept pack and the list of the objects in the
> newly-repacked pack-file _other_ than the kept pack. These lists should
> be disjoint.
>
> Be sure to include a non-kept pack-file and loose objects to be extra
> careful that this is properly behaving with kept packs and not just
> avoiding repacking all pack-files.

Nicely put.

> Co-authored-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>

And to be clear, I am totally OK with this usage of my signed-off-by.

> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index 770d1432046..73452e23896 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -369,10 +369,56 @@ test_expect_success '--write-midx with preferred bitmap tips' '
>  	)
>  '
>
> +get_sorted_objects_from_packs () {
> +	git show-index <$(cat) >raw &&

It seems a little odd to me to pass the name of a single file as input
to get_sorted_objects_from_packs over stdin. I probably would have
expected something like `git show-index <"$1" >raw && ...` instead.

We may also want to s/packs/pack, since this function only will handle
one index at a time.

> +	cut -d" " -f2 raw | sort

Having the sort in there is my fault, but after reading this more
carefully it's definitely unnecessary, since show-index will give us
the results in lexical order by object name already.

> +}
> +
>  test_expect_success '--write-midx -b packs non-kept objects' '
> -	GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
> -		git repack --write-midx -a -b &&
> -	test_subcommand_inexact git pack-objects --honor-pack-keep <trace.txt
> +	git init repo &&
> +	test_when_finished "rm -fr repo" &&
> +	(
> +		cd repo &&
> +
> +		# Create a kept pack-file
> +		test_commit base &&
> +		git repack -ad &&
> +		find $objdir/pack -name "*.idx" >before &&

I thought that here it might be easier to say:

    before="$(find $objdir/pack -name "*.idx")"

> +		>$objdir/pack/$(basename $(cat before) .idx).keep &&

...and then replace "$(cat before)" with "$before", along with the
other uses of the before file below. But it gets a little funny when
you want to discover which is the new pack, where it is more natural to
dump the output of comm into a file.

> +		# Get object list from the one non-kept pack-file
> +		comm -13 before after >new-pack &&

You could write "new_pack=$(comm -13 before after)", but debugging this
test would be difficult if the output of comm there contained more than
one line.

> +		get_sorted_objects_from_packs \
> +			<new-pack \

Though we probably want to check that we only get one line anyway here,
since get_sorted_objects_from_packs will barf if we had more than one
line in file new-pack here, too.

Thanks,
Taylor

Re: [PATCH v2 1/2] t7700: check post-condition in kept-pack test

[Derrick Stolee]

On 3/24/2022 2:58 PM, Taylor Blau wrote:
> On Thu, Mar 24, 2022 at 06:34:56PM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <derrickstolee@github.com>

>> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
>> index 770d1432046..73452e23896 100755
>> --- a/t/t7700-repack.sh
>> +++ b/t/t7700-repack.sh
>> @@ -369,10 +369,56 @@ test_expect_success '--write-midx with preferred bitmap tips' '
>>  	)
>>  '
>>
>> +get_sorted_objects_from_packs () {
>> +	git show-index <$(cat) >raw &&
> 
> It seems a little odd to me to pass the name of a single file as input
> to get_sorted_objects_from_packs over stdin. I probably would have
> expected something like `git show-index <"$1" >raw && ...` instead.

Based on the way we are creating a file whose contents is the name
of the .idx file, we would at least use '$(cat "$1")'. I kind of like
the symmetry of the input/output redirection when using the helper, but
I can easily change this.

> We may also want to s/packs/pack, since this function only will handle
> one index at a time.

Yes.

>> +	cut -d" " -f2 raw | sort
> 
> Having the sort in there is my fault, but after reading this more
> carefully it's definitely unnecessary, since show-index will give us
> the results in lexical order by object name already.

Cool. Will drop.

>> +}
>> +
>>  test_expect_success '--write-midx -b packs non-kept objects' '
>> -	GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
>> -		git repack --write-midx -a -b &&
>> -	test_subcommand_inexact git pack-objects --honor-pack-keep <trace.txt
>> +	git init repo &&
>> +	test_when_finished "rm -fr repo" &&
>> +	(
>> +		cd repo &&
>> +
>> +		# Create a kept pack-file
>> +		test_commit base &&
>> +		git repack -ad &&
>> +		find $objdir/pack -name "*.idx" >before &&
> 
> I thought that here it might be easier to say:
> 
>     before="$(find $objdir/pack -name "*.idx")"
> 
>> +		>$objdir/pack/$(basename $(cat before) .idx).keep &&
> 
> ...and then replace "$(cat before)" with "$before", along with the
> other uses of the before file below. But it gets a little funny when
> you want to discover which is the new pack, where it is more natural to
> dump the output of comm into a file.

For this reason, I'll continue to store the .idx names in files.

>> +		# Get object list from the one non-kept pack-file
>> +		comm -13 before after >new-pack &&
> 
> You could write "new_pack=$(comm -13 before after)", but debugging this
> test would be difficult if the output of comm there contained more than
> one line.
>
>> +		get_sorted_objects_from_packs \
>> +			<new-pack \
> 
> Though we probably want to check that we only get one line anyway here,
> since get_sorted_objects_from_packs will barf if we had more than one
> line in file new-pack here, too.

Thanks. Easy to add a test_line_count before this check.

-Stolee

Re: [PATCH v2 1/2] t7700: check post-condition in kept-pack test

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +get_sorted_objects_from_packs () {
> +	git show-index <$(cat) >raw &&
> +	cut -d" " -f2 raw | sort
> +}

As pointed out by Taylor, this "the standard input gives us the name
of a file to be read" looks strange.  It may work, and it may even
give the easiest interface to use by all the callers, but if we were
designing a more generic helper function suitable to be added to the
test-lib*.sh, we wouldn't design it like so---instead it would be
either "we read the contents of the .idx file from the standard
input" or "the first argument is the name of the .idx file".

>  test_expect_success '--write-midx -b packs non-kept objects' '
> +	git init repo &&
> +	test_when_finished "rm -fr repo" &&
> +	(
> +		cd repo &&
> +
> +		# Create a kept pack-file
> +		test_commit base &&
> +		git repack -ad &&
> +		find $objdir/pack -name "*.idx" >before &&
> +		>$objdir/pack/$(basename $(cat before) .idx).keep &&

We probably want to sanity check "repack -a" by insisting "before"
has found exactly one .idx file, before using it this way.

		test_line_count = 1 before &&
		before=$(cat before) &&
		>"${before%.idx}.keep"

> +		# Create a non-kept pack-file
> +		test_commit other &&
> +		git repack &&
> +
> +		# Create loose objects
> +		test_commit loose &&
> +
> +		# Repack everything
> +		git repack --write-midx -a -b -d &&
> +
> +		# There should be two pack-files now, the
> +		# old, kept pack and the new, non-kept pack.
> +		find $objdir/pack -name "*.idx" | sort >after &&
> +		test_line_count = 2 after &&

OK.  "after" gets sorted because we will pass it to "comm" later.

> +		find $objdir/pack -name "*.keep" >kept &&
> +		test_line_count = 1 kept &&

Since we've made sure "before" is a one-liner earlier, we could just
say

		test_cmp before kept &&

instead, no?

> +		# Get object list from the kept pack.
> +		get_sorted_objects_from_packs \
> +			<before \
> +			>old.objects &&
		
OK.

> +		# Get object list from the one non-kept pack-file
> +		comm -13 before after >new-pack &&

OK.  This should give only one line of output but that is merely
assumed.  We know after has 2 and before has 1, but haven't made
sure that before is a subset of after.

		test_line_count = 1 new-pack &&

> +		get_sorted_objects_from_packs \
> +			<new-pack \
> +			>new.objects &&

OK.

> +		# None of the objects in the new pack should
> +		# exist within the kept pack.
> +		comm -12 old.objects new.objects >shared.objects &&

Great.

> +		test_must_be_empty shared.objects
> +	)
>  '
>  
>  test_expect_success TTY '--quiet disables progress' '

Re: [PATCH v2 1/2] t7700: check post-condition in kept-pack test

[Derrick Stolee]

On 3/25/2022 1:07 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> +get_sorted_objects_from_packs () {
>> +	git show-index <$(cat) >raw &&
>> +	cut -d" " -f2 raw | sort
>> +}
> 
> As pointed out by Taylor, this "the standard input gives us the name
> of a file to be read" looks strange.  It may work, and it may even
> give the easiest interface to use by all the callers, but if we were
> designing a more generic helper function suitable to be added to the
> test-lib*.sh, we wouldn't design it like so---instead it would be
> either "we read the contents of the .idx file from the standard
> input" or "the first argument is the name of the .idx file".

Ok. Can do.
 
>>  test_expect_success '--write-midx -b packs non-kept objects' '
>> +	git init repo &&
>> +	test_when_finished "rm -fr repo" &&
>> +	(
>> +		cd repo &&
>> +
>> +		# Create a kept pack-file
>> +		test_commit base &&
>> +		git repack -ad &&
>> +		find $objdir/pack -name "*.idx" >before &&
>> +		>$objdir/pack/$(basename $(cat before) .idx).keep &&
> 
> We probably want to sanity check "repack -a" by insisting "before"
> has found exactly one .idx file, before using it this way.

> 		test_line_count = 1 before &&
> 		before=$(cat before) &&
> 		>"${before%.idx}.keep"

Good idea. This mixture of a file and variable sharing
a name is a bit muddy for me, though. Using "before_name"
as the variable would be enough of a differentiator.

>> +		find $objdir/pack -name "*.keep" >kept &&
>> +		test_line_count = 1 kept &&
> 
> Since we've made sure "before" is a one-liner earlier, we could just
> say
 
> 		test_cmp before kept &&
> 
> instead, no?

'before' contains a .idx name and 'kept' contains a .keep name,
so this direct comparison does not work. We could do that
additional check like this:

	kept_name=$(cat kept) &&
	echo ${kept_name%.keep}.idx >kept-idx &&
	test_cmp before kept-idx &&

Thanks for taking the time to clean this up, as it might become
a good example for these kinds of post-condition checks of the packs
directory in the future.

Thanks,
-Stolee

Re: [PATCH v2 1/2] t7700: check post-condition in kept-pack test

[Taylor Blau]

On Fri, Mar 25, 2022 at 01:23:27PM -0400, Derrick Stolee wrote:
> > Since we've made sure "before" is a one-liner earlier, we could just
> > say
>
> > 		test_cmp before kept &&
> >
> > instead, no?
>
> 'before' contains a .idx name and 'kept' contains a .keep name,
> so this direct comparison does not work. We could do that
> additional check like this:
>
> 	kept_name=$(cat kept) &&
> 	echo ${kept_name%.keep}.idx >kept-idx &&
> 	test_cmp before kept-idx &&

I think keeping this kind of post-condition check pretty minimal is
favorable, since this is functionality of `git repack` (i.e., that `-a`
leaves you with one kept) that is already tested thoroughly elsewhere.

So I'd probably avoid checking the output altogether, but if we did want
to test it, I think something as quick and cheap as:

    test_line_count = 1 before

would do the trick.

Thanks,
Taylor

Re: [PATCH v2 1/2] t7700: check post-condition in kept-pack test

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> On Fri, Mar 25, 2022 at 01:23:27PM -0400, Derrick Stolee wrote:
>> > Since we've made sure "before" is a one-liner earlier, we could just
>> > say
>>
>> > 		test_cmp before kept &&
>> >
>> > instead, no?
>>
>> 'before' contains a .idx name and 'kept' contains a .keep name,
>> so this direct comparison does not work. We could do that
>> additional check like this:
>>
>> 	kept_name=$(cat kept) &&
>> 	echo ${kept_name%.keep}.idx >kept-idx &&
>> 	test_cmp before kept-idx &&
>
> I think keeping this kind of post-condition check pretty minimal is
> favorable, since this is functionality of `git repack` (i.e., that `-a`
> leaves you with one kept) that is already tested thoroughly elsewhere.

It is nice in principle, but for this particular script, whose
helper function's implementation relies on these "assumptions" to
work correctly (e.g. imagine what happens when 'before' file had 0
or 2 lines in it and you called the get_sorted_objects_from_packs
helper), we should be more defensive, and that is where all my
suggestions in the thread come from.

[PATCH v2 2/2] test-lib-functions: fix test_subcommand_inexact

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

The implementation of test_subcommand_inexact() was originally
introduced in e4d0c11c0 (repack: respect kept objects with '--write-midx
-b', 2021-12-20) with the intention to allow finding a subcommand based
on an initial set of arguments. The inexactness was intended as a way to
allow flexible options beyond that initial set, as opposed to
test_subcommand() which requires that the full list of options is
provided in its entirety.

The implementation began by copying test_subcommand() and replaced the
repeated argument 'printf' statement to append ".*" instead of "," to
each argument. This has a few drawbacks:

1. Most importantly, this repeats the use of ".*" within 'expr', so the
   inexact match is even more flexible than expected. It allows the list
   of arguments to exist as a subsequence (with any other items included
   between those arguments).

2. The line 'expr="$(expr%,}"' that previously removed a trailing comma
   now no longer does anything, since the string ends with ".*".

Both of these issues are fixed by keeping the addition of the comma in
the printf statement, then adding ".*" after stripping out the trailing
comma.

All existing tests continue to pass with this change. There was one
instance from t7700-repack.sh that was taking advantage of this
flexibility, but it was removed in the previous change.

Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 t/test-lib-functions.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 0f439c99d61..8f0e5da8727 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1811,8 +1811,8 @@ test_subcommand_inexact () {
 		shift
 	fi
 
-	local expr=$(printf '"%s".*' "$@")
-	expr="${expr%,}"
+	local expr=$(printf '"%s",' "$@")
+	expr="${expr%,}.*"
 
 	if test -n "$negate"
 	then
-- 
gitgitgadget

Re: [PATCH v2 2/2] test-lib-functions: fix test_subcommand_inexact

[Taylor Blau]

On Thu, Mar 24, 2022 at 06:34:57PM +0000, Derrick Stolee via GitGitGadget wrote:
> All existing tests continue to pass with this change. There was one
> instance from t7700-repack.sh that was taking advantage of this
> flexibility, but it was removed in the previous change.

In my tree, the test modified by the previous patch was the only caller
of `test_subcommand_inexact()`. Looking through the output of:

    git for-each-ref refs/remotes/origin/ --format='%(refname)' | xargs -L1 \
      git -P grep -l test_subcommand_inexact -- t | sort -u

it doesn't look like there are any other topics in flight [1] that call
test_subcommand_inexact(), either.

Unless you have any pending series which want to call
test_subcommand_inexact, I'd be just as happy to get rid of the function
entirely.

Thanks,
Taylor

[1]: My `origin` points to Junio's tree, so I'm looking through all of
the topic branches before integration, not just the standard "master",
"next", etc.

Re: [PATCH v2 2/2] test-lib-functions: fix test_subcommand_inexact

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <derrickstolee@github.com>
>
> The implementation of test_subcommand_inexact() was originally
> introduced in e4d0c11c0 (repack: respect kept objects with '--write-midx
> -b', 2021-12-20) with the intention to allow finding a subcommand based
> on an initial set of arguments. The inexactness was intended as a way to
> allow flexible options beyond that initial set, as opposed to
> test_subcommand() which requires that the full list of options is
> provided in its entirety.
>
> The implementation began by copying test_subcommand() and replaced the
> repeated argument 'printf' statement to append ".*" instead of "," to
> each argument. This has a few drawbacks:
>
> 1. Most importantly, this repeats the use of ".*" within 'expr', so the
>    inexact match is even more flexible than expected. It allows the list
>    of arguments to exist as a subsequence (with any other items included
>    between those arguments).
>
> 2. The line 'expr="$(expr%,}"' that previously removed a trailing comma
>    now no longer does anything, since the string ends with ".*".
>
> Both of these issues are fixed by keeping the addition of the comma in
> the printf statement, then adding ".*" after stripping out the trailing
> comma.
>
> All existing tests continue to pass with this change. There was one
> instance from t7700-repack.sh that was taking advantage of this
> flexibility, but it was removed in the previous change.

Of course all existing tests continue to pass, as we no longer have
any user of test_subcommand_inexact after the previous step ;-).

Among

 (1) doing nothing,
 (2) removing, and
 (3) clarifying the implementation,

my preference would be 2 > 1 > 3.  If we add

 (4) clarify the implementation and document what kind of inexactness we
     tolerate with an updated comment"

to the mix, that would come before all 3 others, though.

Perhaps squash something like this in?

 t/test-lib-functions.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git i/t/test-lib-functions.sh w/t/test-lib-functions.sh
index 0f439c99d6..6f6afae847 100644
--- i/t/test-lib-functions.sh
+++ w/t/test-lib-functions.sh
@@ -1789,8 +1789,8 @@ test_subcommand () {
 }
 
 # Check that the given command was invoked as part of the
-# trace2-format trace on stdin, but without an exact set of
-# arguments.
+# trace2-format trace on stdin, but only require that the
+# initial arguments are given as specified.
 #
 #	test_subcommand [!] <command> <args>... < <trace>
 #

Re: [PATCH v2 2/2] test-lib-functions: fix test_subcommand_inexact

[Derrick Stolee]

On 3/24/2022 4:48 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

>> All existing tests continue to pass with this change. There was one
>> instance from t7700-repack.sh that was taking advantage of this
>> flexibility, but it was removed in the previous change.
> 
> Of course all existing tests continue to pass, as we no longer have
> any user of test_subcommand_inexact after the previous step ;-).

Yeah, I definitely should have checked to see if there were other
uses of this. I thought there was, but I was mistaken.

> Among
> 
>  (1) doing nothing,
>  (2) removing, and
>  (3) clarifying the implementation,
> 
> my preference would be 2 > 1 > 3.  If we add

I agree that (2) is the best option here.
 
>  (4) clarify the implementation and document what kind of inexactness we
>      tolerate with an updated comment"
> 
> to the mix, that would come before all 3 others, though.

Is there value in fixing the implementation and adding this comment
if we are to just delete the helper? I suppose that we could prevent
a future contribution from reintroducing the broken implementation.
 
> Perhaps squash something like this in?
> 
>  t/test-lib-functions.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git i/t/test-lib-functions.sh w/t/test-lib-functions.sh
> index 0f439c99d6..6f6afae847 100644
> --- i/t/test-lib-functions.sh
> +++ w/t/test-lib-functions.sh
> @@ -1789,8 +1789,8 @@ test_subcommand () {
>  }
>  
>  # Check that the given command was invoked as part of the
> -# trace2-format trace on stdin, but without an exact set of
> -# arguments.
> +# trace2-format trace on stdin, but only require that the
> +# initial arguments are given as specified.

This is an accurate description of what the fixed implementation
does.

My current feeling is that we should just delete this and refer
to that deletion if anyone considers needing something like it.

Thanks,
-Stolee

Re: [PATCH v2 2/2] test-lib-functions: fix test_subcommand_inexact

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> On 3/24/2022 4:48 PM, Junio C Hamano wrote:
>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>>> All existing tests continue to pass with this change. There was one
>>> instance from t7700-repack.sh that was taking advantage of this
>>> flexibility, but it was removed in the previous change.
>> 
>> Of course all existing tests continue to pass, as we no longer have
>> any user of test_subcommand_inexact after the previous step ;-).
>
> Yeah, I definitely should have checked to see if there were other
> uses of this. I thought there was, but I was mistaken.
>
>> Among
>> 
>>  (1) doing nothing,
>>  (2) removing, and
>>  (3) clarifying the implementation,
>> 
>> my preference would be 2 > 1 > 3.  If we add
>
> I agree that (2) is the best option here.
>  
>>  (4) clarify the implementation and document what kind of inexactness we
>>      tolerate with an updated comment"
>> 
>> to the mix, that would come before all 3 others, though.
>
> Is there value in fixing the implementation and adding this comment
> if we are to just delete the helper? I suppose that we could prevent
> a future contribution from reintroducing the broken implementation.

That is a good thoguth to take into account.

> My current feeling is that we should just delete this and refer
> to that deletion if anyone considers needing something like it.

I am very much in favor of deleting it.

Thanks.

[PATCH v3 0/2] test-lib-functions: fix test_subcommand_inexact

[Derrick Stolee via GitGitGadget]

Junio discovered in [1] that test_subcommand_inexact is more flexible than
initially intended.

[1] https://lore.kernel.org/git/xmqq4k41vdwe.fsf@gitster.g/

The intention was that we do not need to specify the remaining arguments for
a subcommand, but instead the current behavior is to allow the given
arguments to appear as any subsequence within the command (except that the
first "git" instance must be the first argument).

By changing the test that needed the helper, we can avoid the helper in the
first place. Modify the test and remove the helper.


Changes in v3
=============

 * Significant edits to the test in t7700 based on Junio and Taylor's
   feedback.

 * Patch 2 now deletes the helper as it is not used anywhere.

Thanks, -Stolee

Derrick Stolee (2):
  t7700: check post-condition in kept-pack test
  test-lib-functions: remove test_subcommand_inexact

 t/t7700-repack.sh       | 57 ++++++++++++++++++++++++++++++++++++++---
 t/test-lib-functions.sh | 34 ------------------------
 2 files changed, 54 insertions(+), 37 deletions(-)


base-commit: a68dfadae5e95c7f255cf38c9efdcbc2e36d1931
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1185%2Fderrickstolee%2Ftest-subcommand-fix-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1185/derrickstolee/test-subcommand-fix-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1185

Range-diff vs v2:

 1:  f2f8d12929b ! 1:  fb2c550512e t7700: check post-condition in kept-pack test
     @@ t/t7700-repack.sh: test_expect_success '--write-midx with preferred bitmap tips'
       	)
       '
       
     -+get_sorted_objects_from_packs () {
     -+	git show-index <$(cat) >raw &&
     -+	cut -d" " -f2 raw | sort
     ++# The first argument is expected to be a filename
     ++# and that file should contain the name of a .idx
     ++# file. Send the list of objects in that .idx file
     ++# into stdout.
     ++get_sorted_objects_from_pack () {
     ++	git show-index <$(cat "$1") >raw &&
     ++	cut -d" " -f2 raw
      +}
      +
       test_expect_success '--write-midx -b packs non-kept objects' '
     @@ t/t7700-repack.sh: test_expect_success '--write-midx with preferred bitmap tips'
      +		test_commit base &&
      +		git repack -ad &&
      +		find $objdir/pack -name "*.idx" >before &&
     -+		>$objdir/pack/$(basename $(cat before) .idx).keep &&
     ++		test_line_count = 1 before &&
     ++		before_name=$(cat before) &&
     ++		>${before_name%.idx}.keep &&
      +
      +		# Create a non-kept pack-file
      +		test_commit other &&
     @@ t/t7700-repack.sh: test_expect_success '--write-midx with preferred bitmap tips'
      +		find $objdir/pack -name "*.idx" | sort >after &&
      +		test_line_count = 2 after &&
      +		find $objdir/pack -name "*.keep" >kept &&
     -+		test_line_count = 1 kept &&
     ++		kept_name=$(cat kept) &&
     ++		echo ${kept_name%.keep}.idx >kept-idx &&
     ++		test_cmp before kept-idx &&
      +
      +		# Get object list from the kept pack.
     -+		get_sorted_objects_from_packs \
     -+			<before \
     -+			>old.objects &&
     ++		get_sorted_objects_from_pack before >old.objects &&
      +
      +		# Get object list from the one non-kept pack-file
      +		comm -13 before after >new-pack &&
     -+		get_sorted_objects_from_packs \
     -+			<new-pack \
     -+			>new.objects &&
     ++		test_line_count = 1 new-pack &&
     ++		get_sorted_objects_from_pack new-pack >new.objects &&
      +
      +		# None of the objects in the new pack should
      +		# exist within the kept pack.
 2:  ed67b748971 < -:  ----------- test-lib-functions: fix test_subcommand_inexact
 -:  ----------- > 2:  f5a96a121a5 test-lib-functions: remove test_subcommand_inexact

-- 
gitgitgadget

[PATCH v3 1/2] t7700: check post-condition in kept-pack test

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

The '--write-midx -b packs non-kept objects' test in t7700-repack.sh
uses test_subcommand_inexact to check that 'git repack' properly adds
the '--honor-pack-keep' flag to the 'git pack-objects' subcommand.
However, the test_subcommand_inexact helper is more flexible than
initially designed, and this instance is the only one that makes use of
it: there are additional arguments between 'git pack-objects' and the
'--honor-pack-keep' flag. In order to make test_subcommand_inexact more
strict, we need to fix this instance.

This test checks that 'git repack --write-midx -a -b -d' will create a
new pack-file that does not contain the objects within the kept pack.
This behavior is possible because of the multi-pack-index bitmap that
will bitmap objects against multiple packs. Without --write-midx, the
objects in the kept pack would be duplicated so the resulting pack is
closed under reachability and bitmaps can be created against it. This is
discussed in more detail in e4d0c11c0 (repack: respect kept objects with
'--write-midx -b', 2021-12-20) which also introduced this instance of
test_subcommand_inexact.

To better verify the intended post-conditions while also removing this
instance of test_subcommand_inexact, rewrite the test to check the list
of packed objects in the kept pack and the list of the objects in the
newly-repacked pack-file _other_ than the kept pack. These lists should
be disjoint.

Be sure to include a non-kept pack-file and loose objects to be extra
careful that this is properly behaving with kept packs and not just
avoiding repacking all pack-files.

Co-authored-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 t/t7700-repack.sh | 57 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 3 deletions(-)

diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 770d1432046..ca45c4cd2c1 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -369,10 +369,61 @@ test_expect_success '--write-midx with preferred bitmap tips' '
 	)
 '
 
+# The first argument is expected to be a filename
+# and that file should contain the name of a .idx
+# file. Send the list of objects in that .idx file
+# into stdout.
+get_sorted_objects_from_pack () {
+	git show-index <$(cat "$1") >raw &&
+	cut -d" " -f2 raw
+}
+
 test_expect_success '--write-midx -b packs non-kept objects' '
-	GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
-		git repack --write-midx -a -b &&
-	test_subcommand_inexact git pack-objects --honor-pack-keep <trace.txt
+	git init repo &&
+	test_when_finished "rm -fr repo" &&
+	(
+		cd repo &&
+
+		# Create a kept pack-file
+		test_commit base &&
+		git repack -ad &&
+		find $objdir/pack -name "*.idx" >before &&
+		test_line_count = 1 before &&
+		before_name=$(cat before) &&
+		>${before_name%.idx}.keep &&
+
+		# Create a non-kept pack-file
+		test_commit other &&
+		git repack &&
+
+		# Create loose objects
+		test_commit loose &&
+
+		# Repack everything
+		git repack --write-midx -a -b -d &&
+
+		# There should be two pack-files now, the
+		# old, kept pack and the new, non-kept pack.
+		find $objdir/pack -name "*.idx" | sort >after &&
+		test_line_count = 2 after &&
+		find $objdir/pack -name "*.keep" >kept &&
+		kept_name=$(cat kept) &&
+		echo ${kept_name%.keep}.idx >kept-idx &&
+		test_cmp before kept-idx &&
+
+		# Get object list from the kept pack.
+		get_sorted_objects_from_pack before >old.objects &&
+
+		# Get object list from the one non-kept pack-file
+		comm -13 before after >new-pack &&
+		test_line_count = 1 new-pack &&
+		get_sorted_objects_from_pack new-pack >new.objects &&
+
+		# None of the objects in the new pack should
+		# exist within the kept pack.
+		comm -12 old.objects new.objects >shared.objects &&
+		test_must_be_empty shared.objects
+	)
 '
 
 test_expect_success TTY '--quiet disables progress' '
-- 
gitgitgadget

[PATCH v3 2/2] test-lib-functions: remove test_subcommand_inexact

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <derrickstolee@github.com>

The implementation of test_subcommand_inexact() was originally
introduced in e4d0c11c0 (repack: respect kept objects with '--write-midx
-b', 2021-12-20) with the intention to allow finding a subcommand based
on an initial set of arguments. The inexactness was intended as a way to
allow flexible options beyond that initial set, as opposed to
test_subcommand() which requires that the full list of options is
provided in its entirety.

The implementation began by copying test_subcommand() and replaced the
repeated argument 'printf' statement to append ".*" instead of "," to
each argument. This caused it to be more flexible than initially
intended.

The previous change deleted the only use of test_subcommand_inexact, so
instead of editing the helper, delete it.

Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 t/test-lib-functions.sh | 34 ----------------------------------
 1 file changed, 34 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 0f439c99d61..2501fc5706f 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1788,40 +1788,6 @@ test_subcommand () {
 	fi
 }
 
-# Check that the given command was invoked as part of the
-# trace2-format trace on stdin, but without an exact set of
-# arguments.
-#
-#	test_subcommand [!] <command> <args>... < <trace>
-#
-# For example, to look for an invocation of "git pack-objects"
-# with the "--honor-pack-keep" argument, use
-#
-#	GIT_TRACE2_EVENT=event.log git repack ... &&
-#	test_subcommand git pack-objects --honor-pack-keep <event.log
-#
-# If the first parameter passed is !, this instead checks that
-# the given command was not called.
-#
-test_subcommand_inexact () {
-	local negate=
-	if test "$1" = "!"
-	then
-		negate=t
-		shift
-	fi
-
-	local expr=$(printf '"%s".*' "$@")
-	expr="${expr%,}"
-
-	if test -n "$negate"
-	then
-		! grep "\"event\":\"child_start\".*\[$expr\]"
-	else
-		grep "\"event\":\"child_start\".*\[$expr\]"
-	fi
-}
-
 # Check that the given command was invoked as part of the
 # trace2-format trace on stdin.
 #
-- 
gitgitgadget

Re: [PATCH v3 0/2] test-lib-functions: fix test_subcommand_inexact

[Taylor Blau]

On Fri, Mar 25, 2022 at 07:02:45PM +0000, Derrick Stolee via GitGitGadget wrote:
> Junio discovered in [1] that test_subcommand_inexact is more flexible than
> initially intended.
>
> [1] https://lore.kernel.org/git/xmqq4k41vdwe.fsf@gitster.g/
>
> The intention was that we do not need to specify the remaining arguments for
> a subcommand, but instead the current behavior is to allow the given
> arguments to appear as any subsequence within the command (except that the
> first "git" instance must be the first argument).
>
> By changing the test that needed the helper, we can avoid the helper in the
> first place. Modify the test and remove the helper.

I reviewed both patches carefully, and this version looks great to me.
Thanks for all of your patience in cleaning up that sketch of the new
test; you version in v3 here is delightfully easy to follow.

    Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor