Skip to content

Sparse-checkout: Add subcommand and Windows paths #546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Sparse-checkout: Add subcommand and Windows paths #546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
75ee62c
sparse-checkout: extract add_patterns_from_input()
derrickstolee Feb 7, 2020
7990916
sparse-checkout: extract pattern update from 'set' subcommand
derrickstolee Feb 7, 2020
663c40a
sparse-checkout: create 'add' subcommand
derrickstolee Feb 7, 2020
f770df4
sparse-checkout: work with Windows paths
derrickstolee Feb 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Documentation/git-sparse-checkout.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@ directories. The input format matches the output of `git ls-tree --name-only`.
This includes interpreting pathnames that begin with a double quote (") as
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <[email protected]> writes:

> +	switch (m) {
> +	case ADD:
> +		if (core_sparse_checkout_cone)
> +			add_patterns_cone_mode(argc, argv, &pl);
> +		else
> +			add_patterns_literal(argc, argv, &pl);
> +		break;
> +
> +	case REPLACE:
> +		add_patterns_from_input(&pl, argc, argv);
> +		break;
> +	}

Is it just me or do readers find it irritating to see the order of
the arguments seem a bit random?  Those who like Pseudo-OO-in-C
probably would want to consistently see &pl as the first parameter,
which lets them pretend various flavours of add_patterns_*() are
all "methods" to the pattern list object.

> +static void add_patterns_literal(int argc, const char **argv,
> +				 struct pattern_list *pl)
> +{
> +	char *sparse_filename = get_sparse_checkout_filename();
> +	if (add_patterns_from_file_to_list(sparse_filename, "", 0,
> +					   pl, NULL))
> +		die(_("unable to load existing sparse-checkout patterns"));
> +	free(sparse_filename);
> +	add_patterns_from_input(pl, argc, argv);
> +}

It may make it easier to read the caller if this helper were
replaced with one that does not add anything but just read from the
existing file, i.e.

		if (core_sparse_checkout_cone)
			add_patterns_cone_mode(argc, argv, &pl);
		else {
			read_existing_patterns(&pl);
			add_patterns_from_input(&pl, argc, argv);
		}

C-style quoted strings.

'add'::
Update the sparse-checkout file to include additional patterns.
By default, these patterns are read from the command-line arguments,
but they can be read from stdin using the `--stdin` option. When
`core.sparseCheckoutCone` is enabled, the given patterns are interpreted
as directory names as in the 'set' subcommand.

'disable'::
Disable the `core.sparseCheckout` config setting, and restore the
working directory to include all files. Leaves the sparse-checkout
Expand Down
141 changes: 109 additions & 32 deletions builtin/sparse-checkout.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
static const char *empty_base = "";

static char const * const builtin_sparse_checkout_usage[] = {
N_("git sparse-checkout (init|list|set|disable) <options>"),
N_("git sparse-checkout (init|list|set|add|disable) <options>"),
NULL
};

Expand Down Expand Up @@ -394,6 +394,9 @@ static void strbuf_to_cone_pattern(struct strbuf *line, struct pattern_list *pl)

strbuf_trim_trailing_dir_sep(line);

if (strbuf_normalize_path(line))
die(_("could not normalize path %s"), line->buf);

if (!line->len)
return;

Expand All @@ -404,44 +407,24 @@ static void strbuf_to_cone_pattern(struct strbuf *line, struct pattern_list *pl)
}

static char const * const builtin_sparse_checkout_set_usage[] = {
N_("git sparse-checkout set (--stdin | <patterns>)"),
N_("git sparse-checkout (set|add) (--stdin | <patterns>)"),
NULL
};

static struct sparse_checkout_set_opts {
int use_stdin;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <[email protected]> writes:

> -static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
> +static void add_patterns_from_input(struct pattern_list *pl,
> +				    int argc, const char **argv)

Separating out what happens _after_ parse_options into its own
helper function makes sense.

Everything, not limited to this function, works on its input, so the
name suffix does not add much value to the readers.  IOW, I do not
think I, as a new reader of this code, would be confused if this
function were called add_patterns().

If we wanted to add more words to the name, perhaps I'd use them to
describe the shape of the input (e.g. "add_patterns_from_acav") but
that is obvious from the list of input parameter, so...

I haven't read the rest of the series, but will we ever call this
helper function with an array we manufacture ourselves?  If the
input array is known to be NULL terminated (and at this step in the
series, it certainly is, as we are using the ac/av supplied by the C
runtime entry point), perhaps we can omit argc from the parameter to
simplify the calling convention a bit?

>  {
>  	int i;
> -	struct pattern_list pl;
> -	int result;
> -	int changed_config = 0;
> -
> -	static struct option builtin_sparse_checkout_set_options[] = {
> -		OPT_BOOL(0, "stdin", &set_opts.use_stdin,
> -			 N_("read patterns from standard in")),
> -		OPT_END(),
> -	};
> -
> -	repo_read_index(the_repository);
> -	require_clean_work_tree(the_repository,
> -				N_("set sparse-checkout patterns"), NULL, 1, 0);
> -
> -	memset(&pl, 0, sizeof(pl));
> -
> -	argc = parse_options(argc, argv, prefix,
> -			     builtin_sparse_checkout_set_options,
> -			     builtin_sparse_checkout_set_usage,
> -			     PARSE_OPT_KEEP_UNKNOWN);
> -
>  	if (core_sparse_checkout_cone) {
>  		struct strbuf line = STRBUF_INIT;
>  
> -		hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0);
> -		hashmap_init(&pl.parent_hashmap, pl_hashmap_cmp, NULL, 0);
> -		pl.use_cone_patterns = 1;
> +		hashmap_init(&pl->recursive_hashmap, pl_hashmap_cmp, NULL, 0);
> +		hashmap_init(&pl->parent_hashmap, pl_hashmap_cmp, NULL, 0);
> +		pl->use_cone_patterns = 1;
> ...

} set_opts;

static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
static void add_patterns_from_input(struct pattern_list *pl,
int argc, const char **argv)
{
int i;
struct pattern_list pl;
int result;
int changed_config = 0;

static struct option builtin_sparse_checkout_set_options[] = {
OPT_BOOL(0, "stdin", &set_opts.use_stdin,
N_("read patterns from standard in")),
OPT_END(),
};

repo_read_index(the_repository);
require_clean_work_tree(the_repository,
N_("set sparse-checkout patterns"), NULL, 1, 0);

memset(&pl, 0, sizeof(pl));

argc = parse_options(argc, argv, prefix,
builtin_sparse_checkout_set_options,
builtin_sparse_checkout_set_usage,
PARSE_OPT_KEEP_UNKNOWN);

if (core_sparse_checkout_cone) {
struct strbuf line = STRBUF_INIT;

hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0);
hashmap_init(&pl.parent_hashmap, pl_hashmap_cmp, NULL, 0);
pl.use_cone_patterns = 1;
hashmap_init(&pl->recursive_hashmap, pl_hashmap_cmp, NULL, 0);
hashmap_init(&pl->parent_hashmap, pl_hashmap_cmp, NULL, 0);
pl->use_cone_patterns = 1;

if (set_opts.use_stdin) {
struct strbuf unquoted = STRBUF_INIT;
Expand All @@ -455,15 +438,15 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
strbuf_swap(&unquoted, &line);
}

strbuf_to_cone_pattern(&line, &pl);
strbuf_to_cone_pattern(&line, pl);
}

strbuf_release(&unquoted);
} else {
for (i = 0; i < argc; i++) {
strbuf_setlen(&line, 0);
strbuf_addstr(&line, argv[i]);
strbuf_to_cone_pattern(&line, &pl);
strbuf_to_cone_pattern(&line, pl);
}
}
} else {
Expand All @@ -473,13 +456,84 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
while (!strbuf_getline(&line, stdin)) {
size_t len;
char *buf = strbuf_detach(&line, &len);
add_pattern(buf, empty_base, 0, &pl, 0);
add_pattern(buf, empty_base, 0, pl, 0);
}
} else {
for (i = 0; i < argc; i++)
add_pattern(argv[i], empty_base, 0, &pl, 0);
add_pattern(argv[i], empty_base, 0, pl, 0);
}
}
}

enum modify_type {
REPLACE,
ADD,
};

static void add_patterns_cone_mode(int argc, const char **argv,
struct pattern_list *pl)
{
struct strbuf buffer = STRBUF_INIT;
struct pattern_entry *pe;
struct hashmap_iter iter;
struct pattern_list existing;
char *sparse_filename = get_sparse_checkout_filename();

add_patterns_from_input(pl, argc, argv);

memset(&existing, 0, sizeof(existing));
existing.use_cone_patterns = core_sparse_checkout_cone;

if (add_patterns_from_file_to_list(sparse_filename, "", 0,
&existing, NULL))
die(_("unable to load existing sparse-checkout patterns"));
free(sparse_filename);

hashmap_for_each_entry(&existing.recursive_hashmap, &iter, pe, ent) {
if (!hashmap_contains_parent(&pl->recursive_hashmap,
pe->pattern, &buffer) ||
!hashmap_contains_parent(&pl->parent_hashmap,
pe->pattern, &buffer)) {
strbuf_reset(&buffer);
strbuf_addstr(&buffer, pe->pattern);
insert_recursive_pattern(pl, &buffer);
}
}

clear_pattern_list(&existing);
strbuf_release(&buffer);
}

static void add_patterns_literal(int argc, const char **argv,
struct pattern_list *pl)
{
char *sparse_filename = get_sparse_checkout_filename();
if (add_patterns_from_file_to_list(sparse_filename, "", 0,
pl, NULL))
die(_("unable to load existing sparse-checkout patterns"));
free(sparse_filename);
add_patterns_from_input(pl, argc, argv);
}

