test-lib-functions: fix test_subcommand_inexact
PATCH 1/1 test-lib-functions: fix test_subcommand_inexact
From: Derrick Stolee <[email protected]>
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.
---
test-lib-functions: fix test_subcommand_inexact
This fixes a concern discovered by Junio in [1].
[1] https://lore.kernel.org/git/[email protected]/
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(-)
t/test-lib-functions.sh +2 -2
@@ -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
Signature · 1 line
gitgitgadget
Quoted 22 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
The implementation of
test_subcommand_inexact()was originally introduced ine4d0c11c0(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 totest_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 ".*".
Quoted 6 lines
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
Quoted 10 lines · Derrick Stolee
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_inexacthelper.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 ;-)?
Quoted 26 lines · Junio C Hamano
Quoted 10 lines · Derrick Stolee
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_inexacthelper.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.
Quoted 8 lines
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
Quoted 5 lines · Derrick Stolee
Quoted 1 line
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< ---
t/t7700-repack.sh +25 -3
@@ -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
Quoted 7 lines · Taylor Blau
Quoted 5 lines · Derrick Stolee
Quoted 1 line
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.
Quoted 8 lines
I wonder if we could sidestep the whole issue with
test_subcommand_inexactby 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.
Quoted 18 lines · Junio C Hamano
Quoted 7 lines · Taylor Blau
Quoted 5 lines · Derrick Stolee
Quoted 1 line
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.
Quoted 8 lines
I wonder if we could sidestep the whole issue with
test_subcommand_inexactby 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.
Quoted 6 lines
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
Quoted 5 lines · Derrick Stolee
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< ---
t/t7700-repack.sh +29 -3
@@ -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
Quoted 10 lines · Taylor Blau
Quoted 5 lines · Derrick Stolee
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!
Quoted 6 lines
t/t7700-repack.sh +6
-0+ test_commit kept && + git repack -ad && + + >$packdir/$(basename $packdir/pack-*.pack .pack).keep && + + test_commit unkept &&
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 395
Quoted 10 lines
t/t7700-repack.sh +10
-0+ 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.
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 401
Quoted 6 lines
t/t7700-repack.sh +6
-0+ + 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
Quoted 10 lines · Derrick Stolee
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 ;-)
Quoted 6 lines · Junio C Hamano
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 :)
Quoted 4 lines · Abhradeep Chakraborty
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.
Quoted 3 lines · Junio C Hamano
... 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 :)
v2 cover test-lib-functions: fix test_subcommand_inexact
In reply to Derrick Stolee via GitGitGadgetJunio discovered in [1] that test_subcommand_inexact is more flexible than initially intended.
[1] https://lore.kernel.org/git/[email protected]/
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 <[email protected]>
## t/test-lib-functions.sh ##
Signature · 1 line
gitgitgadget
PATCH v2 1/2 t7700: check post-condition in kept-pack test
From: Derrick Stolee <[email protected]>
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.
--- t/t7700-repack.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 3 deletions(-)
t/t7700-repack.sh +49 -3
@@ -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' '
Signature · 1 line
gitgitgadget
Re: [PATCH v2 1/2] t7700: check post-condition in kept-pack test
Quoted 27 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
The '--write-midx -b packs non-kept objects' test in
t7700-repack.shusestest_subcommand_inexactto check that 'git repack' properly adds the '--honor-pack-keep' flag to the 'git pack-objects' subcommand. However, thetest_subcommand_inexacthelper 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 maketest_subcommand_inexactmore 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 oftest_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.
Quoted 3 lines
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.
Quoted 2 lines
Co-authored-by Taylor Blau <[email protected]>Signed-off-by Taylor Blau <[email protected]>
And to be clear, I am totally OK with this usage of my signed-off-by.
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 373
Quoted 10 lines
t/t7700-repack.sh +2
-0@@ -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.
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 374
Quoted 1 line
t/t7700-repack.sh +1
-0+ 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.
Quoted 5 lines
t/t7700-repack.sh +2
-2+} + test_expect_success '--write-midx -b packs non-kept objects' ' - GIT_TRACE2_EVENT="$(pwd)/trace.txt" \ - git repack --write-midx -a -b &&
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 386
Quoted 10 lines
t/t7700-repack.sh +9
-1- 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")"
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 387
Quoted 1 line
t/t7700-repack.sh +1
-0+ >$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.
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 412
Quoted 2 lines
t/t7700-repack.sh +2
-0+ # 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.
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 391
Quoted 2 lines
t/t7700-repack.sh +2
-0+ 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
Quoted 11 lines · Taylor Blau
Quoted 10 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
t/t7700-repack.sh +0
-0@@ -369,10 +369,56 @@ test_expect_success '--write-midx with preferred bitmap tips' ' ) '
Quoted 6 lines
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 373
Quoted 2 lines
t/t7700-repack.sh +2
-0+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_packsover stdin. I probably would have expected something likegit 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.
Quoted 2 lines
We may also want to s/packs/pack, since this function only will handle one index at a time.
Yes.
Quoted 5 lines
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 374
Quoted 1 line
t/t7700-repack.sh +1
-0+ cut -d" " -f2 raw | sortHaving 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.
Quoted 18 lines
Quoted 5 lines
t/t7700-repack.sh +2
-2+} + test_expect_success '--write-midx -b packs non-kept objects' ' - GIT_TRACE2_EVENT="$(pwd)/trace.txt" \ - git repack --write-midx -a -b &&t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 386
Quoted 10 lines
t/t7700-repack.sh +9
-1- 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:
Quoted 8 lines
before="$(find $objdir/pack -name "*.idx")"t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 387
Quoted 1 line
t/t7700-repack.sh +1
-0+ >$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.
Quoted 7 lines
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 412
Quoted 2 lines
t/t7700-repack.sh +2
-0+ # 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.
Quoted 6 lines
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 391
Quoted 2 lines
t/t7700-repack.sh +2
-0+ 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_packswill 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
In reply to Derrick Stolee via GitGitGadgett/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 375
Quoted 4 lines · Derrick Stolee via GitGitGadget
t/t7700-repack.sh +4
-0+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".
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 387
Quoted 11 lines
test_expect_success'--write-midx -b packs non-kept objects' 't/t7700-repack.sh +10
-0+ 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"Quoted 4 lines
t/t7700-repack.sh +4
-0+ # Create a non-kept pack-file + test_commit other && + git repack && +
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 402
Quoted 10 lines
t/t7700-repack.sh +10
-0+ # 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.
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 404
Quoted 2 lines
t/t7700-repack.sh +2
-0+ 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?
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 409
Quoted 4 lines
t/t7700-repack.sh +4
-0+ # Get object list from the kept pack. + get_sorted_objects_from_packs \ + <before \ + >old.objects &&
OK.
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 412
Quoted 2 lines
t/t7700-repack.sh +2
-0+ # 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 &&
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 415
Quoted 3 lines
t/t7700-repack.sh +3
-0+ get_sorted_objects_from_packs \ + <new-pack \ + >new.objects &&
OK.
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 419
Quoted 3 lines
t/t7700-repack.sh +3
-0+ # None of the objects in the new pack should + # exist within the kept pack. + comm -12 old.objects new.objects >shared.objects &&
Great.
Quoted 5 lines
t/t7700-repack.sh +2
-0+ 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
Quoted 14 lines · Junio C Hamano
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 375
Quoted 4 lines · Derrick Stolee via GitGitGadget
t/t7700-repack.sh +4
-0+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.
Quoted 15 lines
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 387
Quoted 11 lines
test_expect_success'--write-midx -b packs non-kept objects' 't/t7700-repack.sh +10
-0+ 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.
Quoted 3 lines
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.
Quoted 9 lines
t/t7700-repack.sh test_expect_success '--write-midx with preferred bitmap tips' ' · line 404
Quoted 2 lines
t/t7700-repack.sh +2
-0+ 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
Quoted 7 lines · Derrick Stolee
Quoted 6 lines
Since we've made sure "before" is a one-liner earlier, we could just say
test_cmp before kept &&instead, no?
Quoted 7 lines
'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
Quoted 12 lines · Taylor Blau
Quoted 11 lines · Derrick Stolee
Quoted 6 lines
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:
Quoted 7 lines
Quoted 3 lines
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-aleaves 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
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
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.
--- t/test-lib-functions.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
t/test-lib-functions.sh +2 -2
@@ -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
Signature · 1 line
gitgitgadget
Quoted 3 lines · Derrick Stolee via GitGitGadget
All existing tests continue to pass with this change. There was one instance from
t7700-repack.shthat 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.
Quoted 22 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
The implementation of
test_subcommand_inexact()was originally introduced ine4d0c11c0(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 totest_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 ".*".
Quoted 7 lines
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.shthat 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(-)
t/test-lib-functions.sh +2 -2
@@ -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> #
Quoted 8 lines · Junio C Hamano
Quoted 3 lines · Derrick Stolee via GitGitGadget
All existing tests continue to pass with this change. There was one instance from
t7700-repack.shthat 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_inexactafter 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.
Quoted 7 lines
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.
Quoted 4 lines
(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.
Quoted 10 lines
Perhaps squash something like this in?
t/test-lib-functions.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)t/test-lib-functions.sh +0
-0
t/test-lib-functions.sh test_subcommand () { · line 1793
Quoted 7 lines
t/test-lib-functions.sh +2
-2@@ -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
Quoted 23 lines · Derrick Stolee
Quoted 8 lines · Junio C Hamano
Quoted 3 lines · Derrick Stolee via GitGitGadget
All existing tests continue to pass with this change. There was one instance from
t7700-repack.shthat 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_inexactafter 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.
Quoted 7 lines
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.
Quoted 8 lines
Quoted 4 lines
(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.
Quoted 2 lines
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.
v3 cover test-lib-functions: fix test_subcommand_inexact
In reply to Derrick Stolee via GitGitGadgetJunio discovered in [1] that test_subcommand_inexact is more flexible than initially intended.
[1] https://lore.kernel.org/git/[email protected]/
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_inexactSignature · 1 line
gitgitgadget
PATCH v3 1/2 t7700: check post-condition in kept-pack test
From: Derrick Stolee <[email protected]>
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.
--- t/t7700-repack.sh | 57 ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 3 deletions(-)
t/t7700-repack.sh +54 -3
@@ -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' '
Signature · 1 line
gitgitgadget
PATCH v3 2/2 test-lib-functions: remove test_subcommand_inexact
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
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.
--- t/test-lib-functions.sh | 34 ---------------------------------- 1 file changed, 34 deletions(-)
t/test-lib-functions.sh +0 -34
@@ -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. #
Signature · 1 line
gitgitgadget
Quoted 5 lines · Derrick Stolee via GitGitGadget
Junio discovered in [1] that
test_subcommand_inexactis more flexible than initially intended.
Quoted 7 lines
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.
Thanks,
Taylor