Re: [PATCH 1/4] sparse-checkout: extract add_patterns_from_input()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 11, 2020, 16:56 UTC
- Message-ID
- <xmqqo8u5vz9d.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <75ee62caa940e7232e0edb50788302f36a08b5b9.1581433344.git.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> 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?
Show 33 quoted lines
> {
> 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;
> ...