[PATCH] builtin-merge: give a proper error message for invalid strategies in config

[PATCH] builtin-merge: give a proper error message for invalid strategies in config

[Miklos Vajna]

Till now 'git merge -s foobar' bailed out with an error message, but
foobar in pull.twohead or pull.octopus was just silently ignored. It's
better to inform the user then just silently doing nothing.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-merge.c              |   35 +++++++++++++++--------------------
 t/t7601-merge-pull-config.sh |    6 ++++++
 2 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index e97c79e..5037acf 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -93,28 +93,13 @@ static void append_strategy(struct strategy *s)
 	use_strategies[use_strategies_nr++] = s;
 }
 
+static void add_strategies(const char *string, unsigned attr);
+
 static int option_parse_strategy(const struct option *opt,
 				 const char *name, int unset)
 {
-	int i;
-	struct strategy *s;
-
-	if (unset)
-		return 0;
-
-	s = get_strategy(name);
-
-	if (s)
-		append_strategy(s);
-	else {
-		struct strbuf err;
-		strbuf_init(&err, 0);
-		for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
-			strbuf_addf(&err, " %s", all_strategy[i].name);
-		fprintf(stderr, "Could not find merge strategy '%s'.\n", name);
-		fprintf(stderr, "Available strategies are:%s.\n", err.buf);
-		exit(1);
-	}
+	if (!unset)
+		add_strategies(name, 0);
 	return 0;
 }
 
@@ -639,7 +624,7 @@ static void split_merge_strategies(const char *string, struct strategy **list,
 static void add_strategies(const char *string, unsigned attr)
 {
 	struct strategy *list = NULL;
-	int list_alloc = 0, list_nr = 0, i;
+	int list_alloc = 0, list_nr = 0, i, j;
 
 	memset(&list, 0, sizeof(list));
 	split_merge_strategies(string, &list, &list_nr, &list_alloc);
@@ -650,6 +635,16 @@ static void add_strategies(const char *string, unsigned attr)
 			s = get_strategy(list[i].name);
 			if (s)
 				append_strategy(s);
+			else {
+				struct strbuf err;
+
+				strbuf_init(&err, 0);
+				for (j = 0; j < ARRAY_SIZE(all_strategy); j++)
+					strbuf_addf(&err, " %s", all_strategy[j].name);
+				fprintf(stderr, "Could not find merge strategy '%s'.\n", list[i].name);
+				fprintf(stderr, "Available strategies are:%s.\n", err.buf);
+				exit(1);
+			}
 		}
 		return;
 	}
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 95b4d71..ca63451 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -126,4 +126,10 @@ test_expect_success 'merge picks up the best result' '
 	test $auto_count != $resolve_count
 '
 
+test_expect_success 'merge errors out on invalid strategy' '
+	git config pull.twohead "foobar" &&
+	git reset --hard c5 &&
+	test_must_fail git merge c6
+'
+
 test_done
-- 
1.5.6.4.433.g09651.dirty

Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> Till now 'git merge -s foobar' bailed out with an error message, but
> foobar in pull.twohead or pull.octopus was just silently ignored. It's
> better to inform the user then just silently doing nothing.
>
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>

Doesn't this make "git merge -s 'recursive resolve'" to misbehave?

Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Miklos Vajna <vmiklos@frugalware.org> writes:
>
>> Till now 'git merge -s foobar' bailed out with an error message, but
>> foobar in pull.twohead or pull.octopus was just silently ignored. It's
>> better to inform the user then just silently doing nothing.
>>
>> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
>
> Doesn't this make "git merge -s 'recursive resolve'" to misbehave?

Perhaps this would be a better replacement.  It makes get_strategy() to
validate and barf on unknown one.  It is slightly smaller and more
contained.  If/when we add user-defined ones, that logic will also be
contained in the function.

 builtin-merge.c              |   37 ++++++++++++-------------------------
 t/t7601-merge-pull-config.sh |   12 ++++++++++++
 2 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index e97c79e..0fd7985 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -77,6 +77,7 @@ static int option_parse_message(const struct option *opt,
 static struct strategy *get_strategy(const char *name)
 {
 	int i;
+	struct strbuf err;
 
 	if (!name)
 		return NULL;
@@ -84,7 +85,13 @@ static struct strategy *get_strategy(const char *name)
 	for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
 		if (!strcmp(name, all_strategy[i].name))
 			return &all_strategy[i];
-	return NULL;
+
+	strbuf_init(&err, 0);
+	for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
+		strbuf_addf(&err, " %s", all_strategy[i].name);
+	fprintf(stderr, "Could not find merge strategy '%s'.\n", name);
+	fprintf(stderr, "Available strategies are:%s.\n", err.buf);
+	exit(1);
 }
 
 static void append_strategy(struct strategy *s)
@@ -96,25 +103,10 @@ static void append_strategy(struct strategy *s)
 static int option_parse_strategy(const struct option *opt,
 				 const char *name, int unset)
 {
-	int i;
-	struct strategy *s;
-
 	if (unset)
 		return 0;
 
-	s = get_strategy(name);
-
-	if (s)
-		append_strategy(s);
-	else {
-		struct strbuf err;
-		strbuf_init(&err, 0);
-		for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
-			strbuf_addf(&err, " %s", all_strategy[i].name);
-		fprintf(stderr, "Could not find merge strategy '%s'.\n", name);
-		fprintf(stderr, "Available strategies are:%s.\n", err.buf);
-		exit(1);
-	}
+	append_strategy(get_strategy(name));
 	return 0;
 }
 
@@ -643,14 +635,9 @@ static void add_strategies(const char *string, unsigned attr)
 
 	memset(&list, 0, sizeof(list));
 	split_merge_strategies(string, &list, &list_nr, &list_alloc);
-	if (list != NULL) {
-		for (i = 0; i < list_nr; i++) {
-			struct strategy *s;
-
-			s = get_strategy(list[i].name);
-			if (s)
-				append_strategy(s);
-		}
+	if (list) {
+		for (i = 0; i < list_nr; i++)
+			append_strategy(get_strategy(list[i].name));
 		return;
 	}
 	for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 95b4d71..6b9f638 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -126,4 +126,16 @@ test_expect_success 'merge picks up the best result' '
 	test $auto_count != $resolve_count
 '
 
+test_expect_success 'merge errors out on invalid strategy' '
+	git config pull.twohead "foobar" &&
+	git reset --hard c5 &&
+	test_must_fail git merge c6
+'
+
+test_expect_success 'merge errors out on invalid strategy' '
+	git config --unset-all pull.twohead &&
+	git reset --hard c5 &&
+	test_must_fail git merge -s "resolve recursive" c6
+'
+
 test_done

Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 1078 bytes --]

On Mon, Jul 21, 2008 at 10:01:16PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Doesn't this make "git merge -s 'recursive resolve'" to misbehave?

Depends on what do we expect it to do. ;-)

My patch unified the handling of pull.twohead / -s option, so it
(actually unintentionally) made -s accepting multiple strategies as a
space separated list. Given that we already accept multiple strategies
as a space separated list in pull.twohead (and we do _not_ use multiple
pull.twohead entries) I think my patch is more logical.

Though, there was already a thread about how should we specify multiple
strategies on the commandline; and you suggested in

        http://article.gmane.org/gmane.comp.version-control.git/89208

to use -s strategy1 -s strategy2.

In that case, I think your patch is better, and once it hits git.git, I
would like to send a patch that changes the config parsing as well, so
that pull.twohead "foo bar" would be invalid, and the user would have to
have two pull.twohead entries: one for foo and one for bar.

Does this sound reasonable?

Thanks.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> ... I
> would like to send a patch that changes the config parsing as well, so
> that pull.twohead "foo bar" would be invalid, and the user would have to
> have two pull.twohead entries: one for foo and one for bar.

Don't.  pull.* has always been defined as "list of strategies", and -s has
always been defined to take "a" strategy.

IOW, you don't need to break anything further.

[PATCH] t7601: extend the 'merge picks up the best result' test

[Miklos Vajna]

The test only checked if the best result picking code works if there are
multiple strategies set in the config. Add a similar one that tests if
the same true if the -s option of git merge was used multiple times.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Tue, Jul 22, 2008 at 01:24:14AM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Don't.  pull.* has always been defined as "list of strategies", and -s
> has
> always been defined to take "a" strategy.

OK. Here is a testcase for the later. As far as I see the behaviour of
multiple -s was not checked till now.

 t/t7601-merge-pull-config.sh |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 6b9f638..55aa6b5 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -112,6 +112,21 @@ test_expect_success 'setup conflicted merge' '
 # recusive is choosen.
 
 test_expect_success 'merge picks up the best result' '
+	git config --unset-all pull.twohead &&
+	git reset --hard c5 &&
+	git merge -s resolve c6
+	resolve_count=$(conflict_count) &&
+	git reset --hard c5 &&
+	git merge -s recursive c6
+	recursive_count=$(conflict_count) &&
+	git reset --hard c5 &&
+	git merge -s recursive -s resolve c6
+	auto_count=$(conflict_count) &&
+	test $auto_count = $recursive_count &&
+	test $auto_count != $resolve_count
+'
+
+test_expect_success 'merge picks up the best result (from config)' '
 	git config pull.twohead "recursive resolve" &&
 	git reset --hard c5 &&
 	git merge -s resolve c6
-- 
1.5.6.4.433.g09651.dirty

[PATCH] t7601: extend the 'merge picks up the best result' test

[Miklos Vajna]

The test only checked if the best result picking code works if there are
multiple strategies set in the config. Add a similar one that tests if
the same true if the -s option of git merge was used multiple times.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Tue, Jul 22, 2008 at 01:24:14AM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Don't.  pull.* has always been defined as "list of strategies", and -s
> has
> always been defined to take "a" strategy.

OK. Here is a testcase for the later. As far as I see the behaviour of
multiple -s was not checked till now.

 t/t7601-merge-pull-config.sh |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 6b9f638..55aa6b5 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -112,6 +112,21 @@ test_expect_success 'setup conflicted merge' '
 # recusive is choosen.
 
 test_expect_success 'merge picks up the best result' '
+	git config --unset-all pull.twohead &&
+	git reset --hard c5 &&
+	git merge -s resolve c6
+	resolve_count=$(conflict_count) &&
+	git reset --hard c5 &&
+	git merge -s recursive c6
+	recursive_count=$(conflict_count) &&
+	git reset --hard c5 &&
+	git merge -s recursive -s resolve c6
+	auto_count=$(conflict_count) &&
+	test $auto_count = $recursive_count &&
+	test $auto_count != $resolve_count
+'
+
+test_expect_success 'merge picks up the best result (from config)' '
 	git config pull.twohead "recursive resolve" &&
 	git reset --hard c5 &&
 	git merge -s resolve c6
-- 
1.5.6.4.433.g09651.dirty

[PATCH 0/7] Allow custom merge strategies

[Miklos Vajna]

Hi,

This series adds support for custom merge strategies.

The first 3 patches modify builtin-help to allow using it from other
builtins. This is necessary because in the error message of 'git merge
-s foobar' we show something like 'git help -a', but we list only merge
strategies. A command is considered a merge strategy if it has a
git-merge- prefix and is listed in the all_strategy array or it is
somewhere in PATH, but outside `git --exec-path`, so that git-merge-ours
and other strategies are shown, git-merge-index and other
git-merge-named (but not strategy) commands are hidden.

The last two is about removing those problematic 'git-merge-index',
'git-merge-tree' and other bogus commands from the output of 'git merge
-s foobar'. I think the benefit of doing it that way is that we don't
have to maintain a list of commands which are named git-merge-foo but
not strategies _and_ the custom strategies can have a form of
git-merge-foo, without adding extra complexity (like forcing users to
name them git-merge-custom-foo).

NOTE: At the moment the custom strategies are named as git-merge-foo as
well, mainly because I think it's not that problematic to exclude the
already existing git-merge-fo non-strategy commands, but this can be
changed to git-merge-strategy-foo if we really want so.

Also, I'm aware that this is a feature and we are in rc freeze, I just
did not want to keep back this series till 1.6.0 is out.

Miklos Vajna (7):
  Make is_git_command() usable outside builtin-help
  builtin-help: change the current directory back in
    list_commands_in_dir()
  builtin-help: make list_commands() a bit more generic
  builtin-merge: allow using a custom strategy
  Add a new test for using a custom merge strategy
  builtin-help: make it possible to exclude some commands in
    list_commands()
  builtin-merge: avoid non-strategy git-merge commands in error message

 Makefile                |    1 +
 builtin-merge.c         |   30 +++++++++++++++++++++------
 help.c                  |   50 ++++++++++++++++++++++++----------------------
 help.h                  |   19 +++++++++++++++++
 t/t7606-merge-custom.sh |   45 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 114 insertions(+), 31 deletions(-)
 create mode 100644 help.h
 create mode 100755 t/t7606-merge-custom.sh

[PATCH 1/7] Make is_git_command() usable outside builtin-help

[Miklos Vajna]

Other builtins may want to check if a given command is a valid git
command or not as well. Additionally add a new parameter that specifies
a custom prefix, so that the "git-" prefix is no longer hardwired.
Useful for example to limit the search for "git-merge-*".

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 Makefile |    1 +
 help.c   |   25 ++++++++++++++-----------
 help.h   |    6 ++++++
 3 files changed, 21 insertions(+), 11 deletions(-)
 create mode 100644 help.h

diff --git a/Makefile b/Makefile
index b01cf1c..85e79f6 100644
--- a/Makefile
+++ b/Makefile
@@ -346,6 +346,7 @@ LIB_H += git-compat-util.h
 LIB_H += graph.h
 LIB_H += grep.h
 LIB_H += hash.h
