Re: [PATCH] diff: fix lstat() error handling in diff_populate_filespec()

Junio C Hamano [off-list ref] writes:

Andrey Okoshkin [off-list ref] writes:

quoted

I'm not sure why only ENOENT error of lstat() is considered as an
error but passing by other errno values leads to reading of
uninitialized 'struct stat st' variable.  It means that the
populated 'diff_filespec' structure may be incorrectly filled.

Entirely correct.  There is no fundamental reason to try special
casing ENOENT, unless we are clearing the "this is an error" bit
when the errno is ENOENT---but this code does not even do so.  All
errors are errors---we wanted to know the result of lstat() to carry
on, and we couldn't figure out the status.  We do not want to die
immediately (instead we want to show diffs for other paths), so
substituting the result with an empty string is the least bad thing
we can do at this point in the code.

By the way, if the reason why we are hitting this lstat(2) (not the
reason why lstat(2) is failing) is not because !s->oid.valid
(i.e. we are reading the working tree side because that is what is
on one side of the comparison) but because reuse_worktree_file()
told us to (i.e. we actually are trying to fill the filespec with
the blob contents, but found that we have already the same content
in a working tree file and found it more efficient to read from
there, rather than doing the read_sha1_file() on the s->oid.hash),
it probably is better to fall back to the other side of the if/else
when we see an error from this lstat(2) and pretend as if we got
false from reuse_worktree_file().  

That would allow us continue with the correct information we
originally wanted to use; after all, reusing is merely an
optimization.