Re: [PATCH 3/4] sparse-checkout: create 'add' subcommand

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 11, 2020, 17:05 UTC
Message-ID: <xmqqk14tvytp.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <663c40a6909ffac6650a3ee6883b3299f6b2bd90.1581433344.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 12 quoted lines

> +	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.

Show 10 quoted lines

> +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);
		}