+LIB_H += help.h
 LIB_H += list-objects.h
 LIB_H += ll-merge.h
 LIB_H += log-tree.h
diff --git a/help.c b/help.c
index bfc84ae..fb93df0 100644
--- a/help.c
+++ b/help.c
@@ -418,17 +418,20 @@ static int is_executable(const char *name)
 }
 
 static unsigned int list_commands_in_dir(struct cmdnames *cmds,
-					 const char *path)
+					 const char *path,
+					 const char *prefix)
 {
 	unsigned int longest = 0;
-	const char *prefix = "git-";
-	int prefix_len = strlen(prefix);
+	int prefix_len;
 	DIR *dir = opendir(path);
 	struct dirent *de;
 
 	if (!dir || chdir(path))
 		return 0;
 
+	if (!prefix)
+		prefix = "git-";
+       	prefix_len = strlen(prefix);
 	while ((de = readdir(dir)) != NULL) {
 		int entlen;
 
@@ -452,7 +455,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 	return longest;
 }
 
-static unsigned int load_command_list(void)
+static unsigned int load_command_list(const char *prefix)
 {
 	unsigned int longest = 0;
 	unsigned int len;
@@ -461,7 +464,7 @@ static unsigned int load_command_list(void)
 	const char *exec_path = git_exec_path();
 
 	if (exec_path)
-		longest = list_commands_in_dir(&main_cmds, exec_path);
+		longest = list_commands_in_dir(&main_cmds, exec_path, prefix);
 
 	if (!env_path) {
 		fprintf(stderr, "PATH not set\n");
@@ -473,7 +476,7 @@ static unsigned int load_command_list(void)
 		if ((colon = strchr(path, PATH_SEP)))
 			*colon = 0;
 
-		len = list_commands_in_dir(&other_cmds, path);
+		len = list_commands_in_dir(&other_cmds, path, prefix);
 		if (len > longest)
 			longest = len;
 
@@ -497,7 +500,7 @@ static unsigned int load_command_list(void)
 
 static void list_commands(void)
 {
-	unsigned int longest = load_command_list();
+	unsigned int longest = load_command_list(NULL);
 	const char *exec_path = git_exec_path();
 
 	if (main_cmds.cnt) {
@@ -543,9 +546,9 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s)
 	return 0;
 }
 
-static int is_git_command(const char *s)
+int is_git_command(const char *s, const char *prefix)
 {
-	load_command_list();
+	load_command_list(prefix);
 	return is_in_cmdlist(&main_cmds, s) ||
 		is_in_cmdlist(&other_cmds, s);
 }
@@ -566,7 +569,7 @@ static const char *cmd_to_page(const char *git_cmd)
 		return "git";
 	else if (!prefixcmp(git_cmd, "git"))
 		return git_cmd;
-	else if (is_git_command(git_cmd))
+	else if (is_git_command(git_cmd, NULL))
 		return prepend("git-", git_cmd);
 	else
 		return prepend("git", git_cmd);
@@ -704,7 +707,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	}
 
 	alias = alias_lookup(argv[0]);
-	if (alias && !is_git_command(argv[0])) {
+	if (alias && !is_git_command(argv[0], NULL)) {
 		printf("`git %s' is aliased to `%s'\n", argv[0], alias);
 		return 0;
 	}
diff --git a/help.h b/help.h
new file mode 100644
index 0000000..73da8d6
--- /dev/null
+++ b/help.h
@@ -0,0 +1,6 @@
+#ifndef HELP_H
+#define HELP_H
+
+int is_git_command(const char *s, const char *prefix);
+
+#endif /* HELP_H */
-- 
1.6.0.rc0.14.g95f8.dirty

Re: [PATCH 1/7] Make is_git_command() usable outside builtin-help

[Jonathan Nieder]

On Sat, 26 Jul 2008, Miklos Vajna wrote:

> +	if (!prefix)
> +		prefix = "git-";
> +       	prefix_len = strlen(prefix);

The whitespace gave me a start: the diff markup moved the prefix_len
line to the next tab stop, so at first glance it seems there are missing
braces here.  But it is an illusion.  (I mention this so others might
avoid wasting time worrying about it.)

I like the patch so far.  Thanks for the pleasant reading.

Re: [PATCH 1/7] Make is_git_command() usable outside builtin-help

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 707 bytes --]

On Sat, Jul 26, 2008 at 01:12:36PM -0500, Jonathan Nieder <jrnieder@uchicago.edu> wrote:
> On Sat, 26 Jul 2008, Miklos Vajna wrote:
> 
> > +	if (!prefix)
> > +		prefix = "git-";
> > +       	prefix_len = strlen(prefix);
> 
> The whitespace gave me a start: the diff markup moved the prefix_len
> line to the next tab stop, so at first glance it seems there are missing
> braces here.  But it is an illusion.  (I mention this so others might
> avoid wasting time worrying about it.)

In fact it was a whitespace problem: somehow I used spaces there instead
of a tab. I fixed it locally. (Will send an updated series soon.)

> I like the patch so far.  Thanks for the pleasant reading.

:-)

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 2/7] builtin-help: change the current directory back in list_commands_in_dir()

[Miklos Vajna]

That function used to do a chdir() without switching back to the
original directory. That was not a problem till this function was used
only inside builtin-help, but once other builtins use it as well, this
is a problem, for example when the object database path is relative.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 help.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/help.c b/help.c
index fb93df0..d71937e 100644
--- a/help.c
+++ b/help.c
@@ -425,7 +425,9 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 	int prefix_len;
 	DIR *dir = opendir(path);
 	struct dirent *de;
+	static char old_path[PATH_MAX+1];
 
+	getcwd(old_path, sizeof(old_path));
 	if (!dir || chdir(path))
 		return 0;
 
@@ -452,6 +454,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 	}
 	closedir(dir);
 
+	chdir(old_path);
 	return longest;
 }
 
-- 
1.6.0.rc0.14.g95f8.dirty

Re: [PATCH 2/7] builtin-help: change the current directory back in list_commands_in_dir()

[Johannes Schindelin]

Hi,

On Sat, 26 Jul 2008, Miklos Vajna wrote:

> That function used to do a chdir() without switching back to the 
> original directory. That was not a problem till this function was used 
> only inside builtin-help, but once other builtins use it as well, this 
> is a problem, for example when the object database path is relative.

I had to work around that in my patch "git wrapper: DWIM mistyped 
commands", too :-)

Ciao,
Dscho

Re: [PATCH 2/7] builtin-help: change the current directory back in list_commands_in_dir()

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> That function used to do a chdir() without switching back to the
> original directory. That was not a problem till this function was used
> only inside builtin-help, but once other builtins use it as well, this
> is a problem, for example when the object database path is relative.

Why does it even need to chdir() around to begin with?  Doesn't opendir()
work just fine with relative path as an input?

Re: [PATCH 2/7] builtin-help: change the current directory back in list_commands_in_dir()

[Johannes Schindelin]

Hi,

On Sun, 27 Jul 2008, Junio C Hamano wrote:

> Miklos Vajna <vmiklos@frugalware.org> writes:
> 
> > That function used to do a chdir() without switching back to the 
> > original directory. That was not a problem till this function was used 
> > only inside builtin-help, but once other builtins use it as well, this 
> > is a problem, for example when the object database path is relative.
> 
> Why does it even need to chdir() around to begin with?  Doesn't 
> opendir() work just fine with relative path as an input?

It is a consequence of list_commands_in_dir() trying to be cute, and not 
having to construct the full path for the is_executable() check.

Will post a fix in a few minutes.

Ciao,
Dscho

[PATCH] Avoid chdir() in list_commands_in_dir()

[Johannes Schindelin]

The function list_commands_in_dir() tried to be lazy and just chdir()
to the directory which entries it listed, so that the check if the
file is executable could be done on dir->d_name.

However, there is no good reason to jump around wildly just to find
all Git commands.

Instead, have a strbuf and construct the full path dynamically.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 help.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/help.c b/help.c
index 480befe..7af6582 100644
--- a/help.c
+++ b/help.c
@@ -426,17 +426,24 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 	int prefix_len = strlen(prefix);
 	DIR *dir = opendir(path);
 	struct dirent *de;
+	struct strbuf buf = STRBUF_INIT;
+	int len;
 
-	if (!dir || chdir(path))
+	if (!dir)
 		return 0;
 
+	strbuf_addf(&buf, "%s/", path);
+	len = buf.len;
+
 	while ((de = readdir(dir)) != NULL) {
 		int entlen;
 
 		if (prefixcmp(de->d_name, prefix))
 			continue;
 
-		if (!is_executable(de->d_name))
+		strbuf_setlen(&buf, len);
+		strbuf_addstr(&buf, de->d_name);
+		if (!is_executable(buf.buf))
 			continue;
 
 		entlen = strlen(de->d_name) - prefix_len;
@@ -449,6 +456,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 		add_cmdname(cmds, de->d_name + prefix_len, entlen);
 	}
 	closedir(dir);
+	strbuf_release(&buf);
 
 	return longest;
 }
-- 
1.6.0.rc0.97.ge2309

[PATCH 3/7] builtin-help: make list_commands() a bit more generic

[Miklos Vajna]

That function now takes two paramters to control the prefix of the
listed commands, and a second parameter to specify the title of the
table. This can be useful for listing not only all git commands, but
specific ones, like merge strategies.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 help.c |   10 +++++-----
 help.h |    1 +
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/help.c b/help.c
index d71937e..f71fff4 100644
--- a/help.c
+++ b/help.c
@@ -501,13 +501,13 @@ static unsigned int load_command_list(const char *prefix)
 	return longest;
 }
 
