On 2/14/2022 10:52 PM, Elijah Newren wrote: > On Mon, Feb 14, 2022 at 7:50 AM Derrick Stolee <derrickstolee@xxxxxxxxxx> wrote: >> >> On 2/12/2022 7:39 PM, Elijah Newren via GitGitGadget wrote: >>> From: Elijah Newren <newren@xxxxxxxxx> >>> >>> 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@xxxxxxxxx/ >> >> 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@xxxxxxxxx> >>> Signed-off-by: Elijah Newren <newren@xxxxxxxxx> >> >> This could probably use a >> >> Reported-by: Lessley Dennington <lessleydennington@xxxxxxxxx> > > 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@xxxxxxxxx/ > > 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
