* [PATCH 0/2] Avoid run_command() for recursive in builtin-merge @ 2008-08-10 13:20 Miklos Vajna 2008-08-10 13:20 ` [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins Miklos Vajna 2008-08-11 15:03 ` [PATCH] builtin-revert.c: Make use of merge_recursive() Stephan Beyer 0 siblings, 2 replies; 38+ messages in thread From: Miklos Vajna @ 2008-08-10 13:20 UTC (permalink / raw) To: git Hi, The idea (by Dscho) is simple: builtin-merge-recursive already has a merge_recursive() function that is _almost_ ready to be called from builtins without fork+exec, so make use of it. And yes, of course this is not for 1.6.0, I just wanted to send this out before the "suggested 'pencils down' date" of GSoC. Miklos Vajna (2): merge-recursive: prepare merge_recursive() to be called from builtins builtin-merge: avoid run_command_v_opt() for recursive builtin-merge-recursive.c | 19 +++++++--- builtin-merge.c | 82 +++++++++++++++++++++++++++++++-------------- 2 files changed, 70 insertions(+), 31 deletions(-) ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins 2008-08-10 13:20 [PATCH 0/2] Avoid run_command() for recursive in builtin-merge Miklos Vajna @ 2008-08-10 13:20 ` Miklos Vajna 2008-08-10 13:20 ` [PATCH 2/2] builtin-merge: avoid run_command_v_opt() for recursive Miklos Vajna 2008-08-11 15:13 ` [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins Stephan Beyer 2008-08-11 15:03 ` [PATCH] builtin-revert.c: Make use of merge_recursive() Stephan Beyer 1 sibling, 2 replies; 38+ messages in thread From: Miklos Vajna @ 2008-08-10 13:20 UTC (permalink / raw) To: git When other builtins call merge_recursive(), they would have to handle the GIT_MERGE_VERBOSITY environment variable, causing a code duplication. Same story for the git_config() call. It's better to do it when merge_recursive() is called the first time. Signed-off-by: Miklos Vajna <vmiklos@frugalware.org> --- builtin-merge-recursive.c | 19 +++++++++++++------ 1 files changed, 13 insertions(+), 6 deletions(-) diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c index 43e55bf..09aa830 100644 --- a/builtin-merge-recursive.c +++ b/builtin-merge-recursive.c @@ -1218,6 +1218,8 @@ static struct commit_list *reverse_commit_list(struct commit_list *list) return next; } +static int merge_config(const char *var, const char *value, void *cb); + /* * Merge the commits h1 and h2, return the resulting virtual * commit object and a flag indicating the cleanness of the merge. @@ -1233,6 +1235,17 @@ int merge_recursive(struct commit *h1, struct commit *merged_common_ancestors; struct tree *mrtree = mrtree; int clean; + static int initial = 1; + + if (initial) { + git_config(merge_config, NULL); + if (getenv("GIT_MERGE_VERBOSITY")) + verbosity = strtol(getenv("GIT_MERGE_VERBOSITY"), NULL, 10); + if (verbosity >= 5) + buffer_output = 0; + + initial = 0; + } if (show(4)) { output(4, "Merging:"); @@ -1369,10 +1382,6 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) subtree_merge = 1; } - git_config(merge_config, NULL); - if (getenv("GIT_MERGE_VERBOSITY")) - verbosity = strtol(getenv("GIT_MERGE_VERBOSITY"), NULL, 10); - if (argc < 4) die("Usage: %s <base>... -- <head> <remote> ...\n", argv[0]); @@ -1384,8 +1393,6 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) } if (argc - i != 3) /* "--" "<head>" "<remote>" */ die("Not handling anything other than two heads merge."); - if (verbosity >= 5) - buffer_output = 0; branch1 = argv[++i]; branch2 = argv[++i]; -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 2/2] builtin-merge: avoid run_command_v_opt() for recursive 2008-08-10 13:20 ` [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins Miklos Vajna @ 2008-08-10 13:20 ` Miklos Vajna 2008-08-11 18:47 ` Junio C Hamano 2008-08-11 15:13 ` [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins Stephan Beyer 1 sibling, 1 reply; 38+ messages in thread From: Miklos Vajna @ 2008-08-10 13:20 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin The try_merge_strategy() function always ran the strategy in a separate process, though this is not always necessary. The recursive strategy can be called without a fork(). This patch adds a check, and calls recursive in the same process without wasting resources. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org> --- Adding the signoff of Dscho as well, as he wrote the last hunk. builtin-merge.c | 82 ++++++++++++++++++++++++++++++++++++++----------------- 1 files changed, 57 insertions(+), 25 deletions(-) diff --git a/builtin-merge.c b/builtin-merge.c index dde0c7e..2789a78 100644 --- a/builtin-merge.c +++ b/builtin-merge.c @@ -22,6 +22,7 @@ #include "log-tree.h" #include "color.h" #include "rerere.h" +#include "merge-recursive.h" #define DEFAULT_TWOHEAD (1<<0) #define DEFAULT_OCTOPUS (1<<1) @@ -511,28 +512,55 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, struct commit_list *j; struct strbuf buf; - args = xmalloc((4 + commit_list_count(common) + - commit_list_count(remoteheads)) * sizeof(char *)); - strbuf_init(&buf, 0); - strbuf_addf(&buf, "merge-%s", strategy); - args[i++] = buf.buf; - for (j = common; j; j = j->next) - args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1)); - args[i++] = "--"; - args[i++] = head_arg; - for (j = remoteheads; j; j = j->next) - args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1)); - args[i] = NULL; - ret = run_command_v_opt(args, RUN_GIT_CMD); - strbuf_release(&buf); - i = 1; - for (j = common; j; j = j->next) - free((void *)args[i++]); - i += 2; - for (j = remoteheads; j; j = j->next) - free((void *)args[i++]); - free(args); - return -ret; + if (!strcmp(strategy, "recursive")) { + int clean; + struct commit *result; + struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); + int index_fd; + struct commit_list *reversed = NULL; + + if (remoteheads->next) { + error("Not handling anything other than two heads merge."); + return 2; + } + + for (j = common; j; j = j->next) + commit_list_insert(j->item, &reversed); + + index_fd = hold_locked_index(lock, 1); + clean = merge_recursive(lookup_commit(head), + remoteheads->item, head_arg, + (const char *)remoteheads->item->util, + reversed, &result); + if (active_cache_changed && + (write_cache(index_fd, active_cache, active_nr) || + commit_locked_index(lock))) + die ("unable to write %s", get_index_file()); + return clean ? 0 : 1; + } else { + args = xmalloc((4 + commit_list_count(common) + + commit_list_count(remoteheads)) * sizeof(char *)); + strbuf_init(&buf, 0); + strbuf_addf(&buf, "merge-%s", strategy); + args[i++] = buf.buf; + for (j = common; j; j = j->next) + args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1)); + args[i++] = "--"; + args[i++] = head_arg; + for (j = remoteheads; j; j = j->next) + args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1)); + args[i] = NULL; + ret = run_command_v_opt(args, RUN_GIT_CMD); + strbuf_release(&buf); + i = 1; + for (j = common; j; j = j->next) + free((void *)args[i++]); + i += 2; + for (j = remoteheads; j; j = j->next) + free((void *)args[i++]); + free(args); + return -ret; + } } static void count_diff_files(struct diff_queue_struct *q, @@ -670,7 +698,9 @@ static int finish_automerge(struct commit_list *common, struct strbuf buf = STRBUF_INIT; unsigned char result_commit[20]; - free_commit_list(common); + if (strcmp(wt_strategy, "recursive")) + /* recursive already freed it */ + free_commit_list(common); if (allow_fast_forward) { parents = remoteheads; commit_list_insert(lookup_commit(head), &parents); @@ -873,12 +903,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix) for (i = 0; i < argc; i++) { struct object *o; + struct commit *commit; o = peel_to_type(argv[i], 0, NULL, OBJ_COMMIT); if (!o) die("%s - not something we can merge", argv[i]); - remotes = &commit_list_insert(lookup_commit(o->sha1), - remotes)->next; + commit = lookup_commit(o->sha1); + commit->util = (void *)argv[i]; + remotes = &commit_list_insert(commit, remotes)->next; strbuf_addf(&buf, "GITHEAD_%s", sha1_to_hex(o->sha1)); setenv(buf.buf, argv[i], 1); -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 2/2] builtin-merge: avoid run_command_v_opt() for recursive 2008-08-10 13:20 ` [PATCH 2/2] builtin-merge: avoid run_command_v_opt() for recursive Miklos Vajna @ 2008-08-11 18:47 ` Junio C Hamano 2008-08-11 19:07 ` Miklos Vajna 0 siblings, 1 reply; 38+ messages in thread From: Junio C Hamano @ 2008-08-11 18:47 UTC (permalink / raw) To: Miklos Vajna; +Cc: git, Johannes Schindelin Miklos Vajna <vmiklos@frugalware.org> writes: > The try_merge_strategy() function always ran the strategy in a separate > process, though this is not always necessary. The recursive strategy can > be called without a fork(). This patch adds a check, and calls recursive > in the same process without wasting resources. Yes, it saves a fork, but is this really worth it in the bigger picture? Doesn't the current code structure have benefit of allowing git-merge itself do necessary clean-up action when merge-recursive calls any of the die() it has in many places? ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 2/2] builtin-merge: avoid run_command_v_opt() for recursive 2008-08-11 18:47 ` Junio C Hamano @ 2008-08-11 19:07 ` Miklos Vajna 2008-08-11 20:03 ` Junio C Hamano 0 siblings, 1 reply; 38+ messages in thread From: Miklos Vajna @ 2008-08-11 19:07 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Johannes Schindelin [-- Attachment #1: Type: text/plain, Size: 1307 bytes --] On Mon, Aug 11, 2008 at 11:47:07AM -0700, Junio C Hamano <gitster@pobox.com> wrote: > Yes, it saves a fork, but is this really worth it in the bigger picture? > > Doesn't the current code structure have benefit of allowing git-merge > itself do necessary clean-up action when merge-recursive calls any of the > die() it has in many places? As far as I see in most cases merge-recursive does not call die(). Cases when it does are like: - broken snprintf - cache_tree_fully_valid() or cache_tree_update() fails - diff_setup_done() fails - flush_buffer() fails - read_sha1_file() fails - ll_merge() fails etc. In short, I think there are two cases when a die() would be problematic inside merge-recursive when calling it from builtin-merge: - merge-recursive can't handle a merge, but an other strategy could do. This is the case when doing an octopus merge but in that case merge_recursive() is not called at all. - merge-recursive results in conflicts, but an other strategy could handle the merge without conflicts. In this case die() isn't used either, so this will not be a problem. So I don't think there is a case when a die() inside merge-recursive would occur, but an other strategy would handle the merge properly. Unless I missed something. ;-) [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 2/2] builtin-merge: avoid run_command_v_opt() for recursive 2008-08-11 19:07 ` Miklos Vajna @ 2008-08-11 20:03 ` Junio C Hamano 2008-08-11 20:45 ` Miklos Vajna 0 siblings, 1 reply; 38+ messages in thread From: Junio C Hamano @ 2008-08-11 20:03 UTC (permalink / raw) To: Miklos Vajna; +Cc: git, Johannes Schindelin Miklos Vajna <vmiklos@frugalware.org> writes: > So I don't think there is a case when a die() inside merge-recursive > would occur, but an other strategy would handle the merge properly. Normal case is fine. I was not worried too much about "the other strategy will" case, but isn't the general structure of git-merge driver be: - do some set-up computation, and leave info in .git/ - call strategy - depending on how it exits, perform further operation (such as writing out tree out of index and updating HEAD) using the info in .git it left before it called strategy, and clean up whatever was done in the first step (things like "drop the index lock"?). Dying in the strategy and not allowing the clean-up was what I was worried about. If you can guarantee that you are not going to leave the repository in a wedged state, calling merge-recursive internally is perfectly fine, but the codepaths involved need to be carefully vetted for that. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 2/2] builtin-merge: avoid run_command_v_opt() for recursive 2008-08-11 20:03 ` Junio C Hamano @ 2008-08-11 20:45 ` Miklos Vajna 2008-08-11 20:48 ` [PATCH] Add a new test to ensure merging a submodule is handled properly Miklos Vajna 0 siblings, 1 reply; 38+ messages in thread From: Miklos Vajna @ 2008-08-11 20:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Johannes Schindelin [-- Attachment #1: Type: text/plain, Size: 1202 bytes --] On Mon, Aug 11, 2008 at 01:03:24PM -0700, Junio C Hamano <gitster@pobox.com> wrote: > I was not worried too much about "the other strategy will" case, but isn't > the general structure of git-merge driver be: > > - do some set-up computation, and leave info in .git/ > - call strategy > - depending on how it exits, perform further operation (such as > writing out tree out of index and updating HEAD) using the info in > .git it left before it called strategy, and clean up whatever was > done in the first step (things like "drop the index lock"?). atexit() will take care of this. I'll send a testcase for this in a bit, I just wrote it because I was not exactly sure about this. > Dying in the strategy and not allowing the clean-up was what I was worried > about. If you can guarantee that you are not going to leave the > repository in a wedged state, calling merge-recursive internally is > perfectly fine, but the codepaths involved need to be carefully vetted for > that. As far as I see there are no such codepaths, but I may be wrong; in that case new tests will be needed as well, since the current 'make test' output is happy. ;-) [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] Add a new test to ensure merging a submodule is handled properly. 2008-08-11 20:45 ` Miklos Vajna @ 2008-08-11 20:48 ` Miklos Vajna 0 siblings, 0 replies; 38+ messages in thread From: Miklos Vajna @ 2008-08-11 20:48 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Johannes Schindelin In this test we cause a directory / submodule conflict then we check if the index is unlocked properly. Signed-off-by: Miklos Vajna <vmiklos@frugalware.org> --- I'm no exactly sure if this should be included or not, I just wrote it to make sure that a die() insure merge-recursive will not cause an unlocked index at the end. t/t7607-merge-submodule.sh | 40 ++++++++++++++++++++++++++++++++++++++++ 1 files changed, 40 insertions(+), 0 deletions(-) create mode 100755 t/t7607-merge-submodule.sh diff --git a/t/t7607-merge-submodule.sh b/t/t7607-merge-submodule.sh new file mode 100755 index 0000000..552930a --- /dev/null +++ b/t/t7607-merge-submodule.sh @@ -0,0 +1,40 @@ +#!/bin/sh + +test_description='git-merge + +Testing if a directory / submodule conflict is handled properly.' + +. ./test-lib.sh + +test_expect_success 'setup' ' + mkdir lib && + cd lib && + git init && + echo c1 >c1.c && + git add c1.c && + git commit -m "submodule init" && + mkdir ../main && + cd ../main && + git init && + echo main >main.c && + git add main.c && + git commit -m "c0: main init" && + git tag c0 && + mkdir lib && + echo lib >lib/c1.c && + git add lib/c1.c && + git commit -m "c1: lib init" && + git tag c1 && + git reset --hard c0 && + git submodule add "`pwd`/../lib" lib && + git commit -m "c2: add submodule" && + git tag c2 +' + +test_expect_success 'dir/submodule conflict' ' + git reset --hard c1 && + test_must_fail git merge c2 && + test ! -f .git/index.lock +' + +test_done -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins 2008-08-10 13:20 ` [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins Miklos Vajna 2008-08-10 13:20 ` [PATCH 2/2] builtin-merge: avoid run_command_v_opt() for recursive Miklos Vajna @ 2008-08-11 15:13 ` Stephan Beyer 2008-08-11 16:46 ` Miklos Vajna 2008-08-11 19:53 ` Junio C Hamano 1 sibling, 2 replies; 38+ messages in thread From: Stephan Beyer @ 2008-08-11 15:13 UTC (permalink / raw) To: Miklos Vajna; +Cc: git Hi, Miklos Vajna wrote: > When other builtins call merge_recursive(), they would have to handle > the GIT_MERGE_VERBOSITY environment variable, causing a code > duplication. Same story for the git_config() call. It's better to do it > when merge_recursive() is called the first time. Hmm, I have the long-run vision that we have a nice libgit some day, with merge_recursive() being part of it. And I'm a little unsure if libified functions should rely on environment variables. So I'm wondering if the verbosity should be set in the caller functions of merge_recursive(), i.e. that cmd_merge_recursive() and cmd_merge() (or another part of builtin-merge.c) does the getenv("GIT_MERGE_VERBOSITY") stuff and a verbosity value could be a new argument to merge_recursive(). Then other merge_recursive() users don't need to setenv("GIT_MERGE_VERBOSITY", "3", 1) or something, they just pass 3 as the verbosity value. Just a thought, Stephan PS: Your patch looks fine to me. -- Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins 2008-08-11 15:13 ` [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins Stephan Beyer @ 2008-08-11 16:46 ` Miklos Vajna 2008-08-11 19:53 ` Junio C Hamano 1 sibling, 0 replies; 38+ messages in thread From: Miklos Vajna @ 2008-08-11 16:46 UTC (permalink / raw) To: Stephan Beyer; +Cc: git [-- Attachment #1: Type: text/plain, Size: 600 bytes --] On Mon, Aug 11, 2008 at 05:13:03PM +0200, Stephan Beyer <s-beyer@gmx.net> wrote: > Hmm, I have the long-run vision that we have a nice libgit some day, That would be nice, but in that case I would start avoiding die() which is an awful amount of work... (There were multiple threads on the list previously.) > with merge_recursive() being part of it. And I'm a little unsure if > libified functions should rely on environment variables. Well, many libs do this. Random example: ld.so relies on LD_LIBRARY_PATH as well. Anyway sure, using function parameters instead of env vars is more elegant. [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins 2008-08-11 15:13 ` [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins Stephan Beyer 2008-08-11 16:46 ` Miklos Vajna @ 2008-08-11 19:53 ` Junio C Hamano 2008-08-11 20:46 ` Stephan Beyer 1 sibling, 1 reply; 38+ messages in thread From: Junio C Hamano @ 2008-08-11 19:53 UTC (permalink / raw) To: Stephan Beyer; +Cc: Miklos Vajna, git Stephan Beyer <s-beyer@gmx.net> writes: > Hmm, I have the long-run vision that we have a nice libgit some day, > with merge_recursive() being part of it. And I'm a little unsure if > libified functions should rely on environment variables. I think the environment variable is the least of your worries. I do not think anybody has vetted if it is safe to call merge_recursive() more than once in a single run of a process. Things to watch out for are the use of static variables (e.g. current_{file,directory}_set that are used for its (semi-broken) D/F conflict detection), its liberal use of die(), leaking of "virtual commit", to name a few. They are all perfectly fine programming constructs when we assume this is a one-shot "run and clean up with exit" program, but whoever wants to libify it needs to arrange them to be cleaned up inside the "library" without making the code too ugly nor one-shot use too expensive. But such a clean-up may not be too bad as I initially feared, I suppose. After a cursory look in builtin-merge-recursive.c, at least it does not seem to mark objects with random object flags, expecting that nobody else will look at them after it exits --- which would have been very expensive to clean up after the fact. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins 2008-08-11 19:53 ` Junio C Hamano @ 2008-08-11 20:46 ` Stephan Beyer 0 siblings, 0 replies; 38+ messages in thread From: Stephan Beyer @ 2008-08-11 20:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: Miklos Vajna, git Hi, Junio C Hamano wrote: > Stephan Beyer <s-beyer@gmx.net> writes: > > > Hmm, I have the long-run vision that we have a nice libgit some day, > > with merge_recursive() being part of it. And I'm a little unsure if > > libified functions should rely on environment variables. > > I think the environment variable is the least of your worries. > > I do not think anybody has vetted if it is safe to call merge_recursive() > more than once in a single run of a process. I use it in builtin-sequencer.c, without yet spending much effort in libifying it. For the tests, it works. (Each "pick" calls a merge_recursive(), each threeway-merge needing "patch" does.) But I should either spend some effort in improving the libification or, if this gets too time-consuming before Aug 17, I revert the commits that make use of merge_recursive() instead of running git-merge-recursive. > leaking of "virtual commit", Apropos, I think I have some tiny leak fixes lying around here. Would such patches go into 1.6.0 or is it too dangerous? (A free() that fixes a leak in one place could cause a segmentation fault in another place. Of course I can try to explain in the commit messages, why those are leaks in every case.) > But such a clean-up may not be too bad as I initially feared, I suppose. I suppose, too. Regards. -- Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] builtin-revert.c: Make use of merge_recursive() 2008-08-10 13:20 [PATCH 0/2] Avoid run_command() for recursive in builtin-merge Miklos Vajna 2008-08-10 13:20 ` [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins Miklos Vajna @ 2008-08-11 15:03 ` Stephan Beyer 2008-08-11 15:47 ` Johannes Schindelin 1 sibling, 1 reply; 38+ messages in thread From: Stephan Beyer @ 2008-08-11 15:03 UTC (permalink / raw) To: git; +Cc: Miklos Vajna, Stephan Beyer Cherry-pick and revert always ran the merging in a separate process. This patch makes cherry-pick/revert call merge_recursive() instead of running git-merge-recursive. Signed-off-by: Stephan Beyer <s-beyer@gmx.net> --- Hi, I wonder if this patch fits in line. builtin-merge-recursive.c | 2 +- builtin-revert.c | 41 ++++++++++++++++++++++------------------- merge-recursive.h | 1 + 3 files changed, 24 insertions(+), 20 deletions(-) diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c index 09aa830..d8bd21f 100644 --- a/builtin-merge-recursive.c +++ b/builtin-merge-recursive.c @@ -1327,7 +1327,7 @@ static const char *better_branch_name(const char *branch) return name ? name : branch; } -static struct commit *get_ref(const char *ref) +struct commit *get_ref(const char *ref) { unsigned char sha1[20]; struct object *object; diff --git a/builtin-revert.c b/builtin-revert.c index 27881e9..c54cf8a 100644 --- a/builtin-revert.c +++ b/builtin-revert.c @@ -11,6 +11,7 @@ #include "cache-tree.h" #include "diff.h" #include "revision.h" +#include "merge-recursive.h" /* * This implements the builtins revert and cherry-pick. @@ -200,18 +201,14 @@ static void set_author_ident_env(const char *message) sha1_to_hex(commit->object.sha1)); } -static int merge_recursive(const char *base_sha1, +static int merge_recursive_helper(const char *base_sha1, const char *head_sha1, const char *head_name, const char *next_sha1, const char *next_name) { - char buffer[256]; - const char *argv[6]; - int i = 0; - - sprintf(buffer, "GITHEAD_%s", head_sha1); - setenv(buffer, head_name, 1); - sprintf(buffer, "GITHEAD_%s", next_sha1); - setenv(buffer, next_name, 1); + int clean, index_fd; + struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); + struct commit *result, *h1, *h2; + struct commit_list *ca = NULL; /* * This three way merge is an interesting one. We are at @@ -219,15 +216,21 @@ static int merge_recursive(const char *base_sha1, * and $prev on top of us (when reverting), or the change between * $prev and $commit on top of us (when cherry-picking or replaying). */ - argv[i++] = "merge-recursive"; - if (base_sha1) - argv[i++] = base_sha1; - argv[i++] = "--"; - argv[i++] = head_sha1; - argv[i++] = next_sha1; - argv[i++] = NULL; - - return run_command_v_opt(argv, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD); + if (base_sha1) { + struct commit *base = get_ref(base_sha1); + commit_list_insert(base, &ca); + } + h1 = get_ref(head_sha1); + h2 = get_ref(next_sha1); + + index_fd = hold_locked_index(lock, 1); + clean = merge_recursive(h1, h2, head_name, next_name, ca, &result); + if (active_cache_changed && + (write_cache(index_fd, active_cache, active_nr) || + commit_locked_index(lock))) + die("Unable to write index."); + + return clean ? 0 : 1; } static char *help_msg(const unsigned char *sha1) @@ -373,7 +376,7 @@ static int revert_or_cherry_pick(int argc, const char **argv) } } - if (merge_recursive(base == NULL ? + if (merge_recursive_helper(base == NULL ? NULL : sha1_to_hex(base->object.sha1), sha1_to_hex(head), "HEAD", sha1_to_hex(next->object.sha1), oneline) || diff --git a/merge-recursive.h b/merge-recursive.h index f37630a..40f329b 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -1,6 +1,7 @@ #ifndef MERGE_RECURSIVE_H #define MERGE_RECURSIVE_H +struct commit *get_ref(const char *ref); int merge_recursive(struct commit *h1, struct commit *h2, const char *branch1, -- 1.6.0.rc2.264.g563b6 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH] builtin-revert.c: Make use of merge_recursive() 2008-08-11 15:03 ` [PATCH] builtin-revert.c: Make use of merge_recursive() Stephan Beyer @ 2008-08-11 15:47 ` Johannes Schindelin 2008-08-11 19:01 ` Stephan Beyer 0 siblings, 1 reply; 38+ messages in thread From: Johannes Schindelin @ 2008-08-11 15:47 UTC (permalink / raw) To: Stephan Beyer; +Cc: git, Miklos Vajna Hi, On Mon, 11 Aug 2008, Stephan Beyer wrote: > diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c > index 09aa830..d8bd21f 100644 > --- a/builtin-merge-recursive.c > +++ b/builtin-merge-recursive.c > @@ -1327,7 +1327,7 @@ static const char *better_branch_name(const char *branch) > return name ? name : branch; > } > > -static struct commit *get_ref(const char *ref) > +struct commit *get_ref(const char *ref) The name get_ref() is way too generic to be non-static. But I have a hunch that peel_to_type() does a lot of what we want here, if not all of it. > diff --git a/builtin-revert.c b/builtin-revert.c > index 27881e9..c54cf8a 100644 > --- a/builtin-revert.c > +++ b/builtin-revert.c > @@ -219,15 +216,21 @@ static int merge_recursive(const char *base_sha1, > * and $prev on top of us (when reverting), or the change between > * $prev and $commit on top of us (when cherry-picking or replaying). > */ > - argv[i++] = "merge-recursive"; > - if (base_sha1) > - argv[i++] = base_sha1; > - argv[i++] = "--"; > - argv[i++] = head_sha1; > - argv[i++] = next_sha1; > - argv[i++] = NULL; > - > - return run_command_v_opt(argv, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD); > + if (base_sha1) { > + struct commit *base = get_ref(base_sha1); > + commit_list_insert(base, &ca); > + } > + h1 = get_ref(head_sha1); > + h2 = get_ref(next_sha1); > + > + index_fd = hold_locked_index(lock, 1); > + clean = merge_recursive(h1, h2, head_name, next_name, ca, &result); h1 and h2 are not expressive. head_commit and next_commit would be. Rest looks good to me -- even if I had to spend too much time (therefore being not really thorough in the end) verifying that merge_recursive() does not lock the index itself, and that GITHEAD_* definitions are not necessary anymore, since merge_recursive() takes the arguments directly; you might want to make it easier for this reviewer in the future, if you want this reviewer to review your patches, that is. Ciao, Dscho ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] builtin-revert.c: Make use of merge_recursive() 2008-08-11 15:47 ` Johannes Schindelin @ 2008-08-11 19:01 ` Stephan Beyer 2008-08-11 19:09 ` Miklos Vajna 0 siblings, 1 reply; 38+ messages in thread From: Stephan Beyer @ 2008-08-11 19:01 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git, Miklos Vajna Hi, Johannes Schindelin wrote: > Hi, > > On Mon, 11 Aug 2008, Stephan Beyer wrote: > > > diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c > > index 09aa830..d8bd21f 100644 > > --- a/builtin-merge-recursive.c > > +++ b/builtin-merge-recursive.c > > @@ -1327,7 +1327,7 @@ static const char *better_branch_name(const char *branch) > > return name ? name : branch; > > } > > > > -static struct commit *get_ref(const char *ref) > > +struct commit *get_ref(const char *ref) > > The name get_ref() is way too generic to be non-static. That's right. > But I have a hunch that peel_to_type() does a lot of what we want here, > if not all of it. get_ref() has a big advantage over peel_to_type(): it can handle trees, via "virtual commits" (make_virtual_commit()). If you wonder where we need to handle trees on cherry-pick/revert: With the -n (no commit) option you are allowed to have a dirty index. So the recursive merge is not done using the HEAD commit but using the uncommitted tree of the index. Well, a good alternative could be to just make the really cool make_virtual_commit() function non-static. The name could be generic enough. Is it? :-) Or perhaps: make_virtual_commit_from_tree(). Btw I also need get_ref() (or make_virtual_commit()) for threeway fallback of the sequencer "patch -3" instruction ("git am -3"). ;) > > + h1 = get_ref(head_sha1); > > + h2 = get_ref(next_sha1); > > + > > + index_fd = hold_locked_index(lock, 1); > > + clean = merge_recursive(h1, h2, head_name, next_name, ca, &result); > > h1 and h2 are not expressive. head_commit and next_commit would be. This is also quite true. Those names, also "ca", were taken from cmd_merge_recursive(). (This is no excuse, just an explanation.) > Rest looks good to me -- even if I had to spend too much time (therefore > being not really thorough in the end) verifying that merge_recursive() > does not lock the index itself, I can't help here. Miklos has the same change in patch v2/2 and I wonder if you really expect that I don't test my patches, because a double lock wouldn't have worked. > and that GITHEAD_* definitions are not necessary anymore, since merge_recursive() > takes the arguments directly; Ok, I hoped that would've been clear by using head_name/next_name directly in the merge_recursive() arguments, but nevertheless thanks for your comment, ...because: using get_ref() the GITHEAD_* definitions *are* still needed, because it takes the GITHEAD_* name for the virtual commits... Under this additional circumstance, I really tend to make make_virtual_commit() non-static. Kind regards, Stephan -- Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] builtin-revert.c: Make use of merge_recursive() 2008-08-11 19:01 ` Stephan Beyer @ 2008-08-11 19:09 ` Miklos Vajna 2008-08-11 21:44 ` [PATCH] builtin-revert: " Stephan Beyer 0 siblings, 1 reply; 38+ messages in thread From: Miklos Vajna @ 2008-08-11 19:09 UTC (permalink / raw) To: Stephan Beyer; +Cc: Johannes Schindelin, git [-- Attachment #1: Type: text/plain, Size: 398 bytes --] On Mon, Aug 11, 2008 at 09:01:23PM +0200, Stephan Beyer <s-beyer@gmx.net> wrote: > Well, a good alternative could be to just make the really cool > make_virtual_commit() function non-static. > The name could be generic enough. Is it? :-) > Or perhaps: make_virtual_commit_from_tree(). Given that you can't make virtual commits from commits, tags or blobs, I think the name "as is" generic enough. [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] builtin-revert: Make use of merge_recursive() 2008-08-11 19:09 ` Miklos Vajna @ 2008-08-11 21:44 ` Stephan Beyer 2008-08-11 21:46 ` Stephan Beyer 2008-08-11 23:27 ` Junio C Hamano 0 siblings, 2 replies; 38+ messages in thread From: Stephan Beyer @ 2008-08-11 21:44 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Miklos Vajna, Stephan Beyer Cherry-pick and revert always ran the merging in a separate process. This patch makes cherry-pick/revert call merge_recursive() instead of running git-merge-recursive. To be able to cherry-pick/revert -n (without committing) on a dirty index, make_virtual_commit() is needed and thus declared non-static. Also the GITHEAD_* environment definitions are not needed anymore, since the names are direct arguments to make_virtual_commit() and merge_recursive(). Signed-off-by: Stephan Beyer <s-beyer@gmx.net> --- Hi, so I give it a new try. builtin-merge-recursive.c | 2 +- builtin-revert.c | 56 +++++++++++++++++++++++++------------------- merge-recursive.h | 2 + 3 files changed, 35 insertions(+), 25 deletions(-) diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c index 09aa830..395bdf8 100644 --- a/builtin-merge-recursive.c +++ b/builtin-merge-recursive.c @@ -42,7 +42,7 @@ static struct tree *shift_tree_object(struct tree *one, struct tree *two) * - *(int *)commit->object.sha1 set to the virtual id. */ -static struct commit *make_virtual_commit(struct tree *tree, const char *comment) +struct commit *make_virtual_commit(struct tree *tree, const char *comment) { struct commit *commit = xcalloc(1, sizeof(struct commit)); static unsigned virtual_id = 1; diff --git a/builtin-revert.c b/builtin-revert.c index 27881e9..dcee181 100644 --- a/builtin-revert.c +++ b/builtin-revert.c @@ -11,6 +11,7 @@ #include "cache-tree.h" #include "diff.h" #include "revision.h" +#include "merge-recursive.h" /* * This implements the builtins revert and cherry-pick. @@ -200,18 +201,27 @@ static void set_author_ident_env(const char *message) sha1_to_hex(commit->object.sha1)); } -static int merge_recursive(const char *base_sha1, - const char *head_sha1, const char *head_name, - const char *next_sha1, const char *next_name) +static int merge_recursive_helper(const unsigned char *base_sha1, + const unsigned char *head_sha1, const char *head_name, + const unsigned char *next_sha1, const char *next_name) { - char buffer[256]; - const char *argv[6]; - int i = 0; - - sprintf(buffer, "GITHEAD_%s", head_sha1); - setenv(buffer, head_name, 1); - sprintf(buffer, "GITHEAD_%s", next_sha1); - setenv(buffer, next_name, 1); + int clean, index_fd; + struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); + struct commit *result; + struct commit *head_commit, *next_commit; + struct object *head_object = parse_object(head_sha1); + struct commit_list *ca = NULL; + + if (base_sha1) { + struct commit *base = lookup_commit_reference(base_sha1); + commit_list_insert(base, &ca); + } + if (head_object->type == OBJ_TREE) + head_commit = make_virtual_commit((struct tree *)head_object, + head_name); + else + head_commit = (struct commit *)head_object; + next_commit = lookup_commit_reference(next_sha1); /* * This three way merge is an interesting one. We are at @@ -219,15 +229,15 @@ static int merge_recursive(const char *base_sha1, * and $prev on top of us (when reverting), or the change between * $prev and $commit on top of us (when cherry-picking or replaying). */ - argv[i++] = "merge-recursive"; - if (base_sha1) - argv[i++] = base_sha1; - argv[i++] = "--"; - argv[i++] = head_sha1; - argv[i++] = next_sha1; - argv[i++] = NULL; - - return run_command_v_opt(argv, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD); + index_fd = hold_locked_index(lock, 1); + clean = merge_recursive(head_commit, next_commit, + head_name, next_name, ca, &result); + if (active_cache_changed && + (write_cache(index_fd, active_cache, active_nr) || + commit_locked_index(lock))) + die("Unable to write index."); + + return clean ? 0 : 1; } static char *help_msg(const unsigned char *sha1) @@ -373,10 +383,8 @@ static int revert_or_cherry_pick(int argc, const char **argv) } } - if (merge_recursive(base == NULL ? - NULL : sha1_to_hex(base->object.sha1), - sha1_to_hex(head), "HEAD", - sha1_to_hex(next->object.sha1), oneline) || + if (merge_recursive_helper(base == NULL ? NULL : base->object.sha1, + head, "HEAD", next->object.sha1, oneline) || write_cache_as_tree(head, 0, NULL)) { add_to_msg("\nConflicts:\n\n"); read_cache(); diff --git a/merge-recursive.h b/merge-recursive.h index f37630a..a9eead3 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -1,6 +1,8 @@ #ifndef MERGE_RECURSIVE_H #define MERGE_RECURSIVE_H +extern struct commit *make_virtual_commit(struct tree *tree, + const char *comment); int merge_recursive(struct commit *h1, struct commit *h2, const char *branch1, -- 1.6.0.rc2.267.g02e66a ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH] builtin-revert: Make use of merge_recursive() 2008-08-11 21:44 ` [PATCH] builtin-revert: " Stephan Beyer @ 2008-08-11 21:46 ` Stephan Beyer 2008-08-11 22:33 ` Junio C Hamano 2008-08-11 23:27 ` Junio C Hamano 1 sibling, 1 reply; 38+ messages in thread From: Stephan Beyer @ 2008-08-11 21:46 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Miklos Vajna Sorry, I forgot to change this to [PATCH v2] or something. And... Stephan Beyer wrote: > diff --git a/merge-recursive.h b/merge-recursive.h > index f37630a..a9eead3 100644 > --- a/merge-recursive.h > +++ b/merge-recursive.h > @@ -1,6 +1,8 @@ > #ifndef MERGE_RECURSIVE_H > #define MERGE_RECURSIVE_H > > +extern struct commit *make_virtual_commit(struct tree *tree, > + const char *comment); > int merge_recursive(struct commit *h1, > struct commit *h2, > const char *branch1, Is this a mistake that some forward declarations in header files are not declared "extern"? Regards, Stephan -- Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] builtin-revert: Make use of merge_recursive() 2008-08-11 21:46 ` Stephan Beyer @ 2008-08-11 22:33 ` Junio C Hamano 0 siblings, 0 replies; 38+ messages in thread From: Junio C Hamano @ 2008-08-11 22:33 UTC (permalink / raw) To: Stephan Beyer; +Cc: git, Johannes Schindelin, Miklos Vajna Stephan Beyer <s-beyer@gmx.net> writes: > Sorry, I forgot to change this to [PATCH v2] or something. > > And... > > Stephan Beyer wrote: >> diff --git a/merge-recursive.h b/merge-recursive.h >> index f37630a..a9eead3 100644 >> --- a/merge-recursive.h >> +++ b/merge-recursive.h >> @@ -1,6 +1,8 @@ >> #ifndef MERGE_RECURSIVE_H >> #define MERGE_RECURSIVE_H >> >> +extern struct commit *make_virtual_commit(struct tree *tree, >> + const char *comment); >> int merge_recursive(struct commit *h1, >> struct commit *h2, >> const char *branch1, > > Is this a mistake that some forward declarations in header files are not > declared "extern"? Yup, that looks old fashioned. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] builtin-revert: Make use of merge_recursive() 2008-08-11 21:44 ` [PATCH] builtin-revert: " Stephan Beyer 2008-08-11 21:46 ` Stephan Beyer @ 2008-08-11 23:27 ` Junio C Hamano 2008-08-11 23:47 ` Stephan Beyer 2008-08-12 16:45 ` [PATCH] Split out merge_recursive() to merge-recursive.c Miklos Vajna 1 sibling, 2 replies; 38+ messages in thread From: Junio C Hamano @ 2008-08-11 23:27 UTC (permalink / raw) To: Stephan Beyer; +Cc: git, Johannes Schindelin, Miklos Vajna Stephan Beyer <s-beyer@gmx.net> writes: > Cherry-pick and revert always ran the merging in a separate process. > This patch makes cherry-pick/revert call merge_recursive() instead > of running git-merge-recursive. > > To be able to cherry-pick/revert -n (without committing) on a dirty > index, make_virtual_commit() is needed and thus declared non-static. > > Also the GITHEAD_* environment definitions are not needed anymore, > since the names are direct arguments to make_virtual_commit() and > merge_recursive(). > > Signed-off-by: Stephan Beyer <s-beyer@gmx.net> > --- > Hi, > so I give it a new try. Looks good from a cursory reading. I am not absolutely sure if the phoney commit-looking object that has nonsense SHA-1 created by make_virtual_commit() would have any unintended side effects to the rest of the system, but it does not look like it is even inserted into the global object hash table, so this should be Ok. That was the last piece of worry coming from me regarding this "call recursive internally" theme. Would we need to consolidate this and Miklos's "call recursive internally from git-merge wrapper" by making them into three patches? I.e. (1) move bulk of code from builtin-merge-recursive.c to a new file merge-recursive.c and introduce merge_recursive_helper() in there so that both of you and cmd_merge_recursive() itself can call it; (2) make revert.c use merge_recursive_helper(); (3) make builtin-merge.c use merge_recursive_helper(). ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] builtin-revert: Make use of merge_recursive() 2008-08-11 23:27 ` Junio C Hamano @ 2008-08-11 23:47 ` Stephan Beyer 2008-08-11 23:52 ` Junio C Hamano 2008-08-12 16:45 ` [PATCH] Split out merge_recursive() to merge-recursive.c Miklos Vajna 1 sibling, 1 reply; 38+ messages in thread From: Stephan Beyer @ 2008-08-11 23:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Johannes Schindelin, Miklos Vajna Hi, Junio C Hamano wrote: > I am not absolutely sure if the phoney commit-looking object that has > nonsense SHA-1 created by make_virtual_commit() would have any unintended > side effects to the rest of the system, but it does not look like it is > even inserted into the global object hash table, so this should be Ok. Hmm, the git-merge-recursive process that was run before, calls get_ref() and that also used make_virtual_commit() and thus this virtual_id. So if there is some danger regarding this, then it has also been there before my patch :-) > That was the last piece of worry coming from me regarding this "call > recursive internally" theme. Well, for simple cherry-pick/revert your other worries can be ignored, because it only runs merge_recursive() _once_ and the die()s do not hurt. Wrong? > Would we need to consolidate this and Miklos's "call recursive internally > from git-merge wrapper" by making them into three patches? > I.e. > > (1) move bulk of code from builtin-merge-recursive.c to a new file > merge-recursive.c and introduce merge_recursive_helper() in there so > that both of you and cmd_merge_recursive() itself can call it; I'd like to see a "libified" merge-recursive.c, but I wouldn't call the interesting function merge_recursive_helper(), I'd just take merge_recursive(). Of course those index locking could be done in it. Looking at my sequencer code, I'd also be satisfied, if it takes SHAs as argument and no "struct commit *". But then this should be more generic, i.e. OBJ_TAG has to be handled correctly (builtin-revert does that at the beginning at parse-args()). Hmm, then step (1) is ok. :-) > (2) make revert.c use merge_recursive_helper(); > > (3) make builtin-merge.c use merge_recursive_helper(). This is ok then. Thanks and regards, Stephan -- Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] builtin-revert: Make use of merge_recursive() 2008-08-11 23:47 ` Stephan Beyer @ 2008-08-11 23:52 ` Junio C Hamano 0 siblings, 0 replies; 38+ messages in thread From: Junio C Hamano @ 2008-08-11 23:52 UTC (permalink / raw) To: Stephan Beyer; +Cc: git, Johannes Schindelin, Miklos Vajna Stephan Beyer <s-beyer@gmx.net> writes: >> Would we need to consolidate this and Miklos's "call recursive internally >> from git-merge wrapper" by making them into three patches? >> I.e. >> >> (1) move bulk of code from builtin-merge-recursive.c to a new file >> merge-recursive.c and introduce merge_recursive_helper() in there so >> that both of you and cmd_merge_recursive() itself can call it; > > I'd like to see a "libified" merge-recursive.c, but I wouldn't call the > interesting function merge_recursive_helper(), I'd just take > merge_recursive(). > Of course those index locking could be done in it. > > Looking at my sequencer code, I'd also be satisfied, if it takes SHAs > as argument and no "struct commit *". > But then this should be more generic, i.e. OBJ_TAG has to be handled > correctly (builtin-revert does that at the beginning at parse-args()). > > Hmm, then step (1) is ok. :-) > >> (2) make revert.c use merge_recursive_helper(); >> >> (3) make builtin-merge.c use merge_recursive_helper(). > > This is ok then. Thanks. Let me try to be a bit more explicit to avoid confusion. I won't queue this round of patch (neither this nor Miklos's), as we are not in a hurry during the 1.6.0-rc period anyway, but expect (1) to happen first, Ok? ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] Split out merge_recursive() to merge-recursive.c 2008-08-11 23:27 ` Junio C Hamano 2008-08-11 23:47 ` Stephan Beyer @ 2008-08-12 16:45 ` Miklos Vajna 2008-08-12 17:56 ` Stephan Beyer ` (2 more replies) 1 sibling, 3 replies; 38+ messages in thread From: Miklos Vajna @ 2008-08-12 16:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: Stephan Beyer, git Move most of the of code from builtin-merge-recursive.c to a new file merge-recursive.c and introduce merge_recursive_setup() in there so that builtin-merge-recursive and other builtins call it. Signed-off-by: Miklos Vajna <vmiklos@frugalware.org> --- On Mon, Aug 11, 2008 at 04:27:01PM -0700, Junio C Hamano <gitster@pobox.com> wrote: > (1) move bulk of code from builtin-merge-recursive.c to a new file > merge-recursive.c and introduce merge_recursive_helper() in there > so > that both of you and cmd_merge_recursive() itself can call it; Something like this? The function is actually called merge_recursive_setup(), as I think that name is more intuitive, but I can rename it back to _helper() if that's really a problem. Makefile | 1 + builtin-merge-recursive.c | 1327 +----------------------- builtin-merge-recursive.c => merge-recursive.c | 107 +-- merge-recursive.h | 6 +- 4 files changed, 21 insertions(+), 1420 deletions(-) copy builtin-merge-recursive.c => merge-recursive.c (92%) diff --git a/Makefile b/Makefile index 90c5a13..1e6be75 100644 --- a/Makefile +++ b/Makefile @@ -437,6 +437,7 @@ LIB_OBJS += log-tree.o LIB_OBJS += mailmap.o LIB_OBJS += match-trees.o LIB_OBJS += merge-file.o +LIB_OBJS += merge-recursive.o LIB_OBJS += name-hash.o LIB_OBJS += object.o LIB_OBJS += pack-check.o diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c index 43e55bf..8bf2fa5 100644 --- a/builtin-merge-recursive.c +++ b/builtin-merge-recursive.c @@ -1,1307 +1,8 @@ -/* - * Recursive Merge algorithm stolen from git-merge-recursive.py by - * Fredrik Kuivinen. - * The thieves were Alex Riesen and Johannes Schindelin, in June/July 2006 - */ #include "cache.h" -#include "cache-tree.h" #include "commit.h" -#include "blob.h" -#include "builtin.h" -#include "tree-walk.h" -#include "diff.h" -#include "diffcore.h" #include "tag.h" -#include "unpack-trees.h" -#include "string-list.h" -#include "xdiff-interface.h" -#include "ll-merge.h" -#include "interpolate.h" -#include "attr.h" #include "merge-recursive.h" -static int subtree_merge; - -static struct tree *shift_tree_object(struct tree *one, struct tree *two) -{ - unsigned char shifted[20]; - - /* - * NEEDSWORK: this limits the recursion depth to hardcoded - * value '2' to avoid excessive overhead. - */ - shift_tree(one->object.sha1, two->object.sha1, shifted, 2); - if (!hashcmp(two->object.sha1, shifted)) - return two; - return lookup_tree(shifted); -} - -/* - * A virtual commit has - * - (const char *)commit->util set to the name, and - * - *(int *)commit->object.sha1 set to the virtual id. - */ - -static struct commit *make_virtual_commit(struct tree *tree, const char *comment) -{ - struct commit *commit = xcalloc(1, sizeof(struct commit)); - static unsigned virtual_id = 1; - commit->tree = tree; - commit->util = (void*)comment; - *(int*)commit->object.sha1 = virtual_id++; - /* avoid warnings */ - commit->object.parsed = 1; - return commit; -} - -/* - * Since we use get_tree_entry(), which does not put the read object into - * the object pool, we cannot rely on a == b. - */ -static int sha_eq(const unsigned char *a, const unsigned char *b) -{ - if (!a && !b) - return 2; - return a && b && hashcmp(a, b) == 0; -} - -/* - * Since we want to write the index eventually, we cannot reuse the index - * for these (temporary) data. - */ -struct stage_data -{ - struct - { - unsigned mode; - unsigned char sha[20]; - } stages[4]; - unsigned processed:1; -}; - -static struct string_list current_file_set = {NULL, 0, 0, 1}; -static struct string_list current_directory_set = {NULL, 0, 0, 1}; - -static int call_depth = 0; -static int verbosity = 2; -static int diff_rename_limit = -1; -static int merge_rename_limit = -1; -static int buffer_output = 1; -static struct strbuf obuf = STRBUF_INIT; - -static int show(int v) -{ - return (!call_depth && verbosity >= v) || verbosity >= 5; -} - -static void flush_output(void) -{ - if (obuf.len) { - fputs(obuf.buf, stdout); - strbuf_reset(&obuf); - } -} - -static void output(int v, const char *fmt, ...) -{ - int len; - va_list ap; - - if (!show(v)) - return; - - strbuf_grow(&obuf, call_depth * 2 + 2); - memset(obuf.buf + obuf.len, ' ', call_depth * 2); - strbuf_setlen(&obuf, obuf.len + call_depth * 2); - - va_start(ap, fmt); - len = vsnprintf(obuf.buf + obuf.len, strbuf_avail(&obuf), fmt, ap); - va_end(ap); - - if (len < 0) - len = 0; - if (len >= strbuf_avail(&obuf)) { - strbuf_grow(&obuf, len + 2); - va_start(ap, fmt); - len = vsnprintf(obuf.buf + obuf.len, strbuf_avail(&obuf), fmt, ap); - va_end(ap); - if (len >= strbuf_avail(&obuf)) { - die("this should not happen, your snprintf is broken"); - } - } - strbuf_setlen(&obuf, obuf.len + len); - strbuf_add(&obuf, "\n", 1); - if (!buffer_output) - flush_output(); -} - -static void output_commit_title(struct commit *commit) -{ - int i; - flush_output(); - for (i = call_depth; i--;) - fputs(" ", stdout); - if (commit->util) - printf("virtual %s\n", (char *)commit->util); - else { - printf("%s ", find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV)); - if (parse_commit(commit) != 0) - printf("(bad commit)\n"); - else { - const char *s; - int len; - for (s = commit->buffer; *s; s++) - if (*s == '\n' && s[1] == '\n') { - s += 2; - break; - } - for (len = 0; s[len] && '\n' != s[len]; len++) - ; /* do nothing */ - printf("%.*s\n", len, s); - } - } -} - -static int add_cacheinfo(unsigned int mode, const unsigned char *sha1, - const char *path, int stage, int refresh, int options) -{ - struct cache_entry *ce; - ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage, refresh); - if (!ce) - return error("addinfo_cache failed for path '%s'", path); - return add_cache_entry(ce, options); -} - -/* - * This is a global variable which is used in a number of places but - * only written to in the 'merge' function. - * - * index_only == 1 => Don't leave any non-stage 0 entries in the cache and - * don't update the working directory. - * 0 => Leave unmerged entries in the cache and update - * the working directory. - */ -static int index_only = 0; - -static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree) -{ - parse_tree(tree); - init_tree_desc(desc, tree->buffer, tree->size); -} - -static int git_merge_trees(int index_only, - struct tree *common, - struct tree *head, - struct tree *merge) -{ - int rc; - struct tree_desc t[3]; - struct unpack_trees_options opts; - - memset(&opts, 0, sizeof(opts)); - if (index_only) - opts.index_only = 1; - else - opts.update = 1; - opts.merge = 1; - opts.head_idx = 2; - opts.fn = threeway_merge; - opts.src_index = &the_index; - opts.dst_index = &the_index; - - init_tree_desc_from_tree(t+0, common); - init_tree_desc_from_tree(t+1, head); - init_tree_desc_from_tree(t+2, merge); - - rc = unpack_trees(3, t, &opts); - cache_tree_free(&active_cache_tree); - return rc; -} - -struct tree *write_tree_from_memory(void) -{ - struct tree *result = NULL; - - if (unmerged_cache()) { - int i; - output(0, "There are unmerged index entries:"); - for (i = 0; i < active_nr; i++) { - struct cache_entry *ce = active_cache[i]; - if (ce_stage(ce)) - output(0, "%d %.*s", ce_stage(ce), ce_namelen(ce), ce->name); - } - return NULL; - } - - if (!active_cache_tree) - active_cache_tree = cache_tree(); - - if (!cache_tree_fully_valid(active_cache_tree) && - cache_tree_update(active_cache_tree, - active_cache, active_nr, 0, 0) < 0) - die("error building trees"); - - result = lookup_tree(active_cache_tree->sha1); - - return result; -} - -static int save_files_dirs(const unsigned char *sha1, - const char *base, int baselen, const char *path, - unsigned int mode, int stage, void *context) -{ - int len = strlen(path); - char *newpath = xmalloc(baselen + len + 1); - memcpy(newpath, base, baselen); - memcpy(newpath + baselen, path, len); - newpath[baselen + len] = '\0'; - - if (S_ISDIR(mode)) - string_list_insert(newpath, ¤t_directory_set); - else - string_list_insert(newpath, ¤t_file_set); - free(newpath); - - return READ_TREE_RECURSIVE; -} - -static int get_files_dirs(struct tree *tree) -{ - int n; - if (read_tree_recursive(tree, "", 0, 0, NULL, save_files_dirs, NULL)) - return 0; - n = current_file_set.nr + current_directory_set.nr; - return n; -} - -/* - * Returns an index_entry instance which doesn't have to correspond to - * a real cache entry in Git's index. - */ -static struct stage_data *insert_stage_data(const char *path, - struct tree *o, struct tree *a, struct tree *b, - struct string_list *entries) -{ - struct string_list_item *item; - struct stage_data *e = xcalloc(1, sizeof(struct stage_data)); - get_tree_entry(o->object.sha1, path, - e->stages[1].sha, &e->stages[1].mode); - get_tree_entry(a->object.sha1, path, - e->stages[2].sha, &e->stages[2].mode); - get_tree_entry(b->object.sha1, path, - e->stages[3].sha, &e->stages[3].mode); - item = string_list_insert(path, entries); - item->util = e; - return e; -} - -/* - * Create a dictionary mapping file names to stage_data objects. The - * dictionary contains one entry for every path with a non-zero stage entry. - */ -static struct string_list *get_unmerged(void) -{ - struct string_list *unmerged = xcalloc(1, sizeof(struct string_list)); - int i; - - unmerged->strdup_strings = 1; - - for (i = 0; i < active_nr; i++) { - struct string_list_item *item; - struct stage_data *e; - struct cache_entry *ce = active_cache[i]; - if (!ce_stage(ce)) - continue; - - item = string_list_lookup(ce->name, unmerged); - if (!item) { - item = string_list_insert(ce->name, unmerged); - item->util = xcalloc(1, sizeof(struct stage_data)); - } - e = item->util; - e->stages[ce_stage(ce)].mode = ce->ce_mode; - hashcpy(e->stages[ce_stage(ce)].sha, ce->sha1); - } - - return unmerged; -} - -struct rename -{ - struct diff_filepair *pair; - struct stage_data *src_entry; - struct stage_data *dst_entry; - unsigned processed:1; -}; - -/* - * Get information of all renames which occurred between 'o_tree' and - * 'tree'. We need the three trees in the merge ('o_tree', 'a_tree' and - * 'b_tree') to be able to associate the correct cache entries with - * the rename information. 'tree' is always equal to either a_tree or b_tree. - */ -static struct string_list *get_renames(struct tree *tree, - struct tree *o_tree, - struct tree *a_tree, - struct tree *b_tree, - struct string_list *entries) -{ - int i; - struct string_list *renames; - struct diff_options opts; - - renames = xcalloc(1, sizeof(struct string_list)); - diff_setup(&opts); - DIFF_OPT_SET(&opts, RECURSIVE); - opts.detect_rename = DIFF_DETECT_RENAME; - opts.rename_limit = merge_rename_limit >= 0 ? merge_rename_limit : - diff_rename_limit >= 0 ? diff_rename_limit : - 500; - opts.warn_on_too_large_rename = 1; - opts.output_format = DIFF_FORMAT_NO_OUTPUT; - if (diff_setup_done(&opts) < 0) - die("diff setup failed"); - diff_tree_sha1(o_tree->object.sha1, tree->object.sha1, "", &opts); - diffcore_std(&opts); - for (i = 0; i < diff_queued_diff.nr; ++i) { - struct string_list_item *item; - struct rename *re; - struct diff_filepair *pair = diff_queued_diff.queue[i]; - if (pair->status != 'R') { - diff_free_filepair(pair); - continue; - } - re = xmalloc(sizeof(*re)); - re->processed = 0; - re->pair = pair; - item = string_list_lookup(re->pair->one->path, entries); - if (!item) - re->src_entry = insert_stage_data(re->pair->one->path, - o_tree, a_tree, b_tree, entries); - else - re->src_entry = item->util; - - item = string_list_lookup(re->pair->two->path, entries); - if (!item) - re->dst_entry = insert_stage_data(re->pair->two->path, - o_tree, a_tree, b_tree, entries); - else - re->dst_entry = item->util; - item = string_list_insert(pair->one->path, renames); - item->util = re; - } - opts.output_format = DIFF_FORMAT_NO_OUTPUT; - diff_queued_diff.nr = 0; - diff_flush(&opts); - return renames; -} - -static int update_stages(const char *path, struct diff_filespec *o, - struct diff_filespec *a, struct diff_filespec *b, - int clear) -{ - int options = ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE; - if (clear) - if (remove_file_from_cache(path)) - return -1; - if (o) - if (add_cacheinfo(o->mode, o->sha1, path, 1, 0, options)) - return -1; - if (a) - if (add_cacheinfo(a->mode, a->sha1, path, 2, 0, options)) - return -1; - if (b) - if (add_cacheinfo(b->mode, b->sha1, path, 3, 0, options)) - return -1; - return 0; -} - -static int remove_path(const char *name) -{ - int ret; - char *slash, *dirs; - - ret = unlink(name); - if (ret) - return ret; - dirs = xstrdup(name); - while ((slash = strrchr(name, '/'))) { - *slash = '\0'; - if (rmdir(name) != 0) - break; - } - free(dirs); - return ret; -} - -static int remove_file(int clean, const char *path, int no_wd) -{ - int update_cache = index_only || clean; - int update_working_directory = !index_only && !no_wd; - - if (update_cache) { - if (remove_file_from_cache(path)) - return -1; - } - if (update_working_directory) { - unlink(path); - if (errno != ENOENT || errno != EISDIR) - return -1; - remove_path(path); - } - return 0; -} - -static char *unique_path(const char *path, const char *branch) -{ - char *newpath = xmalloc(strlen(path) + 1 + strlen(branch) + 8 + 1); - int suffix = 0; - struct stat st; - char *p = newpath + strlen(path); - strcpy(newpath, path); - *(p++) = '~'; - strcpy(p, branch); - for (; *p; ++p) - if ('/' == *p) - *p = '_'; - while (string_list_has_string(¤t_file_set, newpath) || - string_list_has_string(¤t_directory_set, newpath) || - lstat(newpath, &st) == 0) - sprintf(p, "_%d", suffix++); - - string_list_insert(newpath, ¤t_file_set); - return newpath; -} - -static void flush_buffer(int fd, const char *buf, unsigned long size) -{ - while (size > 0) { - long ret = write_in_full(fd, buf, size); - if (ret < 0) { - /* Ignore epipe */ - if (errno == EPIPE) - break; - die("merge-recursive: %s", strerror(errno)); - } else if (!ret) { - die("merge-recursive: disk full?"); - } - size -= ret; - buf += ret; - } -} - -static int make_room_for_path(const char *path) -{ - int status; - const char *msg = "failed to create path '%s'%s"; - - status = safe_create_leading_directories_const(path); - if (status) { - if (status == -3) { - /* something else exists */ - error(msg, path, ": perhaps a D/F conflict?"); - return -1; - } - die(msg, path, ""); - } - - /* Successful unlink is good.. */ - if (!unlink(path)) - return 0; - /* .. and so is no existing file */ - if (errno == ENOENT) - return 0; - /* .. but not some other error (who really cares what?) */ - return error(msg, path, ": perhaps a D/F conflict?"); -} - -static void update_file_flags(const unsigned char *sha, - unsigned mode, - const char *path, - int update_cache, - int update_wd) -{ - if (index_only) - update_wd = 0; - - if (update_wd) { - enum object_type type; - void *buf; - unsigned long size; - - if (S_ISGITLINK(mode)) - die("cannot read object %s '%s': It is a submodule!", - sha1_to_hex(sha), path); - - buf = read_sha1_file(sha, &type, &size); - if (!buf) - die("cannot read object %s '%s'", sha1_to_hex(sha), path); - if (type != OBJ_BLOB) - die("blob expected for %s '%s'", sha1_to_hex(sha), path); - if (S_ISREG(mode)) { - struct strbuf strbuf; - strbuf_init(&strbuf, 0); - if (convert_to_working_tree(path, buf, size, &strbuf)) { - free(buf); - size = strbuf.len; - buf = strbuf_detach(&strbuf, NULL); - } - } - - if (make_room_for_path(path) < 0) { - update_wd = 0; - free(buf); - goto update_index; - } - if (S_ISREG(mode) || (!has_symlinks && S_ISLNK(mode))) { - int fd; - if (mode & 0100) - mode = 0777; - else - mode = 0666; - fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode); - if (fd < 0) - die("failed to open %s: %s", path, strerror(errno)); - flush_buffer(fd, buf, size); - close(fd); - } else if (S_ISLNK(mode)) { - char *lnk = xmemdupz(buf, size); - safe_create_leading_directories_const(path); - unlink(path); - symlink(lnk, path); - free(lnk); - } else - die("do not know what to do with %06o %s '%s'", - mode, sha1_to_hex(sha), path); - free(buf); - } - update_index: - if (update_cache) - add_cacheinfo(mode, sha, path, 0, update_wd, ADD_CACHE_OK_TO_ADD); -} - -static void update_file(int clean, - const unsigned char *sha, - unsigned mode, - const char *path) -{ - update_file_flags(sha, mode, path, index_only || clean, !index_only); -} - -/* Low level file merging, update and removal */ - -struct merge_file_info -{ - unsigned char sha[20]; - unsigned mode; - unsigned clean:1, - merge:1; -}; - -static void fill_mm(const unsigned char *sha1, mmfile_t *mm) -{ - unsigned long size; - enum object_type type; - - if (!hashcmp(sha1, null_sha1)) { - mm->ptr = xstrdup(""); - mm->size = 0; - return; - } - - mm->ptr = read_sha1_file(sha1, &type, &size); - if (!mm->ptr || type != OBJ_BLOB) - die("unable to read blob object %s", sha1_to_hex(sha1)); - mm->size = size; -} - -static int merge_3way(mmbuffer_t *result_buf, - struct diff_filespec *o, - struct diff_filespec *a, - struct diff_filespec *b, - const char *branch1, - const char *branch2) -{ - mmfile_t orig, src1, src2; - char *name1, *name2; - int merge_status; - - name1 = xstrdup(mkpath("%s:%s", branch1, a->path)); - name2 = xstrdup(mkpath("%s:%s", branch2, b->path)); - - fill_mm(o->sha1, &orig); - fill_mm(a->sha1, &src1); - fill_mm(b->sha1, &src2); - - merge_status = ll_merge(result_buf, a->path, &orig, - &src1, name1, &src2, name2, - index_only); - - free(name1); - free(name2); - free(orig.ptr); - free(src1.ptr); - free(src2.ptr); - return merge_status; -} - -static struct merge_file_info merge_file(struct diff_filespec *o, - struct diff_filespec *a, struct diff_filespec *b, - const char *branch1, const char *branch2) -{ - struct merge_file_info result; - result.merge = 0; - result.clean = 1; - - if ((S_IFMT & a->mode) != (S_IFMT & b->mode)) { - result.clean = 0; - if (S_ISREG(a->mode)) { - result.mode = a->mode; - hashcpy(result.sha, a->sha1); - } else { - result.mode = b->mode; - hashcpy(result.sha, b->sha1); - } - } else { - if (!sha_eq(a->sha1, o->sha1) && !sha_eq(b->sha1, o->sha1)) - result.merge = 1; - - /* - * Merge modes - */ - if (a->mode == b->mode || a->mode == o->mode) - result.mode = b->mode; - else { - result.mode = a->mode; - if (b->mode != o->mode) { - result.clean = 0; - result.merge = 1; - } - } - - if (sha_eq(a->sha1, b->sha1) || sha_eq(a->sha1, o->sha1)) - hashcpy(result.sha, b->sha1); - else if (sha_eq(b->sha1, o->sha1)) - hashcpy(result.sha, a->sha1); - else if (S_ISREG(a->mode)) { - mmbuffer_t result_buf; - int merge_status; - - merge_status = merge_3way(&result_buf, o, a, b, - branch1, branch2); - - if ((merge_status < 0) || !result_buf.ptr) - die("Failed to execute internal merge"); - - if (write_sha1_file(result_buf.ptr, result_buf.size, - blob_type, result.sha)) - die("Unable to add %s to database", - a->path); - - free(result_buf.ptr); - result.clean = (merge_status == 0); - } else if (S_ISGITLINK(a->mode)) { - result.clean = 0; - hashcpy(result.sha, a->sha1); - } else if (S_ISLNK(a->mode)) { - hashcpy(result.sha, a->sha1); - - if (!sha_eq(a->sha1, b->sha1)) - result.clean = 0; - } else { - die("unsupported object type in the tree"); - } - } - - return result; -} - -static void conflict_rename_rename(struct rename *ren1, - const char *branch1, - struct rename *ren2, - const char *branch2) -{ - char *del[2]; - int delp = 0; - const char *ren1_dst = ren1->pair->two->path; - const char *ren2_dst = ren2->pair->two->path; - const char *dst_name1 = ren1_dst; - const char *dst_name2 = ren2_dst; - if (string_list_has_string(¤t_directory_set, ren1_dst)) { - dst_name1 = del[delp++] = unique_path(ren1_dst, branch1); - output(1, "%s is a directory in %s added as %s instead", - ren1_dst, branch2, dst_name1); - remove_file(0, ren1_dst, 0); - } - if (string_list_has_string(¤t_directory_set, ren2_dst)) { - dst_name2 = del[delp++] = unique_path(ren2_dst, branch2); - output(1, "%s is a directory in %s added as %s instead", - ren2_dst, branch1, dst_name2); - remove_file(0, ren2_dst, 0); - } - if (index_only) { - remove_file_from_cache(dst_name1); - remove_file_from_cache(dst_name2); - /* - * Uncomment to leave the conflicting names in the resulting tree - * - * update_file(0, ren1->pair->two->sha1, ren1->pair->two->mode, dst_name1); - * update_file(0, ren2->pair->two->sha1, ren2->pair->two->mode, dst_name2); - */ - } else { - update_stages(dst_name1, NULL, ren1->pair->two, NULL, 1); - update_stages(dst_name2, NULL, NULL, ren2->pair->two, 1); - } - while (delp--) - free(del[delp]); -} - -static void conflict_rename_dir(struct rename *ren1, - const char *branch1) -{ - char *new_path = unique_path(ren1->pair->two->path, branch1); - output(1, "Renamed %s to %s instead", ren1->pair->one->path, new_path); - remove_file(0, ren1->pair->two->path, 0); - update_file(0, ren1->pair->two->sha1, ren1->pair->two->mode, new_path); - free(new_path); -} - -static void conflict_rename_rename_2(struct rename *ren1, - const char *branch1, - struct rename *ren2, - const char *branch2) -{ - char *new_path1 = unique_path(ren1->pair->two->path, branch1); - char *new_path2 = unique_path(ren2->pair->two->path, branch2); - output(1, "Renamed %s to %s and %s to %s instead", - ren1->pair->one->path, new_path1, - ren2->pair->one->path, new_path2); - remove_file(0, ren1->pair->two->path, 0); - update_file(0, ren1->pair->two->sha1, ren1->pair->two->mode, new_path1); - update_file(0, ren2->pair->two->sha1, ren2->pair->two->mode, new_path2); - free(new_path2); - free(new_path1); -} - -static int process_renames(struct string_list *a_renames, - struct string_list *b_renames, - const char *a_branch, - const char *b_branch) -{ - int clean_merge = 1, i, j; - struct string_list a_by_dst = {NULL, 0, 0, 0}, b_by_dst = {NULL, 0, 0, 0}; - const struct rename *sre; - - for (i = 0; i < a_renames->nr; i++) { - sre = a_renames->items[i].util; - string_list_insert(sre->pair->two->path, &a_by_dst)->util - = sre->dst_entry; - } - for (i = 0; i < b_renames->nr; i++) { - sre = b_renames->items[i].util; - string_list_insert(sre->pair->two->path, &b_by_dst)->util - = sre->dst_entry; - } - - for (i = 0, j = 0; i < a_renames->nr || j < b_renames->nr;) { - int compare; - char *src; - struct string_list *renames1, *renames2, *renames2Dst; - struct rename *ren1 = NULL, *ren2 = NULL; - const char *branch1, *branch2; - const char *ren1_src, *ren1_dst; - - if (i >= a_renames->nr) { - compare = 1; - ren2 = b_renames->items[j++].util; - } else if (j >= b_renames->nr) { - compare = -1; - ren1 = a_renames->items[i++].util; - } else { - compare = strcmp(a_renames->items[i].string, - b_renames->items[j].string); - if (compare <= 0) - ren1 = a_renames->items[i++].util; - if (compare >= 0) - ren2 = b_renames->items[j++].util; - } - - /* TODO: refactor, so that 1/2 are not needed */ - if (ren1) { - renames1 = a_renames; - renames2 = b_renames; - renames2Dst = &b_by_dst; - branch1 = a_branch; - branch2 = b_branch; - } else { - struct rename *tmp; - renames1 = b_renames; - renames2 = a_renames; - renames2Dst = &a_by_dst; - branch1 = b_branch; - branch2 = a_branch; - tmp = ren2; - ren2 = ren1; - ren1 = tmp; - } - src = ren1->pair->one->path; - - ren1->dst_entry->processed = 1; - ren1->src_entry->processed = 1; - - if (ren1->processed) - continue; - ren1->processed = 1; - - ren1_src = ren1->pair->one->path; - ren1_dst = ren1->pair->two->path; - - if (ren2) { - const char *ren2_src = ren2->pair->one->path; - const char *ren2_dst = ren2->pair->two->path; - /* Renamed in 1 and renamed in 2 */ - if (strcmp(ren1_src, ren2_src) != 0) - die("ren1.src != ren2.src"); - ren2->dst_entry->processed = 1; - ren2->processed = 1; - if (strcmp(ren1_dst, ren2_dst) != 0) { - clean_merge = 0; - output(1, "CONFLICT (rename/rename): " - "Rename \"%s\"->\"%s\" in branch \"%s\" " - "rename \"%s\"->\"%s\" in \"%s\"%s", - src, ren1_dst, branch1, - src, ren2_dst, branch2, - index_only ? " (left unresolved)": ""); - if (index_only) { - remove_file_from_cache(src); - update_file(0, ren1->pair->one->sha1, - ren1->pair->one->mode, src); - } - conflict_rename_rename(ren1, branch1, ren2, branch2); - } else { - struct merge_file_info mfi; - remove_file(1, ren1_src, 1); - mfi = merge_file(ren1->pair->one, - ren1->pair->two, - ren2->pair->two, - branch1, - branch2); - if (mfi.merge || !mfi.clean) - output(1, "Renamed %s->%s", src, ren1_dst); - - if (mfi.merge) - output(2, "Auto-merged %s", ren1_dst); - - if (!mfi.clean) { - output(1, "CONFLICT (content): merge conflict in %s", - ren1_dst); - clean_merge = 0; - - if (!index_only) - update_stages(ren1_dst, - ren1->pair->one, - ren1->pair->two, - ren2->pair->two, - 1 /* clear */); - } - update_file(mfi.clean, mfi.sha, mfi.mode, ren1_dst); - } - } else { - /* Renamed in 1, maybe changed in 2 */ - struct string_list_item *item; - /* we only use sha1 and mode of these */ - struct diff_filespec src_other, dst_other; - int try_merge, stage = a_renames == renames1 ? 3: 2; - - remove_file(1, ren1_src, index_only || stage == 3); - - hashcpy(src_other.sha1, ren1->src_entry->stages[stage].sha); - src_other.mode = ren1->src_entry->stages[stage].mode; - hashcpy(dst_other.sha1, ren1->dst_entry->stages[stage].sha); - dst_other.mode = ren1->dst_entry->stages[stage].mode; - - try_merge = 0; - - if (string_list_has_string(¤t_directory_set, ren1_dst)) { - clean_merge = 0; - output(1, "CONFLICT (rename/directory): Renamed %s->%s in %s " - " directory %s added in %s", - ren1_src, ren1_dst, branch1, - ren1_dst, branch2); - conflict_rename_dir(ren1, branch1); - } else if (sha_eq(src_other.sha1, null_sha1)) { - clean_merge = 0; - output(1, "CONFLICT (rename/delete): Renamed %s->%s in %s " - "and deleted in %s", - ren1_src, ren1_dst, branch1, - branch2); - update_file(0, ren1->pair->two->sha1, ren1->pair->two->mode, ren1_dst); - } else if (!sha_eq(dst_other.sha1, null_sha1)) { - const char *new_path; - clean_merge = 0; - try_merge = 1; - output(1, "CONFLICT (rename/add): Renamed %s->%s in %s. " - "%s added in %s", - ren1_src, ren1_dst, branch1, - ren1_dst, branch2); - new_path = unique_path(ren1_dst, branch2); - output(1, "Added as %s instead", new_path); - update_file(0, dst_other.sha1, dst_other.mode, new_path); - } else if ((item = string_list_lookup(ren1_dst, renames2Dst))) { - ren2 = item->util; - clean_merge = 0; - ren2->processed = 1; - output(1, "CONFLICT (rename/rename): Renamed %s->%s in %s. " - "Renamed %s->%s in %s", - ren1_src, ren1_dst, branch1, - ren2->pair->one->path, ren2->pair->two->path, branch2); - conflict_rename_rename_2(ren1, branch1, ren2, branch2); - } else - try_merge = 1; - - if (try_merge) { - struct diff_filespec *o, *a, *b; - struct merge_file_info mfi; - src_other.path = (char *)ren1_src; - - o = ren1->pair->one; - if (a_renames == renames1) { - a = ren1->pair->two; - b = &src_other; - } else { - b = ren1->pair->two; - a = &src_other; - } - mfi = merge_file(o, a, b, - a_branch, b_branch); - - if (mfi.clean && - sha_eq(mfi.sha, ren1->pair->two->sha1) && - mfi.mode == ren1->pair->two->mode) - /* - * This messaged is part of - * t6022 test. If you change - * it update the test too. - */ - output(3, "Skipped %s (merged same as existing)", ren1_dst); - else { - if (mfi.merge || !mfi.clean) - output(1, "Renamed %s => %s", ren1_src, ren1_dst); - if (mfi.merge) - output(2, "Auto-merged %s", ren1_dst); - if (!mfi.clean) { - output(1, "CONFLICT (rename/modify): Merge conflict in %s", - ren1_dst); - clean_merge = 0; - - if (!index_only) - update_stages(ren1_dst, - o, a, b, 1); - } - update_file(mfi.clean, mfi.sha, mfi.mode, ren1_dst); - } - } - } - } - string_list_clear(&a_by_dst, 0); - string_list_clear(&b_by_dst, 0); - - return clean_merge; -} - -static unsigned char *stage_sha(const unsigned char *sha, unsigned mode) -{ - return (is_null_sha1(sha) || mode == 0) ? NULL: (unsigned char *)sha; -} - -/* Per entry merge function */ -static int process_entry(const char *path, struct stage_data *entry, - const char *branch1, - const char *branch2) -{ - /* - printf("processing entry, clean cache: %s\n", index_only ? "yes": "no"); - print_index_entry("\tpath: ", entry); - */ - int clean_merge = 1; - unsigned o_mode = entry->stages[1].mode; - unsigned a_mode = entry->stages[2].mode; - unsigned b_mode = entry->stages[3].mode; - unsigned char *o_sha = stage_sha(entry->stages[1].sha, o_mode); - unsigned char *a_sha = stage_sha(entry->stages[2].sha, a_mode); - unsigned char *b_sha = stage_sha(entry->stages[3].sha, b_mode); - - if (o_sha && (!a_sha || !b_sha)) { - /* Case A: Deleted in one */ - if ((!a_sha && !b_sha) || - (sha_eq(a_sha, o_sha) && !b_sha) || - (!a_sha && sha_eq(b_sha, o_sha))) { - /* Deleted in both or deleted in one and - * unchanged in the other */ - if (a_sha) - output(2, "Removed %s", path); - /* do not touch working file if it did not exist */ - remove_file(1, path, !a_sha); - } else { - /* Deleted in one and changed in the other */ - clean_merge = 0; - if (!a_sha) { - output(1, "CONFLICT (delete/modify): %s deleted in %s " - "and modified in %s. Version %s of %s left in tree.", - path, branch1, - branch2, branch2, path); - update_file(0, b_sha, b_mode, path); - } else { - output(1, "CONFLICT (delete/modify): %s deleted in %s " - "and modified in %s. Version %s of %s left in tree.", - path, branch2, - branch1, branch1, path); - update_file(0, a_sha, a_mode, path); - } - } - - } else if ((!o_sha && a_sha && !b_sha) || - (!o_sha && !a_sha && b_sha)) { - /* Case B: Added in one. */ - const char *add_branch; - const char *other_branch; - unsigned mode; - const unsigned char *sha; - const char *conf; - - if (a_sha) { - add_branch = branch1; - other_branch = branch2; - mode = a_mode; - sha = a_sha; - conf = "file/directory"; - } else { - add_branch = branch2; - other_branch = branch1; - mode = b_mode; - sha = b_sha; - conf = "directory/file"; - } - if (string_list_has_string(¤t_directory_set, path)) { - const char *new_path = unique_path(path, add_branch); - clean_merge = 0; - output(1, "CONFLICT (%s): There is a directory with name %s in %s. " - "Added %s as %s", - conf, path, other_branch, path, new_path); - remove_file(0, path, 0); - update_file(0, sha, mode, new_path); - } else { - output(2, "Added %s", path); - update_file(1, sha, mode, path); - } - } else if (a_sha && b_sha) { - /* Case C: Added in both (check for same permissions) and */ - /* case D: Modified in both, but differently. */ - const char *reason = "content"; - struct merge_file_info mfi; - struct diff_filespec o, a, b; - - if (!o_sha) { - reason = "add/add"; - o_sha = (unsigned char *)null_sha1; - } - output(2, "Auto-merged %s", path); - o.path = a.path = b.path = (char *)path; - hashcpy(o.sha1, o_sha); - o.mode = o_mode; - hashcpy(a.sha1, a_sha); - a.mode = a_mode; - hashcpy(b.sha1, b_sha); - b.mode = b_mode; - - mfi = merge_file(&o, &a, &b, - branch1, branch2); - - clean_merge = mfi.clean; - if (mfi.clean) - update_file(1, mfi.sha, mfi.mode, path); - else if (S_ISGITLINK(mfi.mode)) - output(1, "CONFLICT (submodule): Merge conflict in %s " - "- needs %s", path, sha1_to_hex(b.sha1)); - else { - output(1, "CONFLICT (%s): Merge conflict in %s", - reason, path); - - if (index_only) - update_file(0, mfi.sha, mfi.mode, path); - else - update_file_flags(mfi.sha, mfi.mode, path, - 0 /* update_cache */, 1 /* update_working_directory */); - } - } else if (!o_sha && !a_sha && !b_sha) { - /* - * this entry was deleted altogether. a_mode == 0 means - * we had that path and want to actively remove it. - */ - remove_file(1, path, !a_mode); - } else - die("Fatal merge failure, shouldn't happen."); - - return clean_merge; -} - -int merge_trees(struct tree *head, - struct tree *merge, - struct tree *common, - const char *branch1, - const char *branch2, - struct tree **result) -{ - int code, clean; - - if (subtree_merge) { - merge = shift_tree_object(head, merge); - common = shift_tree_object(head, common); - } - - if (sha_eq(common->object.sha1, merge->object.sha1)) { - output(0, "Already uptodate!"); - *result = head; - return 1; - } - - code = git_merge_trees(index_only, common, head, merge); - - if (code != 0) - die("merging of trees %s and %s failed", - sha1_to_hex(head->object.sha1), - sha1_to_hex(merge->object.sha1)); - - if (unmerged_cache()) { - struct string_list *entries, *re_head, *re_merge; - int i; - string_list_clear(¤t_file_set, 1); - string_list_clear(¤t_directory_set, 1); - get_files_dirs(head); - get_files_dirs(merge); - - entries = get_unmerged(); - re_head = get_renames(head, common, head, merge, entries); - re_merge = get_renames(merge, common, head, merge, entries); - clean = process_renames(re_head, re_merge, - branch1, branch2); - for (i = 0; i < entries->nr; i++) { - const char *path = entries->items[i].string; - struct stage_data *e = entries->items[i].util; - if (!e->processed - && !process_entry(path, e, branch1, branch2)) - clean = 0; - } - - string_list_clear(re_merge, 0); - string_list_clear(re_head, 0); - string_list_clear(entries, 1); - - } - else - clean = 1; - - if (index_only) - *result = write_tree_from_memory(); - - return clean; -} - -static struct commit_list *reverse_commit_list(struct commit_list *list) -{ - struct commit_list *next = NULL, *current, *backup; - for (current = list; current; current = backup) { - backup = current->next; - current->next = next; - next = current; - } - return next; -} - -/* - * Merge the commits h1 and h2, return the resulting virtual - * commit object and a flag indicating the cleanness of the merge. - */ -int merge_recursive(struct commit *h1, - struct commit *h2, - const char *branch1, - const char *branch2, - struct commit_list *ca, - struct commit **result) -{ - struct commit_list *iter; - struct commit *merged_common_ancestors; - struct tree *mrtree = mrtree; - int clean; - - if (show(4)) { - output(4, "Merging:"); - output_commit_title(h1); - output_commit_title(h2); - } - - if (!ca) { - ca = get_merge_bases(h1, h2, 1); - ca = reverse_commit_list(ca); - } - - if (show(5)) { - output(5, "found %u common ancestor(s):", commit_list_count(ca)); - for (iter = ca; iter; iter = iter->next) - output_commit_title(iter->item); - } - - merged_common_ancestors = pop_commit(&ca); - if (merged_common_ancestors == NULL) { - /* if there is no common ancestor, make an empty tree */ - struct tree *tree = xcalloc(1, sizeof(struct tree)); - - tree->object.parsed = 1; - tree->object.type = OBJ_TREE; - pretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1); - merged_common_ancestors = make_virtual_commit(tree, "ancestor"); - } - - for (iter = ca; iter; iter = iter->next) { - call_depth++; - /* - * When the merge fails, the result contains files - * with conflict markers. The cleanness flag is - * ignored, it was never actually used, as result of - * merge_trees has always overwritten it: the committed - * "conflicts" were already resolved. - */ - discard_cache(); - merge_recursive(merged_common_ancestors, iter->item, - "Temporary merge branch 1", - "Temporary merge branch 2", - NULL, - &merged_common_ancestors); - call_depth--; - - if (!merged_common_ancestors) - die("merge returned no commit"); - } - - discard_cache(); - if (!call_depth) { - read_cache(); - index_only = 0; - } else - index_only = 1; - - clean = merge_trees(h1->tree, h2->tree, merged_common_ancestors->tree, - branch1, branch2, &mrtree); - - if (index_only) { - *result = make_virtual_commit(mrtree, "merged tree"); - commit_list_insert(h1, &(*result)->parents); - commit_list_insert(h2, &(*result)->parents->next); - } - flush_output(); - return clean; -} - static const char *better_branch_name(const char *branch) { static char githead_env[8 + 40 + 1]; @@ -1334,23 +35,6 @@ static struct commit *get_ref(const char *ref) return (struct commit *)object; } -static int merge_config(const char *var, const char *value, void *cb) -{ - if (!strcasecmp(var, "merge.verbosity")) { - verbosity = git_config_int(var, value); - return 0; - } - if (!strcasecmp(var, "diff.renamelimit")) { - diff_rename_limit = git_config_int(var, value); - return 0; - } - if (!strcasecmp(var, "merge.renamelimit")) { - merge_rename_limit = git_config_int(var, value); - return 0; - } - return git_default_config(var, value, cb); -} - int cmd_merge_recursive(int argc, const char **argv, const char *prefix) { static const char *bases[20]; @@ -1361,6 +45,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) struct commit_list *ca = NULL; struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); int index_fd; + int subtree_merge = 0; if (argv[0]) { int namelen = strlen(argv[0]); @@ -1369,10 +54,8 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) subtree_merge = 1; } - git_config(merge_config, NULL); - if (getenv("GIT_MERGE_VERBOSITY")) - verbosity = strtol(getenv("GIT_MERGE_VERBOSITY"), NULL, 10); - + git_config(merge_recursive_config, NULL); + merge_recursive_setup(subtree_merge); if (argc < 4) die("Usage: %s <base>... -- <head> <remote> ...\n", argv[0]); @@ -1384,8 +67,6 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) } if (argc - i != 3) /* "--" "<head>" "<remote>" */ die("Not handling anything other than two heads merge."); - if (verbosity >= 5) - buffer_output = 0; branch1 = argv[++i]; branch2 = argv[++i]; @@ -1396,7 +77,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) branch1 = better_branch_name(branch1); branch2 = better_branch_name(branch2); - if (show(3)) + if (merge_recursive_verbosity >= 3) printf("Merging %s with %s\n", branch1, branch2); index_fd = hold_locked_index(lock, 1); diff --git a/builtin-merge-recursive.c b/merge-recursive.c similarity index 92% copy from builtin-merge-recursive.c copy to merge-recursive.c index 43e55bf..c3a57ac 100644 --- a/builtin-merge-recursive.c +++ b/merge-recursive.c @@ -42,7 +42,7 @@ static struct tree *shift_tree_object(struct tree *one, struct tree *two) * - *(int *)commit->object.sha1 set to the virtual id. */ -static struct commit *make_virtual_commit(struct tree *tree, const char *comment) +struct commit *make_virtual_commit(struct tree *tree, const char *comment) { struct commit *commit = xcalloc(1, sizeof(struct commit)); static unsigned virtual_id = 1; @@ -83,7 +83,7 @@ static struct string_list current_file_set = {NULL, 0, 0, 1}; static struct string_list current_directory_set = {NULL, 0, 0, 1}; static int call_depth = 0; -static int verbosity = 2; +int merge_recursive_verbosity = 2; static int diff_rename_limit = -1; static int merge_rename_limit = -1; static int buffer_output = 1; @@ -91,7 +91,8 @@ static struct strbuf obuf = STRBUF_INIT; static int show(int v) { - return (!call_depth && verbosity >= v) || verbosity >= 5; + return (!call_depth && merge_recursive_verbosity >= v) || + merge_recursive_verbosity >= 5; } static void flush_output(void) @@ -1302,42 +1303,10 @@ int merge_recursive(struct commit *h1, return clean; } -static const char *better_branch_name(const char *branch) -{ - static char githead_env[8 + 40 + 1]; - char *name; - - if (strlen(branch) != 40) - return branch; - sprintf(githead_env, "GITHEAD_%s", branch); - name = getenv(githead_env); - return name ? name : branch; -} - -static struct commit *get_ref(const char *ref) -{ - unsigned char sha1[20]; - struct object *object; - - if (get_sha1(ref, sha1)) - die("Could not resolve ref '%s'", ref); - object = deref_tag(parse_object(sha1), ref, strlen(ref)); - if (!object) - return NULL; - if (object->type == OBJ_TREE) - return make_virtual_commit((struct tree*)object, - better_branch_name(ref)); - if (object->type != OBJ_COMMIT) - return NULL; - if (parse_commit((struct commit *)object)) - die("Could not parse commit '%s'", sha1_to_hex(object->sha1)); - return (struct commit *)object; -} - -static int merge_config(const char *var, const char *value, void *cb) +int merge_recursive_config(const char *var, const char *value, void *cb) { if (!strcasecmp(var, "merge.verbosity")) { - verbosity = git_config_int(var, value); + merge_recursive_verbosity = git_config_int(var, value); return 0; } if (!strcasecmp(var, "diff.renamelimit")) { @@ -1351,66 +1320,12 @@ static int merge_config(const char *var, const char *value, void *cb) return git_default_config(var, value, cb); } -int cmd_merge_recursive(int argc, const char **argv, const char *prefix) +void merge_recursive_setup(int is_subtree_merge) { - static const char *bases[20]; - static unsigned bases_count = 0; - int i, clean; - const char *branch1, *branch2; - struct commit *result, *h1, *h2; - struct commit_list *ca = NULL; - struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); - int index_fd; - - if (argv[0]) { - int namelen = strlen(argv[0]); - if (8 < namelen && - !strcmp(argv[0] + namelen - 8, "-subtree")) - subtree_merge = 1; - } - - git_config(merge_config, NULL); if (getenv("GIT_MERGE_VERBOSITY")) - verbosity = strtol(getenv("GIT_MERGE_VERBOSITY"), NULL, 10); - - if (argc < 4) - die("Usage: %s <base>... -- <head> <remote> ...\n", argv[0]); - - for (i = 1; i < argc; ++i) { - if (!strcmp(argv[i], "--")) - break; - if (bases_count < sizeof(bases)/sizeof(*bases)) - bases[bases_count++] = argv[i]; - } - if (argc - i != 3) /* "--" "<head>" "<remote>" */ - die("Not handling anything other than two heads merge."); - if (verbosity >= 5) + merge_recursive_verbosity = + strtol(getenv("GIT_MERGE_VERBOSITY"), NULL, 10); + if (merge_recursive_verbosity >= 5) buffer_output = 0; - - branch1 = argv[++i]; - branch2 = argv[++i]; - - h1 = get_ref(branch1); - h2 = get_ref(branch2); - - branch1 = better_branch_name(branch1); - branch2 = better_branch_name(branch2); - - if (show(3)) - printf("Merging %s with %s\n", branch1, branch2); - - index_fd = hold_locked_index(lock, 1); - - for (i = 0; i < bases_count; i++) { - struct commit *ancestor = get_ref(bases[i]); - ca = commit_list_insert(ancestor, &ca); - } - clean = merge_recursive(h1, h2, branch1, branch2, ca, &result); - - if (active_cache_changed && - (write_cache(index_fd, active_cache, active_nr) || - commit_locked_index(lock))) - die ("unable to write %s", get_index_file()); - - return clean ? 0: 1; + subtree_merge = is_subtree_merge; } diff --git a/merge-recursive.h b/merge-recursive.h index f37630a..73e4413 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -14,7 +14,11 @@ int merge_trees(struct tree *head, const char *branch1, const char *branch2, struct tree **result); - +struct commit *make_virtual_commit(struct tree *tree, const char *comment); +int merge_recursive_config(const char *var, const char *value, void *cb); +void merge_recursive_setup(int is_subtree_merge); struct tree *write_tree_from_memory(void); +extern int merge_recursive_verbosity; + #endif -- 1.6.0.rc0.14.g95f8.dirty ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH] Split out merge_recursive() to merge-recursive.c 2008-08-12 16:45 ` [PATCH] Split out merge_recursive() to merge-recursive.c Miklos Vajna @ 2008-08-12 17:56 ` Stephan Beyer 2008-08-12 21:40 ` Miklos Vajna 2008-08-12 20:13 ` [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() Stephan Beyer 2008-08-14 3:17 ` [PATCH] Split out merge_recursive() to merge-recursive.c Junio C Hamano 2 siblings, 1 reply; 38+ messages in thread From: Stephan Beyer @ 2008-08-12 17:56 UTC (permalink / raw) To: Miklos Vajna; +Cc: Junio C Hamano, git Hi, Miklos Vajna wrote: > Move most of the of code from builtin-merge-recursive.c to a new file > merge-recursive.c and introduce merge_recursive_setup() in there so that > builtin-merge-recursive and other builtins call it. So, according to that change my "revert" patch is the same plus this interdiff: --8<-- diff --git a/builtin-revert.c b/builtin-revert.c index dcee181..941b875 100644 --- a/builtin-revert.c +++ b/builtin-revert.c @@ -281,7 +281,8 @@ static int revert_or_cherry_pick(int argc, const char **argv) const char *message, *encoding; const char *defmsg = xstrdup(git_path("MERGE_MSG")); - git_config(git_default_config, NULL); + git_config(merge_recursive_config, NULL); + merge_recursive_setup(0); me = action == REVERT ? "revert" : "cherry-pick"; setenv(GIT_REFLOG_ACTION, me, 0); parse_args(argc, argv); -->8-- I'm fine with that, but my hope(?) was that we could have some more generic function that takes SHAs ("unsigned char *") instead of commits. I don't know if this is bad for builtin-merge, but from the "revert" and "sequencer" point of view this is all I need. Hmm, I think it takes less time to implement it based on your patch than explaining :-) One further comment: > -static int merge_config(const char *var, const char *value, void *cb) > +int merge_recursive_config(const char *var, const char *value, void *cb) > { > if (!strcasecmp(var, "merge.verbosity")) { > - verbosity = git_config_int(var, value); > + merge_recursive_verbosity = git_config_int(var, value); [...] > diff --git a/merge-recursive.h b/merge-recursive.h > index f37630a..73e4413 100644 > --- a/merge-recursive.h > +++ b/merge-recursive.h > @@ -14,7 +14,11 @@ int merge_trees(struct tree *head, > const char *branch1, > const char *branch2, > struct tree **result); > - > +struct commit *make_virtual_commit(struct tree *tree, const char *comment); > +int merge_recursive_config(const char *var, const char *value, void *cb); > +void merge_recursive_setup(int is_subtree_merge); > struct tree *write_tree_from_memory(void); > > +extern int merge_recursive_verbosity; Why this? So we have: 1. "merge.verbosity" config value 2. GIT_MERGE_VERBOSITY environment 3. merge_recursive_verbosity variable I wonder if 3 is really necessary. Kind regards, Stephan -- Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH] Split out merge_recursive() to merge-recursive.c 2008-08-12 17:56 ` Stephan Beyer @ 2008-08-12 21:40 ` Miklos Vajna 0 siblings, 0 replies; 38+ messages in thread From: Miklos Vajna @ 2008-08-12 21:40 UTC (permalink / raw) To: Stephan Beyer; +Cc: Junio C Hamano, git [-- Attachment #1: Type: text/plain, Size: 676 bytes --] On Tue, Aug 12, 2008 at 07:56:29PM +0200, Stephan Beyer <s-beyer@gmx.net> wrote: > > +extern int merge_recursive_verbosity; > > Why this? > > So we have: > 1. "merge.verbosity" config value > 2. GIT_MERGE_VERBOSITY environment > 3. merge_recursive_verbosity variable > > I wonder if 3 is really necessary. Yes, IMHO it is. The idea is that a git_config(merge_recursive_config, NULL); merge_recursive_setup(0); will take merge.verbosity and GIT_MERGE_VERBOSITY into account, finally you can always read merge_recursive_verbosity, and you don't have to deal with the details (if it is set based on GIT_MERGE_VERBOSITY or based on merge.verbosity). [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() 2008-08-12 16:45 ` [PATCH] Split out merge_recursive() to merge-recursive.c Miklos Vajna 2008-08-12 17:56 ` Stephan Beyer @ 2008-08-12 20:13 ` Stephan Beyer 2008-08-12 20:14 ` [PATCH (2)] Make builtin-revert.c use merge_recursive_generic() Stephan Beyer ` (2 more replies) 2008-08-14 3:17 ` [PATCH] Split out merge_recursive() to merge-recursive.c Junio C Hamano 2 siblings, 3 replies; 38+ messages in thread From: Stephan Beyer @ 2008-08-12 20:13 UTC (permalink / raw) To: Miklos Vajna, Junio C Hamano; +Cc: git, Stephan Beyer merge_recursive_generic() takes, in comparison to to merge_recursive(), no commit ("struct commit *") arguments but SHA ids ("unsigned char *"), and no commit list of bases but an array of refs ("const char **"). This makes it more generic in the case that it can also take the SHA of a tree to merge trees without commits, for the bases, the head and the remote. merge_recursive_generic() also handles locking and updating of the index, which is a common use case of merge_recursive(). This patch also rewrites builtin-merge-recursive.c to make use of merge_recursive_generic(). By doing this, I stumbled over the limitation of 20 bases and I've added a warning if this limitation is exceeded. This patch qualifies make_virtual_commit() as static again because this function is not needed anymore outside merge-recursive.c. Signed-off-by: Stephan Beyer <s-beyer@gmx.net> --- Hi, so this patch is marked as (1b) since it is based on Miklos' patch that I called (1a) in my mind :-) I'm unsure if it's good to squash both, but that's the decision of the maintainer - if taken at all. Some further remark that I thought about when writig the commit message: 1. It could be cleaner to choose a "struct string_list" for the base_list in merge_recursive_generic() 2. If we do so, we've had lifted the limitation to 20 bases automatically. 2. Since the patchset that is in work in this thread is for post 1.6.0, I wonder if there should be a patch for the 20 bases limitation in git-merge-recursive for 1.6.0. Either lifting it (ALLOC_GROW foo bar) or printing a warning. Regards, Stephan builtin-merge-recursive.c | 64 +++++++++++++------------------------------- merge-recursive.c | 53 +++++++++++++++++++++++++++++++++++++ merge-recursive.h | 4 ++- 3 files changed, 75 insertions(+), 46 deletions(-) diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c index 8bf2fa5..25f540b 100644 --- a/builtin-merge-recursive.c +++ b/builtin-merge-recursive.c @@ -15,36 +15,13 @@ static const char *better_branch_name(const char *branch) return name ? name : branch; } -static struct commit *get_ref(const char *ref) -{ - unsigned char sha1[20]; - struct object *object; - - if (get_sha1(ref, sha1)) - die("Could not resolve ref '%s'", ref); - object = deref_tag(parse_object(sha1), ref, strlen(ref)); - if (!object) - return NULL; - if (object->type == OBJ_TREE) - return make_virtual_commit((struct tree*)object, - better_branch_name(ref)); - if (object->type != OBJ_COMMIT) - return NULL; - if (parse_commit((struct commit *)object)) - die("Could not parse commit '%s'", sha1_to_hex(object->sha1)); - return (struct commit *)object; -} - int cmd_merge_recursive(int argc, const char **argv, const char *prefix) { - static const char *bases[20]; - static unsigned bases_count = 0; - int i, clean; + const char *bases[21]; + unsigned bases_count = 0; + int i, failed; const char *branch1, *branch2; - struct commit *result, *h1, *h2; - struct commit_list *ca = NULL; - struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); - int index_fd; + unsigned char h1[20], h2[20]; int subtree_merge = 0; if (argv[0]) { @@ -60,10 +37,15 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) die("Usage: %s <base>... -- <head> <remote> ...\n", argv[0]); for (i = 1; i < argc; ++i) { - if (!strcmp(argv[i], "--")) + if (!strcmp(argv[i], "--")) { + bases[bases_count] = NULL; break; - if (bases_count < sizeof(bases)/sizeof(*bases)) + } + if (bases_count < ARRAY_SIZE(bases)-1) bases[bases_count++] = argv[i]; + else + warning("Cannot handle more than %zu bases. " + "Ignoring %s.", ARRAY_SIZE(bases)-1, argv[i]); } if (argc - i != 3) /* "--" "<head>" "<remote>" */ die("Not handling anything other than two heads merge."); @@ -71,8 +53,10 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) branch1 = argv[++i]; branch2 = argv[++i]; - h1 = get_ref(branch1); - h2 = get_ref(branch2); + if (get_sha1(branch1, h1)) + die("Could not resolve ref '%s'", branch1); + if (get_sha1(branch2, h2)) + die("Could not resolve ref '%s'", branch2); branch1 = better_branch_name(branch1); branch2 = better_branch_name(branch2); @@ -80,18 +64,8 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) if (merge_recursive_verbosity >= 3) printf("Merging %s with %s\n", branch1, branch2); - index_fd = hold_locked_index(lock, 1); - - for (i = 0; i < bases_count; i++) { - struct commit *ancestor = get_ref(bases[i]); - ca = commit_list_insert(ancestor, &ca); - } - clean = merge_recursive(h1, h2, branch1, branch2, ca, &result); - - if (active_cache_changed && - (write_cache(index_fd, active_cache, active_nr) || - commit_locked_index(lock))) - die ("unable to write %s", get_index_file()); - - return clean ? 0: 1; + failed = merge_recursive_generic(bases, h1, branch1, h2, branch2); + if (failed < 0) + return 128; /* die() error code */ + return failed; } diff --git a/merge-recursive.c b/merge-recursive.c index c3a57ac..74a9fdc 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1303,6 +1303,59 @@ int merge_recursive(struct commit *h1, return clean; } +static struct commit *get_ref(const unsigned char *sha1, const char *name) +{ + struct object *object; + + object = deref_tag(parse_object(sha1), name, strlen(name)); + if (!object) + return NULL; + if (object->type == OBJ_TREE) + return make_virtual_commit((struct tree*)object, name); + if (object->type != OBJ_COMMIT) + return NULL; + if (parse_commit((struct commit *)object)) + return NULL; + return (struct commit *)object; +} + +int merge_recursive_generic(const char **base_list, + const unsigned char *head_sha1, const char *head_name, + const unsigned char *next_sha1, const char *next_name) +{ + int clean, index_fd; + struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); + struct commit *result; + struct commit *head_commit = get_ref(head_sha1, head_name); + struct commit *next_commit = get_ref(next_sha1, next_name); + struct commit_list *ca = NULL; + + if (base_list) { + int i; + for (i = 0; base_list[i]; ++i) { + unsigned char sha[20]; + struct commit *base; + if (get_sha1(base_list[i], sha)) + return error("Could not resolve ref '%s'", + base_list[i]); + if (!(base = get_ref(sha, base_list[i]))) + return error("Could not parse object '%s'", + base_list[i]); + commit_list_insert(base, &ca); + } + } + + index_fd = hold_locked_index(lock, 1); + clean = merge_recursive(head_commit, next_commit, + head_name, next_name, ca, &result); + if (active_cache_changed && + (write_cache(index_fd, active_cache, active_nr) || + commit_locked_index(lock))) + return error("Unable to write index."); + + return clean ? 0 : 1; +} + int merge_recursive_config(const char *var, const char *value, void *cb) { if (!strcasecmp(var, "merge.verbosity")) { diff --git a/merge-recursive.h b/merge-recursive.h index 73e4413..4dd6476 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -14,7 +14,9 @@ int merge_trees(struct tree *head, const char *branch1, const char *branch2, struct tree **result); -struct commit *make_virtual_commit(struct tree *tree, const char *comment); +extern int merge_recursive_generic(const char **base_list, + const unsigned char *head_sha1, const char *head_name, + const unsigned char *next_sha1, const char *next_name); int merge_recursive_config(const char *var, const char *value, void *cb); void merge_recursive_setup(int is_subtree_merge); struct tree *write_tree_from_memory(void); -- 1.6.0.rc2.281.g6f6cf ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH (2)] Make builtin-revert.c use merge_recursive_generic() 2008-08-12 20:13 ` [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() Stephan Beyer @ 2008-08-12 20:14 ` Stephan Beyer 2008-08-12 21:44 ` [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() Miklos Vajna 2008-08-13 3:17 ` Daniel Barkalow 2 siblings, 0 replies; 38+ messages in thread From: Stephan Beyer @ 2008-08-12 20:14 UTC (permalink / raw) To: Miklos Vajna, Junio C Hamano; +Cc: git, Stephan Beyer Cherry-pick and revert always ran the merging in a separate process. This patch makes cherry-pick/revert call merge_recursive_generic() instead of running a git-merge-recursive process. The GITHEAD_* environment definitions are not needed anymore, since the names are direct arguments to merge_recursive_generic(). Signed-off-by: Stephan Beyer <s-beyer@gmx.net> --- Hi, So this is now based on the new merge-recursive.c based on (1a) and (1b). The number of lines deleted and inserted tells me that we're doing something right here. builtin-revert.c | 50 +++++++++++++------------------------------------- 1 files changed, 13 insertions(+), 37 deletions(-) diff --git a/builtin-revert.c b/builtin-revert.c index 27881e9..2a724ca 100644 --- a/builtin-revert.c +++ b/builtin-revert.c @@ -11,6 +11,7 @@ #include "cache-tree.h" #include "diff.h" #include "revision.h" +#include "merge-recursive.h" /* * This implements the builtins revert and cherry-pick. @@ -200,36 +201,6 @@ static void set_author_ident_env(const char *message) sha1_to_hex(commit->object.sha1)); } -static int merge_recursive(const char *base_sha1, - const char *head_sha1, const char *head_name, - const char *next_sha1, const char *next_name) -{ - char buffer[256]; - const char *argv[6]; - int i = 0; - - sprintf(buffer, "GITHEAD_%s", head_sha1); - setenv(buffer, head_name, 1); - sprintf(buffer, "GITHEAD_%s", next_sha1); - setenv(buffer, next_name, 1); - - /* - * This three way merge is an interesting one. We are at - * $head, and would want to apply the change between $commit - * and $prev on top of us (when reverting), or the change between - * $prev and $commit on top of us (when cherry-picking or replaying). - */ - argv[i++] = "merge-recursive"; - if (base_sha1) - argv[i++] = base_sha1; - argv[i++] = "--"; - argv[i++] = head_sha1; - argv[i++] = next_sha1; - argv[i++] = NULL; - - return run_command_v_opt(argv, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD); -} - static char *help_msg(const unsigned char *sha1) { static char helpbuf[1024]; @@ -266,12 +237,14 @@ static int revert_or_cherry_pick(int argc, const char **argv) { unsigned char head[20]; struct commit *base, *next, *parent; - int i; + int i, fail; char *oneline, *reencoded_message = NULL; + const char *base_list[] = { NULL, NULL }; const char *message, *encoding; const char *defmsg = xstrdup(git_path("MERGE_MSG")); - git_config(git_default_config, NULL); + git_config(merge_recursive_config, NULL); + merge_recursive_setup(0); me = action == REVERT ? "revert" : "cherry-pick"; setenv(GIT_REFLOG_ACTION, me, 0); parse_args(argc, argv); @@ -373,11 +346,14 @@ static int revert_or_cherry_pick(int argc, const char **argv) } } - if (merge_recursive(base == NULL ? - NULL : sha1_to_hex(base->object.sha1), - sha1_to_hex(head), "HEAD", - sha1_to_hex(next->object.sha1), oneline) || - write_cache_as_tree(head, 0, NULL)) { + if (base) + base_list[0] = sha1_to_hex(base->object.sha1); + fail = merge_recursive_generic(base_list, head, "HEAD", + next->object.sha1, oneline); + if (fail < 0) + exit(1); + + if (fail || write_cache_as_tree(head, 0, NULL)) { add_to_msg("\nConflicts:\n\n"); read_cache(); for (i = 0; i < active_nr;) { -- 1.6.0.rc2.281.g6f6cf ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() 2008-08-12 20:13 ` [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() Stephan Beyer 2008-08-12 20:14 ` [PATCH (2)] Make builtin-revert.c use merge_recursive_generic() Stephan Beyer @ 2008-08-12 21:44 ` Miklos Vajna 2008-08-13 17:26 ` Stephan Beyer 2008-08-13 3:17 ` Daniel Barkalow 2 siblings, 1 reply; 38+ messages in thread From: Miklos Vajna @ 2008-08-12 21:44 UTC (permalink / raw) To: Stephan Beyer; +Cc: Junio C Hamano, git [-- Attachment #1: Type: text/plain, Size: 797 bytes --] On Tue, Aug 12, 2008 at 10:13:59PM +0200, Stephan Beyer <s-beyer@gmx.net> wrote: > merge_recursive_generic() takes, in comparison to to merge_recursive(), > no commit ("struct commit *") arguments but SHA ids ("unsigned char *"), > and no commit list of bases but an array of refs ("const char **"). > > This makes it more generic in the case that it can also take the SHA > of a tree to merge trees without commits, for the bases, the head > and the remote. > > merge_recursive_generic() also handles locking and updating of the > index, which is a common use case of merge_recursive(). Then what about adding an extra parameter to merge_recursive_generic() so that merge_recursive_setup() could be a static function? Or is there any reason to keep them separated? Thanks. [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() 2008-08-12 21:44 ` [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() Miklos Vajna @ 2008-08-13 17:26 ` Stephan Beyer 2008-08-13 20:13 ` Miklos Vajna 0 siblings, 1 reply; 38+ messages in thread From: Stephan Beyer @ 2008-08-13 17:26 UTC (permalink / raw) To: Miklos Vajna; +Cc: Junio C Hamano, git [-- Attachment #1: Type: text/plain, Size: 1276 bytes --] Hi, Miklos Vajna wrote: > On Tue, Aug 12, 2008 at 10:13:59PM +0200, Stephan Beyer <s-beyer@gmx.net> wrote: > > merge_recursive_generic() takes, in comparison to to merge_recursive(), > > no commit ("struct commit *") arguments but SHA ids ("unsigned char *"), > > and no commit list of bases but an array of refs ("const char **"). > > > > This makes it more generic in the case that it can also take the SHA > > of a tree to merge trees without commits, for the bases, the head > > and the remote. > > > > merge_recursive_generic() also handles locking and updating of the > > index, which is a common use case of merge_recursive(). > > Then what about adding an extra parameter to merge_recursive_generic() > so that merge_recursive_setup() could be a static function? Could it? I did not intend to replace merge_recursive() by merge_recursive_generic(), because merge_recursive() may be the better choice in cases where the caller only deals with commit objects and never with tree objects directly. Or if the caller does not want to lock the index or do some other stuff with the index... In that case merge_recursive_setup() is still needed, isn't it? Regards, Stephan -- Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() 2008-08-13 17:26 ` Stephan Beyer @ 2008-08-13 20:13 ` Miklos Vajna 0 siblings, 0 replies; 38+ messages in thread From: Miklos Vajna @ 2008-08-13 20:13 UTC (permalink / raw) To: Stephan Beyer; +Cc: Junio C Hamano, git [-- Attachment #1: Type: text/plain, Size: 594 bytes --] On Wed, Aug 13, 2008 at 07:26:22PM +0200, Stephan Beyer <s-beyer@gmx.net> wrote: > I did not intend to replace merge_recursive() by merge_recursive_generic(), > because merge_recursive() may be the better choice in cases where the caller > only deals with commit objects and never with tree objects directly. > Or if the caller does not want to lock the index or do some other stuff > with the index... > In that case merge_recursive_setup() is still needed, isn't it? Hm, OK. I was not aware pure merge_recursive() will be interesting for other builtins even after merge_recursive_generic(). [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() 2008-08-12 20:13 ` [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() Stephan Beyer 2008-08-12 20:14 ` [PATCH (2)] Make builtin-revert.c use merge_recursive_generic() Stephan Beyer 2008-08-12 21:44 ` [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() Miklos Vajna @ 2008-08-13 3:17 ` Daniel Barkalow 2008-08-13 17:29 ` Stephan Beyer 2 siblings, 1 reply; 38+ messages in thread From: Daniel Barkalow @ 2008-08-13 3:17 UTC (permalink / raw) To: Stephan Beyer; +Cc: Miklos Vajna, Junio C Hamano, git On Tue, 12 Aug 2008, Stephan Beyer wrote: > merge_recursive_generic() takes, in comparison to to merge_recursive(), > no commit ("struct commit *") arguments but SHA ids ("unsigned char *"), > and no commit list of bases but an array of refs ("const char **"). > > This makes it more generic in the case that it can also take the SHA > of a tree to merge trees without commits, for the bases, the head > and the remote. > > merge_recursive_generic() also handles locking and updating of the > index, which is a common use case of merge_recursive(). > > This patch also rewrites builtin-merge-recursive.c to make use of > merge_recursive_generic(). By doing this, I stumbled over the > limitation of 20 bases and I've added a warning if this limitation > is exceeded. > > This patch qualifies make_virtual_commit() as static again because > this function is not needed anymore outside merge-recursive.c. You might look at builtin-checkout and see if merge_working_tree() could be made cleaner with this API, or if some other API could accomodate both cases more nicely. (I'm not sure either way, but it would be a good confirmation that the API is properly convenient if that additional case could use it.) -Daniel *This .sig left intentionally blank* ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() 2008-08-13 3:17 ` Daniel Barkalow @ 2008-08-13 17:29 ` Stephan Beyer 2008-08-13 17:54 ` Daniel Barkalow 0 siblings, 1 reply; 38+ messages in thread From: Stephan Beyer @ 2008-08-13 17:29 UTC (permalink / raw) To: Daniel Barkalow; +Cc: Miklos Vajna, Junio C Hamano, git Hi, Daniel Barkalow wrote: > You might look at builtin-checkout and see if merge_working_tree() could > be made cleaner with this API, or if some other API could accomodate both > cases more nicely. Puhh, I've not dug into merging stuff that deep, but for me it does not look that this can be done in a useful way, i.e. merge_working_tree() does not do a recursive merge. Regards. -- Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() 2008-08-13 17:29 ` Stephan Beyer @ 2008-08-13 17:54 ` Daniel Barkalow 2008-08-13 19:55 ` Junio C Hamano 0 siblings, 1 reply; 38+ messages in thread From: Daniel Barkalow @ 2008-08-13 17:54 UTC (permalink / raw) To: Stephan Beyer; +Cc: Miklos Vajna, Junio C Hamano, git On Wed, 13 Aug 2008, Stephan Beyer wrote: > Hi, > > Daniel Barkalow wrote: > > You might look at builtin-checkout and see if merge_working_tree() could > > be made cleaner with this API, or if some other API could accomodate both > > cases more nicely. > > Puhh, I've not dug into merging stuff that deep, but for me it does not > look that this can be done in a useful way, i.e. merge_working_tree() > does not do a recursive merge. Ah, true. It's actually doing a single merge in the way that merge_recursive would do a single merge. I think it ought to be doing a recursive merge, but that's probably a change for later, anyway. (This is for -m, which essentially picks the uncommited changes versus the old branch, applied to the new branch uncommitted) -Daniel *This .sig left intentionally blank* ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() 2008-08-13 17:54 ` Daniel Barkalow @ 2008-08-13 19:55 ` Junio C Hamano 2008-08-13 20:05 ` Stephan Beyer 2008-08-13 20:36 ` Daniel Barkalow 0 siblings, 2 replies; 38+ messages in thread From: Junio C Hamano @ 2008-08-13 19:55 UTC (permalink / raw) To: Daniel Barkalow; +Cc: Stephan Beyer, Miklos Vajna, git Daniel Barkalow <barkalow@iabervon.org> writes: >> Puhh, I've not dug into merging stuff that deep, but for me it does not >> look that this can be done in a useful way, i.e. merge_working_tree() >> does not do a recursive merge. > > Ah, true. It's actually doing a single merge in the way that > merge_recursive would do a single merge. I think it ought to be doing > a recursive merge, but that's probably a change for later, anyway. (This > is for -m, which essentially picks the uncommited changes versus the old > branch, applied to the new branch uncommitted) Why would you think it should be doing a recursive merge? It shouldn't. Think of builtin-merge-recursive.c::merge_trees() as a fancier version of 3-tree variant of "unpack_trees()", with -m and -u option. When you want to perform an exact three-way merge (i.e. you have two states O and B, and you want to apply changes between O and B to your state A, and you _precisely_ know what O is) that's the interface you would want to use, not the recursive one. The recursive behaviour is desirable only when you have A and B and need to infer where O should be, and/or there are multiple O's to deal with (i.e. running "git-merge B" when you are at A). In all the potential users of merge-recursive machinery, namely, "revert", "cherry-pick", "stash apply", "am -3", and "checkout -m", you know what single common tree to use for your three-way merge. These operations, when done with direct C call into merge machinery, should NOT be using the "recursive" one. When you switch branches from A to B with checkout, and you have local changes A', then you would want an exact three-way merge that modifies B by applying changes from A to A'. When you cherry-pick commit C on top of your current HEAD, you want an exact three-way merge that modifies your HEAD by applying changes from C^ to C, and you do not want the merge machinery to take ancestry relation (and criss cross merges) between HEAD and C into account at all. The scripted version of revert/cherry-pick used git-merge-recursive because that is the Porcelain API available, and the current C-rewrite uses it as well, but if we are rewriting it to call merge-recursive machinery directly, it should be making a single merge request to merge_trees(), not "recursive" one. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() 2008-08-13 19:55 ` Junio C Hamano @ 2008-08-13 20:05 ` Stephan Beyer 2008-08-13 20:36 ` Daniel Barkalow 1 sibling, 0 replies; 38+ messages in thread From: Stephan Beyer @ 2008-08-13 20:05 UTC (permalink / raw) To: Junio C Hamano; +Cc: Daniel Barkalow, Miklos Vajna, git Hi, Junio C Hamano wrote: > The scripted version of revert/cherry-pick used git-merge-recursive > because that is the Porcelain API available, and the current C-rewrite > uses it as well, but if we are rewriting it to call merge-recursive > machinery directly, it should be making a single merge request to > merge_trees(), not "recursive" one. This is an interesting point. So perhaps we don't need stuff like my regarding merge_recursive_generic(), at least not for revert/cherry-pick. Regards, Stephan -- Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() 2008-08-13 19:55 ` Junio C Hamano 2008-08-13 20:05 ` Stephan Beyer @ 2008-08-13 20:36 ` Daniel Barkalow 2008-08-13 21:45 ` Junio C Hamano 1 sibling, 1 reply; 38+ messages in thread From: Daniel Barkalow @ 2008-08-13 20:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: Stephan Beyer, Miklos Vajna, git On Wed, 13 Aug 2008, Junio C Hamano wrote: > Daniel Barkalow <barkalow@iabervon.org> writes: > > >> Puhh, I've not dug into merging stuff that deep, but for me it does not > >> look that this can be done in a useful way, i.e. merge_working_tree() > >> does not do a recursive merge. > > > > Ah, true. It's actually doing a single merge in the way that > > merge_recursive would do a single merge. I think it ought to be doing > > a recursive merge, but that's probably a change for later, anyway. (This > > is for -m, which essentially picks the uncommited changes versus the old > > branch, applied to the new branch uncommitted) > > Why would you think it should be doing a recursive merge? It shouldn't. > > Think of builtin-merge-recursive.c::merge_trees() as a fancier version of > 3-tree variant of "unpack_trees()", with -m and -u option. > > When you want to perform an exact three-way merge (i.e. you have two > states O and B, and you want to apply changes between O and B to your > state A, and you _precisely_ know what O is) that's the interface you > would want to use, not the recursive one. The recursive behaviour is > desirable only when you have A and B and need to infer where O should be, > and/or there are multiple O's to deal with (i.e. running "git-merge B" > when you are at A). I understand that you know exactly what the change is, but I'm not convinced that you don't want to consider how O is related to A in determining who to apply that change to A. My naive impression was: - the difference between O and B is to change X to Y, and we can determine this exactly - we need to find X' in A to change it to Y' - we can more accurately find X' in A if we find Z in the common ancestors of O and A such that X in O corresponds to Z in the ancestors, and we get similar benefits in determining the right Y' That is, we care about the common ancestor not only for finding the relevant change in the near side to apply, but also for finding how to apply it to the far side. > In all the potential users of merge-recursive machinery, namely, "revert", > "cherry-pick", "stash apply", "am -3", and "checkout -m", you know what > single common tree to use for your three-way merge. These operations, > when done with direct C call into merge machinery, should NOT be using the > "recursive" one. I agree that they should all be using the same mechanism, but I'm not sure it shouldn't be recursive or take history into account. And I'd certainly believe that just running our merge-recursive won't actually do anything clever with the rest of the history, and that merge_trees() is the most suitable method currently available (which is essentially why I ended up using it in checkout -m, not entirely realizing that I'd removed the "recursive" aspect in the process of skipping parts of merge_recursive that weren't relevant). Maybe merge_trees should get the two sides as structs with a struct tree * and a char * branch name, and the struct could someday get optional struct commit *s for the lineage of the side if somebody comes up with a method for merging that makes use of the component changes. -Daniel *This .sig left intentionally blank* ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() 2008-08-13 20:36 ` Daniel Barkalow @ 2008-08-13 21:45 ` Junio C Hamano 0 siblings, 0 replies; 38+ messages in thread From: Junio C Hamano @ 2008-08-13 21:45 UTC (permalink / raw) To: Daniel Barkalow; +Cc: Stephan Beyer, Miklos Vajna, git Daniel Barkalow <barkalow@iabervon.org> writes: > Maybe merge_trees should get the two sides as structs with a struct tree * > and a char * branch name, and the struct could someday get optional struct > commit *s for the lineage of the side if somebody comes up with a method > for merging that makes use of the component changes. I do agree with you that with some ancestry-based hinting the "find renames" part of merge_trees() postprocessing can do a better job than the current code. Contrary to a widespread misconception we often hear on this list and on #git, merge-recursive detects renames solely by looking at the three endpoints. Some people incorrectly recommend "commit a pure rename, then commit modifications to the renamed path"; such an artificial split would not at all help merging histories with renames if the modification made after renames are too great. Suppose you have this history, where upper branch renames a path and modifies the contents in the renamed path in commits X, Y and Z. You would want to merge the history leading to B to your HEAD, A, to create a merge M: X---Y---Z---B X---Y---Z---B / ===> / \ ---O---o---o---A ---O---o---o---A---M Maybe the rename done between O and X were pure enough that "diff-tree -M O X" would have found it, but commits Y, Z, or B changed the contents of the renamed path too greatly for "diff-tree -M O B" to notice the rename anymore. "git merge B" when you are at A will not find such a rename, but if we allowed rename detection stepwise to look at "diff-tree -M" for (O,X), (X,Y), (Y,Z), (Z,B), (and the same for history between O and A), we might be able to find renames better [*1*]. Suppose instead of merging the whole history leading to B, you would want to apply the small fix made with Y on top of A: X---Y---Z---B X---Y---Z---B / ===> / ---O---o---o---A ---O---o---o---A---Y' You would "checkout A && cherry-pick Y" which amounts to three-way merge using X as "common", A as "mine" and Y as "his", which is the moral equivalent of: read-tree -m -u X A Y but with rename detection. merge_trees(A, Y, X) could traverse ancestry between X and A to find O and figure out where in A the paths that are affected in diff(X,Y) appear by making pairwise "diff-tree -M" to go from X back to O and then forward to A to find out that where the paths touched by Y were originally in O and where they are in A. But you are assuming that "common" and "ours" have any ancestry relationship. You generally cannot. The most extreme case is "am -3" where both "common" and "theirs" are pure trees without any ancestry relationship to anything else. They are built by looking at the "index" lines contained in the patch and contain only those paths that are affected by the patch, and the merge machinery merges that change into arbitrary "ours". You can cherry-pick a change C from history that does not have any common ancestor with your history leading to "ours" ("common"=C^ and "theirs"=C in this case), and the same applies to revert of C ("common"=C and "theirs"=C^). "rebase --onto A C^ C" works the same way. So this "extra ancestry information to help rename discovery" can at most be a hint. More importantly, all of the above does not have anything to do with the "recursiveness" of merge-recursive (which is the difference between merge_trees() and merge_recursive()) at all. So I am still correct to say that "cherry-pick", "revert", "checkout -m", "am -3" and "stash apply" should use the merge_trees() interface, not the recursive one. [Footnote] *1* But that would be quite more expensive than what we currently do, and it only deals with an uninteresting special case of wholesale file rename. I suspect if we ever do the stepwise thing, we would be better off doing not "diff-tree -M" but "blame -C" to really find where the lines affected between O and B ended up in A, and apply the change there. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] Split out merge_recursive() to merge-recursive.c 2008-08-12 16:45 ` [PATCH] Split out merge_recursive() to merge-recursive.c Miklos Vajna 2008-08-12 17:56 ` Stephan Beyer 2008-08-12 20:13 ` [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() Stephan Beyer @ 2008-08-14 3:17 ` Junio C Hamano 2 siblings, 0 replies; 38+ messages in thread From: Junio C Hamano @ 2008-08-14 3:17 UTC (permalink / raw) To: Miklos Vajna; +Cc: Stephan Beyer, git Miklos Vajna <vmiklos@frugalware.org> writes: > Move most of the of code from builtin-merge-recursive.c to a new file > merge-recursive.c and introduce merge_recursive_setup() in there so that > builtin-merge-recursive and other builtins call it. > > Signed-off-by: Miklos Vajna <vmiklos@frugalware.org> > --- > > On Mon, Aug 11, 2008 at 04:27:01PM -0700, Junio C Hamano <gitster@pobox.com> wrote: >> (1) move bulk of code from builtin-merge-recursive.c to a new file >> merge-recursive.c and introduce merge_recursive_helper() in there >> so >> that both of you and cmd_merge_recursive() itself can call it; > > Something like this? Hmm, I think callers except "git merge" implementation should not be even using the "recursive" variant, so in that sense, I do not think you would need to expose anything but merge_trees(). But the new file is called merge-recursive.c not merge-trees.c, and in any case, builtin-merge.c needs to link to the recursive one, so this split is Ok. Exposure of make_virtual_commit() somewhat feels dirty, and I tend to agree that the pure "three-tree merge with rename detection" (aka merge_trees()) should operate on three trees, not commits, though. ^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2008-08-14 3:18 UTC | newest] Thread overview: 38+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-08-10 13:20 [PATCH 0/2] Avoid run_command() for recursive in builtin-merge Miklos Vajna 2008-08-10 13:20 ` [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins Miklos Vajna 2008-08-10 13:20 ` [PATCH 2/2] builtin-merge: avoid run_command_v_opt() for recursive Miklos Vajna 2008-08-11 18:47 ` Junio C Hamano 2008-08-11 19:07 ` Miklos Vajna 2008-08-11 20:03 ` Junio C Hamano 2008-08-11 20:45 ` Miklos Vajna 2008-08-11 20:48 ` [PATCH] Add a new test to ensure merging a submodule is handled properly Miklos Vajna 2008-08-11 15:13 ` [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins Stephan Beyer 2008-08-11 16:46 ` Miklos Vajna 2008-08-11 19:53 ` Junio C Hamano 2008-08-11 20:46 ` Stephan Beyer 2008-08-11 15:03 ` [PATCH] builtin-revert.c: Make use of merge_recursive() Stephan Beyer 2008-08-11 15:47 ` Johannes Schindelin 2008-08-11 19:01 ` Stephan Beyer 2008-08-11 19:09 ` Miklos Vajna 2008-08-11 21:44 ` [PATCH] builtin-revert: " Stephan Beyer 2008-08-11 21:46 ` Stephan Beyer 2008-08-11 22:33 ` Junio C Hamano 2008-08-11 23:27 ` Junio C Hamano 2008-08-11 23:47 ` Stephan Beyer 2008-08-11 23:52 ` Junio C Hamano 2008-08-12 16:45 ` [PATCH] Split out merge_recursive() to merge-recursive.c Miklos Vajna 2008-08-12 17:56 ` Stephan Beyer 2008-08-12 21:40 ` Miklos Vajna 2008-08-12 20:13 ` [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() Stephan Beyer 2008-08-12 20:14 ` [PATCH (2)] Make builtin-revert.c use merge_recursive_generic() Stephan Beyer 2008-08-12 21:44 ` [PATCH (1b)] merge-recursive.c: Add more generic merge_recursive_generic() Miklos Vajna 2008-08-13 17:26 ` Stephan Beyer 2008-08-13 20:13 ` Miklos Vajna 2008-08-13 3:17 ` Daniel Barkalow 2008-08-13 17:29 ` Stephan Beyer 2008-08-13 17:54 ` Daniel Barkalow 2008-08-13 19:55 ` Junio C Hamano 2008-08-13 20:05 ` Stephan Beyer 2008-08-13 20:36 ` Daniel Barkalow 2008-08-13 21:45 ` Junio C Hamano 2008-08-14 3:17 ` [PATCH] Split out merge_recursive() to merge-recursive.c Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).