* [PATCH 0/6] Allow custom merge strategies, take 2
From: Miklos Vajna @ 2008-07-28 1:21 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1217037178.git.vmiklos@frugalware.org>
Hi,
This is just a resend of my series, on top of Dscho's "Avoid chdir() in
list_commands_in_dir()" (f5d600e), including the suggestions I received
from Jonathan Nieder and Dscho.
Miklos Vajna (6):
builtin-help: make is_git_command() usable outside builtin-help
builtin-help: make list_commands() a bit more generic
builtin-help: make it possible to exclude some commands in
list_commands()
builtin-merge: allow using a custom strategy
builtin-merge: avoid non-strategy git-merge commands in error message
Add a new test for using a custom merge strategy
Makefile | 1 +
builtin-merge.c | 32 +++++++++++++++++++++------
help.c | 55 ++++++++++++++++++++++++-----------------------
help.h | 19 ++++++++++++++++
t/t7606-merge-custom.sh | 45 ++++++++++++++++++++++++++++++++++++++
5 files changed, 118 insertions(+), 34 deletions(-)
create mode 100644 help.h
create mode 100755 t/t7606-merge-custom.sh
^ permalink raw reply
* [PATCH 5/6] builtin-merge: avoid non-strategy git-merge commands in error message
From: Miklos Vajna @ 2008-07-28 1:21 UTC (permalink / raw)
To: git
In-Reply-To: <cover.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 <vmiklos@frugalware.org>
---
builtin-merge.c | 15 ++++++++++++++-
1 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/builtin-merge.c b/builtin-merge.c
index cdbc692..29826b1 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -88,8 +88,21 @@ static struct strategy *get_strategy(const char *name)
return &all_strategy[i];
if (!is_git_command(name, "git-merge-")) {
+ struct cmdnames not_strategies;
+
+ memset(¬_strategies, 0, sizeof(struct cmdnames));
+ for (i = 0; i < main_cmds.cnt; i++) {
+ int j, found = 0;
+ struct cmdname *ent = main_cmds.names[i];
+ for (j = 0; j < ARRAY_SIZE(all_strategy); j++)
+ if (!strncmp(ent->name, all_strategy[j].name, ent->len)
+ && !all_strategy[j].name[ent->len])
+ found = 1;
+ if (!found)
+ add_cmdname(¬_strategies, ent->name, ent->len);
+ }
fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name);
- list_commands("git-merge-", "strategies");
+ list_commands("git-merge-", "strategies", ¬_strategies);
exit(1);
}
--
1.6.0.rc0.14.g95f8.dirty
^ permalink raw reply related
* [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands()
From: Miklos Vajna @ 2008-07-28 1:21 UTC (permalink / raw)
To: git
In-Reply-To: <cover.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 <vmiklos@frugalware.org>
---
help.c | 24 ++++++++++--------------
help.h | 14 +++++++++++++-
2 files changed, 23 insertions(+), 15 deletions(-)
diff --git a/help.c b/help.c
index 7a42517..318d6d6 100644
--- a/help.c
+++ b/help.c
@@ -9,6 +9,7 @@
#include "common-cmds.h"
#include "parse-options.h"
#include "run-command.h"
+#include "help.h"
static struct man_viewer_list {
struct man_viewer_list *next;
@@ -300,16 +301,9 @@ static inline void mput_char(char c, unsigned int num)
putchar(c);
}
-static struct cmdnames {
- int alloc;
- int cnt;
- struct cmdname {
- size_t len;
- char name[1];
- } **names;
-} main_cmds, other_cmds;
+struct cmdnames main_cmds, other_cmds;
-static void add_cmdname(struct cmdnames *cmds, const char *name, int len)
+void add_cmdname(struct cmdnames *cmds, const char *name, int len)
{
struct cmdname *ent = xmalloc(sizeof(*ent) + len);
@@ -463,7 +457,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
return longest;
}
-static unsigned int load_command_list(const char *prefix)
+static unsigned int load_command_list(const char *prefix, struct cmdnames *exclude)
{
unsigned int longest = 0;
unsigned int len;
@@ -502,13 +496,15 @@ static unsigned int load_command_list(const char *prefix)
sizeof(*other_cmds.names), cmdname_compare);
uniq(&other_cmds);
exclude_cmds(&other_cmds, &main_cmds);
+ if (exclude)
+ exclude_cmds(&main_cmds, exclude);
return longest;
}
-void list_commands(const char *prefix, const char *title)
+void list_commands(const char *prefix, const char *title, struct cmdnames *exclude)
{
- unsigned int longest = load_command_list(prefix);
+ unsigned int longest = load_command_list(prefix, exclude);
const char *exec_path = git_exec_path();
if (main_cmds.cnt) {
@@ -558,7 +554,7 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s)
int is_git_command(const char *s, const char *prefix)
{
- load_command_list(prefix);
+ load_command_list(prefix, NULL);
return is_in_cmdlist(&main_cmds, s) ||
is_in_cmdlist(&other_cmds, s);
}
@@ -704,7 +700,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
if (show_all) {
printf("usage: %s\n\n", git_usage_string);
- list_commands("git-", "git commands");
+ list_commands("git-", "git commands", NULL);
printf("%s\n", git_more_info_string);
return 0;
}
diff --git a/help.h b/help.h
index 0741662..85d3b74 100644
--- a/help.h
+++ b/help.h
@@ -1,7 +1,19 @@
#ifndef HELP_H
#define HELP_H
+struct cmdnames {
+ int alloc;
+ int cnt;
+ struct cmdname {
+ size_t len;
+ char name[1];
+ } **names;
+};
+
int is_git_command(const char *s, const char *prefix);
-void list_commands(const char *prefix, const char *title);
+void list_commands(const char *prefix, const char *title, struct cmdnames *exclude);
+void add_cmdname(struct cmdnames *cmds, const char *name, int len);
+
+extern struct cmdnames main_cmds, other_cmds;
#endif /* HELP_H */
--
1.6.0.rc0.14.g95f8.dirty
^ permalink raw reply related
* [PATCH 2/6] builtin-help: make list_commands() a bit more generic
From: Miklos Vajna @ 2008-07-28 1:21 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1217207602.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 <vmiklos@frugalware.org>
---
help.c | 18 ++++++++++--------
help.h | 1 +
2 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/help.c b/help.c
index 08188f5..7a42517 100644
--- a/help.c
+++ b/help.c
@@ -506,23 +506,25 @@ static unsigned int load_command_list(const char *prefix)
return longest;
}
-static void list_commands(void)
+void list_commands(const char *prefix, const char *title)
{
- unsigned int longest = load_command_list(NULL);
+ unsigned int longest = load_command_list(prefix);
const char *exec_path = git_exec_path();
if (main_cmds.cnt) {
- printf("available git commands in '%s'\n", exec_path);
- printf("----------------------------");
- mput_char('-', strlen(exec_path));
+ printf("available %s in '%s'\n", title, exec_path);
+ printf("----------------");
+ mput_char('-', strlen(title) + strlen(exec_path));
putchar('\n');
pretty_print_string_list(&main_cmds, longest);
putchar('\n');
}
if (other_cmds.cnt) {
- printf("git commands available from elsewhere on your $PATH\n");
- printf("---------------------------------------------------\n");
+ printf("%s available from elsewhere on your $PATH\n", title);
+ printf("---------------------------------------");
+ mput_char('-', strlen(title));
+ putchar('\n');
pretty_print_string_list(&other_cmds, longest);
putchar('\n');
}
@@ -702,7 +704,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
if (show_all) {
printf("usage: %s\n\n", git_usage_string);
- list_commands();
+ list_commands("git-", "git commands");
printf("%s\n", git_more_info_string);
return 0;
}
diff --git a/help.h b/help.h
index 73da8d6..0741662 100644
--- a/help.h
+++ b/help.h
@@ -2,5 +2,6 @@
#define HELP_H
int is_git_command(const char *s, const char *prefix);
+void list_commands(const char *prefix, const char *title);
#endif /* HELP_H */
--
1.6.0.rc0.14.g95f8.dirty
^ permalink raw reply related
* [PATCH 1/6] builtin-help: make is_git_command() usable outside builtin-help
From: Miklos Vajna @ 2008-07-28 1:21 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1217207602.git.vmiklos@frugalware.org>
Other builtins may want to check if a given command is a valid git
command or not as well. Additionally add a new parameter that specifies
a custom prefix, so that the "git-" prefix is no longer hardwired.
Useful for example to limit the search for "git-merge-*".
Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
Makefile | 1 +
help.c | 25 ++++++++++++++-----------
help.h | 6 ++++++
3 files changed, 21 insertions(+), 11 deletions(-)
create mode 100644 help.h
diff --git a/Makefile b/Makefile
index 798a2f2..351afd7 100644
--- a/Makefile
+++ b/Makefile
@@ -355,6 +355,7 @@ LIB_H += git-compat-util.h
LIB_H += graph.h
LIB_H += grep.h
LIB_H += hash.h
+LIB_H += help.h
LIB_H += list-objects.h
LIB_H += ll-merge.h
LIB_H += log-tree.h
diff --git a/help.c b/help.c
index 3cb1962..08188f5 100644
--- a/help.c
+++ b/help.c
@@ -418,11 +418,11 @@ static int is_executable(const char *name)
}
static unsigned int list_commands_in_dir(struct cmdnames *cmds,
- const char *path)
+ const char *path,
+ const char *prefix)
{
unsigned int longest = 0;
- const char *prefix = "git-";
- int prefix_len = strlen(prefix);
+ int prefix_len;
DIR *dir = opendir(path);
struct dirent *de;
struct strbuf buf = STRBUF_INIT;
@@ -430,6 +430,9 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
if (!dir)
return 0;
+ if (!prefix)
+ prefix = "git-";
+ prefix_len = strlen(prefix);
strbuf_addf(&buf, "%s/", path);
len = buf.len;
@@ -460,7 +463,7 @@ static unsigned int list_commands_in_dir(struct cmdnames *cmds,
return longest;
}
-static unsigned int load_command_list(void)
+static unsigned int load_command_list(const char *prefix)
{
unsigned int longest = 0;
unsigned int len;
@@ -469,7 +472,7 @@ static unsigned int load_command_list(void)
const char *exec_path = git_exec_path();
if (exec_path)
- longest = list_commands_in_dir(&main_cmds, exec_path);
+ longest = list_commands_in_dir(&main_cmds, exec_path, prefix);
if (!env_path) {
fprintf(stderr, "PATH not set\n");
@@ -481,7 +484,7 @@ static unsigned int load_command_list(void)
if ((colon = strchr(path, PATH_SEP)))
*colon = 0;
- len = list_commands_in_dir(&other_cmds, path);
+ len = list_commands_in_dir(&other_cmds, path, prefix);
if (len > longest)
longest = len;
@@ -505,7 +508,7 @@ static unsigned int load_command_list(void)
static void list_commands(void)
{
- unsigned int longest = load_command_list();
+ unsigned int longest = load_command_list(NULL);
const char *exec_path = git_exec_path();
if (main_cmds.cnt) {
@@ -551,9 +554,9 @@ static int is_in_cmdlist(struct cmdnames *c, const char *s)
return 0;
}
-static int is_git_command(const char *s)
+int is_git_command(const char *s, const char *prefix)
{
- load_command_list();
+ load_command_list(prefix);
return is_in_cmdlist(&main_cmds, s) ||
is_in_cmdlist(&other_cmds, s);
}
@@ -574,7 +577,7 @@ static const char *cmd_to_page(const char *git_cmd)
return "git";
else if (!prefixcmp(git_cmd, "git"))
return git_cmd;
- else if (is_git_command(git_cmd))
+ else if (is_git_command(git_cmd, NULL))
return prepend("git-", git_cmd);
else
return prepend("git", git_cmd);
@@ -712,7 +715,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
}
alias = alias_lookup(argv[0]);
- if (alias && !is_git_command(argv[0])) {
+ if (alias && !is_git_command(argv[0], NULL)) {
printf("`git %s' is aliased to `%s'\n", argv[0], alias);
return 0;
}
diff --git a/help.h b/help.h
new file mode 100644
index 0000000..73da8d6
--- /dev/null
+++ b/help.h
@@ -0,0 +1,6 @@
+#ifndef HELP_H
+#define HELP_H
+
+int is_git_command(const char *s, const char *prefix);
+
+#endif /* HELP_H */
--
1.6.0.rc0.14.g95f8.dirty
^ permalink raw reply related
* [PATCH 4/6] builtin-merge: allow using a custom strategy
From: Miklos Vajna @ 2008-07-28 1:21 UTC (permalink / raw)
To: git
In-Reply-To: <cover.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 <vmiklos@frugalware.org>
---
builtin-merge.c | 19 ++++++++++++-------
1 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/builtin-merge.c b/builtin-merge.c
index e78fa18..cdbc692 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -22,6 +22,7 @@
#include "log-tree.h"
#include "color.h"
#include "rerere.h"
+#include "help.h"
#define DEFAULT_TWOHEAD (1<<0)
#define DEFAULT_OCTOPUS (1<<1)
@@ -77,7 +78,7 @@ static int option_parse_message(const struct option *opt,
static struct strategy *get_strategy(const char *name)
{
int i;
- struct strbuf err;
+ struct strategy *ret;
if (!name)
return NULL;
@@ -86,12 +87,16 @@ static struct strategy *get_strategy(const char *name)
if (!strcmp(name, all_strategy[i].name))
return &all_strategy[i];
- strbuf_init(&err, 0);
- for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
- strbuf_addf(&err, " %s", all_strategy[i].name);
- fprintf(stderr, "Could not find merge strategy '%s'.\n", name);
- fprintf(stderr, "Available strategies are:%s.\n", err.buf);
- exit(1);
+ if (!is_git_command(name, "git-merge-")) {
+ fprintf(stderr, "Could not find merge strategy '%s'.\n\n", name);
+ list_commands("git-merge-", "strategies");
+ exit(1);
+ }
+
+ ret = xmalloc(sizeof(struct strategy));
+ memset(ret, 0, sizeof(struct strategy));
+ ret->name = xstrdup(name);
+ return ret;
}
static void append_strategy(struct strategy *s)
--
1.6.0.rc0.14.g95f8.dirty
^ permalink raw reply related
* [PATCH 6/6] Add a new test for using a custom merge strategy
From: Miklos Vajna @ 2008-07-28 1:21 UTC (permalink / raw)
To: git
In-Reply-To: <cover.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 <vmiklos@frugalware.org>
---
t/t7606-merge-custom.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 45 insertions(+), 0 deletions(-)
create mode 100755 t/t7606-merge-custom.sh
diff --git a/t/t7606-merge-custom.sh b/t/t7606-merge-custom.sh
new file mode 100755
index 0000000..f295e56
--- /dev/null
+++ b/t/t7606-merge-custom.sh
@@ -0,0 +1,45 @@
+#!/bin/sh
+
+test_description='git-merge
+
+Testing a custom strategy.'
+
+. ./test-lib.sh
+
+cat > git-merge-theirs << EOF
+#!/bin/sh
+eval git read-tree --reset -u \\\$\$#
+EOF
+chmod +x git-merge-theirs
+PATH=.:$PATH
+export PATH
+
+test_expect_success 'setup' '
+ echo c0 > c0.c &&
+ git add c0.c &&
+ git commit -m c0 &&
+ git tag c0 &&
+ echo c1 > c1.c &&
+ git add c1.c &&
+ git commit -m c1 &&
+ git tag c1 &&
+ git reset --hard c0 &&
+ echo c2 > c2.c &&
+ git add c2.c &&
+ git commit -m c2 &&
+ git tag c2
+'
+
+test_expect_success 'merge c2 with a custom strategy' '
+ git reset --hard c1 &&
+ git merge -s theirs c2 &&
+ test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" &&
+ test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" &&
+ test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
+ git diff --exit-code &&
+ test -f c0.c &&
+ test ! -f c1.c &&
+ test -f c2.c
+'
+
+test_done
--
1.6.0.rc0.14.g95f8.dirty
^ permalink raw reply related
* Re: [PATCH] t/t7001-mv.sh: Propose ability to use git-mv on conflicting entries
From: Junio C Hamano @ 2008-07-28 1:21 UTC (permalink / raw)
To: Petr Baudis; +Cc: git, gitster
In-Reply-To: <7v4p6ayfmw.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> +# Rationale: I cannot git mv around a conflicted file. This is unnecessary
>> +# restriction in case another part of conflict resolution requires me to
>> +# move the file around.
>
> Yes, I would agree this is a reasonable thing to support. Something like
> this patch, perhaps.
> ...
Just in case if somebody is inclined to test the patch and polish it into
a shape good enough for inclusion...
> @@ -177,7 +177,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> } else
> bad = "Cannot overwrite";
> }
> - } else if (cache_name_pos(src, length) < 0)
> + } else if (((pos = cache_name_pos(src, length)) < 0) &&
> + strcmp(active_cache[-1 - pos]->name, src))
There is a bug here; "-1 - pos" needs to be checked against active_nr
before strcmp().
^ permalink raw reply
* Re: [PATCH] Documentation/git-ls-tree.txt: Add a caveat about prefixing pathspec
From: Junio C Hamano @ 2008-07-28 1:26 UTC (permalink / raw)
To: Petr Baudis; +Cc: git
In-Reply-To: <20080728004604.GF32184@machine.or.cz>
Petr Baudis <pasky@suse.cz> writes:
>> We may throw a dice or go with your version, I don't care *that* much
>> about this change, I just wouldn't make it personally.
>
> What is the status of this patch? :-) Dropped altogether?
Left behind on the far side of oblivion; I do not offhand recall what this
was about, sorry.
^ permalink raw reply
* Re: Bizarre missing changes (git bug?)
From: Roman Zippel @ 2008-07-28 1:29 UTC (permalink / raw)
To: Martin Langhoff; +Cc: Linus Torvalds, Tim Harper, git
In-Reply-To: <46a038f90807271625x35c561fdv6dc6b2c312f45fa1@mail.gmail.com>
Hi,
On Mon, 28 Jul 2008, Martin Langhoff wrote:
> On Mon, Jul 28, 2008 at 11:14 AM, Roman Zippel <zippel@linux-m68k.org> wrote:
> > Why are you dismissing what I wrote without even giving it a second
> > thought? I didn't bother with the initial example, because it's so
> > simple, that it's no real challenge.
>
> I can't speak for anyone else, but you do have to keep in mind that a
> solution to this has to be rather fast - and I mean fast in git terms,
> not in scripting-language-fast terms.
You also have to keep in mind, that I haven't really hacked git before, so
I'm just trying to do something with the data I can somehow extract from
it. I seriously didn't thought that anyone wouldn't understand that the
code example was just a proof of concept.
> That you can do it Ruby - and I may be able to do it Perl - has little
> bearing on what can be done inside the git log machinery with a small
> performance penalty.
It also has to do with correctness, is performance more important than
correctness?
Part of the problem is, what is the correct history, as which it should be
displayed via the various interfaces by default.
bye, Roman
^ permalink raw reply
* Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands()
From: Junio C Hamano @ 2008-07-28 1:36 UTC (permalink / raw)
To: Miklos Vajna; +Cc: git
In-Reply-To: <9cc2813166c8b20ffb411c3a28ad86665e60033b.1217207602.git.vmiklos@frugalware.org>
Miklos Vajna <vmiklos@frugalware.org> writes:
> diff --git a/help.h b/help.h
> index 0741662..85d3b74 100644
> --- a/help.h
> +++ b/help.h
> @@ -1,7 +1,19 @@
> #ifndef HELP_H
> #define HELP_H
>
> +struct cmdnames {
> + int alloc;
> + int cnt;
> + struct cmdname {
> + size_t len;
> + char name[1];
> + } **names;
> +};
I thought we do this kind of thing using FLEX_ARRAY macro. Is there any
reason its use is not appropriate here?
^ permalink raw reply
* Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands()
From: Johannes Schindelin @ 2008-07-28 2:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Miklos Vajna, git
In-Reply-To: <7vr69ex00x.fsf@gitster.siamese.dyndns.org>
Hi,
On Sun, 27 Jul 2008, Junio C Hamano wrote:
> Miklos Vajna <vmiklos@frugalware.org> writes:
>
> > diff --git a/help.h b/help.h
> > index 0741662..85d3b74 100644
> > --- a/help.h
> > +++ b/help.h
> > @@ -1,7 +1,19 @@
> > #ifndef HELP_H
> > #define HELP_H
> >
> > +struct cmdnames {
> > + int alloc;
> > + int cnt;
> > + struct cmdname {
> > + size_t len;
> > + char name[1];
> > + } **names;
> > +};
>
> I thought we do this kind of thing using FLEX_ARRAY macro. Is there any
> reason its use is not appropriate here?
I think that came up in the previous review round: the "name" member _is_
NUL-terminated, but could have a ".exe" suffix. The "len" member has the
length excluding ".exe".
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands()
From: Junio C Hamano @ 2008-07-28 3:05 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Miklos Vajna, git
In-Reply-To: <alpine.DEB.1.00.0807280442330.5526@eeepc-johanness>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > +struct cmdnames {
>> > + int alloc;
>> > + int cnt;
>> > + struct cmdname {
>> > + size_t len;
>> > + char name[1];
>> > + } **names;
>> > +};
>>
>> I thought we do this kind of thing using FLEX_ARRAY macro. Is there any
>> reason its use is not appropriate here?
>
> I think that came up in the previous review round: the "name" member _is_
> NUL-terminated, but could have a ".exe" suffix. The "len" member has the
> length excluding ".exe".
Sorry, but I do understand what you are trying to explain.
Marking the flexible member at the end as "last_member[FLEX_ARRAY]" is
about a tiny bit of abstracting out how the exact decl syntax should look
like depending on the compiler.
For example, we have:
struct git_attr {
struct git_attr *next;
unsigned h;
int attr_nr;
char name[FLEX_ARRAY];
};
And h is _not_ the length of the name[] (it is a hash to help us find the
entry quickly). Jjust like "len" is not the length of the name[] in your
structure. In other words, marking as "last_member[FLEX_ARRAY]" does not
have anything to do with what other fields you have in the same structure.
Hmm, am I missing a bigger picture somewhere else?
^ permalink raw reply
* Re: git-scm.com
From: Tom Werner @ 2008-07-28 3:11 UTC (permalink / raw)
To: git
In-Reply-To: <46a038f90807271619l69c085a7o58f50b7d64b7222d@mail.gmail.com>
On Sun, Jul 27, 2008 at 4:19 PM, Martin Langhoff
<martin.langhoff@gmail.com> wrote:
> And the choice of language has nothing to do with helping people
> around. If they care about getting recommendations from list regulars,
> anyway. Maintaining a great user-facing website might be their way of
> engaging and interacting with us.
As cofounder of GitHub I'd like to jump in and say a few words. I'm a
huge fan of git. HUGE. But that should already be obvious. We started
GitHub because we saw that git was a tremendously useful tool but was
missing a way to easily and flexibly share your public and private
code with other developers. That simple idea grew into what we now
like to call "Social Code Hosting."
I find it a bit confusing that some here seem to have a strong dislike
for GitHub. It's true that we haven't been active on the developer
list or in the #git channel on freenode, but we are constantly in
#github and have answered a *great* many questions from developers
that are new to git. At the same time, like Martin finally guesses, we
believe that our contribution to the git community is GitHub itself.
We provide free git hosting for over 16,000 open source repositories!
As mentioned earlier in the thread, the Ruby-Git binding that Scott
and I wrote has been open source from the beginning. While we did not
announce it here, we have publicized it in the Ruby circle (where,
presumably, people would most likely find it useful) and in fact there
are currently 28 forks and 138 watchers of the project. We've also
released albino, facebox, and github-services via GitHub. You can see
all the open source stuff we use at GitHub here:
http://github.com/github.
Perhaps it is the commercial aspect of GitHub that offends. The only
reason that GitHub is as featured and polished as it is, is because we
can make money from it. We hope to soon be working on GitHub full
time. There is no way this could have been possible if we did not
offer paid private repositories. A part of being a commercial
operation is making the main product closed source. One might argue
that we could still have GitHub as a service while making the code
open source, but the truth of the matter is that this is not in the
best interest of our future plans for the company.
I don't like the thought of being at odds with the git developer
community at all, and let me be the first to apologize if we've
offended anyone. It certainly is not our intention. Our goal with
GitHub is to help make git even better by offering a service that
helps people streamline their git workflows and discover projects that
may interest them. We're trying to give back to the community how we
know best: by offering kickass git hosting and associated
collaboration tools.
Having had to implement a git-daemon replacement that fit our needs, I
have some ideas on how to improve git-daemon and fetch-pack with
regards to error messages and logging. I'll be sure to bring those up
on this list. One thing you should probably understand about us is
that we're all about getting things done. Which is one reason we
weren't bothering everyone in here when we started GitHub. We like to
design from first principles, see how good we can do, and then get
feedback from the users. If you're a GitHub user (or have a reason why
you are *not* a GitHub user), we'd love to hear your feedback on ways
we can improve!
Tom Preston-Werner
github.com/mojombo
^ permalink raw reply
* Re: [PATCH 3/6] builtin-help: make it possible to exclude some commands in list_commands()
From: Junio C Hamano @ 2008-07-28 4:29 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Miklos Vajna, git
In-Reply-To: <7vljzmwvww.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> > +struct cmdnames {
>>> > + int alloc;
>>> > + int cnt;
>>> > + struct cmdname {
>>> > + size_t len;
>>> > + char name[1];
>>> > + } **names;
>>> > +};
>>>
>>> I thought we do this kind of thing using FLEX_ARRAY macro. Is there any
>>> reason its use is not appropriate here?
>>
>> I think that came up in the previous review round: the "name" member _is_
>> NUL-terminated, but could have a ".exe" suffix. The "len" member has the
>> length excluding ".exe".
>
> Sorry, but I do understand what you are trying to explain.
Yuck, stupid typo; s/do/do not/.
^ permalink raw reply
* New mailmap file for the kernel
From: Jon Smirl @ 2008-07-28 4:45 UTC (permalink / raw)
To: Git Mailing List
[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]
I made a new mailmap file for the kernel. I'll put it out on LKML tomorrow.
It takes a new philosophy, there is an entry for every email address
in the kernel git tree even if the name associated with it is correct.
I wrote a script and did a lot of manual editing to try and make sure
that every one's name was spelled consistently. There are probably
still some mistakes in it.
As a result of this clean up the number of unique contributors to the
kernel fell from 4,284 to 3,821. A total of 463 errors or about 12% of
all name/email pairs. We clearly need to do some validation.
Now that the file has every valid name/email in it is should be
possible to validate the name/email in all new commits. When a new
developer comes along we'll know it since they won't be in the file.
Maybe Linus will send them a welcome message.
If a developer gets a new email address they need to add a new line
with the address and their name being very careful to make it exactly
match their name that is already in the file. If they want to change
their name they need to make sure and change all copies identically.
Any ideas on how to best modify the scripts to achieve validation?
--
Jon Smirl
jonsmirl@gmail.com
[-- Attachment #2: .mailmap.bz2 --]
[-- Type: application/x-bzip2, Size: 69444 bytes --]
^ permalink raw reply
* [RFC/PATCH v3] merge-base: teach "git merge-base" to accept more than 2 arguments
From: Christian Couder @ 2008-07-28 4:50 UTC (permalink / raw)
To: git, Junio C Hamano, Johannes Schindelin; +Cc: Miklos Vajna, Jakub Narebski
Before this patch "git merge-base" accepted only 2 arguments, so
only merge bases between 2 references could be computed.
The purpose of this patch is to make "git merge-base" accept more
than 2 arguments, so that the merge bases between the first given
reference and all the other references can be computed.
This is easily implemented because the "get_merge_bases_many"
function in "commit.c" already implements the needed computation.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/git-merge-base.txt | 27 +++++++++++++++--------
builtin-merge-base.c | 43 ++++++++++++++++++++++++-------------
commit.h | 2 +
3 files changed, 48 insertions(+), 24 deletions(-)
I only changed the code (according to what Dscho asked) not the
documentation in this version as I had not much time and I need
to think more about it.
diff --git a/Documentation/git-merge-base.txt b/Documentation/git-merge-base.txt
index 1a7ecbf..463c230 100644
--- a/Documentation/git-merge-base.txt
+++ b/Documentation/git-merge-base.txt
@@ -8,26 +8,35 @@ git-merge-base - Find as good common ancestors as possible for a merge
SYNOPSIS
--------
-'git merge-base' [--all] <commit> <commit>
+'git merge-base' [--all] <commit> <commit>...
DESCRIPTION
-----------
-'git-merge-base' finds as good a common ancestor as possible between
-the two commits. That is, given two commits A and B, `git merge-base A
-B` will output a commit which is reachable from both A and B through
-the parent relationship.
+'git-merge-base' finds as good common ancestors as possible between
+the first commit and the other commits. The default behavior is to
+output only one as good as possible common ancestor, called a merge
+base.
+
+For example, given two commits A and B, `git merge-base A B` will
+output a commit which is reachable from both A and B through the
+parent relationship.
+
+Given three commits A, B and C, `git merge-base A B C` will output a
+commit which is reachable through the parent relationship from both A
+and B, or from both A and C.
Given a selection of equally good common ancestors it should not be
relied on to decide in any particular way.
-The 'git-merge-base' algorithm is still in flux - use the source...
-
OPTIONS
-------
--all::
- Output all common ancestors for the two commits instead of
- just one.
+ Output all merge bases for the commits instead of just one. No
+ merge bases in the output should be an ancestor of another
+ merge base in the output. Each common ancestor of the first
+ commit and any of the other commits passed as arguments,
+ should be an ancestor of one of the merge bases in the output.
Author
------
diff --git a/builtin-merge-base.c b/builtin-merge-base.c
index 1cb2925..881363f 100644
--- a/builtin-merge-base.c
+++ b/builtin-merge-base.c
@@ -2,9 +2,11 @@
#include "cache.h"
#include "commit.h"
-static int show_merge_base(struct commit *rev1, struct commit *rev2, int show_all)
+static int show_merge_base(struct commit **rev, int rev_nr, int show_all)
{
- struct commit_list *result = get_merge_bases(rev1, rev2, 0);
+ struct commit_list *result;
+
+ result = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1, 0);
if (!result)
return 1;
@@ -20,13 +22,21 @@ static int show_merge_base(struct commit *rev1, struct commit *rev2, int show_al
}
static const char merge_base_usage[] =
-"git merge-base [--all] <commit-id> <commit-id>";
+"git merge-base [--all] <commit-id> <commit-id>...";
+
+static struct commit *get_commit_reference(const char *arg)
+{
+ unsigned char revkey[20];
+ if (get_sha1(arg, revkey))
+ die("Not a valid object name %s", arg);
+ return lookup_commit_reference(revkey);
+}
int cmd_merge_base(int argc, const char **argv, const char *prefix)
{
- struct commit *rev1, *rev2;
- unsigned char rev1key[20], rev2key[20];
+ struct commit **rev;
int show_all = 0;
+ int rev_nr = 0;
git_config(git_default_config, NULL);
@@ -38,15 +48,18 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix)
usage(merge_base_usage);
argc--; argv++;
}
- if (argc != 3)
+ if (argc < 3)
usage(merge_base_usage);
- if (get_sha1(argv[1], rev1key))
- die("Not a valid object name %s", argv[1]);
- if (get_sha1(argv[2], rev2key))
- die("Not a valid object name %s", argv[2]);
- rev1 = lookup_commit_reference(rev1key);
- rev2 = lookup_commit_reference(rev2key);
- if (!rev1 || !rev2)
- return 1;
- return show_merge_base(rev1, rev2, show_all);
+
+ rev = xmalloc((argc - 1) * sizeof(*rev));
+
+ do {
+ struct commit *r = get_commit_reference(argv[1]);
+ if (!r)
+ return 1;
+ rev[rev_nr++] = r;
+ argc--; argv++;
+ } while (argc > 1);
+
+ return show_merge_base(rev, rev_nr, show_all);
}
diff --git a/commit.h b/commit.h
index 77de962..4829a5c 100644
--- a/commit.h
+++ b/commit.h
@@ -121,6 +121,8 @@ int read_graft_file(const char *graft_file);
struct commit_graft *lookup_commit_graft(const unsigned char *sha1);
extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2, int cleanup);
+extern struct commit_list *get_merge_bases_many(struct commit *one,
+ int n, struct commit **twos, int cleanup);
extern struct commit_list *get_octopus_merge_bases(struct commit_list *in);
extern int register_shallow(const unsigned char *sha1);
--
1.6.0.rc0.43.g00eb.dirty
^ permalink raw reply related
* Re: [PATCH 1/3] git-gui: Adapt discovery of oguilib to execdir 'libexec/git-core'
From: Steffen Prohaska @ 2008-07-28 5:01 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git, Johannes Sixt
In-Reply-To: <20080727212405.GA10075@spearce.org>
On Jul 27, 2008, at 11:24 PM, Shawn O. Pearce wrote:
> Steffen Prohaska <prohaska@zib.de> wrote:
>> The new execdir has is two levels below the root directory, while
>> the old execdir 'bin' was only one level below. This commit
>> adapts the discovery of oguilib that uses relative paths
>> accordingly.
> ...
>> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
>> index 940677c..baccd57 100755
>> --- a/git-gui/git-gui.sh
>> +++ b/git-gui/git-gui.sh
>> @@ -52,7 +52,9 @@ catch {rename send {}} ; # What an evil concept...
>> set oguilib {@@GITGUI_LIBDIR@@}
>> set oguirel {@@GITGUI_RELATIVE@@}
>> if {$oguirel eq {1}} {
>> - set oguilib [file dirname [file dirname [file normalize $argv0]]]
>> + set oguilib [file dirname \
>> + [file dirname \
>> + [file dirname [file normalize $argv0]]]]
>> set oguilib [file join $oguilib share git-gui lib]
>
> Hmmph. This actually comes up incorrectly on my system. The issue
> appears to be `git --exec-path` gives me $prefix/libexec/git-core,
> and git-gui installs its library into $prefix/libexec/share, which
> is wrong. It should have gone to $prefix/share.
I am not seeing this problem because I am installing using the
toplevel makefile, which sets and exports sharedir to $prefix/share.
> I wonder if this is better. Your other two patches seem fine.
>
> --8<--
> [PATCH] git-gui: Correct installation of library to be $prefix/share
>
> We always wanted the library for git-gui to install into the
> $prefix/share directory, not $prefix/libexec/share. All of
> the files in our library are platform independent and may
> be reused across systems, like any other content stored in
> the share directory.
>
> Our computation of where our library should install to was broken
> when git itself started installing to $prefix/libexec/git-core,
> which was one level down from where we expected it to be.
>
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> ---
> Makefile | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index b19fb2d..f72ab6c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -32,6 +32,9 @@ endif
> ifndef gitexecdir
> gitexecdir := $(shell git --exec-path)
> endif
> +ifeq (git-core,$(notdir $(gitexecdir)))
> + gitexecdir := $(patsubst %/,%,$(dir $(gitexecdir)))
> +endif
But gitexecdir has the correct value, no? gitexecdir is used
at several places in the makefile. It seems wrong to strip
'git-core' from gitexecdir. But I must admit that I do not
understand all the details of git-gui's Makefile. So maybe
you know better.
Isn't only the computation of sharedir based on gitexecdir wrong?
> ifndef sharedir
> sharedir := $(dir $(gitexecdir))share
and could be replaced with this (instead of your patch):
ifndef sharedir
+ifeq (git-core,$(notdir $(gitexecdir)))
+ sharedir := $(dir $(patsubst %/,%,$(dir $(gitexecdir))))share
+else
sharedir := $(dir $(gitexecdir))share
endif
+endif
Steffen
^ permalink raw reply
* Re: Bizarre missing changes (git bug?)
From: Linus Torvalds @ 2008-07-28 5:00 UTC (permalink / raw)
To: Roman Zippel; +Cc: Tim Harper, git
In-Reply-To: <Pine.LNX.4.64.0807280141140.6791@localhost.localdomain>
On Mon, 28 Jul 2008, Roman Zippel wrote:
>
> Of course I did:
>
> $ git log --parents --name-only --full-history lib/vsprintf.c | grep -e ^commit | wc -l
> 5929
>
> This is same amount of commits as for gitk.
OF COURSE it's the same numbr of commits. That's what gitk uses. That's
what you *have* to use.
You don't actually understand how git works, do you?
> These are the same 20 commits (with parents) from a simple git-log.
So what? The point I tried to make is that _any_ algorithm that gets the
above case right by actually simplifying the commits "post facto" probably
gets any case right. You tried to find some more interestign case, but you
missed the whole point - even the "simple" case is quite hard enough.
IOW, don't look for anythign more difficult, because if you do, you don't
understand the problem to begin with!
Do you not understand that the problem is that "post facto" isn't actually
acceptable? Have you looked at all at the revision reading code? Hmm?
The regular merge simplification does the simplification _before_ it has
gathered the whole history of commits. And that is really really
important.
And I realize that you don't even seem to understand the difference. But
to simplify it for you, let me give you a challenge. Try this:
time sh -c "git log --parents --full-history lib/vsprintf.c | head"
and if it takes noticeably more than a tenth of a second on my machine,
you lose.
Because that's roughly what it takes right now, and it's what means that
effectively the normal log is instantaneous. It's why you can start
looking at the log without waiting for three seconds, even though the
_full_ log may take three seconds to compute (ok, on my machine it takes
2.3s, but whatever).
And it's why gitk can start printing out the history _before_ three
seconds has passed. And that's really really important.
Try it. Really. Just do "gitk lib/vsprintf.c" and look at how it does
things incrementally. It doesn't wait for a couple of seconds and then
show things.
Absolutely EVERYTHING in git would be totally trivial if you did it all
based on generating first the whole history, and then simplifying it from
there. But git would be unusably slow in real life, and it would scale
_horribly_ badly with history size.
So _all_ the normal code actually generates the history INCREMENTALLY, and
it absolutely _has_ to work that way.
Linus
^ permalink raw reply
* Re: Bizarre missing changes (git bug?)
From: Linus Torvalds @ 2008-07-28 5:30 UTC (permalink / raw)
To: Roman Zippel; +Cc: Tim Harper, git
In-Reply-To: <alpine.LFD.1.10.0807272148030.3486@nehalem.linux-foundation.org>
On Sun, 27 Jul 2008, Linus Torvalds wrote:
>
> And it's why gitk can start printing out the history _before_ three
> seconds has passed. And that's really really important.
Btw, the reason it's really really important is that "three seconds" can
actually easily be "three minutes" - if the project is big, or if you
simply don't have everything in the cache, so you actually need to do tons
of IO to generate the whole history.
So every normal operation absolutely _must_ be incremental and not rely on
any calculation of the whole history in order to then simplify it.
Of course, post-processing is fine for some things. For example, in the
thread I pointed you to originally (see filter-branch + full-history in
google, or look in some git archive) I suggested a post-processing of the
merge history for filter-branch. I suspect it's very acceptable for _that_
kind of use to "batch" things up and not do them with partial knowledge.
But this incremental thing is why I for example suggest people should use
"git gui blame" instead of "git blame" when looking for problems - because
the latter cannot be done incrementally, and as a result can cause really
irritating delays (exactly because it basically needs to synchronously
walk back to the beginning of history).
The kernel repo, btw, is pretty small in this regard. The cases that
caused much more pain were the insane KDE ones that were something like
ten times the size. We've optimized things pretty aggressively, but...
Btw, if I sound irritated, it's because we had all these discussions about
three _years_ ago when git got started. This is not a new issue. It's
hard.
I've been pushing on people to do things incrementally very hard over the
last few years because it's such a _huge_ usability issue.
For example, I've pointed you to the incremental nature of "gitk" as an
example of how things should work, but that's actually fairly recent: it
wasn't that long ago that "gitk" used to pass in "--topo-order" or
"--date-order" to the core git revision machinery, and that actually is
another of those "global" operations that you need the whole history for.
So gitk actually used to pause for three seconds (or ten. or thirty)
before it would show the results. I'm really happy to report that Paul
finally did the (trivial) topo-sort in gitk, meaning that he could re-sort
it as necessary and keep things incremental. It was one of my biggest UI
gripes for the longest time (and I wasted time adding a special "partial
output mode" that gitk didn't even then end up using because Paul did
things the right way).
Btw, from a git log viewer standpoint, the "merge history simplification"
is all the exact same problem as the "--topo-order" flag is: you could
use the (incremental and very verbose)
git log --full-history --parents
output as the base-line, and then you could do the commit simplification
of things interactively.
But "git log" itself cannot do it by default, since that would mean that
git log itself would have to wait for the whole history to be generated.
That's because output to a pipe is fundamentally linear (ie it cannot
"re-write" the things it has already shown as it finds a simplification:
there is no incremental way to rewrite things "after the fact").
Linus
^ permalink raw reply
* Re: [RFC/PATCH v3] merge-base: teach "git merge-base" to accept more than 2 arguments
From: Junio C Hamano @ 2008-07-28 5:37 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Johannes Schindelin, Miklos Vajna, Jakub Narebski
In-Reply-To: <20080728065023.743930d6.chriscool@tuxfamily.org>
Christian Couder <chriscool@tuxfamily.org> writes:
> Before this patch "git merge-base" accepted only 2 arguments, so
> only merge bases between 2 references could be computed.
>
> The purpose of this patch is to make "git merge-base" accept more
> than 2 arguments, so that the merge bases between the first given
> reference and all the other references can be computed.
I have trouble with this wording, but I'll comment on the documentation
part in a separate message.
> +static struct commit *get_commit_reference(const char *arg)
> +{
> + unsigned char revkey[20];
> + if (get_sha1(arg, revkey))
> + die("Not a valid object name %s", arg);
> + return lookup_commit_reference(revkey);
> +}
This returns a NULL when you feed a tree to the command, and...
> int cmd_merge_base(int argc, const char **argv, const char *prefix)
> {
> + struct commit **rev;
> int show_all = 0;
> + int rev_nr = 0;
>
> git_config(git_default_config, NULL);
>
> @@ -38,15 +48,18 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix)
> usage(merge_base_usage);
> argc--; argv++;
> }
> + if (argc < 3)
> usage(merge_base_usage);
> +
> + rev = xmalloc((argc - 1) * sizeof(*rev));
> +
> + do {
> + struct commit *r = get_commit_reference(argv[1]);
> + if (!r)
> + return 1;
... the command silently exits with 1.
^ permalink raw reply
* Re: [PATCH] make sure parsed wildcard refspec ends with slash
From: Jeff King @ 2008-07-28 5:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Daniel Barkalow, git
In-Reply-To: <7vsktv3l9k.fsf_-_@gitster.siamese.dyndns.org>
On Sat, Jul 26, 2008 at 11:15:51PM -0700, Junio C Hamano wrote:
> > I have a nagging suspicion that it might be a simpler and cleaner change
> > to change parse_refspec_internal() to keep the trailing slash, instead of
> > dropping it. Then the check you added is not needed (the trailing slash
> > guarantees that the pattern matches at the hierarchy boundary), neither
> > any of the change in this patch.
>
> This is the other variant, and it turns out that I was right. Among the
> 64-18 = 46 new lines, 30 are from the new test file. Two existing
> "matching part is followed by '/'" tests are removed.
Looks like you have already applied it, but I will chime in that of the
two, I think this is the more sensible change. Stripping the '/' felt
like a loss of information to me. IIRC, when "support only /*" was
discussed, the thinking was "let's keep it tight now, and we can loosen
it later." Keeping the '/' means that the code is much more sane if
other globbing rules come in the future.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH v2] merge-base: teach "git merge-base" to accept more than 2 arguments
From: Christian Couder @ 2008-07-28 5:49 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Miklos Vajna
In-Reply-To: <alpine.DEB.1.00.0807271631470.5526@eeepc-johanness>
Le dimanche 27 juillet 2008, Johannes Schindelin a écrit :
> Hi,
>
> On Sun, 27 Jul 2008, Christian Couder wrote:
> > diff --git a/builtin-merge-base.c b/builtin-merge-base.c
> > index 1cb2925..f2c9756 100644
> > --- a/builtin-merge-base.c
> > +++ b/builtin-merge-base.c
> > @@ -38,15 +48,22 @@ int cmd_merge_base(int argc, const char **argv,
> > const char *prefix) usage(merge_base_usage);
> > argc--; argv++;
> > }
> > - if (argc != 3)
> > + if (argc < 3)
> > usage(merge_base_usage);
> > - if (get_sha1(argv[1], rev1key))
> > - die("Not a valid object name %s", argv[1]);
> > - if (get_sha1(argv[2], rev2key))
> > - die("Not a valid object name %s", argv[2]);
> > - rev1 = lookup_commit_reference(rev1key);
> > - rev2 = lookup_commit_reference(rev2key);
> > - if (!rev1 || !rev2)
> > +
> > + rev1 = get_commit_reference(argv[1]);
> > + if (!rev1)
> > return 1;
>
> Why do you special case rev1? Is it so special? Just handle it together
> with all of the other arguments!
>
> IOW have one commit array, and do not call it "prev".
Ok, I have done that in v3.
> > - return show_merge_base(rev1, rev2, show_all);
> > + argc--; argv++;
> > +
> > + do {
> > + struct commit *rev2 = get_commit_reference(argv[1]);
> > + if (!rev2)
> > + return 1;
> > + ALLOC_GROW(prev2, prev2_nr + 1, prev2_alloc);
> > + prev2[prev2_nr++] = rev2;
> > + argc--; argv++;
> > + } while (argc > 1);
>
> Now, this is ugly. You know beforehand the _exact_ number of arguments,
> and yet you dynamically grow the array?
You are right. I guess I dealt too much with complex parsing of arguments
where you can have options everywhere, and perhaps I also felt somewhat
guilty for not having used ALLOC_GROW before, or something.
> Also, why do you use a do { }
> while(), when a for () would be much, much clearer?
Well, we already know that there are at least 2 commits to parse, so it
feels a little bit wastefull to check first again. Also the code to parse
the [--all|-a] option before use a while loop with "argc--; argv++;" at the
end, so I think it is more coherent like that.
Thanks,
Christian.
> BTW I seem to recall that get_merge_bases_many() was _not_ the same as
> get_merge_octopus(). Could you please remind me what _many() does?
>
> Ciao,
> Dscho
^ permalink raw reply
* Re: [RFC/PATCH v3] merge-base: teach "git merge-base" to accept more than 2 arguments
From: Junio C Hamano @ 2008-07-28 5:46 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Johannes Schindelin, Miklos Vajna, Jakub Narebski
In-Reply-To: <20080728065023.743930d6.chriscool@tuxfamily.org>
Christian Couder <chriscool@tuxfamily.org> writes:
> Before this patch "git merge-base" accepted only 2 arguments, so
> only merge bases between 2 references could be computed.
>
> The purpose of this patch is to make "git merge-base" accept more
> than 2 arguments, so that the merge bases between the first given
> reference and all the other references can be computed.
After thinking about this a bit more, I think that saying "we now allow
computing merge base between more than 2" is misleading.
I ended up rewriting almost all the lines. The points are:
* The command is still about computing merge-bases between two commits,
and one of these two commits is the first command line argument as
before. With more than one remaining commits on the command line, you
can specify a hypothetical commit that is a merge between all of them.
* We did not formally define what a merge-base is. Define it.
* With a proper definition of merge-base as "best common ancestors", with
the definition of "good/better/best" between common ancestors, we do
not have to say anything redundant about --all. It outputs all merge
bases, as opposed to one chosen at random.
* Have definition and give illustrations in a new Discussion section.
---
Documentation/git-merge-base.txt | 77 ++++++++++++++++++++++++++++++++-----
1 files changed, 66 insertions(+), 11 deletions(-)
diff --git a/Documentation/git-merge-base.txt b/Documentation/git-merge-base.txt
index 1a7ecbf..fd4b5c9 100644
--- a/Documentation/git-merge-base.txt
+++ b/Documentation/git-merge-base.txt
@@ -8,26 +8,81 @@ git-merge-base - Find as good common ancestors as possible for a merge
SYNOPSIS
--------
-'git merge-base' [--all] <commit> <commit>
+'git merge-base' [--all] <commit> <commit>...
DESCRIPTION
-----------
-'git-merge-base' finds as good a common ancestor as possible between
-the two commits. That is, given two commits A and B, `git merge-base A
-B` will output a commit which is reachable from both A and B through
-the parent relationship.
+'git-merge-base' finds best common ancestor(s) between two commits to use
+in a three-way merge. One common ancestor is 'better' than another common
+ancestor if the latter is an ancestor of the former. A common ancestor
+that does not have any better common ancestor than it is a 'best common
+ancestor', i.e. a 'merge base'. Note that there can be more than one
+merge bases between two commits.
-Given a selection of equally good common ancestors it should not be
-relied on to decide in any particular way.
-
-The 'git-merge-base' algorithm is still in flux - use the source...
+Among the two commits to compute their merge bases, one is specified by
+the first commit argument on the command line; the other commit is a
+(possibly hypothetical) commit that is a merge across all the remaining
+commits on the command line. As the most common special case, giving only
+two commits from the command line means computing the merge base between
+the given two commits.
OPTIONS
-------
--all::
- Output all common ancestors for the two commits instead of
- just one.
+ Output all merge bases for the commits, instead of just one.
+
+DISCUSSION
+----------
+
+Given two commits 'A' and 'B', `git merge-base A B` will output a commit
+which is reachable from both 'A' and 'B' through the parent relationship.
+
+For example, with this topology:
+
+ o---o---o---B
+ /
+ ---o---1---o---o---o---A
+
+the merge base between 'A' and 'B' is '1'.
+
+Given three commits 'A', 'B' and 'C', `git merge-base A B C` will compute the
+merge base between 'A' and an hypothetical commit 'M', which is a merge
+between 'B' and 'C'. For example, with this topology:
+
+ o---o---o---o---C
+ /
+ / o---o---o---B
+ / /
+ ---2---1---o---o---o---A
+
+the result of `git merge-base A B C` is '1'. This is because the
+equivalent topology with a merge commit 'M' between 'B' and 'C' is:
+
+
+ o---o---o---o---o
+ / \
+ / o---o---o---o---M
+ / /
+ ---2---1---o---o---o---A
+
+and the result of `git merge-base A M` is '1'. Commit '2' is also a
+common ancestor between 'A' and 'M', but '1' is a better common ancestor,
+because '2' is an ancestor of '1'. Hence, '2' is not a merge base.
+
+When the history involves criss-cross merges, there can be more than one
+'best' common ancestors between two commits. For example, with this
+topology:
+
+ ---1---o---A
+ \ /
+ X
+ / \
+ ---2---o---o---B
+
+both '1' and '2' are merge-base of A and B. Neither one is better than
+the other (both are 'best' merge base). When `--all` option is not given,
+it is unspecified which best one is output.
Author
------
^ permalink raw reply related
* [PATCH 1/2] Refactor, adding prepare_git_cmd(const char **argv)
From: Steffen Prohaska @ 2008-07-28 5:50 UTC (permalink / raw)
To: Johannes Sixt
Cc: git, Junio C Hamano, Johannes Schindelin, Shawn O. Pearce,
Steffen Prohaska
prepare_git_cmd(const char **argv) adds a first entry "git" to
the array argv. The new array is allocated on the heap. It's
the caller's responsibility to release it with free(). The code
was already present in execv_git_cmd() but could not be used from
outside. Now it can also be called for preparing the command list
in the MinGW codepath in run-command.c.
Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
exec_cmd.c | 7 ++++++-
exec_cmd.h | 1 +
2 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/exec_cmd.c b/exec_cmd.c
index 0ed768d..ce6741e 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -78,7 +78,7 @@ void setup_path(void)
strbuf_release(&new_path);
}
-int execv_git_cmd(const char **argv)
+const char **prepare_git_cmd(const char **argv)
{
int argc;
const char **nargv;
@@ -91,6 +91,11 @@ int execv_git_cmd(const char **argv)
for (argc = 0; argv[argc]; argc++)
nargv[argc + 1] = argv[argc];
nargv[argc + 1] = NULL;
+ return nargv;
+}
+
+int execv_git_cmd(const char **argv) {
+ const char **nargv = prepare_git_cmd(argv);
trace_argv_printf(nargv, "trace: exec:");
/* execvp() can only ever return if it fails */
diff --git a/exec_cmd.h b/exec_cmd.h
index 0c46cd5..594f961 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -5,6 +5,7 @@ extern void git_set_argv_exec_path(const char *exec_path);
extern void git_set_argv0_path(const char *path);
extern const char* git_exec_path(void);
extern void setup_path(void);
+extern const char **prepare_git_cmd(const char **argv);
extern int execv_git_cmd(const char **argv); /* NULL terminated */
extern int execl_git_cmd(const char *cmd, ...);
extern const char *system_path(const char *path);
--
1.6.0.rc0.79.gb0320
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox