[1/4] sparse-checkout: extract add_patterns_from_input()

Commit Message

Johannes Schindelin via GitGitGadget Feb. 11, 2020, 3:02 p.m. UTC

From: Derrick Stolee <[e-mail]>

In anticipation of extending the sparse-checkout builtin with "add"
and "remove" subcommands, extract the code that fills a pattern list
based on the input values. The input changes depending on the
presence of "--stdin" or the value of core.sparseCheckoutCone.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 builtin/sparse-checkout.c | 64 +++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 29 deletions(-)

Comments

Junio C Hamano Feb. 11, 2020, 4:56 p.m. UTC | #1

"Derrick Stolee via GitGitGadget" <[e-mail]> 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;
> ...

Patch

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 7aeb384362..41d8aaf9a2 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -412,36 +412,16 @@  static struct sparse_checkout_set_opts {
 	int use_stdin;
 } 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;
@@ -455,7 +435,7 @@  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);
@@ -463,7 +443,7 @@  static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
 			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 {
@@ -473,13 +453,39 @@  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);
 		}
 	}
+}
+
+static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
+{
+	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);
+
+	add_patterns_from_input(&pl, argc, argv);
 
 	if (!core_apply_sparse_checkout) {
 		set_config(MODE_ALL_PATTERNS);