Re: GIT 1.6.0-rc1

Alex Riesen [off-list ref] writes:

Junio C Hamano, Tue, Jul 29, 2008 10:36:19 +0200:

quoted

Junio C Hamano [off-list ref] writes:

quoted

Ok, I took a deeper look at the codepaths involved.  Although it does work
around the issue, I do not think your patch alone is the "correct" one in
the longer term.

Thought so. I just didn't know the code around

quoted

quoted

It needs a bit of explanation, and the explanation won't be exactly
"plain, small and short", unfortunately.

Alex, I ran the full test with this, but only on Linux boxes; obviously
not on any flavor of Windows.  I think it is correct, and the "first line
of defence" fix is the same as your patch, so I'd assume it would work for
you as well.  But extra eyeballs are always appreciated.

Well, it works on Cygwin too. And I had my eyeballs on the code
(wondered first if it will cause more fs accesses than before: it
will, in the racy check. Which is correct, AFAICT)

I thought racy check won't even trigger for gitlinks, no?

ce_modified_check_fs() has 3 call sites:

 - the call site in ie_match_stat() is protected with is_racy_timestamp()
   that is always false for gitlinks;

 - the call site in ie_modified() we just took care of in the current
   thread;

 - the other call site is in ce_smudge_racily_clean_entry(), which is
   called from write_index() but it also is protected with
   is_racy_timestamp() that is always false for gitlinks.

 

Junio C Hamano, Wed, Jul 30, 2008 00:03:44 +0200:

Alex Riesen [off-list ref] writes:

quoted

quoted

Alex, I ran the full test with this, but only on Linux boxes; obviously
not on any flavor of Windows.  I think it is correct, and the "first line
of defence" fix is the same as your patch, so I'd assume it would work for
you as well.  But extra eyeballs are always appreciated.

Well, it works on Cygwin too. And I had my eyeballs on the code
(wondered first if it will cause more fs accesses than before: it
will, in the racy check. Which is correct, AFAICT)

I thought racy check won't even trigger for gitlinks, no?

I didn't know. But now, come to think of it, there wouldn't be much
point - the gitlinks are always compared by content, aren't they?

ce_modified_check_fs() has 3 call sites:

 - the call site in ie_match_stat() is protected with is_racy_timestamp()
   that is always false for gitlinks;

 - the call site in ie_modified() we just took care of in the current
   thread;

 - the other call site is in ce_smudge_racily_clean_entry(), which is
   called from write_index() but it also is protected with
   is_racy_timestamp() that is always false for gitlinks.

So, the change in ce_modified_check_fs is not really needed, because
the gitlink case is never executed?