# [PATCH 1/7] Make is_git_command() usable outside builtin-help 46 messages from 2008-07-26 to 2008-07-29. Participants: Miklos Vajna, Johannes Schindelin, Jonathan Nieder, Junio C Hamano. Thread: https://gitlist.dev/t/14690 ## Miklos Vajna, 2008-07-26 11:54 Subject: [PATCH 0/7] Allow custom merge strategies Message-ID: URL: https://gitlist.dev/e/cover.1217037178.git.vmiklos%40frugalware.org ``` Hi, This series adds support for custom merge strategies. The first 3 patches modify builtin-help to allow using it from other builtins. This is necessary because in the error message of 'git merge -s foobar' we show something like 'git help -a', but we list only merge strategies. A command is considered a merge strategy if it has a git-merge- prefix and is listed in the all_strategy array or it is somewhere in PATH, but outside `git --exec-path`, so that git-merge-ours and other strategies are shown, git-merge-index and other git-merge-named (but not strategy) commands are hidden. The last two is about removing those problematic 'git-merge-index', 'git-merge-tree' and other bogus commands from the output of 'git merge -s foobar'. I think the benefit of doing it that way is that we don't have to maintain a list of commands which are named git-merge-foo but not strategies _and_ the custom strategies can have a form of git-merge-foo, without adding extra complexity (like forcing users to name them git-merge-custom-foo). NOTE: At the moment the custom strategies are named as git-merge-foo as well, mainly because I think it's not that problematic to exclude the already existing git-merge-fo non-strategy commands, but this can be changed to git-merge-strategy-foo if we really want so. Also, I'm aware that this is a feature and we are in rc freeze, I just did not want to keep back this series till 1.6.0 is out. Miklos Vajna (7): Make is_git_command() usable outside builtin-help builtin-help: change the current directory back in list_commands_in_dir() builtin-help: make list_commands() a bit more generic builtin-merge: allow using a custom strategy Add a new test for using a custom merge strategy builtin-help: make it possible to exclude some commands in list_commands() builtin-merge: avoid non-strategy git-merge commands in error message Makefile | 1 + builtin-merge.c | 30 +++++++++++++++++++++------ help.c | 50 ++++++++++++++++++++++++---------------------- help.h | 19 +++++++++++++++++ t/t7606-merge-custom.sh | 45 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 114 insertions(+), 31 deletions(-) create mode 100644 help.h create mode 100755 t/t7606-merge-custom.sh ``` ## Miklos Vajna, 2008-07-26 11:54 Subject: [PATCH 1/7] Make is_git_command() usable outside builtin-help Message-ID: URL: https://gitlist.dev/e/f311372167c02868ccf5aa4dc03c97b7f956d855.1217037178.git.vmiklos%40frugalware.org In-Reply-To: ``` 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 --- 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 ``` ## Miklos Vajna, 2008-07-26 11:54 Subject: [PATCH] t7601: extend the 'merge picks up the best result' test Message-ID: <1217073292-27945-1-git-send-email-vmiklos@frugalware.org> URL: https://gitlist.dev/e/1217073292-27945-1-git-send-email-vmiklos%40frugalware.org In-Reply-To: ``` 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 --- On Tue, Jul 22, 2008 at 01:24:14AM -0700, Junio C Hamano 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 ``` ## Miklos Vajna, 2008-07-26 11:54 Subject: [PATCH 2/7] builtin-help: change the current directory back in list_commands_in_dir() Message-ID: <4f2b03391e3f85cf2224f97a2a7765d08707bd73.1217037178.git.vmiklos@frugalware.org> URL: https://gitlist.dev/e/4f2b03391e3f85cf2224f97a2a7765d08707bd73.1217037178.git.vmiklos%40frugalware.org In-Reply-To: <1217073292-27945-1-git-send-email-vmiklos@frugalware.org> ``` 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 --- 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 ``` ## Miklos Vajna, 2008-07-26 11:54 Subject: [PATCH 3/7] builtin-help: make list_commands() a bit more generic Message-ID: <68749731fe8de8b2a9ffc53963291aeab9256b82.1217037178.git.vmiklos@frugalware.org> URL: https://gitlist.dev/e/68749731fe8de8b2a9ffc53963291aeab9256b82.1217037178.git.vmiklos%40frugalware.org In-Reply-To: <4f2b03391e3f85cf2224f97a2a7765d08707bd73.1217037178.git.vmiklos@frugalware.org> ``` 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 --- 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 ``` ## Miklos Vajna, 2008-07-26 11:54 Subject: [PATCH 4/7] builtin-merge: allow using a custom strategy Message-ID: <99c90877657be0ed439a9918f500d6ad132550a2.1217037178.git.vmiklos@frugalware.org> URL: https://gitlist.dev/e/99c90877657be0ed439a9918f500d6ad132550a2.1217037178.git.vmiklos%40frugalware.org In-Reply-To: <68749731fe8de8b2a9ffc53963291aeab9256b82.1217037178.git.vmiklos@frugalware.org> ``` 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 --- 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 ``` ## Miklos Vajna, 2008-07-26 11:54 Subject: [PATCH 5/7] Add a new test for using a custom merge strategy Message-ID: <13f39dd6752a34beeb6ecccb51dbc7546ef87182.1217037178.git.vmiklos@frugalware.org> URL: https://gitlist.dev/e/13f39dd6752a34beeb6ecccb51dbc7546ef87182.1217037178.git.vmiklos%40frugalware.org In-Reply-To: <99c90877657be0ed439a9918f500d6ad132550a2.1217037178.git.vmiklos@frugalware.org> ``` 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 --- 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 ``` ## Miklos Vajna, 2008-07-26 11:54 Subject: [PATCH 6/7] builtin-help: make it possible to exclude some commands in list_commands() Message-ID: URL: https://gitlist.dev/e/fc448aa651ad9e8e9d1cf5556a665b5bd6074628.1217037178.git.vmiklos%40frugalware.org In-Reply-To: <13f39dd6752a34beeb6ecccb51dbc7546ef87182.1217037178.git.vmiklos@frugalware.org> ``` 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 --- 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 ``` ## Miklos Vajna, 2008-07-26 11:54 Subject: [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message Message-ID: <65a569c389352c5d0500b8c44c28e5572352e6ba.1217037178.git.vmiklos@frugalware.org> URL: https://gitlist.dev/e/65a569c389352c5d0500b8c44c28e5572352e6ba.1217037178.git.vmiklos%40frugalware.org In-Reply-To: ``` 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 --- 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 ``` ## Miklos Vajna, 2008-07-26 12:33 Subject: Re: [PATCH] t7601: extend the 'merge picks up the best result' test Message-ID: <20080726123345.GH32057@genesis.frugalware.org> URL: https://gitlist.dev/e/20080726123345.GH32057%40genesis.frugalware.org In-Reply-To: <1217073292-27945-1-git-send-email-vmiklos@frugalware.org> ``` On Sat, Jul 26, 2008 at 01:54:45PM +0200, Miklos Vajna 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 ``` ## Johannes Schindelin, 2008-07-26 14:58 Subject: Re: [PATCH 2/7] builtin-help: change the current directory back in list_commands_in_dir() Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807261657150.26810%40eeepc-johanness In-Reply-To: <4f2b03391e3f85cf2224f97a2a7765d08707bd73.1217037178.git.vmiklos@frugalware.org> ``` 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 ``` ## Johannes Schindelin, 2008-07-26 15:08 Subject: Re: [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807261701520.26810%40eeepc-johanness In-Reply-To: <65a569c389352c5d0500b8c44c28e5572352e6ba.1217037178.git.vmiklos@frugalware.org> ``` 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 ``` ## Miklos Vajna, 2008-07-26 15:25 Subject: Re: [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message Message-ID: <20080726152502.GL32057@genesis.frugalware.org> URL: https://gitlist.dev/e/20080726152502.GL32057%40genesis.frugalware.org In-Reply-To: ``` On Sat, Jul 26, 2008 at 05:08:11PM +0200, Johannes Schindelin 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); ``` ## Miklos Vajna, 2008-07-26 15:27 Subject: [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message Message-ID: <1217086044-28713-1-git-send-email-vmiklos@frugalware.org> URL: https://gitlist.dev/e/1217086044-28713-1-git-send-email-vmiklos%40frugalware.org In-Reply-To: <20080726152502.GL32057@genesis.frugalware.org> ``` 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 --- 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 ``` ## Johannes Schindelin, 2008-07-26 15:38 Subject: Re: [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807261737260.26810%40eeepc-johanness In-Reply-To: <20080726152502.GL32057@genesis.frugalware.org> ``` Hi, On Sat, 26 Jul 2008, Miklos Vajna wrote: > On Sat, Jul 26, 2008 at 05:08:11PM +0200, Johannes Schindelin 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 ``` ## Miklos Vajna, 2008-07-26 16:00 Subject: Re: [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message Message-ID: <20080726160022.GM32057@genesis.frugalware.org> URL: https://gitlist.dev/e/20080726160022.GM32057%40genesis.frugalware.org In-Reply-To: ``` On Sat, Jul 26, 2008 at 05:38:55PM +0200, Johannes Schindelin 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. ``` ## Johannes Schindelin, 2008-07-26 17:01 Subject: Re: [PATCH 7/7] builtin-merge: avoid non-strategy git-merge commands in error message Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807261900180.26810%40eeepc-johanness In-Reply-To: <20080726160022.GM32057@genesis.frugalware.org> ``` Hi, On Sat, 26 Jul 2008, Miklos Vajna wrote: > On Sat, Jul 26, 2008 at 05:38:55PM +0200, Johannes Schindelin wrote: > > > - if (!strcmp(main_cmds.names[i]->name, all_strategy[j].name)) > > > + if (!strncmp(ent->name, all_strategy[j].name, ent->len)) > > > > Oops... that is not what I meant. You'd have to check if > > !all_strategy[j].name[ent->len], too... > > Hmm. So let's say ent->name is "ours.exe", ent->len is set to 4. > > Then !strncmp(ent->name, all_strategy[j].name, ent->len) will be true, > and the command will not be added to the exclude list. What I meant is: if you change the strcmp to strncmp because one of the both strings is not supposed to be NUL terminated, you still want to make sure that one is not a strict prefix of the other. Ciao, Dscho ``` ## Miklos Vajna, 2008-07-26 17:12 Subject: [PATCH] builtin-merge: avoid non-strategy git-merge commands in error message Message-ID: <1217092367-29114-1-git-send-email-vmiklos@frugalware.org> URL: https://gitlist.dev/e/1217092367-29114-1-git-send-email-vmiklos%40frugalware.org In-Reply-To: ``` 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 --- On Sat, Jul 26, 2008 at 07:01:16PM +0200, Johannes Schindelin 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 ``` ## Jonathan Nieder, 2008-07-26 18:12 Subject: Re: [PATCH 1/7] Make is_git_command() usable outside builtin-help Message-ID: URL: https://gitlist.dev/e/Pine.GSO.4.62.0807261301300.15728%40harper.uchicago.edu In-Reply-To: ``` 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. ``` ## Jonathan Nieder, 2008-07-26 18:28 Subject: Re: [PATCH 3/7] builtin-help: make list_commands() a bit more generic Message-ID: URL: https://gitlist.dev/e/Pine.GSO.4.62.0807261319310.16184%40harper.uchicago.edu In-Reply-To: <68749731fe8de8b2a9ffc53963291aeab9256b82.1217037178.git.vmiklos@frugalware.org> ``` 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) ``` ## Miklos Vajna, 2008-07-26 21:40 Subject: [PATCH 3/7] builtin-help: make list_commands() a bit more generic Message-ID: <3db8fbf82ab865b9dab5899f9a7e867ef005ecb6.1217108282.git.vmiklos@frugalware.org> URL: https://gitlist.dev/e/3db8fbf82ab865b9dab5899f9a7e867ef005ecb6.1217108282.git.vmiklos%40frugalware.org In-Reply-To: ``` 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 --- On Sat, Jul 26, 2008 at 01:28:39PM -0500, Jonathan Nieder 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 ``` ## Junio C Hamano, 2008-07-27 20:02 Subject: Re: [PATCH 2/7] builtin-help: change the current directory back in list_commands_in_dir() Message-ID: <7vwsj7yu1q.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwsj7yu1q.fsf%40gitster.siamese.dyndns.org In-Reply-To: <4f2b03391e3f85cf2224f97a2a7765d08707bd73.1217037178.git.vmiklos@frugalware.org> ``` Miklos Vajna writes: > That function used to do a chdir() without switching back to the > original directory. That was not a problem till this function was used > only inside builtin-help, but once other builtins use it as well, this > is a problem, for example when the object database path is relative. Why does it even need to chdir() around to begin with? Doesn't opendir() work just fine with relative path as an input? ``` ## Johannes Schindelin, 2008-07-27 20:21 Subject: Re: [PATCH 2/7] builtin-help: change the current directory back in list_commands_in_dir() Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807272219350.5526%40eeepc-johanness In-Reply-To: <7vwsj7yu1q.fsf@gitster.siamese.dyndns.org> ``` Hi, On Sun, 27 Jul 2008, Junio C Hamano wrote: > Miklos Vajna writes: > > > That function used to do a chdir() without switching back to the > > original directory. That was not a problem till this function was used > > only inside builtin-help, but once other builtins use it as well, this > > is a problem, for example when the object database path is relative. > > Why does it even need to chdir() around to begin with? Doesn't > opendir() work just fine with relative path as an input? It is a consequence of list_commands_in_dir() trying to be cute, and not having to construct the full path for the is_executable() check. Will post a fix in a few minutes. Ciao, Dscho ``` ## Johannes Schindelin, 2008-07-27 20:34 Subject: [PATCH] Avoid chdir() in list_commands_in_dir() Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807272233160.5526%40eeepc-johanness In-Reply-To: ``` 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 --- 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 ``` ## Miklos Vajna, 2008-07-28 01:18 Subject: Re: [PATCH 1/7] Make is_git_command() usable outside builtin-help Message-ID: <20080728011828.GI32057@genesis.frugalware.org> URL: https://gitlist.dev/e/20080728011828.GI32057%40genesis.frugalware.org In-Reply-To: ``` On Sat, Jul 26, 2008 at 01:12:36PM -0500, Jonathan Nieder 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. :-) ``` ## Miklos Vajna, 2008-07-28 01:21 Subject: [PATCH 0/6] Allow custom merge strategies, take 2 Message-ID: URL: https://gitlist.dev/e/cover.1217207602.git.vmiklos%40frugalware.org In-Reply-To: ``` Hi, This is just a resend of my series, on top of Dscho's "Avoid chdir() in list_commands_in_dir()" (f5d600e), including the suggestions I received from Jonathan Nieder and Dscho. Miklos Vajna (6): builtin-help: make is_git_command() usable outside builtin-help builtin-help: make list_commands() a bit more generic builtin-help: make it possible to exclude some commands in list_commands() builtin-merge: allow using a custom strategy builtin-merge: avoid non-strategy git-merge commands in error message Add a new test for using a custom merge strategy Makefile | 1 + builtin-merge.c | 32 +++++++++++++++++++++------ help.c | 55 ++++++++++++++++++++++++----------------------- help.h | 19 ++++++++++++++++ t/t7606-merge-custom.sh | 45 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 118 insertions(+), 34 deletions(-) create mode 100644 help.h create mode 100755 t/t7606-merge-custom.sh ``` ## Miklos Vajna, 2008-07-28 01:21 Subject: [PATCH 1/6] builtin-help: make is_git_command() usable outside builtin-help Message-ID: URL: https://gitlist.dev/e/fd19583955e9cea5b78a465d23bc127a51940048.1217207602.git.vmiklos%40frugalware.org In-Reply-To: ``` 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 --- 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 ``` ## Miklos Vajna, 2008-07-28 01:21 Subject: [PATCH 2/6] builtin-help: make list_commands() a bit more generic Message-ID: <5a003a0e20d0942c946680e4eade8e9d19f0036b.1217207602.git.vmiklos@frugalware.org> URL: https://gitlist.dev/e/5a003a0e20d0942c946680e4eade8e9d19f0036b.1217207602.git.vmiklos%40frugalware.org In-Reply-To: ``` 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 --- 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 ``` ## Miklos Vajna, 2008-07-28 01:21 Subject: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() Message-ID: <9cc2813166c8b20ffb411c3a28ad86665e60033b.1217207602.git.vmiklos@frugalware.org> URL: https://gitlist.dev/e/9cc2813166c8b20ffb411c3a28ad86665e60033b.1217207602.git.vmiklos%40frugalware.org In-Reply-To: <5a003a0e20d0942c946680e4eade8e9d19f0036b.1217207602.git.vmiklos@frugalware.org> ``` 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 --- 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 ``` ## Miklos Vajna, 2008-07-28 01:21 Subject: [PATCH 4/6] builtin-merge: allow using a custom strategy Message-ID: <10c4292de30b5868bd9fb54b557e8dae827f1241.1217207602.git.vmiklos@frugalware.org> URL: https://gitlist.dev/e/10c4292de30b5868bd9fb54b557e8dae827f1241.1217207602.git.vmiklos%40frugalware.org In-Reply-To: <9cc2813166c8b20ffb411c3a28ad86665e60033b.1217207602.git.vmiklos@frugalware.org> ``` 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 --- 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 ``` ## Miklos Vajna, 2008-07-28 01:21 Subject: [PATCH 5/6] builtin-merge: avoid non-strategy git-merge commands in error message Message-ID: <42e8615f6cbd236e40b19f2a754807f08e4b85a6.1217207602.git.vmiklos@frugalware.org> URL: https://gitlist.dev/e/42e8615f6cbd236e40b19f2a754807f08e4b85a6.1217207602.git.vmiklos%40frugalware.org In-Reply-To: <10c4292de30b5868bd9fb54b557e8dae827f1241.1217207602.git.vmiklos@frugalware.org> ``` 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 --- 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 ``` ## Miklos Vajna, 2008-07-28 01:21 Subject: [PATCH 6/6] Add a new test for using a custom merge strategy Message-ID: <03fa2187a72957d98d63ab899b39e9adc2edfe99.1217207602.git.vmiklos@frugalware.org> URL: https://gitlist.dev/e/03fa2187a72957d98d63ab899b39e9adc2edfe99.1217207602.git.vmiklos%40frugalware.org In-Reply-To: <42e8615f6cbd236e40b19f2a754807f08e4b85a6.1217207602.git.vmiklos@frugalware.org> ``` 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 --- 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 ``` ## Junio C Hamano, 2008-07-28 01:36 Subject: Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() Message-ID: <7vr69ex00x.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr69ex00x.fsf%40gitster.siamese.dyndns.org In-Reply-To: <9cc2813166c8b20ffb411c3a28ad86665e60033b.1217207602.git.vmiklos@frugalware.org> ``` Miklos Vajna 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? ``` ## Johannes Schindelin, 2008-07-28 02:43 Subject: Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807280442330.5526%40eeepc-johanness In-Reply-To: <7vr69ex00x.fsf@gitster.siamese.dyndns.org> ``` Hi, On Sun, 27 Jul 2008, Junio C Hamano wrote: > Miklos Vajna writes: > > > diff --git a/help.h b/help.h > > index 0741662..85d3b74 100644 > > --- a/help.h > > +++ b/help.h > > @@ -1,7 +1,19 @@ > > #ifndef HELP_H > > #define HELP_H > > > > +struct cmdnames { > > + int alloc; > > + int cnt; > > + struct cmdname { > > + size_t len; > > + char name[1]; > > + } **names; > > +}; > > I thought we do this kind of thing using FLEX_ARRAY macro. Is there any > reason its use is not appropriate here? I think that came up in the previous review round: the "name" member _is_ NUL-terminated, but could have a ".exe" suffix. The "len" member has the length excluding ".exe". Ciao, Dscho ``` ## Junio C Hamano, 2008-07-28 03:05 Subject: Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() Message-ID: <7vljzmwvww.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vljzmwvww.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin 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? ``` ## Junio C Hamano, 2008-07-28 04:29 Subject: Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() Message-ID: <7vhcaaws18.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vhcaaws18.fsf%40gitster.siamese.dyndns.org In-Reply-To: <7vljzmwvww.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano writes: > Johannes Schindelin writes: > >>> > +struct cmdnames { >>> > + int alloc; >>> > + int cnt; >>> > + struct cmdname { >>> > + size_t len; >>> > + char name[1]; >>> > + } **names; >>> > +}; >>> >>> I thought we do this kind of thing using FLEX_ARRAY macro. Is there any >>> reason its use is not appropriate here? >> >> I think that came up in the previous review round: the "name" member _is_ >> NUL-terminated, but could have a ".exe" suffix. The "len" member has the >> length excluding ".exe". > > Sorry, but I do understand what you are trying to explain. Yuck, stupid typo; s/do/do not/. ``` ## Johannes Schindelin, 2008-07-28 09:58 Subject: Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807281157220.2725%40eeepc-johanness In-Reply-To: <7vljzmwvww.fsf@gitster.siamese.dyndns.org> ``` Hi, On Sun, 27 Jul 2008, Junio C Hamano wrote: > Johannes Schindelin 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 ``` ## Johannes Schindelin, 2008-07-28 13:06 Subject: Re: [PATCH 5/6] builtin-merge: avoid non-strategy git-merge commands in error message Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807281505010.2725%40eeepc-johanness In-Reply-To: <42e8615f6cbd236e40b19f2a754807f08e4b85a6.1217207602.git.vmiklos@frugalware.org> ``` 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 ``` ## Johannes Schindelin, 2008-07-28 13:12 Subject: Re: [PATCH 6/6] Add a new test for using a custom merge strategy Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807281506510.2725%40eeepc-johanness In-Reply-To: <03fa2187a72957d98d63ab899b39e9adc2edfe99.1217207602.git.vmiklos@frugalware.org> ``` Hi, On Mon, 28 Jul 2008, Miklos Vajna wrote: > Testing is done by creating a simple git-merge-theirs strategy which is > the opposite of ours. Using this in real merges is not recommended but > it's perfect for our testing needs. Note that what was asked for, and what Junio implemented before deciding that it would do more harm than good in git.git, is not the same as what you provide. Your -theirs is a strict opposite of -ours, i.e. the tree after the merge will be identical to the "merged" branch's tip's. The -theirs which was asked for (and which I truly think is insane) wanted to do a merge, and in case of merge conflicts take the "upstream" version _only of the conflicting hunks_. Just to make sure everyone grasps why this is bad: - there is not only a real chance, but a high probability that a merge conflict means that some related, unconflicting change relies on _one_ version of the conflicting hunk which might very well be "ours", and - since we have a _recursive_ merge, the notion of "upstream" is completely bogus when working on any merge that has more than one merge base. This merge would succeed, but actively be wrong. Just to make sure people do not have to ask for that version of "-theirs" again, Dscho ``` ## Miklos Vajna, 2008-07-28 23:24 Subject: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands() Message-ID: <5ad105819efb1c905bd01db3d08eb3422d283b3b.1217287269.git.vmiklos@frugalware.org> URL: https://gitlist.dev/e/5ad105819efb1c905bd01db3d08eb3422d283b3b.1217287269.git.vmiklos%40frugalware.org In-Reply-To: <7vr69ex00x.fsf@gitster.siamese.dyndns.org> ``` 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 --- On Sun, Jul 27, 2008 at 06:36:30PM -0700, Junio C Hamano 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 ``` ## Miklos Vajna, 2008-07-28 23:39 Subject: [PATCH 6/6] Add a new test for using a custom merge strategy Message-ID: URL: https://gitlist.dev/e/f3af7df2dda2dcb7801314cc993470264259f970.1217288180.git.vmiklos%40frugalware.org In-Reply-To: ``` 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 --- On Mon, Jul 28, 2008 at 03:12:59PM +0200, Johannes Schindelin 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 ``` ## Miklos Vajna, 2008-07-28 23:48 Subject: [PATCH] builtin-merge: allow using a custom strategy Message-ID: <1217288908-21124-1-git-send-email-vmiklos@frugalware.org> URL: https://gitlist.dev/e/1217288908-21124-1-git-send-email-vmiklos%40frugalware.org In-Reply-To: ``` 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 --- On Mon, Jul 28, 2008 at 03:06:09PM +0200, Johannes Schindelin 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 ``` ## Johannes Schindelin, 2008-07-28 23:54 Subject: Re: [PATCH 6/6] Add a new test for using a custom merge strategy Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807290153300.2725%40eeepc-johanness In-Reply-To: ``` Hi, On Tue, 29 Jul 2008, Miklos Vajna wrote: > Testing is done by creating a simple git-merge-theirs strategy which > just picks the upstream tree. (In other words, this is not the opposite > of -s ours.) Actually, this _is_ the opposite of -s ours, no? -s ours just takes our tree, your -s theirs just takes their tree. Sorry for the confusion I caused, Dscho ``` ## Miklos Vajna, 2008-07-28 23:58 Subject: Re: [PATCH 6/6] Add a new test for using a custom merge strategy Message-ID: <20080728235847.GR32057@genesis.frugalware.org> URL: https://gitlist.dev/e/20080728235847.GR32057%40genesis.frugalware.org In-Reply-To: ``` On Tue, Jul 29, 2008 at 01:54:17AM +0200, Johannes Schindelin 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. ;-) ``` ## Johannes Schindelin, 2008-07-28 23:59 Subject: Re: [PATCH] builtin-merge: allow using a custom strategy Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0807290158470.2725%40eeepc-johanness In-Reply-To: <1217288908-21124-1-git-send-email-vmiklos@frugalware.org> ``` Hi, On Tue, 29 Jul 2008, Miklos Vajna wrote: > On Mon, Jul 28, 2008 at 03:06:09PM +0200, Johannes Schindelin > 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 ``` ## Miklos Vajna, 2008-07-29 00:01 Subject: Re: [PATCH 6/6] Add a new test for using a custom merge strategy Message-ID: <20080729000104.GS32057@genesis.frugalware.org> URL: https://gitlist.dev/e/20080729000104.GS32057%40genesis.frugalware.org In-Reply-To: <20080728235847.GR32057@genesis.frugalware.org> ``` On Tue, Jul 29, 2008 at 01:58:47AM +0200, Miklos Vajna wrote: > So I guess my original patch was right, then. s/patch/description/. The check for the same trees are still valid. ```