On 12/7/21 9:04 AM, Elijah Newren wrote:
On Sat, Dec 4, 2021 at 1:40 PM Victoria Dye <vdye@xxxxxxxxxx> wrote:
Elijah Newren via GitGitGadget wrote:
From: Elijah Newren <newren@xxxxxxxxx>
The previously suggested workflow:
git sparse-checkout init ...
git sparse-checkout set ...
Suffered from three problems:
1) It would delete nearly all files in the first step, then
restore them in the second. That was poor performance and
forced unnecessary rebuilds.
2) The two-step process resulted in two progress bars, which
was suboptimal from a UI point of view for wrappers that
invoked both of these commands but only exposed a single
command to their end users.
3) With cone mode, the first step would delete nearly all
ignored files everywhere, because everything was considered
to be outside of the specified sparsity paths. (The user was
not allowed to specify any sparsity paths in the `init` step.)
Avoid these problems by teaching `set` to understand the extra
parameters that `init` takes and performing any necessary initialization
if not already in a sparse checkout.
I really like this change! It always seemed weird that `set` would
implicitly `init`, but without any of the options in `init`.
Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
---
builtin/sparse-checkout.c | 52 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 51 insertions(+), 1 deletion(-)
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index e252b82136e..cf6a6c6c3a7 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -682,17 +682,26 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
}
static char const * const builtin_sparse_checkout_set_usage[] = {
- N_("git sparse-checkout set (--stdin | <patterns>)"),
+ N_("git sparse-checkout set [--cone] [--[no-]sparse-index] (--stdin | <patterns>)"),
Since `cone` is an `OPT_BOOL`, shouldn't it appear in the usage string as
`--[no-]cone`? Without a `--no-cone` option, it's not clear how a user would
disable cone mode (since `set` preserves the existing cone mode setting if
`--cone` isn't given).
Yeah, fair point. When copying from init I probably should have double checked.
Also, it makes me wonder if we should just make cone mode the default...
That was actually my first thought when reviewing this change! It feels
like this would be a great moment to take the leap on that, but I can
also see it warranting a separate series.
NULL
};
static struct sparse_checkout_set_opts {
+ int cone_mode;
+ int sparse_index;
int use_stdin;
} set_opts;
static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
{
+ int mode, record_mode;
+ const char *default_patterns[] = {"/*", "!/*/"};
+
static struct option builtin_sparse_checkout_set_options[] = {
+ OPT_BOOL(0, "cone", &set_opts.cone_mode,
+ 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(0, "stdin", &set_opts.use_stdin,
N_("read patterns from standard in")),
I know this isn't part of this patch, but is `stdin` really a bool (i.e.
someone might specify `--no-stdin`)? If not, I think `OPT_SET_INT` might be
more appropriate.
Good point. OPT_SET_INT() wouldn't fix it, though; you need to use
OPT_BOOL_F or OPT_SET_INT_F and pass PARSE_OPT_NONEG as a flag. I can
include that.
OPT_END(),
@@ -700,11 +709,52 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
repo_read_index(the_repository);
+ set_opts.cone_mode = -1;
+ set_opts.sparse_index = -1;
+
argc = parse_options(argc, argv, prefix,
builtin_sparse_checkout_set_options,
builtin_sparse_checkout_set_usage,
PARSE_OPT_KEEP_UNKNOWN);
+ /* Determine if we need to record the mode; ensure sparse checkout on */
+ record_mode = (set_opts.cone_mode != -1) || !core_apply_sparse_checkout;
+ core_apply_sparse_checkout = 1;
+
+ /* If not specified, use previous definition of cone mode */
+ if (set_opts.cone_mode == -1 && core_apply_sparse_checkout)
I *think* this is supposed go before the `core_apply_sparse_checkout = 1;`
above it (if the intention is to only use the pre-existing cone mode setting
when already in a sparse checkout). If not, the `core_apply_sparse_checkout`
part of the condition is redundant and can be removed entirely.
Eek. I had it there originally, then moved it later while doing
various changes. This definitely should be later; thanks for
catching.
+ set_opts.cone_mode = core_sparse_checkout_cone;
+
+ /* Set cone/non-cone mode appropriately */
+ if (set_opts.cone_mode == 1) {
+ mode = MODE_CONE_PATTERNS;
+ core_sparse_checkout_cone = 1;
+ } else {
+ mode = MODE_ALL_PATTERNS;
+ }
+ if (record_mode && set_config(mode))
+ return 1;
+
+ /* Set sparse-index/non-sparse-index mode if specified */
+ if (set_opts.sparse_index >= 0) {
+ if (set_sparse_index_config(the_repository, set_opts.sparse_index) < 0)
+ die(_("failed to modify sparse-index config"));
+
+ /* force an index rewrite */
+ repo_read_index(the_repository);
+ the_repository->index->updated_workdir = 1;
+ }
+
+ /*
+ * Cone mode automatically specifies the toplevel directory. For
+ * non-cone mode, if nothing is specified, manually select just the
+ * top-level directory (much as 'init' would do).
+ */
+ if (!core_sparse_checkout_cone && argc == 0) {
+ argv = default_patterns;
+ argc = 2;
+ }
+
return modify_pattern_list(argc, argv, set_opts.use_stdin, REPLACE);
}
