Re: [PATCH 3/7] sparse-checkout: pay attention to prefix for {set, add}

[Derrick Stolee <derrickstolee@github.com>]

On 2/14/2022 10:52 PM, Elijah Newren wrote:
> On Mon, Feb 14, 2022 at 7:50 AM Derrick Stolee <derrickstolee@github.com> wrote:
>>
>> On 2/12/2022 7:39 PM, Elijah Newren via GitGitGadget wrote:
>>> From: Elijah Newren <newren@gmail.com>
>>>
>>> In cone mode, non-option arguments to set & add are clearly paths, and
>>> as such, we should pay attention to prefix.
>>>
>>> In non-cone mode, it is not clear that folks intend to provide paths
>>> since the inputs are gitignore-style patterns.  Paying attention to
>>> prefix would prevent folks from doing things like
>>>    git sparse-checkout add /.gitattributes
>>>    git sparse-checkout add '/toplevel-dir/*'
>>> In fact, the former will result in
>>>    fatal: '/.gitattributes' is outside repository...
>>> while the later will result in
>>>    fatal: Invalid path '/toplevel-dir': No such file or directory
>>> despite the fact that both are valid gitignore-style patterns that would
>>> select real files if added to the sparse-checkout file.  However, these
>>> commands can be run successfully from the toplevel directory, and many
>>> gitignore-style patterns ARE paths, and bash completion seems to be
>>> suggesting directories and files, so perhaps for consistency we pay
>>> attention to the prefix?  It's not clear what is okay here, but maybe
>>> that's yet another reason to deprecate non-cone mode as we will do later
>>> in this series.
>>>
>>> For now, incorporate prefix into the positional arguments for either
>>> cone or non-cone mode.  For additional discussion of this issue, see
>>> https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/
>>
>> Perhaps this was covered in the issue, but for non-cone mode, it
>> matters if there is a leading slash or not in the pattern. Will
>> this change make it impossible for a user to input that distinction?
>>
>> Will there still be a difference between:
>>
>>         git sparse-checkout set --no-cone /.vs/
>>
>> and
>>
>>         git sparse-checkout set --no-cone .vs/
>>
>> ?
> 
> If you are in the toplevel directory, you can run either of these and
> they have the same meaning they traditionally had.
> 
> Before this patch, if you are in a subdirectory, the first of those
> would have specified a toplevel ".vs" directory, and the second would
> have specified a ".vs/" directory in the toplevel OR any subdirectory.
> Those choices might be what the user wanted, or both of those could be
> a nasty surprise for the user.
> 
> After this patch, if you are in a subdirectory, the first of those
> throw an error:
>     $ git sparse-checkout set --no-cone /.vs/
>     fatal: Invalid path '/.vs': No such file or directory
> (which might be an annoyance, but how would you possibly specify a
> leading slash on a path that needs to be prefixed anyway?)  The second
> will specify a SUBDIR/.vs/ from the toplevel directory (which again,
> might be what the user wanted, or might be a nasty surprise if they
> were trying to specify a pattern relative to the root).
> 
> Does this change make sense?  For some users, sure -- especially those
> with the idea that you specify paths for non-cone mode (though
> bash-completion may guide folks to presume that).  But for those who
> understand that non-cone mode is all about patterns and that we have a
> single toplevel file where everything must be recorded, it's possibly
> detrimental to them.  To me, I wonder if it seems fraught with nasty
> surprises for us to do anything other than throw an error when
> --no-cone is specified and we are in a subdirectory.  Perhaps I should
> do that instead of this change here.

I'd be in favor of this second approach of requiring the base directory.

>>> Helped-by: Junio Hamano <gitster@pobox.com>
>>> Signed-off-by: Elijah Newren <newren@gmail.com>
>>
>> This could probably use a
>>
>>   Reported-by: Lessley Dennington <lessleydennington@gmail.com>
> 
> It'd be more of a "Report-Formalized-by:" if we were to include such a
> tag.  Check the history here:
> https://lore.kernel.org/git/52d638fc-e7e7-1b0a-482b-cff7c9500b92@gmail.com/
> 
> In short: I was the original reporter; I noted the issue while
> reviewing her completion series.  The bug was not related to her
> series, but her series did prompt me to check and discover the issue.
> She didn't want the issue to get lost, and decided to make a formal
> report.

That makes sense. I wasn't caught up with that conversation.

>> These tests could use a non-cone-mode version to demonstrate the behavior
>> in that mode.
> 
> Fair enough, though I hesitated in part because I wasn't sure we even
> wanted to make that change, and I figured getting that answer might be
> useful before writing the tests.

Understandable.

Thanks,
-Stolee