From: René Scharfe <l.s.r@web.de>
Date: Sun, 03 Aug 2014 22:59:51 GMT
Subject: Re: Bug report about symlinks
Message-ID: <53DEBEE7.6070009@web.de>
In-Reply-To: <xmqqk36ptrs6.fsf@gitster.dls.corp.google.com>

Am 03.08.2014 um 19:19 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> How about the patch below?  Before it checks if an index entry exists
>> in the work tree, it checks if its path includes a symlink.
>
> Honestly, I didn't expect the fix to be in the refresh-index code
> path, but doing this there sort of makes sense.

I found it through observation, not understanding.  Just looked for 
stat/lstat calls executed by git status for b/different and b/equal 
using strace; debugging printfs told me where they came from.

>> And do we need to use the threaded_ variant of the function here?
>
> Hmmm, this is a tangent, but you comment made me wonder if we also
> need to adjust preload_thread() in preload-index.c somehow, but we
> do not touch CE_UPTODATE there, so it probably is not necessary.

The function calls ce_mark_uptodate(), which does set CE_UPTODATE.  It 
calls threaded_has_symlink_leading_path() before lstat() already, 
however.  (Since f62ce3de: Make index preloading check the whole path to 
the file.)

> The caller of refresh_cache_ent() is walking an array of sorted
> pathnames aka istate->cache[] in a single-threaded fashion, possibly
> with a pathspec to limit the scan.

There are two direct callers (refresh_index(), refresh_cache_entry()) 
and several indirect ones.  Do we have a way to detect unsynchronized 
parallel access to the has_symlink_leading_path()-cache?  Checking the 
full callers-of-callers tree manually looks a bit scary to me.

> Do you mean that we may want to
> make istate->cache[] scanned by multiple threads?  I am not sure.

No, I didn't want to suggest any performance improvements.  I'm only 
interested in correctness for now.

René