> On 05 Oct 2017, at 05:46, Jeff King <peff@xxxxxxxx> wrote: > > On Wed, Oct 04, 2017 at 08:30:05PM +0100, Thomas Gummerer wrote: > >>> 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@xxxxxxxxxxxx/ Cheers, Lars
