{"thread":{"id":"46187","subject":"[PATCH] checkout: don't write merge results into the object database","startedAt":"2017-06-15T11:34:01Z","lastAt":"2017-06-15T21:56:11Z","messageCount":4,"participants":["René Scharfe","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"322313","messageId":"b46827a5-2a8f-c343-ac1f-814fc8a16943@web.de","threadId":"46187","inReplyTo":null,"subject":"[PATCH] checkout: don't write merge results into the object database","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-06-15T11:33:42Z","receivedAt":"2017-06-15T11:34:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Merge results need to be written to the worktree, of course, but we\ndon't necessarily need object entries for them, especially if they\ncontain conflict markers.  Use pretend_sha1_file() to create fake\nblobs to pass to make_cache_entry() and checkout_entry() instead.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n builtin/checkout.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 1624eed7e7..f51c15af9f 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -215,7 +215,7 @@ static int checkout_merged(int pos, const struct checkout *state)\n \n \t/*\n \t * NEEDSWORK:\n-\t * There is absolutely no reason to write this as a blob object\n+\t * There is absolutely no reason to build a fake blob object\n \t * and create a phony cache entry.  This hack is primarily to get\n \t * to the write_entry() machinery that massages the contents to\n \t * work-tree format and writes out which only allows it for a\n@@ -225,8 +225,8 @@ static int checkout_merged(int pos, const struct checkout *state)\n \t * (it also writes the merge result to the object database even\n \t * when it may contain conflicts).\n \t */\n-\tif (write_sha1_file(result_buf.ptr, result_buf.size,\n-\t\t\t    blob_type, oid.hash))\n+\tif (pretend_sha1_file(result_buf.ptr, result_buf.size,\n+\t\t\t      OBJ_BLOB, oid.hash))\n \t\tdie(_(\"Unable to add merge result for '%s'\"), path);\n \tfree(result_buf.ptr);\n \tce = make_cache_entry(mode, oid.hash, path, 2, 0);\n-- \n2.13.0\n\n"},{"id":"322325","messageId":"20170615135751.qxn6bsfsxz5es236@sigill.intra.peff.net","threadId":"46187","inReplyTo":"b46827a5-2a8f-c343-ac1f-814fc8a16943@web.de","subject":"Re: [PATCH] checkout: don't write merge results into the object database","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-15T13:57:52Z","receivedAt":"2017-06-15T13:58:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 15, 2017 at 01:33:42PM +0200, René Scharfe wrote:\n\n> Merge results need to be written to the worktree, of course, but we\n> don't necessarily need object entries for them, especially if they\n> contain conflict markers.  Use pretend_sha1_file() to create fake\n> blobs to pass to make_cache_entry() and checkout_entry() instead.\n\nConceptually this makes sense, although I have a comment below.\n\nOut of curiosity, did this come up when looking into the cherry-pick\nsegfault/error from a few hours ago?\n\n> @@ -225,8 +225,8 @@ static int checkout_merged(int pos, const struct checkout *state)\n>  \t * (it also writes the merge result to the object database even\n>  \t * when it may contain conflicts).\n>  \t */\n> -\tif (write_sha1_file(result_buf.ptr, result_buf.size,\n> -\t\t\t    blob_type, oid.hash))\n> +\tif (pretend_sha1_file(result_buf.ptr, result_buf.size,\n> +\t\t\t      OBJ_BLOB, oid.hash))\n>  \t\tdie(_(\"Unable to add merge result for '%s'\"), path);\n>  \tfree(result_buf.ptr);\n\nI wondered if pretend_sha1_file() makes a copy of the buffer, and indeed\nit does. So this is correct.\n\nBut that raises an interesting question: how big are these objects, and\nis it a good idea to store them in RAM? Obviously they're in RAM\nalready, but I'm not sure if it's one-at-a-time. We could be bumping the\npeak RAM used if there's a large number of these entries. Touching the\non-disk odb does feel hacky, but it also means they behave like other\nobjects.\n\nIf it is a concern, I think it could be solved by \"unpretending\" after\nour call to checkout_entry completes. That would need a new call in\nsha1_file.c, but it should be easy to write.\n\n-Peff\n"},{"id":"322356","messageId":"f7d38553-484b-ee81-e059-2c737dad2bc4@web.de","threadId":"46187","inReplyTo":"20170615135751.qxn6bsfsxz5es236@sigill.intra.peff.net","subject":"Re: [PATCH] checkout: don't write merge results into the object database","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-06-15T15:48:18Z","receivedAt":"2017-06-15T15:48:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.06.2017 um 15:57 schrieb Jeff King:\n> On Thu, Jun 15, 2017 at 01:33:42PM +0200, René Scharfe wrote:\n> \n>> Merge results need to be written to the worktree, of course, but we\n>> don't necessarily need object entries for them, especially if they\n>> contain conflict markers.  Use pretend_sha1_file() to create fake\n>> blobs to pass to make_cache_entry() and checkout_entry() instead.\n> \n> Conceptually this makes sense, although I have a comment below.\n> \n> Out of curiosity, did this come up when looking into the cherry-pick\n> segfault/error from a few hours ago?\n\nNo, it came up in the discussion of Dscho's memory leak plug series [1].\n\n>> @@ -225,8 +225,8 @@ static int checkout_merged(int pos, const struct checkout *state)\n>>   \t * (it also writes the merge result to the object database even\n>>   \t * when it may contain conflicts).\n>>   \t */\n>> -\tif (write_sha1_file(result_buf.ptr, result_buf.size,\n>> -\t\t\t    blob_type, oid.hash))\n>> +\tif (pretend_sha1_file(result_buf.ptr, result_buf.size,\n>> +\t\t\t      OBJ_BLOB, oid.hash))\n>>   \t\tdie(_(\"Unable to add merge result for '%s'\"), path);\n>>   \tfree(result_buf.ptr);\n> \n> I wondered if pretend_sha1_file() makes a copy of the buffer, and indeed\n> it does. So this is correct.\n> \n> But that raises an interesting question: how big are these objects, and\n> is it a good idea to store them in RAM? Obviously they're in RAM\n> already, but I'm not sure if it's one-at-a-time. We could be bumping the\n> peak RAM used if there's a large number of these entries. Touching the\n> on-disk odb does feel hacky, but it also means they behave like other\n> objects.\n> \n> If it is a concern, I think it could be solved by \"unpretending\" after\n> our call to checkout_entry completes. That would need a new call in\n> sha1_file.c, but it should be easy to write.\n\nGood point; we'd accumulate fake entries that we'll never need again.\nThe patch should clean them up.\n\nAlternatively we could finally address the NEEDSWORK comment and\nprovide a way to checkout a file without faking an index entry..\n\nRené\n\n\n[1] https://public-inbox.org/git/2704e145927c851c4163a68cfdfd5ada48fff21d.1493906085.git.johannes.schindelin@gmx.de/\n"},{"id":"322404","messageId":"xmqqefukzzik.fsf@gitster.mtv.corp.google.com","threadId":"46187","inReplyTo":"f7d38553-484b-ee81-e059-2c737dad2bc4@web.de","subject":"Re: [PATCH] checkout: don't write merge results into the object database","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-15T21:56:03Z","receivedAt":"2017-06-15T21:56:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n>> If it is a concern, I think it could be solved by \"unpretending\" after\n>> our call to checkout_entry completes. That would need a new call in\n>> sha1_file.c, but it should be easy to write.\n>\n> Good point; we'd accumulate fake entries that we'll never need again.\n\nHopefully we are not pretending to have the same object from two\ncallsites; this one may knows the merged one no longer needs to be\nin core, but some other callsite wanted to pretend a blob with the\nsame contents is in the object store, what happens to it?  I do not\nthink we want to refcount ;-)\n\n> The patch should clean them up.\n>\n> Alternatively we could finally address the NEEDSWORK comment and\n> provide a way to checkout a file without faking an index entry..\n\nYeah, we should not need index entry, and we should not need the\nblob object name.\n\n    ... thinks a bit more ...\n\nHaving said that, in the longer term, it may be safe to write an\nactual object out to the object store.  The convert-to-working-tree\nbackends currently work only on raw bytes, but it is not inplausible\nfor some new interfaces to want to pass the object name to the\nbackend and tell it either togive raw (converted) bytes back, or to\nwrite out the bytes directly to the filesystem, bypassing the main\nGit process.  If we \"pretend\" in this process, not just we accumulate\ncruft in-core as Peff points out, we risk giving out an invalid object\nname to such external mechanisms.\n\nI do not think it is too bad to leave a handful of temporary blobs\nthat are written out by \"git checkout -m <other-branch>\" in the\nobject store to be GC'ed.\n\n"}]}