Re: [PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t

From: "Stefan Frühwirth" <stefan.fruehwirth@uni-graz.at>
To: Jeff King <peff@peff.net>, Eric Sunshine <sunshine@sunshineco.com>
Cc: <git@vger.kernel.org>
Subject: Re: [PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t
Date: Tue, 16 Feb 2016 13:14:09 +0100	[thread overview]
Message-ID: <56C31291.1010308@uni-graz.at> (raw)
In-Reply-To: <20160216055043.GB28237@sigill.intra.peff.net>

Thank you for working on this! Let me just address two things from an 
outsider's perspective:

On 2016-02-16 at 06:50, Jeff King wrote:
> Yeah, maybe. There were two reasons I avoided adding a test.
>
> One, I secretly hoped that by dragging my feet we could get consensus on
> just ripping out merge-tree entirely. ;)

Since I cannot comment on the code quality or maturity of merge-tree, 
but would appreciate if this tool worked as expected, let me quote 
Chris' comment[1] on why he uses merge-tree instead of just "git merge", 
hoping that it has an impact upon your decision whether to keep the code 
or "ripping it out":

"One might ask, why not use the porcelain 'git merge' command? One
reason is, in the context of the two phase commit protocol, we'd rather
do pretty much everything we possibly can in the voting stage, leaving
ourselves with nothing to do in the finish phase except updating the
head to the commit we just created, and possibly updating the working
directory--operations that are guaranteed to work. Since 'git merge'
will update the head, we'd prefer to do it during the final phase of the
commit, but we can't guarantee it will work. There is not a convenient
way to do a merge dry run during the voting phase. Although I can
conceive of ways to do the merge during the voting phase and roll back
to the previous head if we need to, that feels a little riskier. Doing
the merge ourselves, here, also frees us from having to work with a
working directory, required by the porcelain 'git merge' command. This
means we can use bare repositories and/or have transactions that use
a head other than the repositories 'current' head."

> Two, I'm not sure what the test output _should_ be. I think this case is
> buggy. So we can check that we don't corrupt the heap, which is nice,

What do you mean exactly by "this case is buggy"? Doesn't make sense to me.

Stefan

[1] https://github.com/Pylons/acidfs/blob/master/acidfs/__init__.py