On Tue, Feb 16, 2016 at 12:09:15AM -0500, Eric Sunshine wrote: > > - 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. > > + 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 <sunsh...@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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
