{"thread":{"id":"41417","subject":"malloc memory corruption on merge-tree with leading newline","startedAt":"2016-02-15T21:39:39Z","lastAt":"2016-02-19T12:48:01Z","messageCount":10,"participants":["Stefan Frühwirth","Jeff King","Eric Sunshine","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"278208","messageId":"56C2459B.5060805@uni-graz.at","threadId":"41417","inReplyTo":null,"subject":"malloc memory corruption on merge-tree with leading newline","fromName":"Stefan Frühwirth","fromEmail":"stefan.fruehwirth@uni-graz.at","sentAt":"2016-02-15T21:39:39Z","receivedAt":"2016-02-15T21:39:39Z","isPatch":false,"sender":{"key":"stefan.fruehwirth@uni-graz.at","avatar":null},"body":"Hi,\n\nin one specific circumstance, git-merge-tree exits with a segfault \ncaused by \"*** Error in `git': malloc(): memory corruption (fast)\":\n\nThere has to be at least one commit first (as far as I can tell it \ndoesn't matter what content). Then create a tree containing a file with \na leading newline character (\\n) followed by some random string, and \nanother tree with a file containing a string without leading newline. \nNow merge trees: Segmentation fault.\n\nThere is a test case[1] kindly provided by chrisrossi, which he crafted \nafter I discovered the problem[2] in the context of Pylons/acidfs.\n\nBest,\nStefan\n\n[1] https://gist.github.com/chrisrossi/f09c8bed70b364f9f12e\n[2] https://github.com/Pylons/acidfs/issues/3\n\nFor in-line reference, here's the test case:\n\ngit init bug\ncd bug\necho b > a\ngit add a\ngit commit -m \"first commit\"\necho > b\necho -n a >> b\ngit add b\ngit commit -m \"second commit, first branch\"\ngit checkout HEAD~1\ngit checkout -b other\necho -n a > b\ngit add b\ngit commit -m \"second commit, second branch\"\ngit merge-tree HEAD~1 master other\ncd ..\nrm -rf bug\n"},{"id":"278222","messageId":"56C24930.1010606@uni-graz.at","threadId":"41417","inReplyTo":"56C2459B.5060805@uni-graz.at","subject":"Re: malloc memory corruption on merge-tree with leading newline","fromName":"Stefan Frühwirth","fromEmail":"stefan.fruehwirth@uni-graz.at","sentAt":"2016-02-15T21:54:56Z","receivedAt":"2016-02-15T21:54:56Z","isPatch":false,"sender":{"key":"stefan.fruehwirth@uni-graz.at","avatar":null},"body":"Addendum: Problem occurs with version 2.7.1 as well as version 1.9.1.\n\nOn 15/02/16 22:39, Stefan Frühwirth wrote:\n\n> in one specific circumstance, git-merge-tree exits with a segfault\n> caused by \"*** Error in `git': malloc(): memory corruption (fast)\":\n\nStefan\n"},{"id":"278254","messageId":"20160216011258.GA11961@sigill.intra.peff.net","threadId":"41417","inReplyTo":"56C2459B.5060805@uni-graz.at","subject":"[PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-16T01:12:58Z","receivedAt":"2016-02-16T01:12:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 15, 2016 at 10:39:39PM +0100, Stefan Frühwirth wrote:\n\n> in one specific circumstance, git-merge-tree exits with a segfault caused by\n> \"*** Error in `git': malloc(): memory corruption (fast)\":\n> \n> There has to be at least one commit first (as far as I can tell it doesn't\n> matter what content). Then create a tree containing a file with a leading\n> newline character (\\n) followed by some random string, and another tree with\n> a file containing a string without leading newline. Now merge trees:\n> Segmentation fault.\n> \n> There is a test case[1] kindly provided by chrisrossi, which he crafted\n> after I discovered the problem[2] in the context of Pylons/acidfs.\n\nWow, I had to look up what \"git merge-tree\" even is. It looks like a\nproof-of-concept added by 492e075 (Handling large files with GIT,\n2006-02-14) that has somehow hung around forever.\n\nI find some of the merging code there questionable, and I wonder if\npeople are actually using it.  And yet there is this report, and it has\nreceived one or two fixes over the years. So maybe people are.\n\nAnyway, here is an immediate fix for the memory corruption. I'm pretty\nsure the _result_ is still buggy in this case, as explained below. I\nsuspect this weird add/add case should just be a full conflict (like it\nis for the normal merge code), and we should just be using ll_merge()\ndirectly. But I have to admit I have very little desire to think hard on\nthis crufty code. My first preference would be to remove it, but I don't\nwant to hurt people who might actually be using it. But they can do\ntheir own hard-thinking.\n\n-- >8 --\nSubject: merge_blobs: use strbuf instead of manually-sized mmfile_t\n\nThe ancient merge_blobs function (which is used nowhere\nexcept in the equally ancient git-merge-tree, which does\nnot itself seem to be called by any modern git code), tries\nto create a plausible base object for an add/add conflict by\nfinding the common parts of the \"ours\" and \"theirs\" blobs.\nIt does so by calling xdiff with XDIFF_EMIT_COMMON, and\nstores the result in a buffer that is as big as the smaller\nof \"ours\" and \"theirs\".\n\nIn theory, this is right; we cannot have more common content\nthan is in the smaller of the two blobs. But in practice,\nxdiff may give us more: if neither file ends in a newline,\nwe get the \"\\nNo newline at end of file\" marker.\n\nThis is somewhat of a bug in itself (the \"no newline\" string\nbecomes part of the blob output!), but much worse is that we\nmay overflow our output buffer with this string (if the\ncommon content was otherwise close to the size of the\nsmaller blob).\n\nThe minimal fix for the memory corruption is to size the\nbuffer appropriately. We could do so by manually adding in\nan extra 29 bytes for the \"no newline\" string to our buffer\nsize. But that's somewhat fragile. Instead, let's replace\nthe fixed-size output buffer with a strbuf which can grow as\nnecessary.\n\nReported-by: Stefan Frühwirth <stefan.fruehwirth@uni-graz.at>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n merge-blobs.c | 23 ++++++++++++-----------\n 1 file changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/merge-blobs.c b/merge-blobs.c\nindex ddca601..acfd110 100644\n--- a/merge-blobs.c\n+++ b/merge-blobs.c\n@@ -51,19 +51,16 @@ static void *three_way_filemerge(const char *path, mmfile_t *base, mmfile_t *our\n static int common_outf(void *priv_, mmbuffer_t *mb, int nbuf)\n {\n \tint i;\n-\tmmfile_t *dst = priv_;\n+\tstruct strbuf *dst = priv_;\n \n-\tfor (i = 0; i < nbuf; i++) {\n-\t\tmemcpy(dst->ptr + dst->size, mb[i].ptr, mb[i].size);\n-\t\tdst->size += mb[i].size;\n-\t}\n+\tfor (i = 0; i < nbuf; i++)\n+\t\tstrbuf_add(dst, mb[i].ptr, mb[i].size);\n \treturn 0;\n }\n \n static int generate_common_file(mmfile_t *res, mmfile_t *f1, mmfile_t *f2)\n {\n-\tunsigned long size = f1->size < f2->size ? f1->size : f2->size;\n-\tvoid *ptr = xmalloc(size);\n+\tstruct strbuf out = STRBUF_INIT;\n \txpparam_t xpp;\n \txdemitconf_t xecfg;\n \txdemitcb_t ecb;\n@@ -75,11 +72,15 @@ static int generate_common_file(mmfile_t *res, mmfile_t *f1, mmfile_t *f2)\n \txecfg.flags = XDL_EMIT_COMMON;\n \tecb.outf = common_outf;\n \n-\tres->ptr = ptr;\n-\tres->size = 0;\n+\tecb.priv = &out;\n+\tif (xdi_diff(f1, f2, &xpp, &xecfg, &ecb) < 0) {\n+\t\tstrbuf_release(&out);\n+\t\treturn -1;\n+\t}\n \n-\tecb.priv = res;\n-\treturn xdi_diff(f1, f2, &xpp, &xecfg, &ecb);\n+\tres->size = out.len; /* avoid long/size_t pointer mismatch below */\n+\tres->ptr = strbuf_detach(&out, NULL);\n+\treturn 0;\n }\n \n void *merge_blobs(const char *path, struct blob *base, struct blob *our, struct blob *their, unsigned long *size)\n-- \n2.7.1.574.gccd43a9\n"},{"id":"278276","messageId":"20160216050915.GA5765@flurp.local","threadId":"41417","inReplyTo":"20160216011258.GA11961@sigill.intra.peff.net","subject":"Re: [PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-16T05:09:15Z","receivedAt":"2016-02-16T05:09:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 15, 2016 at 08:12:58PM -0500, Jeff King wrote:\n> On Mon, Feb 15, 2016 at 10:39:39PM +0100, Stefan Frühwirth wrote:\n> > in one specific circumstance, git-merge-tree exits with a segfault caused by\n> > \"*** Error in `git': malloc(): memory corruption (fast)\":\n> > \n> > There is a test case[1] kindly provided by chrisrossi, which he crafted\n> > after I discovered the problem[2] in the context of Pylons/acidfs.\n> \n> -- >8 --\n> Subject: merge_blobs: use strbuf instead of manually-sized mmfile_t\n> \n> [...]\n> It does so by calling xdiff with XDIFF_EMIT_COMMON, and\n> stores the result in a buffer that is as big as the smaller\n> of \"ours\" and \"theirs\".\n> \n> In theory, this is right; we cannot have more common content\n> than is in the smaller of the two blobs. But in practice,\n> xdiff may give us more: if neither file ends in a newline,\n> we get the \"\\nNo newline at end of file\" marker.\n> [...]\n> \n> Reported-by: Stefan Frühwirth <stefan.fruehwirth@uni-graz.at>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> diff --git a/merge-blobs.c b/merge-blobs.c\n> @@ -51,19 +51,16 @@ static void *three_way_filemerge(const char *path, mmfile_t *base, mmfile_t *our\n>  static int common_outf(void *priv_, mmbuffer_t *mb, int nbuf)\n>  {\n>  \tint i;\n> -\tmmfile_t *dst = priv_;\n> +\tstruct strbuf *dst = priv_;\n>  \n> -\tfor (i = 0; i < nbuf; i++) {\n> -\t\tmemcpy(dst->ptr + dst->size, mb[i].ptr, mb[i].size);\n> -\t\tdst->size += mb[i].size;\n> -\t}\n> +\tfor (i = 0; i < nbuf; i++)\n> +\t\tstrbuf_add(dst, mb[i].ptr, mb[i].size);\n>  \treturn 0;\n>  }\n>  \n>  static int generate_common_file(mmfile_t *res, mmfile_t *f1, mmfile_t *f2)\n>  {\n> -\tunsigned long size = f1->size < f2->size ? f1->size : f2->size;\n> -\tvoid *ptr = xmalloc(size);\n> +\tstruct strbuf out = STRBUF_INIT;\n>  \txpparam_t xpp;\n>  \txdemitconf_t xecfg;\n>  \txdemitcb_t ecb;\n> @@ -75,11 +72,15 @@ static int generate_common_file(mmfile_t *res, mmfile_t *f1, mmfile_t *f2)\n>  \txecfg.flags = XDL_EMIT_COMMON;\n>  \tecb.outf = common_outf;\n>  \n> -\tres->ptr = ptr;\n> -\tres->size = 0;\n> +\tecb.priv = &out;\n> +\tif (xdi_diff(f1, f2, &xpp, &xecfg, &ecb) < 0) {\n> +\t\tstrbuf_release(&out);\n> +\t\treturn -1;\n> +\t}\n>  \n> -\tecb.priv = res;\n> -\treturn xdi_diff(f1, f2, &xpp, &xecfg, &ecb);\n> +\tres->size = out.len; /* avoid long/size_t pointer mismatch below */\n\nIt took a minute or two for me to realize that \"mismatch below\" was\ntalking about the second argument to strbuf_detach(). I tried\nrewriting the comment to mention the second argument explicitly, but\ncouldn't come up with one sufficiently succinct. Oh well.\n\n> +\tres->ptr = strbuf_detach(&out, NULL);\n> +\treturn 0;\n>  }\n\nMy reviewed-by may not be worth much since this code is new to me\ntoo, but this patch looks \"obviously correct\" to me, so:\n\n    Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>\n\nPerhaps squash in the following test which I adapted from the\nreproduction recipe provided by Chris Rossi[1]?\n\n[1] https://gist.github.com/chrisrossi/f09c8bed70b364f9f12e\n\n\n--- 8< ---\ndiff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\nindex 9015e47..1f2aa74 100755\n--- a/t/t4300-merge-tree.sh\n+++ b/t/t4300-merge-tree.sh\n@@ -352,4 +352,22 @@ test_expect_success 'turn tree to file' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success \"don't underallocate result buffer\" '\n+\ttest_when_finished \"git checkout master\" &&\n+\tgit checkout --orphan some &&\n+\tgit rm -rf . &&\n+\tprintf \"b\\n\" >a &&\n+\tgit add a &&\n+\tgit commit -m \"first commit\" &&\n+\tprintf \"\\na\" >b &&\n+\tgit add b &&\n+\tgit commit -m \"second commit, first branch\" &&\n+\tgit checkout @^ &&\n+\tgit checkout -b other &&\n+\tprintf \"a\" >b &&\n+\tgit add b &&\n+\tgit commit -m \"second commit, second branch\" &&\n+\tgit merge-tree @^ some other\n+'\n+\n test_done\n--- 8< ---\n"},{"id":"278279","messageId":"20160216055043.GB28237@sigill.intra.peff.net","threadId":"41417","inReplyTo":"20160216050915.GA5765@flurp.local","subject":"Re: [PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-16T05:50:43Z","receivedAt":"2016-02-16T05:50:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2016 at 12:09:15AM -0500, Eric Sunshine wrote:\n\n> > -\tecb.priv = res;\n> > -\treturn xdi_diff(f1, f2, &xpp, &xecfg, &ecb);\n> > +\tres->size = out.len; /* avoid long/size_t pointer mismatch below */\n> \n> It took a minute or two for me to realize that \"mismatch below\" was\n> talking about the second argument to strbuf_detach(). I tried\n> rewriting the comment to mention the second argument explicitly, but\n> couldn't come up with one sufficiently succinct. Oh well.\n\nMaybe \"avoid long/size_t mismatch in strbuf_detach\" would be better.\n\n> > +\tres->ptr = strbuf_detach(&out, NULL);\n> > +\treturn 0;\n> >  }\n> \n> My reviewed-by may not be worth much since this code is new to me\n> too, but this patch looks \"obviously correct\" to me, so:\n> \n>     Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>\n> \n> Perhaps squash in the following test which I adapted from the\n> reproduction recipe provided by Chris Rossi[1]?\n> \n> [1] https://gist.github.com/chrisrossi/f09c8bed70b364f9f12e\n\nYeah, maybe. There were two reasons I avoided adding a test.\n\nOne, I secretly hoped that by dragging my feet we could get consensus on\njust ripping out merge-tree entirely. ;)\n\nTwo, I'm not sure what the test output _should_ be. I think this case is\nbuggy. So we can check that we don't corrupt the heap, which is nice,\nbut I don't like adding a test that doesn't test_cmp the expected output\n(and see my earlier comments re: thinking hard).\n\nBut if we are going to keep it, maybe some exercise of the code is\nbetter than none.\n\n-Peff\n"},{"id":"278296","messageId":"56C31291.1010308@uni-graz.at","threadId":"41417","inReplyTo":"20160216055043.GB28237@sigill.intra.peff.net","subject":"Re: [PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t","fromName":"Stefan Frühwirth","fromEmail":"stefan.fruehwirth@uni-graz.at","sentAt":"2016-02-16T12:14:09Z","receivedAt":"2016-02-16T12:14:09Z","isPatch":true,"sender":{"key":"stefan.fruehwirth@uni-graz.at","avatar":null},"body":"Thank you for working on this! Let me just address two things from an \noutsider's perspective:\n\nOn 2016-02-16 at 06:50, Jeff King wrote:\n> Yeah, maybe. There were two reasons I avoided adding a test.\n>\n> One, I secretly hoped that by dragging my feet we could get consensus on\n> just ripping out merge-tree entirely. ;)\n\nSince I cannot comment on the code quality or maturity of merge-tree, \nbut would appreciate if this tool worked as expected, let me quote \nChris' comment[1] on why he uses merge-tree instead of just \"git merge\", \nhoping that it has an impact upon your decision whether to keep the code \nor \"ripping it out\":\n\n\"One might ask, why not use the porcelain 'git merge' command? One\nreason is, in the context of the two phase commit protocol, we'd rather\ndo pretty much everything we possibly can in the voting stage, leaving\nourselves with nothing to do in the finish phase except updating the\nhead to the commit we just created, and possibly updating the working\ndirectory--operations that are guaranteed to work. Since 'git merge'\nwill update the head, we'd prefer to do it during the final phase of the\ncommit, but we can't guarantee it will work. There is not a convenient\nway to do a merge dry run during the voting phase. Although I can\nconceive of ways to do the merge during the voting phase and roll back\nto the previous head if we need to, that feels a little riskier. Doing\nthe merge ourselves, here, also frees us from having to work with a\nworking directory, required by the porcelain 'git merge' command. This\nmeans we can use bare repositories and/or have transactions that use\na head other than the repositories 'current' head.\"\n\n> Two, I'm not sure what the test output _should_ be. I think this case is\n> buggy. So we can check that we don't corrupt the heap, which is nice,\n\nWhat do you mean exactly by \"this case is buggy\"? Doesn't make sense to me.\n\nStefan\n\n[1] https://github.com/Pylons/acidfs/blob/master/acidfs/__init__.py\n"},{"id":"278385","messageId":"20160216203526.GA27484@sigill.intra.peff.net","threadId":"41417","inReplyTo":"56C31291.1010308@uni-graz.at","subject":"Re: [PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-16T20:35:26Z","receivedAt":"2016-02-16T20:35:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2016 at 01:14:09PM +0100, Stefan Frühwirth wrote:\n\n> On 2016-02-16 at 06:50, Jeff King wrote:\n> >Yeah, maybe. There were two reasons I avoided adding a test.\n> >\n> >One, I secretly hoped that by dragging my feet we could get consensus on\n> >just ripping out merge-tree entirely. ;)\n> \n> Since I cannot comment on the code quality or maturity of merge-tree, but\n> would appreciate if this tool worked as expected, let me quote Chris'\n> comment[1] on why he uses merge-tree instead of just \"git merge\", hoping\n> that it has an impact upon your decision whether to keep the code or\n> \"ripping it out\":\n> \n> \"One might ask, why not use the porcelain 'git merge' command? One\n> reason is, in the context of the two phase commit protocol, we'd rather\n> do pretty much everything we possibly can in the voting stage, leaving\n> ourselves with nothing to do in the finish phase except updating the\n> head to the commit we just created, and possibly updating the working\n> directory--operations that are guaranteed to work. Since 'git merge'\n> will update the head, we'd prefer to do it during the final phase of the\n> commit, but we can't guarantee it will work. There is not a convenient\n> way to do a merge dry run during the voting phase. Although I can\n> conceive of ways to do the merge during the voting phase and roll back\n> to the previous head if we need to, that feels a little riskier. Doing\n> the merge ourselves, here, also frees us from having to work with a\n> working directory, required by the porcelain 'git merge' command. This\n> means we can use bare repositories and/or have transactions that use\n> a head other than the repositories 'current' head.\"\n\nYeah, I agree there isn't a great solution in git here. Using \"git\nmerge\" is definitely wrong if you don't want to touch HEAD or have a\nworking directory.  If you _just_ care about doing the tree-level merge\nwithout content-level merging inside blobs, you can do it in the index\nlike:\n\n  export GIT_INDEX_FILE=temp.index\n  base=$(git merge-base $ours $theirs)\n  git read-tree -i -m --aggressive $base $ours $theirs\n\nIf you want to do content-level resolving on top of that, you can do it\nwith:\n\n  git merge-index git-merge-one-file -a\n\nthough it will need a temp directory to write out conflicts and\nresolved content.\n\nThat was what powered GitHub's \"merge\" button for many years, though we\nrecently switched to using libgit2's merge (mostly because the\nmerge-one-file bit can be slow; we didn't care about seeing the\nconflicts, and as a shell script it runs O(# of merged files)\nprocesses).\n\nI don't think merge-tree is at all the wrong tool, in the sense that it\nis being used as designed. But it is using merge code that is different\nfrom literally the rest of git. That means you're going to run into\nweird bugs (like this one), and places where it does not behave the\nsame.  This add/add case, for instance, is usually a conflict in a\nnormal git merge (try your test case with \"git merge\"), but merge-tree\ntries to do a partial content merge with a base that never actually\nexisted[1].\n\nSo I worry that merge-tree's existence is a disservice to people like\nChris, because there is no disclaimer that it is effectively\nunmaintained.\n\n> >Two, I'm not sure what the test output _should_ be. I think this case is\n> >buggy. So we can check that we don't corrupt the heap, which is nice,\n> \n> What do you mean exactly by \"this case is buggy\"? Doesn't make sense to me.\n\nActually, I'm not sure now.  Look at the output of git-merge-tree on\nyour test case, once my patch is applied[2]:\n\n  $ git merge-tree HEAD~1 master other\n  added in both\n    our    100644 9fe188c32cdde9723a119c69548e3768882827d1 b\n    their  100644 2e65efe2a145dda7ee51d1741299f848e5bf752e b\n  @@ -1,2 +1,5 @@\n  +<<<<<<< .our\n   \n  +=======\n  +>>>>>>> .their\n   a\n  \\ No newline at end of file\n\nWe blindly stick the \"No newline at end of file\" marker into the base\nfile content (which is what caused the overflow in the first place). So\nour three-way merge is proceeding from a bogus state that has that extra\nmarker in it.  I _thought_ that was what I was seeing in the output\nabove.\n\nBut I think in practice it magically works, because it looks like both\nsides remove that (so the correct merge resolution is to drop it, as\nwell). And the output above is because it shows the file content as a\ndiff against the \"ours\" version (you can tell from the hunk header that\nthe \"No newline\" marker is not part of the diff content).\n\nSo merge-blobs.c:generate_common_file() is definitely buggy, but I think\nthe bug gets unintentionally canceled out by the follow-on three-way\nmerge. Which is...good, I guess?\n\n-Peff\n\n[1] So one obvious alternative to my patch (besides killing off\n    merge-tree entirely) would be to rip out the weird add/add handling.\n    That would bring merge-tree in line with the rest of git, and would\n    eliminate the buffer-overflowing code entirely.\n\n[2] You can trigger it without my patch, too, but you need input files\n    which differ by more than 29 bytes.\n"},{"id":"278391","messageId":"xmqqmvr0mjmc.fsf@gitster.mtv.corp.google.com","threadId":"41417","inReplyTo":"20160216055043.GB28237@sigill.intra.peff.net","subject":"Re: [PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-16T21:27:07Z","receivedAt":"2016-02-16T21:27:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Yeah, maybe. There were two reasons I avoided adding a test.\n>\n> One, I secretly hoped that by dragging my feet we could get consensus on\n> just ripping out merge-tree entirely. ;)\n>\n> Two, I'm not sure what the test output _should_ be. I think this case is\n> buggy. So we can check that we don't corrupt the heap, which is nice,\n> but I don't like adding a test that doesn't test_cmp the expected output\n> (and see my earlier comments re: thinking hard).\n\nThree, I know the existence of the program is not more than \"we\ncould do something like this\" illustration by Linus, and its output\nis in no way _designed_ to be so.  We know today that it does not do\nadd/add conflict correctly thanks to this thread, but if we are\ngoing to keep this program, we'd need to start from really designing\nwhat its behaviour _should_ be, before casting the desirable\nbehaviour into stone with tests.\n\n> But if we are going to keep it, maybe some exercise of the code is\n> better than none.\n\nSo, \"exercise\" is better than none in the sense that it would\nvalidate that \"the command does something without barfing\", but I\nthink there is a downside to casting its current behaviour as the\nexpected output in stone, if we are going to keep it.\n"},{"id":"278664","messageId":"56C70E05.8020508@uni-graz.at","threadId":"41417","inReplyTo":"20160216203526.GA27484@sigill.intra.peff.net","subject":"Re: [PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t","fromName":"Stefan Frühwirth","fromEmail":"stefan.fruehwirth@uni-graz.at","sentAt":"2016-02-19T12:43:49Z","receivedAt":"2016-02-19T12:43:49Z","isPatch":true,"sender":{"key":"stefan.fruehwirth@uni-graz.at","avatar":null},"body":"On 2016-02-16 at 21:35 Jeff King wrote:\n\n> Yeah, I agree there isn't a great solution in git here. Using \"git\n> merge\" is definitely wrong if you don't want to touch HEAD or have a\n> working directory.  If you _just_ care about doing the tree-level merge\n> without content-level merging inside blobs, you can do it in the index\n> like:\n>\n>    export GIT_INDEX_FILE=temp.index\n>    base=$(git merge-base $ours $theirs)\n>    git read-tree -i -m --aggressive $base $ours $theirs\n>\n> If you want to do content-level resolving on top of that, you can do it\n> with:\n>\n>    git merge-index git-merge-one-file -a\n>\n> though it will need a temp directory to write out conflicts and\n> resolved content.\n\nThat's an interesting alternative, I'll give it a try!\n\n> I don't think merge-tree is at all the wrong tool, in the sense that it\n> is being used as designed. But it is using merge code that is different\n> from literally the rest of git. That means you're going to run into\n> weird bugs (like this one), and places where it does not behave the\n> same.  This add/add case, for instance, is usually a conflict in a\n> normal git merge (try your test case with \"git merge\"), but merge-tree\n> tries to do a partial content merge with a base that never actually\n> existed[1].\n\nThank you for clarifying, I understand.\n\n> So I worry that merge-tree's existence is a disservice to people like\n> Chris, because there is no disclaimer that it is effectively\n> unmaintained.\n\nI agree, I don't want to advocate continuing development under these \nconditions.\n\n> So merge-blobs.c:generate_common_file() is definitely buggy, but I think\n> the bug gets unintentionally canceled out by the follow-on three-way\n> merge. Which is...good, I guess?\n\nWell I don't know how to handle all this with respect to my original \nproblem, but that's completely off topic. Anyway: Thanks!\n\nStefan\n"},{"id":"278665","messageId":"56C70F01.9060609@uni-graz.at","threadId":"41417","inReplyTo":"xmqqmvr0mjmc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t","fromName":"Stefan Frühwirth","fromEmail":"stefan.fruehwirth@uni-graz.at","sentAt":"2016-02-19T12:48:01Z","receivedAt":"2016-02-19T12:48:01Z","isPatch":true,"sender":{"key":"stefan.fruehwirth@uni-graz.at","avatar":null},"body":"On 2016-02-16 at 22:27 Junio C Hamano wrote:\n\n> Three, I know the existence of the program is not more than \"we\n> could do something like this\" illustration by Linus, and its output\n> is in no way _designed_ to be so.  We know today that it does not do\n\nWell, then it is just really sad that the manpage doesn't say so. This \nshould be corrected immediately in order to prevent someone to build \nmore (e.g.) libraries on top of it.\n\nStefan\n"}]}