On Tue, Jul 29, 2008 at 12:47:05PM -0700, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> 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?
I like the idea. ;-)
Here is what I did:
- I changed only load_command_list() and list_commands().
load_command_list() still supports filtering, but the result is loaded
to a pair of "struct cmdnames", supplied by the caller.
- list_commands() works from this pair, additionally supporting the
custom title.
- export add_cmdname(), exclude_cmds() and is_in_cmdlist() without any
modification in help.h
The rest is done in builtin-merge.c, as you suggested.
The nice thing is that before this change builtin-merge accepted for
example '-s index' (because git-merge-index was a valid command), but
now this is no longer true.
Miklos Vajna (4):
builtin-help: make some internal functions available to other
builtins
builtin-merge: allow using a custom strategy
Add a new test for using a custom merge strategy
Add a second testcase for handling invalid strategies in git-merge
Makefile | 1 +
builtin-merge.c | 42 +++++++++++++++++++++----
help.c | 77 ++++++++++++++++++++++++-----------------------
help.h | 23 ++++++++++++++
t/t7600-merge.sh | 5 +++
t/t7606-merge-custom.sh | 46 ++++++++++++++++++++++++++++
6 files changed, 149 insertions(+), 45 deletions(-)
create mode 100644 help.h
create mode 100755 t/t7606-merge-custom.sh
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