-static void list_commands(void)
+void list_commands(const char *prefix, const char *title)
 {
-	unsigned int longest = load_command_list(NULL);
+	unsigned int longest = load_command_list(prefix);
 	const char *exec_path = git_exec_path();
 
 	if (main_cmds.cnt) {
-		printf("available git commands in '%s'\n", exec_path);
+		printf("available %s in '%s'\n", title, exec_path);
 		printf("----------------------------");
 		mput_char('-', strlen(exec_path));
 		putchar('\n');
@@ -516,7 +516,7 @@ static void list_commands(void)
 	}
 
 	if (other_cmds.cnt) {
-		printf("git commands available from elsewhere on your $PATH\n");
+		printf("%s available from elsewhere on your $PATH\n", title);
 		printf("---------------------------------------------------\n");
 		pretty_print_string_list(&other_cmds, longest);
 		putchar('\n');
@@ -697,7 +697,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	if (show_all) {
 		printf("usage: %s\n\n", git_usage_string);
-		list_commands();
+		list_commands("git-", "git commands");
 		printf("%s\n", git_more_info_string);
 		return 0;
 	}
diff --git a/help.h b/help.h
index 73da8d6..0741662 100644
--- a/help.h
+++ b/help.h
@@ -2,5 +2,6 @@
 #define HELP_H
 
 int is_git_command(const char *s, const char *prefix);
+void list_commands(const char *prefix, const char *title);
 
 #endif /* HELP_H */
-- 
1.6.0.rc0.14.g95f8.dirty

Re: [PATCH 3/7] builtin-help: make list_commands() a bit more generic

[Jonathan Nieder]

On Sat, 26 Jul 2008, Miklos Vajna wrote:

> -static void list_commands(void)
> +void list_commands(const char *prefix, const char *title)
>  {
> -	unsigned int longest = load_command_list(NULL);
> +	unsigned int longest = load_command_list(prefix);
>  	const char *exec_path = git_exec_path();
>  
>  	if (main_cmds.cnt) {
> -		printf("available git commands in '%s'\n", exec_path);
> +		printf("available %s in '%s'\n", title, exec_path);
>  		printf("----------------------------");
>  		mput_char('-', strlen(exec_path));
>  		putchar('\n');

Should this be

	printf("available %s in '%s'\n", title, exec_path);
	printf("----------------");
	mput_char('-', strlen(exec_path) + strlen(title));
	putchar('\n');

?

(same question goes for the if(other_cmds.cnt) block, too)

[PATCH 3/7] builtin-help: make list_commands() a bit more generic

[Miklos Vajna]

That function now takes two paramters to control the prefix of the
listed commands, and a second parameter to specify the title of the
table. This can be useful for listing not only all git commands, but
specific ones, like merge strategies.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Sat, Jul 26, 2008 at 01:28:39PM -0500, Jonathan Nieder <jrnieder@uchicago.edu> wrote:
> >     if (main_cmds.cnt) {
> > -           printf("available git commands in '%s'\n", exec_path);
> > +           printf("available %s in '%s'\n", title, exec_path);
> >             printf("----------------------------");
> >             mput_char('-', strlen(exec_path));
> >             putchar('\n');
>
> Should this be
>
>       printf("available %s in '%s'\n", title, exec_path);
>       printf("----------------");
>       mput_char('-', strlen(exec_path) + strlen(title));
>       putchar('\n');
>
> ?
>
> (same question goes for the if(other_cmds.cnt) block, too)

Right. Here is an updated patch.

Also available at git://repo.or.cz/git/vmiklos.git, branch 'merge-custom'.

 help.c |   18 ++++++++++--------
 help.h |    1 +
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/help.c b/help.c
index d71937e..ec7999d 100644
--- a/help.c
+++ b/help.c
@@ -501,23 +501,25 @@ static unsigned int load_command_list(const char *prefix)
 	return longest;
 }
 
-static void list_commands(void)
+void list_commands(const char *prefix, const char *title)
 {
-	unsigned int longest = load_command_list(NULL);
+	unsigned int longest = load_command_list(prefix);
 	const char *exec_path = git_exec_path();
 
 	if (main_cmds.cnt) {
-		printf("available git commands in '%s'\n", exec_path);
-		printf("----------------------------");
-		mput_char('-', strlen(exec_path));
+		printf("available %s in '%s'\n", title, exec_path);
+		printf("----------------");
+		mput_char('-', strlen(title) + strlen(exec_path));
 		putchar('\n');
 		pretty_print_string_list(&main_cmds, longest);
 		putchar('\n');
 	}
 
 	if (other_cmds.cnt) {
-		printf("git commands available from elsewhere on your $PATH\n");
-		printf("---------------------------------------------------\n");
+		printf("%s available from elsewhere on your $PATH\n", title);
+		printf("---------------------------------------");
+		mput_char('-', strlen(title));
+		putchar('\n');
 		pretty_print_string_list(&other_cmds, longest);
 		putchar('\n');
 	}
@@ -697,7 +699,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	if (show_all) {
 		printf("usage: %s\n\n", git_usage_string);
-		list_commands();
+		list_commands("git-", "git commands");
 		printf("%s\n", git_more_info_string);
 		return 0;
 	}
diff --git a/help.h b/help.h
index 73da8d6..0741662 100644
--- a/help.h
+++ b/help.h
@@ -2,5 +2,6 @@
 #define HELP_H
 
 int is_git_command(const char *s, const char *prefix);
+void list_commands(const char *prefix, const char *title);
 
 #endif /* HELP_H */
-- 
1.6.0.rc0.14.g95f8.dirty

[PATCH 4/7] builtin-merge: allow using a custom strategy

[Miklos Vajna]

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().

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-merge.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index e78fa18..cdbc692 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -22,6 +22,7 @@
 #include "log-tree.h"
 #include "color.h"
 #include "rerere.h"
+#include "help.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -77,7 +78,7 @@ static int option_parse_message(const struct option *opt,
 static struct strategy *get_strategy(const char *name)
 {
 	int i;
-	struct strbuf err;
+	struct strategy *ret;
 
 	if (!name)
 		return NULL;
@@ -86,12 +87,16 @@ static struct strategy *get_strategy(const char *name)
 		if (!strcmp(name, all_strategy[i].name))
 			return &all_strategy[i];
 
-	strbuf_init(&err, 0);
-	for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
-		strbuf_addf(&err, " %s", all_strategy[i].name);
-	fprintf(stderr, "Could not find merge strategy '%s'.\n", name);
-	fprintf(stderr, "Available strategies are:%s.\n", err.buf);
-	exit(1);
+	if (!is_git_command(name, "git-merge-")) {
+		fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name);
+		list_commands("git-merge-", "strategies");
+		exit(1);
+	}
+
+	ret = xmalloc(sizeof(struct strategy));
+	memset(ret, 0, sizeof(struct strategy));
+	ret->name = xstrdup(name);
+	return ret;
 }
 
 static void append_strategy(struct strategy *s)
-- 
1.6.0.rc0.14.g95f8.dirty

[PATCH 5/7] Add a new test for using a custom merge strategy

[Miklos Vajna]

Testing is done by creating a simple git-merge-theirs strategy which is
the opposite of ours. Using this in real merges is not recommended but
it's perfect for our testing needs.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 t/t7606-merge-custom.sh |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 45 insertions(+), 0 deletions(-)
 create mode 100755 t/t7606-merge-custom.sh

diff --git a/t/t7606-merge-custom.sh b/t/t7606-merge-custom.sh
new file mode 100755
index 0000000..f295e56
--- /dev/null
+++ b/t/t7606-merge-custom.sh
@@ -0,0 +1,45 @@
+#!/bin/sh
+
+test_description='git-merge
+
+Testing a custom strategy.'
+
+. ./test-lib.sh
+
+cat > git-merge-theirs << EOF
+#!/bin/sh
+eval git read-tree --reset -u \\\$\$#
+EOF
+chmod +x git-merge-theirs
+PATH=.:$PATH
+export PATH
+
+test_expect_success 'setup' '
+	echo c0 > c0.c &&
+	git add c0.c &&
+	git commit -m c0 &&
+	git tag c0 &&
+	echo c1 > c1.c &&
+	git add c1.c &&
+	git commit -m c1 &&
+	git tag c1 &&
+	git reset --hard c0 &&
+	echo c2 > c2.c &&
+	git add c2.c &&
+	git commit -m c2 &&
+	git tag c2
+'
+
+test_expect_success 'merge c2 with a custom strategy' '
+	git reset --hard c1 &&
+	git merge -s theirs c2 &&
+	test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" &&
+	test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" &&
+	test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
+	git diff --exit-code &&
+	test -f c0.c &&
+	test ! -f c1.c &&
+	test -f c2.c
+'
+
+test_done
-- 
1.6.0.rc0.14.g95f8.dirty

[PATCH 6/7] builtin-help: make it possible to exclude some commands in list_commands()

[Miklos Vajna]

The supposed method is to build a list of commands to be excluded using
add_cmdname(), then pass the list as the new exclude parameter. If no
exclude is needed, NULL should be used.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 help.c |   24 ++++++++++--------------
 help.h |   14 +++++++++++++-
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/help.c b/help.c
index f71fff4..de1be6d 100644
--- a/help.c
+++ b/help.c
@@ -9,6 +9,7 @@
 #include "common-cmds.h"
 #include "parse-options.h"
 #include "run-command.h"
+#include "help.h"
 
 static struct man_viewer_list {
 	struct man_viewer_list *next;
@@ -300,16 +301,9 @@ static inline void mput_char(char c, unsigned int num)
 		putchar(c);
 }
 
-static struct cmdnames {
-	int alloc;
-	int cnt;
-	struct cmdname {
-		size_t len;
-		char name[1];
-	} **names;
-} main_cmds, other_cmds;
+struct cmdnames main_cmds, other_cmds;
 
-static void add_cmdname(struct cmdnames *cmds, const char *name, int len)
+void add_cmdname(struct cmdnames *cmds, const char *name, int len)
 {
 	struct cmdname *ent = xmalloc(sizeof(*ent) + len);
 
@@ -458,7 +452,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 	return longest;
 }
 
-static unsigned int load_command_list(const char *prefix)
+static unsigned int load_command_list(const char *prefix, struct cmdnames *exclude)
 {
 	unsigned int longest = 0;
 	unsigned int len;
@@ -497,13 +491,15 @@ static unsigned int load_command_list(const char *prefix)
 	      sizeof(*other_cmds.names), cmdname_compare);
 	uniq(&other_cmds);
 	exclude_cmds(&other_cmds, &main_cmds);
+	if (exclude)
+		exclude_cmds(&main_cmds, exclude);
 
 	return longest;
 }
 
-void list_commands(const char *prefix, const char *title)
+void list_commands(const char *prefix, const char *title, struct cmdnames *exclude)
 {
-	unsigned int longest = load_command_list(prefix);
+	unsigned int longest = load_command_list(prefix, exclude);
 	const char *exec_path = git_exec_path();
 
 	if (main_cmds.cnt) {
@@ -551,7 +547,7 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s)
 
 int is_git_command(const char *s, const char *prefix)
 {
-	load_command_list(prefix);
+	load_command_list(prefix, NULL);
 	return is_in_cmdlist(&main_cmds, s) ||
 		is_in_cmdlist(&other_cmds, s);
 }
@@ -697,7 +693,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	if (show_all) {
 		printf("usage: %s\n\n", git_usage_string);
-		list_commands("git-", "git commands");
+		list_commands("git-", "git commands", NULL);
 		printf("%s\n", git_more_info_string);
 		return 0;
 	}
diff --git a/help.h b/help.h
index 0741662..85d3b74 100644
--- a/help.h
+++ b/help.h
@@ -1,7 +1,19 @@
 #ifndef HELP_H
 #define HELP_H
 
+struct cmdnames {
+	int alloc;
+	int cnt;
+	struct cmdname {
+		size_t len;
+		char name[1];
+	} **names;
+};
+
 int is_git_command(const char *s, const char *prefix);
-void list_commands(const char *prefix, const char *title);
+void list_commands(const char *prefix, const char *title, struct cmdnames *exclude);
+void add_cmdname(struct cmdnames *cmds, const char *name, int len);
+
+extern struct cmdnames main_cmds, other_cmds;
 
 #endif /* HELP_H */
-- 
1.6.0.rc0.14.g95f8.dirty

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

[Miklos Vajna]

If an invalid strategy is supplied, like -s foobar, then git-merge
listed all git-merge-* commands. This is not perfect, since for example
git-merge-index is not a valid strategy.

These are now removed from the output by scanning the list of main
commands; if the git-merge-foo command is listed in the all_strategy
list, then it's shown, otherwise excluded. This does not exclude
commands somewhere else in the PATH, where custom strategies are
expected.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-merge.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index cdbc692..4084e07 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -88,8 +88,19 @@ static struct strategy *get_strategy(const char *name)
 			return &all_strategy[i];
 
 	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;
+			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));
+		}
 		fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name);
-		list_commands("git-merge-", "strategies");
+		list_commands("git-merge-", "strategies", &not_strategies);
 		exit(1);
 	}
 
-- 
1.6.0.rc0.14.g95f8.dirty

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

[Johannes Schindelin]

Hi,

On Sat, 26 Jul 2008, Miklos Vajna 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?

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

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.

Ciao,
Dscho

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

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 2890 bytes --]

On Sat, Jul 26, 2008 at 05:08:11PM +0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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 #2: Type: application/pgp-signature, Size: 197 bytes --]

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

[Miklos Vajna]

If an invalid strategy is supplied, like -s foobar, then git-merge
listed all git-merge-* commands. This is not perfect, since for example
git-merge-index is not a valid strategy.

These are now removed from the output by scanning the list of main
commands; if the git-merge-foo command is listed in the all_strategy
list, then it's shown, otherwise excluded. This does not exclude
commands somewhere else in the PATH, where custom strategies are
expected.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

I just realized I can resend the last patch as the others are unchanged,
so here it is.

 builtin-merge.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index cdbc692..b0d1de5 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -88,8 +88,20 @@ static struct strategy *get_strategy(const char *name)
 			return &all_strategy[i];
 
 	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;
+			char *ent = main_cmds.names[i];
+			for (j = 0; j < ARRAY_SIZE(all_strategy); j++)
+				if (!strncmp(ent->name, all_strategy[j].name, ent->len))
+					found = 1;
+			if (!found)
+				add_cmdname(&not_strategies, ent->name, ent->len);
+		}
 		fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name);
-		list_commands("git-merge-", "strategies");
+		list_commands("git-merge-", "strategies", &not_strategies);
 		exit(1);
 	}
 
-- 
1.6.0.rc0.14.g95f8.dirty

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

[Johannes Schindelin]

Hi,

On Sat, 26 Jul 2008, Miklos Vajna wrote:

> On Sat, Jul 26, 2008 at 05:08:11PM +0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

Thanks.

> -				if (!strcmp(main_cmds.names[i]->name, all_strategy[j].name))
> +				if (!strncmp(ent->name, all_strategy[j].name, ent->len))

Oops... that is not what I meant.  You'd have to check if 
!all_strategy[j].name[ent->len], too...

Ciao,
Dscho

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

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 725 bytes --]

On Sat, Jul 26, 2008 at 05:38:55PM +0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > -				if (!strcmp(main_cmds.names[i]->name, all_strategy[j].name))
> > +				if (!strncmp(ent->name, all_strategy[j].name, ent->len))
> 
> Oops... that is not what I meant.  You'd have to check if 
> !all_strategy[j].name[ent->len], too...

Hmm. So let's say ent->name is "ours.exe", ent->len is set to 4.

Then !strncmp(ent->name, all_strategy[j].name, ent->len) will be true,
and the command will not be added to the exclude list.

However, if I check for !all_strategy[j].name[ent->len], that will be
false, so 'ours' will be excluded from the available strategy list.

Have I missed something?

Thanks.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

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

[Johannes Schindelin]

Hi,

On Sat, 26 Jul 2008, Miklos Vajna wrote:

> On Sat, Jul 26, 2008 at 05:38:55PM +0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > > -				if (!strcmp(main_cmds.names[i]->name, all_strategy[j].name))
> > > +				if (!strncmp(ent->name, all_strategy[j].name, ent->len))
> > 
> > Oops... that is not what I meant.  You'd have to check if 
> > !all_strategy[j].name[ent->len], too...
> 
> Hmm. So let's say ent->name is "ours.exe", ent->len is set to 4.
> 
> Then !strncmp(ent->name, all_strategy[j].name, ent->len) will be true,
> and the command will not be added to the exclude list.

What I meant is: if you change the strcmp to strncmp because one of the 
both strings is not supposed to be NUL terminated, you still want to make 
sure that one is not a strict prefix of the other.

Ciao,
Dscho

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

[Miklos Vajna]

If an invalid strategy is supplied, like -s foobar, then git-merge
listed all git-merge-* commands. This is not perfect, since for example
git-merge-index is not a valid strategy.

These are now removed from the output by scanning the list of main
commands; if the git-merge-foo command is listed in the all_strategy
list, then it's shown, otherwise excluded. This does not exclude
commands somewhere else in the PATH, where custom strategies are
expected.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Sat, Jul 26, 2008 at 07:01:16PM +0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > Hmm. So let's say ent->name is "ours.exe", ent->len is set to 4.
> >
> > Then !strncmp(ent->name, all_strategy[j].name, ent->len) will be
> > true,
> > and the command will not be added to the exclude list.
>
> What I meant is: if you change the strcmp to strncmp because one of
> the
> both strings is not supposed to be NUL terminated, you still want to
> make
> sure that one is not a strict prefix of the other.

