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

On Tue, Feb 16, 2016 at 12:09:15AM -0500, Eric Sunshine wrote:

quoted

-	ecb.priv = res;
-	return xdi_diff(f1, f2, &xpp, &xecfg, &ecb);
+	res->size = out.len; /* avoid long/size_t pointer mismatch below */

It took a minute or two for me to realize that "mismatch below" was
talking about the second argument to strbuf_detach(). I tried
rewriting the comment to mention the second argument explicitly, but
couldn't come up with one sufficiently succinct. Oh well.

Maybe "avoid long/size_t mismatch in strbuf_detach" would be better.

quoted

+	res->ptr = strbuf_detach(&out, NULL);
+	return 0;
 }

My reviewed-by may not be worth much since this code is new to me
too, but this patch looks "obviously correct" to me, so:

    Reviewed-by: Eric Sunshine [off-list ref]

Perhaps squash in the following test which I adapted from the
reproduction recipe provided by Chris Rossi[1]?

[1] https://gist.github.com/chrisrossi/f09c8bed70b364f9f12e

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. ;)

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,
but I don't like adding a test that doesn't test_cmp the expected output
(and see my earlier comments re: thinking hard).

But if we are going to keep it, maybe some exercise of the code is
better than none.

-Peff