Hi, On Mon, Mar 27, 2023 at 12:55 AM William Sprent via GitGitGadget <gitgitgadget@xxxxxxxxx> 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@xxxxxxxxx> Thanks for sending this in, William!
