Re: [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message

[Miklos Vajna <vmiklos@xxxxxxxxxxxxxx>]

On Sat, Jul 26, 2008 at 05:08:11PM +0200, Johannes Schindelin <Johannes.Schindelin@xxxxxx> wrote:
> > +		memset(&not_strategies, 0, sizeof(struct cmdnames));
> > +		for (i = 0; i < main_cmds.cnt; i++) {
> 
> Looking through all the discovered git commands?  Cute...  But does that 
> not exclude the commands that are in PATH, starting with git-merge-, even 
> if they are custom strategies?

main_cmds contains only commands in /usr/libexec/git-core, while I guess
custom strategies are elsewhere in PATH, which commands are in
other_cmds, not in main_cmds.

Sample output at me:

$ git merge -s theirss c2
HEAD is now at 1f5e3cc c1
Could not find merge strategy 'theirss'.

available strategies in '/usr/libexec/git-core/'
--------------------------------------------------
  octopus     ours        recursive   resolve     subtree

strategies available from elsewhere on your $PATH
---------------------------------------------------
  theirs

and I have git-merge-theirs in ~/bin (which is in PATH).

> 
> > +			int j, found = 0;
> > +			for (j = 0; j < ARRAY_SIZE(all_strategy); j++)
> > +				if (!strcmp(main_cmds.names[i]->name, all_strategy[j].name))
> > +					found = 1;
> > +			if (!found)
> > +				add_cmdname(&not_strategies, main_cmds.names[i]->name, strlen(main_cmds.names[i]->name));
> 
> Better have a local variable "name" instead of writing out 
> "main_cmds.names[i]->name" all the time...

Fixed.

> Oh, and you assume that the names are NUL-terminated (which I assume is 
> not the case in general, as the len member is the only thing that makes 
> struct cmdnames different from struct string_list.

I think the purpose of it is different, but the argument is still valie.
That len member is to be able to have ->name contain "foo.exe" while
having len at 3, so that git help -a will avoid the .exe suffixes.

Changed.

(I do not want to resend a full series yet, but I pushed out an amended
patch to repo.or.cz in the 'merge-custom' branch.)

diff --git a/builtin-merge.c b/builtin-merge.c
index 4084e07..b0d1de5 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -93,11 +93,12 @@ static struct strategy *get_strategy(const char *name)
 		memset(&not_strategies, 0, sizeof(struct cmdnames));
 		for (i = 0; i < main_cmds.cnt; i++) {
 			int j, found = 0;
+			char *ent = main_cmds.names[i];
 			for (j = 0; j < ARRAY_SIZE(all_strategy); j++)
-				if (!strcmp(main_cmds.names[i]->name, all_strategy[j].name))
+				if (!strncmp(ent->name, all_strategy[j].name, ent->len))
 					found = 1;
 			if (!found)
-				add_cmdname(&not_strategies, main_cmds.names[i]->name, strlen(main_cmds.names[i]->name));
+				add_cmdname(&not_strategies, ent->name, ent->len);
 		}
 		fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name);
 		list_commands("git-merge-", "strategies", &not_strategies);

Attachment: pgp5f900hVnaq.pgp
Description: PGP signature