Re: [PATCHv2] git-mv: Keep moved index entries inact

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2110 bytes --]

Hi,

On Mon, 28 Jul 2008, Johannes Schindelin wrote:

> On Mon, 28 Jul 2008, Johannes Schindelin wrote:
> 
> > On Mon, 28 Jul 2008, SZEDER Gábor wrote:
> > 
> > > there is a race somewhere in these 'git-mv: Keep moved index entries 
> > > inact' changes.
> > > 
> > > The test cases 'git mv should overwrite symlink to a file' or 'git 
> > > mv should overwrite file with a symlink' fail occasionaly.  It's 
> > > quite non-deterministic:  I have run t7001-mv.sh in a loop (see 
> > > below) and one or the other usually fails around 50 runs (but 
> > > sometimes only after 150).  Adding some tracing echos to the tests 
> > > shows that both tests fail when running 'git diff-files' at the end.
> > 
> > To make it more convenient to test: with this patch it fails all the 
> > time:
> 
> Ooops.  Seems like I changed the test 23 to fail, instead of test 24.  
> However, I think it is the same bug: the index is newer by one second, 
> so it seems that the patch for builtin-mv.c did not really keep the data 
> "intact".
> 
> Note that a test case should use test-chmtime to force this scenario, 
> not sleep a second.
> 
> Unfortunately, I already spent my Git time budget for today, so the ball 
> is out of my half for now.

Hah!  I had a few minutes, and this is my analysis:

Just try to "mv" a file, and look at the _ctime_ before and after.  Yes, 
that is right, at least on my system (ext3) it _changes_.

So the test 23 and 24 in t7001-mv.sh are totally bogus.  They purport to 
test that git-mv retains the whole meta-information in the cache and 
therefore the index does not need to be updated.

However, it _does_ need to be updated, exactly because ctime changed.

Only that the test failed to test what it tried to test, instead 
succeeding erroneously, just because the index was racy most of the time 
and got silently updated.

So, this is the analysis.  The fixes will have to be done by somebody 
else, because /me goes running now.

(Possible fixes I envisage: update ctime via stat() after rename(), or 
just give up and scrap the whole "leave cache_entry inact" thing.)

Ciao,
Dscho