Aah, I forgot about all_strategy[j].name is NUL terminated.

Updated patch below.

 builtin-merge.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index cdbc692..29826b1 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -88,8 +88,21 @@ static struct strategy *get_strategy(const char *name)
 			return &all_strategy[i];
 
 	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);
+		}
 		fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name);
-		list_commands("git-merge-", "strategies");
+		list_commands("git-merge-", "strategies", &not_strategies);
 		exit(1);
 	}
 
-- 
1.6.0.rc0.14.g95f8.dirty

[PATCH 0/6] Allow custom merge strategies, take 2

[Miklos Vajna]

Hi,

This is just a resend of my series, on top of Dscho's "Avoid chdir() in
list_commands_in_dir()" (f5d600e), including the suggestions I received
from Jonathan Nieder and Dscho.

Miklos Vajna (6):
  builtin-help: make is_git_command() usable outside builtin-help
  builtin-help: make list_commands() a bit more generic
  builtin-help: make it possible to exclude some commands in
    list_commands()
  builtin-merge: allow using a custom strategy
  builtin-merge: avoid non-strategy git-merge commands in error message
  Add a new test for using a custom merge strategy

 Makefile                |    1 +
 builtin-merge.c         |   32 +++++++++++++++++++++------
 help.c                  |   55 ++++++++++++++++++++++++-----------------------
 help.h                  |   19 ++++++++++++++++
 t/t7606-merge-custom.sh |   45 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 118 insertions(+), 34 deletions(-)
 create mode 100644 help.h
 create mode 100755 t/t7606-merge-custom.sh

[PATCH 1/6] builtin-help: make is_git_command() usable outside builtin-help

[Miklos Vajna]

Other builtins may want to check if a given command is a valid git
command or not as well. Additionally add a new parameter that specifies
a custom prefix, so that the "git-" prefix is no longer hardwired.
Useful for example to limit the search for "git-merge-*".

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 Makefile |    1 +
 help.c   |   25 ++++++++++++++-----------
 help.h   |    6 ++++++
 3 files changed, 21 insertions(+), 11 deletions(-)
 create mode 100644 help.h

diff --git a/Makefile b/Makefile
index 798a2f2..351afd7 100644
--- a/Makefile
+++ b/Makefile
@@ -355,6 +355,7 @@ LIB_H += git-compat-util.h
 LIB_H += graph.h
 LIB_H += grep.h
 LIB_H += hash.h
+LIB_H += help.h
 LIB_H += list-objects.h
 LIB_H += ll-merge.h
 LIB_H += log-tree.h
diff --git a/help.c b/help.c
index 3cb1962..08188f5 100644
--- a/help.c
+++ b/help.c
@@ -418,11 +418,11 @@ static int is_executable(const char *name)
 }
 
 static unsigned int list_commands_in_dir(struct cmdnames *cmds,
-					 const char *path)
+					 const char *path,
+					 const char *prefix)
 {
 	unsigned int longest = 0;
-	const char *prefix = "git-";
-	int prefix_len = strlen(prefix);
+	int prefix_len;
 	DIR *dir = opendir(path);
 	struct dirent *de;
 	struct strbuf buf = STRBUF_INIT;
@@ -430,6 +430,9 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 
 	if (!dir)
 		return 0;
+	if (!prefix)
+		prefix = "git-";
+	prefix_len = strlen(prefix);
 
 	strbuf_addf(&buf, "%s/", path);
 	len = buf.len;
@@ -460,7 +463,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 	return longest;
 }
 
-static unsigned int load_command_list(void)
+static unsigned int load_command_list(const char *prefix)
 {
 	unsigned int longest = 0;
 	unsigned int len;
@@ -469,7 +472,7 @@ static unsigned int load_command_list(void)
 	const char *exec_path = git_exec_path();
 
 	if (exec_path)
-		longest = list_commands_in_dir(&main_cmds, exec_path);
+		longest = list_commands_in_dir(&main_cmds, exec_path, prefix);
 
 	if (!env_path) {
 		fprintf(stderr, "PATH not set\n");
@@ -481,7 +484,7 @@ static unsigned int load_command_list(void)
 		if ((colon = strchr(path, PATH_SEP)))
 			*colon = 0;
 
-		len = list_commands_in_dir(&other_cmds, path);
+		len = list_commands_in_dir(&other_cmds, path, prefix);
 		if (len > longest)
 			longest = len;
 
@@ -505,7 +508,7 @@ static unsigned int load_command_list(void)
 
 static void list_commands(void)
 {
-	unsigned int longest = load_command_list();
+	unsigned int longest = load_command_list(NULL);
 	const char *exec_path = git_exec_path();
 
 	if (main_cmds.cnt) {
@@ -551,9 +554,9 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s)
 	return 0;
 }
 
-static int is_git_command(const char *s)
+int is_git_command(const char *s, const char *prefix)
 {
-	load_command_list();
+	load_command_list(prefix);
 	return is_in_cmdlist(&main_cmds, s) ||
 		is_in_cmdlist(&other_cmds, s);
 }
@@ -574,7 +577,7 @@ static const char *cmd_to_page(const char *git_cmd)
 		return "git";
 	else if (!prefixcmp(git_cmd, "git"))
 		return git_cmd;
-	else if (is_git_command(git_cmd))
+	else if (is_git_command(git_cmd, NULL))
 		return prepend("git-", git_cmd);
 	else
 		return prepend("git", git_cmd);
@@ -712,7 +715,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	}
 
 	alias = alias_lookup(argv[0]);
-	if (alias && !is_git_command(argv[0])) {
+	if (alias && !is_git_command(argv[0], NULL)) {
 		printf("`git %s' is aliased to `%s'\n", argv[0], alias);
 		return 0;
 	}
diff --git a/help.h b/help.h
new file mode 100644
index 0000000..73da8d6
--- /dev/null
+++ b/help.h
@@ -0,0 +1,6 @@
+#ifndef HELP_H
+#define HELP_H
+
+int is_git_command(const char *s, const char *prefix);
+
+#endif /* HELP_H */
-- 
1.6.0.rc0.14.g95f8.dirty

[PATCH 2/6] builtin-help: make list_commands() a bit more generic

[Miklos Vajna]

That function now takes two paramters to control the prefix of the
listed commands, and a second parameter to specify the title of the
table. This can be useful for listing not only all git commands, but
specific ones, like merge strategies.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 help.c |   18 ++++++++++--------
 help.h |    1 +
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/help.c b/help.c
index 08188f5..7a42517 100644
--- a/help.c
+++ b/help.c
@@ -506,23 +506,25 @@ static unsigned int load_command_list(const char *prefix)
 	return longest;
 }
 
-static void list_commands(void)
+void list_commands(const char *prefix, const char *title)
 {
-	unsigned int longest = load_command_list(NULL);
+	unsigned int longest = load_command_list(prefix);
 	const char *exec_path = git_exec_path();
 
 	if (main_cmds.cnt) {
-		printf("available git commands in '%s'\n", exec_path);
-		printf("----------------------------");
-		mput_char('-', strlen(exec_path));
+		printf("available %s in '%s'\n", title, exec_path);
+		printf("----------------");
+		mput_char('-', strlen(title) + strlen(exec_path));
 		putchar('\n');
 		pretty_print_string_list(&main_cmds, longest);
 		putchar('\n');
 	}
 
 	if (other_cmds.cnt) {
-		printf("git commands available from elsewhere on your $PATH\n");
-		printf("---------------------------------------------------\n");
+		printf("%s available from elsewhere on your $PATH\n", title);
+		printf("---------------------------------------");
+		mput_char('-', strlen(title));
+		putchar('\n');
 		pretty_print_string_list(&other_cmds, longest);
 		putchar('\n');
 	}
@@ -702,7 +704,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	if (show_all) {
 		printf("usage: %s\n\n", git_usage_string);
-		list_commands();
+		list_commands("git-", "git commands");
 		printf("%s\n", git_more_info_string);
 		return 0;
 	}
diff --git a/help.h b/help.h
index 73da8d6..0741662 100644
--- a/help.h
+++ b/help.h
@@ -2,5 +2,6 @@
 #define HELP_H
 
 int is_git_command(const char *s, const char *prefix);
+void list_commands(const char *prefix, const char *title);
 
 #endif /* HELP_H */
-- 
1.6.0.rc0.14.g95f8.dirty

[PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands()

[Miklos Vajna]

The supposed method is to build a list of commands to be excluded using
add_cmdname(), then pass the list as the new exclude parameter. If no
exclude is needed, NULL should be used.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 help.c |   24 ++++++++++--------------
 help.h |   14 +++++++++++++-
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/help.c b/help.c
index 7a42517..318d6d6 100644
--- a/help.c
+++ b/help.c
@@ -9,6 +9,7 @@
 #include "common-cmds.h"
 #include "parse-options.h"
 #include "run-command.h"
+#include "help.h"
 
 static struct man_viewer_list {
 	struct man_viewer_list *next;
@@ -300,16 +301,9 @@ static inline void mput_char(char c, unsigned int num)
 		putchar(c);
 }
 
-static struct cmdnames {
-	int alloc;
-	int cnt;
-	struct cmdname {
-		size_t len;
-		char name[1];
-	} **names;
-} main_cmds, other_cmds;
+struct cmdnames main_cmds, other_cmds;
 
-static void add_cmdname(struct cmdnames *cmds, const char *name, int len)
+void add_cmdname(struct cmdnames *cmds, const char *name, int len)
 {
 	struct cmdname *ent = xmalloc(sizeof(*ent) + len);
 
@@ -463,7 +457,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 	return longest;
 }
 
-static unsigned int load_command_list(const char *prefix)
+static unsigned int load_command_list(const char *prefix, struct cmdnames *exclude)
 {
 	unsigned int longest = 0;
 	unsigned int len;
@@ -502,13 +496,15 @@ static unsigned int load_command_list(const char *prefix)
 	      sizeof(*other_cmds.names), cmdname_compare);
 	uniq(&other_cmds);
 	exclude_cmds(&other_cmds, &main_cmds);
+	if (exclude)
+		exclude_cmds(&main_cmds, exclude);
 
 	return longest;
 }
 
-void list_commands(const char *prefix, const char *title)
+void list_commands(const char *prefix, const char *title, struct cmdnames *exclude)
 {
-	unsigned int longest = load_command_list(prefix);
+	unsigned int longest = load_command_list(prefix, exclude);
 	const char *exec_path = git_exec_path();
 
 	if (main_cmds.cnt) {
@@ -558,7 +554,7 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s)
 
 int is_git_command(const char *s, const char *prefix)
 {
-	load_command_list(prefix);
+	load_command_list(prefix, NULL);
 	return is_in_cmdlist(&main_cmds, s) ||
 		is_in_cmdlist(&other_cmds, s);
 }
@@ -704,7 +700,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	if (show_all) {
 		printf("usage: %s\n\n", git_usage_string);
-		list_commands("git-", "git commands");
+		list_commands("git-", "git commands", NULL);
 		printf("%s\n", git_more_info_string);
 		return 0;
 	}
diff --git a/help.h b/help.h
index 0741662..85d3b74 100644
--- a/help.h
+++ b/help.h
@@ -1,7 +1,19 @@
 #ifndef HELP_H
 #define HELP_H
 
+struct cmdnames {
+	int alloc;
+	int cnt;
+	struct cmdname {
+		size_t len;
+		char name[1];
+	} **names;
+};
+
 int is_git_command(const char *s, const char *prefix);
-void list_commands(const char *prefix, const char *title);
+void list_commands(const char *prefix, const char *title, struct cmdnames *exclude);
+void add_cmdname(struct cmdnames *cmds, const char *name, int len);
+
+extern struct cmdnames main_cmds, other_cmds;
 
 #endif /* HELP_H */
-- 
1.6.0.rc0.14.g95f8.dirty

Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands()

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> diff --git a/help.h b/help.h
> index 0741662..85d3b74 100644
> --- a/help.h
> +++ b/help.h
> @@ -1,7 +1,19 @@
>  #ifndef HELP_H
>  #define HELP_H
>  
> +struct cmdnames {
> +	int alloc;
> +	int cnt;
> +	struct cmdname {
> +		size_t len;
> +		char name[1];
> +	} **names;
> +};

I thought we do this kind of thing using FLEX_ARRAY macro.  Is there any
reason its use is not appropriate here?

Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands()

[Johannes Schindelin]

Hi,

On Sun, 27 Jul 2008, Junio C Hamano wrote:

> Miklos Vajna <vmiklos@frugalware.org> writes:
> 
> > diff --git a/help.h b/help.h
> > index 0741662..85d3b74 100644
> > --- a/help.h
> > +++ b/help.h
> > @@ -1,7 +1,19 @@
> >  #ifndef HELP_H
> >  #define HELP_H
> >  
> > +struct cmdnames {
> > +	int alloc;
> > +	int cnt;
> > +	struct cmdname {
> > +		size_t len;
> > +		char name[1];
> > +	} **names;
> > +};
> 
> I thought we do this kind of thing using FLEX_ARRAY macro.  Is there any
> reason its use is not appropriate here?

I think that came up in the previous review round: the "name" member _is_ 
NUL-terminated, but could have a ".exe" suffix.  The "len" member has the 
length excluding ".exe".

Ciao,
Dscho

Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands()

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > +struct cmdnames {
>> > +	int alloc;
>> > +	int cnt;
>> > +	struct cmdname {
>> > +		size_t len;
>> > +		char name[1];
>> > +	} **names;
>> > +};
>> 
>> I thought we do this kind of thing using FLEX_ARRAY macro.  Is there any
>> reason its use is not appropriate here?
>
> I think that came up in the previous review round: the "name" member _is_ 
> NUL-terminated, but could have a ".exe" suffix.  The "len" member has the 
> length excluding ".exe".

Sorry, but I do understand what you are trying to explain.

Marking the flexible member at the end as "last_member[FLEX_ARRAY]" is
about a tiny bit of abstracting out how the exact decl syntax should look
like depending on the compiler.

For example, we have:

        struct git_attr {
                struct git_attr *next;
                unsigned h;
                int attr_nr;
                char name[FLEX_ARRAY];
        };

And h is _not_ the length of the name[] (it is a hash to help us find the
entry quickly).  Jjust like "len" is not the length of the name[] in your
structure.  In other words, marking as "last_member[FLEX_ARRAY]" does not
have anything to do with what other fields you have in the same structure.

Hmm, am I missing a bigger picture somewhere else?

Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> > +struct cmdnames {
>>> > +	int alloc;
>>> > +	int cnt;
>>> > +	struct cmdname {
>>> > +		size_t len;
>>> > +		char name[1];
>>> > +	} **names;
>>> > +};
>>> 
>>> I thought we do this kind of thing using FLEX_ARRAY macro.  Is there any
>>> reason its use is not appropriate here?
>>
>> I think that came up in the previous review round: the "name" member _is_ 
>> NUL-terminated, but could have a ".exe" suffix.  The "len" member has the 
>> length excluding ".exe".
>
> Sorry, but I do understand what you are trying to explain.

Yuck, stupid typo; s/do/do not/.

Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands()

[Johannes Schindelin]

Hi,

On Sun, 27 Jul 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > +struct cmdnames {
> >> > +	int alloc;
> >> > +	int cnt;
> >> > +	struct cmdname {
> >> > +		size_t len;
> >> > +		char name[1];
> >> > +	} **names;
> >> > +};
> >> 
> >> I thought we do this kind of thing using FLEX_ARRAY macro.  Is there any
> >> reason its use is not appropriate here?
> >
> > I think that came up in the previous review round: the "name" member _is_ 
> > NUL-terminated, but could have a ".exe" suffix.  The "len" member has the 
> > length excluding ".exe".
> 
> Sorry, but I do understand what you are trying to explain.
> 
> Marking the flexible member at the end as "last_member[FLEX_ARRAY]" is
> about a tiny bit of abstracting out how the exact decl syntax should look
> like depending on the compiler.

Ah, sorry, I misunderstood.  I thought your complaint was about something 
else.

Ciao,
Dscho

[PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands()

[Miklos Vajna]

The supposed method is to build a list of commands to be excluded using
add_cmdname(), then pass the list as the new exclude parameter. If no
exclude is needed, NULL should be used.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Sun, Jul 27, 2008 at 06:36:30PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> > +   struct cmdname {
> > +           size_t len;
> > +           char name[1];
> > +   } **names;
> > +};
>
> I thought we do this kind of thing using FLEX_ARRAY macro.  Is there
> any
> reason its use is not appropriate here?

Now I'm using it. :-)

Note that FLEX_ARRAY would be a drop-in replacement only in case it
would be name[0], so I needed

-       struct cmdname *ent = xmalloc(sizeof(*ent) + len);
+       struct cmdname *ent = xmalloc(sizeof(*ent) + len + 1);

later in add_cmdname().

 help.c |   26 +++++++++++---------------
 help.h |   14 +++++++++++++-
 2 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/help.c b/help.c
index 7a42517..858f76a 100644
--- a/help.c
+++ b/help.c
@@ -9,6 +9,7 @@
 #include "common-cmds.h"
 #include "parse-options.h"
 #include "run-command.h"
+#include "help.h"
 
 static struct man_viewer_list {
 	struct man_viewer_list *next;
@@ -300,18 +301,11 @@ static inline void mput_char(char c, unsigned int num)
 		putchar(c);
 }
 
-static struct cmdnames {
-	int alloc;
-	int cnt;
-	struct cmdname {
-		size_t len;
-		char name[1];
-	} **names;
-} main_cmds, other_cmds;
+struct cmdnames main_cmds, other_cmds;
 
-static void add_cmdname(struct cmdnames *cmds, const char *name, int len)
+void add_cmdname(struct cmdnames *cmds, const char *name, int len)
 {
-	struct cmdname *ent = xmalloc(sizeof(*ent) + len);
+	struct cmdname *ent = xmalloc(sizeof(*ent) + len + 1);
 
 	ent->len = len;
 	memcpy(ent->name, name, len);
@@ -463,7 +457,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 	return longest;
 }
 
-static unsigned int load_command_list(const char *prefix)
+static unsigned int load_command_list(const char *prefix, struct cmdnames *exclude)
 {
 	unsigned int longest = 0;
 	unsigned int len;
@@ -502,13 +496,15 @@ static unsigned int load_command_list(const char *prefix)
 	      sizeof(*other_cmds.names), cmdname_compare);
 	uniq(&other_cmds);
 	exclude_cmds(&other_cmds, &main_cmds);
+	if (exclude)
+		exclude_cmds(&main_cmds, exclude);
 
 	return longest;
 }
 
-void list_commands(const char *prefix, const char *title)
+void list_commands(const char *prefix, const char *title, struct cmdnames *exclude)
 {
-	unsigned int longest = load_command_list(prefix);
+	unsigned int longest = load_command_list(prefix, exclude);
 	const char *exec_path = git_exec_path();
 
 	if (main_cmds.cnt) {
@@ -558,7 +554,7 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s)
 
 int is_git_command(const char *s, const char *prefix)
 {
-	load_command_list(prefix);
+	load_command_list(prefix, NULL);
 	return is_in_cmdlist(&main_cmds, s) ||
 		is_in_cmdlist(&other_cmds, s);
 }
@@ -704,7 +700,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	if (show_all) {
 		printf("usage: %s\n\n", git_usage_string);
-		list_commands("git-", "git commands");
+		list_commands("git-", "git commands", NULL);
 		printf("%s\n", git_more_info_string);
 		return 0;
 	}
diff --git a/help.h b/help.h
index 0741662..3eb8cfb 100644
--- a/help.h
+++ b/help.h
@@ -1,7 +1,19 @@
 #ifndef HELP_H
 #define HELP_H
 
+struct cmdnames {
+	int alloc;
+	int cnt;
+	struct cmdname {
+		size_t len;
+		char name[FLEX_ARRAY];
+	} **names;
+};
+
 int is_git_command(const char *s, const char *prefix);
-void list_commands(const char *prefix, const char *title);
+void list_commands(const char *prefix, const char *title, struct cmdnames *exclude);
+void add_cmdname(struct cmdnames *cmds, const char *name, int len);
+
+extern struct cmdnames main_cmds, other_cmds;
 
 #endif /* HELP_H */
-- 
1.6.0.rc0.14.g95f8.dirty

[PATCH 4/6] builtin-merge: allow using a custom strategy

[Miklos Vajna]

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().

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-merge.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index e78fa18..cdbc692 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -22,6 +22,7 @@
 #include "log-tree.h"
 #include "color.h"
 #include "rerere.h"
+#include "help.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -77,7 +78,7 @@ static int option_parse_message(const struct option *opt,
 static struct strategy *get_strategy(const char *name)
 {
 	int i;
-	struct strbuf err;
+	struct strategy *ret;
 
 	if (!name)
 		return NULL;
@@ -86,12 +87,16 @@ static struct strategy *get_strategy(const char *name)
 		if (!strcmp(name, all_strategy[i].name))
 			return &all_strategy[i];
 
-	strbuf_init(&err, 0);
-	for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
-		strbuf_addf(&err, " %s", all_strategy[i].name);
-	fprintf(stderr, "Could not find merge strategy '%s'.\n", name);
-	fprintf(stderr, "Available strategies are:%s.\n", err.buf);
-	exit(1);
+	if (!is_git_command(name, "git-merge-")) {
+		fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name);
+		list_commands("git-merge-", "strategies");
+		exit(1);
+	}
+
+	ret = xmalloc(sizeof(struct strategy));
+	memset(ret, 0, sizeof(struct strategy));
+	ret->name = xstrdup(name);
+	return ret;
 }
 
 static void append_strategy(struct strategy *s)
-- 
1.6.0.rc0.14.g95f8.dirty

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

[Miklos Vajna]

If an invalid strategy is supplied, like -s foobar, then git-merge
listed all git-merge-* commands. This is not perfect, since for example
git-merge-index is not a valid strategy.

These are now removed from the output by scanning the list of main
commands; if the git-merge-foo command is listed in the all_strategy
list, then it's shown, otherwise excluded. This does not exclude
commands somewhere else in the PATH, where custom strategies are
expected.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-merge.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index cdbc692..29826b1 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -88,8 +88,21 @@ static struct strategy *get_strategy(const char *name)
 			return &all_strategy[i];
 
 	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);
+		}
 		fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name);
-		list_commands("git-merge-", "strategies");
+		list_commands("git-merge-", "strategies", &not_strategies);
 		exit(1);
 	}
 
-- 
1.6.0.rc0.14.g95f8.dirty

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

[Johannes Schindelin]

Hi,

On Mon, 28 Jul 2008, Miklos Vajna wrote:

> -		list_commands("git-merge-", "strategies");
> +		list_commands("git-merge-", "strategies", &not_strategies);

The change in the signature of list_commands() is not part of this patch.  
So at least one of your commits should result in an uncompileable 
revision...

Ciao,
Dscho

[PATCH] builtin-merge: allow using a custom strategy

[Miklos Vajna]

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.

These are removed from the output by scanning the list of main commands;
if the git-merge-foo command is listed in the all_strategy list, then
it's shown, otherwise excluded. This does not exclude commands somewhere
else in the PATH, where custom strategies are expected.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Mon, Jul 28, 2008 at 03:06:09PM +0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> The change in the signature of list_commands() is not part of this
> patch.  So at least one of your commits should result in an
> uncompileable revision...

Right. I just squashed patch 4 and 5, and this solves the problem: patch
3 changes the signature and fixes all usage (in help.c, it's not used
outside help.c), then the squashed patch introduces the usage of the new
list_commands() in builtin-merge.c.

(Also in the 'merge-custom' branch of vmiklos.git on repo.or.cz.)

 builtin-merge.c |   32 +++++++++++++++++++++++++-------
 1 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index e78fa18..29826b1 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -22,6 +22,7 @@
 #include "log-tree.h"
 #include "color.h"
 #include "rerere.h"
+#include "help.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -77,7 +78,7 @@ static int option_parse_message(const struct option *opt,
 static struct strategy *get_strategy(const char *name)
 {
 	int i;
-	struct strbuf err;
+	struct strategy *ret;
 
 	if (!name)
 		return NULL;
@@ -86,12 +87,29 @@ static struct strategy *get_strategy(const char *name)
 		if (!strcmp(name, all_strategy[i].name))
 			return &all_strategy[i];
 
-	strbuf_init(&err, 0);
-	for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
-		strbuf_addf(&err, " %s", all_strategy[i].name);
-	fprintf(stderr, "Could not find merge strategy '%s'.\n", name);
-	fprintf(stderr, "Available strategies are:%s.\n", err.buf);
-	exit(1);
+	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);
+		}
+		fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name);
+		list_commands("git-merge-", "strategies", &not_strategies);
+		exit(1);
+	}
+
+	ret = xmalloc(sizeof(struct strategy));
+	memset(ret, 0, sizeof(struct strategy));
+	ret->name = xstrdup(name);
+	return ret;
 }
 
 static void append_strategy(struct strategy *s)
-- 
1.6.0.rc0.14.g95f8.dirty

Re: [PATCH] builtin-merge: allow using a custom strategy

[Johannes Schindelin]

Hi,

On Tue, 29 Jul 2008, Miklos Vajna wrote:

> On Mon, Jul 28, 2008 at 03:06:09PM +0200, Johannes Schindelin 
> <Johannes.Schindelin@gmx.de> wrote:
>
> > The change in the signature of list_commands() is not part of this 
> > patch.  So at least one of your commits should result in an 
> > uncompileable revision...
> 
> Right. I just squashed patch 4 and 5, and this solves the problem: patch 
> 3 changes the signature and fixes all usage (in help.c, it's not used 
> outside help.c), then the squashed patch introduces the usage of the new 
> list_commands() in builtin-merge.c.

Thanks,
Dscho

[PATCH 6/6] Add a new test for using a custom merge strategy

[Miklos Vajna]

Testing is done by creating a simple git-merge-theirs strategy which is
the opposite of ours. Using this in real merges is not recommended but
it's perfect for our testing needs.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 t/t7606-merge-custom.sh |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 45 insertions(+), 0 deletions(-)
 create mode 100755 t/t7606-merge-custom.sh

diff --git a/t/t7606-merge-custom.sh b/t/t7606-merge-custom.sh
new file mode 100755
index 0000000..f295e56
--- /dev/null
+++ b/t/t7606-merge-custom.sh
@@ -0,0 +1,45 @@
+#!/bin/sh
+
+test_description='git-merge
+
+Testing a custom strategy.'
+
+. ./test-lib.sh
+
+cat > git-merge-theirs << EOF
+#!/bin/sh
+eval git read-tree --reset -u \\\$\$#
+EOF
+chmod +x git-merge-theirs
+PATH=.:$PATH
+export PATH
+
+test_expect_success 'setup' '
+	echo c0 > c0.c &&
+	git add c0.c &&
+	git commit -m c0 &&
+	git tag c0 &&
+	echo c1 > c1.c &&
+	git add c1.c &&
+	git commit -m c1 &&
+	git tag c1 &&
+	git reset --hard c0 &&
+	echo c2 > c2.c &&
+	git add c2.c &&
+	git commit -m c2 &&
+	git tag c2
+'
+
+test_expect_success 'merge c2 with a custom strategy' '
+	git reset --hard c1 &&
+	git merge -s theirs c2 &&
+	test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" &&
+	test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" &&
+	test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
+	git diff --exit-code &&
+	test -f c0.c &&
+	test ! -f c1.c &&
+	test -f c2.c
+'
+
+test_done
-- 
1.6.0.rc0.14.g95f8.dirty

