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

Jeff King [off-list ref] 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).