Re: [PATCH] builtin-commit: Refresh cache after adding files.

["Kristian Høgsberg" <krh@redhat.com>]

On Fri, 2007-11-09 at 10:24 -0800, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Fri, 9 Nov 2007, Kristian Høgsberg wrote:
> >
> >> This fixes the race in the last test int t3700.
> >
> > Well, it is not a race.  My fault.  I thought it was.
> >
> > What you basically did was to make sure that the index is up-to-date after 
> > adding the files.  You might even want to say that in the commit message, 
> > and only then say that it fixes t3700, too.
> 
> Ah, it all makes sense.  I should have been more relaxed and
> careful when I first read your t3700 patch.
> 
> If this were a breakage in racy-git-avoidance code, then
> refresh_cache() Kristian added (or "add --refresh" immediately
> after "git commit" in the test script) would have been fooled by
> the same raciness and wouldn't have changed a thing.
> 
> This discussion exposes a problem with add_files_to_cache()
> function.
> 
> Try this in a clean repository that tracks Makefile:
> 
> 	$ git diff-files --name-only    ;# no output
>         $ touch Makefile
>         $ git diff-files --name-only
> 	Makefile
>         $ git add -u
>         $ git diff-files --name-only
> 	Makefile
>         $ git add Makefile
>         $ git diff-files --name-only    ;# no output
> 
> I think this is a broken behaviour.  As long as we are adding
> the contents from a path on the filesystem to the corresponding
> location in the index, it should sync the stat bits for the
> entry in the index with the filesystem to make the entry
> stat-clean.  IOW, we should not see stat-dirtiness reported
> after "add -u".

Yup, that's what I expected, and why I couldn't quite understand why the
refresh was necessary.

> The funny thing is that add_files_to_cache() run by "git add -u"
> calls add_file_to_cache() to add the updated contents for
> touched paths, but the latter function is used in the more
> explicit "git add Makefile" codepath, without any magic
> postprocessing after the function returns to sync the stat
> info.  IOW, both "add -u" and "add Makefile" ends up calling
> add_file_to_cache("Makefile") and I do not see why we are
> getting different results.

There is some timing involved in this, which may explain the different
results you see.  On my laptop I can trigger the add_files_to_cache()
problem roughly 4 out of 5 times, but on my faster desktop, it can't
trigger it.

> And add_file_to_cache(), which calls add_file_to_index() on
> the_index, does call the fill_stat_cache_info() to sync the stat
> information by itself, as it should be.  I am still puzzled with
> this.
> 
> So I think Kristian's two refresh_cache() do fix the issue, but
> at the same time I _think_ it is a workaround for broken
> add_files_to_cache() behaviour, which is what we should fix.

OK, I'll resend the patch with a better description, and add a refresh
call for the user index too, for the git commit <file> case.

cheers,
Kristian