* [PATCH] builtin-merge: give a proper error message for invalid strategies in config @ 2008-07-21 16:10 Miklos Vajna 2008-07-22 5:01 ` Junio C Hamano 0 siblings, 1 reply; 71+ messages in thread From: Miklos Vajna @ 2008-07-21 16:10 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Till now 'git merge -s foobar' bailed out with an error message, but foobar in pull.twohead or pull.octopus was just silently ignored. It's better to inform the user then just silently doing nothing. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- builtin-merge.c | 35 +++++++++++++++-------------------- t/t7601-merge-pull-config.sh | 6 ++++++ 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/builtin-merge.c b/builtin-merge.c index e97c79e..5037acf 100644 --- a/builtin-merge.c +++ b/builtin-merge.c @@ -93,28 +93,13 @@ static void append_strategy(struct strategy *s) use_strategies[use_strategies_nr++] = s; } +static void add_strategies(const char *string, unsigned attr); + static int option_parse_strategy(const struct option *opt, const char *name, int unset) { - int i; - struct strategy *s; - - if (unset) - return 0; - - s = get_strategy(name); - - if (s) - append_strategy(s); - else { - struct strbuf err; - strbuf_init(&err, 0); - for (i = 0; i < ARRAY_SIZE(all_strategy); i++) - strbuf_addf(&err, " %s", all_strategy[i].name); - fprintf(stderr, "Could not find merge strategy '%s'.\n", name); - fprintf(stderr, "Available strategies are:%s.\n", err.buf); - exit(1); - } + if (!unset) + add_strategies(name, 0); return 0; } @@ -639,7 +624,7 @@ static void split_merge_strategies(const char *string, struct strategy **list, static void add_strategies(const char *string, unsigned attr) { struct strategy *list = NULL; - int list_alloc = 0, list_nr = 0, i; + int list_alloc = 0, list_nr = 0, i, j; memset(&list, 0, sizeof(list)); split_merge_strategies(string, &list, &list_nr, &list_alloc); @@ -650,6 +635,16 @@ static void add_strategies(const char *string, unsigned attr) s = get_strategy(list[i].name); if (s) append_strategy(s); + else { + struct strbuf err; + + strbuf_init(&err, 0); + for (j = 0; j < ARRAY_SIZE(all_strategy); j++) + strbuf_addf(&err, " %s", all_strategy[j].name); + fprintf(stderr, "Could not find merge strategy '%s'.\n", list[i].name); + fprintf(stderr, "Available strategies are:%s.\n", err.buf); + exit(1); + } } return; } diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh index 95b4d71..ca63451 100755 --- a/t/t7601-merge-pull-config.sh +++ b/t/t7601-merge-pull-config.sh @@ -126,4 +126,10 @@ test_expect_success 'merge picks up the best result' ' test $auto_count != $resolve_count ' +test_expect_success 'merge errors out on invalid strategy' ' + git config pull.twohead "foobar" && + git reset --hard c5 && + test_must_fail git merge c6 +' + test_done -- 1.5.6.4.433.g09651.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config 2008-07-21 16:10 [PATCH] builtin-merge: give a proper error message for invalid strategies in config Miklos Vajna @ 2008-07-22 5:01 ` Junio C Hamano 2008-07-22 5:22 ` Junio C Hamano 2008-07-22 7:39 ` Miklos Vajna 0 siblings, 2 replies; 71+ messages in thread From: Junio C Hamano @ 2008-07-22 5:01 UTC (permalink / raw) To: Miklos Vajna; +Cc: git Miklos Vajna <vmiklos@frugalware•org> writes: > Till now 'git merge -s foobar' bailed out with an error message, but > foobar in pull.twohead or pull.octopus was just silently ignored. It's > better to inform the user then just silently doing nothing. > > Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> Doesn't this make "git merge -s 'recursive resolve'" to misbehave? ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config 2008-07-22 5:01 ` Junio C Hamano @ 2008-07-22 5:22 ` Junio C Hamano 2008-07-22 7:39 ` Miklos Vajna 1 sibling, 0 replies; 71+ messages in thread From: Junio C Hamano @ 2008-07-22 5:22 UTC (permalink / raw) To: Miklos Vajna; +Cc: git Junio C Hamano <gitster@pobox•com> writes: > Miklos Vajna <vmiklos@frugalware•org> writes: > >> Till now 'git merge -s foobar' bailed out with an error message, but >> foobar in pull.twohead or pull.octopus was just silently ignored. It's >> better to inform the user then just silently doing nothing. >> >> Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> > > Doesn't this make "git merge -s 'recursive resolve'" to misbehave? Perhaps this would be a better replacement. It makes get_strategy() to validate and barf on unknown one. It is slightly smaller and more contained. If/when we add user-defined ones, that logic will also be contained in the function. builtin-merge.c | 37 ++++++++++++------------------------- t/t7601-merge-pull-config.sh | 12 ++++++++++++ 2 files changed, 24 insertions(+), 25 deletions(-) diff --git a/builtin-merge.c b/builtin-merge.c index e97c79e..0fd7985 100644 --- a/builtin-merge.c +++ b/builtin-merge.c @@ -77,6 +77,7 @@ static int option_parse_message(const struct option *opt, static struct strategy *get_strategy(const char *name) { int i; + struct strbuf err; if (!name) return NULL; @@ -84,7 +85,13 @@ static struct strategy *get_strategy(const char *name) for (i = 0; i < ARRAY_SIZE(all_strategy); i++) if (!strcmp(name, all_strategy[i].name)) return &all_strategy[i]; - return NULL; + + strbuf_init(&err, 0); + for (i = 0; i < ARRAY_SIZE(all_strategy); i++) + strbuf_addf(&err, " %s", all_strategy[i].name); + fprintf(stderr, "Could not find merge strategy '%s'.\n", name); + fprintf(stderr, "Available strategies are:%s.\n", err.buf); + exit(1); } static void append_strategy(struct strategy *s) @@ -96,25 +103,10 @@ static void append_strategy(struct strategy *s) static int option_parse_strategy(const struct option *opt, const char *name, int unset) { - int i; - struct strategy *s; - if (unset) return 0; - s = get_strategy(name); - - if (s) - append_strategy(s); - else { - struct strbuf err; - strbuf_init(&err, 0); - for (i = 0; i < ARRAY_SIZE(all_strategy); i++) - strbuf_addf(&err, " %s", all_strategy[i].name); - fprintf(stderr, "Could not find merge strategy '%s'.\n", name); - fprintf(stderr, "Available strategies are:%s.\n", err.buf); - exit(1); - } + append_strategy(get_strategy(name)); return 0; } @@ -643,14 +635,9 @@ static void add_strategies(const char *string, unsigned attr) memset(&list, 0, sizeof(list)); split_merge_strategies(string, &list, &list_nr, &list_alloc); - if (list != NULL) { - for (i = 0; i < list_nr; i++) { - struct strategy *s; - - s = get_strategy(list[i].name); - if (s) - append_strategy(s); - } + if (list) { + for (i = 0; i < list_nr; i++) + append_strategy(get_strategy(list[i].name)); return; } for (i = 0; i < ARRAY_SIZE(all_strategy); i++) diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh index 95b4d71..6b9f638 100755 --- a/t/t7601-merge-pull-config.sh +++ b/t/t7601-merge-pull-config.sh @@ -126,4 +126,16 @@ test_expect_success 'merge picks up the best result' ' test $auto_count != $resolve_count ' +test_expect_success 'merge errors out on invalid strategy' ' + git config pull.twohead "foobar" && + git reset --hard c5 && + test_must_fail git merge c6 +' + +test_expect_success 'merge errors out on invalid strategy' ' + git config --unset-all pull.twohead && + git reset --hard c5 && + test_must_fail git merge -s "resolve recursive" c6 +' + test_done ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config 2008-07-22 5:01 ` Junio C Hamano 2008-07-22 5:22 ` Junio C Hamano @ 2008-07-22 7:39 ` Miklos Vajna 2008-07-22 8:24 ` Junio C Hamano 1 sibling, 1 reply; 71+ messages in thread From: Miklos Vajna @ 2008-07-22 7:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: git [-- Attachment #1: Type: text/plain, Size: 1078 bytes --] On Mon, Jul 21, 2008 at 10:01:16PM -0700, Junio C Hamano <gitster@pobox•com> wrote: > Doesn't this make "git merge -s 'recursive resolve'" to misbehave? Depends on what do we expect it to do. ;-) My patch unified the handling of pull.twohead / -s option, so it (actually unintentionally) made -s accepting multiple strategies as a space separated list. Given that we already accept multiple strategies as a space separated list in pull.twohead (and we do _not_ use multiple pull.twohead entries) I think my patch is more logical. Though, there was already a thread about how should we specify multiple strategies on the commandline; and you suggested in http://article.gmane.org/gmane.comp.version-control.git/89208 to use -s strategy1 -s strategy2. In that case, I think your patch is better, and once it hits git.git, I would like to send a patch that changes the config parsing as well, so that pull.twohead "foo bar" would be invalid, and the user would have to have two pull.twohead entries: one for foo and one for bar. Does this sound reasonable? Thanks. [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config 2008-07-22 7:39 ` Miklos Vajna @ 2008-07-22 8:24 ` Junio C Hamano 2008-07-22 17:05 ` [PATCH] t7601: extend the 'merge picks up the best result' test Miklos Vajna 2008-07-26 11:54 ` Miklos Vajna 0 siblings, 2 replies; 71+ messages in thread From: Junio C Hamano @ 2008-07-22 8:24 UTC (permalink / raw) To: Miklos Vajna; +Cc: git Miklos Vajna <vmiklos@frugalware•org> writes: > ... I > would like to send a patch that changes the config parsing as well, so > that pull.twohead "foo bar" would be invalid, and the user would have to > have two pull.twohead entries: one for foo and one for bar. Don't. pull.* has always been defined as "list of strategies", and -s has always been defined to take "a" strategy. IOW, you don't need to break anything further. ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH] t7601: extend the 'merge picks up the best result' test 2008-07-22 8:24 ` Junio C Hamano @ 2008-07-22 17:05 ` Miklos Vajna 2008-07-26 11:54 ` Miklos Vajna 1 sibling, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-22 17:05 UTC (permalink / raw) To: Junio C Hamano; +Cc: git The test only checked if the best result picking code works if there are multiple strategies set in the config. Add a similar one that tests if the same true if the -s option of git merge was used multiple times. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- On Tue, Jul 22, 2008 at 01:24:14AM -0700, Junio C Hamano <gitster@pobox•com> wrote: > Don't. pull.* has always been defined as "list of strategies", and -s > has > always been defined to take "a" strategy. OK. Here is a testcase for the later. As far as I see the behaviour of multiple -s was not checked till now. t/t7601-merge-pull-config.sh | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-) diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh index 6b9f638..55aa6b5 100755 --- a/t/t7601-merge-pull-config.sh +++ b/t/t7601-merge-pull-config.sh @@ -112,6 +112,21 @@ test_expect_success 'setup conflicted merge' ' # recusive is choosen. test_expect_success 'merge picks up the best result' ' + git config --unset-all pull.twohead && + git reset --hard c5 && + git merge -s resolve c6 + resolve_count=$(conflict_count) && + git reset --hard c5 && + git merge -s recursive c6 + recursive_count=$(conflict_count) && + git reset --hard c5 && + git merge -s recursive -s resolve c6 + auto_count=$(conflict_count) && + test $auto_count = $recursive_count && + test $auto_count != $resolve_count +' + +test_expect_success 'merge picks up the best result (from config)' ' git config pull.twohead "recursive resolve" && git reset --hard c5 && git merge -s resolve c6 -- 1.5.6.4.433.g09651.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH] t7601: extend the 'merge picks up the best result' test 2008-07-22 8:24 ` Junio C Hamano 2008-07-22 17:05 ` [PATCH] t7601: extend the 'merge picks up the best result' test Miklos Vajna @ 2008-07-26 11:54 ` Miklos Vajna 2008-07-26 11:54 ` [PATCH 0/7] Allow custom merge strategies Miklos Vajna 2008-07-26 12:33 ` [PATCH] t7601: extend the 'merge picks up the best result' test Miklos Vajna 1 sibling, 2 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-26 11:54 UTC (permalink / raw) To: git The test only checked if the best result picking code works if there are multiple strategies set in the config. Add a similar one that tests if the same true if the -s option of git merge was used multiple times. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- On Tue, Jul 22, 2008 at 01:24:14AM -0700, Junio C Hamano <gitster@pobox•com> wrote: > Don't. pull.* has always been defined as "list of strategies", and -s > has > always been defined to take "a" strategy. OK. Here is a testcase for the later. As far as I see the behaviour of multiple -s was not checked till now. t/t7601-merge-pull-config.sh | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-) diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh index 6b9f638..55aa6b5 100755 --- a/t/t7601-merge-pull-config.sh +++ b/t/t7601-merge-pull-config.sh @@ -112,6 +112,21 @@ test_expect_success 'setup conflicted merge' ' # recusive is choosen. test_expect_success 'merge picks up the best result' ' + git config --unset-all pull.twohead && + git reset --hard c5 && + git merge -s resolve c6 + resolve_count=$(conflict_count) && + git reset --hard c5 && + git merge -s recursive c6 + recursive_count=$(conflict_count) && + git reset --hard c5 && + git merge -s recursive -s resolve c6 + auto_count=$(conflict_count) && + test $auto_count = $recursive_count && + test $auto_count != $resolve_count +' + +test_expect_success 'merge picks up the best result (from config)' ' git config pull.twohead "recursive resolve" && git reset --hard c5 && git merge -s resolve c6 -- 1.5.6.4.433.g09651.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 0/7] Allow custom merge strategies @ 2008-07-26 11:54 ` Miklos Vajna 2008-07-26 11:54 ` [PATCH 1/7] Make is_git_command() usable outside builtin-help Miklos Vajna ` (7 more replies) 0 siblings, 8 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-26 11:54 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 1/7] Make is_git_command() usable outside builtin-help 2008-07-26 11:54 ` [PATCH 0/7] Allow custom merge strategies Miklos Vajna @ 2008-07-26 11:54 ` Miklos Vajna 2008-07-26 18:12 ` Jonathan Nieder 2008-07-26 11:54 ` [PATCH 2/7] builtin-help: change the current directory back in list_commands_in_dir() Miklos Vajna ` (6 subsequent siblings) 7 siblings, 1 reply; 71+ messages in thread From: Miklos Vajna @ 2008-07-26 11:54 UTC (permalink / raw) To: git Other builtins may want to check if a given command is a valid git command or not as well. Additionally add a new parameter that specifies a custom prefix, so that the "git-" prefix is no longer hardwired. Useful for example to limit the search for "git-merge-*". Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- Makefile | 1 + help.c | 25 ++++++++++++++----------- help.h | 6 ++++++ 3 files changed, 21 insertions(+), 11 deletions(-) create mode 100644 help.h diff --git a/Makefile b/Makefile index b01cf1c..85e79f6 100644 --- a/Makefile +++ b/Makefile @@ -346,6 +346,7 @@ LIB_H += git-compat-util.h LIB_H += graph.h LIB_H += grep.h LIB_H += hash.h +LIB_H += help.h LIB_H += list-objects.h LIB_H += ll-merge.h LIB_H += log-tree.h diff --git a/help.c b/help.c index bfc84ae..fb93df0 100644 --- a/help.c +++ b/help.c @@ -418,17 +418,20 @@ static int is_executable(const char *name) } static unsigned int list_commands_in_dir(struct cmdnames *cmds, - const char *path) + const char *path, + const char *prefix) { unsigned int longest = 0; - const char *prefix = "git-"; - int prefix_len = strlen(prefix); + int prefix_len; DIR *dir = opendir(path); struct dirent *de; if (!dir || chdir(path)) return 0; + if (!prefix) + prefix = "git-"; + prefix_len = strlen(prefix); while ((de = readdir(dir)) != NULL) { int entlen; @@ -452,7 +455,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds, return longest; } -static unsigned int load_command_list(void) +static unsigned int load_command_list(const char *prefix) { unsigned int longest = 0; unsigned int len; @@ -461,7 +464,7 @@ static unsigned int load_command_list(void) const char *exec_path = git_exec_path(); if (exec_path) - longest = list_commands_in_dir(&main_cmds, exec_path); + longest = list_commands_in_dir(&main_cmds, exec_path, prefix); if (!env_path) { fprintf(stderr, "PATH not set\n"); @@ -473,7 +476,7 @@ static unsigned int load_command_list(void) if ((colon = strchr(path, PATH_SEP))) *colon = 0; - len = list_commands_in_dir(&other_cmds, path); + len = list_commands_in_dir(&other_cmds, path, prefix); if (len > longest) longest = len; @@ -497,7 +500,7 @@ static unsigned int load_command_list(void) static void list_commands(void) { - unsigned int longest = load_command_list(); + unsigned int longest = load_command_list(NULL); const char *exec_path = git_exec_path(); if (main_cmds.cnt) { @@ -543,9 +546,9 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s) return 0; } -static int is_git_command(const char *s) +int is_git_command(const char *s, const char *prefix) { - load_command_list(); + load_command_list(prefix); return is_in_cmdlist(&main_cmds, s) || is_in_cmdlist(&other_cmds, s); } @@ -566,7 +569,7 @@ static const char *cmd_to_page(const char *git_cmd) return "git"; else if (!prefixcmp(git_cmd, "git")) return git_cmd; - else if (is_git_command(git_cmd)) + else if (is_git_command(git_cmd, NULL)) return prepend("git-", git_cmd); else return prepend("git", git_cmd); @@ -704,7 +707,7 @@ int cmd_help(int argc, const char **argv, const char *prefix) } alias = alias_lookup(argv[0]); - if (alias && !is_git_command(argv[0])) { + if (alias && !is_git_command(argv[0], NULL)) { printf("`git %s' is aliased to `%s'\n", argv[0], alias); return 0; } diff --git a/help.h b/help.h new file mode 100644 index 0000000..73da8d6 --- /dev/null +++ b/help.h @@ -0,0 +1,6 @@ +#ifndef HELP_H +#define HELP_H + +int is_git_command(const char *s, const char *prefix); + +#endif /* HELP_H */ -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 1/7] Make is_git_command() usable outside builtin-help 2008-07-26 11:54 ` [PATCH 1/7] Make is_git_command() usable outside builtin-help Miklos Vajna @ 2008-07-26 18:12 ` Jonathan Nieder 2008-07-28 1:18 ` Miklos Vajna 0 siblings, 1 reply; 71+ messages in thread From: Jonathan Nieder @ 2008-07-26 18:12 UTC (permalink / raw) To: Miklos Vajna; +Cc: git 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. ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 1/7] Make is_git_command() usable outside builtin-help 2008-07-26 18:12 ` Jonathan Nieder @ 2008-07-28 1:18 ` Miklos Vajna 0 siblings, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-28 1:18 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git [-- Attachment #1: Type: text/plain, Size: 707 bytes --] On Sat, Jul 26, 2008 at 01:12:36PM -0500, Jonathan Nieder <jrnieder@uchicago•edu> wrote: > On Sat, 26 Jul 2008, Miklos Vajna wrote: > > > + if (!prefix) > > + prefix = "git-"; > > + prefix_len = strlen(prefix); > > The whitespace gave me a start: the diff markup moved the prefix_len > line to the next tab stop, so at first glance it seems there are missing > braces here. But it is an illusion. (I mention this so others might > avoid wasting time worrying about it.) In fact it was a whitespace problem: somehow I used spaces there instead of a tab. I fixed it locally. (Will send an updated series soon.) > I like the patch so far. Thanks for the pleasant reading. :-) [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 2/7] builtin-help: change the current directory back in list_commands_in_dir() 2008-07-26 11:54 ` [PATCH 0/7] Allow custom merge strategies Miklos Vajna 2008-07-26 11:54 ` [PATCH 1/7] Make is_git_command() usable outside builtin-help Miklos Vajna @ 2008-07-26 11:54 ` Miklos Vajna 2008-07-26 14:58 ` Johannes Schindelin 2008-07-27 20:02 ` Junio C Hamano 2008-07-26 11:54 ` [PATCH 3/7] builtin-help: make list_commands() a bit more generic Miklos Vajna ` (5 subsequent siblings) 7 siblings, 2 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-26 11:54 UTC (permalink / raw) To: git That function used to do a chdir() without switching back to the original directory. That was not a problem till this function was used only inside builtin-help, but once other builtins use it as well, this is a problem, for example when the object database path is relative. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- help.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/help.c b/help.c index fb93df0..d71937e 100644 --- a/help.c +++ b/help.c @@ -425,7 +425,9 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds, int prefix_len; DIR *dir = opendir(path); struct dirent *de; + static char old_path[PATH_MAX+1]; + getcwd(old_path, sizeof(old_path)); if (!dir || chdir(path)) return 0; @@ -452,6 +454,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds, } closedir(dir); + chdir(old_path); return longest; } -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 2/7] builtin-help: change the current directory back in list_commands_in_dir() 2008-07-26 11:54 ` [PATCH 2/7] builtin-help: change the current directory back in list_commands_in_dir() Miklos Vajna @ 2008-07-26 14:58 ` Johannes Schindelin 2008-07-27 20:02 ` Junio C Hamano 1 sibling, 0 replies; 71+ messages in thread From: Johannes Schindelin @ 2008-07-26 14:58 UTC (permalink / raw) To: Miklos Vajna; +Cc: git 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 2/7] builtin-help: change the current directory back in list_commands_in_dir() 2008-07-26 11:54 ` [PATCH 2/7] builtin-help: change the current directory back in list_commands_in_dir() Miklos Vajna 2008-07-26 14:58 ` Johannes Schindelin @ 2008-07-27 20:02 ` Junio C Hamano 2008-07-27 20:21 ` Johannes Schindelin 1 sibling, 1 reply; 71+ messages in thread From: Junio C Hamano @ 2008-07-27 20:02 UTC (permalink / raw) To: Miklos Vajna; +Cc: git 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? ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 2/7] builtin-help: change the current directory back in list_commands_in_dir() 2008-07-27 20:02 ` Junio C Hamano @ 2008-07-27 20:21 ` Johannes Schindelin 2008-07-27 20:34 ` [PATCH] Avoid chdir() " Johannes Schindelin 0 siblings, 1 reply; 71+ messages in thread From: Johannes Schindelin @ 2008-07-27 20:21 UTC (permalink / raw) To: Junio C Hamano; +Cc: Miklos Vajna, git Hi, On Sun, 27 Jul 2008, Junio C Hamano wrote: > Miklos Vajna <vmiklos@frugalware•org> writes: > > > That function used to do a chdir() without switching back to the > > original directory. That was not a problem till this function was used > > only inside builtin-help, but once other builtins use it as well, this > > is a problem, for example when the object database path is relative. > > Why does it even need to chdir() around to begin with? Doesn't > opendir() work just fine with relative path as an input? It is a consequence of list_commands_in_dir() trying to be cute, and not having to construct the full path for the is_executable() check. Will post a fix in a few minutes. Ciao, Dscho ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH] Avoid chdir() in list_commands_in_dir() 2008-07-27 20:21 ` Johannes Schindelin @ 2008-07-27 20:34 ` Johannes Schindelin 0 siblings, 0 replies; 71+ messages in thread From: Johannes Schindelin @ 2008-07-27 20:34 UTC (permalink / raw) To: Junio C Hamano; +Cc: Miklos Vajna, git The function list_commands_in_dir() tried to be lazy and just chdir() to the directory which entries it listed, so that the check if the file is executable could be done on dir->d_name. However, there is no good reason to jump around wildly just to find all Git commands. Instead, have a strbuf and construct the full path dynamically. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx•de> --- help.c | 12 ++++++++++-- 1 files changed, 10 insertions(+), 2 deletions(-) diff --git a/help.c b/help.c index 480befe..7af6582 100644 --- a/help.c +++ b/help.c @@ -426,17 +426,24 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds, int prefix_len = strlen(prefix); DIR *dir = opendir(path); struct dirent *de; + struct strbuf buf = STRBUF_INIT; + int len; - if (!dir || chdir(path)) + if (!dir) return 0; + strbuf_addf(&buf, "%s/", path); + len = buf.len; + while ((de = readdir(dir)) != NULL) { int entlen; if (prefixcmp(de->d_name, prefix)) continue; - if (!is_executable(de->d_name)) + strbuf_setlen(&buf, len); + strbuf_addstr(&buf, de->d_name); + if (!is_executable(buf.buf)) continue; entlen = strlen(de->d_name) - prefix_len; @@ -449,6 +456,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds, add_cmdname(cmds, de->d_name + prefix_len, entlen); } closedir(dir); + strbuf_release(&buf); return longest; } -- 1.6.0.rc0.97.ge2309 ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 3/7] builtin-help: make list_commands() a bit more generic 2008-07-26 11:54 ` [PATCH 0/7] Allow custom merge strategies Miklos Vajna 2008-07-26 11:54 ` [PATCH 1/7] Make is_git_command() usable outside builtin-help Miklos Vajna 2008-07-26 11:54 ` [PATCH 2/7] builtin-help: change the current directory back in list_commands_in_dir() Miklos Vajna @ 2008-07-26 11:54 ` Miklos Vajna 2008-07-26 18:28 ` Jonathan Nieder 2008-07-26 11:54 ` [PATCH 4/7] builtin-merge: allow using a custom strategy Miklos Vajna ` (4 subsequent siblings) 7 siblings, 1 reply; 71+ messages in thread From: Miklos Vajna @ 2008-07-26 11:54 UTC (permalink / raw) To: git That function now takes two paramters to control the prefix of the listed commands, and a second parameter to specify the title of the table. This can be useful for listing not only all git commands, but specific ones, like merge strategies. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- help.c | 10 +++++----- help.h | 1 + 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/help.c b/help.c index d71937e..f71fff4 100644 --- a/help.c +++ b/help.c @@ -501,13 +501,13 @@ static unsigned int load_command_list(const char *prefix) return longest; } -static void list_commands(void) +void list_commands(const char *prefix, const char *title) { - unsigned int longest = load_command_list(NULL); + unsigned int longest = load_command_list(prefix); const char *exec_path = git_exec_path(); if (main_cmds.cnt) { - printf("available git commands in '%s'\n", exec_path); + printf("available %s in '%s'\n", title, exec_path); printf("----------------------------"); mput_char('-', strlen(exec_path)); putchar('\n'); @@ -516,7 +516,7 @@ static void list_commands(void) } if (other_cmds.cnt) { - printf("git commands available from elsewhere on your $PATH\n"); + printf("%s available from elsewhere on your $PATH\n", title); printf("---------------------------------------------------\n"); pretty_print_string_list(&other_cmds, longest); putchar('\n'); @@ -697,7 +697,7 @@ int cmd_help(int argc, const char **argv, const char *prefix) if (show_all) { printf("usage: %s\n\n", git_usage_string); - list_commands(); + list_commands("git-", "git commands"); printf("%s\n", git_more_info_string); return 0; } diff --git a/help.h b/help.h index 73da8d6..0741662 100644 --- a/help.h +++ b/help.h @@ -2,5 +2,6 @@ #define HELP_H int is_git_command(const char *s, const char *prefix); +void list_commands(const char *prefix, const char *title); #endif /* HELP_H */ -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 3/7] builtin-help: make list_commands() a bit more generic 2008-07-26 11:54 ` [PATCH 3/7] builtin-help: make list_commands() a bit more generic Miklos Vajna @ 2008-07-26 18:28 ` Jonathan Nieder 2008-07-26 21:40 ` Miklos Vajna 0 siblings, 1 reply; 71+ messages in thread From: Jonathan Nieder @ 2008-07-26 18:28 UTC (permalink / raw) To: Miklos Vajna; +Cc: git On Sat, 26 Jul 2008, Miklos Vajna wrote: > -static void list_commands(void) > +void list_commands(const char *prefix, const char *title) > { > - unsigned int longest = load_command_list(NULL); > + unsigned int longest = load_command_list(prefix); > const char *exec_path = git_exec_path(); > > if (main_cmds.cnt) { > - printf("available git commands in '%s'\n", exec_path); > + printf("available %s in '%s'\n", title, exec_path); > printf("----------------------------"); > mput_char('-', strlen(exec_path)); > putchar('\n'); Should this be printf("available %s in '%s'\n", title, exec_path); printf("----------------"); mput_char('-', strlen(exec_path) + strlen(title)); putchar('\n'); ? (same question goes for the if(other_cmds.cnt) block, too) ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 3/7] builtin-help: make list_commands() a bit more generic 2008-07-26 18:28 ` Jonathan Nieder @ 2008-07-26 21:40 ` Miklos Vajna 0 siblings, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-26 21:40 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git That function now takes two paramters to control the prefix of the listed commands, and a second parameter to specify the title of the table. This can be useful for listing not only all git commands, but specific ones, like merge strategies. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- On Sat, Jul 26, 2008 at 01:28:39PM -0500, Jonathan Nieder <jrnieder@uchicago•edu> wrote: > > if (main_cmds.cnt) { > > - printf("available git commands in '%s'\n", exec_path); > > + printf("available %s in '%s'\n", title, exec_path); > > printf("----------------------------"); > > mput_char('-', strlen(exec_path)); > > putchar('\n'); > > Should this be > > printf("available %s in '%s'\n", title, exec_path); > printf("----------------"); > mput_char('-', strlen(exec_path) + strlen(title)); > putchar('\n'); > > ? > > (same question goes for the if(other_cmds.cnt) block, too) Right. Here is an updated patch. Also available at git://repo.or.cz/git/vmiklos.git, branch 'merge-custom'. help.c | 18 ++++++++++-------- help.h | 1 + 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/help.c b/help.c index d71937e..ec7999d 100644 --- a/help.c +++ b/help.c @@ -501,23 +501,25 @@ static unsigned int load_command_list(const char *prefix) return longest; } -static void list_commands(void) +void list_commands(const char *prefix, const char *title) { - unsigned int longest = load_command_list(NULL); + unsigned int longest = load_command_list(prefix); const char *exec_path = git_exec_path(); if (main_cmds.cnt) { - printf("available git commands in '%s'\n", exec_path); - printf("----------------------------"); - mput_char('-', strlen(exec_path)); + printf("available %s in '%s'\n", title, exec_path); + printf("----------------"); + mput_char('-', strlen(title) + strlen(exec_path)); putchar('\n'); pretty_print_string_list(&main_cmds, longest); putchar('\n'); } if (other_cmds.cnt) { - printf("git commands available from elsewhere on your $PATH\n"); - printf("---------------------------------------------------\n"); + printf("%s available from elsewhere on your $PATH\n", title); + printf("---------------------------------------"); + mput_char('-', strlen(title)); + putchar('\n'); pretty_print_string_list(&other_cmds, longest); putchar('\n'); } @@ -697,7 +699,7 @@ int cmd_help(int argc, const char **argv, const char *prefix) if (show_all) { printf("usage: %s\n\n", git_usage_string); - list_commands(); + list_commands("git-", "git commands"); printf("%s\n", git_more_info_string); return 0; } diff --git a/help.h b/help.h index 73da8d6..0741662 100644 --- a/help.h +++ b/help.h @@ -2,5 +2,6 @@ #define HELP_H int is_git_command(const char *s, const char *prefix); +void list_commands(const char *prefix, const char *title); #endif /* HELP_H */ -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 4/7] builtin-merge: allow using a custom strategy 2008-07-26 11:54 ` [PATCH 0/7] Allow custom merge strategies Miklos Vajna ` (2 preceding siblings ...) 2008-07-26 11:54 ` [PATCH 3/7] builtin-help: make list_commands() a bit more generic Miklos Vajna @ 2008-07-26 11:54 ` Miklos Vajna 2008-07-26 11:54 ` [PATCH 5/7] Add a new test for using a custom merge strategy Miklos Vajna ` (3 subsequent siblings) 7 siblings, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-26 11:54 UTC (permalink / raw) To: git Allow using a custom strategy, as long as it's named git-merge-foo. The error handling is now done using is_git_command(). The list of available strategies is now shown by list_commands(). Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- builtin-merge.c | 19 ++++++++++++------- 1 files changed, 12 insertions(+), 7 deletions(-) diff --git a/builtin-merge.c b/builtin-merge.c index e78fa18..cdbc692 100644 --- a/builtin-merge.c +++ b/builtin-merge.c @@ -22,6 +22,7 @@ #include "log-tree.h" #include "color.h" #include "rerere.h" +#include "help.h" #define DEFAULT_TWOHEAD (1<<0) #define DEFAULT_OCTOPUS (1<<1) @@ -77,7 +78,7 @@ static int option_parse_message(const struct option *opt, static struct strategy *get_strategy(const char *name) { int i; - struct strbuf err; + struct strategy *ret; if (!name) return NULL; @@ -86,12 +87,16 @@ static struct strategy *get_strategy(const char *name) if (!strcmp(name, all_strategy[i].name)) return &all_strategy[i]; - strbuf_init(&err, 0); - for (i = 0; i < ARRAY_SIZE(all_strategy); i++) - strbuf_addf(&err, " %s", all_strategy[i].name); - fprintf(stderr, "Could not find merge strategy '%s'.\n", name); - fprintf(stderr, "Available strategies are:%s.\n", err.buf); - exit(1); + if (!is_git_command(name, "git-merge-")) { + fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name); + list_commands("git-merge-", "strategies"); + exit(1); + } + + ret = xmalloc(sizeof(struct strategy)); + memset(ret, 0, sizeof(struct strategy)); + ret->name = xstrdup(name); + return ret; } static void append_strategy(struct strategy *s) -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 5/7] Add a new test for using a custom merge strategy 2008-07-26 11:54 ` [PATCH 0/7] Allow custom merge strategies Miklos Vajna ` (3 preceding siblings ...) 2008-07-26 11:54 ` [PATCH 4/7] builtin-merge: allow using a custom strategy Miklos Vajna @ 2008-07-26 11:54 ` Miklos Vajna 2008-07-26 11:54 ` [PATCH 6/7] builtin-help: make it possible to exclude some commands in list_commands() Miklos Vajna ` (2 subsequent siblings) 7 siblings, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-26 11:54 UTC (permalink / raw) To: git Testing is done by creating a simple git-merge-theirs strategy which is the opposite of ours. Using this in real merges is not recommended but it's perfect for our testing needs. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- t/t7606-merge-custom.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 45 insertions(+), 0 deletions(-) create mode 100755 t/t7606-merge-custom.sh diff --git a/t/t7606-merge-custom.sh b/t/t7606-merge-custom.sh new file mode 100755 index 0000000..f295e56 --- /dev/null +++ b/t/t7606-merge-custom.sh @@ -0,0 +1,45 @@ +#!/bin/sh + +test_description='git-merge + +Testing a custom strategy.' + +. ./test-lib.sh + +cat > git-merge-theirs << EOF +#!/bin/sh +eval git read-tree --reset -u \\\$\$# +EOF +chmod +x git-merge-theirs +PATH=.:$PATH +export PATH + +test_expect_success 'setup' ' + echo c0 > c0.c && + git add c0.c && + git commit -m c0 && + git tag c0 && + echo c1 > c1.c && + git add c1.c && + git commit -m c1 && + git tag c1 && + git reset --hard c0 && + echo c2 > c2.c && + git add c2.c && + git commit -m c2 && + git tag c2 +' + +test_expect_success 'merge c2 with a custom strategy' ' + git reset --hard c1 && + git merge -s theirs c2 && + test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" && + test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" && + test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" && + git diff --exit-code && + test -f c0.c && + test ! -f c1.c && + test -f c2.c +' + +test_done -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 6/7] builtin-help: make it possible to exclude some commands in list_commands() 2008-07-26 11:54 ` [PATCH 0/7] Allow custom merge strategies Miklos Vajna ` (4 preceding siblings ...) 2008-07-26 11:54 ` [PATCH 5/7] Add a new test for using a custom merge strategy Miklos Vajna @ 2008-07-26 11:54 ` Miklos Vajna 2008-07-26 11:54 ` [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message Miklos Vajna 2008-07-28 1:21 ` [PATCH 0/6] Allow custom merge strategies, take 2 Miklos Vajna 7 siblings, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-26 11:54 UTC (permalink / raw) To: git The supposed method is to build a list of commands to be excluded using add_cmdname(), then pass the list as the new exclude parameter. If no exclude is needed, NULL should be used. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- help.c | 24 ++++++++++-------------- help.h | 14 +++++++++++++- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/help.c b/help.c index f71fff4..de1be6d 100644 --- a/help.c +++ b/help.c @@ -9,6 +9,7 @@ #include "common-cmds.h" #include "parse-options.h" #include "run-command.h" +#include "help.h" static struct man_viewer_list { struct man_viewer_list *next; @@ -300,16 +301,9 @@ static inline void mput_char(char c, unsigned int num) putchar(c); } -static struct cmdnames { - int alloc; - int cnt; - struct cmdname { - size_t len; - char name[1]; - } **names; -} main_cmds, other_cmds; +struct cmdnames main_cmds, other_cmds; -static void add_cmdname(struct cmdnames *cmds, const char *name, int len) +void add_cmdname(struct cmdnames *cmds, const char *name, int len) { struct cmdname *ent = xmalloc(sizeof(*ent) + len); @@ -458,7 +452,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds, return longest; } -static unsigned int load_command_list(const char *prefix) +static unsigned int load_command_list(const char *prefix, struct cmdnames *exclude) { unsigned int longest = 0; unsigned int len; @@ -497,13 +491,15 @@ static unsigned int load_command_list(const char *prefix) sizeof(*other_cmds.names), cmdname_compare); uniq(&other_cmds); exclude_cmds(&other_cmds, &main_cmds); + if (exclude) + exclude_cmds(&main_cmds, exclude); return longest; } -void list_commands(const char *prefix, const char *title) +void list_commands(const char *prefix, const char *title, struct cmdnames *exclude) { - unsigned int longest = load_command_list(prefix); + unsigned int longest = load_command_list(prefix, exclude); const char *exec_path = git_exec_path(); if (main_cmds.cnt) { @@ -551,7 +547,7 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s) int is_git_command(const char *s, const char *prefix) { - load_command_list(prefix); + load_command_list(prefix, NULL); return is_in_cmdlist(&main_cmds, s) || is_in_cmdlist(&other_cmds, s); } @@ -697,7 +693,7 @@ int cmd_help(int argc, const char **argv, const char *prefix) if (show_all) { printf("usage: %s\n\n", git_usage_string); - list_commands("git-", "git commands"); + list_commands("git-", "git commands", NULL); printf("%s\n", git_more_info_string); return 0; } diff --git a/help.h b/help.h index 0741662..85d3b74 100644 --- a/help.h +++ b/help.h @@ -1,7 +1,19 @@ #ifndef HELP_H #define HELP_H +struct cmdnames { + int alloc; + int cnt; + struct cmdname { + size_t len; + char name[1]; + } **names; +}; + int is_git_command(const char *s, const char *prefix); -void list_commands(const char *prefix, const char *title); +void list_commands(const char *prefix, const char *title, struct cmdnames *exclude); +void add_cmdname(struct cmdnames *cmds, const char *name, int len); + +extern struct cmdnames main_cmds, other_cmds; #endif /* HELP_H */ -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message 2008-07-26 11:54 ` [PATCH 0/7] Allow custom merge strategies Miklos Vajna ` (5 preceding siblings ...) 2008-07-26 11:54 ` [PATCH 6/7] builtin-help: make it possible to exclude some commands in list_commands() Miklos Vajna @ 2008-07-26 11:54 ` Miklos Vajna 2008-07-26 15:08 ` Johannes Schindelin 2008-07-28 1:21 ` [PATCH 0/6] Allow custom merge strategies, take 2 Miklos Vajna 7 siblings, 1 reply; 71+ messages in thread From: Miklos Vajna @ 2008-07-26 11:54 UTC (permalink / raw) To: git If an invalid strategy is supplied, like -s foobar, then git-merge listed all git-merge-* commands. This is not perfect, since for example git-merge-index is not a valid strategy. These are now removed from the output by scanning the list of main commands; if the git-merge-foo command is listed in the all_strategy list, then it's shown, otherwise excluded. This does not exclude commands somewhere else in the PATH, where custom strategies are expected. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- builtin-merge.c | 13 ++++++++++++- 1 files changed, 12 insertions(+), 1 deletions(-) diff --git a/builtin-merge.c b/builtin-merge.c index cdbc692..4084e07 100644 --- a/builtin-merge.c +++ b/builtin-merge.c @@ -88,8 +88,19 @@ static struct strategy *get_strategy(const char *name) return &all_strategy[i]; if (!is_git_command(name, "git-merge-")) { + struct cmdnames not_strategies; + + memset(¬_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(¬_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", ¬_strategies); exit(1); } -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message 2008-07-26 11:54 ` [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message Miklos Vajna @ 2008-07-26 15:08 ` Johannes Schindelin 2008-07-26 15:25 ` Miklos Vajna 0 siblings, 1 reply; 71+ messages in thread From: Johannes Schindelin @ 2008-07-26 15:08 UTC (permalink / raw) To: Miklos Vajna; +Cc: git Hi, On Sat, 26 Jul 2008, Miklos Vajna wrote: > + memset(¬_strategies, 0, sizeof(struct cmdnames)); > + for (i = 0; i < main_cmds.cnt; i++) { Looking through all the discovered git commands? Cute... But does that not exclude the commands that are in PATH, starting with git-merge-, even if they are custom strategies? > + int j, found = 0; > + for (j = 0; j < ARRAY_SIZE(all_strategy); j++) > + if (!strcmp(main_cmds.names[i]->name, all_strategy[j].name)) > + found = 1; > + if (!found) > + add_cmdname(¬_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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message 2008-07-26 15:08 ` Johannes Schindelin @ 2008-07-26 15:25 ` Miklos Vajna 2008-07-26 15:27 ` Miklos Vajna 2008-07-26 15:38 ` Johannes Schindelin 0 siblings, 2 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-26 15:25 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git [-- Attachment #1: Type: text/plain, Size: 2890 bytes --] On Sat, Jul 26, 2008 at 05:08:11PM +0200, Johannes Schindelin <Johannes.Schindelin@gmx•de> wrote: > > + memset(¬_strategies, 0, sizeof(struct cmdnames)); > > + for (i = 0; i < main_cmds.cnt; i++) { > > Looking through all the discovered git commands? Cute... But does that > not exclude the commands that are in PATH, starting with git-merge-, even > if they are custom strategies? main_cmds contains only commands in /usr/libexec/git-core, while I guess custom strategies are elsewhere in PATH, which commands are in other_cmds, not in main_cmds. Sample output at me: $ git merge -s theirss c2 HEAD is now at 1f5e3cc c1 Could not find merge strategy 'theirss'. available strategies in '/usr/libexec/git-core/' -------------------------------------------------- octopus ours recursive resolve subtree strategies available from elsewhere on your $PATH --------------------------------------------------- theirs and I have git-merge-theirs in ~/bin (which is in PATH). > > > + int j, found = 0; > > + for (j = 0; j < ARRAY_SIZE(all_strategy); j++) > > + if (!strcmp(main_cmds.names[i]->name, all_strategy[j].name)) > > + found = 1; > > + if (!found) > > + add_cmdname(¬_strategies, main_cmds.names[i]->name, strlen(main_cmds.names[i]->name)); > > Better have a local variable "name" instead of writing out > "main_cmds.names[i]->name" all the time... Fixed. > Oh, and you assume that the names are NUL-terminated (which I assume is > not the case in general, as the len member is the only thing that makes > struct cmdnames different from struct string_list. I think the purpose of it is different, but the argument is still valie. That len member is to be able to have ->name contain "foo.exe" while having len at 3, so that git help -a will avoid the .exe suffixes. Changed. (I do not want to resend a full series yet, but I pushed out an amended patch to repo.or.cz in the 'merge-custom' branch.) diff --git a/builtin-merge.c b/builtin-merge.c index 4084e07..b0d1de5 100644 --- a/builtin-merge.c +++ b/builtin-merge.c @@ -93,11 +93,12 @@ static struct strategy *get_strategy(const char *name) memset(¬_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(¬_strategies, main_cmds.names[i]->name, strlen(main_cmds.names[i]->name)); + add_cmdname(¬_strategies, ent->name, ent->len); } fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name); list_commands("git-merge-", "strategies", ¬_strategies); [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message 2008-07-26 15:25 ` Miklos Vajna @ 2008-07-26 15:27 ` Miklos Vajna 2008-07-26 15:38 ` Johannes Schindelin 1 sibling, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-26 15:27 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git If an invalid strategy is supplied, like -s foobar, then git-merge listed all git-merge-* commands. This is not perfect, since for example git-merge-index is not a valid strategy. These are now removed from the output by scanning the list of main commands; if the git-merge-foo command is listed in the all_strategy list, then it's shown, otherwise excluded. This does not exclude commands somewhere else in the PATH, where custom strategies are expected. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- I just realized I can resend the last patch as the others are unchanged, so here it is. builtin-merge.c | 14 +++++++++++++- 1 files changed, 13 insertions(+), 1 deletions(-) diff --git a/builtin-merge.c b/builtin-merge.c index cdbc692..b0d1de5 100644 --- a/builtin-merge.c +++ b/builtin-merge.c @@ -88,8 +88,20 @@ static struct strategy *get_strategy(const char *name) return &all_strategy[i]; if (!is_git_command(name, "git-merge-")) { + struct cmdnames not_strategies; + + memset(¬_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(¬_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", ¬_strategies); exit(1); } -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message 2008-07-26 15:25 ` Miklos Vajna 2008-07-26 15:27 ` Miklos Vajna @ 2008-07-26 15:38 ` Johannes Schindelin 2008-07-26 16:00 ` Miklos Vajna 1 sibling, 1 reply; 71+ messages in thread From: Johannes Schindelin @ 2008-07-26 15:38 UTC (permalink / raw) To: Miklos Vajna; +Cc: git Hi, On Sat, 26 Jul 2008, Miklos Vajna wrote: > On Sat, Jul 26, 2008 at 05:08:11PM +0200, Johannes Schindelin <Johannes.Schindelin@gmx•de> wrote: > > > + memset(¬_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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message 2008-07-26 15:38 ` Johannes Schindelin @ 2008-07-26 16:00 ` Miklos Vajna 2008-07-26 17:01 ` Johannes Schindelin 0 siblings, 1 reply; 71+ messages in thread From: Miklos Vajna @ 2008-07-26 16:00 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git [-- Attachment #1: Type: text/plain, Size: 725 bytes --] On Sat, Jul 26, 2008 at 05:38:55PM +0200, Johannes Schindelin <Johannes.Schindelin@gmx•de> wrote: > > - if (!strcmp(main_cmds.names[i]->name, all_strategy[j].name)) > > + if (!strncmp(ent->name, all_strategy[j].name, ent->len)) > > Oops... that is not what I meant. You'd have to check if > !all_strategy[j].name[ent->len], too... Hmm. So let's say ent->name is "ours.exe", ent->len is set to 4. Then !strncmp(ent->name, all_strategy[j].name, ent->len) will be true, and the command will not be added to the exclude list. However, if I check for !all_strategy[j].name[ent->len], that will be false, so 'ours' will be excluded from the available strategy list. Have I missed something? Thanks. [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message 2008-07-26 16:00 ` Miklos Vajna @ 2008-07-26 17:01 ` Johannes Schindelin 2008-07-26 17:12 ` [PATCH] " Miklos Vajna 0 siblings, 1 reply; 71+ messages in thread From: Johannes Schindelin @ 2008-07-26 17:01 UTC (permalink / raw) To: Miklos Vajna; +Cc: git Hi, On Sat, 26 Jul 2008, Miklos Vajna wrote: > On Sat, Jul 26, 2008 at 05:38:55PM +0200, Johannes Schindelin <Johannes.Schindelin@gmx•de> wrote: > > > - if (!strcmp(main_cmds.names[i]->name, all_strategy[j].name)) > > > + if (!strncmp(ent->name, all_strategy[j].name, ent->len)) > > > > Oops... that is not what I meant. You'd have to check if > > !all_strategy[j].name[ent->len], too... > > Hmm. So let's say ent->name is "ours.exe", ent->len is set to 4. > > Then !strncmp(ent->name, all_strategy[j].name, ent->len) will be true, > and the command will not be added to the exclude list. What I meant is: if you change the strcmp to strncmp because one of the both strings is not supposed to be NUL terminated, you still want to make sure that one is not a strict prefix of the other. Ciao, Dscho ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH] builtin-merge: avoid non-strategy git-merge commands in error message 2008-07-26 17:01 ` Johannes Schindelin @ 2008-07-26 17:12 ` Miklos Vajna 0 siblings, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-26 17:12 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git If an invalid strategy is supplied, like -s foobar, then git-merge listed all git-merge-* commands. This is not perfect, since for example git-merge-index is not a valid strategy. These are now removed from the output by scanning the list of main commands; if the git-merge-foo command is listed in the all_strategy list, then it's shown, otherwise excluded. This does not exclude commands somewhere else in the PATH, where custom strategies are expected. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- On Sat, Jul 26, 2008 at 07:01:16PM +0200, Johannes Schindelin <Johannes.Schindelin@gmx•de> wrote: > > Hmm. So let's say ent->name is "ours.exe", ent->len is set to 4. > > > > Then !strncmp(ent->name, all_strategy[j].name, ent->len) will be > > true, > > and the command will not be added to the exclude list. > > What I meant is: if you change the strcmp to strncmp because one of > the > both strings is not supposed to be NUL terminated, you still want to > make > sure that one is not a strict prefix of the other. Aah, I forgot about all_strategy[j].name is NUL terminated. Updated patch below. builtin-merge.c | 15 ++++++++++++++- 1 files changed, 14 insertions(+), 1 deletions(-) diff --git a/builtin-merge.c b/builtin-merge.c index cdbc692..29826b1 100644 --- a/builtin-merge.c +++ b/builtin-merge.c @@ -88,8 +88,21 @@ static struct strategy *get_strategy(const char *name) return &all_strategy[i]; if (!is_git_command(name, "git-merge-")) { + struct cmdnames not_strategies; + + memset(¬_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(¬_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", ¬_strategies); exit(1); } -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 0/6] Allow custom merge strategies, take 2 2008-07-26 11:54 ` [PATCH 0/7] Allow custom merge strategies Miklos Vajna ` (6 preceding siblings ...) 2008-07-26 11:54 ` [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message Miklos Vajna @ 2008-07-28 1:21 ` Miklos Vajna 2008-07-28 1:21 ` [PATCH 1/6] builtin-help: make is_git_command() usable outside builtin-help Miklos Vajna ` (6 more replies) 7 siblings, 7 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-28 1:21 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 1/6] builtin-help: make is_git_command() usable outside builtin-help 2008-07-28 1:21 ` [PATCH 0/6] Allow custom merge strategies, take 2 Miklos Vajna @ 2008-07-28 1:21 ` Miklos Vajna 2008-07-28 1:21 ` [PATCH 2/6] builtin-help: make list_commands() a bit more generic Miklos Vajna ` (5 subsequent siblings) 6 siblings, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-28 1:21 UTC (permalink / raw) To: git Other builtins may want to check if a given command is a valid git command or not as well. Additionally add a new parameter that specifies a custom prefix, so that the "git-" prefix is no longer hardwired. Useful for example to limit the search for "git-merge-*". Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- Makefile | 1 + help.c | 25 ++++++++++++++----------- help.h | 6 ++++++ 3 files changed, 21 insertions(+), 11 deletions(-) create mode 100644 help.h diff --git a/Makefile b/Makefile index 798a2f2..351afd7 100644 --- a/Makefile +++ b/Makefile @@ -355,6 +355,7 @@ LIB_H += git-compat-util.h LIB_H += graph.h LIB_H += grep.h LIB_H += hash.h +LIB_H += help.h LIB_H += list-objects.h LIB_H += ll-merge.h LIB_H += log-tree.h diff --git a/help.c b/help.c index 3cb1962..08188f5 100644 --- a/help.c +++ b/help.c @@ -418,11 +418,11 @@ static int is_executable(const char *name) } static unsigned int list_commands_in_dir(struct cmdnames *cmds, - const char *path) + const char *path, + const char *prefix) { unsigned int longest = 0; - const char *prefix = "git-"; - int prefix_len = strlen(prefix); + int prefix_len; DIR *dir = opendir(path); struct dirent *de; struct strbuf buf = STRBUF_INIT; @@ -430,6 +430,9 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds, if (!dir) return 0; + if (!prefix) + prefix = "git-"; + prefix_len = strlen(prefix); strbuf_addf(&buf, "%s/", path); len = buf.len; @@ -460,7 +463,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds, return longest; } -static unsigned int load_command_list(void) +static unsigned int load_command_list(const char *prefix) { unsigned int longest = 0; unsigned int len; @@ -469,7 +472,7 @@ static unsigned int load_command_list(void) const char *exec_path = git_exec_path(); if (exec_path) - longest = list_commands_in_dir(&main_cmds, exec_path); + longest = list_commands_in_dir(&main_cmds, exec_path, prefix); if (!env_path) { fprintf(stderr, "PATH not set\n"); @@ -481,7 +484,7 @@ static unsigned int load_command_list(void) if ((colon = strchr(path, PATH_SEP))) *colon = 0; - len = list_commands_in_dir(&other_cmds, path); + len = list_commands_in_dir(&other_cmds, path, prefix); if (len > longest) longest = len; @@ -505,7 +508,7 @@ static unsigned int load_command_list(void) static void list_commands(void) { - unsigned int longest = load_command_list(); + unsigned int longest = load_command_list(NULL); const char *exec_path = git_exec_path(); if (main_cmds.cnt) { @@ -551,9 +554,9 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s) return 0; } -static int is_git_command(const char *s) +int is_git_command(const char *s, const char *prefix) { - load_command_list(); + load_command_list(prefix); return is_in_cmdlist(&main_cmds, s) || is_in_cmdlist(&other_cmds, s); } @@ -574,7 +577,7 @@ static const char *cmd_to_page(const char *git_cmd) return "git"; else if (!prefixcmp(git_cmd, "git")) return git_cmd; - else if (is_git_command(git_cmd)) + else if (is_git_command(git_cmd, NULL)) return prepend("git-", git_cmd); else return prepend("git", git_cmd); @@ -712,7 +715,7 @@ int cmd_help(int argc, const char **argv, const char *prefix) } alias = alias_lookup(argv[0]); - if (alias && !is_git_command(argv[0])) { + if (alias && !is_git_command(argv[0], NULL)) { printf("`git %s' is aliased to `%s'\n", argv[0], alias); return 0; } diff --git a/help.h b/help.h new file mode 100644 index 0000000..73da8d6 --- /dev/null +++ b/help.h @@ -0,0 +1,6 @@ +#ifndef HELP_H +#define HELP_H + +int is_git_command(const char *s, const char *prefix); + +#endif /* HELP_H */ -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 2/6] builtin-help: make list_commands() a bit more generic 2008-07-28 1:21 ` [PATCH 0/6] Allow custom merge strategies, take 2 Miklos Vajna 2008-07-28 1:21 ` [PATCH 1/6] builtin-help: make is_git_command() usable outside builtin-help Miklos Vajna @ 2008-07-28 1:21 ` Miklos Vajna 2008-07-28 1:21 ` [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() Miklos Vajna ` (4 subsequent siblings) 6 siblings, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-28 1:21 UTC (permalink / raw) To: git That function now takes two paramters to control the prefix of the listed commands, and a second parameter to specify the title of the table. This can be useful for listing not only all git commands, but specific ones, like merge strategies. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- help.c | 18 ++++++++++-------- help.h | 1 + 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/help.c b/help.c index 08188f5..7a42517 100644 --- a/help.c +++ b/help.c @@ -506,23 +506,25 @@ static unsigned int load_command_list(const char *prefix) return longest; } -static void list_commands(void) +void list_commands(const char *prefix, const char *title) { - unsigned int longest = load_command_list(NULL); + unsigned int longest = load_command_list(prefix); const char *exec_path = git_exec_path(); if (main_cmds.cnt) { - printf("available git commands in '%s'\n", exec_path); - printf("----------------------------"); - mput_char('-', strlen(exec_path)); + printf("available %s in '%s'\n", title, exec_path); + printf("----------------"); + mput_char('-', strlen(title) + strlen(exec_path)); putchar('\n'); pretty_print_string_list(&main_cmds, longest); putchar('\n'); } if (other_cmds.cnt) { - printf("git commands available from elsewhere on your $PATH\n"); - printf("---------------------------------------------------\n"); + printf("%s available from elsewhere on your $PATH\n", title); + printf("---------------------------------------"); + mput_char('-', strlen(title)); + putchar('\n'); pretty_print_string_list(&other_cmds, longest); putchar('\n'); } @@ -702,7 +704,7 @@ int cmd_help(int argc, const char **argv, const char *prefix) if (show_all) { printf("usage: %s\n\n", git_usage_string); - list_commands(); + list_commands("git-", "git commands"); printf("%s\n", git_more_info_string); return 0; } diff --git a/help.h b/help.h index 73da8d6..0741662 100644 --- a/help.h +++ b/help.h @@ -2,5 +2,6 @@ #define HELP_H int is_git_command(const char *s, const char *prefix); +void list_commands(const char *prefix, const char *title); #endif /* HELP_H */ -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() 2008-07-28 1:21 ` [PATCH 0/6] Allow custom merge strategies, take 2 Miklos Vajna 2008-07-28 1:21 ` [PATCH 1/6] builtin-help: make is_git_command() usable outside builtin-help Miklos Vajna 2008-07-28 1:21 ` [PATCH 2/6] builtin-help: make list_commands() a bit more generic Miklos Vajna @ 2008-07-28 1:21 ` Miklos Vajna 2008-07-28 1:36 ` Junio C Hamano 2008-07-28 1:21 ` [PATCH 4/6] builtin-merge: allow using a custom strategy Miklos Vajna ` (3 subsequent siblings) 6 siblings, 1 reply; 71+ messages in thread From: Miklos Vajna @ 2008-07-28 1:21 UTC (permalink / raw) To: git The supposed method is to build a list of commands to be excluded using add_cmdname(), then pass the list as the new exclude parameter. If no exclude is needed, NULL should be used. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- help.c | 24 ++++++++++-------------- help.h | 14 +++++++++++++- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/help.c b/help.c index 7a42517..318d6d6 100644 --- a/help.c +++ b/help.c @@ -9,6 +9,7 @@ #include "common-cmds.h" #include "parse-options.h" #include "run-command.h" +#include "help.h" static struct man_viewer_list { struct man_viewer_list *next; @@ -300,16 +301,9 @@ static inline void mput_char(char c, unsigned int num) putchar(c); } -static struct cmdnames { - int alloc; - int cnt; - struct cmdname { - size_t len; - char name[1]; - } **names; -} main_cmds, other_cmds; +struct cmdnames main_cmds, other_cmds; -static void add_cmdname(struct cmdnames *cmds, const char *name, int len) +void add_cmdname(struct cmdnames *cmds, const char *name, int len) { struct cmdname *ent = xmalloc(sizeof(*ent) + len); @@ -463,7 +457,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds, return longest; } -static unsigned int load_command_list(const char *prefix) +static unsigned int load_command_list(const char *prefix, struct cmdnames *exclude) { unsigned int longest = 0; unsigned int len; @@ -502,13 +496,15 @@ static unsigned int load_command_list(const char *prefix) sizeof(*other_cmds.names), cmdname_compare); uniq(&other_cmds); exclude_cmds(&other_cmds, &main_cmds); + if (exclude) + exclude_cmds(&main_cmds, exclude); return longest; } -void list_commands(const char *prefix, const char *title) +void list_commands(const char *prefix, const char *title, struct cmdnames *exclude) { - unsigned int longest = load_command_list(prefix); + unsigned int longest = load_command_list(prefix, exclude); const char *exec_path = git_exec_path(); if (main_cmds.cnt) { @@ -558,7 +554,7 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s) int is_git_command(const char *s, const char *prefix) { - load_command_list(prefix); + load_command_list(prefix, NULL); return is_in_cmdlist(&main_cmds, s) || is_in_cmdlist(&other_cmds, s); } @@ -704,7 +700,7 @@ int cmd_help(int argc, const char **argv, const char *prefix) if (show_all) { printf("usage: %s\n\n", git_usage_string); - list_commands("git-", "git commands"); + list_commands("git-", "git commands", NULL); printf("%s\n", git_more_info_string); return 0; } diff --git a/help.h b/help.h index 0741662..85d3b74 100644 --- a/help.h +++ b/help.h @@ -1,7 +1,19 @@ #ifndef HELP_H #define HELP_H +struct cmdnames { + int alloc; + int cnt; + struct cmdname { + size_t len; + char name[1]; + } **names; +}; + int is_git_command(const char *s, const char *prefix); -void list_commands(const char *prefix, const char *title); +void list_commands(const char *prefix, const char *title, struct cmdnames *exclude); +void add_cmdname(struct cmdnames *cmds, const char *name, int len); + +extern struct cmdnames main_cmds, other_cmds; #endif /* HELP_H */ -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() 2008-07-28 1:21 ` [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() Miklos Vajna @ 2008-07-28 1:36 ` Junio C Hamano 2008-07-28 2:43 ` Johannes Schindelin 2008-07-28 23:24 ` Miklos Vajna 0 siblings, 2 replies; 71+ messages in thread From: Junio C Hamano @ 2008-07-28 1:36 UTC (permalink / raw) To: Miklos Vajna; +Cc: git 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? ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() 2008-07-28 1:36 ` Junio C Hamano @ 2008-07-28 2:43 ` Johannes Schindelin 2008-07-28 3:05 ` Junio C Hamano 2008-07-28 23:24 ` Miklos Vajna 1 sibling, 1 reply; 71+ messages in thread From: Johannes Schindelin @ 2008-07-28 2:43 UTC (permalink / raw) To: Junio C Hamano; +Cc: Miklos Vajna, git Hi, On Sun, 27 Jul 2008, Junio C Hamano wrote: > Miklos Vajna <vmiklos@frugalware•org> writes: > > > diff --git a/help.h b/help.h > > index 0741662..85d3b74 100644 > > --- a/help.h > > +++ b/help.h > > @@ -1,7 +1,19 @@ > > #ifndef HELP_H > > #define HELP_H > > > > +struct cmdnames { > > + int alloc; > > + int cnt; > > + struct cmdname { > > + size_t len; > > + char name[1]; > > + } **names; > > +}; > > I thought we do this kind of thing using FLEX_ARRAY macro. Is there any > reason its use is not appropriate here? I think that came up in the previous review round: the "name" member _is_ NUL-terminated, but could have a ".exe" suffix. The "len" member has the length excluding ".exe". Ciao, Dscho ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() 2008-07-28 2:43 ` Johannes Schindelin @ 2008-07-28 3:05 ` Junio C Hamano 2008-07-28 4:29 ` Junio C Hamano 2008-07-28 9:58 ` Johannes Schindelin 0 siblings, 2 replies; 71+ messages in thread From: Junio C Hamano @ 2008-07-28 3:05 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Miklos Vajna, git Johannes Schindelin <Johannes.Schindelin@gmx•de> writes: >> > +struct cmdnames { >> > + int alloc; >> > + int cnt; >> > + struct cmdname { >> > + size_t len; >> > + char name[1]; >> > + } **names; >> > +}; >> >> I thought we do this kind of thing using FLEX_ARRAY macro. Is there any >> reason its use is not appropriate here? > > I think that came up in the previous review round: the "name" member _is_ > NUL-terminated, but could have a ".exe" suffix. The "len" member has the > length excluding ".exe". Sorry, but I do understand what you are trying to explain. Marking the flexible member at the end as "last_member[FLEX_ARRAY]" is about a tiny bit of abstracting out how the exact decl syntax should look like depending on the compiler. For example, we have: struct git_attr { struct git_attr *next; unsigned h; int attr_nr; char name[FLEX_ARRAY]; }; And h is _not_ the length of the name[] (it is a hash to help us find the entry quickly). Jjust like "len" is not the length of the name[] in your structure. In other words, marking as "last_member[FLEX_ARRAY]" does not have anything to do with what other fields you have in the same structure. Hmm, am I missing a bigger picture somewhere else? ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() 2008-07-28 3:05 ` Junio C Hamano @ 2008-07-28 4:29 ` Junio C Hamano 2008-07-28 9:58 ` Johannes Schindelin 1 sibling, 0 replies; 71+ messages in thread From: Junio C Hamano @ 2008-07-28 4:29 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Miklos Vajna, git Junio C Hamano <gitster@pobox•com> writes: > Johannes Schindelin <Johannes.Schindelin@gmx•de> writes: > >>> > +struct cmdnames { >>> > + int alloc; >>> > + int cnt; >>> > + struct cmdname { >>> > + size_t len; >>> > + char name[1]; >>> > + } **names; >>> > +}; >>> >>> I thought we do this kind of thing using FLEX_ARRAY macro. Is there any >>> reason its use is not appropriate here? >> >> I think that came up in the previous review round: the "name" member _is_ >> NUL-terminated, but could have a ".exe" suffix. The "len" member has the >> length excluding ".exe". > > Sorry, but I do understand what you are trying to explain. Yuck, stupid typo; s/do/do not/. ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() 2008-07-28 3:05 ` Junio C Hamano 2008-07-28 4:29 ` Junio C Hamano @ 2008-07-28 9:58 ` Johannes Schindelin 1 sibling, 0 replies; 71+ messages in thread From: Johannes Schindelin @ 2008-07-28 9:58 UTC (permalink / raw) To: Junio C Hamano; +Cc: Miklos Vajna, git Hi, On Sun, 27 Jul 2008, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx•de> writes: > > >> > +struct cmdnames { > >> > + int alloc; > >> > + int cnt; > >> > + struct cmdname { > >> > + size_t len; > >> > + char name[1]; > >> > + } **names; > >> > +}; > >> > >> I thought we do this kind of thing using FLEX_ARRAY macro. Is there any > >> reason its use is not appropriate here? > > > > I think that came up in the previous review round: the "name" member _is_ > > NUL-terminated, but could have a ".exe" suffix. The "len" member has the > > length excluding ".exe". > > Sorry, but I do understand what you are trying to explain. > > Marking the flexible member at the end as "last_member[FLEX_ARRAY]" is > about a tiny bit of abstracting out how the exact decl syntax should look > like depending on the compiler. Ah, sorry, I misunderstood. I thought your complaint was about something else. Ciao, Dscho ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() 2008-07-28 1:36 ` Junio C Hamano 2008-07-28 2:43 ` Johannes Schindelin @ 2008-07-28 23:24 ` Miklos Vajna 1 sibling, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-28 23:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: git The supposed method is to build a list of commands to be excluded using add_cmdname(), then pass the list as the new exclude parameter. If no exclude is needed, NULL should be used. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- On Sun, Jul 27, 2008 at 06:36:30PM -0700, Junio C Hamano <gitster@pobox•com> wrote: > > + struct cmdname { > > + size_t len; > > + char name[1]; > > + } **names; > > +}; > > I thought we do this kind of thing using FLEX_ARRAY macro. Is there > any > reason its use is not appropriate here? Now I'm using it. :-) Note that FLEX_ARRAY would be a drop-in replacement only in case it would be name[0], so I needed - struct cmdname *ent = xmalloc(sizeof(*ent) + len); + struct cmdname *ent = xmalloc(sizeof(*ent) + len + 1); later in add_cmdname(). help.c | 26 +++++++++++--------------- help.h | 14 +++++++++++++- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/help.c b/help.c index 7a42517..858f76a 100644 --- a/help.c +++ b/help.c @@ -9,6 +9,7 @@ #include "common-cmds.h" #include "parse-options.h" #include "run-command.h" +#include "help.h" static struct man_viewer_list { struct man_viewer_list *next; @@ -300,18 +301,11 @@ static inline void mput_char(char c, unsigned int num) putchar(c); } -static struct cmdnames { - int alloc; - int cnt; - struct cmdname { - size_t len; - char name[1]; - } **names; -} main_cmds, other_cmds; +struct cmdnames main_cmds, other_cmds; -static void add_cmdname(struct cmdnames *cmds, const char *name, int len) +void add_cmdname(struct cmdnames *cmds, const char *name, int len) { - struct cmdname *ent = xmalloc(sizeof(*ent) + len); + struct cmdname *ent = xmalloc(sizeof(*ent) + len + 1); ent->len = len; memcpy(ent->name, name, len); @@ -463,7 +457,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds, return longest; } -static unsigned int load_command_list(const char *prefix) +static unsigned int load_command_list(const char *prefix, struct cmdnames *exclude) { unsigned int longest = 0; unsigned int len; @@ -502,13 +496,15 @@ static unsigned int load_command_list(const char *prefix) sizeof(*other_cmds.names), cmdname_compare); uniq(&other_cmds); exclude_cmds(&other_cmds, &main_cmds); + if (exclude) + exclude_cmds(&main_cmds, exclude); return longest; } -void list_commands(const char *prefix, const char *title) +void list_commands(const char *prefix, const char *title, struct cmdnames *exclude) { - unsigned int longest = load_command_list(prefix); + unsigned int longest = load_command_list(prefix, exclude); const char *exec_path = git_exec_path(); if (main_cmds.cnt) { @@ -558,7 +554,7 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s) int is_git_command(const char *s, const char *prefix) { - load_command_list(prefix); + load_command_list(prefix, NULL); return is_in_cmdlist(&main_cmds, s) || is_in_cmdlist(&other_cmds, s); } @@ -704,7 +700,7 @@ int cmd_help(int argc, const char **argv, const char *prefix) if (show_all) { printf("usage: %s\n\n", git_usage_string); - list_commands("git-", "git commands"); + list_commands("git-", "git commands", NULL); printf("%s\n", git_more_info_string); return 0; } diff --git a/help.h b/help.h index 0741662..3eb8cfb 100644 --- a/help.h +++ b/help.h @@ -1,7 +1,19 @@ #ifndef HELP_H #define HELP_H +struct cmdnames { + int alloc; + int cnt; + struct cmdname { + size_t len; + char name[FLEX_ARRAY]; + } **names; +}; + int is_git_command(const char *s, const char *prefix); -void list_commands(const char *prefix, const char *title); +void list_commands(const char *prefix, const char *title, struct cmdnames *exclude); +void add_cmdname(struct cmdnames *cmds, const char *name, int len); + +extern struct cmdnames main_cmds, other_cmds; #endif /* HELP_H */ -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 4/6] builtin-merge: allow using a custom strategy 2008-07-28 1:21 ` [PATCH 0/6] Allow custom merge strategies, take 2 Miklos Vajna ` (2 preceding siblings ...) 2008-07-28 1:21 ` [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() Miklos Vajna @ 2008-07-28 1:21 ` Miklos Vajna 2008-07-28 1:21 ` [PATCH 5/6] builtin-merge: avoid non-strategy git-merge commands in error message Miklos Vajna ` (2 subsequent siblings) 6 siblings, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-28 1:21 UTC (permalink / raw) To: git Allow using a custom strategy, as long as it's named git-merge-foo. The error handling is now done using is_git_command(). The list of available strategies is now shown by list_commands(). Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- builtin-merge.c | 19 ++++++++++++------- 1 files changed, 12 insertions(+), 7 deletions(-) diff --git a/builtin-merge.c b/builtin-merge.c index e78fa18..cdbc692 100644 --- a/builtin-merge.c +++ b/builtin-merge.c @@ -22,6 +22,7 @@ #include "log-tree.h" #include "color.h" #include "rerere.h" +#include "help.h" #define DEFAULT_TWOHEAD (1<<0) #define DEFAULT_OCTOPUS (1<<1) @@ -77,7 +78,7 @@ static int option_parse_message(const struct option *opt, static struct strategy *get_strategy(const char *name) { int i; - struct strbuf err; + struct strategy *ret; if (!name) return NULL; @@ -86,12 +87,16 @@ static struct strategy *get_strategy(const char *name) if (!strcmp(name, all_strategy[i].name)) return &all_strategy[i]; - strbuf_init(&err, 0); - for (i = 0; i < ARRAY_SIZE(all_strategy); i++) - strbuf_addf(&err, " %s", all_strategy[i].name); - fprintf(stderr, "Could not find merge strategy '%s'.\n", name); - fprintf(stderr, "Available strategies are:%s.\n", err.buf); - exit(1); + if (!is_git_command(name, "git-merge-")) { + fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name); + list_commands("git-merge-", "strategies"); + exit(1); + } + + ret = xmalloc(sizeof(struct strategy)); + memset(ret, 0, sizeof(struct strategy)); + ret->name = xstrdup(name); + return ret; } static void append_strategy(struct strategy *s) -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 5/6] builtin-merge: avoid non-strategy git-merge commands in error message 2008-07-28 1:21 ` [PATCH 0/6] Allow custom merge strategies, take 2 Miklos Vajna ` (3 preceding siblings ...) 2008-07-28 1:21 ` [PATCH 4/6] builtin-merge: allow using a custom strategy Miklos Vajna @ 2008-07-28 1:21 ` Miklos Vajna 2008-07-28 13:06 ` Johannes Schindelin 2008-07-28 1:21 ` [PATCH 6/6] Add a new test for using a custom merge strategy Miklos Vajna 2008-07-29 15:24 ` [PATCH 0/5] Allow custom merge strategies, take 3 Miklos Vajna 6 siblings, 1 reply; 71+ messages in thread From: Miklos Vajna @ 2008-07-28 1:21 UTC (permalink / raw) To: git If an invalid strategy is supplied, like -s foobar, then git-merge listed all git-merge-* commands. This is not perfect, since for example git-merge-index is not a valid strategy. These are now removed from the output by scanning the list of main commands; if the git-merge-foo command is listed in the all_strategy list, then it's shown, otherwise excluded. This does not exclude commands somewhere else in the PATH, where custom strategies are expected. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- builtin-merge.c | 15 ++++++++++++++- 1 files changed, 14 insertions(+), 1 deletions(-) diff --git a/builtin-merge.c b/builtin-merge.c index cdbc692..29826b1 100644 --- a/builtin-merge.c +++ b/builtin-merge.c @@ -88,8 +88,21 @@ static struct strategy *get_strategy(const char *name) return &all_strategy[i]; if (!is_git_command(name, "git-merge-")) { + struct cmdnames not_strategies; + + memset(¬_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(¬_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", ¬_strategies); exit(1); } -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 5/6] builtin-merge: avoid non-strategy git-merge commands in error message 2008-07-28 1:21 ` [PATCH 5/6] builtin-merge: avoid non-strategy git-merge commands in error message Miklos Vajna @ 2008-07-28 13:06 ` Johannes Schindelin 2008-07-28 23:48 ` [PATCH] builtin-merge: allow using a custom strategy Miklos Vajna 0 siblings, 1 reply; 71+ messages in thread From: Johannes Schindelin @ 2008-07-28 13:06 UTC (permalink / raw) To: Miklos Vajna; +Cc: git Hi, On Mon, 28 Jul 2008, Miklos Vajna wrote: > - list_commands("git-merge-", "strategies"); > + list_commands("git-merge-", "strategies", ¬_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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH] builtin-merge: allow using a custom strategy 2008-07-28 13:06 ` Johannes Schindelin @ 2008-07-28 23:48 ` Miklos Vajna 2008-07-28 23:59 ` Johannes Schindelin 0 siblings, 1 reply; 71+ messages in thread From: Miklos Vajna @ 2008-07-28 23:48 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git Allow using a custom strategy, as long as it's named git-merge-foo. The error handling is now done using is_git_command(). The list of available strategies is now shown by list_commands(). If an invalid strategy is supplied, like -s foobar, then git-merge would list all git-merge-* commands. This is not perfect, since for example git-merge-index is not a valid strategy. These are removed from the output by scanning the list of main commands; if the git-merge-foo command is listed in the all_strategy list, then it's shown, otherwise excluded. This does not exclude commands somewhere else in the PATH, where custom strategies are expected. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- On Mon, Jul 28, 2008 at 03:06:09PM +0200, Johannes Schindelin <Johannes.Schindelin@gmx•de> wrote: > The change in the signature of list_commands() is not part of this > patch. So at least one of your commits should result in an > uncompileable revision... Right. I just squashed patch 4 and 5, and this solves the problem: patch 3 changes the signature and fixes all usage (in help.c, it's not used outside help.c), then the squashed patch introduces the usage of the new list_commands() in builtin-merge.c. (Also in the 'merge-custom' branch of vmiklos.git on repo.or.cz.) builtin-merge.c | 32 +++++++++++++++++++++++++------- 1 files changed, 25 insertions(+), 7 deletions(-) diff --git a/builtin-merge.c b/builtin-merge.c index e78fa18..29826b1 100644 --- a/builtin-merge.c +++ b/builtin-merge.c @@ -22,6 +22,7 @@ #include "log-tree.h" #include "color.h" #include "rerere.h" +#include "help.h" #define DEFAULT_TWOHEAD (1<<0) #define DEFAULT_OCTOPUS (1<<1) @@ -77,7 +78,7 @@ static int option_parse_message(const struct option *opt, static struct strategy *get_strategy(const char *name) { int i; - struct strbuf err; + struct strategy *ret; if (!name) return NULL; @@ -86,12 +87,29 @@ static struct strategy *get_strategy(const char *name) if (!strcmp(name, all_strategy[i].name)) return &all_strategy[i]; - strbuf_init(&err, 0); - for (i = 0; i < ARRAY_SIZE(all_strategy); i++) - strbuf_addf(&err, " %s", all_strategy[i].name); - fprintf(stderr, "Could not find merge strategy '%s'.\n", name); - fprintf(stderr, "Available strategies are:%s.\n", err.buf); - exit(1); + if (!is_git_command(name, "git-merge-")) { + struct cmdnames not_strategies; + + memset(¬_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(¬_strategies, ent->name, ent->len); + } + fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name); + list_commands("git-merge-", "strategies", ¬_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 ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH] builtin-merge: allow using a custom strategy 2008-07-28 23:48 ` [PATCH] builtin-merge: allow using a custom strategy Miklos Vajna @ 2008-07-28 23:59 ` Johannes Schindelin 0 siblings, 0 replies; 71+ messages in thread From: Johannes Schindelin @ 2008-07-28 23:59 UTC (permalink / raw) To: Miklos Vajna; +Cc: git Hi, On Tue, 29 Jul 2008, Miklos Vajna wrote: > On Mon, Jul 28, 2008 at 03:06:09PM +0200, Johannes Schindelin > <Johannes.Schindelin@gmx•de> wrote: > > > The change in the signature of list_commands() is not part of this > > patch. So at least one of your commits should result in an > > uncompileable revision... > > Right. I just squashed patch 4 and 5, and this solves the problem: patch > 3 changes the signature and fixes all usage (in help.c, it's not used > outside help.c), then the squashed patch introduces the usage of the new > list_commands() in builtin-merge.c. Thanks, Dscho ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 6/6] Add a new test for using a custom merge strategy 2008-07-28 1:21 ` [PATCH 0/6] Allow custom merge strategies, take 2 Miklos Vajna ` (4 preceding siblings ...) 2008-07-28 1:21 ` [PATCH 5/6] builtin-merge: avoid non-strategy git-merge commands in error message Miklos Vajna @ 2008-07-28 1:21 ` Miklos Vajna 2008-07-28 13:12 ` Johannes Schindelin 2008-07-29 15:24 ` [PATCH 0/5] Allow custom merge strategies, take 3 Miklos Vajna 6 siblings, 1 reply; 71+ messages in thread From: Miklos Vajna @ 2008-07-28 1:21 UTC (permalink / raw) To: git Testing is done by creating a simple git-merge-theirs strategy which is the opposite of ours. Using this in real merges is not recommended but it's perfect for our testing needs. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- t/t7606-merge-custom.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 45 insertions(+), 0 deletions(-) create mode 100755 t/t7606-merge-custom.sh diff --git a/t/t7606-merge-custom.sh b/t/t7606-merge-custom.sh new file mode 100755 index 0000000..f295e56 --- /dev/null +++ b/t/t7606-merge-custom.sh @@ -0,0 +1,45 @@ +#!/bin/sh + +test_description='git-merge + +Testing a custom strategy.' + +. ./test-lib.sh + +cat > git-merge-theirs << EOF +#!/bin/sh +eval git read-tree --reset -u \\\$\$# +EOF +chmod +x git-merge-theirs +PATH=.:$PATH +export PATH + +test_expect_success 'setup' ' + echo c0 > c0.c && + git add c0.c && + git commit -m c0 && + git tag c0 && + echo c1 > c1.c && + git add c1.c && + git commit -m c1 && + git tag c1 && + git reset --hard c0 && + echo c2 > c2.c && + git add c2.c && + git commit -m c2 && + git tag c2 +' + +test_expect_success 'merge c2 with a custom strategy' ' + git reset --hard c1 && + git merge -s theirs c2 && + test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" && + test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" && + test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" && + git diff --exit-code && + test -f c0.c && + test ! -f c1.c && + test -f c2.c +' + +test_done -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 6/6] Add a new test for using a custom merge strategy 2008-07-28 1:21 ` [PATCH 6/6] Add a new test for using a custom merge strategy Miklos Vajna @ 2008-07-28 13:12 ` Johannes Schindelin 2008-07-28 23:39 ` Miklos Vajna 0 siblings, 1 reply; 71+ messages in thread From: Johannes Schindelin @ 2008-07-28 13:12 UTC (permalink / raw) To: Miklos Vajna; +Cc: git 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 6/6] Add a new test for using a custom merge strategy 2008-07-28 13:12 ` Johannes Schindelin @ 2008-07-28 23:39 ` Miklos Vajna 2008-07-28 23:54 ` Johannes Schindelin 0 siblings, 1 reply; 71+ messages in thread From: Miklos Vajna @ 2008-07-28 23:39 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git Testing is done by creating a simple git-merge-theirs strategy which just picks the upstream tree. (In other words, this is not the opposite of -s ours.) Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- On Mon, Jul 28, 2008 at 03:12:59PM +0200, Johannes Schindelin <Johannes.Schindelin@gmx•de> wrote: > Note that what was asked for, and what Junio implemented before > deciding > that it would do more harm than good in git.git, is not the same as > what > you provide. Thanks, now I see the difference. The updated commit message is hopefully better. Also I added a check to make sure the upstream and the result tree is the same as well. t/t7606-merge-custom.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 46 insertions(+), 0 deletions(-) create mode 100755 t/t7606-merge-custom.sh diff --git a/t/t7606-merge-custom.sh b/t/t7606-merge-custom.sh new file mode 100755 index 0000000..13e8ff5 --- /dev/null +++ b/t/t7606-merge-custom.sh @@ -0,0 +1,46 @@ +#!/bin/sh + +test_description='git-merge + +Testing a custom strategy.' + +. ./test-lib.sh + +cat > git-merge-theirs << EOF +#!/bin/sh +eval git read-tree --reset -u \\\$\$# +EOF +chmod +x git-merge-theirs +PATH=.:$PATH +export PATH + +test_expect_success 'setup' ' + echo c0 > c0.c && + git add c0.c && + git commit -m c0 && + git tag c0 && + echo c1 > c1.c && + git add c1.c && + git commit -m c1 && + git tag c1 && + git reset --hard c0 && + echo c2 > c2.c && + git add c2.c && + git commit -m c2 && + git tag c2 +' + +test_expect_success 'merge c2 with a custom strategy' ' + git reset --hard c1 && + git merge -s theirs c2 && + test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" && + test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" && + test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" && + test "$(git rev-parse c2^{tree})" = "$(git rev-parse HEAD^{tree})" && + git diff --exit-code && + test -f c0.c && + test ! -f c1.c && + test -f c2.c +' + +test_done -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 6/6] Add a new test for using a custom merge strategy 2008-07-28 23:39 ` Miklos Vajna @ 2008-07-28 23:54 ` Johannes Schindelin 2008-07-28 23:58 ` Miklos Vajna 0 siblings, 1 reply; 71+ messages in thread From: Johannes Schindelin @ 2008-07-28 23:54 UTC (permalink / raw) To: Miklos Vajna; +Cc: git 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 ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 6/6] Add a new test for using a custom merge strategy 2008-07-28 23:54 ` Johannes Schindelin @ 2008-07-28 23:58 ` Miklos Vajna 2008-07-29 0:01 ` Miklos Vajna 0 siblings, 1 reply; 71+ messages in thread From: Miklos Vajna @ 2008-07-28 23:58 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git [-- Attachment #1: Type: text/plain, Size: 540 bytes --] On Tue, Jul 29, 2008 at 01:54:17AM +0200, Johannes Schindelin <Johannes.Schindelin@gmx•de> wrote: > Actually, this _is_ the opposite of -s ours, no? -s ours just takes our > tree, your -s theirs just takes their tree. > > Sorry for the confusion I caused, Aah. :-) I did not read the source of git-merge-ours and based on your description I thought my knowledge / the doc about -s ours was not correct. So I guess my original patch was right, then. Note to self: "take 2" gets messy, time to send a "take 3" soon. ;-) [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 6/6] Add a new test for using a custom merge strategy 2008-07-28 23:58 ` Miklos Vajna @ 2008-07-29 0:01 ` Miklos Vajna 0 siblings, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-29 0:01 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git [-- Attachment #1: Type: text/plain, Size: 204 bytes --] On Tue, Jul 29, 2008 at 01:58:47AM +0200, Miklos Vajna <vmiklos@frugalware•org> wrote: > So I guess my original patch was right, then. s/patch/description/. The check for the same trees are still valid. [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 0/5] Allow custom merge strategies, take 3 2008-07-28 1:21 ` [PATCH 0/6] Allow custom merge strategies, take 2 Miklos Vajna ` (5 preceding siblings ...) 2008-07-28 1:21 ` [PATCH 6/6] Add a new test for using a custom merge strategy Miklos Vajna @ 2008-07-29 15:24 ` Miklos Vajna 2008-07-29 15:24 ` [PATCH 1/5] builtin-help: make is_git_command() usable outside builtin-help Miklos Vajna ` (4 more replies) 6 siblings, 5 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-29 15:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Hi, Changes from 'take 2': - the testcase now makes sure the trees are the same for 'theirs' as well. - patch 4 and 5 are squashed so there is no compile error after applying only some of the patches. (or when bisecting) - using FLEX_ARRAY in help.h - the first two patches are unchanged Miklos Vajna (5): builtin-help: make is_git_command() usable outside builtin-help builtin-help: make list_commands() a bit more generic builtin-help: make it possible to exclude some commands in list_commands() builtin-merge: allow using a custom strategy Add a new test for using a custom merge strategy Makefile | 1 + builtin-merge.c | 32 ++++++++++++++++++++----- help.c | 57 ++++++++++++++++++++++++----------------------- help.h | 19 +++++++++++++++ t/t7606-merge-custom.sh | 46 +++++++++++++++++++++++++++++++++++++ 5 files changed, 120 insertions(+), 35 deletions(-) create mode 100644 help.h create mode 100755 t/t7606-merge-custom.sh ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 1/5] builtin-help: make is_git_command() usable outside builtin-help 2008-07-29 15:24 ` [PATCH 0/5] Allow custom merge strategies, take 3 Miklos Vajna @ 2008-07-29 15:24 ` Miklos Vajna 2008-07-29 15:25 ` [PATCH 2/5] builtin-help: make list_commands() a bit more generic Miklos Vajna ` (3 subsequent siblings) 4 siblings, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-29 15:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Other builtins may want to check if a given command is a valid git command or not as well. Additionally add a new parameter that specifies a custom prefix, so that the "git-" prefix is no longer hardwired. Useful for example to limit the search for "git-merge-*". Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- Makefile | 1 + help.c | 25 ++++++++++++++----------- help.h | 6 ++++++ 3 files changed, 21 insertions(+), 11 deletions(-) create mode 100644 help.h diff --git a/Makefile b/Makefile index 798a2f2..351afd7 100644 --- a/Makefile +++ b/Makefile @@ -355,6 +355,7 @@ LIB_H += git-compat-util.h LIB_H += graph.h LIB_H += grep.h LIB_H += hash.h +LIB_H += help.h LIB_H += list-objects.h LIB_H += ll-merge.h LIB_H += log-tree.h diff --git a/help.c b/help.c index 3cb1962..08188f5 100644 --- a/help.c +++ b/help.c @@ -418,11 +418,11 @@ static int is_executable(const char *name) } static unsigned int list_commands_in_dir(struct cmdnames *cmds, - const char *path) + const char *path, + const char *prefix) { unsigned int longest = 0; - const char *prefix = "git-"; - int prefix_len = strlen(prefix); + int prefix_len; DIR *dir = opendir(path); struct dirent *de; struct strbuf buf = STRBUF_INIT; @@ -430,6 +430,9 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds, if (!dir) return 0; + if (!prefix) + prefix = "git-"; + prefix_len = strlen(prefix); strbuf_addf(&buf, "%s/", path); len = buf.len; @@ -460,7 +463,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds, return longest; } -static unsigned int load_command_list(void) +static unsigned int load_command_list(const char *prefix) { unsigned int longest = 0; unsigned int len; @@ -469,7 +472,7 @@ static unsigned int load_command_list(void) const char *exec_path = git_exec_path(); if (exec_path) - longest = list_commands_in_dir(&main_cmds, exec_path); + longest = list_commands_in_dir(&main_cmds, exec_path, prefix); if (!env_path) { fprintf(stderr, "PATH not set\n"); @@ -481,7 +484,7 @@ static unsigned int load_command_list(void) if ((colon = strchr(path, PATH_SEP))) *colon = 0; - len = list_commands_in_dir(&other_cmds, path); + len = list_commands_in_dir(&other_cmds, path, prefix); if (len > longest) longest = len; @@ -505,7 +508,7 @@ static unsigned int load_command_list(void) static void list_commands(void) { - unsigned int longest = load_command_list(); + unsigned int longest = load_command_list(NULL); const char *exec_path = git_exec_path(); if (main_cmds.cnt) { @@ -551,9 +554,9 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s) return 0; } -static int is_git_command(const char *s) +int is_git_command(const char *s, const char *prefix) { - load_command_list(); + load_command_list(prefix); return is_in_cmdlist(&main_cmds, s) || is_in_cmdlist(&other_cmds, s); } @@ -574,7 +577,7 @@ static const char *cmd_to_page(const char *git_cmd) return "git"; else if (!prefixcmp(git_cmd, "git")) return git_cmd; - else if (is_git_command(git_cmd)) + else if (is_git_command(git_cmd, NULL)) return prepend("git-", git_cmd); else return prepend("git", git_cmd); @@ -712,7 +715,7 @@ int cmd_help(int argc, const char **argv, const char *prefix) } alias = alias_lookup(argv[0]); - if (alias && !is_git_command(argv[0])) { + if (alias && !is_git_command(argv[0], NULL)) { printf("`git %s' is aliased to `%s'\n", argv[0], alias); return 0; } diff --git a/help.h b/help.h new file mode 100644 index 0000000..73da8d6 --- /dev/null +++ b/help.h @@ -0,0 +1,6 @@ +#ifndef HELP_H +#define HELP_H + +int is_git_command(const char *s, const char *prefix); + +#endif /* HELP_H */ -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 2/5] builtin-help: make list_commands() a bit more generic 2008-07-29 15:24 ` [PATCH 0/5] Allow custom merge strategies, take 3 Miklos Vajna 2008-07-29 15:24 ` [PATCH 1/5] builtin-help: make is_git_command() usable outside builtin-help Miklos Vajna @ 2008-07-29 15:25 ` Miklos Vajna 2008-07-29 15:25 ` [PATCH 3/5] builtin-help: make it possible to exclude some commands in list_commands() Miklos Vajna ` (2 subsequent siblings) 4 siblings, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-29 15:25 UTC (permalink / raw) To: Junio C Hamano; +Cc: git That function now takes two paramters to control the prefix of the listed commands, and a second parameter to specify the title of the table. This can be useful for listing not only all git commands, but specific ones, like merge strategies. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- help.c | 18 ++++++++++-------- help.h | 1 + 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/help.c b/help.c index 08188f5..7a42517 100644 --- a/help.c +++ b/help.c @@ -506,23 +506,25 @@ static unsigned int load_command_list(const char *prefix) return longest; } -static void list_commands(void) +void list_commands(const char *prefix, const char *title) { - unsigned int longest = load_command_list(NULL); + unsigned int longest = load_command_list(prefix); const char *exec_path = git_exec_path(); if (main_cmds.cnt) { - printf("available git commands in '%s'\n", exec_path); - printf("----------------------------"); - mput_char('-', strlen(exec_path)); + printf("available %s in '%s'\n", title, exec_path); + printf("----------------"); + mput_char('-', strlen(title) + strlen(exec_path)); putchar('\n'); pretty_print_string_list(&main_cmds, longest); putchar('\n'); } if (other_cmds.cnt) { - printf("git commands available from elsewhere on your $PATH\n"); - printf("---------------------------------------------------\n"); + printf("%s available from elsewhere on your $PATH\n", title); + printf("---------------------------------------"); + mput_char('-', strlen(title)); + putchar('\n'); pretty_print_string_list(&other_cmds, longest); putchar('\n'); } @@ -702,7 +704,7 @@ int cmd_help(int argc, const char **argv, const char *prefix) if (show_all) { printf("usage: %s\n\n", git_usage_string); - list_commands(); + list_commands("git-", "git commands"); printf("%s\n", git_more_info_string); return 0; } diff --git a/help.h b/help.h index 73da8d6..0741662 100644 --- a/help.h +++ b/help.h @@ -2,5 +2,6 @@ #define HELP_H int is_git_command(const char *s, const char *prefix); +void list_commands(const char *prefix, const char *title); #endif /* HELP_H */ -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 3/5] builtin-help: make it possible to exclude some commands in list_commands() 2008-07-29 15:24 ` [PATCH 0/5] Allow custom merge strategies, take 3 Miklos Vajna 2008-07-29 15:24 ` [PATCH 1/5] builtin-help: make is_git_command() usable outside builtin-help Miklos Vajna 2008-07-29 15:25 ` [PATCH 2/5] builtin-help: make list_commands() a bit more generic Miklos Vajna @ 2008-07-29 15:25 ` Miklos Vajna 2008-07-29 19:46 ` Junio C Hamano 2008-07-29 15:25 ` [PATCH 4/5] builtin-merge: allow using a custom strategy Miklos Vajna 2008-07-29 15:25 ` [PATCH 5/5] Add a new test for using a custom merge strategy Miklos Vajna 4 siblings, 1 reply; 71+ messages in thread From: Miklos Vajna @ 2008-07-29 15:25 UTC (permalink / raw) To: Junio C Hamano; +Cc: git The supposed method is to build a list of commands to be excluded using add_cmdname(), then pass the list as the new exclude parameter. If no exclude is needed, NULL should be used. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- help.c | 26 +++++++++++--------------- help.h | 14 +++++++++++++- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/help.c b/help.c index 7a42517..858f76a 100644 --- a/help.c +++ b/help.c @@ -9,6 +9,7 @@ #include "common-cmds.h" #include "parse-options.h" #include "run-command.h" +#include "help.h" static struct man_viewer_list { struct man_viewer_list *next; @@ -300,18 +301,11 @@ static inline void mput_char(char c, unsigned int num) putchar(c); } -static struct cmdnames { - int alloc; - int cnt; - struct cmdname { - size_t len; - char name[1]; - } **names; -} main_cmds, other_cmds; +struct cmdnames main_cmds, other_cmds; -static void add_cmdname(struct cmdnames *cmds, const char *name, int len) +void add_cmdname(struct cmdnames *cmds, const char *name, int len) { - struct cmdname *ent = xmalloc(sizeof(*ent) + len); + struct cmdname *ent = xmalloc(sizeof(*ent) + len + 1); ent->len = len; memcpy(ent->name, name, len); @@ -463,7 +457,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds, return longest; } -static unsigned int load_command_list(const char *prefix) +static unsigned int load_command_list(const char *prefix, struct cmdnames *exclude) { unsigned int longest = 0; unsigned int len; @@ -502,13 +496,15 @@ static unsigned int load_command_list(const char *prefix) sizeof(*other_cmds.names), cmdname_compare); uniq(&other_cmds); exclude_cmds(&other_cmds, &main_cmds); + if (exclude) + exclude_cmds(&main_cmds, exclude); return longest; } -void list_commands(const char *prefix, const char *title) +void list_commands(const char *prefix, const char *title, struct cmdnames *exclude) { - unsigned int longest = load_command_list(prefix); + unsigned int longest = load_command_list(prefix, exclude); const char *exec_path = git_exec_path(); if (main_cmds.cnt) { @@ -558,7 +554,7 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s) int is_git_command(const char *s, const char *prefix) { - load_command_list(prefix); + load_command_list(prefix, NULL); return is_in_cmdlist(&main_cmds, s) || is_in_cmdlist(&other_cmds, s); } @@ -704,7 +700,7 @@ int cmd_help(int argc, const char **argv, const char *prefix) if (show_all) { printf("usage: %s\n\n", git_usage_string); - list_commands("git-", "git commands"); + list_commands("git-", "git commands", NULL); printf("%s\n", git_more_info_string); return 0; } diff --git a/help.h b/help.h index 0741662..3eb8cfb 100644 --- a/help.h +++ b/help.h @@ -1,7 +1,19 @@ #ifndef HELP_H #define HELP_H +struct cmdnames { + int alloc; + int cnt; + struct cmdname { + size_t len; + char name[FLEX_ARRAY]; + } **names; +}; + int is_git_command(const char *s, const char *prefix); -void list_commands(const char *prefix, const char *title); +void list_commands(const char *prefix, const char *title, struct cmdnames *exclude); +void add_cmdname(struct cmdnames *cmds, const char *name, int len); + +extern struct cmdnames main_cmds, other_cmds; #endif /* HELP_H */ -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 3/5] builtin-help: make it possible to exclude some commands in list_commands() 2008-07-29 15:25 ` [PATCH 3/5] builtin-help: make it possible to exclude some commands in list_commands() Miklos Vajna @ 2008-07-29 19:46 ` Junio C Hamano 2008-07-29 21:25 ` Miklos Vajna 0 siblings, 1 reply; 71+ messages in thread From: Junio C Hamano @ 2008-07-29 19:46 UTC (permalink / raw) To: Miklos Vajna; +Cc: git Miklos Vajna <vmiklos@frugalware•org> writes: > The supposed method is to build a list of commands to be excluded using > add_cmdname(), then pass the list as the new exclude parameter. If no > exclude is needed, NULL should be used. You require that exclude is a sorted list; this should be documented somewhere to avoid future misuses. There is no need for adding extra qsort() anywhere in the patchset because the only user you are adding is in the next patch that sends a subset of the commands in main_cmds in the order they are found in the list, and main_cmds is already sorted. ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 3/5] builtin-help: make it possible to exclude some commands in list_commands() 2008-07-29 19:46 ` Junio C Hamano @ 2008-07-29 21:25 ` Miklos Vajna 0 siblings, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-29 21:25 UTC (permalink / raw) To: Junio C Hamano; +Cc: git The supposed method is to build a list of commands to be excluded using add_cmdname(), then pass the list as the new exclude parameter. If no exclude is needed, NULL should be used. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- On Tue, Jul 29, 2008 at 12:46:59PM -0700, Junio C Hamano <gitster@pobox•com> wrote: > You require that exclude is a sorted list; this should be documented > somewhere to avoid future misuses. This version now adds a comment in load_command_list() about this. help.c | 27 ++++++++++++--------------- help.h | 14 +++++++++++++- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/help.c b/help.c index 7a42517..26aa3e0 100644 --- a/help.c +++ b/help.c @@ -9,6 +9,7 @@ #include "common-cmds.h" #include "parse-options.h" #include "run-command.h" +#include "help.h" static struct man_viewer_list { struct man_viewer_list *next; @@ -300,18 +301,11 @@ static inline void mput_char(char c, unsigned int num) putchar(c); } -static struct cmdnames { - int alloc; - int cnt; - struct cmdname { - size_t len; - char name[1]; - } **names; -} main_cmds, other_cmds; +struct cmdnames main_cmds, other_cmds; -static void add_cmdname(struct cmdnames *cmds, const char *name, int len) +void add_cmdname(struct cmdnames *cmds, const char *name, int len) { - struct cmdname *ent = xmalloc(sizeof(*ent) + len); + struct cmdname *ent = xmalloc(sizeof(*ent) + len + 1); ent->len = len; memcpy(ent->name, name, len); @@ -463,7 +457,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds, return longest; } -static unsigned int load_command_list(const char *prefix) +static unsigned int load_command_list(const char *prefix, struct cmdnames *exclude) { unsigned int longest = 0; unsigned int len; @@ -502,13 +496,16 @@ static unsigned int load_command_list(const char *prefix) sizeof(*other_cmds.names), cmdname_compare); uniq(&other_cmds); exclude_cmds(&other_cmds, &main_cmds); + if (exclude) + /* Here we require that exclude is a sorted list. */ + exclude_cmds(&main_cmds, exclude); return longest; } -void list_commands(const char *prefix, const char *title) +void list_commands(const char *prefix, const char *title, struct cmdnames *exclude) { - unsigned int longest = load_command_list(prefix); + unsigned int longest = load_command_list(prefix, exclude); const char *exec_path = git_exec_path(); if (main_cmds.cnt) { @@ -558,7 +555,7 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s) int is_git_command(const char *s, const char *prefix) { - load_command_list(prefix); + load_command_list(prefix, NULL); return is_in_cmdlist(&main_cmds, s) || is_in_cmdlist(&other_cmds, s); } @@ -704,7 +701,7 @@ int cmd_help(int argc, const char **argv, const char *prefix) if (show_all) { printf("usage: %s\n\n", git_usage_string); - list_commands("git-", "git commands"); + list_commands("git-", "git commands", NULL); printf("%s\n", git_more_info_string); return 0; } diff --git a/help.h b/help.h index 0741662..3eb8cfb 100644 --- a/help.h +++ b/help.h @@ -1,7 +1,19 @@ #ifndef HELP_H #define HELP_H +struct cmdnames { + int alloc; + int cnt; + struct cmdname { + size_t len; + char name[FLEX_ARRAY]; + } **names; +}; + int is_git_command(const char *s, const char *prefix); -void list_commands(const char *prefix, const char *title); +void list_commands(const char *prefix, const char *title, struct cmdnames *exclude); +void add_cmdname(struct cmdnames *cmds, const char *name, int len); + +extern struct cmdnames main_cmds, other_cmds; #endif /* HELP_H */ -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 4/5] builtin-merge: allow using a custom strategy 2008-07-29 15:24 ` [PATCH 0/5] Allow custom merge strategies, take 3 Miklos Vajna ` (2 preceding siblings ...) 2008-07-29 15:25 ` [PATCH 3/5] builtin-help: make it possible to exclude some commands in list_commands() Miklos Vajna @ 2008-07-29 15:25 ` Miklos Vajna 2008-07-29 19:47 ` Junio C Hamano 2008-07-29 15:25 ` [PATCH 5/5] Add a new test for using a custom merge strategy Miklos Vajna 4 siblings, 1 reply; 71+ messages in thread From: Miklos Vajna @ 2008-07-29 15:25 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Allow using a custom strategy, as long as it's named git-merge-foo. The error handling is now done using is_git_command(). The list of available strategies is now shown by list_commands(). If an invalid strategy is supplied, like -s foobar, then git-merge would list all git-merge-* commands. This is not perfect, since for example git-merge-index is not a valid strategy. These are removed from the output by scanning the list of main commands; if the git-merge-foo command is listed in the all_strategy list, then it's shown, otherwise excluded. This does not exclude commands somewhere else in the PATH, where custom strategies are expected. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- builtin-merge.c | 32 +++++++++++++++++++++++++------- 1 files changed, 25 insertions(+), 7 deletions(-) diff --git a/builtin-merge.c b/builtin-merge.c index e78fa18..29826b1 100644 --- a/builtin-merge.c +++ b/builtin-merge.c @@ -22,6 +22,7 @@ #include "log-tree.h" #include "color.h" #include "rerere.h" +#include "help.h" #define DEFAULT_TWOHEAD (1<<0) #define DEFAULT_OCTOPUS (1<<1) @@ -77,7 +78,7 @@ static int option_parse_message(const struct option *opt, static struct strategy *get_strategy(const char *name) { int i; - struct strbuf err; + struct strategy *ret; if (!name) return NULL; @@ -86,12 +87,29 @@ static struct strategy *get_strategy(const char *name) if (!strcmp(name, all_strategy[i].name)) return &all_strategy[i]; - strbuf_init(&err, 0); - for (i = 0; i < ARRAY_SIZE(all_strategy); i++) - strbuf_addf(&err, " %s", all_strategy[i].name); - fprintf(stderr, "Could not find merge strategy '%s'.\n", name); - fprintf(stderr, "Available strategies are:%s.\n", err.buf); - exit(1); + if (!is_git_command(name, "git-merge-")) { + struct cmdnames not_strategies; + + memset(¬_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(¬_strategies, ent->name, ent->len); + } + fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name); + list_commands("git-merge-", "strategies", ¬_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 ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 4/5] builtin-merge: allow using a custom strategy 2008-07-29 15:25 ` [PATCH 4/5] builtin-merge: allow using a custom strategy Miklos Vajna @ 2008-07-29 19:47 ` Junio C Hamano 2008-07-29 23:16 ` [PATCH 0/4] Allow custom merge strategies, take 4 Miklos Vajna 0 siblings, 1 reply; 71+ messages in thread From: Junio C Hamano @ 2008-07-29 19:47 UTC (permalink / raw) To: Miklos Vajna; +Cc: git Miklos Vajna <vmiklos@frugalware•org> writes: > Allow using a custom strategy, as long as it's named git-merge-foo. The > error handling is now done using is_git_command(). The list of available > strategies is now shown by list_commands(). > > If an invalid strategy is supplied, like -s foobar, then git-merge would > list all git-merge-* commands. This is not perfect, since for example > git-merge-index is not a valid strategy. > ... > + if (!is_git_command(name, "git-merge-")) { > + struct cmdnames not_strategies; > + > + memset(¬_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(¬_strategies, ent->name, ent->len); > + } This feels overly wasteful. Granted, this is not a performance critical codepath, but you list all commands that begin with "git-merge-" in is_git_command(), only to discard it and then iterate over 140+ main_cmds list only to cull the ones whose name do not appear in the strategies list. Perhaps this shows that changing the function is_git_command() is a wrong approach (for one thing, with the custom prefix, it is not about "Is it a git command" anymore). Wouldn't it be easier to read if you did this part like this instead? * make load_command_list capable of loading into a "struct cmdnames" (or pair if you want) supplied by the caller; * use it to grab all commands whose name begin with "git-merge-" here; * Check if name appears in that list; if it doesn't, you already have the list of commands that could be merge strategy for error reporting. If you also update list_commands() not to run load_command_list() itself but take caller-supplied list of commands, then the API would become much cleaner. The caller would not be limited to "filter with prefix" anymore. Hmm? ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 0/4] Allow custom merge strategies, take 4 2008-07-29 19:47 ` Junio C Hamano @ 2008-07-29 23:16 ` Miklos Vajna 2008-07-29 23:16 ` [PATCH 1/4] builtin-help: make some internal functions available to other builtins Miklos Vajna ` (3 more replies) 0 siblings, 4 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-29 23:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Tue, Jul 29, 2008 at 12:47:05PM -0700, Junio C Hamano <gitster@pobox•com> wrote: > This feels overly wasteful. Granted, this is not a performance critical > codepath, but you list all commands that begin with "git-merge-" in > is_git_command(), only to discard it and then iterate over 140+ main_cmds > list only to cull the ones whose name do not appear in the strategies > list. > > Perhaps this shows that changing the function is_git_command() is a wrong > approach (for one thing, with the custom prefix, it is not about "Is it a > git command" anymore). Wouldn't it be easier to read if you did this part > like this instead? > > * make load_command_list capable of loading into a "struct cmdnames" (or > pair if you want) supplied by the caller; > > * use it to grab all commands whose name begin with "git-merge-" here; > > * Check if name appears in that list; if it doesn't, you already have the > list of commands that could be merge strategy for error reporting. > > If you also update list_commands() not to run load_command_list() itself > but take caller-supplied list of commands, then the API would become much > cleaner. The caller would not be limited to "filter with prefix" anymore. > > Hmm? I like the idea. ;-) Here is what I did: - I changed only load_command_list() and list_commands(). load_command_list() still supports filtering, but the result is loaded to a pair of "struct cmdnames", supplied by the caller. - list_commands() works from this pair, additionally supporting the custom title. - export add_cmdname(), exclude_cmds() and is_in_cmdlist() without any modification in help.h The rest is done in builtin-merge.c, as you suggested. The nice thing is that before this change builtin-merge accepted for example '-s index' (because git-merge-index was a valid command), but now this is no longer true. Miklos Vajna (4): builtin-help: make some internal functions available to other builtins builtin-merge: allow using a custom strategy Add a new test for using a custom merge strategy Add a second testcase for handling invalid strategies in git-merge Makefile | 1 + builtin-merge.c | 42 +++++++++++++++++++++---- help.c | 77 ++++++++++++++++++++++++----------------------- help.h | 23 ++++++++++++++ t/t7600-merge.sh | 5 +++ t/t7606-merge-custom.sh | 46 ++++++++++++++++++++++++++++ 6 files changed, 149 insertions(+), 45 deletions(-) create mode 100644 help.h create mode 100755 t/t7606-merge-custom.sh ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 1/4] builtin-help: make some internal functions available to other builtins 2008-07-29 23:16 ` [PATCH 0/4] Allow custom merge strategies, take 4 Miklos Vajna @ 2008-07-29 23:16 ` Miklos Vajna 2008-07-30 6:21 ` Junio C Hamano 2008-07-29 23:16 ` [PATCH 2/4] builtin-merge: allow using a custom strategy Miklos Vajna ` (2 subsequent siblings) 3 siblings, 1 reply; 71+ messages in thread From: Miklos Vajna @ 2008-07-29 23:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Make load_command_list() capable of filtering for a given prefix and loading into a pair of "struct cmdnames" supplied by the caller. Make the static add_cmdname(), exclude_cmds() and is_in_cmdlist() functions non-static. Make list_commands() accept a custom title, and work from a pair of "struct cmdnames" supplied by the caller. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- Makefile | 1 + help.c | 77 +++++++++++++++++++++++++++++++------------------------------ help.h | 23 ++++++++++++++++++ 3 files changed, 63 insertions(+), 38 deletions(-) create mode 100644 help.h diff --git a/Makefile b/Makefile index 52c67c1..83d79af 100644 --- a/Makefile +++ b/Makefile @@ -355,6 +355,7 @@ LIB_H += git-compat-util.h LIB_H += graph.h LIB_H += grep.h LIB_H += hash.h +LIB_H += help.h LIB_H += list-objects.h LIB_H += ll-merge.h LIB_H += log-tree.h diff --git a/help.c b/help.c index 3cb1962..88c0d5b 100644 --- a/help.c +++ b/help.c @@ -9,6 +9,7 @@ #include "common-cmds.h" #include "parse-options.h" #include "run-command.h" +#include "help.h" static struct man_viewer_list { struct man_viewer_list *next; @@ -300,18 +301,11 @@ static inline void mput_char(char c, unsigned int num) putchar(c); } -static struct cmdnames { - int alloc; - int cnt; - struct cmdname { - size_t len; - char name[1]; - } **names; -} main_cmds, other_cmds; +struct cmdnames main_cmds, other_cmds; -static void add_cmdname(struct cmdnames *cmds, const char *name, int len) +void add_cmdname(struct cmdnames *cmds, const char *name, int len) { - struct cmdname *ent = xmalloc(sizeof(*ent) + len); + struct cmdname *ent = xmalloc(sizeof(*ent) + len + 1); ent->len = len; memcpy(ent->name, name, len); @@ -342,7 +336,7 @@ static void uniq(struct cmdnames *cmds) cmds->cnt = j; } -static void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes) +void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes) { int ci, cj, ei; int cmp; @@ -418,11 +412,11 @@ static int is_executable(const char *name) } static unsigned int list_commands_in_dir(struct cmdnames *cmds, - const char *path) + const char *path, + const char *prefix) { unsigned int longest = 0; - const char *prefix = "git-"; - int prefix_len = strlen(prefix); + int prefix_len; DIR *dir = opendir(path); struct dirent *de; struct strbuf buf = STRBUF_INIT; @@ -430,6 +424,9 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds, if (!dir) return 0; + if (!prefix) + prefix = "git-"; + prefix_len = strlen(prefix); strbuf_addf(&buf, "%s/", path); len = buf.len; @@ -460,7 +457,9 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds, return longest; } -static unsigned int load_command_list(void) +unsigned int load_command_list(const char *prefix, + struct cmdnames *main_cmds, + struct cmdnames *other_cmds) { unsigned int longest = 0; unsigned int len; @@ -469,7 +468,7 @@ static unsigned int load_command_list(void) const char *exec_path = git_exec_path(); if (exec_path) - longest = list_commands_in_dir(&main_cmds, exec_path); + longest = list_commands_in_dir(main_cmds, exec_path, prefix); if (!env_path) { fprintf(stderr, "PATH not set\n"); @@ -481,7 +480,7 @@ static unsigned int load_command_list(void) if ((colon = strchr(path, PATH_SEP))) *colon = 0; - len = list_commands_in_dir(&other_cmds, path); + len = list_commands_in_dir(other_cmds, path, prefix); if (len > longest) longest = len; @@ -491,36 +490,38 @@ static unsigned int load_command_list(void) } free(paths); - qsort(main_cmds.names, main_cmds.cnt, - sizeof(*main_cmds.names), cmdname_compare); - uniq(&main_cmds); + qsort(main_cmds->names, main_cmds->cnt, + sizeof(*main_cmds->names), cmdname_compare); + uniq(main_cmds); - qsort(other_cmds.names, other_cmds.cnt, - sizeof(*other_cmds.names), cmdname_compare); - uniq(&other_cmds); - exclude_cmds(&other_cmds, &main_cmds); + qsort(other_cmds->names, other_cmds->cnt, + sizeof(*other_cmds->names), cmdname_compare); + uniq(other_cmds); + exclude_cmds(other_cmds, main_cmds); return longest; } -static void list_commands(void) +void list_commands(const char *title, unsigned int longest, + struct cmdnames *main_cmds, struct cmdnames *other_cmds) { - unsigned int longest = load_command_list(); const char *exec_path = git_exec_path(); - if (main_cmds.cnt) { - printf("available git commands in '%s'\n", exec_path); - printf("----------------------------"); - mput_char('-', strlen(exec_path)); + if (main_cmds->cnt) { + printf("available %s in '%s'\n", title, exec_path); + printf("----------------"); + mput_char('-', strlen(title) + strlen(exec_path)); putchar('\n'); - pretty_print_string_list(&main_cmds, longest); + pretty_print_string_list(main_cmds, longest); putchar('\n'); } - if (other_cmds.cnt) { - printf("git commands available from elsewhere on your $PATH\n"); - printf("---------------------------------------------------\n"); - pretty_print_string_list(&other_cmds, longest); + if (other_cmds->cnt) { + printf("%s available from elsewhere on your $PATH\n", title); + printf("---------------------------------------"); + mput_char('-', strlen(title)); + putchar('\n'); + pretty_print_string_list(other_cmds, longest); putchar('\n'); } } @@ -542,7 +543,7 @@ void list_common_cmds_help(void) } } -static int is_in_cmdlist(struct cmdnames *c, const char *s) +int is_in_cmdlist(struct cmdnames *c, const char *s) { int i; for (i = 0; i < c->cnt; i++) @@ -553,7 +554,6 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s) static int is_git_command(const char *s) { - load_command_list(); return is_in_cmdlist(&main_cmds, s) || is_in_cmdlist(&other_cmds, s); } @@ -698,8 +698,9 @@ int cmd_help(int argc, const char **argv, const char *prefix) builtin_help_usage, 0); if (show_all) { + unsigned int longest = load_command_list("git-", &main_cmds, &other_cmds); printf("usage: %s\n\n", git_usage_string); - list_commands(); + list_commands("git commands", longest, &main_cmds, &other_cmds); printf("%s\n", git_more_info_string); return 0; } diff --git a/help.h b/help.h new file mode 100644 index 0000000..d614e54 --- /dev/null +++ b/help.h @@ -0,0 +1,23 @@ +#ifndef HELP_H +#define HELP_H + +struct cmdnames { + int alloc; + int cnt; + struct cmdname { + size_t len; + char name[FLEX_ARRAY]; + } **names; +}; + +unsigned int load_command_list(const char *prefix, + struct cmdnames *main_cmds, + struct cmdnames *other_cmds); +void add_cmdname(struct cmdnames *cmds, const char *name, int len); +/* Here we require that excludes is a sorted list. */ +void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes); +int is_in_cmdlist(struct cmdnames *c, const char *s); +void list_commands(const char *title, unsigned int longest, + struct cmdnames *main_cmds, struct cmdnames *other_cmds); + +#endif /* HELP_H */ -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 1/4] builtin-help: make some internal functions available to other builtins 2008-07-29 23:16 ` [PATCH 1/4] builtin-help: make some internal functions available to other builtins Miklos Vajna @ 2008-07-30 6:21 ` Junio C Hamano 0 siblings, 0 replies; 71+ messages in thread From: Junio C Hamano @ 2008-07-30 6:21 UTC (permalink / raw) To: Miklos Vajna; +Cc: git Thanks; all four patches queued. ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 2/4] builtin-merge: allow using a custom strategy 2008-07-29 23:16 ` [PATCH 0/4] Allow custom merge strategies, take 4 Miklos Vajna 2008-07-29 23:16 ` [PATCH 1/4] builtin-help: make some internal functions available to other builtins Miklos Vajna @ 2008-07-29 23:16 ` Miklos Vajna 2008-07-29 23:17 ` [PATCH 3/4] Add a new test for using a custom merge strategy Miklos Vajna 2008-07-29 23:17 ` [PATCH 4/4] Add a second testcase for handling invalid strategies in git-merge Miklos Vajna 3 siblings, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-29 23:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Allow using a custom strategy, as long as it's named git-merge-foo. The error handling is now done using is_git_command(). The list of available strategies is now shown by list_commands(). If an invalid strategy is supplied, like -s foobar, then git-merge would list all git-merge-* commands. This is not perfect, since for example git-merge-index is not a valid strategy. These are removed from the output by scanning the list of main commands; if the git-merge-foo command is listed in the all_strategy list, then it's shown, otherwise excluded. This does not exclude commands somewhere else in the PATH, where custom strategies are expected. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- builtin-merge.c | 42 +++++++++++++++++++++++++++++++++++------- 1 files changed, 35 insertions(+), 7 deletions(-) diff --git a/builtin-merge.c b/builtin-merge.c index e78fa18..99b307a 100644 --- a/builtin-merge.c +++ b/builtin-merge.c @@ -22,6 +22,7 @@ #include "log-tree.h" #include "color.h" #include "rerere.h" +#include "help.h" #define DEFAULT_TWOHEAD (1<<0) #define DEFAULT_OCTOPUS (1<<1) @@ -77,7 +78,9 @@ static int option_parse_message(const struct option *opt, static struct strategy *get_strategy(const char *name) { int i; - struct strbuf err; + struct strategy *ret; + static struct cmdnames main_cmds, other_cmds; + static int longest = 0; if (!name) return NULL; @@ -86,12 +89,37 @@ static struct strategy *get_strategy(const char *name) if (!strcmp(name, all_strategy[i].name)) return &all_strategy[i]; - strbuf_init(&err, 0); - for (i = 0; i < ARRAY_SIZE(all_strategy); i++) - strbuf_addf(&err, " %s", all_strategy[i].name); - fprintf(stderr, "Could not find merge strategy '%s'.\n", name); - fprintf(stderr, "Available strategies are:%s.\n", err.buf); - exit(1); + if (!longest) { + struct cmdnames not_strategies; + + memset(&main_cmds, 0, sizeof(struct cmdnames)); + memset(&other_cmds, 0, sizeof(struct cmdnames)); + memset(¬_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(¬_strategies, ent->name, ent->len); + exclude_cmds(&main_cmds, ¬_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 ^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 3/4] Add a new test for using a custom merge strategy 2008-07-29 23:16 ` [PATCH 0/4] Allow custom merge strategies, take 4 Miklos Vajna 2008-07-29 23:16 ` [PATCH 1/4] builtin-help: make some internal functions available to other builtins Miklos Vajna 2008-07-29 23:16 ` [PATCH 2/4] builtin-merge: allow using a custom strategy Miklos Vajna @ 2008-07-29 23:17 ` Miklos Vajna 2008-07-29 23:43 ` Junio C Hamano 2008-07-29 23:17 ` [PATCH 4/4] Add a second testcase for handling invalid strategies in git-merge Miklos Vajna 3 siblings, 1 reply; 71+ messages in thread From: Miklos Vajna @ 2008-07-29 23:17 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Testing is done by creating a simple git-merge-theirs strategy which is the opposite of ours. Using this in real merges is not recommended but it's perfect for our testing needs. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- t/t7606-merge-custom.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 46 insertions(+), 0 deletions(-) create mode 100755 t/t7606-merge-custom.sh diff --git a/t/t7606-merge-custom.sh b/t/t7606-merge-custom.sh new file mode 100755 index 0000000..13e8ff5 --- /dev/null +++ b/t/t7606-merge-custom.sh @@ -0,0 +1,46 @@ +#!/bin/sh + +test_description='git-merge + +Testing a custom strategy.' + +. ./test-lib.sh + +cat > git-merge-theirs << EOF +#!/bin/sh +eval git read-tree --reset -u \\\$\$# +EOF +chmod +x git-merge-theirs +PATH=.:$PATH +export PATH + +test_expect_success 'setup' ' + echo c0 > c0.c && + git add c0.c && + git commit -m c0 && + git tag c0 && + echo c1 > c1.c && + git add c1.c && + git commit -m c1 && + git tag c1 && + git reset --hard c0 && + echo c2 > c2.c && + git add c2.c && + git commit -m c2 && + git tag c2 +' + +test_expect_success 'merge c2 with a custom strategy' ' + git reset --hard c1 && + git merge -s theirs c2 && + test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" && + test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" && + test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" && + test "$(git rev-parse c2^{tree})" = "$(git rev-parse HEAD^{tree})" && + git diff --exit-code && + test -f c0.c && + test ! -f c1.c && + test -f c2.c +' + +test_done -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 3/4] Add a new test for using a custom merge strategy 2008-07-29 23:17 ` [PATCH 3/4] Add a new test for using a custom merge strategy Miklos Vajna @ 2008-07-29 23:43 ` Junio C Hamano 2008-07-30 17:25 ` Miklos Vajna 0 siblings, 1 reply; 71+ messages in thread From: Junio C Hamano @ 2008-07-29 23:43 UTC (permalink / raw) To: Miklos Vajna; +Cc: git 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 ;-) ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 3/4] Add a new test for using a custom merge strategy 2008-07-29 23:43 ` Junio C Hamano @ 2008-07-30 17:25 ` Miklos Vajna 0 siblings, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-30 17:25 UTC (permalink / raw) To: Junio C Hamano; +Cc: git [-- Attachment #1: Type: text/plain, Size: 598 bytes --] On Tue, Jul 29, 2008 at 04:43:53PM -0700, Junio C Hamano <gitster@pobox•com> wrote: > By the way, this eval shows why "theirs" cannot be a symmetric operation > of "ours". You are taking the last remote HEAD even when you are merging > more than one remote into the current branch at once. "ours" can be > sensibly defined for an octopus, but "theirs" has this "which theirs" > problem ;-) Oh, well, sure. But _if_ it turns out there is a demand for that kind of git-merge-theirs, then I suppose we could still give up if we are given two or more remotes, just like merge-resolve and others do. [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 4/4] Add a second testcase for handling invalid strategies in git-merge 2008-07-29 23:16 ` [PATCH 0/4] Allow custom merge strategies, take 4 Miklos Vajna ` (2 preceding siblings ...) 2008-07-29 23:17 ` [PATCH 3/4] Add a new test for using a custom merge strategy Miklos Vajna @ 2008-07-29 23:17 ` Miklos Vajna 2008-07-29 23:42 ` Junio C Hamano 3 siblings, 1 reply; 71+ messages in thread From: Miklos Vajna @ 2008-07-29 23:17 UTC (permalink / raw) To: Junio C Hamano; +Cc: git This one tests '-s index' which is interesting because git-merge-index is an existing git command but it is not a valid strategy. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- t/t7600-merge.sh | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh index 5eeb6c2..0329aee 100755 --- a/t/t7600-merge.sh +++ b/t/t7600-merge.sh @@ -230,6 +230,10 @@ test_expect_success 'test option parsing' ' test_must_fail git merge ' +test_expect_success 'reject non-strategy with a git-merge-foo name' ' + test_must_fail git merge -s index c1 +' + test_expect_success 'merge c0 with c1' ' git reset --hard c0 && git merge c1 && -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH 4/4] Add a second testcase for handling invalid strategies in git-merge 2008-07-29 23:17 ` [PATCH 4/4] Add a second testcase for handling invalid strategies in git-merge Miklos Vajna @ 2008-07-29 23:42 ` Junio C Hamano 2008-07-30 17:27 ` Miklos Vajna 0 siblings, 1 reply; 71+ messages in thread From: Junio C Hamano @ 2008-07-29 23:42 UTC (permalink / raw) To: Miklos Vajna; +Cc: git Miklos Vajna <vmiklos@frugalware•org> writes: > This one tests '-s index' which is interesting because git-merge-index > is an existing git command but it is not a valid strategy. > > Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> > --- > t/t7600-merge.sh | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh > index 5eeb6c2..0329aee 100755 > --- a/t/t7600-merge.sh > +++ b/t/t7600-merge.sh > @@ -230,6 +230,10 @@ test_expect_success 'test option parsing' ' > test_must_fail git merge > ' > > +test_expect_success 'reject non-strategy with a git-merge-foo name' ' > + test_must_fail git merge -s index c1 > +' > + True, but with the old code that might blindly have executed git-merge-index the test would also have failed, and you would want to tell two cases apart, wouldn't you? ^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 4/4] Add a second testcase for handling invalid strategies in git-merge 2008-07-29 23:42 ` Junio C Hamano @ 2008-07-30 17:27 ` Miklos Vajna 0 siblings, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-30 17:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: git [-- Attachment #1: Type: text/plain, Size: 384 bytes --] On Tue, Jul 29, 2008 at 04:42:53PM -0700, Junio C Hamano <gitster@pobox•com> wrote: > True, but with the old code that might blindly have executed > git-merge-index the test would also have failed, and you would want to > tell two cases apart, wouldn't you? Hmm, I wonder what is the right approach to test it. Should I exit with a different error code with the strategy is invalid? [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 5/5] Add a new test for using a custom merge strategy 2008-07-29 15:24 ` [PATCH 0/5] Allow custom merge strategies, take 3 Miklos Vajna ` (3 preceding siblings ...) 2008-07-29 15:25 ` [PATCH 4/5] builtin-merge: allow using a custom strategy Miklos Vajna @ 2008-07-29 15:25 ` Miklos Vajna 4 siblings, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-29 15:25 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Testing is done by creating a simple git-merge-theirs strategy which is the opposite of ours. Using this in real merges is not recommended but it's perfect for our testing needs. Signed-off-by: Miklos Vajna <vmiklos@frugalware•org> --- t/t7606-merge-custom.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 46 insertions(+), 0 deletions(-) create mode 100755 t/t7606-merge-custom.sh diff --git a/t/t7606-merge-custom.sh b/t/t7606-merge-custom.sh new file mode 100755 index 0000000..13e8ff5 --- /dev/null +++ b/t/t7606-merge-custom.sh @@ -0,0 +1,46 @@ +#!/bin/sh + +test_description='git-merge + +Testing a custom strategy.' + +. ./test-lib.sh + +cat > git-merge-theirs << EOF +#!/bin/sh +eval git read-tree --reset -u \\\$\$# +EOF +chmod +x git-merge-theirs +PATH=.:$PATH +export PATH + +test_expect_success 'setup' ' + echo c0 > c0.c && + git add c0.c && + git commit -m c0 && + git tag c0 && + echo c1 > c1.c && + git add c1.c && + git commit -m c1 && + git tag c1 && + git reset --hard c0 && + echo c2 > c2.c && + git add c2.c && + git commit -m c2 && + git tag c2 +' + +test_expect_success 'merge c2 with a custom strategy' ' + git reset --hard c1 && + git merge -s theirs c2 && + test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" && + test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" && + test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" && + test "$(git rev-parse c2^{tree})" = "$(git rev-parse HEAD^{tree})" && + git diff --exit-code && + test -f c0.c && + test ! -f c1.c && + test -f c2.c +' + +test_done -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 71+ messages in thread
* Re: [PATCH] t7601: extend the 'merge picks up the best result' test 2008-07-26 11:54 ` Miklos Vajna 2008-07-26 11:54 ` [PATCH 0/7] Allow custom merge strategies Miklos Vajna @ 2008-07-26 12:33 ` Miklos Vajna 1 sibling, 0 replies; 71+ messages in thread From: Miklos Vajna @ 2008-07-26 12:33 UTC (permalink / raw) To: git [-- Attachment #1: Type: text/plain, Size: 454 bytes --] On Sat, Jul 26, 2008 at 01:54:45PM +0200, Miklos Vajna <vmiklos@frugalware•org> wrote: > The test only checked if the best result picking code works if there are > multiple strategies set in the config. Add a similar one that tests if > the same true if the -s option of git merge was used multiple times. Ignore this one. I run git send-email *.patch when sending out the other series and forgot to do an rm *.patch before format-patch. Sorry, Miklos [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 71+ messages in thread
end of thread, other threads:[~2008-07-30 17:28 UTC | newest] Thread overview: 71+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-07-21 16:10 [PATCH] builtin-merge: give a proper error message for invalid strategies in config Miklos Vajna 2008-07-22 5:01 ` Junio C Hamano 2008-07-22 5:22 ` Junio C Hamano 2008-07-22 7:39 ` Miklos Vajna 2008-07-22 8:24 ` Junio C Hamano 2008-07-22 17:05 ` [PATCH] t7601: extend the 'merge picks up the best result' test Miklos Vajna 2008-07-26 11:54 ` Miklos Vajna 2008-07-26 11:54 ` [PATCH 0/7] Allow custom merge strategies Miklos Vajna 2008-07-26 11:54 ` [PATCH 1/7] Make is_git_command() usable outside builtin-help Miklos Vajna 2008-07-26 18:12 ` Jonathan Nieder 2008-07-28 1:18 ` Miklos Vajna 2008-07-26 11:54 ` [PATCH 2/7] builtin-help: change the current directory back in list_commands_in_dir() Miklos Vajna 2008-07-26 14:58 ` Johannes Schindelin 2008-07-27 20:02 ` Junio C Hamano 2008-07-27 20:21 ` Johannes Schindelin 2008-07-27 20:34 ` [PATCH] Avoid chdir() " Johannes Schindelin 2008-07-26 11:54 ` [PATCH 3/7] builtin-help: make list_commands() a bit more generic Miklos Vajna 2008-07-26 18:28 ` Jonathan Nieder 2008-07-26 21:40 ` Miklos Vajna 2008-07-26 11:54 ` [PATCH 4/7] builtin-merge: allow using a custom strategy Miklos Vajna 2008-07-26 11:54 ` [PATCH 5/7] Add a new test for using a custom merge strategy Miklos Vajna 2008-07-26 11:54 ` [PATCH 6/7] builtin-help: make it possible to exclude some commands in list_commands() Miklos Vajna 2008-07-26 11:54 ` [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message Miklos Vajna 2008-07-26 15:08 ` Johannes Schindelin 2008-07-26 15:25 ` Miklos Vajna 2008-07-26 15:27 ` Miklos Vajna 2008-07-26 15:38 ` Johannes Schindelin 2008-07-26 16:00 ` Miklos Vajna 2008-07-26 17:01 ` Johannes Schindelin 2008-07-26 17:12 ` [PATCH] " Miklos Vajna 2008-07-28 1:21 ` [PATCH 0/6] Allow custom merge strategies, take 2 Miklos Vajna 2008-07-28 1:21 ` [PATCH 1/6] builtin-help: make is_git_command() usable outside builtin-help Miklos Vajna 2008-07-28 1:21 ` [PATCH 2/6] builtin-help: make list_commands() a bit more generic Miklos Vajna 2008-07-28 1:21 ` [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() Miklos Vajna 2008-07-28 1:36 ` Junio C Hamano 2008-07-28 2:43 ` Johannes Schindelin 2008-07-28 3:05 ` Junio C Hamano 2008-07-28 4:29 ` Junio C Hamano 2008-07-28 9:58 ` Johannes Schindelin 2008-07-28 23:24 ` Miklos Vajna 2008-07-28 1:21 ` [PATCH 4/6] builtin-merge: allow using a custom strategy Miklos Vajna 2008-07-28 1:21 ` [PATCH 5/6] builtin-merge: avoid non-strategy git-merge commands in error message Miklos Vajna 2008-07-28 13:06 ` Johannes Schindelin 2008-07-28 23:48 ` [PATCH] builtin-merge: allow using a custom strategy Miklos Vajna 2008-07-28 23:59 ` Johannes Schindelin 2008-07-28 1:21 ` [PATCH 6/6] Add a new test for using a custom merge strategy Miklos Vajna 2008-07-28 13:12 ` Johannes Schindelin 2008-07-28 23:39 ` Miklos Vajna 2008-07-28 23:54 ` Johannes Schindelin 2008-07-28 23:58 ` Miklos Vajna 2008-07-29 0:01 ` Miklos Vajna 2008-07-29 15:24 ` [PATCH 0/5] Allow custom merge strategies, take 3 Miklos Vajna 2008-07-29 15:24 ` [PATCH 1/5] builtin-help: make is_git_command() usable outside builtin-help Miklos Vajna 2008-07-29 15:25 ` [PATCH 2/5] builtin-help: make list_commands() a bit more generic Miklos Vajna 2008-07-29 15:25 ` [PATCH 3/5] builtin-help: make it possible to exclude some commands in list_commands() Miklos Vajna 2008-07-29 19:46 ` Junio C Hamano 2008-07-29 21:25 ` Miklos Vajna 2008-07-29 15:25 ` [PATCH 4/5] builtin-merge: allow using a custom strategy Miklos Vajna 2008-07-29 19:47 ` Junio C Hamano 2008-07-29 23:16 ` [PATCH 0/4] Allow custom merge strategies, take 4 Miklos Vajna 2008-07-29 23:16 ` [PATCH 1/4] builtin-help: make some internal functions available to other builtins Miklos Vajna 2008-07-30 6:21 ` Junio C Hamano 2008-07-29 23:16 ` [PATCH 2/4] builtin-merge: allow using a custom strategy Miklos Vajna 2008-07-29 23:17 ` [PATCH 3/4] Add a new test for using a custom merge strategy Miklos Vajna 2008-07-29 23:43 ` Junio C Hamano 2008-07-30 17:25 ` Miklos Vajna 2008-07-29 23:17 ` [PATCH 4/4] Add a second testcase for handling invalid strategies in git-merge Miklos Vajna 2008-07-29 23:42 ` Junio C Hamano 2008-07-30 17:27 ` Miklos Vajna 2008-07-29 15:25 ` [PATCH 5/5] Add a new test for using a custom merge strategy Miklos Vajna 2008-07-26 12:33 ` [PATCH] t7601: extend the 'merge picks up the best result' test Miklos Vajna
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox