On 05 Oct 2017, at 05:46, Jeff King [off-list ref] wrote:

On Wed, Oct 04, 2017 at 08:30:05PM +0100, Thomas Gummerer wrote:

quoted

quoted

So I dunno. This approach is a _lot_ more convenient than trying to
rebuild all the dependencies from scratch, and it runs way faster than
valgrind.  It did find the cases that led to the patches in this
series, and at least one more: if the lstat() at the end of
entry.c:write_entry() fails, we write nonsense into the cache_entry.

Yeah valgrind found that one too, as I tried (and apparently failed :))
to explain in the cover letter.  I just haven't found the time yet to
actually try and go fix that one.

No, I just have poor memory. :)

The obvious fix is that we should check the return value of `lstat`, but
the bigger question is why and when that would fail.

The case triggered by t0021 is using the new "delayed" filter mechanism.
So at the time that write_entry() finishes, we don't actually have the
file in the filesystem. I think we need to recognize that we got delayed
and didn't actually check anything out, and skip that whole "if
(state->refresh_cache)" block. It's not clear to me, though, how we tell
the difference between the delayed and normal cases in that function.

Oh. Great catch!

But I think this lstat could also fail if we are checking out and
somebody else racily deletes our file. This is presumably sufficiently
rare that I actually wonder if we should just bail with an error, so
that the user knows something funny is going on.

That sounds good to me!


I tried to address both issues here:
https://public-inbox.org/git/20171005104407.65948-1-lars.schneider@autodesk.com/

Cheers,
Lars