threads / patch / 14690

patch, 7 partsMake is_git_command() usable outside builtin-help

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

## tl;dr

46 messages between Jul 26, 2008 and Jul 29, 2008. Diffs are folded; open one to read it.

replies: 45people: 4as markdown or json

Miklos Vajna· Jul 26, 2008, 11:54 UTC · lore

[PATCH 0/7] Allow custom merge strategies

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

Miklos Vajna· Jul 26, 2008, 11:54 UTC · re: Miklos Vajna · lore

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

Show changes to 3 files +21 −11

Makefile, help.c, 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

Miklos Vajna· Jul 26, 2008, 11:54 UTC · re: Miklos Vajna · lore

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

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

Show changes to t/t7601-merge-pull-config.sh +15 −0

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

Miklos Vajna· Jul 26, 2008, 11:54 UTC · re: Miklos Vajna · lore

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

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

Show changes to help.c +3 −0

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

Miklos Vajna· Jul 26, 2008, 11:54 UTC · re: Miklos Vajna · lore

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

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

Show changes to 2 files +6 −5

help.c, help.h

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

Miklos Vajna· Jul 26, 2008, 11:54 UTC · re: Miklos Vajna · lore

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

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

Show changes to builtin-merge.c +12 −7

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

Miklos Vajna· Jul 26, 2008, 11:54 UTC · re: Miklos Vajna · lore

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

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

Show changes to t/t7606-merge-custom.sh +45 −0

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

Miklos Vajna· Jul 26, 2008, 11:54 UTC · re: Miklos Vajna · lore

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

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

Show changes to 2 files +23 −15

help.c, help.h

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

Miklos Vajna· Jul 26, 2008, 11:54 UTC · re: Miklos Vajna · lore

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

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

Show changes to builtin-merge.c +12 −1

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

Johannes Schindelin· Jul 26, 2008, 15:08 UTC · re: Miklos Vajna · lore

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

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?

Show 6 quoted lines

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

Miklos Vajna· Jul 26, 2008, 15:25 UTC · re: Johannes Schindelin · lore

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

On Sat, Jul 26, 2008 at 05:08:11PM +0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 6 quoted lines

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

Show 10 quoted lines

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

Show changes to builtin-merge.c +3 −2

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);

Miklos Vajna· Jul 26, 2008, 15:27 UTC · re: Miklos Vajna · lore

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

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.

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

Show changes to builtin-merge.c +13 −1

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

Johannes Schindelin· Jul 26, 2008, 15:38 UTC · re: Miklos Vajna · lore

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

Hi,

On Sat, 26 Jul 2008, Miklos Vajna wrote:

Show 11 quoted lines

> 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

Miklos Vajna· Jul 26, 2008, 16:00 UTC · re: Johannes Schindelin · lore

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

On Sat, Jul 26, 2008 at 05:38:55PM +0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 5 quoted lines

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

Johannes Schindelin· Jul 26, 2008, 17:01 UTC · re: Miklos Vajna · lore

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

Hi,

On Sat, 26 Jul 2008, Miklos Vajna wrote:

Show 11 quoted lines

> 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

Miklos Vajna· Jul 26, 2008, 17:12 UTC · re: Johannes Schindelin · lore

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

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.

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:

Show 11 quoted lines

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

Show changes to builtin-merge.c +14 −1

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

Jonathan Nieder· Jul 26, 2008, 18:28 UTC · re: Miklos Vajna · lore

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

On Sat, 26 Jul 2008, Miklos Vajna wrote:

Show 13 quoted lines

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

Miklos Vajna· Jul 26, 2008, 21:40 UTC · re: Jonathan Nieder · lore

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

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

On Sat, Jul 26, 2008 at 01:28:39PM -0500, Jonathan Nieder <jrnieder@uchicago.edu> wrote:

Show 17 quoted lines

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

Show changes to 2 files +11 −8

help.c, help.h

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

Johannes Schindelin· Jul 26, 2008, 14:58 UTC · re: Miklos Vajna · lore

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

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

Junio C Hamano· Jul 27, 2008, 20:02 UTC · re: Miklos Vajna · lore

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

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?

Johannes Schindelin· Jul 27, 2008, 20:21 UTC · re: Junio C Hamano · lore

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

Hi,

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

