Re: [PATCH 4/7] textconv: don't convert for every operation

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> -static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
> +static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one,
> +		int try_textconv)
>  {
>  	const char *textconv;
>  
> @@ -304,7 +305,7 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
>  	else if (diff_populate_filespec(one, 0))
>  		return -1;
>  
> -	if ((textconv = get_textconv(one))) {
> +	if (try_textconv && (textconv = get_textconv(one))) {
>  		size_t size;
>  		mf->ptr = run_textconv(textconv, one, &size);
>  		if (!mf->ptr)
> @@ -1396,7 +1397,7 @@ static void builtin_diff(const char *name_a,
>  		}
>  	}

Isn't this function "fill_mmfile()" and its callers leaky as a sieve?

The original fill_mmfile() was to only borrow one->data and users of
mmfile_t never had to worry about freeing what's in mf->ptr (and freeing
them would have been actively wrong).

I am also somewhat worried about the performance impact of running
get_textconv() to the same filespec many times when no textconv is
defined, which is the normal case we should optimize for.  It appears that
diff_filespec_load_driver() is optimized for a wrong case (i.e. "we
already know this needs a custom driver and we know which one").

I am inclined to suggest reverting the whole series (including the ones
that are already in 'master') and start over from scratch, limiting the
run_textconv() to only inside diff.c::builtin_diff() (in the else {} block
where "Crazy xcl interfaces.." comment appears).