[PATCH 0/4] Allow custom merge strategies, take 4

From: Miklos Vajna <vmiklos@frugalware.org>
Date: Jul 29, 2008, 23:16 UTC
Message-ID: <cover.1217372486.git.vmiklos@frugalware.org>
In-Reply-To: <7v4p68eame.fsf@gitster.siamese.dyndns.org>

On Tue, Jul 29, 2008 at 12:47:05PM -0700, Junio C Hamano <gitster@pobox.com> wrote:

Show 24 quoted lines

> 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