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
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 <redacted>
---
Makefile | 1 +
help.c | 25 ++++++++++++++-----------
help.h | 6 ++++++
3 files changed, 21 insertions(+), 11 deletions(-)
create mode 100644 help.h
@@ -418,17 +418,20 @@ static int is_executable(const char *name)}staticunsignedintlist_commands_in_dir(structcmdnames*cmds,-constchar*path)+constchar*path,+constchar*prefix){unsignedintlongest=0;-constchar*prefix="git-";-intprefix_len=strlen(prefix);+intprefix_len;DIR*dir=opendir(path);structdirent*de;if(!dir||chdir(path))return0;+if(!prefix)+prefix="git-";+prefix_len=strlen(prefix);while((de=readdir(dir))!=NULL){intentlen;
@@ -452,7 +455,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,returnlongest;}-staticunsignedintload_command_list(void)+staticunsignedintload_command_list(constchar*prefix){unsignedintlongest=0;unsignedintlen;
@@ -461,7 +464,7 @@ static unsigned int load_command_list(void)constchar*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)staticvoidlist_commands(void){-unsignedintlongest=load_command_list();+unsignedintlongest=load_command_list(NULL);constchar*exec_path=git_exec_path();if(main_cmds.cnt){
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 <redacted>
---
help.c | 24 ++++++++++--------------
help.h | 14 +++++++++++++-
2 files changed, 23 insertions(+), 15 deletions(-)
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 <redacted>
---
help.c | 10 +++++-----
help.h | 1 +
2 files changed, 6 insertions(+), 5 deletions(-)
@@ -501,13 +501,13 @@ static unsigned int load_command_list(const char *prefix)returnlongest;}-staticvoidlist_commands(void)+voidlist_commands(constchar*prefix,constchar*title){-unsignedintlongest=load_command_list(NULL);+unsignedintlongest=load_command_list(prefix);constchar*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');
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 <redacted>
---
builtin-merge.c | 13 ++++++++++++-
1 files changed, 12 insertions(+), 1 deletions(-)
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 <redacted>
---
builtin-merge.c | 19 ++++++++++++-------
1 files changed, 12 insertions(+), 7 deletions(-)
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 <redacted>
---
On Tue, Jul 22, 2008 at 01:24:14AM -0700, Junio C Hamano [off-list ref] 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(-)
@@ -112,6 +112,21 @@ test_expect_success 'setup conflicted merge' '# recusive is choosen. test_expect_success'merge picks up the best result''+gitconfig--unset-allpull.twohead&&+gitreset--hardc5&&+gitmerge-sresolvec6+resolve_count=$(conflict_count)&&+gitreset--hardc5&&+gitmerge-srecursivec6+recursive_count=$(conflict_count)&&+gitreset--hardc5&&+gitmerge-srecursive-sresolvec6+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)''gitconfigpull.twohead"recursive resolve"&&gitreset--hardc5&&gitmerge-sresolvec6
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 <redacted>
---
t/t7606-merge-custom.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 45 insertions(+), 0 deletions(-)
create mode 100755 t/t7606-merge-custom.sh
@@ -0,0 +1,45 @@+#!/bin/sh++test_description='git-merge++Testingacustomstrategy.'++../test-lib.sh++cat>git-merge-theirs<<EOF+#!/bin/sh+evalgitread-tree--reset-u\\\$\$#+EOF+chmod+xgit-merge-theirs+PATH=.:$PATH+exportPATH++test_expect_success'setup''+echoc0>c0.c&&+gitaddc0.c&&+gitcommit-mc0&&+gittagc0&&+echoc1>c1.c&&+gitaddc1.c&&+gitcommit-mc1&&+gittagc1&&+gitreset--hardc0&&+echoc2>c2.c&&+gitaddc2.c&&+gitcommit-mc2&&+gittagc2+'++test_expect_success'merge c2 with a custom strategy''+gitreset--hardc1&&+gitmerge-stheirsc2&&+test"$(gitrev-parsec1)"!="$(gitrev-parseHEAD)"&&+test"$(gitrev-parsec1)"="$(gitrev-parseHEAD^1)"&&+test"$(gitrev-parsec2)"="$(gitrev-parseHEAD^2)"&&+gitdiff--exit-code&&+test-fc0.c&&+test!-fc1.c&&+test-fc2.c+'++test_done
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 <redacted>
---
help.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
On Sat, Jul 26, 2008 at 01:54:45PM +0200, Miklos Vajna [off-list ref] 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
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:45:03
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
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:45:03
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
On Sat, Jul 26, 2008 at 05:08:11PM +0200, Johannes Schindelin [off-list ref] wrote:
quoted
+ 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).
quoted
+ 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.)
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 <redacted>
---
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(-)
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:45:03
Hi,
On Sat, 26 Jul 2008, Miklos Vajna wrote:
On Sat, Jul 26, 2008 at 05:08:11PM +0200, Johannes Schindelin [off-list ref] wrote:
quoted
quoted
+ 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
On Sat, Jul 26, 2008 at 05:38:55PM +0200, Johannes Schindelin [off-list ref] wrote:
quoted
- 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.
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:45:03
Hi,
On Sat, 26 Jul 2008, Miklos Vajna wrote:
On Sat, Jul 26, 2008 at 05:38:55PM +0200, Johannes Schindelin [off-list ref] wrote:
quoted
quoted
- 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
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 <redacted>
---
On Sat, Jul 26, 2008 at 07:01:16PM +0200, Johannes Schindelin [off-list ref] wrote:
quoted
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(-)
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.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:45:03
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)
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 <redacted>
---
On Sat, Jul 26, 2008 at 01:28:39PM -0500, Jonathan Nieder [off-list ref] wrote:
quoted
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(-)
@@ -501,23 +501,25 @@ static unsigned int load_command_list(const char *prefix)returnlongest;}-staticvoidlist_commands(void)+voidlist_commands(constchar*prefix,constchar*title){-unsignedintlongest=load_command_list(NULL);+unsignedintlongest=load_command_list(prefix);constchar*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');}
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.
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
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 <redacted>
---
builtin-merge.c | 15 ++++++++++++++-
1 files changed, 14 insertions(+), 1 deletions(-)
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 <redacted>
---
help.c | 24 ++++++++++--------------
help.h | 14 +++++++++++++-
2 files changed, 23 insertions(+), 15 deletions(-)
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 <redacted>
---
help.c | 18 ++++++++++--------
help.h | 1 +
2 files changed, 11 insertions(+), 8 deletions(-)
@@ -506,23 +506,25 @@ static unsigned int load_command_list(const char *prefix)returnlongest;}-staticvoidlist_commands(void)+voidlist_commands(constchar*prefix,constchar*title){-unsignedintlongest=load_command_list(NULL);+unsignedintlongest=load_command_list(prefix);constchar*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');}
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 <redacted>
---
Makefile | 1 +
help.c | 25 ++++++++++++++-----------
help.h | 6 ++++++
3 files changed, 21 insertions(+), 11 deletions(-)
create mode 100644 help.h
@@ -418,11 +418,11 @@ static int is_executable(const char *name)}staticunsignedintlist_commands_in_dir(structcmdnames*cmds,-constchar*path)+constchar*path,+constchar*prefix){unsignedintlongest=0;-constchar*prefix="git-";-intprefix_len=strlen(prefix);+intprefix_len;DIR*dir=opendir(path);structdirent*de;structstrbufbuf=STRBUF_INIT;
@@ -430,6 +430,9 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,if(!dir)return0;+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,returnlongest;}-staticunsignedintload_command_list(void)+staticunsignedintload_command_list(constchar*prefix){unsignedintlongest=0;unsignedintlen;
@@ -469,7 +472,7 @@ static unsigned int load_command_list(void)constchar*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)staticvoidlist_commands(void){-unsignedintlongest=load_command_list();+unsignedintlongest=load_command_list(NULL);constchar*exec_path=git_exec_path();if(main_cmds.cnt){
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 <redacted>
---
builtin-merge.c | 19 ++++++++++++-------
1 files changed, 12 insertions(+), 7 deletions(-)
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 <redacted>
---
t/t7606-merge-custom.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 45 insertions(+), 0 deletions(-)
create mode 100755 t/t7606-merge-custom.sh
@@ -0,0 +1,45 @@+#!/bin/sh++test_description='git-merge++Testingacustomstrategy.'++../test-lib.sh++cat>git-merge-theirs<<EOF+#!/bin/sh+evalgitread-tree--reset-u\\\$\$#+EOF+chmod+xgit-merge-theirs+PATH=.:$PATH+exportPATH++test_expect_success'setup''+echoc0>c0.c&&+gitaddc0.c&&+gitcommit-mc0&&+gittagc0&&+echoc1>c1.c&&+gitaddc1.c&&+gitcommit-mc1&&+gittagc1&&+gitreset--hardc0&&+echoc2>c2.c&&+gitaddc2.c&&+gitcommit-mc2&&+gittagc2+'++test_expect_success'merge c2 with a custom strategy''+gitreset--hardc1&&+gitmerge-stheirsc2&&+test"$(gitrev-parsec1)"!="$(gitrev-parseHEAD)"&&+test"$(gitrev-parsec1)"="$(gitrev-parseHEAD^1)"&&+test"$(gitrev-parsec2)"="$(gitrev-parseHEAD^2)"&&+gitdiff--exit-code&&+test-fc0.c&&+test!-fc1.c&&+test-fc2.c+'++test_done
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
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:45:03
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
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 <redacted>
---
On Mon, Jul 28, 2008 at 03:12:59PM +0200, Johannes Schindelin [off-list ref] 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
@@ -0,0 +1,46 @@+#!/bin/sh++test_description='git-merge++Testingacustomstrategy.'++../test-lib.sh++cat>git-merge-theirs<<EOF+#!/bin/sh+evalgitread-tree--reset-u\\\$\$#+EOF+chmod+xgit-merge-theirs+PATH=.:$PATH+exportPATH++test_expect_success'setup''+echoc0>c0.c&&+gitaddc0.c&&+gitcommit-mc0&&+gittagc0&&+echoc1>c1.c&&+gitaddc1.c&&+gitcommit-mc1&&+gittagc1&&+gitreset--hardc0&&+echoc2>c2.c&&+gitaddc2.c&&+gitcommit-mc2&&+gittagc2+'++test_expect_success'merge c2 with a custom strategy''+gitreset--hardc1&&+gitmerge-stheirsc2&&+test"$(gitrev-parsec1)"!="$(gitrev-parseHEAD)"&&+test"$(gitrev-parsec1)"="$(gitrev-parseHEAD^1)"&&+test"$(gitrev-parsec2)"="$(gitrev-parseHEAD^2)"&&+test"$(gitrev-parsec2^{tree})"="$(gitrev-parseHEAD^{tree})"&&+gitdiff--exit-code&&+test-fc0.c&&+test!-fc1.c&&+test-fc2.c+'++test_done
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 <redacted>
---
On Mon, Jul 28, 2008 at 03:06:09PM +0200, Johannes Schindelin [off-list ref] 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(-)
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:45:04
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
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:45:04
Hi,
On Tue, 29 Jul 2008, Miklos Vajna wrote:
On Mon, Jul 28, 2008 at 03:06:09PM +0200, Johannes Schindelin
[off-list ref] wrote:
quoted
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.
On Tue, Jul 29, 2008 at 01:54:17AM +0200, Johannes Schindelin [off-list ref] 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. ;-)
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 <redacted>
---
t/t7606-merge-custom.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 46 insertions(+), 0 deletions(-)
create mode 100755 t/t7606-merge-custom.sh
@@ -0,0 +1,46 @@+#!/bin/sh++test_description='git-merge++Testingacustomstrategy.'++../test-lib.sh++cat>git-merge-theirs<<EOF+#!/bin/sh+evalgitread-tree--reset-u\\\$\$#+EOF+chmod+xgit-merge-theirs+PATH=.:$PATH+exportPATH++test_expect_success'setup''+echoc0>c0.c&&+gitaddc0.c&&+gitcommit-mc0&&+gittagc0&&+echoc1>c1.c&&+gitaddc1.c&&+gitcommit-mc1&&+gittagc1&&+gitreset--hardc0&&+echoc2>c2.c&&+gitaddc2.c&&+gitcommit-mc2&&+gittagc2+'++test_expect_success'merge c2 with a custom strategy''+gitreset--hardc1&&+gitmerge-stheirsc2&&+test"$(gitrev-parsec1)"!="$(gitrev-parseHEAD)"&&+test"$(gitrev-parsec1)"="$(gitrev-parseHEAD^1)"&&+test"$(gitrev-parsec2)"="$(gitrev-parseHEAD^2)"&&+test"$(gitrev-parsec2^{tree})"="$(gitrev-parseHEAD^{tree})"&&+gitdiff--exit-code&&+test-fc0.c&&+test!-fc1.c&&+test-fc2.c+'++test_done
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 <redacted>
---
Makefile | 1 +
help.c | 25 ++++++++++++++-----------
help.h | 6 ++++++
3 files changed, 21 insertions(+), 11 deletions(-)
create mode 100644 help.h
@@ -418,11 +418,11 @@ static int is_executable(const char *name)}staticunsignedintlist_commands_in_dir(structcmdnames*cmds,-constchar*path)+constchar*path,+constchar*prefix){unsignedintlongest=0;-constchar*prefix="git-";-intprefix_len=strlen(prefix);+intprefix_len;DIR*dir=opendir(path);structdirent*de;structstrbufbuf=STRBUF_INIT;
@@ -430,6 +430,9 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,if(!dir)return0;+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,returnlongest;}-staticunsignedintload_command_list(void)+staticunsignedintload_command_list(constchar*prefix){unsignedintlongest=0;unsignedintlen;
@@ -469,7 +472,7 @@ static unsigned int load_command_list(void)constchar*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)staticvoidlist_commands(void){-unsignedintlongest=load_command_list();+unsignedintlongest=load_command_list(NULL);constchar*exec_path=git_exec_path();if(main_cmds.cnt){
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 <redacted>
---
help.c | 26 +++++++++++---------------
help.h | 14 +++++++++++++-
2 files changed, 24 insertions(+), 16 deletions(-)
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 <redacted>
---
help.c | 18 ++++++++++--------
help.h | 1 +
2 files changed, 11 insertions(+), 8 deletions(-)
@@ -506,23 +506,25 @@ static unsigned int load_command_list(const char *prefix)returnlongest;}-staticvoidlist_commands(void)+voidlist_commands(constchar*prefix,constchar*title){-unsignedintlongest=load_command_list(NULL);+unsignedintlongest=load_command_list(prefix);constchar*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');}
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
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 <redacted>
---
builtin-merge.c | 32 +++++++++++++++++++++++++-------
1 files changed, 25 insertions(+), 7 deletions(-)
From: Junio C Hamano <hidden> Date: 2016-06-15 22:45:04
Miklos Vajna [off-list ref] 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.
From: Junio C Hamano <hidden> Date: 2016-06-15 22:45:04
Miklos Vajna [off-list ref] 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?
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 <redacted>
---
On Tue, Jul 29, 2008 at 12:46:59PM -0700, Junio C Hamano [off-list ref] 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(-)
@@ -463,7 +457,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,returnlongest;}-staticunsignedintload_command_list(constchar*prefix)+staticunsignedintload_command_list(constchar*prefix,structcmdnames*exclude){unsignedintlongest=0;unsignedintlen;
@@ -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);returnlongest;}-voidlist_commands(constchar*prefix,constchar*title)+voidlist_commands(constchar*prefix,constchar*title,structcmdnames*exclude){-unsignedintlongest=load_command_list(prefix);+unsignedintlongest=load_command_list(prefix,exclude);constchar*exec_path=git_exec_path();if(main_cmds.cnt){
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 <redacted>
---
t/t7606-merge-custom.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 46 insertions(+), 0 deletions(-)
create mode 100755 t/t7606-merge-custom.sh
@@ -0,0 +1,46 @@+#!/bin/sh++test_description='git-merge++Testingacustomstrategy.'++../test-lib.sh++cat>git-merge-theirs<<EOF+#!/bin/sh+evalgitread-tree--reset-u\\\$\$#+EOF+chmod+xgit-merge-theirs+PATH=.:$PATH+exportPATH++test_expect_success'setup''+echoc0>c0.c&&+gitaddc0.c&&+gitcommit-mc0&&+gittagc0&&+echoc1>c1.c&&+gitaddc1.c&&+gitcommit-mc1&&+gittagc1&&+gitreset--hardc0&&+echoc2>c2.c&&+gitaddc2.c&&+gitcommit-mc2&&+gittagc2+'++test_expect_success'merge c2 with a custom strategy''+gitreset--hardc1&&+gitmerge-stheirsc2&&+test"$(gitrev-parsec1)"!="$(gitrev-parseHEAD)"&&+test"$(gitrev-parsec1)"="$(gitrev-parseHEAD^1)"&&+test"$(gitrev-parsec2)"="$(gitrev-parseHEAD^2)"&&+test"$(gitrev-parsec2^{tree})"="$(gitrev-parseHEAD^{tree})"&&+gitdiff--exit-code&&+test-fc0.c&&+test!-fc1.c&&+test-fc2.c+'++test_done
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 <redacted>
---
t/t7600-merge.sh | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
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 <redacted>
---
builtin-merge.c | 42 +++++++++++++++++++++++++++++++++++-------
1 files changed, 35 insertions(+), 7 deletions(-)
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 <redacted>
---
Makefile | 1 +
help.c | 77 +++++++++++++++++++++++++++++++------------------------------
help.h | 23 ++++++++++++++++++
3 files changed, 63 insertions(+), 38 deletions(-)
create mode 100644 help.h
@@ -418,11 +412,11 @@ static int is_executable(const char *name)}staticunsignedintlist_commands_in_dir(structcmdnames*cmds,-constchar*path)+constchar*path,+constchar*prefix){unsignedintlongest=0;-constchar*prefix="git-";-intprefix_len=strlen(prefix);+intprefix_len;DIR*dir=opendir(path);structdirent*de;structstrbufbuf=STRBUF_INIT;
@@ -430,6 +424,9 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,if(!dir)return0;+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,returnlongest;}-staticunsignedintload_command_list(void)+unsignedintload_command_list(constchar*prefix,+structcmdnames*main_cmds,+structcmdnames*other_cmds){unsignedintlongest=0;unsignedintlen;
@@ -469,7 +468,7 @@ static unsigned int load_command_list(void)constchar*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);returnlongest;}-staticvoidlist_commands(void)+voidlist_commands(constchar*title,unsignedintlongest,+structcmdnames*main_cmds,structcmdnames*other_cmds){-unsignedintlongest=load_command_list();constchar*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');}}
@@ -0,0 +1,23 @@+#ifndef HELP_H+#define HELP_H++structcmdnames{+intalloc;+intcnt;+structcmdname{+size_tlen;+charname[FLEX_ARRAY];+}**names;+};++unsignedintload_command_list(constchar*prefix,+structcmdnames*main_cmds,+structcmdnames*other_cmds);+voidadd_cmdname(structcmdnames*cmds,constchar*name,intlen);+/* Here we require that excludes is a sorted list. */+voidexclude_cmds(structcmdnames*cmds,structcmdnames*excludes);+intis_in_cmdlist(structcmdnames*c,constchar*s);+voidlist_commands(constchar*title,unsignedintlongest,+structcmdnames*main_cmds,structcmdnames*other_cmds);++#endif /* HELP_H */
On Tue, Jul 29, 2008 at 12:47:05PM -0700, Junio C Hamano [off-list ref] 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
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 ;-)
On Tue, Jul 29, 2008 at 04:43:53PM -0700, Junio C Hamano [off-list ref] 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.