{"thread":{"id":"6218","subject":"[PATCH] Speedup recursive by flushing index only once for all entries","startedAt":"2007-01-04T10:47:19Z","lastAt":"2007-01-13T11:01:27Z","messageCount":40,"participants":["Alex Riesen","Johannes Schindelin","Junio C Hamano","Linus Torvalds","Sergey Vlasov","Jakub Narebski","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"30809","messageId":"81b0412b0701040247k47e398e6q34dd5233bb5706f6@mail.gmail.com","threadId":"6218","inReplyTo":null,"subject":"[PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-01-04T10:47:19Z","receivedAt":"2007-01-04T10:47:19Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Merge-recursive is very slow in repos with lots of files,\nespecially if lots of them change absolutely identically.\nUpdating index once after all of them changes speedups\nmerge quite noticable.\n\nSigned-off-by: Alex Riesen \n---\n\nJohannes, I remember suggesting to do index flush for all\nentries instead for every entry. It is already quite time ago,\nbut ... was there any reasons for not doing this?\nThe patch speeds it up a lot and no wonder: index is 6Mb\nhere, and this is cygwin.\n\n merge-recursive.c | 5 ++---\n 1 files changed, 2 insertions(+), 3 deletions(-)\n\n\nFrom d0e4d791cef8b307b32954a8b80c4aabd41755a9 Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen \nDate: Thu, 4 Jan 2007 11:22:47 +0100\nSubject: Speedup recursive by flushing index only once for all entries\n\nSigned-off-by: Alex Riesen \n---\n\n merge-recursive.c | 5 ++---\n 1 files changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex bac16f5..4d3a2ce 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1083,9 +1083,6 @@ static int process_entry(const char *path, struct stage_data *entry,\n \t} else\n \t\tdie(\"Fatal merge failure, shouldn't happen.\");\n \n-\tif (cache_dirty)\n-\t\tflush_cache();\n-\n \treturn clean_merge;\n }\n \n@@ -1133,6 +1130,8 @@ static int merge_trees(struct tree *head,\n \t\t\tif (!process_entry(path, e, branch1, branch2))\n \t\t\t\tclean = 0;\n \t\t}\n+\t\tif (cache_dirty)\n+\t\t\tflush_cache();\n \n \t\tpath_list_clear(re_merge, 0);\n \t\tpath_list_clear(re_head, 0);\n-- \n1.5.0.rc0.g8bc4b-dirty\n\n"},{"id":"30810","messageId":"Pine.LNX.4.63.0701041327490.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6218","inReplyTo":"81b0412b0701040247k47e398e6q34dd5233bb5706f6@mail.gmail.com","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-01-04T12:33:26Z","receivedAt":"2007-01-04T12:33:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 4 Jan 2007, Alex Riesen wrote:\n\n> Johannes, I remember suggesting to do index flush for all\n> entries instead for every entry. It is already quite time ago,\n> but ... was there any reasons for not doing this?\n\nI wanted to be on the safe side, and eventually look through the code \nagain for possible problems.\n\nI think what you did is safe, since you moved the call from \nprocess_entry() to its sole caller, merge_trees().\n\nHowever, I was wondering if the index has to be written at all. \nI expect the written index (except the last one, of course) to have no \nuser...\n\nCiao,\nDscho\n"},{"id":"30811","messageId":"81b0412b0701040447u329dcf9bvcd7adb9e9d199f18@mail.gmail.com","threadId":"6218","inReplyTo":"Pine.LNX.4.63.0701041327490.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-01-04T12:47:08Z","receivedAt":"2007-01-04T12:47:08Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 1/4/07, Johannes Schindelin wrote:\n>\n> > Johannes, I remember suggesting to do index flush for all\n> > entries instead for every entry. It is already quite time ago,\n> > but ... was there any reasons for not doing this?\n>\n> I wanted to be on the safe side, and eventually look through the code\n> again for possible problems.\n>\n> I think what you did is safe, since you moved the call from\n> process_entry() to its sole caller, merge_trees().\n\nMe too, just wondered why didn't we do this back then.\nAnyway, my \"monster-merge\" and the builtin tests pass with\nno visible problems.\n\n> However, I was wondering if the index has to be written at all.\n> I expect the written index (except the last one, of course) to have no\n> user...\n\nGood question...\n"},{"id":"30833","messageId":"7v8xgileza.fsf@assigned-by-dhcp.cox.net","threadId":"6218","inReplyTo":"81b0412b0701040447u329dcf9bvcd7adb9e9d199f18@mail.gmail.com","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-04T20:22:01Z","receivedAt":"2007-01-04T20:22:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Riesen\" writes:\n\n> Me too, just wondered why didn't we do this back then.\n> Anyway, my \"monster-merge\" and the builtin tests pass with\n> no visible problems.\n>\n>> However, I was wondering if the index has to be written at all.\n>> I expect the written index (except the last one, of course) to have no\n>> user...\n>\n> Good question...\n\nThat's most likely because you played safe, and started from the\nPython version whose only way to manipulate the index and write\nout a tree was to actually write the index out.\n\nSo let's step back a bit.\n\n * The top-level interface is merge() function that takes two\n heads and the ancestor, and is responsible for coming up with\n a merged tree in *result if it can. The main() calls it to\n see if things merge cleanly, and wants the index populated\n with the final merge result, be it clean or unmerged.\n\n * merge() in addition to two heads and their ancestor takes\n call-depth because it does the \"recursive\" business. When it\n operates with positive call-depth, it is coming up with a\n virtual commit that has the merge result tree of two merge\n bases. In this case index_only is set to true and what is in\n the working tree does not matter (i.e. usual \"your local\n modification will be clobbered, cannot merge\" check should\n not apply [*1*]). It calls setup_index() to read in the true\n index (for the outermost case) or a temporary index (from one\n of the ancestor trees in the recursive case) before passing\n control to the real workhorse, merge_trees().\n\n What merge() does before its call to setup_index() does not\n depend on what the index is (even in recursive case, because\n the real work done by merge_trees() in the recursively called\n merge() is preceded by the call to setup_index()). When\n merge() returns, the index contents, both in-core and on the\n filesystem, matter only for the outermost call to merge().\n\n So that suggests that flush_cache() in main() needs to stay.\n\n * merge_trees() takes the two heads and the ancestor, and comes\n up with the merge result, using in-core index. It calls\n git_merge_trees() which is 3-way \"read-tree -m\", and calls\n git_write_tree() with two purposes: (1) to see if the merge\n was cleanly done, and (2) to get the tree to be used as the\n virtual ancestor (in other words, for the outermost merge,\n writing of the tree is not important -- a tree is produced as\n an unused side effect of seeing if the merge was clean). If\n it results in a clean merge for the outermost merge, the call\n to flush_cache() in main() is what matters -- the calling\n script of merge-recursive writes a tree out from the index\n written by that call. I'll address the need for\n git_write_tree() to write the index shortly.\n\n If the call to git_merge_trees() results in conflicted merge,\n merge_trees() falls into the rename detection codepath. Most\n of the work is done in process_renames(), which updates the\n index, and then remaining unmatched paths are dealt with by\n calling process_entry(), which also updates the index. These\n index updates could all be done in-core without writing the\n resulting index file out; we should not discard nor re-read\n the index while they are processing one path at a time.\n\n In a sense, merge_trees() does what 3-way \"read-tree -m\"\n could have done if it were rename aware.\n\n * git_merge_trees() is a bit questionable. It reads from the\n current_index_file which is the true index for the outermost\n merge or the temporary index populated from the 'head'\n parameter give to it earlier by merge(). I think this use of\n temporary index is not necessary. In other words, we could\n start from an empty cache if index_only, I think.\n\n And I think the reason git_write_tree() writes the index out\n is because of this call to read_cache_from() at the beginning\n of git_merge_trees(). It is told to write a tree out of the\n current in-core index -- so I do not know why it needs to\n call read_cache_from() to begin with.\n\nGiven the above analysis, it seems to me that the current code\ntoo heavily inherited the invariant that the in-core and on\nfilesystem index should match at every step from the original\nPython version. I think your patch goes in the right direction\nto correct that, but it does not spell out the new invariant the\ncode assumes cleanly enough. For example, I do not think you\nwould want the call to flush_cache() in process_renames() for\nthe same reason you took out the call from process_entry() --\nthey both make many calls to update_file() and remove_file() to\ntouch the index and the working tree.\n\nHow about making the invariants to be:\n\n\tupon entry of merge(), the in-core cache is populated as\n\tappropriate for the merge. That is, it has the contents\n\tof the true index for the outermost one, and discarded\n\tfor the virtual ancestor merge.\n\n\tupon exit from merge(), the in-core cache holds the\n\tmerge result for that round of merge. That is, it is\n\tsuitable for flush_cache() to leave the final result for\n\tthe outermost merge, and it is a merged index that wrote\n\tthe virtual ancestor tree was written out from for inner\n\tmerges.\n\nThe codeflow would then become like this:\n\n\tmain() {\n hold_lock_file_for_update(lock, git_path(\"index\"), 1);\n merge();\n\t\twrite_cache() || close || commit_lock_file();\n\t}\n\n\tmerge() {\n \twhile (more than one ancestor) {\n\t\t\tdiscard_cache();\n merge(two ancestors using their common);\n\t\t}\n\t\tdiscard_cache();\n\t\tif (call_depth == 0) {\n \tread_cache();\n index_only = 0;\n\t\t}\n\t\telse\n \tindex_only = 1;\n merge_trees();\n\t}\n\n merge_trees() {\n\t\tif (up to date)\n \treturn;\n\t\tgit_merge_trees();\n\t\tif (in-core index is unmerged) {\n\t\t\tprocess_renames();\n\t\t}\n if (index_only)\n \tgit_write_tree();\n }\n\n git_merge_trees() {\n \tunpack_trees();\n }\n\n git_write_tree() {\n\t\tif (stale cache tree)\n \tcache_tree_update();\n lookup_tree();\n\t}\n\n process_renames(), process_entry() {\n\t\tcall remove_file() and update_file() as needed,\n\t\ttrusting that the caller set up the in-core\n index as appropriate and previous calls to these\n functions left the in-core index to correctly\n reflect what have been done so far.\n }\n\nBy the way, Alex, you seem to heavily work on Cygwin, and I am\ninterested in your experience with Shawn's sliding mmap() on\nreal projects, as I suspect Cygwin would be more heavily\nimpacted with any mmap() related changes. You already said\nperformance is \"bearable\". Does that mean it was better and\ngot worse but still bearable, or it was unusably slow but now it\nis bearably usable?\n"},{"id":"30879","messageId":"81b0412b0701050322u67131900xea969b2da9981a94@mail.gmail.com","threadId":"6218","inReplyTo":"7v8xgileza.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-01-05T11:22:39Z","receivedAt":"2007-01-05T11:22:39Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 1/4/07, Junio C Hamano wrote:\n> >> However, I was wondering if the index has to be written at all.\n> >> I expect the written index (except the last one, of course) to have no\n> >> user...\n> >\n> > Good question...\n>\n> That's most likely because you played safe, and started from the\n> Python version whose only way to manipulate the index and write\n> out a tree was to actually write the index out.\n\nYes, it was because of that. Just didn't thought about when to\nupdate index at all (never really had a time).\n\n> So let's step back a bit.\n\nExcellent analysis, thanks! I suspect heavily it will work as is.\nNow, if only someone could find time to code it up...\n\n> By the way, Alex, you seem to heavily work on Cygwin, and I am\n> interested in your experience with Shawn's sliding mmap() on\n> real projects, as I suspect Cygwin would be more heavily\n> impacted with any mmap() related changes. You already said\n> performance is \"bearable\". Does that mean it was better and\n> got worse but still bearable, or it was unusably slow but now it\n> is bearably usable?\n\nIt is usably slow: ~30 sec for a commit (I stopped using normal\ncommit, using update-index and simplified index commit now),\naround minute for the recursive merges (if they are simple),\n~10 sec for a hot-cache hard reset. Avoiding gitk whenever\npossible.\n\nCompared to my only linux system here:\n\n2-3 times slower in diff-tree for 44K files, around 9k differences\n(0.2 sec against 0.6 sec, it quickly adds up if you do it often,\nlike when merging (it's slower for really big merges, constrained\nby CPU and memory), commiting, gitk).\n\nThe windows machine is a corporate Lenovo 2.66MHz/1Gb,SATA laptop.\nThe linux machine is 1.2MHz/384Mb, IDE noname notebook.\n\nI somehow adapted myself to it though (reading mails, drinking coffee).\n"},{"id":"31057","messageId":"20070107163112.GA9336@steel.home","threadId":"6218","inReplyTo":"81b0412b0701050322u67131900xea969b2da9981a94@mail.gmail.com","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Alex Riesen","fromEmail":"fork0@t-online.de","sentAt":"2007-01-07T16:31:12Z","receivedAt":"2007-01-07T16:31:12Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Alex Riesen, Fri, Jan 05, 2007 12:22:39 +0100:\n> >So let's step back a bit.\n> \n> Excellent analysis, thanks! I suspect heavily it will work as is.\n> Now, if only someone could find time to code it up...\n> \n\nI'm sorry for asking (because I'm partly guilty in the mess the\nmerge-recursive is), but could you accept at least the patch which\nstarted the thread? It's not as if it breaks something, or giving\nwrong ideas, or anything. It's incomplete, but so long noone seem to\nbe able to find the time to finish the job, the patch will improve the\nstate of affairs a bit, will it not?\n"},{"id":"31401","messageId":"7v3b6ibvtc.fsf@assigned-by-dhcp.cox.net","threadId":"6218","inReplyTo":"20070107163112.GA9336@steel.home","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-10T18:06:39Z","receivedAt":"2007-01-10T18:06:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"fork0@t-online.de (Alex Riesen) writes:\n\n> Alex Riesen, Fri, Jan 05, 2007 12:22:39 +0100:\n>> >So let's step back a bit.\n>> \n>> Excellent analysis, thanks! I suspect heavily it will work as is.\n>> Now, if only someone could find time to code it up...\n>> \n>\n> I'm sorry for asking (because I'm partly guilty in the mess the\n> merge-recursive is), but could you accept at least the patch which\n> started the thread? It's not as if it breaks something, or giving\n> wrong ideas, or anything. It's incomplete, but so long noone seem to\n> be able to find the time to finish the job, the patch will improve the\n> state of affairs a bit, will it not?\n\nI'm hacking on this right now.\n"},{"id":"31407","messageId":"7vr6u2adgx.fsf@assigned-by-dhcp.cox.net","threadId":"6218","inReplyTo":"20070107163112.GA9336@steel.home","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-10T19:28:14Z","receivedAt":"2007-01-10T19:28:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This comes on top of yours. \n\nI'm reproducing all the merges in linux-2.6 history to make sure\nthe base one, yours and this produce the same result (the same\nclean merge, or the same unmerged index and the same diff from\nHEAD). So far it is looking good.\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Wed, 10 Jan 2007 11:20:58 -0800\nSubject: [PATCH] merge-recursive: do not use on-file index when not needed.\n\nThis revamps the merge-recursive implementation following the\noutline in:\n\n\tMessage-ID: <7v8xgileza.fsf@assigned-by-dhcp.cox.net>\n\nThere is no need to write out the index until the very end just\nonce from merge-recursive. Also there is no need to write out\nthe resulting tree object for the simple case of merging with a\nsingle merge base.\n\nSigned-off-by: Junio C Hamano \n---\n merge-recursive.c | 169 ++++++++++++++--------------------------------------\n 1 files changed, 46 insertions(+), 123 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex aab4c34..5237021 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -110,35 +110,6 @@ static void output_commit_title(struct commit *commit)\n \t}\n }\n \n-static const char *current_index_file = NULL;\n-static const char *original_index_file;\n-static const char *temporary_index_file;\n-static int cache_dirty = 0;\n-\n-static int flush_cache(void)\n-{\n-\t/* flush temporary index */\n-\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n-\tint fd = hold_lock_file_for_update(lock, current_index_file, 1);\n-\tif (write_cache(fd, active_cache, active_nr) ||\n-\t\t\tclose(fd) || commit_lock_file(lock))\n-\t\tdie (\"unable to write %s\", current_index_file);\n-\tdiscard_cache();\n-\tcache_dirty = 0;\n-\treturn 0;\n-}\n-\n-static void setup_index(int temp)\n-{\n-\tcurrent_index_file = temp ? temporary_index_file: original_index_file;\n-\tif (cache_dirty) {\n-\t\tdiscard_cache();\n-\t\tcache_dirty = 0;\n-\t}\n-\tunlink(temporary_index_file);\n-\tdiscard_cache();\n-}\n-\n static struct cache_entry *make_cache_entry(unsigned int mode,\n \t\tconst unsigned char *sha1, const char *path, int stage, int refresh)\n {\n@@ -167,9 +138,6 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,\n \t\tconst char *path, int stage, int refresh, int options)\n {\n \tstruct cache_entry *ce;\n-\tif (!cache_dirty)\n-\t\tread_cache_from(current_index_file);\n-\tcache_dirty++;\n \tce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage, refresh);\n \tif (!ce)\n \t\treturn error(\"cache_addinfo failed: %s\", strerror(cache_errno));\n@@ -187,26 +155,6 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,\n */\n static int index_only = 0;\n \n-static int git_read_tree(struct tree *tree)\n-{\n-\tint rc;\n-\tstruct object_list *trees = NULL;\n-\tstruct unpack_trees_options opts;\n-\n-\tif (cache_dirty)\n-\t\tdie(\"read-tree with dirty cache\");\n-\n-\tmemset(&opts, 0, sizeof(opts));\n-\tobject_list_append(&tree->object, &trees);\n-\trc = unpack_trees(trees, &opts);\n-\tcache_tree_free(&active_cache_tree);\n-\n-\tif (rc == 0)\n-\t\tcache_dirty = 1;\n-\n-\treturn rc;\n-}\n-\n static int git_merge_trees(int index_only,\n \t\t\t struct tree *common,\n \t\t\t struct tree *head,\n@@ -216,11 +164,6 @@ static int git_merge_trees(int index_only,\n \tstruct object_list *trees = NULL;\n \tstruct unpack_trees_options opts;\n \n-\tif (!cache_dirty) {\n-\t\tread_cache_from(current_index_file);\n-\t\tcache_dirty = 1;\n-\t}\n-\n \tmemset(&opts, 0, sizeof(opts));\n \tif (index_only)\n \t\topts.index_only = 1;\n@@ -236,39 +179,37 @@ static int git_merge_trees(int index_only,\n \n \trc = unpack_trees(trees, &opts);\n \tcache_tree_free(&active_cache_tree);\n-\n-\tcache_dirty = 1;\n-\n \treturn rc;\n }\n \n+static int unmerged_index(void)\n+{\n+\tint i;\n+\tfor (i = 0; i < active_nr; i++) {\n+\t\tstruct cache_entry *ce = active_cache[i];\n+\t\tif (ce_stage(ce))\n+\t\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n static struct tree *git_write_tree(void)\n {\n \tstruct tree *result = NULL;\n \n-\tif (cache_dirty) {\n-\t\tunsigned i;\n-\t\tfor (i = 0; i < active_nr; i++) {\n-\t\t\tstruct cache_entry *ce = active_cache[i];\n-\t\t\tif (ce_stage(ce))\n-\t\t\t\treturn NULL;\n-\t\t}\n-\t} else\n-\t\tread_cache_from(current_index_file);\n+\tif (unmerged_index())\n+\t\treturn NULL;\n \n \tif (!active_cache_tree)\n \t\tactive_cache_tree = cache_tree();\n \n \tif (!cache_tree_fully_valid(active_cache_tree) &&\n-\t\t\tcache_tree_update(active_cache_tree,\n-\t\t\t\tactive_cache, active_nr, 0, 0) < 0)\n+\t cache_tree_update(active_cache_tree,\n+\t\t\t active_cache, active_nr, 0, 0) < 0)\n \t\tdie(\"error building trees\");\n \n \tresult = lookup_tree(active_cache_tree->sha1);\n \n-\tflush_cache();\n-\tcache_dirty = 0;\n-\n \treturn result;\n }\n \n@@ -331,10 +272,7 @@ static struct path_list *get_unmerged(void)\n \tint i;\n \n \tunmerged->strdup_paths = 1;\n-\tif (!cache_dirty) {\n-\t\tread_cache_from(current_index_file);\n-\t\tcache_dirty++;\n-\t}\n+\n \tfor (i = 0; i < active_nr; i++) {\n \t\tstruct path_list_item *item;\n \t\tstruct stage_data *e;\n@@ -469,9 +407,6 @@ static int remove_file(int clean, const char *path, int no_wd)\n \tint update_working_directory = !index_only && !no_wd;\n \n \tif (update_cache) {\n-\t\tif (!cache_dirty)\n-\t\t\tread_cache_from(current_index_file);\n-\t\tcache_dirty++;\n \t\tif (remove_file_from_cache(path))\n \t\t\treturn -1;\n \t}\n@@ -1105,9 +1040,7 @@ static int merge_trees(struct tree *head,\n \t\t sha1_to_hex(head->object.sha1),\n \t\t sha1_to_hex(merge->object.sha1));\n \n-\t*result = git_write_tree();\n-\n-\tif (!*result) {\n+\tif (unmerged_index()) {\n \t\tstruct path_list *entries, *re_head, *re_merge;\n \t\tint i;\n \t\tpath_list_clear(¤t_file_set, 1);\n@@ -1128,17 +1061,11 @@ static int merge_trees(struct tree *head,\n \t\t\tif (!process_entry(path, e, branch1, branch2))\n \t\t\t\tclean = 0;\n \t\t}\n-\t\tif (cache_dirty)\n-\t\t\tflush_cache();\n \n \t\tpath_list_clear(re_merge, 0);\n \t\tpath_list_clear(re_head, 0);\n \t\tpath_list_clear(entries, 1);\n \n-\t\tif (clean || index_only)\n-\t\t\t*result = git_write_tree();\n-\t\telse\n-\t\t\t*result = NULL;\n \t} else {\n \t\tclean = 1;\n \t\tprintf(\"merging of trees %s and %s resulted in %s\\n\",\n@@ -1146,6 +1073,8 @@ static int merge_trees(struct tree *head,\n \t\t sha1_to_hex(merge->object.sha1),\n \t\t sha1_to_hex((*result)->object.sha1));\n \t}\n+\tif (index_only)\n+\t\t*result = git_write_tree();\n \n \treturn clean;\n }\n@@ -1170,10 +1099,10 @@ static int merge(struct commit *h1,\n \t\t const char *branch1,\n \t\t const char *branch2,\n \t\t int call_depth /* =0 */,\n-\t\t struct commit *ancestor /* =None */,\n+\t\t struct commit_list *ca,\n \t\t struct commit **result)\n {\n-\tstruct commit_list *ca = NULL, *iter;\n+\tstruct commit_list *iter;\n \tstruct commit *merged_common_ancestors;\n \tstruct tree *mrtree;\n \tint clean;\n@@ -1182,10 +1111,10 @@ static int merge(struct commit *h1,\n \toutput_commit_title(h1);\n \toutput_commit_title(h2);\n \n-\tif (ancestor)\n-\t\tcommit_list_insert(ancestor, &ca);\n-\telse\n-\t\tca = reverse_commit_list(get_merge_bases(h1, h2, 1));\n+\tif (!ca) {\n+\t\tca = get_merge_bases(h1, h2, 1);\n+\t\tca = reverse_commit_list(ca);\n+\t}\n \n \toutput(\"found %u common ancestor(s):\", commit_list_count(ca));\n \tfor (iter = ca; iter; iter = iter->next)\n@@ -1211,6 +1140,7 @@ static int merge(struct commit *h1,\n \t\t * merge_trees has always overwritten it: the commited\n \t\t * \"conflicts\" were already resolved.\n \t\t */\n+\t\tdiscard_cache();\n \t\tmerge(merged_common_ancestors, iter->item,\n \t\t \"Temporary merge branch 1\",\n \t\t \"Temporary merge branch 2\",\n@@ -1223,25 +1153,21 @@ static int merge(struct commit *h1,\n \t\t\tdie(\"merge returned no commit\");\n \t}\n \n+\tdiscard_cache();\n \tif (call_depth == 0) {\n-\t\tsetup_index(0 /* $GIT_DIR/index */);\n+\t\tread_cache();\n \t\tindex_only = 0;\n-\t} else {\n-\t\tsetup_index(1 /* temporary index */);\n-\t\tgit_read_tree(h1->tree);\n+\t} else\n \t\tindex_only = 1;\n-\t}\n \n \tclean = merge_trees(h1->tree, h2->tree, merged_common_ancestors->tree,\n \t\t\t branch1, branch2, &mrtree);\n \n-\tif (!ancestor && (clean || index_only)) {\n+\tif (index_only) {\n \t\t*result = make_virtual_commit(mrtree, \"merged tree\");\n \t\tcommit_list_insert(h1, &(*result)->parents);\n \t\tcommit_list_insert(h2, &(*result)->parents->next);\n-\t} else\n-\t\t*result = NULL;\n-\n+\t}\n \treturn clean;\n }\n \n@@ -1277,19 +1203,16 @@ static struct commit *get_ref(const char *ref)\n \n int main(int argc, char *argv[])\n {\n-\tstatic const char *bases[2];\n+\tstatic const char *bases[20];\n \tstatic unsigned bases_count = 0;\n \tint i, clean;\n \tconst char *branch1, *branch2;\n \tstruct commit *result, *h1, *h2;\n+\tstruct commit_list *ca = NULL;\n+\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n+\tint index_fd;\n \n \tgit_config(git_default_config); /* core.filemode */\n-\toriginal_index_file = getenv(INDEX_ENVIRONMENT);\n-\n-\tif (!original_index_file)\n-\t\toriginal_index_file = xstrdup(git_path(\"index\"));\n-\n-\ttemporary_index_file = xstrdup(git_path(\"mrg-rcrsv-tmp-idx\"));\n \n \tif (argc < 4)\n \t\tdie(\"Usage: %s ... -- ...\\n\", argv[0]);\n@@ -1313,18 +1236,18 @@ int main(int argc, char *argv[])\n \tbranch2 = better_branch_name(branch2);\n \tprintf(\"Merging %s with %s\\n\", branch1, branch2);\n \n-\tif (bases_count == 1) {\n-\t\tstruct commit *ancestor = get_ref(bases[0]);\n-\t\tclean = merge(h1, h2, branch1, branch2, 0, ancestor, &result);\n-\t} else\n-\t\tclean = merge(h1, h2, branch1, branch2, 0, NULL, &result);\n+\tindex_fd = hold_lock_file_for_update(lock, get_index_file(), 1);\n \n-\tif (cache_dirty)\n-\t\tflush_cache();\n+\tfor (i = 0; i < bases_count; i++) {\n+\t\tstruct commit *ancestor = get_ref(bases[i]);\n+\t\tca = commit_list_insert(ancestor, &ca);\n+\t}\n+\tclean = merge(h1, h2, branch1, branch2, 0, ca, &result);\n+\n+\tif (active_cache_changed &&\n+\t (write_cache(index_fd, active_cache, active_nr) ||\n+\t close(index_fd) || commit_lock_file(lock)))\n+\t\t\tdie (\"unable to write %s\", get_index_file());\n \n \treturn clean ? 0: 1;\n }\n-\n-/*\n-vim: sw=8 noet\n-*/\n-- \n1.4.4.4.gc3d6\n"},{"id":"31415","messageId":"7vy7oa8rbm.fsf@assigned-by-dhcp.cox.net","threadId":"6218","inReplyTo":"7vr6u2adgx.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-10T22:11:57Z","receivedAt":"2007-01-10T22:11:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> This comes on top of yours. \n>\n> I'm reproducing all the merges in linux-2.6 history to make sure\n> the base one, yours and this produce the same result (the same\n> clean merge, or the same unmerged index and the same diff from\n> HEAD). So far it is looking good.\n\nLooks like this is good to go -- among the 2700+ merges I've\nfinished about 1000 of them and the results from the three\nimplementation exactly match.\n"},{"id":"31418","messageId":"81b0412b0701101507n764aed73p31c7533e743283f0@mail.gmail.com","threadId":"6218","inReplyTo":"7vr6u2adgx.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-01-10T23:07:39Z","receivedAt":"2007-01-10T23:07:39Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 1/10/07, Junio C Hamano wrote:\n> This comes on top of yours.\n>\n> I'm reproducing all the merges in linux-2.6 history to make sure\n> the base one, yours and this produce the same result (the same\n> clean merge, or the same unmerged index and the same diff from\n> HEAD). So far it is looking good.\n\nYep. Tried the monster merge on it: 1m15sec on that small laptop.\n\nFor whatever reason your patch left an \"if (cache_dirty) flush_cache()\",\nthat's after my patch + yours. Had it removed.\n"},{"id":"31420","messageId":"Pine.LNX.4.64.0701101521410.3594@woody.osdl.org","threadId":"6218","inReplyTo":"81b0412b0701101507n764aed73p31c7533e743283f0@mail.gmail.com","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2007-01-10T23:23:18Z","receivedAt":"2007-01-10T23:23:18Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 11 Jan 2007, Alex Riesen wrote:\n> \n> Yep. Tried the monster merge on it: 1m15sec on that small laptop.\n\nIs that supposed to be good? That still sounds really slow to me. What \nkind of nasty project are you doing? Is this the 44k file project, and \nunder cygwin? Or is it that bad even under Linux?\n\n\t\t\tLinus\n"},{"id":"31424","messageId":"7v4pqy8kqk.fsf@assigned-by-dhcp.cox.net","threadId":"6218","inReplyTo":"81b0412b0701101507n764aed73p31c7533e743283f0@mail.gmail.com","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-11T00:34:11Z","receivedAt":"2007-01-11T00:34:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Riesen\" writes:\n\n> On 1/10/07, Junio C Hamano wrote:\n>> This comes on top of yours.\n>>\n>> I'm reproducing all the merges in linux-2.6 history to make sure\n>> the base one, yours and this produce the same result (the same\n>> clean merge, or the same unmerged index and the same diff from\n>> HEAD). So far it is looking good.\n>\n> Yep. Tried the monster merge on it: 1m15sec on that small laptop.\n\nIs that supposed to be a good news? It sounds awfully slow.\n\n> For whatever reason your patch left an \"if (cache_dirty) flush_cache()\",\n> that's after my patch + yours. Had it removed.\n\nThat's because my copy of \"your patch\" has the fix-up I\nsuggested to remove the flush from process_renames() already --\nthe removal of that one and removal from process_entry() you did\nlogically belong to each other.\n"},{"id":"31435","messageId":"Pine.LNX.4.63.0701110913140.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6218","inReplyTo":"Pine.LNX.4.64.0701101521410.3594@woody.osdl.org","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-01-11T08:14:24Z","receivedAt":"2007-01-11T08:14:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 10 Jan 2007, Linus Torvalds wrote:\n\n> On Thu, 11 Jan 2007, Alex Riesen wrote:\n> > \n> > Yep. Tried the monster merge on it: 1m15sec on that small laptop.\n> \n> Is that supposed to be good? That still sounds really slow to me. What \n> kind of nasty project are you doing? Is this the 44k file project, and \n> under cygwin? Or is it that bad even under Linux?\n\nThis _is_ cygwin. And 1m15sec is actually very, very good, if you happen \nto know that it took more than 10 minutes(!) when we started our quest of \ninbuilding recursive merge.\n\nCiao,\nDscho\n"},{"id":"31436","messageId":"Pine.LNX.4.63.0701110914300.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6218","inReplyTo":"7vr6u2adgx.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-01-11T08:15:59Z","receivedAt":"2007-01-11T08:15:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 10 Jan 2007, Junio C Hamano wrote:\n\n> Subject: [PATCH] merge-recursive: do not use on-file index when not needed.\n> \n> This revamps the merge-recursive implementation following the\n> outline in:\n> \n> \tMessage-ID: <7v8xgileza.fsf@assigned-by-dhcp.cox.net>\n\nThank you very much! I know, it was my task and I punted, but my time is \nreally scarce these days.\n\nCiao,\nDscho\n"},{"id":"31442","messageId":"81b0412b0701110102m5264696dg68a573e9d5f2a17c@mail.gmail.com","threadId":"6218","inReplyTo":"Pine.LNX.4.64.0701101521410.3594@woody.osdl.org","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-01-11T09:02:03Z","receivedAt":"2007-01-11T09:02:03Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 1/11/07, Linus Torvalds wrote:\n> >\n> > Yep. Tried the monster merge on it: 1m15sec on that small laptop.\n>\n> Is that supposed to be good? That still sounds really slow to me. What\n> kind of nasty project are you doing? Is this the 44k file project, and\n> under cygwin? Or is it that bad even under Linux?\n\nIt is that \"bad\" on a 384Mb linux laptop and 1.2GHz Celeron.\nYes, it is that 44k files project. The previous code finishes\nthat merge on that laptop in about 20 minutes, so it's defnitely\nan improvement. My cygwin machine has a lot more memory (2Gb),\nso I can't really compare them here.\n"},{"id":"31443","messageId":"81b0412b0701110103p5f67b955gee6ff6194e6ea68d@mail.gmail.com","threadId":"6218","inReplyTo":"Pine.LNX.4.63.0701110913140.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-01-11T09:03:48Z","receivedAt":"2007-01-11T09:03:48Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 1/11/07, Johannes Schindelin wrote:\n> > > Yep. Tried the monster merge on it: 1m15sec on that small laptop.\n> >\n> > Is that supposed to be good? That still sounds really slow to me. What\n> > kind of nasty project are you doing? Is this the 44k file project, and\n> > under cygwin? Or is it that bad even under Linux?\n>\n> This _is_ cygwin. And 1m15sec is actually very, very good, if you happen\n\nNo, this is linux, in a very constrained conditions. On cygwin I\nhaven't tried it yet.\n\n> to know that it took more than 10 minutes(!) when we started our quest of\n> inbuilding recursive merge.\n\nRight.\n"},{"id":"31465","messageId":"81b0412b0701110411s7ccb45c1t9ececa9301760bae@mail.gmail.com","threadId":"6218","inReplyTo":"81b0412b0701110103p5f67b955gee6ff6194e6ea68d@mail.gmail.com","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-01-11T12:11:28Z","receivedAt":"2007-01-11T12:11:28Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 1/11/07, Alex Riesen wrote:\n> > > > Yep. Tried the monster merge on it: 1m15sec on that small laptop.\n> > >\n> > > Is that supposed to be good? That still sounds really slow to me. What\n> > > kind of nasty project are you doing? Is this the 44k file project, and\n> > > under cygwin? Or is it that bad even under Linux?\n> >\n> > This _is_ cygwin. And 1m15sec is actually very, very good, if you happen\n>\n> No, this is linux, in a very constrained conditions. On cygwin I\n> haven't tried it yet.\n\nWell, tried it now. Strangely enough - there is almost no speedup with\nJunio's patch on top of mine. I must have done something stupid,\nbut cannot find what. And the times reported by time on cygwin jump\nwildly: from 29 to 38 sec!\n"},{"id":"31479","messageId":"Pine.LNX.4.64.0701110823300.3594@woody.osdl.org","threadId":"6218","inReplyTo":"81b0412b0701110102m5264696dg68a573e9d5f2a17c@mail.gmail.com","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2007-01-11T16:38:51Z","receivedAt":"2007-01-11T16:38:51Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 11 Jan 2007, Alex Riesen wrote:\n> On 1/11/07, Linus Torvalds wrote:\n> > >\n> > > Yep. Tried the monster merge on it: 1m15sec on that small laptop.\n> > \n> > Is that supposed to be good? That still sounds really slow to me. What\n> > kind of nasty project are you doing? Is this the 44k file project, and\n> > under cygwin? Or is it that bad even under Linux?\n> \n> It is that \"bad\" on a 384Mb linux laptop and 1.2GHz Celeron.\n> Yes, it is that 44k files project. The previous code finishes\n> that merge on that laptop in about 20 minutes, so it's defnitely\n> an improvement. My cygwin machine has a lot more memory (2Gb),\n> so I can't really compare them here.\n\nOk. Junio, I'd suggest putting it into 1.5.0, then - it's a fairly simple \nthing, after all, and if it's the difference between 20 minutes and just \nover one minute, it clearly matters.\n\nWith 384MB of memory, and 44 thousand files, I bet the problem is just \nthat the working set doesn't fit entirely in RAM. It probably caches \n*most* of it, but with inodes and directories being spread out on disk \n(and I assume there are more files in the actual working tree), so writing \nout a 6MB index file (or whatever) and then reading it back several times \njust ends up generating IO simply because 6MB is actually a noticeable \nchunk of memory in that situation.\n\n(It also generates a ton of tree objects early, so the effect at run-time \nis probably much more than 6MB).\n\nThat said, I think we actually have another problem entirely:\n\nLook at \"write_cache()\", Junio: isn't it leaking memory like mad?\n\nShouldn't we have something like this?\n\nIt's entirely possible that the _real_ problem with the \"flush the index \nall the time\" was that it just caused this bug: tons and tons of lost \nmemory, causing git-merge-recursive to grow explosively (~6MB per \ncache flush, and a _lot_ of cache flushes), which on a 384MB machine \nquickly uses up memory and causes totally unnecessary swapping.\n\nOf course, it's also entirely possible that I'm a complete retard, and \njust didn't see where the data buffer is still used or freed.\n\n\"Linus - complete retard or hero in shining armor? You decide!\"\n\n\t\tLinus\n\n---\ndiff --git a/read-cache.c b/read-cache.c\nindex 8ecd826..c54a611 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1010,7 +1010,7 @@ int write_cache(int newfd, struct cache_entry **cache, int entries)\n \t\tif (data &&\n \t\t !write_index_ext_header(&c, newfd, CACHE_EXT_TREE, sz) &&\n \t\t !ce_write(&c, newfd, data, sz))\n-\t\t\t;\n+\t\t\tfree(data);\n \t\telse {\n \t\t\tfree(data);\n \t\t\treturn -1;\n"},{"id":"31481","messageId":"81b0412b0701110943s274bfbcbkfea0fcb294ccb820@mail.gmail.com","threadId":"6218","inReplyTo":"Pine.LNX.4.64.0701110823300.3594@woody.osdl.org","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-01-11T17:43:55Z","receivedAt":"2007-01-11T17:43:55Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 1/11/07, Linus Torvalds wrote:\n> That said, I think we actually have another problem entirely:\n>\n> Look at \"write_cache()\", Junio: isn't it leaking memory like mad?\n\nUnless it is used in some corner case not covered by tests - it\nlooks like it does leak memory like mad. With the patch the\nmemory usage for 44k-merge is more than halved!\n"},{"id":"31482","messageId":"Pine.LNX.4.64.0701111001150.3594@woody.osdl.org","threadId":"6218","inReplyTo":"81b0412b0701110943s274bfbcbkfea0fcb294ccb820@mail.gmail.com","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2007-01-11T18:02:52Z","receivedAt":"2007-01-11T18:02:52Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 11 Jan 2007, Alex Riesen wrote:\n\n> On 1/11/07, Linus Torvalds wrote:\n> > That said, I think we actually have another problem entirely:\n> > \n> > Look at \"write_cache()\", Junio: isn't it leaking memory like mad?\n> \n> Unless it is used in some corner case not covered by tests - it\n> looks like it does leak memory like mad. With the patch the\n> memory usage for 44k-merge is more than halved!\n\nIs that halving on _top_ of your and Junio's fixes to not flush \nunnecessarily?\n\nJunio, I looked and looked, and that trivial one-liner definitely looks \nright to me. The pointer is not free'd by anybody else, and none of the \nthings we call in to with it save it away, and they expect the caller to \nmanage it.\n\nAnd it does pass all the tests, although I don't know how much coverage \nthey have in this area..\n\n\t\tLinus\n"},{"id":"31489","messageId":"7vfyah48j2.fsf@assigned-by-dhcp.cox.net","threadId":"6218","inReplyTo":"Pine.LNX.4.64.0701110823300.3594@woody.osdl.org","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-11T20:23:45Z","receivedAt":"2007-01-11T20:23:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> That said, I think we actually have another problem entirely:\n>\n> Look at \"write_cache()\", Junio: isn't it leaking memory like mad?\n>\n> Shouldn't we have something like this?\n>\n> It's entirely possible that the _real_ problem with the \"flush the index \n> all the time\" was that it just caused this bug: tons and tons of lost \n> memory, causing git-merge-recursive to grow explosively (~6MB per \n> cache flush, and a _lot_ of cache flushes), which on a 384MB machine \n> quickly uses up memory and causes totally unnecessary swapping.\n\nYou are right -- there is absolutely no reason to retain this\nmemory. It is a serialized representation of cache-tree data\nonly to be stored in the index, and no other user of this data\nexists. Thanks for spotting this.\n\nWriting out 6MB per every path changed in a merge would still be\nan unnecessary overhead over the one in 'next', so there is no\nreason to replace 'next' with this single liner of yours, but I\nam interested in seeing how much of the 20-minute vs 1-minute\ndifference is attributable to this leak, just out of curiosity.\n\nAlex, if you have a chance, could you apply Linus's single-liner\non top of 'master', without either of the merge-recursive\npatches in 'next', and see what kind of numbers you would get?\n"},{"id":"31490","messageId":"7v8xg947vz.fsf@assigned-by-dhcp.cox.net","threadId":"6218","inReplyTo":"81b0412b0701110411s7ccb45c1t9ececa9301760bae@mail.gmail.com","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-11T20:37:36Z","receivedAt":"2007-01-11T20:37:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Riesen\" writes:\n\n> Well, tried it now. Strangely enough - there is almost no speedup with\n> Junio's patch on top of mine.\n\nMine is primarily meant to be a conceptual clean-up. While it\ndoes save unnecessary write-tree at the end, and it also saves\nunnecessary write-cache/read-cache for the recursive part when\nyou have more than one merge base, I would not be surprised if\nthe effect of tons of unnecessary write-index, once per path\ninvolved in the merge, which was what your patch removed, would\ndrawf anything else.\n\nSince single merge base cases dominate in practice, you might\nsee improvements from the avoidance of the final write-tree but\nprobably would not see much benefit of write-cache/read-cache\navoidance unless your merge has many merge bases.\n"},{"id":"31498","messageId":"20070111214826.GC6058@steel.home","threadId":"6218","inReplyTo":"Pine.LNX.4.64.0701111001150.3594@woody.osdl.org","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Alex Riesen","fromEmail":"fork0@t-online.de","sentAt":"2007-01-11T21:48:26Z","receivedAt":"2007-01-11T21:48:26Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Linus Torvalds, Thu, Jan 11, 2007 19:02:52 +0100:\n> > > That said, I think we actually have another problem entirely:\n> > > \n> > > Look at \"write_cache()\", Junio: isn't it leaking memory like mad?\n> > \n> > Unless it is used in some corner case not covered by tests - it\n> > looks like it does leak memory like mad. With the patch the\n> > memory usage for 44k-merge is more than halved!\n> \n> Is that halving on _top_ of your and Junio's fixes to not flush \n> unnecessarily?\n\nYes.\n\n> And it does pass all the tests, although I don't know how much coverage \n> they have in this area..\n\nQuite a bit: criss-cross, renames, simple. My 44k-merge is just a\nprimitive two-head merge useful only as a stress test (it doesn't even\nhas any renames).\n"},{"id":"31503","messageId":"20070111221053.GD6058@steel.home","threadId":"6218","inReplyTo":"7vfyah48j2.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Alex Riesen","fromEmail":"fork0@t-online.de","sentAt":"2007-01-11T22:10:53Z","receivedAt":"2007-01-11T22:10:53Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Thu, Jan 11, 2007 21:23:45 +0100:\n> > That said, I think we actually have another problem entirely:\n> >\n> > Look at \"write_cache()\", Junio: isn't it leaking memory like mad?\n> >\n> > Shouldn't we have something like this?\n> >\n> > It's entirely possible that the _real_ problem with the \"flush the index \n> > all the time\" was that it just caused this bug: tons and tons of lost \n> > memory, causing git-merge-recursive to grow explosively (~6MB per \n> > cache flush, and a _lot_ of cache flushes), which on a 384MB machine \n> > quickly uses up memory and causes totally unnecessary swapping.\n> \n> You are right -- there is absolutely no reason to retain this\n> memory. It is a serialized representation of cache-tree data\n> only to be stored in the index, and no other user of this data\n> exists. Thanks for spotting this.\n> \n> Writing out 6MB per every path changed in a merge would still be\n> an unnecessary overhead over the one in 'next', so there is no\n> reason to replace 'next' with this single liner of yours, but I\n> am interested in seeing how much of the 20-minute vs 1-minute\n> difference is attributable to this leak, just out of curiosity.\n> \n> Alex, if you have a chance, could you apply Linus's single-liner\n> on top of 'master', without either of the merge-recursive\n> patches in 'next', and see what kind of numbers you would get?\n\nWith regard to speed: not noticable on the cygwin machine. The\n384Mb-laptop liked it: moved into 40-50 sec range (it had real\nproblems (minutes) doing that merge without at least my first patch.\nBecause of the leak, as we now understand).\n\nIt must have been large leak, as I really have seen the memory usage\ndropping down significantly.\n"},{"id":"31509","messageId":"Pine.LNX.4.64.0701111424400.3594@woody.osdl.org","threadId":"6218","inReplyTo":"20070111221053.GD6058@steel.home","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2007-01-11T22:28:21Z","receivedAt":"2007-01-11T22:28:21Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 11 Jan 2007, Alex Riesen wrote:\n> \n> It must have been large leak, as I really have seen the memory usage\n> dropping down significantly.\n\nI really think it was about 6MB (or whatever your index file size was) per \nevery single resolved file. I think merge-recursive used to flush the \nindex file every time it resolved something, and every flush would \nbasically leak the whole buffer used to write the index.\n\nAnyway, 40-50 sec on a fairly weak laptop for a 44k-file merge sounds like \ngit doesn't have to be totally embarrassed. I'm not saying we shouldn't be \nable to do it faster, but it's at least _possible_ that a lot of the time \nspent is now spent doing real work (ie maybe you actually have a fair \namount of file-level merging? Maybe it's 40-50 sec because there's some \namount of real IO going on, and a fair amount of real work done too?)\n\n\t\t\tLinus\n"},{"id":"31517","messageId":"7vk5zt15ot.fsf@assigned-by-dhcp.cox.net","threadId":"6218","inReplyTo":"Pine.LNX.4.64.0701111424400.3594@woody.osdl.org","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-11T23:53:22Z","receivedAt":"2007-01-11T23:53:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Thu, 11 Jan 2007, Alex Riesen wrote:\n>> \n>> It must have been large leak, as I really have seen the memory usage\n>> dropping down significantly.\n>\n> I really think it was about 6MB (or whatever your index file size was) per \n> every single resolved file. I think merge-recursive used to flush the \n> index file every time it resolved something, and every flush would \n> basically leak the whole buffer used to write the index.\n\nThis does not change the conclusion \"leaking is bad\", but it was\nnot as bad as \"the whole buffer used to write the index\".\n\nThere are 10 4-byte ints, 20-bye SHA1, and a short plus pathname\nand padding stored per a file and Alex has 44k files. They are\nnot included in that *data the code was leaking, I would suspect\nmaybe a meg or so per path.\n\nThe version of merge-recursive in 'next' would not write out the\nindex at all until the very end once, so that makes this leak\nsomewhat irrelevant for that particular program ;-) but thanks\nfor the fix.\n\nHere is the list of what I have queued for 'master' (I am\nsending the list because it will be some time before I can push\nthem out):\n\nEric Wong (1):\n Avoid errors and warnings when attempting to do I/O on zero bytes\n\nJunio C Hamano (5):\n Document git-init\n index-pack: write-or-die instead of unchecked write-in-full.\n config-set: check write-in-full returns in set_multivar\n git-rm: do not fail on already removed file.\n git-status: wording update to deal with deleted files.\n\nLinus Torvalds (3):\n write-cache: do not leak the serialized cache-tree data.\n write_in_full: really write in full or return error on disk full.\n Better error messages for corrupt databases\n"},{"id":"31524","messageId":"20070112001827.GE6058@steel.home","threadId":"6218","inReplyTo":"Pine.LNX.4.64.0701111424400.3594@woody.osdl.org","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Alex Riesen","fromEmail":"fork0@t-online.de","sentAt":"2007-01-12T00:18:27Z","receivedAt":"2007-01-12T00:18:27Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Linus Torvalds, Thu, Jan 11, 2007 23:28:21 +0100:\n> > \n> > It must have been large leak, as I really have seen the memory usage\n> > dropping down significantly.\n> \n> I really think it was about 6MB (or whatever your index file size was) per \n> every single resolved file. I think merge-recursive used to flush the \n> index file every time it resolved something, and every flush would \n> basically leak the whole buffer used to write the index.\n\nLooks like. Resulting merge has about 10 files changed, all the other\nfiles are same, just got different ways on the branches to be merged.\n\n> Anyway, 40-50 sec on a fairly weak laptop for a 44k-file merge sounds like \n> git doesn't have to be totally embarrassed. I'm not saying we shouldn't be \n> able to do it faster, but it's at least _possible_ that a lot of the time \n> spent is now spent doing real work (ie maybe you actually have a fair \n> amount of file-level merging? Maybe it's 40-50 sec because there's some \n> amount of real IO going on, and a fair amount of real work done too?)\n\nSome work, definitely: git diff-tree branch1 branch2 lists 9042\ndifferences, among them some on the same files.\n"},{"id":"31563","messageId":"20070112184839.9431ddff.vsu@altlinux.ru","threadId":"6218","inReplyTo":"7vr6u2adgx.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Sergey Vlasov","fromEmail":"vsu@altlinux.ru","sentAt":"2007-01-12T15:48:39Z","receivedAt":"2007-01-12T15:48:39Z","isPatch":true,"sender":{"key":"vsu@altlinux.ru","avatar":"https://avatars.githubusercontent.com/u/616082?v=4"},"body":"On Wed, 10 Jan 2007 11:28:14 -0800 Junio C Hamano wrote:\n\n> From: Junio C Hamano \n> Date: Wed, 10 Jan 2007 11:20:58 -0800\n> Subject: [PATCH] merge-recursive: do not use on-file index when not needed.\n>\n> This revamps the merge-recursive implementation following the\n> outline in:\n>\n> \tMessage-ID: <7v8xgileza.fsf@assigned-by-dhcp.cox.net>\n>\n> There is no need to write out the index until the very end just\n> once from merge-recursive. Also there is no need to write out\n> the resulting tree object for the simple case of merging with a\n> single merge base.\n>\n> Signed-off-by: Junio C Hamano \n\nThis commit broke t3401-rebase-partial.sh:\n\n...\n* ok 3: rebase topic branch against new master and check git-am did not get halted\n\n* expecting success: git-checkout -f my-topic-branch-merge &&\n git-rebase --merge master-merge &&\n test ! -d .git/.dotest-merge\nFirst, rewinding head to replay your work on top of it...\nHEAD is now at 5f97179... Add C.\nMerging master-merge with my-topic-branch-merge~1\nMerging:\n5f97179 Add C.\n1be2c8e Add B.\nfound 1 common ancestor(s):\n0e8cba9 Add A.\n.../git-rebase: line 82: 11517 Segmentation fault git-merge-$strategy \"$cmt^\" -- \"$hd\" \"$cmt\"\nUnknown exit code (139) from command: git-merge-recursive 1be2c8e0eba8a7a383d0403facb1c72c622c0939^ -- HEAD 1be2c8e0eba8a7a383d0403facb1c72c622c0939\n* FAIL 4: rebase --merge topic branch that was partially merged upstream\n git-checkout -f my-topic-branch-merge &&\n git-rebase --merge master-merge &&\n test ! -d .git/.dotest-merge\n\n* failed 1 among 4 test(s)\n\n> @@ -1105,9 +1040,7 @@ static int merge_trees(struct tree *head,\n> \t\t sha1_to_hex(head->object.sha1),\n> \t\t sha1_to_hex(merge->object.sha1));\n>\n> -\t*result = git_write_tree();\n\nPreviously *result was set here...\n\n> -\n> -\tif (!*result) {\n> +\tif (unmerged_index()) {\n> \t\tstruct path_list *entries, *re_head, *re_merge;\n> \t\tint i;\n> \t\tpath_list_clear(¤t_file_set, 1);\n> @@ -1128,17 +1061,11 @@ static int merge_trees(struct tree *head,\n> \t\t\tif (!process_entry(path, e, branch1, branch2))\n> \t\t\t\tclean = 0;\n> \t\t}\n> -\t\tif (cache_dirty)\n> -\t\t\tflush_cache();\n>\n> \t\tpath_list_clear(re_merge, 0);\n> \t\tpath_list_clear(re_head, 0);\n> \t\tpath_list_clear(entries, 1);\n>\n> -\t\tif (clean || index_only)\n> -\t\t\t*result = git_write_tree();\n> -\t\telse\n> -\t\t\t*result = NULL;\n> \t} else {\n> \t\tclean = 1;\n> \t\tprintf(\"merging of trees %s and %s resulted in %s\\n\",\n> @@ -1146,6 +1073,8 @@ static int merge_trees(struct tree *head,\n> \t\t sha1_to_hex(merge->object.sha1),\n> \t\t sha1_to_hex((*result)->object.sha1));\n\n...and it is still used here - however, after the patch *result is\nuninitialized at this point.\n\n> \t}\n> +\tif (index_only)\n> +\t\t*result = git_write_tree();\n\nToo late...\n\n>\n> \treturn clean;\n> }\n\n\n"},{"id":"31565","messageId":"81b0412b0701120938o1606dcachf2553a83b47921b1@mail.gmail.com","threadId":"6218","inReplyTo":"20070112184839.9431ddff.vsu@altlinux.ru","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-01-12T17:38:26Z","receivedAt":"2007-01-12T17:38:26Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 1/12/07, Sergey Vlasov wrote:\n> > Subject: [PATCH] merge-recursive: do not use on-file index when not needed.\n>\n> This commit broke t3401-rebase-partial.sh:\n>\n> ...\n> * ok 3: rebase topic branch against new master and check git-am did not get halted\n>\n\nHmm... Can't reproduce. Do you have your own patches in the tree?\nOr could you post your merge-recursive.c?\n"},{"id":"31570","messageId":"7vr6u0t87q.fsf@assigned-by-dhcp.cox.net","threadId":"6218","inReplyTo":"20070112184839.9431ddff.vsu@altlinux.ru","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-12T18:23:37Z","receivedAt":"2007-01-12T18:23:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Vlasov writes:\n\n> On Wed, 10 Jan 2007 11:28:14 -0800 Junio C Hamano wrote:\n>\n>> This revamps the merge-recursive implementation following the\n>> outline in:\n>> ...\n> This commit broke t3401-rebase-partial.sh:\n> ...\n> ...and it is still used here - however, after the patch *result is\n> uninitialized at this point.\n\nVery true. This untested patch should fix it.\n\nNote that this stops (relative to the older\nversion of merge-recursive that always wrote a tree even when it\nwas not needed) reporting the tree object name for outermost\nmerge, but I think that reporting was primarily meant for people\nwho are debugging merge-recursive and did not have a real\nvalue. We could even remove the whole printf(), which I tend to\nprefer.\n\n--\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 5237021..40c12aa 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1066,15 +1066,17 @@ static int merge_trees(struct tree *head,\n \t\tpath_list_clear(re_head, 0);\n \t\tpath_list_clear(entries, 1);\n \n-\t} else {\n+\t}\n+\telse\n \t\tclean = 1;\n+\n+\tif (index_only) {\n+\t\t*result = git_write_tree();\n \t\tprintf(\"merging of trees %s and %s resulted in %s\\n\",\n \t\t sha1_to_hex(head->object.sha1),\n \t\t sha1_to_hex(merge->object.sha1),\n \t\t sha1_to_hex((*result)->object.sha1));\n \t}\n-\tif (index_only)\n-\t\t*result = git_write_tree();\n \n \treturn clean;\n }\n"},{"id":"31581","messageId":"7v8xg8t3aj.fsf_-_@assigned-by-dhcp.cox.net","threadId":"6218","inReplyTo":"7vr6u0t87q.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] merge-recursive: do not report the resulting tree object name","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-12T20:09:56Z","receivedAt":"2007-01-12T20:09:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"It is not available in the outermost merge, and it is only\nuseful for debugging merge-recursive in the inner merges.\n\nSergey Vlasov noticed that the old code accesses an\nuninitialized location.\n\nSigned-off-by: Junio C Hamano \n---\n\n Junio C Hamano writes:\n\n > Very true. This untested patch should fix it.\n >\n > Note that this stops (relative to the older\n > version of merge-recursive that always wrote a tree even when it\n > was not needed) reporting the tree object name for outermost\n > merge, but I think that reporting was primarily meant for people\n > who are debugging merge-recursive and did not have a real\n > value. We could even remove the whole printf(), which I tend to\n > prefer.\n\n So I'd commit this -- I tested it this time, with\n\n \tif (result) *result = NULL\n\n at the beginning of that function.\n\n merge-recursive.c | 9 +++------\n 1 files changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 5237021..b4acbb7 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1066,13 +1066,10 @@ static int merge_trees(struct tree *head,\n \t\tpath_list_clear(re_head, 0);\n \t\tpath_list_clear(entries, 1);\n \n-\t} else {\n-\t\tclean = 1;\n-\t\tprintf(\"merging of trees %s and %s resulted in %s\\n\",\n-\t\t sha1_to_hex(head->object.sha1),\n-\t\t sha1_to_hex(merge->object.sha1),\n-\t\t sha1_to_hex((*result)->object.sha1));\n \t}\n+\telse\n+\t\tclean = 1;\n+\n \tif (index_only)\n \t\t*result = git_write_tree();\n \n-- \n1.5.0.rc1.g397d\n"},{"id":"31586","messageId":"20070112203042.GA8127@steel.home","threadId":"6218","inReplyTo":"7vr6u0t87q.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Alex Riesen","fromEmail":"fork0@t-online.de","sentAt":"2007-01-12T20:30:42Z","receivedAt":"2007-01-12T20:30:42Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Fri, Jan 12, 2007 19:23:37 +0100:\n> > ...and it is still used here - however, after the patch *result is\n> > uninitialized at this point.\n> \n> Very true. This untested patch should fix it.\n> \n\nI had to initialize mrtree of merge() with NULL to reproduce it.\nSneaky bastard...\n\n> We could even remove the whole printf(), which I tend to prefer.\n\nI agree. The merges of this kind a rare in comparison to simple ones.\n"},{"id":"31592","messageId":"20070112203721.GA4562@procyon.home","threadId":"6218","inReplyTo":"81b0412b0701120938o1606dcachf2553a83b47921b1@mail.gmail.com","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Sergey Vlasov","fromEmail":"vsu@altlinux.ru","sentAt":"2007-01-12T20:37:21Z","receivedAt":"2007-01-12T20:37:21Z","isPatch":true,"sender":{"key":"vsu@altlinux.ru","avatar":"https://avatars.githubusercontent.com/u/616082?v=4"},"body":"On Fri, Jan 12, 2007 at 06:38:26PM +0100, Alex Riesen wrote:\n> On 1/12/07, Sergey Vlasov wrote:\n> >> Subject: [PATCH] merge-recursive: do not use on-file index when not \n> >needed.\n> >\n> >This commit broke t3401-rebase-partial.sh:\n> >\n> >...\n> >* ok 3: rebase topic branch against new master and check git-am did not \n> >get halted\n> >\n> \n> Hmm... Can't reproduce. Do you have your own patches in the tree?\n\nI had when I encountered the problem, but then retested with clean\n'master' (4494c656e2e29c468c48c9c2b20595342056e9dc) and got the same\ncrash. But since the problem is an uninitialized stack variable, you\ncan get anything depending on the phase of the moon from it (however,\nValgrind should still be able to catch it).\n"},{"id":"31593","messageId":"20070112210705.GB4562@procyon.home","threadId":"6218","inReplyTo":"7vr6u0t87q.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Speedup recursive by flushing index only once for all entries","fromName":"Sergey Vlasov","fromEmail":"vsu@altlinux.ru","sentAt":"2007-01-12T21:07:05Z","receivedAt":"2007-01-12T21:07:05Z","isPatch":true,"sender":{"key":"vsu@altlinux.ru","avatar":"https://avatars.githubusercontent.com/u/616082?v=4"},"body":"On Fri, Jan 12, 2007 at 10:23:37AM -0800, Junio C Hamano wrote:\n> Sergey Vlasov writes:\n> \n> > On Wed, 10 Jan 2007 11:28:14 -0800 Junio C Hamano wrote:\n> >\n> >> This revamps the merge-recursive implementation following the\n> >> outline in:\n> >> ...\n> > This commit broke t3401-rebase-partial.sh:\n> > ...\n> > ...and it is still used here - however, after the patch *result is\n> > uninitialized at this point.\n> \n> Very true. This untested patch should fix it.\n\nBTW, the same code does not crash on another (x86_64) machine;\nhowever, valgrind-3.2.1 complains:\n\n==20571== Use of uninitialised value of size 8\n==20571== at 0x411FF2: sha1_to_hex (sha1_file.c:125)\n==20571== by 0x405D90: merge_trees (merge-recursive.c:1071)\n==20571== by 0x406044: merge (merge-recursive.c:1163)\n==20571== by 0x40641D: main (merge-recursive.c:1245)\n\nAfter the patch valgrind does not complain anymore.\n\n> Note that this stops (relative to the older\n> version of merge-recursive that always wrote a tree even when it\n> was not needed) reporting the tree object name for outermost\n> merge, but I think that reporting was primarily meant for people\n> who are debugging merge-recursive and did not have a real\n> value. We could even remove the whole printf(), which I tend to\n> prefer.\n\nIf that printf() is just a debug output, we should definitely remove\nit - the merge output is verbose enough already.\n\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 5237021..40c12aa 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -1066,15 +1066,17 @@ static int merge_trees(struct tree *head,\n> \t\tpath_list_clear(re_head, 0);\n> \t\tpath_list_clear(entries, 1);\n> \n> -\t} else {\n> +\t}\n> +\telse\n> \t\tclean = 1;\n> +\n> +\tif (index_only) {\n> +\t\t*result = git_write_tree();\n\nHmm, can git_write_tree() return NULL at this point? Does the code in\nthe if (unmerged_index()) {...} branch above resolve all unmerged\nindex entries? It probably should, if I understand the\nmerge-recursive logic...\n\n> \t\tprintf(\"merging of trees %s and %s resulted in %s\\n\",\n> \t\t sha1_to_hex(head->object.sha1),\n> \t\t sha1_to_hex(merge->object.sha1),\n> \t\t sha1_to_hex((*result)->object.sha1));\n> \t}\n> -\tif (index_only)\n> -\t\t*result = git_write_tree();\n> \n> \treturn clean;\n> }\n"},{"id":"31612","messageId":"Pine.LNX.4.63.0701130034000.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6218","inReplyTo":"7v8xg8t3aj.fsf_-_@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] merge-recursive: do not report the resulting tree object name","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-01-12T23:36:09Z","receivedAt":"2007-01-12T23:36:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nI like this patch. merge-recursive is very talkative, to the intimidating \nastonishment of unsuspecting users.\n\nThe real information is in the conflict markers now, which tell the user \nwhere what hunk came from. And the names in the markers are _really_ \nuseful now.\n\nCiao,\nDscho\n"},{"id":"31614","messageId":"7vbql3pxz8.fsf@assigned-by-dhcp.cox.net","threadId":"6218","inReplyTo":"Pine.LNX.4.63.0701130034000.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] merge-recursive: do not report the resulting tree object name","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-13T00:32:59Z","receivedAt":"2007-01-13T00:32:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I like this patch. merge-recursive is very talkative, to the intimidating \n> astonishment of unsuspecting users.\n\nThis is a smallish example:\n\n $ git merge jc/merge-base\n 1\tTrying really trivial in-index merge...\n 2\tfatal: Merge requires file-level merging\n 3\tNope.\n 4\tMerging HEAD with jc/merge-base\n 5\tMerging:\n 6\tb60daf0 Make git-prune-packed a bit more chatty.\n 7\t5b75a55 Teach \"git-merge-base --check-ancestry\" about refs.\n 8\tfound 1 common ancestor(s):\n 9\t1c23d79 Don't die in git-http-fetch when fetching packs.\n 10\tAuto-merging Makefile\n 11\tAuto-merging builtin-branch.c\n 12\tAuto-merging builtin-reflog.c\n 13\tCONFLICT (content): Merge conflict in builtin-reflog.c\n 14\tAuto-merging builtin.h\n 15\tAuto-merging git.c\n 16\tRemoving merge-base.c\n 17\tResolved 'builtin-reflog.c' using previous resolution.\n 18\tAutomatic merge failed; fix conflicts and then commit the result.\n\nAmong these, I think lines 2..3 are somewhat confusing but I am\nused to seeing them and do not mind them too much.\n\nLines 4..9 do not have any real information that helps the end\nuser (even though it would be a very good debugging aid for\nmerge-recursive developers).\n\nLines 10..16 are useful, but I think we probably should show\nthem only for outermost merges.\n\nAn multi-base example:\n\n $ git merge 82560983997c961d9deafe0074b787c8484c2e1d\n 1\tMerging HEAD with 82560983997c961d9deafe0074b787c8484c2e1d\n 2\tMerging:\n 3\t9ee93dc Merge for-each-ref to sync gitweb fully with 'next'...\n 4\t8256098 gitweb: Print commit message without title in commi...\n 5\tfound 2 common ancestor(s):\n 6\tb2d3476 Gitweb - provide site headers and footers\n 7\t1259404 Merge branch 'maint'\n 8\t Merging:\n 9\t b2d3476 Gitweb - provide site headers and footers\n 10\t 1259404 Merge branch 'maint'\n 11\t found 1 common ancestor(s):\n 12\t 128eead gitweb: document webserver configuration for comm...\n 13\t Auto-merging Makefile\n 14\t Auto-merging gitweb/gitweb.perl\n 15\t CONFLICT (content): Merge conflict in gitweb/gitweb.perl\n 16\tAuto-merging gitweb/gitweb.perl\n 17\tMerge made by recursive.\n 18\t gitweb/gitweb.css | 2 +\n 19\t gitweb/gitweb.perl | 165 ++++++++++++++++++++++++++++++++...\n 20\t 2 files changed, 117 insertions(+), 50 deletions(-)\n\nI do not think we need to show 1..15 at all, perhaps without\n\"export GIT_MERGE_BASE_DEBUG=YesPlease\".\n"},{"id":"31617","messageId":"eo9apg$sqo$1@sea.gmane.org","threadId":"6218","inReplyTo":"7vbql3pxz8.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] merge-recursive: do not report the resulting tree object name","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-01-13T00:57:43Z","receivedAt":"2007-01-13T00:57:43Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n\n> I do not think we need to show 1..15 at all, perhaps without\n> \"export GIT_MERGE_BASE_DEBUG=YesPlease\".\n\nOr a -v/--verbose (or even -v -v -v) flag set.\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"31621","messageId":"20070113051447.GA22063@spearce.org","threadId":"6218","inReplyTo":"7vbql3pxz8.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] merge-recursive: do not report the resulting tree object name","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-01-13T05:14:47Z","receivedAt":"2007-01-13T05:14:47Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> $ git merge jc/merge-base\n> 1\tTrying really trivial in-index merge...\n> 2\tfatal: Merge requires file-level merging\n> 3\tNope.\n> 4\tMerging HEAD with jc/merge-base\n> 5\tMerging:\n> 6\tb60daf0 Make git-prune-packed a bit more chatty.\n> 7\t5b75a55 Teach \"git-merge-base --check-ancestry\" about refs.\n> 8\tfound 1 common ancestor(s):\n> 9\t1c23d79 Don't die in git-http-fetch when fetching packs.\n> 10\tAuto-merging Makefile\n> 11\tAuto-merging builtin-branch.c\n> 12\tAuto-merging builtin-reflog.c\n> 13\tCONFLICT (content): Merge conflict in builtin-reflog.c\n> 14\tAuto-merging builtin.h\n> 15\tAuto-merging git.c\n> 16\tRemoving merge-base.c\n> 17\tResolved 'builtin-reflog.c' using previous resolution.\n> 18\tAutomatic merge failed; fix conflicts and then commit the result.\n> \n> Among these, I think lines 2..3 are somewhat confusing but I am\n> used to seeing them and do not mind them too much.\n\nIn my experience these lines scare new users. And then they start\nto ignore other \"fatal:\" messages from Git because they can safely\nignore this particular one. Not good. One reason I like my patch\nthat's in next.\n\n> Lines 4..9 do not have any real information that helps the end\n> user (even though it would be a very good debugging aid for\n> merge-recursive developers).\n\nI agree. I've grown used to seeing them and read it for\nentertainment. Clearly I need to get out more. They probably\nshould be relegated to a GIT_MERGE_OPTIONS environment variable\nflag or to a command line parameter, as they are probably only\nuseful when debugging the application itself.\n \n> Lines 10..16 are useful, but I think we probably should show\n> them only for outermost merges.\n\nActually I think that only 13 is useful. 10-12,14-17 are\npretty useless messages in my mind. I really don't care that\nmerge-recursive automatically merged these files, as in all cases but\nthe one reported by line 13 the merge was successful. The diffstat\nthat is normally displayed by git-merge after a successful merge\nshows you what files were modified by the other branch. It also\noften causes the output of merge-recursive to scroll off the screen,\nmaking those messages even less useful.\n\n> An multi-base example:\n> 16\tAuto-merging gitweb/gitweb.perl\n> 17\tMerge made by recursive.\n> 18\t gitweb/gitweb.css | 2 +\n> 19\t gitweb/gitweb.perl | 165 ++++++++++++++++++++++++++++++++...\n> 20\t 2 files changed, 117 insertions(+), 50 deletions(-)\n> \n> I do not think we need to show 1..15 at all, perhaps without\n> \"export GIT_MERGE_BASE_DEBUG=YesPlease\".\n\nYes, I agree. Except I'd say 1..16, for the reason stated above.\n\nBut then I would like a progress meter, showing % of files resolved,\nto keep the user entertained. Alex has 1 min+ merges. 1 minute\nof absolutely no feedback is not very nice to a new user.\n\nMaybe when I'm done hacking on git-describe performance improvements\nI'll look at merge-recursive.\n\n-- \nShawn.\n"},{"id":"31623","messageId":"7vk5zrmmqx.fsf@assigned-by-dhcp.cox.net","threadId":"6218","inReplyTo":"20070113051447.GA22063@spearce.org","subject":"Re: [PATCH] merge-recursive: do not report the resulting tree object name","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-13T07:03:50Z","receivedAt":"2007-01-13T07:03:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n>> Among these, I think lines 2..3 are somewhat confusing but I am\n>> used to seeing them and do not mind them too much.\n>\n> In my experience these lines scare new users. And then they start\n> to ignore other \"fatal:\" messages from Git because they can safely\n> ignore this particular one.\n\nI tend to agree; we could do something like the attached.\n\n>> Lines 10..16 are useful, but I think we probably should show\n>> them only for outermost merges.\n>\n> Actually I think that only 13 is useful. 10-12,14-17 are\n> pretty useless messages in my mind.\n\nI am not sure. It is nice to view which paths have\ncontent-level merges as it is more significant than path-level\nmerges.\n\nI think the output from merge-recursive can be categorized into\n5 verbosity levels:\n\n 1. \"CONFLICT\", \"Rename\", \"Adding here instead due to D/F conflict\" (outermost)\n\n 2. \"Auto-merged successfully\" (outermost)\n\n 3. The first \"Merging X with Y\".\n\n 4. outermost \"Merging:\\ntitle1\\ntitle2\".\n\n 5. outermost \"found N common ancestors\\nancestor1\\nancestor2\\n...\"\n and anything from inner merge.\n\nI would prefer the default verbosity level to be 2 (that is,\nshow both 1 and 2); your \"quieter\" option would show only level\n1, and somebody who is debugging reursive would ask for all\nlevels.\n\n-- >8 --\n[PATCH] Make 'trivial merge' attempt less verbose.\n\nThis replaces die() calls in unpack-trees with simple and quiet\nexit(1) when we are trying trivial merges only and die() is\nabout the case that cannot trivially be merged (i.e. not a\nserious corruption error but expected). Also it makes the\nnontrivial merges exit early.\n\nAnd then this updates git-merge so that we do not have to say\n\"trying...\" followed by \"wonderful\" or \"nope\".\n\nSigned-off-by: Junio C Hamano \n---\n git-merge.sh | 36 ++++++++++++++++++++++--------------\n unpack-trees.c | 29 ++++++++++++++++++-----------\n 2 files changed, 40 insertions(+), 25 deletions(-)\n\ndiff --git a/git-merge.sh b/git-merge.sh\nindex 3eef048..6240a73 100755\n--- a/git-merge.sh\n+++ b/git-merge.sh\n@@ -302,21 +302,29 @@ f,*)\n \t# one common. See if it is really trivial.\n \tgit var GIT_COMMITTER_IDENT >/dev/null || exit\n \n-\techo \"Trying really trivial in-index merge...\"\n \tgit-update-index --refresh 2>/dev/null\n-\tif git-read-tree --trivial -m -u -v $common $head \"$1\" &&\n-\t result_tree=$(git-write-tree)\n-\tthen\n-\t echo \"Wonderful.\"\n-\t result_commit=$(\n-\t echo \"$merge_msg\" |\n-\t git-commit-tree $result_tree -p HEAD -p \"$1\"\n-\t ) || exit\n-\t finish \"$result_commit\" \"In-index merge\"\n-\t dropsave\n-\t exit 0\n-\tfi\n-\techo \"Nope.\"\n+\tgit-read-tree --trivial -m -u -v $common $head \"$1\"\n+\tcase \"$?\" in\n+\t1)\t: expected failure from non-trivial merge\n+\t\t;;\n+\t0)\n+\t\tif result_tree=$(git-write-tree)\n+\t\tthen\n+\t\t\techo \"Trivially merged in index.\"\n+\t\t\tresult_commit=$(\n+\t\t\t\techo \"$merge_msg\" |\n+\t\t\t\tgit-commit-tree $result_tree -p HEAD -p \"$1\"\n+\t\t\t) || exit\n+\t\t\tfinish \"$result_commit\" \"In-index merge\"\n+\t\t\tdropsave\n+\t\t\texit 0\n+\t\tfi\n+\t\t;;\n+\t*)\n+\t\t: This could be serious failure.\n+\t\techo >&2 \"Tried trivial merge but did not work; don't worry...\"\n+\t\t;;\n+\tesac\n \t;;\n *)\n \t# An octopus. If we can reach all the remote we are up to date.\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 2e2232c..0fca83b 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -409,17 +409,17 @@ int unpack_trees(struct object_list *trees, struct unpack_trees_options *o)\n \t\t\treturn -1;\n \t}\n \n-\tif (o->trivial_merges_only && o->nontrivial_merge)\n-\t\tdie(\"Merge requires file-level merging\");\n-\n \tcheck_updates(active_cache, active_nr, o);\n \treturn 0;\n }\n \n /* Here come the merge functions */\n \n-static void reject_merge(struct cache_entry *ce)\n+static void reject_merge(struct cache_entry *ce,\n+\t\t\t struct unpack_trees_options *o)\n {\n+\tif (o->trivial_merges_only)\n+\t\texit(1);\n \tdie(\"Entry '%s' would be overwritten by merge. Cannot merge.\",\n \t ce->name);\n }\n@@ -459,6 +459,8 @@ static void verify_uptodate(struct cache_entry *ce,\n \t}\n \tif (errno == ENOENT)\n \t\treturn;\n+\tif (o->trivial_merges_only)\n+\t\texit(1);\n \tdie(\"Entry '%s' not uptodate. Cannot merge.\", ce->name);\n }\n \n@@ -473,15 +475,18 @@ static void invalidate_ce_path(struct cache_entry *ce)\n * is not tracked, unless it is ignored.\n */\n static void verify_absent(const char *path, const char *action,\n-\t\tstruct unpack_trees_options *o)\n+\t\t\t struct unpack_trees_options *o)\n {\n \tstruct stat st;\n \n \tif (o->index_only || o->reset || !o->update)\n \t\treturn;\n-\tif (!lstat(path, &st) && !(o->dir && excluded(o->dir, path)))\n+\tif (!lstat(path, &st) && !(o->dir && excluded(o->dir, path))) {\n+\t\tif (o->trivial_merges_only)\n+\t\t\texit(1);\n \t\tdie(\"Untracked working tree file '%s' \"\n \t\t \"would be %s by merge.\", path, action);\n+\t}\n }\n \n static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n@@ -617,7 +622,7 @@ int threeway_merge(struct cache_entry **stages,\n \t/* #14, #14ALT, #2ALT */\n \tif (remote && !df_conflict_head && head_match && !remote_match) {\n \t\tif (index && !same(index, remote) && !same(index, head))\n-\t\t\treject_merge(index);\n+\t\t\treject_merge(index, o);\n \t\treturn merged_entry(remote, index, o);\n \t}\n \t/*\n@@ -625,7 +630,7 @@ int threeway_merge(struct cache_entry **stages,\n \t * make sure that it matches head.\n \t */\n \tif (index && !same(index, head)) {\n-\t\treject_merge(index);\n+\t\treject_merge(index, o);\n \t}\n \n \tif (head) {\n@@ -677,6 +682,8 @@ int threeway_merge(struct cache_entry **stages,\n \t}\n \n \to->nontrivial_merge = 1;\n+\tif (o->trivial_merges_only)\n+\t\texit(1);\n \n \t/* #2, #3, #4, #6, #7, #9, #11. */\n \tcount = 0;\n@@ -743,11 +750,11 @@ int twoway_merge(struct cache_entry **src,\n \t\telse {\n \t\t\t/* all other failures */\n \t\t\tif (oldtree)\n-\t\t\t\treject_merge(oldtree);\n+\t\t\t\treject_merge(oldtree, o);\n \t\t\tif (current)\n-\t\t\t\treject_merge(current);\n+\t\t\t\treject_merge(current, o);\n \t\t\tif (newtree)\n-\t\t\t\treject_merge(newtree);\n+\t\t\t\treject_merge(newtree, o);\n \t\t\treturn -1;\n \t\t}\n \t}\n-- \n1.5.0.rc1.g120b\n"},{"id":"31629","messageId":"Pine.LNX.4.63.0701131159510.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6218","inReplyTo":"eo9apg$sqo$1@sea.gmane.org","subject":"Re: [PATCH] merge-recursive: do not report the resulting tree object name","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-01-13T11:01:27Z","receivedAt":"2007-01-13T11:01:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 13 Jan 2007, Jakub Narebski wrote:\n\n> Junio C Hamano wrote:\n> \n> > I do not think we need to show 1..15 at all, perhaps without\n> > \"export GIT_MERGE_BASE_DEBUG=YesPlease\".\n> \n> Or a -v/--verbose (or even -v -v -v) flag set.\n\n... and you'd pass them from git-pull to git-merge to git-merge-recursive? \nThree different programs parse the same option? And worse, the other \nstrategies ignore the setting? What about strategies people implemented \non _their_ side, which do not know about the \"-v\" flag? I don't think so.\n\nCiao,\nDscho\n"}]}