Re: [PATCH 6/6] Add a new test for using a custom merge strategy

[Johannes Schindelin]

Hi,

On Mon, 28 Jul 2008, Miklos Vajna wrote:

> Testing is done by creating a simple git-merge-theirs strategy which is 
> the opposite of ours. Using this in real merges is not recommended but 
> it's perfect for our testing needs.

Note that what was asked for, and what Junio implemented before deciding 
that it would do more harm than good in git.git, is not the same as what 
you provide.

Your -theirs is a strict opposite of -ours, i.e. the tree after the 
merge will be identical to the "merged" branch's tip's.

The -theirs which was asked for (and which I truly think is insane) wanted 
to do a merge, and in case of merge conflicts take the "upstream" version 
_only of the conflicting hunks_.

Just to make sure everyone grasps why this is bad:

- there is not only a real chance, but a high probability that a merge 
  conflict means that some related, unconflicting change relies on _one_ 
  version of the conflicting hunk which might very well be "ours", and

- since we have a _recursive_ merge, the notion of "upstream" is 
  completely bogus when working on any merge that has more than one merge 
  base.  This merge would succeed, but actively be wrong.

Just to make sure people do not have to ask for that version of 
"-theirs" again,
Dscho

[PATCH 6/6] Add a new test for using a custom merge strategy

[Miklos Vajna]

Testing is done by creating a simple git-merge-theirs strategy which
just picks the upstream tree. (In other words, this is not the opposite
of -s ours.)

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Mon, Jul 28, 2008 at 03:12:59PM +0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Note that what was asked for, and what Junio implemented before
> deciding
> that it would do more harm than good in git.git, is not the same as
> what
> you provide.

Thanks, now I see the difference. The updated commit message is
hopefully better.

Also I added a check to make sure the upstream and the result tree is
the same as well.

 t/t7606-merge-custom.sh |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)
 create mode 100755 t/t7606-merge-custom.sh

diff --git a/t/t7606-merge-custom.sh b/t/t7606-merge-custom.sh
new file mode 100755
index 0000000..13e8ff5
--- /dev/null
+++ b/t/t7606-merge-custom.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+
+test_description='git-merge
+
+Testing a custom strategy.'
+
+. ./test-lib.sh
+
+cat > git-merge-theirs << EOF
+#!/bin/sh
+eval git read-tree --reset -u \\\$\$#
+EOF
+chmod +x git-merge-theirs
+PATH=.:$PATH
+export PATH
+
+test_expect_success 'setup' '
+	echo c0 > c0.c &&
+	git add c0.c &&
+	git commit -m c0 &&
+	git tag c0 &&
+	echo c1 > c1.c &&
+	git add c1.c &&
+	git commit -m c1 &&
+	git tag c1 &&
+	git reset --hard c0 &&
+	echo c2 > c2.c &&
+	git add c2.c &&
+	git commit -m c2 &&
+	git tag c2
+'
+
+test_expect_success 'merge c2 with a custom strategy' '
+	git reset --hard c1 &&
+	git merge -s theirs c2 &&
+	test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" &&
+	test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" &&
+	test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
+	test "$(git rev-parse c2^{tree})" = "$(git rev-parse HEAD^{tree})" &&
+	git diff --exit-code &&
+	test -f c0.c &&
+	test ! -f c1.c &&
+	test -f c2.c
+'
+
+test_done
-- 
1.6.0.rc0.14.g95f8.dirty

Re: [PATCH 6/6] Add a new test for using a custom merge strategy

[Johannes Schindelin]

Hi,

On Tue, 29 Jul 2008, Miklos Vajna wrote:

> Testing is done by creating a simple git-merge-theirs strategy which
> just picks the upstream tree. (In other words, this is not the opposite
> of -s ours.)

Actually, this _is_ the opposite of -s ours, no?  -s ours just takes our 
tree, your -s theirs just takes their tree.

Sorry for the confusion I caused,
Dscho

Re: [PATCH 6/6] Add a new test for using a custom merge strategy

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

On Tue, Jul 29, 2008 at 01:54:17AM +0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Actually, this _is_ the opposite of -s ours, no?  -s ours just takes our 
> tree, your -s theirs just takes their tree.
> 
> Sorry for the confusion I caused,

Aah. :-)

I did not read the source of git-merge-ours and based on your
description I thought my knowledge / the doc about -s ours was not
correct.

So I guess my original patch was right, then.

Note to self: "take 2" gets messy, time to send a "take 3" soon. ;-)

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 6/6] Add a new test for using a custom merge strategy

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 204 bytes --]

On Tue, Jul 29, 2008 at 01:58:47AM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:
> So I guess my original patch was right, then.

s/patch/description/. The check for the same trees are still valid.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 0/5] Allow custom merge strategies, take 3

[Miklos Vajna]

Hi,

Changes from 'take 2':

- the testcase now makes sure the trees are the same for 'theirs' as
  well.

- patch 4 and 5 are squashed so there is no compile error after applying
  only some of the patches. (or when bisecting)

- using FLEX_ARRAY in help.h

- the first two patches are unchanged

Miklos Vajna (5):
  builtin-help: make is_git_command() usable outside builtin-help
  builtin-help: make list_commands() a bit more generic
  builtin-help: make it possible to exclude some commands in
    list_commands()
  builtin-merge: allow using a custom strategy
  Add a new test for using a custom merge strategy

 Makefile                |    1 +
 builtin-merge.c         |   32 ++++++++++++++++++++-----
 help.c                  |   57 ++++++++++++++++++++++++-----------------------
 help.h                  |   19 +++++++++++++++
 t/t7606-merge-custom.sh |   46 +++++++++++++++++++++++++++++++++++++
 5 files changed, 120 insertions(+), 35 deletions(-)
 create mode 100644 help.h
 create mode 100755 t/t7606-merge-custom.sh

[PATCH 1/5] builtin-help: make is_git_command() usable outside builtin-help

[Miklos Vajna]

Other builtins may want to check if a given command is a valid git
command or not as well. Additionally add a new parameter that specifies
a custom prefix, so that the "git-" prefix is no longer hardwired.
Useful for example to limit the search for "git-merge-*".

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 Makefile |    1 +
 help.c   |   25 ++++++++++++++-----------
 help.h   |    6 ++++++
 3 files changed, 21 insertions(+), 11 deletions(-)
 create mode 100644 help.h

diff --git a/Makefile b/Makefile
index 798a2f2..351afd7 100644
--- a/Makefile
+++ b/Makefile
@@ -355,6 +355,7 @@ LIB_H += git-compat-util.h
 LIB_H += graph.h
 LIB_H += grep.h
 LIB_H += hash.h
+LIB_H += help.h
 LIB_H += list-objects.h
 LIB_H += ll-merge.h
 LIB_H += log-tree.h
diff --git a/help.c b/help.c
index 3cb1962..08188f5 100644
--- a/help.c
+++ b/help.c
@@ -418,11 +418,11 @@ static int is_executable(const char *name)
 }
 
 static unsigned int list_commands_in_dir(struct cmdnames *cmds,
-					 const char *path)
+					 const char *path,
+					 const char *prefix)
 {
 	unsigned int longest = 0;
-	const char *prefix = "git-";
-	int prefix_len = strlen(prefix);
+	int prefix_len;
 	DIR *dir = opendir(path);
 	struct dirent *de;
 	struct strbuf buf = STRBUF_INIT;
@@ -430,6 +430,9 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 
 	if (!dir)
 		return 0;
+	if (!prefix)
+		prefix = "git-";
+	prefix_len = strlen(prefix);
 
 	strbuf_addf(&buf, "%s/", path);
 	len = buf.len;
@@ -460,7 +463,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 	return longest;
 }
 
