test-lib-functions: fix test_subcommand_inexact

[derrickstolee]

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

cc: gitster@pobox.com
cc: chakrabortyabhradeep79@gmail.com
cc: Taylor Blau me@ttaylorr.com

[derrickstolee]

/submit

[gitgitgadget]

Submitted as pull.1185.git.1647894845421.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1185/derrickstolee/test-subcommand-fix-v1

To fetch this version to local tag pr-1185/derrickstolee/test-subcommand-fix-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1185/derrickstolee/test-subcommand-fix-v1

[gitgitgadget]

This branch is now known as ds/fix-test-subcommand-inexact.

[gitgitgadget]

This patch series was integrated into seen via git@2823f7b.

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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 ;-)?

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Taylor Blau wrote (reply to this):

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

[gitgitgadget]

User Taylor Blau <me@ttaylorr.com> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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.

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Taylor Blau wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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 ;-)

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Abhradeep Chakraborty wrote (reply to this):

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 :)

[derrickstolee]

/submit

[gitgitgadget]

Submitted as pull.1185.v2.git.1648146897.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1185/derrickstolee/test-subcommand-fix-v2

To fetch this version to local tag pr-1185/derrickstolee/test-subcommand-fix-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1185/derrickstolee/test-subcommand-fix-v2

[gitgitgadget]

On the Git mailing list, Taylor Blau wrote (reply to this):

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.

[gitgitgadget]

On the Git mailing list, Taylor Blau wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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>
 #

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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.

[gitgitgadget]

This branch is now known as ds/t7700-kept-pack-test.

[gitgitgadget]

This patch series was integrated into seen via git@efcf525.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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.

[gitgitgadget]

This patch series was integrated into seen via git@0d6d19f.

[gitgitgadget]

On the Git mailing list, Abhradeep Chakraborty wrote (reply to this):

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 :)

[derrickstolee]

/submit

[gitgitgadget]

Submitted as pull.1185.v3.git.1648234967.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1185/derrickstolee/test-subcommand-fix-v3

To fetch this version to local tag pr-1185/derrickstolee/test-subcommand-fix-v3:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1185/derrickstolee/test-subcommand-fix-v3

[gitgitgadget]

This patch series was integrated into seen via git@5ff6174.

[gitgitgadget]

This patch series was integrated into seen via git@aa6d166.

[gitgitgadget]

This patch series was integrated into seen via git@22cb3c2.

[gitgitgadget]

There was a status update in the "New Topics" section about the branch ds/t7700-kept-pack-test on the Git mailing list:

Test clean-up.

Will merge to 'next'.
source: <pull.1185.v3.git.1648234967.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@b193d33.

[gitgitgadget]

This patch series was integrated into next via git@fb03a86.

[gitgitgadget]

On the Git mailing list, Taylor Blau wrote (reply to this):

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