Re: [PATCH] checkout: don't write merge results into the object database

From: Jeff King <peff@peff.net>
Date: Jun 15, 2017, 13:57 UTC
Message-ID: <20170615135751.qxn6bsfsxz5es236@sigill.intra.peff.net>
In-Reply-To: <b46827a5-2a8f-c343-ac1f-814fc8a16943@web.de>

On Thu, Jun 15, 2017 at 01:33:42PM +0200, René Scharfe wrote:

> Merge results need to be written to the worktree, of course, but we
> don't necessarily need object entries for them, especially if they
> contain conflict markers.  Use pretend_sha1_file() to create fake
> blobs to pass to make_cache_entry() and checkout_entry() instead.

Conceptually this makes sense, although I have a comment below.

Out of curiosity, did this come up when looking into the cherry-pick segfault/error from a few hours ago?

Show 10 quoted lines

> @@ -225,8 +225,8 @@ static int checkout_merged(int pos, const struct checkout *state)
>  	 * (it also writes the merge result to the object database even
>  	 * when it may contain conflicts).
>  	 */
> -	if (write_sha1_file(result_buf.ptr, result_buf.size,
> -			    blob_type, oid.hash))
> +	if (pretend_sha1_file(result_buf.ptr, result_buf.size,
> +			      OBJ_BLOB, oid.hash))
>  		die(_("Unable to add merge result for '%s'"), path);
>  	free(result_buf.ptr);

I wondered if pretend_sha1_file() makes a copy of the buffer, and indeed it does. So this is correct.

But that raises an interesting question: how big are these objects, and is it a good idea to store them in RAM? Obviously they're in RAM already, but I'm not sure if it's one-at-a-time. We could be bumping the peak RAM used if there's a large number of these entries. Touching the on-disk odb does feel hacky, but it also means they behave like other objects.

If it is a concern, I think it could be solved by "unpretending" after our call to checkout_entry completes. That would need a new call in sha1_file.c, but it should be easy to write.

-Peff