threads / patch / 14747

patch, 4 partsAdd a new test for using a custom merge strategy

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

## tl;dr

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

replies: 9people: 2as markdown or json

Miklos Vajna· Jul 29, 2008, 23:16 UTC · lore

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

On Tue, Jul 29, 2008 at 12:47:05PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
Show 24 quoted lines
> This feels overly wasteful.  Granted, this is not a performance critical
> codepath, but you list all commands that begin with "git-merge-" in
> is_git_command(), only to discard it and then iterate over 140+ main_cmds
> list only to cull the ones whose name do not appear in the strategies
> list.
>
> Perhaps this shows that changing the function is_git_command() is a wrong
> approach (for one thing, with the custom prefix, it is not about "Is it a
> git command" anymore).  Wouldn't it be easier to read if you did this part
> like this instead?
>
>  * make load_command_list capable of loading into a "struct cmdnames" (or
>    pair if you want) supplied by the caller;
>
>  * use it to grab all commands whose name begin with "git-merge-" here;
>
>  * Check if name appears in that list; if it doesn't, you already have the
>    list of commands that could be merge strategy for error reporting.
>
> If you also update list_commands() not to run load_command_list() itself
> but take caller-supplied list of commands, then the API would become much
> cleaner.  The caller would not be limited to "filter with prefix" anymore.
>
> Hmm?
I like the idea. ;-)
Here is what I did:
- I changed only load_command_list() and list_commands().
  load_command_list() still supports filtering, but the result is loaded
  to a pair of "struct cmdnames", supplied by the caller.
- list_commands() works from this pair, additionally supporting the
  custom title.
- export add_cmdname(), exclude_cmds() and is_in_cmdlist() without any
  modification in help.h
The rest is done in builtin-merge.c, as you suggested.

The nice thing is that before this change builtin-merge accepted for example '-s index' (because git-merge-index was a valid command), but now this is no longer true.

Miklos Vajna (4):
  builtin-help: make some internal functions available to other
    builtins
  builtin-merge: allow using a custom strategy
  Add a new test for using a custom merge strategy
  Add a second testcase for handling invalid strategies in git-merge
 Makefile                |    1 +
 builtin-merge.c         |   42 +++++++++++++++++++++----
 help.c                  |   77 ++++++++++++++++++++++++-----------------------
 help.h                  |   23 ++++++++++++++
 t/t7600-merge.sh        |    5 +++
 t/t7606-merge-custom.sh |   46 ++++++++++++++++++++++++++++
 6 files changed, 149 insertions(+), 45 deletions(-)
 create mode 100644 help.h
 create mode 100755 t/t7606-merge-custom.sh
Miklos Vajna· Jul 29, 2008, 23:16 UTC · re: Miklos Vajna · lore

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

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
Show changes to 3 files +63 −38

Makefile, help.c, 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
Miklos Vajna· Jul 29, 2008, 23:16 UTC · re: Miklos Vajna · lore

[PATCH 2/4] 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>
---
 builtin-merge.c |   42 +++++++++++++++++++++++++++++++++++-------
 1 files changed, 35 insertions(+), 7 deletions(-)
Show changes to builtin-merge.c +35 −7
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
Miklos Vajna· Jul 29, 2008, 23:17 UTC · re: Miklos Vajna · lore

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
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
Miklos Vajna· Jul 29, 2008, 23:17 UTC · re: Miklos Vajna · lore

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

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(-)
Show changes to t/t7600-merge.sh +4 −0
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
Junio C Hamano· Jul 29, 2008, 23:42 UTC · re: Miklos Vajna · lore

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

Miklos Vajna <vmiklos@frugalware.org> writes:
Show 20 quoted lines
> 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?

Miklos Vajna· Jul 30, 2008, 17:27 UTC · re: Junio C Hamano · lore

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

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?

Junio C Hamano· Jul 29, 2008, 23:43 UTC · re: Miklos Vajna · lore

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

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

Miklos Vajna· Jul 30, 2008, 17:25 UTC · re: Junio C Hamano · lore

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

On Tue, Jul 29, 2008 at 04:43:53PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> 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.

← back to recent threads