Show 9 quoted lines

> 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

Johannes Schindelin· Jul 27, 2008, 20:34 UTC · re: Johannes Schindelin · lore

[PATCH] Avoid chdir() in list_commands_in_dir()

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

Show changes to help.c +10 −2

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

Miklos Vajna· Jul 26, 2008, 12:33 UTC · re: Miklos Vajna · lore

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

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

Jonathan Nieder· Jul 26, 2008, 18:12 UTC · re: Miklos Vajna · lore

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

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.

Miklos Vajna· Jul 28, 2008, 01:18 UTC · re: Jonathan Nieder · lore

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

On Sat, Jul 26, 2008 at 01:12:36PM -0500, Jonathan Nieder <jrnieder@uchicago.edu> wrote:

Show 10 quoted lines

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

:-)

Miklos Vajna· Jul 28, 2008, 01:21 UTC · re: Miklos Vajna · lore

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

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

Miklos Vajna· Jul 28, 2008, 01:21 UTC · re: Miklos Vajna · lore

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

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

Show changes to 3 files +21 −11

Makefile, help.c, 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

Miklos Vajna· Jul 28, 2008, 01:21 UTC · re: Miklos Vajna · lore

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

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

Show changes to 2 files +11 −8

help.c, help.h

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

Miklos Vajna· Jul 28, 2008, 01:21 UTC · re: Miklos Vajna · lore

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

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

Show changes to 2 files +23 −15

help.c, help.h

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

Miklos Vajna· Jul 28, 2008, 01:21 UTC · re: Miklos Vajna · lore

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

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

Show changes to builtin-merge.c +12 −7

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

Miklos Vajna· Jul 28, 2008, 01:21 UTC · re: Miklos Vajna · lore

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

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.

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

Show changes to builtin-merge.c +14 −1

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

Miklos Vajna· Jul 28, 2008, 01:21 UTC · re: Miklos Vajna · lore

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

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

Show changes to t/t7606-merge-custom.sh +45 −0

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

Johannes Schindelin· Jul 28, 2008, 13:12 UTC · re: Miklos Vajna · lore

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

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

Miklos Vajna· Jul 28, 2008, 23:39 UTC · re: Johannes Schindelin · lore

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

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:

Show 5 quoted lines

> 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

Show changes to t/t7606-merge-custom.sh +46 −0

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

Johannes Schindelin· Jul 28, 2008, 23:54 UTC · re: Miklos Vajna · lore

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

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

Miklos Vajna· Jul 28, 2008, 23:58 UTC · re: Johannes Schindelin · lore

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

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

Miklos Vajna· Jul 29, 2008, 00:01 UTC · re: Miklos Vajna · lore

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

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.

Johannes Schindelin· Jul 28, 2008, 13:06 UTC · re: Miklos Vajna · lore

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

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

Miklos Vajna· Jul 28, 2008, 23:48 UTC · re: Johannes Schindelin · lore

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

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

Show changes to builtin-merge.c +25 −7

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

Johannes Schindelin· Jul 28, 2008, 23:59 UTC · re: Miklos Vajna · lore

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

Hi,

On Tue, 29 Jul 2008, Miklos Vajna wrote:

Show 11 quoted lines

> 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

Junio C Hamano· Jul 28, 2008, 01:36 UTC · re: Miklos Vajna · lore

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

Miklos Vajna <vmiklos@frugalware.org> writes:

Show 16 quoted lines

> 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?

Johannes Schindelin· Jul 28, 2008, 02:43 UTC · re: Junio C Hamano · lore

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

Hi,

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

Show 21 quoted lines

> 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

Junio C Hamano· Jul 28, 2008, 03:05 UTC · re: Johannes Schindelin · lore

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

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

Show 15 quoted lines

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

Junio C Hamano· Jul 28, 2008, 04:29 UTC · re: Junio C Hamano · lore

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

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

Show 19 quoted lines

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

Johannes Schindelin· Jul 28, 2008, 09:58 UTC · re: Junio C Hamano · lore

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

Hi,

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

Show 23 quoted lines

> 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

Miklos Vajna· Jul 28, 2008, 23:24 UTC · re: Junio C Hamano · lore

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

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:

Show 9 quoted lines

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

Show changes to 2 files +24 −16

help.c, help.h

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