Re: [PATCH] diff: discard blob data from stat-unmatched pairs

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

Jeff King <peff@peff.net> writes:

> Subject: [PATCH] diff: discard blob data from stat-unmatched pairs
>
> When performing a tree-level diff against the working tree, we may find
> that our index stat information is dirty, so we queue a filepair to be
> examined later. If the actual content hasn't changed, we call this a
> stat-unmatch; the stat information was out of date, but there's no
> actual diff.  Normally diffcore_std() would detect and remove these
> identical filepairs via diffcore_skip_stat_unmatch().  However, when
> "--quiet" is used, we want to stop the diff as soon as we see any
> changes, so we check for stat-unmatches immediately in diff_change().
>
> That check may require us to actually load the file contents into the
> pair of diff_filespecs. If we find that the pair isn't a stat-unmatch,
> then no big deal; we'd likely load the contents later anyway to generate
> a patch, do rename detection, etc, so we want to hold on to it. But if
> it a stat-unmatch, then we have no more use for that data; the whole
> point is that we're going discard the pair. However, we never free the
> allocated diff_filespec data.

Nicely spotted.  So we can discard when quiet is in effect after
this check, which makes sense.

After reading the initial analysis, I wondered if the fix we did in
f34b205f (diff: do not quit early on stat-dirty files, 2014-01-25)
was suboptimal and we should have instead done the "if QUICK, check
if the pair is merely stat-unmatch" in the loop(s) that call
diff_change(), hoping that it may have given us a better control
over the lifetime of the filespecs in each diff_filepair, but I do
not think it would made much difference.

>  	if (options->flags.quick && options->skip_stat_unmatch &&
> -	    !diff_filespec_check_stat_unmatch(options->repo, p))
> +	    !diff_filespec_check_stat_unmatch(options->repo, p)) {
> +		diff_free_filespec_data(p->one);
> +		diff_free_filespec_data(p->two);
>  		return;
> +	}

Thanks.  Will queue (with that s/it/& is/ typofix mentioned
elsewhere).