From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 29 Jul 2008 19:47:05 GMT
Subject: Re: [PATCH 4/5] builtin-merge: allow using a custom strategy
Message-ID: <7v4p68eame.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <722d66a5a897b694f374aa96bd58aff01a2a5932.1217344803.git.vmiklos@frugalware.org>

Miklos Vajna <vmiklos@frugalware.org> writes:

> Allow using a custom strategy, as long as it's named git-merge-foo. The
> error handling is now done using is_git_command(). The list of available
> strategies is now shown by list_commands().
>
> If an invalid strategy is supplied, like -s foobar, then git-merge would
> list all git-merge-* commands. This is not perfect, since for example
> git-merge-index is not a valid strategy.
> ...
> +	if (!is_git_command(name, "git-merge-")) {
> +		struct cmdnames not_strategies;
> +
> +		memset(&not_strategies, 0, sizeof(struct cmdnames));
> +		for (i = 0; i < main_cmds.cnt; i++) {
> +			int j, found = 0;
> +			struct cmdname *ent = main_cmds.names[i];
> +			for (j = 0; j < ARRAY_SIZE(all_strategy); j++)
> +				if (!strncmp(ent->name, all_strategy[j].name, ent->len)
> +						&& !all_strategy[j].name[ent->len])
> +					found = 1;
> +			if (!found)
> +				add_cmdname(&not_strategies, ent->name, ent->len);
> +		}

This feels overly wasteful.  Granted, this is not a performance critical
codepath, but you list all commands that begin with "git-merge-" in
is_git_command(), only to discard it and then iterate over 140+ main_cmds
list only to cull the ones whose name do not appear in the strategies
list.

Perhaps this shows that changing the function is_git_command() is a wrong
approach (for one thing, with the custom prefix, it is not about "Is it a
git command" anymore).  Wouldn't it be easier to read if you did this part
like this instead?

 * make load_command_list capable of loading into a "struct cmdnames" (or
   pair if you want) supplied by the caller;

 * use it to grab all commands whose name begin with "git-merge-" here;

 * Check if name appears in that list; if it doesn't, you already have the
   list of commands that could be merge strategy for error reporting.

If you also update list_commands() not to run load_command_list() itself
but take caller-supplied list of commands, then the API would become much
cleaner.  The caller would not be limited to "filter with prefix" anymore.

Hmm?