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

From: Jeff King <peff@peff.net>
Date: Apr 15, 2011, 23:56 UTC
Message-ID: <20110415235628.GA9334@sigill.intra.peff.net>
In-Reply-To: <36a715a966a22207135f60532e723f6d87dd1ffb.1302881295.git.git@drmicha.warpmail.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.

Show 5 quoted lines

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