static int modify_pattern_list(int argc, const char **argv, enum modify_type m)
{
int result;
int changed_config = 0;
struct pattern_list pl;
memset(&pl, 0, sizeof(pl));

switch (m) {
case ADD:
if (core_sparse_checkout_cone)
add_patterns_cone_mode(argc, argv, &pl);
else
add_patterns_literal(argc, argv, &pl);
break;

case REPLACE:
add_patterns_from_input(&pl, argc, argv);
break;
}

if (!core_apply_sparse_checkout) {
set_config(MODE_ALL_PATTERNS);
Expand All @@ -496,6 +550,27 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
return result;
}

static int sparse_checkout_set(int argc, const char **argv, const char *prefix,
enum modify_type m)
{
static struct option builtin_sparse_checkout_set_options[] = {
OPT_BOOL(0, "stdin", &set_opts.use_stdin,
N_("read patterns from standard in")),
OPT_END(),
};

repo_read_index(the_repository);
require_clean_work_tree(the_repository,
N_("set sparse-checkout patterns"), NULL, 1, 0);

argc = parse_options(argc, argv, prefix,
builtin_sparse_checkout_set_options,
builtin_sparse_checkout_set_usage,
PARSE_OPT_KEEP_UNKNOWN);

return modify_pattern_list(argc, argv, m);
}

static int sparse_checkout_disable(int argc, const char **argv)
{
struct pattern_list pl;
Expand Down Expand Up @@ -544,7 +619,9 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix)
if (!strcmp(argv[0], "init"))
return sparse_checkout_init(argc, argv);
if (!strcmp(argv[0], "set"))
return sparse_checkout_set(argc, argv, prefix);
return sparse_checkout_set(argc, argv, prefix, REPLACE);
if (!strcmp(argv[0], "add"))
return sparse_checkout_set(argc, argv, prefix, ADD);
if (!strcmp(argv[0], "disable"))
return sparse_checkout_disable(argc, argv);
}
Expand Down
73 changes: 73 additions & 0 deletions t/t1091-sparse-checkout-builtin.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,21 @@ test_expect_success 'set sparse-checkout using --stdin' '
check_files repo "a folder1 folder2"
'

test_expect_success 'add to sparse-checkout' '
cat repo/.git/info/sparse-checkout >expect &&
cat >add <<-\EOF &&
pattern1
/folder1/
pattern2
EOF
cat add >>expect &&
git -C repo sparse-checkout add --stdin <add &&
git -C repo sparse-checkout list >actual &&
test_cmp expect actual &&
test_cmp expect repo/.git/info/sparse-checkout &&
check_files repo "a folder1 folder2"
'

test_expect_success 'cone mode: match patterns' '
git -C repo config --worktree core.sparseCheckoutCone true &&
rm -rf repo/a repo/folder1 repo/folder2 &&
Expand Down Expand Up @@ -219,8 +234,52 @@ test_expect_success 'cone mode: set with nested folders' '
test_cmp repo/.git/info/sparse-checkout expect
'

test_expect_success 'cone mode: add independent path' '
git -C repo sparse-checkout set deep/deeper1 &&
git -C repo sparse-checkout add folder1 &&
cat >expect <<-\EOF &&
/*
!/*/
/deep/
!/deep/*/
/deep/deeper1/
/folder1/
EOF
test_cmp expect repo/.git/info/sparse-checkout &&
check_files repo a deep folder1
'

test_expect_success 'cone mode: add sibling path' '
git -C repo sparse-checkout set deep/deeper1 &&
git -C repo sparse-checkout add deep/deeper2 &&
cat >expect <<-\EOF &&
/*
!/*/
/deep/
!/deep/*/
/deep/deeper1/
/deep/deeper2/
EOF
test_cmp expect repo/.git/info/sparse-checkout &&
check_files repo a deep
'

test_expect_success 'cone mode: add parent path' '
git -C repo sparse-checkout set deep/deeper1 folder1 &&
git -C repo sparse-checkout add deep &&
cat >expect <<-\EOF &&
/*
!/*/
/deep/
/folder1/
EOF
test_cmp expect repo/.git/info/sparse-checkout &&
check_files repo a deep folder1
'

test_expect_success 'revert to old sparse-checkout on bad update' '
test_when_finished git -C repo reset --hard &&
git -C repo sparse-checkout set deep &&
echo update >repo/deep/deeper2/a &&
cp repo/.git/info/sparse-checkout expect &&
test_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&
Expand Down Expand Up @@ -438,4 +497,18 @@ test_expect_success BSLASHPSPEC 'pattern-checks: escaped characters' '
test_cmp list-expect list-actual
'

test_expect_success MINGW 'cone mode replaces backslashes with slashes' '
git -C repo sparse-checkout set deep\\deeper1 &&
cat >expect <<-\EOF &&
/*
!/*/
/deep/
!/deep/*/
/deep/deeper1/
EOF
test_cmp expect repo/.git/info/sparse-checkout &&
check_files repo a deep &&
check_files repo/deep a deeper1
'

test_done