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/ ? > Helped-by: Junio Hamano <gitster@xxxxxxxxx> > Signed-off-by: Elijah Newren <newren@xxxxxxxxx> This could probably use a Reported-by: Lessley Dennington <lessleydennington@xxxxxxxxx> > +static void sanitize_paths(int argc, const char **argv, const char *prefix) > +{ > + if (!argc) > + return; > + > + if (prefix && *prefix) { > + /* > + * The args are not pathspecs, so unfortunately we > + * cannot imitate how cmd_add() uses parse_pathspec(). > + */ > + int i; > + int prefix_len = strlen(prefix); > + > + for (i = 0; i < argc; i++) > + argv[i] = prefix_path(prefix, prefix_len, argv[i]); > + } > +} > + > static char const * const builtin_sparse_checkout_add_usage[] = { > N_("git sparse-checkout add (--stdin | <patterns>)"), > NULL > @@ -708,6 +726,8 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix) > builtin_sparse_checkout_add_usage, > PARSE_OPT_KEEP_UNKNOWN); > > + sanitize_paths(argc, argv, prefix); > + > return modify_pattern_list(argc, argv, add_opts.use_stdin, ADD); > } > > @@ -759,6 +779,8 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix) > if (!core_sparse_checkout_cone && argc == 0) { > argv = default_patterns; > argc = default_patterns_nr; > + } else { > + sanitize_paths(argc, argv, prefix); > } Code changes appear to do as described: apply the prefix everywhere it matters, no matter the mode. > +test_expect_success 'set from subdir pays attention to prefix' ' > + git -C repo sparse-checkout disable && > + git -C repo/deep sparse-checkout set --cone deeper2 ../folder1 && > + > + git -C repo sparse-checkout list >actual && > + > + cat >expect <<-\EOF && > + deep/deeper2 > + folder1 > + EOF > + test_cmp expect actual > +' > + > +test_expect_success 'add from subdir pays attention to prefix' ' > + git -C repo sparse-checkout set --cone deep/deeper2 && > + git -C repo/deep sparse-checkout add deeper1/deepest ../folder1 && > + > + git -C repo sparse-checkout list >actual && > + > + cat >expect <<-\EOF && > + deep/deeper1/deepest > + deep/deeper2 > + folder1 > + EOF > + test_cmp expect actual > +' > + > test_done These tests could use a non-cone-mode version to demonstrate the behavior in that mode. Thanks, -Stolee
