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

From: Derrick Stolee <derrickstolee@github.com>
Date: Mar 25, 2022, 14:03 UTC
Message-ID: <fc1db75c-868c-08c6-7087-b70460653460@github.com>
In-Reply-To: <xmqqtubnf568.fsf@gitster.g>

On 3/24/2022 4:48 PM, Junio C Hamano wrote:

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

Show 6 quoted lines

>> 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.

Show 7 quoted 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.

>  (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.

Show 17 quoted lines

> 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