[PATCH 2/2] read-cache.c: fix timestamp comparison

The mtime recorded in the cache entry is not time_t anymore but
of type (unsigned int).  This casts the comparison and also adds
fuzz factor of 1 second.

Signed-off-by: Junio C Hamano <redacted>
---

 * With these two patches the t7501 Heisenbug seems to go away.
   I do not understand why this fuzz factor of 1 second helps,
   but it apparently does.  I do not want to commit this before
   I understand why, but it is past my bedtime.

 read-cache.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 9554896..745c3fe 100644
--- a/read-cache.c
+++ b/read-cache.c

@@ -180,7 +180,7 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 static int is_racy_timestamp(struct index_state *istate, struct cache_entry *ce)
 {
 	return (istate->timestamp &&
-		istate->timestamp <= ce->ce_mtime);
+		((unsigned int) istate->timestamp) <= ce->ce_mtime + 1);
 }
 
 int ie_match_stat(struct index_state *istate,

-- 
1.5.4.rc4.5.g36a1

On Mon, 21 Jan 2008, Junio C Hamano wrote:

The mtime recorded in the cache entry is not time_t anymore but
of type (unsigned int).  This casts the comparison and also adds
fuzz factor of 1 second.

I really don't think this is the right fix.

The real problem seems to be that the whole "racy_timestamp()" thing 
*hides* the bug, and your change actually makes racy_timestamp() just hide 
things even more (by making it trigger all the time).

Adding a "sleep 1" to your test-case (appended) actually makes it fail 
reliably (without your hack - with your "+1" hack I assume you'd need to 
make the sleep longer).

So the problem is that the test-suite actually *hid* the bug by doing 
thigns so fast that the racy code triggered, and that in turn somehow 
fixed things up. 

Will experiment more now that I have a case that reliably fails. The 
commit that causes this literally shouldn't have caused any semantic 
changes at all, so this is rather interesting.

		Linus

---
#!/bin/sh

test_description='reduced 7501'

. ./test-lib.sh

test_expect_success setup '

        >file &&
        git add file &&
        test_tick &&
        git commit -m initial

'

if test -f ../trace.log
then
        mv ../trace.log ../trace.log.old
fi

test_expect_success 'partial commit that involves removal (1)' '

        test_tick &&
        git rm --cached file &&
        mv file elif &&
	sleep 1 &&
        git add elif &&
	sleep 1 &&

        strace -v -o ../trace.log git commit -m "Partial: add elif" elif &&

        git diff-tree --name-status HEAD^ HEAD >current &&
        echo "A	elif" >expected &&
        diff expected current

'

test_done

Hi,

On Mon, 21 Jan 2008, Linus Torvalds wrote:

On Mon, 21 Jan 2008, Junio C Hamano wrote:

quoted

The mtime recorded in the cache entry is not time_t anymore but of 
type (unsigned int).  This casts the comparison and also adds fuzz 
factor of 1 second.

I really don't think this is the right fix.

Junio, have you tried Hannes' fix (using ln instead of cp -p)?  I would 
not be surprised if the same happened on Linux as on Windows, only much 
rarer, since Linux is so darned fast...

Ciao,
Dscho

On Mon, 21 Jan 2008, Linus Torvalds wrote:

Will experiment more now that I have a case that reliably fails. The 
commit that causes this literally shouldn't have caused any semantic 
changes at all, so this is rather interesting.

Intriguing. It's the "ce_mode" -> CE_REMOVE changes that trigger this bug.

The problem goes away with this one-liner.

I haven't figured out *why*, yet, but the reason I checked CE_REMOVE was 
that it was the only part that wasn't just a pure network order change.

I suspect we have some code-path that didn't check for explicit removal, 
but that happened to check for "mode doesn't match", so clearing ce_mode 
just magically triggered it.

Still looking.

		Linus

---

diff --git a/unpack-trees.c b/unpack-trees.c
index ff46fd6..d6fcf60 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -613,6 +613,7 @@ static int deleted_entry(struct cache_entry *ce, struct cache_entry *old,
 	else
 		verify_absent(ce, "removed", o);
 	ce->ce_flags |= CE_REMOVE;
+	ce->ce_mode = 0;
 	add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);
 	invalidate_ce_path(ce);
 	return 1;

On Mon, 21 Jan 2008, Linus Torvalds wrote:

Still looking.

Damn.

The comment that I also  moved says it all.

I'd forgotten about that really ugly special case. It's no longer ugly, 
but missing that part of the ce_mode handling cleanup certainly explains 
the test-suite failing.

		Linus

---
 read-cache.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index f5f9c3d..58a9b95 100644
--- a/read-cache.c
+++ b/read-cache.c

@@ -124,6 +124,9 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 {
 	unsigned int changed = 0;
 
+	if (ce->ce_flags & CE_REMOVE)
+		return MODE_CHANGED | DATA_CHANGED | TYPE_CHANGED;
+
 	switch (ce->ce_mode & S_IFMT) {
 	case S_IFREG:
 		changed |= !S_ISREG(st->st_mode) ? TYPE_CHANGED : 0;

@@ -145,8 +149,6 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 		else if (ce_compare_gitlink(ce))
 			changed |= DATA_CHANGED;
 		return changed;
-	case 0: /* Special case: unmerged file in index */
-		return MODE_CHANGED | DATA_CHANGED | TYPE_CHANGED;
 	default:
 		die("internal error: ce_mode is %o", ce->ce_mode);
 	}

Hi,

On Mon, 21 Jan 2008, Linus Torvalds wrote:

Not that I understand what is _really_ going on, but shouldn't the comment 
be actually moved, not be deleted?

Ciao,
Dscho

On Mon, 21 Jan 2008, Johannes Schindelin wrote:

Not that I understand what is _really_ going on, but shouldn't the comment 
be actually moved, not be deleted?

Well, the thing is, it's not a special case any more, and you can now see 
the code, and say "that's obviously correct".

The whole point of that function is to compare a index entry with the stat 
information, and it does that by validating it in special ways. It used to 
be that the "ce_mode = 0" was a special case. Now it isn't.

A deleted entry can obviously never match an entry that is still on disk 
(regardless of *any* other issues). So now the

	if (ce->ce_flags & CE_REMOVE)
		return MODE_CHANGED | DATA_CHANGED | TYPE_CHANGED;

statement is in no way a special case - rather the reverse, it's a lot 
more obvious than testing for inode ownership changes etc.

So I removed the comment, because it doesn't make sense any more.

Of course, I could have - instead of deleting it - changed it from 
"Special case: .." to something like "A deleted index entry doesn't match 
any on-disk file", but does that actually add any information to the above 
two lines? Sure, we can comment things, but shouldn't we comment the ones 
that are subtle or odd, rather than the obvious ones?

This was why the CE_REMOVE bit was done in the first place: to remove the 
rather special case of ce_mode being zero meaning something special.

			Linus