-static unsigned int load_command_list(void)
+static unsigned int load_command_list(const char *prefix)
 {
 	unsigned int longest = 0;
 	unsigned int len;
@@ -469,7 +472,7 @@ static unsigned int load_command_list(void)
 	const char *exec_path = git_exec_path();
 
 	if (exec_path)
-		longest = list_commands_in_dir(&main_cmds, exec_path);
+		longest = list_commands_in_dir(&main_cmds, exec_path, prefix);
 
 	if (!env_path) {
 		fprintf(stderr, "PATH not set\n");
@@ -481,7 +484,7 @@ static unsigned int load_command_list(void)
 		if ((colon = strchr(path, PATH_SEP)))
 			*colon = 0;
 
-		len = list_commands_in_dir(&other_cmds, path);
+		len = list_commands_in_dir(&other_cmds, path, prefix);
 		if (len > longest)
 			longest = len;
 
@@ -505,7 +508,7 @@ static unsigned int load_command_list(void)
 
 static void list_commands(void)
 {
-	unsigned int longest = load_command_list();
+	unsigned int longest = load_command_list(NULL);
 	const char *exec_path = git_exec_path();
 
 	if (main_cmds.cnt) {
@@ -551,9 +554,9 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s)
 	return 0;
 }
 
-static int is_git_command(const char *s)
+int is_git_command(const char *s, const char *prefix)
 {
-	load_command_list();
+	load_command_list(prefix);
 	return is_in_cmdlist(&main_cmds, s) ||
 		is_in_cmdlist(&other_cmds, s);
 }
@@ -574,7 +577,7 @@ static const char *cmd_to_page(const char *git_cmd)
 		return "git";
 	else if (!prefixcmp(git_cmd, "git"))
 		return git_cmd;
-	else if (is_git_command(git_cmd))
+	else if (is_git_command(git_cmd, NULL))
 		return prepend("git-", git_cmd);
 	else
 		return prepend("git", git_cmd);
@@ -712,7 +715,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	}
 
 	alias = alias_lookup(argv[0]);
-	if (alias && !is_git_command(argv[0])) {
+	if (alias && !is_git_command(argv[0], NULL)) {
 		printf("`git %s' is aliased to `%s'\n", argv[0], alias);
 		return 0;
 	}
diff --git a/help.h b/help.h
new file mode 100644
index 0000000..73da8d6
--- /dev/null
+++ b/help.h
@@ -0,0 +1,6 @@
+#ifndef HELP_H
+#define HELP_H
+
+int is_git_command(const char *s, const char *prefix);
+
+#endif /* HELP_H */
-- 
1.6.0.rc0.14.g95f8.dirty

[PATCH 2/5] builtin-help: make list_commands() a bit more generic

[Miklos Vajna]

That function now takes two paramters to control the prefix of the
listed commands, and a second parameter to specify the title of the
table. This can be useful for listing not only all git commands, but
specific ones, like merge strategies.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 help.c |   18 ++++++++++--------
 help.h |    1 +
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/help.c b/help.c
index 08188f5..7a42517 100644
--- a/help.c
+++ b/help.c
@@ -506,23 +506,25 @@ static unsigned int load_command_list(const char *prefix)
 	return longest;
 }
 
-static void list_commands(void)
+void list_commands(const char *prefix, const char *title)
 {
-	unsigned int longest = load_command_list(NULL);
+	unsigned int longest = load_command_list(prefix);
 	const char *exec_path = git_exec_path();
 
 	if (main_cmds.cnt) {
-		printf("available git commands in '%s'\n", exec_path);
-		printf("----------------------------");
-		mput_char('-', strlen(exec_path));
+		printf("available %s in '%s'\n", title, exec_path);
+		printf("----------------");
+		mput_char('-', strlen(title) + strlen(exec_path));
 		putchar('\n');
 		pretty_print_string_list(&main_cmds, longest);
 		putchar('\n');
 	}
 
 	if (other_cmds.cnt) {
-		printf("git commands available from elsewhere on your $PATH\n");
-		printf("---------------------------------------------------\n");
+		printf("%s available from elsewhere on your $PATH\n", title);
+		printf("---------------------------------------");
+		mput_char('-', strlen(title));
+		putchar('\n');
 		pretty_print_string_list(&other_cmds, longest);
 		putchar('\n');
 	}
@@ -702,7 +704,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	if (show_all) {
 		printf("usage: %s\n\n", git_usage_string);
-		list_commands();
+		list_commands("git-", "git commands");
 		printf("%s\n", git_more_info_string);
 		return 0;
 	}
diff --git a/help.h b/help.h
index 73da8d6..0741662 100644
--- a/help.h
+++ b/help.h
@@ -2,5 +2,6 @@
 #define HELP_H
 
 int is_git_command(const char *s, const char *prefix);
+void list_commands(const char *prefix, const char *title);
 
 #endif /* HELP_H */
-- 
1.6.0.rc0.14.g95f8.dirty

[PATCH 3/5] builtin-help: make it possible to exclude some commands in list_commands()

[Miklos Vajna]

The supposed method is to build a list of commands to be excluded using
add_cmdname(), then pass the list as the new exclude parameter. If no
exclude is needed, NULL should be used.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 help.c |   26 +++++++++++---------------
 help.h |   14 +++++++++++++-
 2 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/help.c b/help.c
index 7a42517..858f76a 100644
--- a/help.c
+++ b/help.c
@@ -9,6 +9,7 @@
 #include "common-cmds.h"
 #include "parse-options.h"
 #include "run-command.h"
+#include "help.h"
 
 static struct man_viewer_list {
 	struct man_viewer_list *next;
@@ -300,18 +301,11 @@ static inline void mput_char(char c, unsigned int num)
 		putchar(c);
 }
 
-static struct cmdnames {
-	int alloc;
-	int cnt;
-	struct cmdname {
-		size_t len;
-		char name[1];
-	} **names;
-} main_cmds, other_cmds;
+struct cmdnames main_cmds, other_cmds;
 
-static void add_cmdname(struct cmdnames *cmds, const char *name, int len)
+void add_cmdname(struct cmdnames *cmds, const char *name, int len)
 {
-	struct cmdname *ent = xmalloc(sizeof(*ent) + len);
+	struct cmdname *ent = xmalloc(sizeof(*ent) + len + 1);
 
 	ent->len = len;
 	memcpy(ent->name, name, len);
@@ -463,7 +457,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 	return longest;
 }
 
-static unsigned int load_command_list(const char *prefix)
+static unsigned int load_command_list(const char *prefix, struct cmdnames *exclude)
 {
 	unsigned int longest = 0;
 	unsigned int len;
@@ -502,13 +496,15 @@ static unsigned int load_command_list(const char *prefix)
 	      sizeof(*other_cmds.names), cmdname_compare);
 	uniq(&other_cmds);
 	exclude_cmds(&other_cmds, &main_cmds);
+	if (exclude)
+		exclude_cmds(&main_cmds, exclude);
 
 	return longest;
 }
 
-void list_commands(const char *prefix, const char *title)
+void list_commands(const char *prefix, const char *title, struct cmdnames *exclude)
 {
-	unsigned int longest = load_command_list(prefix);
+	unsigned int longest = load_command_list(prefix, exclude);
 	const char *exec_path = git_exec_path();
 
 	if (main_cmds.cnt) {
@@ -558,7 +554,7 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s)
 
 int is_git_command(const char *s, const char *prefix)
 {
-	load_command_list(prefix);
+	load_command_list(prefix, NULL);
 	return is_in_cmdlist(&main_cmds, s) ||
 		is_in_cmdlist(&other_cmds, s);
 }
@@ -704,7 +700,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	if (show_all) {
 		printf("usage: %s\n\n", git_usage_string);
-		list_commands("git-", "git commands");
+		list_commands("git-", "git commands", NULL);
 		printf("%s\n", git_more_info_string);
 		return 0;
 	}
diff --git a/help.h b/help.h
index 0741662..3eb8cfb 100644
--- a/help.h
+++ b/help.h
@@ -1,7 +1,19 @@
 #ifndef HELP_H
 #define HELP_H
 
+struct cmdnames {
+	int alloc;
+	int cnt;
+	struct cmdname {
+		size_t len;
+		char name[FLEX_ARRAY];
+	} **names;
+};
+
 int is_git_command(const char *s, const char *prefix);
-void list_commands(const char *prefix, const char *title);
+void list_commands(const char *prefix, const char *title, struct cmdnames *exclude);
+void add_cmdname(struct cmdnames *cmds, const char *name, int len);
+
+extern struct cmdnames main_cmds, other_cmds;
 
 #endif /* HELP_H */
-- 
1.6.0.rc0.14.g95f8.dirty

Re: [PATCH 3/5] builtin-help: make it possible to exclude some commands in list_commands()

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> The supposed method is to build a list of commands to be excluded using
> add_cmdname(), then pass the list as the new exclude parameter. If no
> exclude is needed, NULL should be used.

You require that exclude is a sorted list; this should be documented
somewhere to avoid future misuses.

There is no need for adding extra qsort() anywhere in the patchset because
the only user you are adding is in the next patch that sends a subset of
the commands in main_cmds in the order they are found in the list, and
main_cmds is already sorted.

[PATCH 3/5] builtin-help: make it possible to exclude some commands in list_commands()

[Miklos Vajna]

The supposed method is to build a list of commands to be excluded using
add_cmdname(), then pass the list as the new exclude parameter. If no
exclude is needed, NULL should be used.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Tue, Jul 29, 2008 at 12:46:59PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> You require that exclude is a sorted list; this should be documented
> somewhere to avoid future misuses.

This version now adds a comment in load_command_list() about this.

 help.c |   27 ++++++++++++---------------
 help.h |   14 +++++++++++++-
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/help.c b/help.c
index 7a42517..26aa3e0 100644
--- a/help.c
+++ b/help.c
@@ -9,6 +9,7 @@
 #include "common-cmds.h"
 #include "parse-options.h"
 #include "run-command.h"
+#include "help.h"
 
 static struct man_viewer_list {
 	struct man_viewer_list *next;
@@ -300,18 +301,11 @@ static inline void mput_char(char c, unsigned int num)
 		putchar(c);
 }
 
-static struct cmdnames {
-	int alloc;
-	int cnt;
-	struct cmdname {
-		size_t len;
-		char name[1];
-	} **names;
-} main_cmds, other_cmds;
+struct cmdnames main_cmds, other_cmds;
 
-static void add_cmdname(struct cmdnames *cmds, const char *name, int len)
+void add_cmdname(struct cmdnames *cmds, const char *name, int len)
 {
-	struct cmdname *ent = xmalloc(sizeof(*ent) + len);
+	struct cmdname *ent = xmalloc(sizeof(*ent) + len + 1);
 
 	ent->len = len;
 	memcpy(ent->name, name, len);
@@ -463,7 +457,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 	return longest;
 }
 
-static unsigned int load_command_list(const char *prefix)
+static unsigned int load_command_list(const char *prefix, struct cmdnames *exclude)
 {
 	unsigned int longest = 0;
 	unsigned int len;
@@ -502,13 +496,16 @@ static unsigned int load_command_list(const char *prefix)
 	      sizeof(*other_cmds.names), cmdname_compare);
 	uniq(&other_cmds);
 	exclude_cmds(&other_cmds, &main_cmds);
+	if (exclude)
+		/* Here we require that exclude is a sorted list. */
+		exclude_cmds(&main_cmds, exclude);
 
 	return longest;
 }
 
-void list_commands(const char *prefix, const char *title)
+void list_commands(const char *prefix, const char *title, struct cmdnames *exclude)
 {
-	unsigned int longest = load_command_list(prefix);
+	unsigned int longest = load_command_list(prefix, exclude);
 	const char *exec_path = git_exec_path();
 
 	if (main_cmds.cnt) {
@@ -558,7 +555,7 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s)
 
 int is_git_command(const char *s, const char *prefix)
 {
-	load_command_list(prefix);
+	load_command_list(prefix, NULL);
 	return is_in_cmdlist(&main_cmds, s) ||
 		is_in_cmdlist(&other_cmds, s);
 }
@@ -704,7 +701,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	if (show_all) {
 		printf("usage: %s\n\n", git_usage_string);
-		list_commands("git-", "git commands");
+		list_commands("git-", "git commands", NULL);
 		printf("%s\n", git_more_info_string);
 		return 0;
 	}
diff --git a/help.h b/help.h
index 0741662..3eb8cfb 100644
--- a/help.h
+++ b/help.h
@@ -1,7 +1,19 @@
 #ifndef HELP_H
 #define HELP_H
 
+struct cmdnames {
+	int alloc;
+	int cnt;
+	struct cmdname {
+		size_t len;
+		char name[FLEX_ARRAY];
+	} **names;
+};
+
 int is_git_command(const char *s, const char *prefix);
-void list_commands(const char *prefix, const char *title);
+void list_commands(const char *prefix, const char *title, struct cmdnames *exclude);
+void add_cmdname(struct cmdnames *cmds, const char *name, int len);
+
+extern struct cmdnames main_cmds, other_cmds;
 
 #endif /* HELP_H */
-- 
1.6.0.rc0.14.g95f8.dirty

[PATCH 4/5] builtin-merge: allow using a custom strategy

[Miklos Vajna]

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.

These are removed from the output by scanning the list of main commands;
if the git-merge-foo command is listed in the all_strategy list, then
it's shown, otherwise excluded. This does not exclude commands somewhere
else in the PATH, where custom strategies are expected.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-merge.c |   32 +++++++++++++++++++++++++-------
 1 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index e78fa18..29826b1 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -22,6 +22,7 @@
 #include "log-tree.h"
 #include "color.h"
 #include "rerere.h"
+#include "help.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -77,7 +78,7 @@ static int option_parse_message(const struct option *opt,
 static struct strategy *get_strategy(const char *name)
 {
 	int i;
-	struct strbuf err;
+	struct strategy *ret;
 
 	if (!name)
 		return NULL;
@@ -86,12 +87,29 @@ static struct strategy *get_strategy(const char *name)
 		if (!strcmp(name, all_strategy[i].name))
 			return &all_strategy[i];
 
-	strbuf_init(&err, 0);
-	for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
-		strbuf_addf(&err, " %s", all_strategy[i].name);
-	fprintf(stderr, "Could not find merge strategy '%s'.\n", name);
-	fprintf(stderr, "Available strategies are:%s.\n", err.buf);
-	exit(1);
+	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);
+		}
+		fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name);
+		list_commands("git-merge-", "strategies", &not_strategies);
+		exit(1);
+	}
+
+	ret = xmalloc(sizeof(struct strategy));
+	memset(ret, 0, sizeof(struct strategy));
+	ret->name = xstrdup(name);
+	return ret;
 }
 
 static void append_strategy(struct strategy *s)
-- 
1.6.0.rc0.14.g95f8.dirty

Re: [PATCH 4/5] builtin-merge: allow using a custom strategy

[Junio C Hamano]

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?

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

[Miklos Vajna]

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

[PATCH 1/4] builtin-help: make some internal functions available to other builtins

[Miklos Vajna]

Make load_command_list() capable of filtering for a given prefix and
loading into a pair of "struct cmdnames" supplied by the caller.

Make the static add_cmdname(), exclude_cmds() and is_in_cmdlist()
functions non-static.

Make list_commands() accept a custom title, and work from a pair of
"struct cmdnames" supplied by the caller.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 Makefile |    1 +
 help.c   |   77 +++++++++++++++++++++++++++++++------------------------------
 help.h   |   23 ++++++++++++++++++
 3 files changed, 63 insertions(+), 38 deletions(-)
 create mode 100644 help.h

diff --git a/Makefile b/Makefile
index 52c67c1..83d79af 100644
--- a/Makefile
+++ b/Makefile
@@ -355,6 +355,7 @@ LIB_H += git-compat-util.h
 LIB_H += graph.h
 LIB_H += grep.h
 LIB_H += hash.h
+LIB_H += help.h
 LIB_H += list-objects.h
 LIB_H += ll-merge.h
 LIB_H += log-tree.h
diff --git a/help.c b/help.c
index 3cb1962..88c0d5b 100644
--- a/help.c
+++ b/help.c
@@ -9,6 +9,7 @@
 #include "common-cmds.h"
 #include "parse-options.h"
 #include "run-command.h"
+#include "help.h"
 
 static struct man_viewer_list {
 	struct man_viewer_list *next;
@@ -300,18 +301,11 @@ static inline void mput_char(char c, unsigned int num)
 		putchar(c);
 }
 
-static struct cmdnames {
-	int alloc;
-	int cnt;
-	struct cmdname {
-		size_t len;
-		char name[1];
-	} **names;
-} main_cmds, other_cmds;
+struct cmdnames main_cmds, other_cmds;
 
-static void add_cmdname(struct cmdnames *cmds, const char *name, int len)
+void add_cmdname(struct cmdnames *cmds, const char *name, int len)
 {
-	struct cmdname *ent = xmalloc(sizeof(*ent) + len);
+	struct cmdname *ent = xmalloc(sizeof(*ent) + len + 1);
 
 	ent->len = len;
 	memcpy(ent->name, name, len);
@@ -342,7 +336,7 @@ static void uniq(struct cmdnames *cmds)
 	cmds->cnt = j;
 }
 
-static void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)
+void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)
 {
 	int ci, cj, ei;
 	int cmp;
@@ -418,11 +412,11 @@ static int is_executable(const char *name)
 }
 
 static unsigned int list_commands_in_dir(struct cmdnames *cmds,
-					 const char *path)
+					 const char *path,
+					 const char *prefix)
 {
 	unsigned int longest = 0;
-	const char *prefix = "git-";
-	int prefix_len = strlen(prefix);
+	int prefix_len;
 	DIR *dir = opendir(path);
 	struct dirent *de;
 	struct strbuf buf = STRBUF_INIT;
@@ -430,6 +424,9 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 
 	if (!dir)
 		return 0;
+	if (!prefix)
+		prefix = "git-";
+	prefix_len = strlen(prefix);
 
 	strbuf_addf(&buf, "%s/", path);
 	len = buf.len;
@@ -460,7 +457,9 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
 	return longest;
 }
 
-static unsigned int load_command_list(void)
+unsigned int load_command_list(const char *prefix,
+		struct cmdnames *main_cmds,
+		struct cmdnames *other_cmds)
 {
 	unsigned int longest = 0;
 	unsigned int len;
@@ -469,7 +468,7 @@ static unsigned int load_command_list(void)
 	const char *exec_path = git_exec_path();
 
 	if (exec_path)
-		longest = list_commands_in_dir(&main_cmds, exec_path);
+		longest = list_commands_in_dir(main_cmds, exec_path, prefix);
 
 	if (!env_path) {
 		fprintf(stderr, "PATH not set\n");
@@ -481,7 +480,7 @@ static unsigned int load_command_list(void)
 		if ((colon = strchr(path, PATH_SEP)))
 			*colon = 0;
 
-		len = list_commands_in_dir(&other_cmds, path);
+		len = list_commands_in_dir(other_cmds, path, prefix);
 		if (len > longest)
 			longest = len;
 
@@ -491,36 +490,38 @@ static unsigned int load_command_list(void)
 	}
 	free(paths);
 
-	qsort(main_cmds.names, main_cmds.cnt,
-	      sizeof(*main_cmds.names), cmdname_compare);
-	uniq(&main_cmds);
+	qsort(main_cmds->names, main_cmds->cnt,
+	      sizeof(*main_cmds->names), cmdname_compare);
+	uniq(main_cmds);
 
-	qsort(other_cmds.names, other_cmds.cnt,
-	      sizeof(*other_cmds.names), cmdname_compare);
-	uniq(&other_cmds);
-	exclude_cmds(&other_cmds, &main_cmds);
+	qsort(other_cmds->names, other_cmds->cnt,
+	      sizeof(*other_cmds->names), cmdname_compare);
+	uniq(other_cmds);
+	exclude_cmds(other_cmds, main_cmds);
 
 	return longest;
 }
 
