Re: [PATCH] combine-diff: use textconv for combined diff format

[Jeff King <peff@peff.net>]

On Fri, Apr 15, 2011 at 05:29:05PM +0200, Michael J Gruber wrote:

> Currently, we ignore textconv and binary status for the combined diff
> formats (-c, -cc) which was never intended.

Thanks for working on this.

I think it would be simpler to work on the binary half first. Then it
would be clear where the binary codepath diverges, and sticking the
textconv helpers in there would be easier (the helpers were, after all,
written because it was retrofitting existing diff code that already
handled binaries differently).

The whole grab_blob() thing seems like an unnecessary duplication of the
diff_filespec code. I think if we can switch to a more uniform use of
diff_filespec code, the memory management might end up simpler.

> +	if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) && elem->textconv) {
> +		struct diff_filespec *df = alloc_filespec(elem->path);
> +		fill_filespec(df, elem->sha1, elem->mode);
> +		result_size = fill_textconv(elem->textconv, df, &result);
> +	}

The memory management with fill_textconv is kind of ugly. Sometimes it
returns memory which must be freed, and sometimes not. Looking at the
diff.c code, I think in this case it will always need freed (because
elem->textconv is non-NULL). Sorry, that was a mess I created a long
time ago that you now get to deal with. :)

-Peff