| Message ID | ed67b7489719a01c88d7a6765e7499c1157da32e.1648146897.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-lib-functions: fix test_subcommand_inexact | expand |
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.
"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> #
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
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.
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 0f439c99d61..8f0e5da8727 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -1811,8 +1811,8 @@ test_subcommand_inexact () { shift fi - local expr=$(printf '"%s".*' "$@") - expr="${expr%,}" + local expr=$(printf '"%s",' "$@") + expr="${expr%,}.*" if test -n "$negate" then