Re: [RFC PATCH v4 00/19] Sparse checkout

[Nguyen Thai Ngoc Duy <pclouds@gmail.com>]

2009/8/21 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> The recent assume-unchanged "breakage" that lets Git overwrite
>> assume-unchanged files worried me. I sat back, checked and wrote tests
>
> Yeah, it worries me, too.  Does the fix to make sure the sparse stuff
> won't be broken apply equally to assume-unchanged?  Does the series have
> such fixes to assume-unchanged bit as well?

This series does not fix assume-unchanged bit. I'd like to focus on
skip-worktree bit now. I still need to write a few more tests for
git-apply, git-checkout... but I think they are safe. It's up to you
to see if changes apply to assume-unchanged bit, in patches 4/19 and
5/19. I don't know if I understand assume-unchanged semantics
correctly anymore :-)

Anyway I think we could put a big fat warning above ce_uptodate()
macro definition, saying that this bit/macro could be faked by
assume-unchanged or skip-worktree bit, so don't rely on that macro
when it comes to writing (at least for skip-worktree part).

Hmm.. _or_ we could make it clear whether it is truly uptodate, or
faked uptodate. Some code path will be updated to only trust "truly
uptodate" bit, which is clearer and easier to grasp than messy logics
like "if (ce_uptodate(ce) && !ce_skip_worktree(ce))". Something like
this as a starting point (for demonstration only, I don't think it
compiles)

diff --git a/cache.h b/cache.h
index a401daf..05fb746 100644
--- a/cache.h
+++ b/cache.h
@@ -179,6 +179,7 @@ struct cache_entry {

 /* Only remove in work directory, not index */
 #define CE_WT_REMOVE (0x400000)
+#define CE_ASSUME_UPTODATE  (0x800000)

 /*
  * Extended on-disk flags
@@ -236,9 +237,11 @@ static inline size_t ce_namelen(const struct
cache_entry *ce)
 			    ondisk_cache_entry_extended_size(ce_namelen(ce)) : \
 			    ondisk_cache_entry_size(ce_namelen(ce)))
 #define ce_stage(ce) ((CE_STAGEMASK & (ce)->ce_flags) >> CE_STAGESHIFT)
-#define ce_uptodate(ce) ((ce)->ce_flags & CE_UPTODATE)
+#define ce_truely_uptodate(ce) ((ce)->ce_flags & CE_UPTODATE)
+#define ce_uptodate(ce) ((ce)->ce_flags & (CE_UPTODATE | CE_ASSUME_UPTODATE))
 #define ce_skip_worktree(ce) ((ce)->ce_flags & CE_SKIP_WORKTREE)
 #define ce_mark_uptodate(ce) ((ce)->ce_flags |= CE_UPTODATE)
+#define ce_mark_assume_uptodate(ce) ((ce)->ce_flags |= CE_ASSUME_UPTODATE)

 #define ce_permissions(mode) (((mode) & 0100) ? 0755 : 0644)
 static inline unsigned int create_ce_mode(unsigned int mode)
diff --git a/read-cache.c b/read-cache.c
index 5ee7d9d..c022955 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1005,7 +1005,7 @@ static struct cache_entry
*refresh_cache_ent(struct index_state *istate,
 		return ce;

 	if (!ignore_valid && ((ce->ce_flags & CE_VALID) || ce_skip_worktree(ce))) {
-		ce_mark_uptodate(ce);
+		ce_mark_assume_uptodate(ce);
 		return ce;
 	}

Still thinking of it. Seems like a good change...

> By the way, I think the first patch in the earlier series, 540e694
> (Prevent diff machinery from examining assume-unchanged entries on
> worktree, 2009-08-11), is a good change regardless of the sparse
> implementation, and I'm inclined to say that we should merge that part
> (and I suspect there will be similar fixes to really ignore differences to
> CE_VALID entries) first and then queue this new series on top.

I have no problem with that.
-- 
Duy