-static void list_commands(void)
+void list_commands(const char *title, unsigned int longest,
+		struct cmdnames *main_cmds, struct cmdnames *other_cmds)
 {
-	unsigned int longest = load_command_list();
 	const char *exec_path = git_exec_path();
 
-	if (main_cmds.cnt) {
-		printf("available git commands in '%s'\n", exec_path);
-		printf("----------------------------");
-		mput_char('-', strlen(exec_path));
+	if (main_cmds->cnt) {
+		printf("available %s in '%s'\n", title, exec_path);
+		printf("----------------");
+		mput_char('-', strlen(title) + strlen(exec_path));
 		putchar('\n');
-		pretty_print_string_list(&main_cmds, longest);
+		pretty_print_string_list(main_cmds, longest);
 		putchar('\n');
 	}
 
-	if (other_cmds.cnt) {
-		printf("git commands available from elsewhere on your $PATH\n");
-		printf("---------------------------------------------------\n");
-		pretty_print_string_list(&other_cmds, longest);
+	if (other_cmds->cnt) {
+		printf("%s available from elsewhere on your $PATH\n", title);
+		printf("---------------------------------------");
+		mput_char('-', strlen(title));
+		putchar('\n');
+		pretty_print_string_list(other_cmds, longest);
 		putchar('\n');
 	}
 }
@@ -542,7 +543,7 @@ void list_common_cmds_help(void)
 	}
 }
 
-static int is_in_cmdlist(struct cmdnames *c, const char *s)
+int is_in_cmdlist(struct cmdnames *c, const char *s)
 {
 	int i;
 	for (i = 0; i < c->cnt; i++)
@@ -553,7 +554,6 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s)
 
 static int is_git_command(const char *s)
 {
-	load_command_list();
 	return is_in_cmdlist(&main_cmds, s) ||
 		is_in_cmdlist(&other_cmds, s);
 }
@@ -698,8 +698,9 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 			builtin_help_usage, 0);
 
 	if (show_all) {
+		unsigned int longest = load_command_list("git-", &main_cmds, &other_cmds);
 		printf("usage: %s\n\n", git_usage_string);
-		list_commands();
+		list_commands("git commands", longest, &main_cmds, &other_cmds);
 		printf("%s\n", git_more_info_string);
 		return 0;
 	}
diff --git a/help.h b/help.h
new file mode 100644
index 0000000..d614e54
--- /dev/null
+++ b/help.h
@@ -0,0 +1,23 @@
+#ifndef HELP_H
+#define HELP_H
+
+struct cmdnames {
+	int alloc;
+	int cnt;
+	struct cmdname {
+		size_t len;
+		char name[FLEX_ARRAY];
+	} **names;
+};
+
+unsigned int load_command_list(const char *prefix,
+		struct cmdnames *main_cmds,
+		struct cmdnames *other_cmds);
+void add_cmdname(struct cmdnames *cmds, const char *name, int len);
+/* Here we require that excludes is a sorted list. */
+void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);
+int is_in_cmdlist(struct cmdnames *c, const char *s);
+void list_commands(const char *title, unsigned int longest,
+		struct cmdnames *main_cmds, struct cmdnames *other_cmds);
+
+#endif /* HELP_H */
-- 
1.6.0.rc0.14.g95f8.dirty

Re: [PATCH 1/4] builtin-help: make some internal functions available to other builtins

[Junio C Hamano]

Thanks; all four patches queued.

[PATCH 2/4] builtin-merge: allow using a custom strategy

[Miklos Vajna]

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.

These are removed from the output by scanning the list of main commands;
if the git-merge-foo command is listed in the all_strategy list, then
it's shown, otherwise excluded. This does not exclude commands somewhere
else in the PATH, where custom strategies are expected.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-merge.c |   42 +++++++++++++++++++++++++++++++++++-------
 1 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index e78fa18..99b307a 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -22,6 +22,7 @@
 #include "log-tree.h"
 #include "color.h"
 #include "rerere.h"
+#include "help.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -77,7 +78,9 @@ static int option_parse_message(const struct option *opt,
 static struct strategy *get_strategy(const char *name)
 {
 	int i;
-	struct strbuf err;
+	struct strategy *ret;
+	static struct cmdnames main_cmds, other_cmds;
+	static int longest = 0;
 
 	if (!name)
 		return NULL;
@@ -86,12 +89,37 @@ static struct strategy *get_strategy(const char *name)
 		if (!strcmp(name, all_strategy[i].name))
 			return &all_strategy[i];
 
-	strbuf_init(&err, 0);
-	for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
-		strbuf_addf(&err, " %s", all_strategy[i].name);
-	fprintf(stderr, "Could not find merge strategy '%s'.\n", name);
-	fprintf(stderr, "Available strategies are:%s.\n", err.buf);
-	exit(1);
+	if (!longest) {
+		struct cmdnames not_strategies;
+
+		memset(&main_cmds, 0, sizeof(struct cmdnames));
+		memset(&other_cmds, 0, sizeof(struct cmdnames));
+		memset(&not_strategies, 0, sizeof(struct cmdnames));
+		longest = load_command_list("git-merge-", &main_cmds,
+				&other_cmds);
+		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);
+			exclude_cmds(&main_cmds, &not_strategies);
+		}
+	}
+	if (!is_in_cmdlist(&main_cmds, name) && !is_in_cmdlist(&other_cmds, name)) {
+
+		fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name);
+		list_commands("strategies", longest, &main_cmds, &other_cmds);
+		exit(1);
+	}
+
+	ret = xmalloc(sizeof(struct strategy));
+	memset(ret, 0, sizeof(struct strategy));
+	ret->name = xstrdup(name);
+	return ret;
 }
 
 static void append_strategy(struct strategy *s)
-- 
1.6.0.rc0.14.g95f8.dirty

[PATCH 3/4] Add a new test for using a custom merge strategy

[Miklos Vajna]

Testing is done by creating a simple git-merge-theirs strategy which is
the opposite of ours. Using this in real merges is not recommended but
it's perfect for our testing needs.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 t/t7606-merge-custom.sh |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)
 create mode 100755 t/t7606-merge-custom.sh

diff --git a/t/t7606-merge-custom.sh b/t/t7606-merge-custom.sh
new file mode 100755
index 0000000..13e8ff5
--- /dev/null
+++ b/t/t7606-merge-custom.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+
+test_description='git-merge
+
+Testing a custom strategy.'
+
+. ./test-lib.sh
+
+cat > git-merge-theirs << EOF
+#!/bin/sh
+eval git read-tree --reset -u \\\$\$#
+EOF
+chmod +x git-merge-theirs
+PATH=.:$PATH
+export PATH
+
+test_expect_success 'setup' '
+	echo c0 > c0.c &&
+	git add c0.c &&
+	git commit -m c0 &&
+	git tag c0 &&
+	echo c1 > c1.c &&
+	git add c1.c &&
+	git commit -m c1 &&
+	git tag c1 &&
+	git reset --hard c0 &&
+	echo c2 > c2.c &&
+	git add c2.c &&
+	git commit -m c2 &&
+	git tag c2
+'
+
+test_expect_success 'merge c2 with a custom strategy' '
+	git reset --hard c1 &&
+	git merge -s theirs c2 &&
+	test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" &&
+	test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" &&
+	test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
+	test "$(git rev-parse c2^{tree})" = "$(git rev-parse HEAD^{tree})" &&
+	git diff --exit-code &&
+	test -f c0.c &&
+	test ! -f c1.c &&
+	test -f c2.c
+'
+
+test_done
-- 
1.6.0.rc0.14.g95f8.dirty

Re: [PATCH 3/4] Add a new test for using a custom merge strategy

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> +cat > git-merge-theirs << EOF
> +#!/bin/sh
> +eval git read-tree --reset -u \\\$\$#
> +EOF

This should be $SHELL_PATH, instead of hardcoded /bin/sh, to be easy on
people on Solaris and other systems.

Other than that, the patch is good and there is no need to resend it.
Thanks.

By the way, this eval shows why "theirs" cannot be a symmetric operation
of "ours".  You are taking the last remote HEAD even when you are merging
more than one remote into the current branch at once.  "ours" can be
sensibly defined for an octopus, but "theirs" has this "which theirs"
problem ;-)

Re: [PATCH 3/4] Add a new test for using a custom merge strategy

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 598 bytes --]

On Tue, Jul 29, 2008 at 04:43:53PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> By the way, this eval shows why "theirs" cannot be a symmetric operation
> of "ours".  You are taking the last remote HEAD even when you are merging
> more than one remote into the current branch at once.  "ours" can be
> sensibly defined for an octopus, but "theirs" has this "which theirs"
> problem ;-)

Oh, well, sure. But _if_ it turns out there is a demand for that kind of
git-merge-theirs, then I suppose we could still give up if we are given
two or more remotes, just like merge-resolve and others do.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 4/4] Add a second testcase for handling invalid strategies in git-merge

[Miklos Vajna]

This one tests '-s index' which is interesting because git-merge-index
is an existing git command but it is not a valid strategy.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 t/t7600-merge.sh |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 5eeb6c2..0329aee 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -230,6 +230,10 @@ test_expect_success 'test option parsing' '
 	test_must_fail git merge
 '
 
+test_expect_success 'reject non-strategy with a git-merge-foo name' '
+	test_must_fail git merge -s index c1
+'
+
 test_expect_success 'merge c0 with c1' '
 	git reset --hard c0 &&
 	git merge c1 &&
-- 
1.6.0.rc0.14.g95f8.dirty

Re: [PATCH 4/4] Add a second testcase for handling invalid strategies in git-merge

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> This one tests '-s index' which is interesting because git-merge-index
> is an existing git command but it is not a valid strategy.
>
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> ---
>  t/t7600-merge.sh |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 5eeb6c2..0329aee 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -230,6 +230,10 @@ test_expect_success 'test option parsing' '
>  	test_must_fail git merge
>  '
>  
> +test_expect_success 'reject non-strategy with a git-merge-foo name' '
> +	test_must_fail git merge -s index c1
> +'
> +

True, but with the old code that might blindly have executed
git-merge-index the test would also have failed, and you would want to
tell two cases apart, wouldn't you?

Re: [PATCH 4/4] Add a second testcase for handling invalid strategies in git-merge

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 384 bytes --]

On Tue, Jul 29, 2008 at 04:42:53PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> True, but with the old code that might blindly have executed
> git-merge-index the test would also have failed, and you would want to
> tell two cases apart, wouldn't you?

Hmm, I wonder what is the right approach to test it. Should I exit with
a different error code with the strategy is invalid?

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 5/5] Add a new test for using a custom merge strategy

[Miklos Vajna]

Testing is done by creating a simple git-merge-theirs strategy which is
the opposite of ours. Using this in real merges is not recommended but
it's perfect for our testing needs.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 t/t7606-merge-custom.sh |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)
 create mode 100755 t/t7606-merge-custom.sh

diff --git a/t/t7606-merge-custom.sh b/t/t7606-merge-custom.sh
new file mode 100755
index 0000000..13e8ff5
--- /dev/null
+++ b/t/t7606-merge-custom.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+
+test_description='git-merge
+
+Testing a custom strategy.'
+
+. ./test-lib.sh
+
+cat > git-merge-theirs << EOF
+#!/bin/sh
+eval git read-tree --reset -u \\\$\$#
+EOF
+chmod +x git-merge-theirs
+PATH=.:$PATH
+export PATH
+
+test_expect_success 'setup' '
+	echo c0 > c0.c &&
+	git add c0.c &&
+	git commit -m c0 &&
+	git tag c0 &&
+	echo c1 > c1.c &&
+	git add c1.c &&
+	git commit -m c1 &&
+	git tag c1 &&
+	git reset --hard c0 &&
+	echo c2 > c2.c &&
+	git add c2.c &&
+	git commit -m c2 &&
+	git tag c2
+'
+
+test_expect_success 'merge c2 with a custom strategy' '
+	git reset --hard c1 &&
+	git merge -s theirs c2 &&
+	test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" &&
+	test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" &&
+	test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
+	test "$(git rev-parse c2^{tree})" = "$(git rev-parse HEAD^{tree})" &&
+	git diff --exit-code &&
+	test -f c0.c &&
+	test ! -f c1.c &&
+	test -f c2.c
+'
+
+test_done
-- 
1.6.0.rc0.14.g95f8.dirty

Re: [PATCH] t7601: extend the 'merge picks up the best result' test

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 454 bytes --]

On Sat, Jul 26, 2008 at 01:54:45PM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:
> The test only checked if the best result picking code works if there are
> multiple strategies set in the config. Add a similar one that tests if
> the same true if the -s option of git merge was used multiple times.

Ignore this one. I run git send-email *.patch when sending out the other
series and forgot to do an rm *.patch before format-patch.

Sorry,
Miklos

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]