Re: [PATCH v2 0/2] builtin/sparse-checkout: add check-rules command

[Elijah Newren <newren@gmail.com>]

Hi,

On Mon, Mar 27, 2023 at 12:55 AM William Sprent via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> Hi
>
> This v2 addresses comments from Elijah's review comments.
>
> There's one thing worth highlighting. Elijah pointed out that the the
> "check-rules cone mode is default" test would be stronger if the test itself
> started with a 'git sparse-checkout set --no-cone' to explicitly test that
> the default interpretation of the rules passed with the '--rules-file'
> option is cone mode even though the current checkout is non-cone. I
> implemented this and it exposed that the option did not actually behave that
> way, and that the test only verified the default behaviour of a bare
> repository.
>
> I've modified the logic of the '--rules-file' option such that it defaults
> to cone mode unless combined with '--no-cone', and I've added a line to the
> documentation to make this more explicit.
>
> The alternative would be to have '--rules-file' assume non cone mode when in
> a non cone mode checkout, but I think this depends a bit on "how deprecated"
> non-cone mode is vs. how important it is to have the option behave
> consistently with 'sparse-checkout set' (which respects the current
> checkout).

Ooh, nice catch.

Given that the whole point of check-rules and --rules-file is to
determine how a _different_ sparse checkout would behave, I agree with
your decision here that the current sparse checkout should not affect
how --rules-file functions.

>
> Changes since v1:
>
>  * Explicitly state in documentation that '--rules-file' expects newline
>    separated rules.
>  * Explicitly state in documentation that '-z' does not affect the
>    '--rules-file' input.
>  * Fixup typo where 'When called with the --rules-file <file> flag' was
>    missing "flag".
>  * Fixup behaviour in 'check-rules --rules-file', such that it defaults to
>    accepting cone mode patterns when in a non cone checkout.
>  * Remember to release string buffers in 'check_rules()'.
>  * Explicitly state in documentation that '--rules-file' defaults to cone
>    mode unless combined with '--no-cone'.
>  * Better test that the default of '--rules-file' is to expect '--cone-mode'
>    by running 'check-rules' in a non-cone mode checkout.

This round addresses all my issues, and I agree with the other change
made in this round.  So, v2 is:

Reviewed-by: Elijah Newren <newren@gmail.com>

Thanks for sending this in, William!