{"thread":{"id":"15153","subject":"[bug] git `next' does not do trivial merges","startedAt":"2008-08-22T06:36:39Z","lastAt":"2008-08-28T13:43:00Z","messageCount":22,"participants":["Paolo Bonzini","Jeff King","Miklos Vajna","Junio C Hamano","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"88092","messageId":"g8lmpn$jvk$1@ger.gmane.org","threadId":"15153","inReplyTo":null,"subject":"[bug] git `next' does not do trivial merges","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2008-08-22T06:36:39Z","receivedAt":"2008-08-22T06:36:39Z","isPatch":false,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"I had already posted this bug report yesterday but it was hidden in a \ncover letter at \nhttp://permalink.gmane.org/gmane.comp.version-control.git/93143 -- so \nI'll copy the relevant info here:\n\n> \tThe included testcases demonstrate that trivial merges are\n> \tcurrently broken.  The failing test is:\n> \n> \t  git init\n> \t  echo a > a\n> \t  git add a\n> \t  git commit -ma \n> \t  git checkout -b branch\n> \t  echo b > b\n> \t  git add b\n> \t  git commit -mb\n> \t  git checkout master\n> \t  git merge --no-ff -s resolve branch\n> \n> \tRunning this in 1.5.5 shows: \n> \n> \t  Trying really trivial in-index merge...\n> \t  Wonderful.\n> \t  In-index merge\n> \n> \twhile `next' gives\n> \n> \t  Trying really trivial in-index merge...\n> \t  error: Untracked working tree file 'a' would be overwritten by merge.\n> \t  Nope.\n> \t  Trying simple merge.\n> \t  Merge made by resolve.\n\nPaolo\n"},{"id":"88177","messageId":"20080822193117.GA6361@coredump.intra.peff.net","threadId":"15153","inReplyTo":"g8lmpn$jvk$1@ger.gmane.org","subject":"Re: [bug] git `next' does not do trivial merges","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-08-22T19:31:17Z","receivedAt":"2008-08-22T19:31:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 22, 2008 at 08:36:39AM +0200, Paolo Bonzini wrote:\n\n> I had already posted this bug report yesterday but it was hidden in a  \n> cover letter at  \n> http://permalink.gmane.org/gmane.comp.version-control.git/93143 -- so  \n> I'll copy the relevant info here:\n\nSadly, this bisects to 1c7b76b (Build in merge).\n\n-Peff\n"},{"id":"88258","messageId":"20080823060839.GB23800@genesis.frugalware.org","threadId":"15153","inReplyTo":"20080822193117.GA6361@coredump.intra.peff.net","subject":"Re: [bug] git `next' does not do trivial merges","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-08-23T06:08:39Z","receivedAt":"2008-08-23T06:08:39Z","isPatch":false,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Fri, Aug 22, 2008 at 03:31:17PM -0400, Jeff King <peff@peff.net> wrote:\n> On Fri, Aug 22, 2008 at 08:36:39AM +0200, Paolo Bonzini wrote:\n> \n> > I had already posted this bug report yesterday but it was hidden in a  \n> > cover letter at  \n> > http://permalink.gmane.org/gmane.comp.version-control.git/93143 -- so  \n> > I'll copy the relevant info here:\n> \n> Sadly, this bisects to 1c7b76b (Build in merge).\n\nI guessed the bug is in builtin-merge.c::read_tree_trivial(), but I\ndon't see how it is different to builtin-read-tree.c::cmd_read_tree(),\nat least the unpack_trees_options struct is the same.\n\nI'm on it..\n"},{"id":"88262","messageId":"1219479293-29111-1-git-send-email-vmiklos@frugalware.org","threadId":"15153","inReplyTo":"20080823060839.GB23800@genesis.frugalware.org","subject":"[PATCH] Fix in-index merge.","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-08-23T08:14:53Z","receivedAt":"2008-08-23T08:14:53Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"There were 3 issues:\n\n1) We need read_cache() before using unpack_trees().\n\n2) commit_tree() frees the parents, so we need to malloc them.\n\n3) The current HEAD was missing from the parent list.\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n\nOn Fri, Aug 22, 2008 at 08:36:39AM +0200, Paolo Bonzini <bonzini@gnu.org> wrote:\n> I had already posted this bug report yesterday but it was hidden in a\n> cover\n> letter at\n> http://permalink.gmane.org/gmane.comp.version-control.git/93143\n\nThis should fix the issue.\n\n builtin-merge.c          |   11 +++++++----\n t/t7607-merge-inindex.sh |   29 +++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 4 deletions(-)\n create mode 100755 t/t7607-merge-inindex.sh\n\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex a201c66..dffe4b8 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -469,6 +469,7 @@ static int read_tree_trivial(unsigned char *common, unsigned char *head,\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tstruct unpack_trees_options opts;\n \n+\tread_cache();\n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = 2;\n \topts.src_index = &the_index;\n@@ -651,13 +652,15 @@ static void add_strategies(const char *string, unsigned attr)\n static int merge_trivial(void)\n {\n \tunsigned char result_tree[20], result_commit[20];\n-\tstruct commit_list parent;\n+\tstruct commit_list *parent = xmalloc(sizeof(struct commit_list *));\n \n \twrite_tree_trivial(result_tree);\n \tprintf(\"Wonderful.\\n\");\n-\tparent.item = remoteheads->item;\n-\tparent.next = NULL;\n-\tcommit_tree(merge_msg.buf, result_tree, &parent, result_commit);\n+\tparent->item = lookup_commit(head);\n+\tparent->next = xmalloc(sizeof(struct commit_list *));\n+\tparent->next->item = remoteheads->item;\n+\tparent->next->next = NULL;\n+\tcommit_tree(merge_msg.buf, result_tree, parent, result_commit);\n \tfinish(result_commit, \"In-index merge\");\n \tdrop_save();\n \treturn 0;\ndiff --git a/t/t7607-merge-inindex.sh b/t/t7607-merge-inindex.sh\nnew file mode 100755\nindex 0000000..98b778f\n--- /dev/null\n+++ b/t/t7607-merge-inindex.sh\n@@ -0,0 +1,29 @@\n+#!/bin/sh\n+\n+test_description='git-merge\n+\n+Testing in-index merge.'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo a > a &&\n+\tgit add a &&\n+\tgit commit -m a &&\n+\tgit tag a &&\n+\tgit checkout -b branch\n+\techo b > b &&\n+\tgit add b &&\n+\tgit commit -m b &&\n+\tgit tag b\n+'\n+\n+test_expect_success 'in-index merge' '\n+\tgit checkout master &&\n+\tgit merge --no-ff -s resolve branch > out &&\n+\tgrep Wonderful. out &&\n+\ttest \"$(git rev-parse a)\" = \"$(git rev-parse HEAD^1)\" &&\n+\ttest \"$(git rev-parse b)\" = \"$(git rev-parse HEAD^2)\"\n+'\n+\n+test_done\n-- \n1.6.0.rc3.17.gc14c8.dirty\n"},{"id":"88264","messageId":"1219479422-29148-1-git-send-email-vmiklos@frugalware.org","threadId":"15153","inReplyTo":"1219479293-29111-1-git-send-email-vmiklos@frugalware.org","subject":"[PATCH] builtin-merge: fail properly when we are in the middle of a conflicted merge","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-08-23T08:17:02Z","receivedAt":"2008-08-23T08:17:02Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"Using unmerged_cache() without reading the cache first never will return\nanything. However, if we read the cache early then we have to discard it\nwhen we want to read it again from the disk.\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n\nThe code part is pretty much from you, I wanted to add your signoff, but\nthen realized that you'll do it anyway. ;-)\n\n builtin-merge.c        |    6 +++---\n t/t7608-merge-dirty.sh |   33 +++++++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 3 deletions(-)\n create mode 100755 t/t7608-merge-dirty.sh\n\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex dffe4b8..71bd13b 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -565,8 +565,6 @@ static int checkout_fast_forward(unsigned char *head, unsigned char *remote)\n \tstruct dir_struct dir;\n \tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n \n-\tif (read_cache_unmerged())\n-\t\tdie(\"you need to resolve your current index first\");\n \trefresh_cache(REFRESH_QUIET);\n \n \tfd = hold_locked_index(lock_file, 1);\n@@ -746,6 +744,7 @@ static int evaluate_result(void)\n \tint cnt = 0;\n \tstruct rev_info rev;\n \n+\tdiscard_cache();\n \tif (read_cache() < 0)\n \t\tdie(\"failed to read the cache\");\n \n@@ -779,7 +778,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tstruct commit_list **remotes = &remoteheads;\n \n \tsetup_work_tree();\n-\tif (unmerged_cache())\n+\tif (read_cache_unmerged())\n \t\tdie(\"You are in the middle of a conflicted merge.\");\n \n \t/*\n@@ -1076,6 +1075,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t}\n \n \t\t/* Automerge succeeded. */\n+\t\tdiscard_cache();\n \t\twrite_tree_trivial(result_tree);\n \t\tautomerge_was_ok = 1;\n \t\tbreak;\ndiff --git a/t/t7608-merge-dirty.sh b/t/t7608-merge-dirty.sh\nnew file mode 100755\nindex 0000000..fb567f6\n--- /dev/null\n+++ b/t/t7608-merge-dirty.sh\n@@ -0,0 +1,33 @@\n+#!/bin/sh\n+\n+test_description='git-merge\n+\n+Merge should fail if the index has unresolved entries.'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo a > a &&\n+\tgit add a &&\n+\tgit commit -m a &&\n+\tgit tag a &&\n+\tgit checkout -b branch1\n+\techo b > a &&\n+\tgit add a &&\n+\tgit commit -m b &&\n+\tgit tag b\n+\tgit checkout -b branch2 HEAD~1\n+\techo c > a &&\n+\tgit add a &&\n+\tgit commit -m c &&\n+\tgit tag c\n+'\n+\n+test_expect_success 'in-index merge' '\n+\tgit checkout branch1 &&\n+\ttest_must_fail git merge branch2 &&\n+\ttest_must_fail git merge branch2 2> out &&\n+\tgrep \"You are in the middle of a conflicted merge\" out\n+'\n+\n+test_done\n-- \n1.6.0.rc3.17.gc14c8.dirty\n"},{"id":"88275","messageId":"7vd4k02jyr.fsf@gitster.siamese.dyndns.org","threadId":"15153","inReplyTo":"1219479293-29111-1-git-send-email-vmiklos@frugalware.org","subject":"Re: [PATCH] Fix in-index merge.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-23T08:50:52Z","receivedAt":"2008-08-23T08:50:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna <vmiklos@frugalware.org> writes:\n\n> There were 3 issues:\n>\n> 1) We need read_cache() before using unpack_trees().\n>\n> 2) commit_tree() frees the parents, so we need to malloc them.\n>\n> 3) The current HEAD was missing from the parent list.\n>\n> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>\n> ---\n>\n> On Fri, Aug 22, 2008 at 08:36:39AM +0200, Paolo Bonzini <bonzini@gnu.org> wrote:\n>> I had already posted this bug report yesterday but it was hidden in a\n>> cover\n>> letter at\n>> http://permalink.gmane.org/gmane.comp.version-control.git/93143\n>\n> This should fix the issue.\n>\n>  builtin-merge.c          |   11 +++++++----\n>  t/t7607-merge-inindex.sh |   29 +++++++++++++++++++++++++++++\n>  2 files changed, 36 insertions(+), 4 deletions(-)\n>  create mode 100755 t/t7607-merge-inindex.sh\n\nPlease do not add new testfiles unnecessarily.  You already have test\nscripts that cover \"git-merge\".  Add new ones to them.  The same comment\ngoes to the other patch.\n"},{"id":"88277","messageId":"7v3akw2jgo.fsf@gitster.siamese.dyndns.org","threadId":"15153","inReplyTo":"1219479422-29148-1-git-send-email-vmiklos@frugalware.org","subject":"Re: [PATCH] builtin-merge: fail properly when we are in the middle of a conflicted merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-23T09:01:43Z","receivedAt":"2008-08-23T09:01:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna <vmiklos@frugalware.org> writes:\n\n> Using unmerged_cache() without reading the cache first never will return\n> anything. However, if we read the cache early then we have to discard it\n> when we want to read it again from the disk.\n\nWith this, I do not think you would need to keep the read_cache() you\nadded to the beginning of the read_tree_trivial(), for the same reason you\nare removing read_cache_unmerged() from the beginning of\ncheckout_fast_forward() in this patch.\n"},{"id":"88281","messageId":"48AFD62F.2090500@gnu.org","threadId":"15153","inReplyTo":"1219479293-29111-1-git-send-email-vmiklos@frugalware.org","subject":"Re: [PATCH] Fix in-index merge.","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2008-08-23T09:19:43Z","receivedAt":"2008-08-23T09:19:43Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"Miklos Vajna wrote:\n> There were 3 issues:\n> \n> 1) We need read_cache() before using unpack_trees().\n> \n> 2) commit_tree() frees the parents, so we need to malloc them.\n> \n> 3) The current HEAD was missing from the parent list.\n\nUnfortunately it does not really work yet, since the files only present\non the second branch are not added.  I do get\n\nTrying really trivial in-index merge...\nWonderful.\nIn-index merge\n\nbut then \"git status\" gives:\n\n# On branch master\n# Untracked files:\n#   (use \"git add <file>...\" to include in what will be committed)\n#\n#       b\n\n(in my testcase for pre-merge, this results in a failure of the\n\"octopus\" testcase).\n\nPaolo\n"},{"id":"88284","messageId":"7vljyo12n2.fsf@gitster.siamese.dyndns.org","threadId":"15153","inReplyTo":"1219479422-29148-1-git-send-email-vmiklos@frugalware.org","subject":"Re: [PATCH] builtin-merge: fail properly when we are in the middle of a conflicted merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-23T09:50:25Z","receivedAt":"2008-08-23T09:50:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna <vmiklos@frugalware.org> writes:\n\n> Using unmerged_cache() without reading the cache first never will return\n> anything. However, if we read the cache early then we have to discard it\n> when we want to read it again from the disk.\n\nI do not think the fix is correct with or without this one.  You are\nwriting a wrong index and recording a wrong tree object in the commit.\n\nI have a two-liner fix for the issue I am testing right now.\n\ndiff --git c/t/t7605-merge-resolve.sh w/t/t7605-merge-resolve.sh\nindex ee21a10..a251dac 100755\n--- c/t/t7605-merge-resolve.sh\n+++ w/t/t7605-merge-resolve.sh\n@@ -36,7 +36,9 @@ test_expect_success 'merge c1 to c2' '\n \tgit diff --exit-code &&\n \ttest -f c0.c &&\n \ttest -f c1.c &&\n-\ttest -f c2.c\n+\ttest -f c2.c &&\n+\ttest 3 = $(git ls-tree -r HEAD | wc -l) &&\n+\ttest 2 = $(git ls-files)\n '\n \n test_expect_success 'merge c2 to c3 (fails)' '\n"},{"id":"88285","messageId":"7vej4g12dv.fsf@gitster.siamese.dyndns.org","threadId":"15153","inReplyTo":"48AFD62F.2090500@gnu.org","subject":"Re: [PATCH] Fix in-index merge.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-23T09:55:56Z","receivedAt":"2008-08-23T09:55:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Here is a tentative fix on top of Miklos's two patches, even though I\nwon't take them as-is until the test scripts are cleaned up.\n\n t/t7605-merge-resolve.sh |    4 +++-\n unpack-trees.c           |    2 ++\n 2 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git c/t/t7605-merge-resolve.sh w/t/t7605-merge-resolve.sh\nindex ee21a10..be21ded 100755\n--- c/t/t7605-merge-resolve.sh\n+++ w/t/t7605-merge-resolve.sh\n@@ -36,7 +36,9 @@ test_expect_success 'merge c1 to c2' '\n \tgit diff --exit-code &&\n \ttest -f c0.c &&\n \ttest -f c1.c &&\n-\ttest -f c2.c\n+\ttest -f c2.c &&\n+\ttest 3 = $(git ls-tree -r HEAD | wc -l) &&\n+\ttest 2 = $(git ls-files | wc -l)\n '\n \n test_expect_success 'merge c2 to c3 (fails)' '\ndiff --git c/unpack-trees.c w/unpack-trees.c\nindex cba0aca..016fd46 100644\n--- c/unpack-trees.c\n+++ w/unpack-trees.c\n@@ -378,6 +378,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \tmemset(&o->result, 0, sizeof(o->result));\n \tif (o->src_index)\n \t\to->result.timestamp = o->src_index->timestamp;\n+\tif (o->dst_index)\n+\t\to->result.alloc = xmalloc(1);\n \to->merge_size = len;\n \n \tif (!dfc)\n"},{"id":"88286","messageId":"7v7ia8126z.fsf@gitster.siamese.dyndns.org","threadId":"15153","inReplyTo":"7vej4g12dv.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix in-index merge.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-23T10:00:04Z","receivedAt":"2008-08-23T10:00:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Here is a tentative fix on top of Miklos's two patches, even though I\n> won't take them as-is until the test scripts are cleaned up.\n>\n>  t/t7605-merge-resolve.sh |    4 +++-\n>  unpack-trees.c           |    2 ++\n>  2 files changed, 5 insertions(+), 1 deletions(-)\n>\n> diff --git c/t/t7605-merge-resolve.sh w/t/t7605-merge-resolve.sh\n> index ee21a10..be21ded 100755\n> --- c/t/t7605-merge-resolve.sh\n> +++ w/t/t7605-merge-resolve.sh\n> @@ -36,7 +36,9 @@ test_expect_success 'merge c1 to c2' '\n>  \tgit diff --exit-code &&\n>  \ttest -f c0.c &&\n>  \ttest -f c1.c &&\n> -\ttest -f c2.c\n> +\ttest -f c2.c &&\n> +\ttest 3 = $(git ls-tree -r HEAD | wc -l) &&\n> +\ttest 2 = $(git ls-files | wc -l)\n>  '\n\nSorry, the last \"2\" should obviously be \"3\".  We are making sure all three\npaths are included in the result.\n\n>  \n>  test_expect_success 'merge c2 to c3 (fails)' '\n> diff --git c/unpack-trees.c w/unpack-trees.c\n> index cba0aca..016fd46 100644\n> --- c/unpack-trees.c\n> +++ w/unpack-trees.c\n> @@ -378,6 +378,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n>  \tmemset(&o->result, 0, sizeof(o->result));\n>  \tif (o->src_index)\n>  \t\to->result.timestamp = o->src_index->timestamp;\n> +\tif (o->dst_index)\n> +\t\to->result.alloc = xmalloc(1);\n>  \to->merge_size = len;\n>  \n>  \tif (!dfc)\n"},{"id":"88289","messageId":"7vbpzkypwz.fsf_-_@gitster.siamese.dyndns.org","threadId":"15153","inReplyTo":"7v7ia8126z.fsf@gitster.siamese.dyndns.org","subject":"[RFH] two and half potential fixlets to the in-core index handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-23T10:41:16Z","receivedAt":"2008-08-23T10:41:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"While debugging merge-in-C with Miklos, I noticed that relatively recent\ncodepaths that deal with the in-core index are subtly broken when a single\nprocess calls read_cache() more than once.\n\nA historical invariant in the API is \"if you have read the index or\npopulated it, then the next read_cache() is a no-op and you need to run\ndiscard_cache() beforehand if you want to read a new on-disk index.\"  I\nthink this was broken by relatively recent unpack-trees rewrite, namely\n34110cd (Make 'unpack_trees()' have a separate source and destination\nindex, 2008-03-06) and also 7a51ed6 (Make on-disk index representation\nseparate from in-core one, 2008-01-14).\n\nWith the current code, unpack_trees() builds its merge into a cleaned\no->result (an index_state structure), and it is copied to o->dst_index,\nwhich often is the_index.  But the logic in read_from_index() that\nimplements the historical invariant checks if istate->alloc is NULL (in\nwhich case the istate is empty -- just after startup, or just after\ndiscard_index()).  The \"result\" index_state does not have alloc because it\nis built from scratch and never allocated anything.\n\nThis wouldn't work very well.\n\nA hacky solution I have in the attached patch is to waste an xmalloc(1)\nand store it there when o->result is created, and also make\nread_from_index() pay attention to the cache_nr and the cache_changed\nbit. I think it is the safest and minimum fix.\n\nAnother independent issue is that istate has name_hash_initialized bit\nthat records the validity of the name_hash.  discard_index() frees the\nname_hash, but it never resets the bit to zero.  I am not sure what the\nramifications of not doing so, but it certainly feels wrong.\n\nAlso, discard_index() did not free the array of pointers istate->cache[];\nI think it should.\n\n read-cache.c   |   11 ++++-------\n unpack-trees.c |    2 ++\n 2 files changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git c/read-cache.c w/read-cache.c\nindex 2c03ec3..b29f263 100644\n--- c/read-cache.c\n+++ w/read-cache.c\n@@ -1144,7 +1144,7 @@ static inline size_t estimate_cache_size(size_t ondisk_size, unsigned int entrie\n \treturn ondisk_size + entries*per_entry;\n }\n \n-/* remember to discard_cache() before reading a different cache! */\n+/* remember to discard_index() before reading a different cache! */\n int read_index_from(struct index_state *istate, const char *path)\n {\n \tint fd, i;\n@@ -1155,7 +1155,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \tsize_t mmap_size;\n \n \terrno = EBUSY;\n-\tif (istate->alloc)\n+\tif (istate->alloc || istate->cache_nr || istate->cache_changed)\n \t\treturn istate->cache_nr;\n \n \terrno = ENOENT;\n@@ -1240,15 +1240,12 @@ unmap:\n \n int discard_index(struct index_state *istate)\n {\n-\tistate->cache_nr = 0;\n-\tistate->cache_changed = 0;\n-\tistate->timestamp = 0;\n \tfree_hash(&istate->name_hash);\n \tcache_tree_free(&(istate->cache_tree));\n \tfree(istate->alloc);\n-\tistate->alloc = NULL;\n+\tfree(istate->cache);\n \n-\t/* no need to throw away allocated active_cache */\n+\tmemset(istate, 0, sizeof(*istate));\n \treturn 0;\n }\n \ndiff --git c/unpack-trees.c w/unpack-trees.c\nindex cba0aca..016fd46 100644\n--- c/unpack-trees.c\n+++ w/unpack-trees.c\n@@ -378,6 +378,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \tmemset(&o->result, 0, sizeof(o->result));\n \tif (o->src_index)\n \t\to->result.timestamp = o->src_index->timestamp;\n+\tif (o->dst_index)\n+\t\to->result.alloc = xmalloc(1);\n \to->merge_size = len;\n \n \tif (!dfc)\n"},{"id":"88288","messageId":"1219488083-4535-1-git-send-email-vmiklos@frugalware.org","threadId":"15153","inReplyTo":"7vd4k02jyr.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] Fix in-index merge.","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-08-23T10:41:23Z","receivedAt":"2008-08-23T10:41:23Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"There were 3 issues:\n\n1) We need read_cache() before using unpack_trees().\n\n2) commit_tree() frees the parents, so we need to malloc them.\n\n3) The current HEAD was missing from the parent list.\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n\nOn Sat, Aug 23, 2008 at 01:50:52AM -0700, Junio C Hamano <gitster@pobox.com> wrote:\n> Please do not add new testfiles unnecessarily.  You already have test\n> scripts that cover \"git-merge\".  Add new ones to them.  The same\n> comment\n> goes to the other patch.\n\nHere it is.\n\n builtin-merge.c  |   11 +++++++----\n t/t7600-merge.sh |    9 +++++++++\n 2 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex a201c66..dffe4b8 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -469,6 +469,7 @@ static int read_tree_trivial(unsigned char *common, unsigned char *head,\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tstruct unpack_trees_options opts;\n \n+\tread_cache();\n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = 2;\n \topts.src_index = &the_index;\n@@ -651,13 +652,15 @@ static void add_strategies(const char *string, unsigned attr)\n static int merge_trivial(void)\n {\n \tunsigned char result_tree[20], result_commit[20];\n-\tstruct commit_list parent;\n+\tstruct commit_list *parent = xmalloc(sizeof(struct commit_list *));\n \n \twrite_tree_trivial(result_tree);\n \tprintf(\"Wonderful.\\n\");\n-\tparent.item = remoteheads->item;\n-\tparent.next = NULL;\n-\tcommit_tree(merge_msg.buf, result_tree, &parent, result_commit);\n+\tparent->item = lookup_commit(head);\n+\tparent->next = xmalloc(sizeof(struct commit_list *));\n+\tparent->next->item = remoteheads->item;\n+\tparent->next->next = NULL;\n+\tcommit_tree(merge_msg.buf, result_tree, parent, result_commit);\n \tfinish(result_commit, \"In-index merge\");\n \tdrop_save();\n \treturn 0;\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex fee8fb7..dbc90bc 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -498,4 +498,13 @@ test_expect_success 'merge fast-forward in a dirty tree' '\n \n test_debug 'gitk --all'\n \n+test_expect_success 'in-index merge' '\n+\tgit reset --hard c0 &&\n+\tgit merge --no-ff -s resolve c1 > out &&\n+\tgrep \"Wonderful.\" out &&\n+\tverify_parents $c0 $c1\n+'\n+\n+test_debug 'gitk --all'\n+\n test_done\n-- \n1.6.0.rc3.17.gc14c8.dirty\n"},{"id":"88291","messageId":"1219489071-5679-1-git-send-email-vmiklos@frugalware.org","threadId":"15153","inReplyTo":"7v3akw2jgo.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] builtin-merge: fail properly when we are in the middle of a conflicted merge","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-08-23T10:57:51Z","receivedAt":"2008-08-23T10:57:51Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"Using unmerged_cache() without reading the cache first never will return\nanything. However, if we read the cache early then we have to discard it\nwhen we want to read it again from the disk.\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n\nOn Sat, Aug 23, 2008 at 02:01:43AM -0700, Junio C Hamano <gitster@pobox.com> wrote:\n> With this, I do not think you would need to keep the read_cache() you\n> added to the beginning of the read_tree_trivial(), for the same reason\n> you\n> are removing read_cache_unmerged() from the beginning of\n> checkout_fast_forward() in this patch.\n\nRight, I removed it as well. Also fixed the testcase (no more new file).\n\n builtin-merge.c            |    7 +++----\n t/t3030-merge-recursive.sh |   11 +++++++++++\n 2 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex dffe4b8..b280444 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -469,7 +469,6 @@ static int read_tree_trivial(unsigned char *common, unsigned char *head,\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tstruct unpack_trees_options opts;\n \n-\tread_cache();\n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = 2;\n \topts.src_index = &the_index;\n@@ -565,8 +564,6 @@ static int checkout_fast_forward(unsigned char *head, unsigned char *remote)\n \tstruct dir_struct dir;\n \tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n \n-\tif (read_cache_unmerged())\n-\t\tdie(\"you need to resolve your current index first\");\n \trefresh_cache(REFRESH_QUIET);\n \n \tfd = hold_locked_index(lock_file, 1);\n@@ -746,6 +743,7 @@ static int evaluate_result(void)\n \tint cnt = 0;\n \tstruct rev_info rev;\n \n+\tdiscard_cache();\n \tif (read_cache() < 0)\n \t\tdie(\"failed to read the cache\");\n \n@@ -779,7 +777,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tstruct commit_list **remotes = &remoteheads;\n \n \tsetup_work_tree();\n-\tif (unmerged_cache())\n+\tif (read_cache_unmerged())\n \t\tdie(\"You are in the middle of a conflicted merge.\");\n \n \t/*\n@@ -1076,6 +1074,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t}\n \n \t\t/* Automerge succeeded. */\n+\t\tdiscard_cache();\n \t\twrite_tree_trivial(result_tree);\n \t\tautomerge_was_ok = 1;\n \t\tbreak;\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex aff3603..f288015 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -269,6 +269,17 @@ test_expect_success 'merge-recursive result' '\n \n '\n \n+test_expect_success 'fail if the index has unresolved entries' '\n+\n+\trm -fr [abcd] &&\n+\tgit checkout -f \"$c1\" &&\n+\n+\ttest_must_fail git merge \"$c5\" &&\n+\ttest_must_fail git merge \"$c5\" 2> out &&\n+\tgrep \"You are in the middle of a conflicted merge\" out\n+\n+'\n+\n test_expect_success 'merge-recursive remove conflict' '\n \n \trm -fr [abcd] &&\n-- \n1.6.0.rc3.17.gc14c8.dirty\n"},{"id":"88301","messageId":"alpine.LFD.1.10.0808231104500.3363@nehalem.linux-foundation.org","threadId":"15153","inReplyTo":"7vbpzkypwz.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: [RFH] two and half potential fixlets to the in-core index handling","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-08-23T18:13:14Z","receivedAt":"2008-08-23T18:13:14Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 23 Aug 2008, Junio C Hamano wrote:\n> \n> A hacky solution I have in the attached patch is to waste an xmalloc(1)\n> and store it there when o->result is created, and also make\n> read_from_index() pay attention to the cache_nr and the cache_changed\n> bit. I think it is the safest and minimum fix.\n\nHmm. Wouldn't it be nicer to just add another bit to istate? We have the \nspace already, since we already have a bitfield there, with just one bit \nused?\n\nSomething like this..\n\nUntested. Of course.\n\n\t\t\tLinus\n\n---\n cache.h        |    3 ++-\n read-cache.c   |    4 +++-\n unpack-trees.c |    1 +\n 3 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 68ce6e6..6e41ec4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -222,7 +222,8 @@ struct index_state {\n \tstruct cache_tree *cache_tree;\n \ttime_t timestamp;\n \tvoid *alloc;\n-\tunsigned name_hash_initialized : 1;\n+\tunsigned name_hash_initialized : 1,\n+\t\t initialized : 1;\n \tstruct hash_table name_hash;\n };\n \ndiff --git a/read-cache.c b/read-cache.c\nindex 2c03ec3..35fec46 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1155,7 +1155,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \tsize_t mmap_size;\n \n \terrno = EBUSY;\n-\tif (istate->alloc)\n+\tif (istate->initialized)\n \t\treturn istate->cache_nr;\n \n \terrno = ENOENT;\n@@ -1195,6 +1195,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \t * index size\n \t */\n \tistate->alloc = xmalloc(estimate_cache_size(mmap_size, istate->cache_nr));\n+\tistate->initialized = 1;\n \n \tsrc_offset = sizeof(*hdr);\n \tdst_offset = 0;\n@@ -1247,6 +1248,7 @@ int discard_index(struct index_state *istate)\n \tcache_tree_free(&(istate->cache_tree));\n \tfree(istate->alloc);\n \tistate->alloc = NULL;\n+\tistate->initialized = 0;\n \n \t/* no need to throw away allocated active_cache */\n \treturn 0;\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex cba0aca..ef21c62 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -376,6 +376,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \tstate.refresh_cache = 1;\n \n \tmemset(&o->result, 0, sizeof(o->result));\n+\to->result.initialized = 1;\n \tif (o->src_index)\n \t\to->result.timestamp = o->src_index->timestamp;\n \to->merge_size = len;\n"},{"id":"88305","messageId":"7v1w0fzgpc.fsf@gitster.siamese.dyndns.org","threadId":"15153","inReplyTo":"alpine.LFD.1.10.0808231104500.3363@nehalem.linux-foundation.org","subject":"Re: [RFH] two and half potential fixlets to the in-core index handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-23T19:14:55Z","receivedAt":"2008-08-23T19:14:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Sat, 23 Aug 2008, Junio C Hamano wrote:\n>> \n>> A hacky solution I have in the attached patch is to waste an xmalloc(1)\n>> and store it there when o->result is created, and also make\n>> read_from_index() pay attention to the cache_nr and the cache_changed\n>> bit. I think it is the safest and minimum fix.\n>\n> Hmm. Wouldn't it be nicer to just add another bit to istate? We have the \n> space already, since we already have a bitfield there, with just one bit \n> used?\n\ncache_changed can also become a single-bit field.\n\nThe oldest \"the_index\" users work from it in the BSS initialized all\nzero.  We'd also need to mark it as initialized, wouldn't we?\n"},{"id":"88307","messageId":"7vwsi7y1tp.fsf@gitster.siamese.dyndns.org","threadId":"15153","inReplyTo":"7v1w0fzgpc.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFH] two and half potential fixlets to the in-core index handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-23T19:21:38Z","receivedAt":"2008-08-23T19:21:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Linus Torvalds <torvalds@linux-foundation.org> writes:\n>\n>> On Sat, 23 Aug 2008, Junio C Hamano wrote:\n>>> \n>>> A hacky solution I have in the attached patch is to waste an xmalloc(1)\n>>> and store it there when o->result is created, and also make\n>>> read_from_index() pay attention to the cache_nr and the cache_changed\n>>> bit. I think it is the safest and minimum fix.\n>>\n>> Hmm. Wouldn't it be nicer to just add another bit to istate? We have the \n>> space already, since we already have a bitfield there, with just one bit \n>> used?\n>\n> cache_changed can also become a single-bit field.\n>\n> The oldest \"the_index\" users work from it in the BSS initialized all\n> zero.  We'd also need to mark it as initialized, wouldn't we?\n\nAh, sorry, even they start with read_cache() to read .git/index (which may\nnot exist yet, which is fine).\n\nSorry for a stupid thinko.\n"},{"id":"88314","messageId":"7vk5e7y0a0.fsf@gitster.siamese.dyndns.org","threadId":"15153","inReplyTo":"1219489071-5679-1-git-send-email-vmiklos@frugalware.org","subject":"Re: [PATCH] builtin-merge: fail properly when we are in the middle of a conflicted merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-23T19:55:03Z","receivedAt":"2008-08-23T19:55:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We've exchanged quite a few \"here is a better one\", \"oops, that is not\nenough\", which is confusing to bystanders.  Here is my proposed final\nseries, meant to be applied to 'maint'.\n\n  [1/2] merge: fix numerus bugs around \"trivial merge\" area\n  [2/2] unpack_trees(): protect the handcrafted in-core index from read_cache()\n"},{"id":"88315","messageId":"7vfxovy06u.fsf_-_@gitster.siamese.dyndns.org","threadId":"15153","inReplyTo":"7vk5e7y0a0.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 1/2] merge: fix numerus bugs around \"trivial merge\" area","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-23T19:56:57Z","receivedAt":"2008-08-23T19:56:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The \"trivial merge\" codepath wanted to optimize itself by making an\ninternal call to the read-tree machinery, but it did not read the index\nbefore doing so, and the codepath was never exercised.  Incidentally, this\nfailure to read the index upfront meant that the safety to refuse doing\nanything when the index is unmerged did not kick in, either.\n\nThese two problem are fixed by using read_cache_unmerged() that does read\nthe index before checking if it is unmerged at the beginning of\ncmd_merge().\n\nThe primary logic of the merge, however, had an assumption that the\nprocess never read the index in-core, and write_cache_as_tree() call it\nmakes from write_tree_trivial() will always read from the on-disk index\nthe strategies created and write it out as a tree.  This assumption is now\nbroken by the above fix.  It now calls discard_cache() before calling\nwrite_tree_trivial() when it wants to write the on-disk index as a tree to\nfix this issue.\n\nWhen multiple strategies are tried, their results are evaluated by reading\nthe resulting index and inspecting it.  The codepath needs to make a call\nto read_cache() for each successful strategy, and for that to work, they\nneed to discard_cache() the one from the previous round.\n\nAlso the \"trivial merge\" forgot that the current commit is one of the\nparents of the resulting commit.\n\nThis still has breakage in the way it writes out the resulting tree out of\nthe trivial merge codepath, which is a topic of the next patch.  One test\nin t7605-merge-resolve.sh exposes this breakage.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-merge.c            |   16 +++++++++-------\n t/t3030-merge-recursive.sh |   11 +++++++++++\n t/t7600-merge.sh           |    9 +++++++++\n t/t7605-merge-resolve.sh   |    6 ++++--\n 4 files changed, 33 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex a201c66..b280444 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -564,8 +564,6 @@ static int checkout_fast_forward(unsigned char *head, unsigned char *remote)\n \tstruct dir_struct dir;\n \tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n \n-\tif (read_cache_unmerged())\n-\t\tdie(\"you need to resolve your current index first\");\n \trefresh_cache(REFRESH_QUIET);\n \n \tfd = hold_locked_index(lock_file, 1);\n@@ -651,13 +649,15 @@ static void add_strategies(const char *string, unsigned attr)\n static int merge_trivial(void)\n {\n \tunsigned char result_tree[20], result_commit[20];\n-\tstruct commit_list parent;\n+\tstruct commit_list *parent = xmalloc(sizeof(struct commit_list *));\n \n \twrite_tree_trivial(result_tree);\n \tprintf(\"Wonderful.\\n\");\n-\tparent.item = remoteheads->item;\n-\tparent.next = NULL;\n-\tcommit_tree(merge_msg.buf, result_tree, &parent, result_commit);\n+\tparent->item = lookup_commit(head);\n+\tparent->next = xmalloc(sizeof(struct commit_list *));\n+\tparent->next->item = remoteheads->item;\n+\tparent->next->next = NULL;\n+\tcommit_tree(merge_msg.buf, result_tree, parent, result_commit);\n \tfinish(result_commit, \"In-index merge\");\n \tdrop_save();\n \treturn 0;\n@@ -743,6 +743,7 @@ static int evaluate_result(void)\n \tint cnt = 0;\n \tstruct rev_info rev;\n \n+\tdiscard_cache();\n \tif (read_cache() < 0)\n \t\tdie(\"failed to read the cache\");\n \n@@ -776,7 +777,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tstruct commit_list **remotes = &remoteheads;\n \n \tsetup_work_tree();\n-\tif (unmerged_cache())\n+\tif (read_cache_unmerged())\n \t\tdie(\"You are in the middle of a conflicted merge.\");\n \n \t/*\n@@ -1073,6 +1074,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t}\n \n \t\t/* Automerge succeeded. */\n+\t\tdiscard_cache();\n \t\twrite_tree_trivial(result_tree);\n \t\tautomerge_was_ok = 1;\n \t\tbreak;\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex aff3603..f288015 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -269,6 +269,17 @@ test_expect_success 'merge-recursive result' '\n \n '\n \n+test_expect_success 'fail if the index has unresolved entries' '\n+\n+\trm -fr [abcd] &&\n+\tgit checkout -f \"$c1\" &&\n+\n+\ttest_must_fail git merge \"$c5\" &&\n+\ttest_must_fail git merge \"$c5\" 2> out &&\n+\tgrep \"You are in the middle of a conflicted merge\" out\n+\n+'\n+\n test_expect_success 'merge-recursive remove conflict' '\n \n \trm -fr [abcd] &&\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex fee8fb7..dbc90bc 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -498,4 +498,13 @@ test_expect_success 'merge fast-forward in a dirty tree' '\n \n test_debug 'gitk --all'\n \n+test_expect_success 'in-index merge' '\n+\tgit reset --hard c0 &&\n+\tgit merge --no-ff -s resolve c1 > out &&\n+\tgrep \"Wonderful.\" out &&\n+\tverify_parents $c0 $c1\n+'\n+\n+test_debug 'gitk --all'\n+\n test_done\ndiff --git a/t/t7605-merge-resolve.sh b/t/t7605-merge-resolve.sh\nindex ee21a10..5c53608 100755\n--- a/t/t7605-merge-resolve.sh\n+++ b/t/t7605-merge-resolve.sh\n@@ -27,7 +27,7 @@ test_expect_success 'setup' '\n \tgit tag c3\n '\n \n-test_expect_success 'merge c1 to c2' '\n+test_expect_failure 'merge c1 to c2' '\n \tgit reset --hard c1 &&\n \tgit merge -s resolve c2 &&\n \ttest \"$(git rev-parse c1)\" != \"$(git rev-parse HEAD)\" &&\n@@ -36,7 +36,9 @@ test_expect_success 'merge c1 to c2' '\n \tgit diff --exit-code &&\n \ttest -f c0.c &&\n \ttest -f c1.c &&\n-\ttest -f c2.c\n+\ttest -f c2.c &&\n+\ttest 3 = $(git ls-tree -r HEAD | wc -l) &&\n+\ttest 3 = $(git ls-files | wc -l)\n '\n \n test_expect_success 'merge c2 to c3 (fails)' '\n-- \n1.6.0.51.g078ae\n"},{"id":"88316","messageId":"7vbpzjy05x.fsf_-_@gitster.siamese.dyndns.org","threadId":"15153","inReplyTo":"7vk5e7y0a0.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 2/2] unpack_trees(): protect the handcrafted in-core index from read_cache()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-23T19:57:30Z","receivedAt":"2008-08-23T19:57:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"unpack_trees() rebuilds the in-core index from scratch by allocating a new\nstructure and finishing it off by copying the built one to the final\nindex.\n\nThe resulting in-core index is Ok for most use, but read_cache() does not\nrecognize it as such.  The function is meant to be no-op if you already\nhave loaded the index, until you call discard_cache().\n\nThis change the way read_cache() detects an already initialized in-core\nindex, by introducing an extra bit, and marks the handcrafted in-core\nindex as initialized, to avoid this problem.\n\nA better fix in the longer term would be to change the read_cache() API so\nthat it will always discard and re-read from the on-disk index to avoid\nconfusion.  But there are higher level API that have relied on the current\nsemantics, and they and their users all need to get converted, which is\noutside the scope of 'maint' track.\n\nAn example of such a higher level API is write_cache_as_tree(), which is\nused by git-write-tree as well as later Porcelains like git-merge, revert\nand cherry-pick.  In the longer term, we should remove read_cache() from\nthere and add one to cmd_write_tree(); other callers expect that the\nin-core index they prepared is what gets written as a tree so no other\nchange is necessary for this particular codepath.\n\nThe original version of this patch marked the index by pointing an\notherwise wasted malloc'ed memory with o->result.alloc, but this version\nuses Linus's idea to use a new \"initialized\" bit, which is conceptually\nmuch cleaner.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n cache.h                  |    3 ++-\n read-cache.c             |    4 +++-\n t/t7605-merge-resolve.sh |    2 +-\n unpack-trees.c           |    1 +\n 4 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 2475de9..884fae8 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -222,7 +222,8 @@ struct index_state {\n \tstruct cache_tree *cache_tree;\n \ttime_t timestamp;\n \tvoid *alloc;\n-\tunsigned name_hash_initialized : 1;\n+\tunsigned name_hash_initialized : 1,\n+\t\t initialized : 1;\n \tstruct hash_table name_hash;\n };\n \ndiff --git a/read-cache.c b/read-cache.c\nindex 2c03ec3..35fec46 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1155,7 +1155,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \tsize_t mmap_size;\n \n \terrno = EBUSY;\n-\tif (istate->alloc)\n+\tif (istate->initialized)\n \t\treturn istate->cache_nr;\n \n \terrno = ENOENT;\n@@ -1195,6 +1195,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \t * index size\n \t */\n \tistate->alloc = xmalloc(estimate_cache_size(mmap_size, istate->cache_nr));\n+\tistate->initialized = 1;\n \n \tsrc_offset = sizeof(*hdr);\n \tdst_offset = 0;\n@@ -1247,6 +1248,7 @@ int discard_index(struct index_state *istate)\n \tcache_tree_free(&(istate->cache_tree));\n \tfree(istate->alloc);\n \tistate->alloc = NULL;\n+\tistate->initialized = 0;\n \n \t/* no need to throw away allocated active_cache */\n \treturn 0;\ndiff --git a/t/t7605-merge-resolve.sh b/t/t7605-merge-resolve.sh\nindex 5c53608..f1f86dd 100755\n--- a/t/t7605-merge-resolve.sh\n+++ b/t/t7605-merge-resolve.sh\n@@ -27,7 +27,7 @@ test_expect_success 'setup' '\n \tgit tag c3\n '\n \n-test_expect_failure 'merge c1 to c2' '\n+test_expect_success 'merge c1 to c2' '\n \tgit reset --hard c1 &&\n \tgit merge -s resolve c2 &&\n \ttest \"$(git rev-parse c1)\" != \"$(git rev-parse HEAD)\" &&\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex cba0aca..ef21c62 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -376,6 +376,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \tstate.refresh_cache = 1;\n \n \tmemset(&o->result, 0, sizeof(o->result));\n+\to->result.initialized = 1;\n \tif (o->src_index)\n \t\to->result.timestamp = o->src_index->timestamp;\n \to->merge_size = len;\n-- \n1.6.0.51.g078ae\n"},{"id":"88342","messageId":"7vtzdbuqb6.fsf@gitster.siamese.dyndns.org","threadId":"15153","inReplyTo":"7vfxovy06u.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] merge: fix numerus bugs around \"trivial merge\" area","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-24T01:58:37Z","receivedAt":"2008-08-24T01:58:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> The primary logic of the merge, however, had an assumption that the\n> process never read the index in-core, and write_cache_as_tree() call it\n> makes from write_tree_trivial() will always read from the on-disk index\n> the strategies created and write it out as a tree.  This assumption is now\n> broken by the above fix.  It now calls discard_cache() before calling\n> write_tree_trivial() when it wants to write the on-disk index as a tree to\n> fix this issue.\n\nBy the way, in the medium term, if we are serious about making an internal\ncall to merge_recursive() from cmd_merge(), I think we may be better off\nmaking it the responsibility for try_merge_strategy() to leave an\ncommittable state in the in-core index (aka \"the_index\") when they return\nwith 0 (success) status.  After calling external ones via the run_command\ninterface, it should do a read_cache() (after calling discard_cache() if\nneeded); if it calls merge_recursive(), hopefully you already have the\ncommittable state in the in-core index.\n\nThat way, when automerge succeeds, write_tree_trivial() can write that\nin-core index out and create the tree object to be committed. The\ncallchain to use merge_recursive() can avoid having to write to the\non-disk index, read it again and write out the tree from it.\n"},{"id":"88885","messageId":"1219930980-29114-1-git-send-email-vmiklos@frugalware.org","threadId":"15153","inReplyTo":"7vtzdbuqb6.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] builtin-merge: avoid run_command_v_opt() for recursive and subtree","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-08-28T13:43:00Z","receivedAt":"2008-08-28T13:43:00Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"The try_merge_strategy() function always ran the strategy in a separate\nprocess, though this is not always necessary. The recursive and subtree\nstrategy can be called without a fork(). This patch adds a check, and\ncalls recursive in the same process without wasting resources.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n\nOn Sat, Aug 23, 2008 at 06:58:37PM -0700, Junio C Hamano <gitster@pobox.com> wrote:\n> By the way, in the medium term, if we are serious about making an\n> internal call to merge_recursive() from cmd_merge(), I think we may be\n> better off making it the responsibility for try_merge_strategy() to\n> leave an committable state in the in-core index (aka \"the_index\") when\n> they return with 0 (success) status.  After calling external ones via\n> the run_command interface, it should do a read_cache() (after calling\n> discard_cache() if needed); if it calls merge_recursive(), hopefully\n> you already have the committable state in the in-core index.\n>\n> That way, when automerge succeeds, write_tree_trivial() can write that\n> in-core index out and create the tree object to be committed. The\n> callchain to use merge_recursive() can avoid having to write to the\n> on-disk index, read it again and write out the tree from it.\n\nThis is mostly the same patch I submitted earlier, but this is now on\ntop of mv/merge-recursive, and now it does what you suggested above.\n\n builtin-merge.c |   92 +++++++++++++++++++++++++++++++++++++-----------------\n 1 files changed, 63 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex 0bff26e..3a38089 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -22,6 +22,7 @@\n #include \"log-tree.h\"\n #include \"color.h\"\n #include \"rerere.h\"\n+#include \"merge-recursive.h\"\n \n #define DEFAULT_TWOHEAD (1<<0)\n #define DEFAULT_OCTOPUS (1<<1)\n@@ -511,28 +512,64 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \tstruct commit_list *j;\n \tstruct strbuf buf;\n \n-\targs = xmalloc((4 + commit_list_count(common) +\n-\t\t\tcommit_list_count(remoteheads)) * sizeof(char *));\n-\tstrbuf_init(&buf, 0);\n-\tstrbuf_addf(&buf, \"merge-%s\", strategy);\n-\targs[i++] = buf.buf;\n-\tfor (j = common; j; j = j->next)\n-\t\targs[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));\n-\targs[i++] = \"--\";\n-\targs[i++] = head_arg;\n-\tfor (j = remoteheads; j; j = j->next)\n-\t\targs[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));\n-\targs[i] = NULL;\n-\tret = run_command_v_opt(args, RUN_GIT_CMD);\n-\tstrbuf_release(&buf);\n-\ti = 1;\n-\tfor (j = common; j; j = j->next)\n-\t\tfree((void *)args[i++]);\n-\ti += 2;\n-\tfor (j = remoteheads; j; j = j->next)\n-\t\tfree((void *)args[i++]);\n-\tfree(args);\n-\treturn -ret;\n+\tif (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n+\t\tint clean;\n+\t\tstruct commit *result;\n+\t\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n+\t\tint index_fd;\n+\t\tstruct commit_list *reversed = NULL;\n+\t\tstruct merge_options o;\n+\n+\t\tif (remoteheads->next) {\n+\t\t\terror(\"Not handling anything other than two heads merge.\");\n+\t\t\treturn 2;\n+\t\t}\n+\n+\t\tinit_merge_options(&o);\n+\t\tif (!strcmp(strategy, \"subtree\"))\n+\t\t\to.subtree_merge = 1;\n+\n+\t\to.branch1 = head_arg;\n+\t\to.branch2 = remoteheads->item->util;\n+\n+\t\tfor (j = common; j; j = j->next)\n+\t\t\tcommit_list_insert(j->item, &reversed);\n+\n+\t\tindex_fd = hold_locked_index(lock, 1);\n+\t\tclean = merge_recursive(&o, lookup_commit(head),\n+\t\t\t\tremoteheads->item, reversed, &result);\n+\t\tif (active_cache_changed &&\n+\t\t\t\t(write_cache(index_fd, active_cache, active_nr) ||\n+\t\t\t\t commit_locked_index(lock)))\n+\t\t\tdie (\"unable to write %s\", get_index_file());\n+\t\treturn clean ? 0 : 1;\n+\t} else {\n+\t\targs = xmalloc((4 + commit_list_count(common) +\n+\t\t\t\t\tcommit_list_count(remoteheads)) * sizeof(char *));\n+\t\tstrbuf_init(&buf, 0);\n+\t\tstrbuf_addf(&buf, \"merge-%s\", strategy);\n+\t\targs[i++] = buf.buf;\n+\t\tfor (j = common; j; j = j->next)\n+\t\t\targs[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));\n+\t\targs[i++] = \"--\";\n+\t\targs[i++] = head_arg;\n+\t\tfor (j = remoteheads; j; j = j->next)\n+\t\t\targs[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));\n+\t\targs[i] = NULL;\n+\t\tret = run_command_v_opt(args, RUN_GIT_CMD);\n+\t\tstrbuf_release(&buf);\n+\t\ti = 1;\n+\t\tfor (j = common; j; j = j->next)\n+\t\t\tfree((void *)args[i++]);\n+\t\ti += 2;\n+\t\tfor (j = remoteheads; j; j = j->next)\n+\t\t\tfree((void *)args[i++]);\n+\t\tfree(args);\n+\t\tdiscard_cache();\n+\t\tif (read_cache() < 0)\n+\t\t\tdie(\"failed to read the cache\");\n+\t\treturn -ret;\n+\t}\n }\n \n static void count_diff_files(struct diff_queue_struct *q,\n@@ -743,10 +780,6 @@ static int evaluate_result(void)\n \tint cnt = 0;\n \tstruct rev_info rev;\n \n-\tdiscard_cache();\n-\tif (read_cache() < 0)\n-\t\tdie(\"failed to read the cache\");\n-\n \t/* Check how many files differ. */\n \tinit_revisions(&rev, \"\");\n \tsetup_revisions(0, NULL, &rev, NULL);\n@@ -880,12 +913,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \n \tfor (i = 0; i < argc; i++) {\n \t\tstruct object *o;\n+\t\tstruct commit *commit;\n \n \t\to = peel_to_type(argv[i], 0, NULL, OBJ_COMMIT);\n \t\tif (!o)\n \t\t\tdie(\"%s - not something we can merge\", argv[i]);\n-\t\tremotes = &commit_list_insert(lookup_commit(o->sha1),\n-\t\t\tremotes)->next;\n+\t\tcommit = lookup_commit(o->sha1);\n+\t\tcommit->util = (void *)argv[i];\n+\t\tremotes = &commit_list_insert(commit, remotes)->next;\n \n \t\tstrbuf_addf(&buf, \"GITHEAD_%s\", sha1_to_hex(o->sha1));\n \t\tsetenv(buf.buf, argv[i], 1);\n@@ -1079,7 +1114,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t}\n \n \t\t/* Automerge succeeded. */\n-\t\tdiscard_cache();\n \t\twrite_tree_trivial(result_tree);\n \t\tautomerge_was_ok = 1;\n \t\tbreak;\n-- \n1.6.0\n"}]}