| Message ID | 5e27cad17a7080170f476684610391bd56024f36.1644712798.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: sparse checkout: make --cone mode the default, and check add/set argument validity | expand |
On 2/12/2022 7:39 PM, Elijah Newren via GitGitGadget wrote: > From: Elijah Newren <newren@gmail.com> > > The set and add subcommands accept multiple positional arguments. > The meaning of these arguments differs slightly in the two modes: > > Cone mode only accepts directories. If given a file, it would > previously treat it as a directory, causing not just the file itself to > be included but all sibling files as well -- likely against users' > expectations. Throw an error if the specified path is a file in the > index. Provide a --skip-checks argument to allow users to override > (e.g. for the case when the given path IS a directory on another > branch). I agree that this is likely to be an improvement for users. The sparse-checkout builtin isn't integrated with the sparse index yet. At least not integrated upstream: we have commits in microsoft/git that we plan to send when other things in flight are merged. This change likely introduces a new opportunity for the index to expand, so I will keep that in mind when upstreaming. > Non-cone mode accepts general gitignore patterns. However, it has an > O(N*M) performance baked into its design, where all N index files must > be matched against all M sparse-checkout patterns with EVERY call to > unpack_trees() that updates the working tree. As such, it is important > to keep the number of patterns small, and thus we should warn users to > prefer passing directories and more generic glob patterns to get the > paths they want instead of listing each individual file. (The > --skip-checks argument can also be used to bypass this warning.) Also, > even when users do want to specify individual files, they will often > want to do so by providing a leading '/' (to avoid selecting the same > filename in all subdirectories), in which case this error message would > never trigger anyway. I think the case of "I want only one file from this directory" and "I want files with the given name pattern" are the main reason to still use non-cone-mode. Users with this need usually have a directory full of large files, and they choose which of those large files they need using sparse-checkout. The repository reorganization required to use cone mode for this use is perhaps too great (or they haven't thought about doing it). For this reason, I would prefer that we do not do these checks when not in cone mode. > +test_expect_success 'by default, cone mode will error out when passed files' ' > + git -C repo sparse-checkout reapply --cone && > + test_must_fail git -C repo sparse-checkout add .gitignore 2>error && > + > + grep ".gitignore.*is not a directory" error > +' > + > +test_expect_success 'by default, non-cone mode will warn on individual files' ' > + git -C repo sparse-checkout reapply --no-cone && > + git -C repo sparse-checkout add .gitignore 2>warning && > + > + grep "passing directories or less specific patterns is recommended" warning > +' So I would expect this second test to have test_must_be_empty warning to show that no warning occurs when specifying a file in non-cone-mode. Thanks, -Stolee
On Mon, Feb 14, 2022 at 7:56 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> > > > > The set and add subcommands accept multiple positional arguments. > > The meaning of these arguments differs slightly in the two modes: > > > > Cone mode only accepts directories. If given a file, it would > > previously treat it as a directory, causing not just the file itself to > > be included but all sibling files as well -- likely against users' > > expectations. Throw an error if the specified path is a file in the > > index. Provide a --skip-checks argument to allow users to override > > (e.g. for the case when the given path IS a directory on another > > branch). > > I agree that this is likely to be an improvement for users. The > sparse-checkout builtin isn't integrated with the sparse index > yet. At least not integrated upstream: we have commits in microsoft/git > that we plan to send when other things in flight are merged. This > change likely introduces a new opportunity for the index to expand, > so I will keep that in mind when upstreaming. Actually, I thought about that during development, and my presumption was that we would not expand the index. We've survived a few years without reporting any argument errors to the user and folks seem to usually get things right, so while I think it adds value to report on likely errors, I don't think it's important for us to catch and warn on every potential misuse. I think the probable errors are the ones where they specify a <file> that exists in both the working tree and index. The remaining ones are less probable, and also possibly quite expensive to catch. I'm not sure it's worth the cost to try to report those. > > Non-cone mode accepts general gitignore patterns. However, it has an > > O(N*M) performance baked into its design, where all N index files must > > be matched against all M sparse-checkout patterns with EVERY call to > > unpack_trees() that updates the working tree. As such, it is important > > to keep the number of patterns small, and thus we should warn users to > > prefer passing directories and more generic glob patterns to get the > > paths they want instead of listing each individual file. (The > > --skip-checks argument can also be used to bypass this warning.) Also, > > even when users do want to specify individual files, they will often > > want to do so by providing a leading '/' (to avoid selecting the same > > filename in all subdirectories), in which case this error message would > > never trigger anyway. > > I think the case of "I want only one file from this directory" and "I > want files with the given name pattern" are the main reason to still > use non-cone-mode. Users with this need usually have a directory full > of large files, and they choose which of those large files they need > using sparse-checkout. The repository reorganization required to use > cone mode for this use is perhaps too great (or they haven't thought > about doing it). For this reason, I would prefer that we do not do > these checks when not in cone mode. If they "only want one file from this directory", isn't the correct way to specify that by mentioning the path with a leading slash? Otherwise, they'd potentially grab files with similar names from many directories, right? So, even in that usecase, we should still error out if they specify a <filename> rather than /<filename>. Perhaps my reasoning should lead with that and I should fix up the warning message a bit, but I still think we should probably give a warning even for those who are explicitly wanting the usecase you mention. Also, note this is a warning and not an error -- and the warning can be suppressed with --skip-checks. > > +test_expect_success 'by default, cone mode will error out when passed files' ' > > + git -C repo sparse-checkout reapply --cone && > > + test_must_fail git -C repo sparse-checkout add .gitignore 2>error && > > + > > + grep ".gitignore.*is not a directory" error > > +' > > + > > +test_expect_success 'by default, non-cone mode will warn on individual files' ' > > + git -C repo sparse-checkout reapply --no-cone && > > + git -C repo sparse-checkout add .gitignore 2>warning && > > + > > + grep "passing directories or less specific patterns is recommended" warning > > +' > > So I would expect this second test to have > > test_must_be_empty warning > > to show that no warning occurs when specifying a file in non-cone-mode. or perhaps grep "please specify a leading slash to select a single file" warning ?
On 2/14/2022 11:17 PM, Elijah Newren wrote: > On Mon, Feb 14, 2022 at 7:56 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> >>> >>> The set and add subcommands accept multiple positional arguments. >>> The meaning of these arguments differs slightly in the two modes: >>> >>> Cone mode only accepts directories. If given a file, it would >>> previously treat it as a directory, causing not just the file itself to >>> be included but all sibling files as well -- likely against users' >>> expectations. Throw an error if the specified path is a file in the >>> index. Provide a --skip-checks argument to allow users to override >>> (e.g. for the case when the given path IS a directory on another >>> branch). >> >> I agree that this is likely to be an improvement for users. The >> sparse-checkout builtin isn't integrated with the sparse index >> yet. At least not integrated upstream: we have commits in microsoft/git >> that we plan to send when other things in flight are merged. This >> change likely introduces a new opportunity for the index to expand, >> so I will keep that in mind when upstreaming. > > Actually, I thought about that during development, and my presumption > was that we would not expand the index. We've survived a few years > without reporting any argument errors to the user and folks seem to > usually get things right, so while I think it adds value to report on > likely errors, I don't think it's important for us to catch and warn > on every potential misuse. I think the probable errors are the ones > where they specify a <file> that exists in both the working tree and > index. The remaining ones are less probable, and also possibly quite > expensive to catch. I'm not sure it's worth the cost to try to report > those. Since we start with only the files at root, if a user specifies a directory at least two levels deep, then we don't have enough information in the index to find that directory without parsing trees. Doing a simple index_name_pos() call would expand the sparse index to a full one. The checks of this kind typically follow this pattern: int pos = index_name_pos(index, name, strlen(name)); if (pos >= 0) /* we have 'name' in the index! */ else /* we don't have 'name' in the index! */ (Of course, there is some special logic for directories to see if they exist, since 'pos' is a position within the list of files, unless it points to a sparse directory entry.) A different method that checks only for existence could find a sparse directory and then search trees for the contained path. Returning a boolean answer provides a mechanism for not expanding the index. But we are getting sidetracked. I'll worry about this later. The important thing is that we can solve this problem, so it shouldn't block your feature. >>> Non-cone mode accepts general gitignore patterns. However, it has an >>> O(N*M) performance baked into its design, where all N index files must >>> be matched against all M sparse-checkout patterns with EVERY call to >>> unpack_trees() that updates the working tree. As such, it is important >>> to keep the number of patterns small, and thus we should warn users to >>> prefer passing directories and more generic glob patterns to get the >>> paths they want instead of listing each individual file. (The >>> --skip-checks argument can also be used to bypass this warning.) Also, >>> even when users do want to specify individual files, they will often >>> want to do so by providing a leading '/' (to avoid selecting the same >>> filename in all subdirectories), in which case this error message would >>> never trigger anyway. >> >> I think the case of "I want only one file from this directory" and "I >> want files with the given name pattern" are the main reason to still >> use non-cone-mode. Users with this need usually have a directory full >> of large files, and they choose which of those large files they need >> using sparse-checkout. The repository reorganization required to use >> cone mode for this use is perhaps too great (or they haven't thought >> about doing it). For this reason, I would prefer that we do not do >> these checks when not in cone mode. > > If they "only want one file from this directory", isn't the correct > way to specify that by mentioning the path with a leading slash? > Otherwise, they'd potentially grab files with similar names from many > directories, right? So, even in that usecase, we should still error > out if they specify a <filename> rather than /<filename>. Perhaps my > reasoning should lead with that and I should fix up the warning > message a bit, but I still think we should probably give a warning > even for those who are explicitly wanting the usecase you mention. Checking for the leading slash can be a big help to make sure users are getting the matches they expect. > Also, note this is a warning and not an error -- and the warning can > be suppressed with --skip-checks. I want to avoid surprising users with new warnings for things they have been doing for years, especially if these tasks run as part of a script that checks stderr output as a failure condition. >>> +test_expect_success 'by default, cone mode will error out when passed files' ' >>> + git -C repo sparse-checkout reapply --cone && >>> + test_must_fail git -C repo sparse-checkout add .gitignore 2>error && >>> + >>> + grep ".gitignore.*is not a directory" error >>> +' >>> + >>> +test_expect_success 'by default, non-cone mode will warn on individual files' ' >>> + git -C repo sparse-checkout reapply --no-cone && >>> + git -C repo sparse-checkout add .gitignore 2>warning && >>> + >>> + grep "passing directories or less specific patterns is recommended" warning >>> +' >> >> So I would expect this second test to have >> >> test_must_be_empty warning >> >> to show that no warning occurs when specifying a file in non-cone-mode. > > or perhaps > > grep "please specify a leading slash to select a single file" warning > > ? Right. Whatever matches the behavior we land on. Thanks, -Stolee
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index bec423d5af9..8f8d2bd6ba5 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -1,4 +1,5 @@ #include "builtin.h" +#include "cache.h" #include "config.h" #include "dir.h" #include "parse-options.h" @@ -681,8 +682,11 @@ static int modify_pattern_list(int argc, const char **argv, int use_stdin, return result; } -static void sanitize_paths(int argc, const char **argv, const char *prefix) +static void sanitize_paths(int argc, const char **argv, + const char *prefix, int skip_checks) { + int i; + if (!argc) return; @@ -691,26 +695,49 @@ static void sanitize_paths(int argc, const char **argv, const char *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]); } + + if (skip_checks) + return; + + for (i = 0; i < argc; i++) { + struct cache_entry *ce; + struct index_state *index = the_repository->index; + int pos = index_name_pos(index, argv[i], strlen(argv[i])); + + if (pos < 0) + continue; + ce = index->cache[pos]; + if (S_ISSPARSEDIR(ce->ce_mode)) + continue; + + if (core_sparse_checkout_cone) + die(_("\"%s\" is not a directory; to treat it as a directory anyway, rerun with --skip-checks"), argv[i]); + else + warning(_("path \"%s\" is an individual file; passing directories or less specific patterns is recommended (see NON-CONE PROBLEMS in the git-sparse-checkout manual)."), argv[i]); + } } static char const * const builtin_sparse_checkout_add_usage[] = { - N_("git sparse-checkout add (--stdin | <patterns>)"), + N_("git sparse-checkout add [--skip-checks] (--stdin | <patterns>)"), NULL }; static struct sparse_checkout_add_opts { + int skip_checks; int use_stdin; } add_opts; static int sparse_checkout_add(int argc, const char **argv, const char *prefix) { static struct option builtin_sparse_checkout_add_options[] = { + OPT_BOOL_F(0, "skip-checks", &add_opts.skip_checks, + N_("skip some sanity checks on the given paths that might give false positives"), + PARSE_OPT_NONEG), OPT_BOOL(0, "stdin", &add_opts.use_stdin, N_("read patterns from standard in")), OPT_END(), @@ -726,19 +753,20 @@ 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); + sanitize_paths(argc, argv, prefix, add_opts.skip_checks); return modify_pattern_list(argc, argv, add_opts.use_stdin, ADD); } static char const * const builtin_sparse_checkout_set_usage[] = { - N_("git sparse-checkout set [--[no-]cone] [--[no-]sparse-index] (--stdin | <patterns>)"), + N_("git sparse-checkout set [--[no-]cone] [--[no-]sparse-index] [--skip-checks] (--stdin | <patterns>)"), NULL }; static struct sparse_checkout_set_opts { int cone_mode; int sparse_index; + int skip_checks; int use_stdin; } set_opts; @@ -752,6 +780,9 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix) N_("initialize the sparse-checkout in cone mode")), OPT_BOOL(0, "sparse-index", &set_opts.sparse_index, N_("toggle the use of a sparse index")), + OPT_BOOL_F(0, "skip-checks", &set_opts.skip_checks, + N_("skip some sanity checks on the given paths that might give false positives"), + PARSE_OPT_NONEG), OPT_BOOL_F(0, "stdin", &set_opts.use_stdin, N_("read patterns from standard in"), PARSE_OPT_NONEG), @@ -780,7 +811,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix) argv = default_patterns; argc = default_patterns_nr; } else { - sanitize_paths(argc, argv, prefix); + sanitize_paths(argc, argv, prefix, set_opts.skip_checks); } return modify_pattern_list(argc, argv, set_opts.use_stdin, REPLACE); diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index b9e6987f5a6..1d95fa47258 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -580,7 +580,7 @@ test_expect_success 'different sparse-checkouts with worktrees' ' ' test_expect_success 'set using filename keeps file on-disk' ' - git -C repo sparse-checkout set a deep && + git -C repo sparse-checkout set --skip-checks a deep && cat >expect <<-\EOF && /* !/*/ @@ -843,4 +843,18 @@ test_expect_success 'add from subdir pays attention to prefix' ' test_cmp expect actual ' +test_expect_success 'by default, cone mode will error out when passed files' ' + git -C repo sparse-checkout reapply --cone && + test_must_fail git -C repo sparse-checkout add .gitignore 2>error && + + grep ".gitignore.*is not a directory" error +' + +test_expect_success 'by default, non-cone mode will warn on individual files' ' + git -C repo sparse-checkout reapply --no-cone && + git -C repo sparse-checkout add .gitignore 2>warning && + + grep "passing directories or less specific patterns is recommended" warning +' + test_done