On 3/13/2022 3:41 PM, Junio C Hamano wrote:
> "Abhradeep Chakraborty via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> writes:
>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
>> index 0f439c99d61..07a2b60c103 100644
>> --- a/t/test-lib-functions.sh
>> +++ b/t/test-lib-functions.sh
>> @@ -1811,7 +1811,7 @@ test_subcommand_inexact () {
>> shift
>> fi
>>
>> - local expr=$(printf '"%s".*' "$@")
>> + local expr=$(printf '.*"%s".*' "$@")
>
> The original wanted to make sure that the arguments to the helper
> are initial items of a comma separated list, and an existing caller,
> for example, i.e.
>
> test_subcommand_inexact git pack-objects --honor-pack-keep <trace
>
> is relying on the behaviour to make sure 'git', 'pack-objects', ...
> appear at the beginning of "[...]" enclosed list. This change
> breaks its ability to notice that an insertion of unrelated token
> before 'git' as an error.
>
> In other words, it looks like an uncalled-for selfish change.
This change was my recommendation, but I see that it is probably
not ideal.
> Why can't you specify what should NOT come before "fetch" in your
> use of this helper?
>
>> expr="${expr%,}"
>
> The preimage already has this problem, but the stripping of trailing
> comma here is a result of mistaken copy-and-paste from the exact
> variant, I think. test_subcommand uses
>
> local expr=$(printf '"%s",' "$@")
>
> to concatenate "$@" into a single comma-separated string, so it
> perfectly makes sense to drop the last one here, but with or without
> your change here, neither is adding a comma that need to be
> stripped.
>
>
> It is not _your_ theme, but I think this helper is poorly designed,
> especially compared to the original "exact" variant.
>
> 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
> }
>
>
> I've already touched that "${expr%,}" there is a totally useful
> statement that will always be a no-op.
>
> When "test_subcommand_inexact git pack-objects" is run, the printf
> assigns to $expr:
>
> expr='"git".*"pack-objects".*'
>
> and the actual grep command invoked becomes
>
> grep '"event":"child_start".*\["git".*"pack-objects".*\]'
>
> I am not sure if that is what we really want.
Ah, yes this certainly seems to not be the expected plan. It does
allow for more flexibility than intended: the intention was to
add flexibility at the end of the command, but instead adds
flexibility throughout, only caring that a certain list of options
is present as a subsequence (except that the first item is the
first item, namely "git" in most cases).
That unintended flexibility would allow the current needs to use
test_subcommand_inexact ! git fetch
as desired, but there is the additional worries about whether it
is too flexible for the existing uses.
> I wonder if it was more like this that the original wanted to grep for:
>
> grep '"event":"child_start".*\["git","pack-objects",.*\]'
>
> in which case the two lines there should be more like
>
> local expr=$(printf '"%s",' "$@")
> expr="${expr%,}.*"
>
> I would think. This comes from Derrick's e4d0c11c (repack: respect
> kept objects with '--write-midx -b', 2021-12-20).
Yep, that was definitely the intention, but I wrote it wrong.
I'm torn between "let's fix it to work as intended and do something
different for this test case" and "this flexibility is unexpected,
but still gives us enough information to trust the tests."
If you think that we should fix the helper to work differently, then
I can work on a patch to do so, so Abhradeep doesn't get too
sidetracked on that.
Thanks,
-Stolee
