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

From: Jeff King <peff@peff.net>
Date: Feb 16, 2016, 05:50 UTC
Message-ID: <20160216055043.GB28237@sigill.intra.peff.net>
In-Reply-To: <20160216050915.GA5765@flurp.local>

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

Show 8 quoted lines

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

Show 13 quoted lines

> > +	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 <sunshine@sunshineco.com>
> 
> 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