[PATCH] index: be careful when handling long names

[PATCH] index: be careful when handling long names

[Junio C Hamano]

We currently use lower 12-bit (masked with CE_NAMEMASK) in the
ce_flags field to store the length of the name in cache_entry,
without checking the length parameter given to
create_ce_flags().  This can make us store incorrect length.

Currently we are mostly protected by the fact that many
codepaths first copy the path in a variable of size PATH_MAX,
which typically is 4096 that happens to match the limit, but
that feels like a bug waiting to happen.  Besides, that would
not allow us to shorten the width of CE_NAMEMASK to use the bits
for new flags.

This redefines the meaning of the name length stored in the
cache_entry.  A name that does not fit is represented by storing
CE_NAMEMASK in the field, and the actual length needs to be
computed by actually counting the bytes in the name[] field.
This way, only the unusually long paths need to suffer.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 Junio C Hamano <gitster@pobox.com> writes:

 > About the CE_NAMEMASK limitation (and currently we do not check
 > it, so I think we would be screwed when a pathname that is
 > longer than (CE_NAMEMASK+1) and still fits under PATH_MAX is
 > given), I think we do not have to limit the maximum pathname
 > length.  Instead we can teach create_ce_flags() and ce_namelen()
 > that a name longer than 2k (or 4k) has the NAMEMASK bits that
 > are all 1 and ce->name[] must be counted if so (with an obvious
 > optimization to start counting at byte position 2k or 4k in
 > ce_namelen()).

 This should fix it.  Passes tests including the new test that
 the existing code fails.

 cache.h          |   17 +++++++++++++++--
 t/t0000-basic.sh |   18 ++++++++++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index 39331c2..ad53acc 100644
--- a/cache.h
+++ b/cache.h
@@ -114,8 +114,21 @@ struct cache_entry {
 #define CE_VALID     (0x8000)
 #define CE_STAGESHIFT 12
 
-#define create_ce_flags(len, stage) htons((len) | ((stage) << CE_STAGESHIFT))
-#define ce_namelen(ce) (CE_NAMEMASK & ntohs((ce)->ce_flags))
+static inline unsigned create_ce_flags(size_t len, unsigned stage)
+{
+	if (len >= CE_NAMEMASK)
+		len = CE_NAMEMASK;
+	return htons(len | (stage << CE_STAGESHIFT));
+}
+
+static inline size_t ce_namelen(const struct cache_entry *ce)
+{
+	size_t len = ntohs((ce)->ce_flags) & CE_NAMEMASK;
+	if (len < CE_NAMEMASK) /* likely */
+		return len;
+	return strlen(ce->name + CE_NAMEMASK) + CE_NAMEMASK;
+}
+
 #define ce_size(ce) cache_entry_size(ce_namelen(ce))
 #define ce_stage(ce) ((CE_STAGEMASK & ntohs((ce)->ce_flags)) >> CE_STAGESHIFT)
 
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 4e49d59..40551a3 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -297,4 +297,22 @@ test_expect_success 'absolute path works as expected' '
 	test "$sym" = "$(test-absolute-path $dir2/syml)"
 '
 
+test_expect_success 'very long name in the index handled sanely' '
+
+	a=a && # 1
+	a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 16
+	a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 256
+	a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 4096
+	a=${a}q
+
+	>path4 &&
+	git update-index --add path4 &&
+	(
+		git ls-files -s path4 |
+		sed -e "s/	.*/	/" |
+		tr -d "\012"
+		echo "$a"
+	) | git update-index --index-info
+'
+
 test_done

Re: [PATCH] index: be careful when handling long names

[Alex Riesen]

Junio C Hamano, Sun, Jan 13, 2008 23:36:34 +0100:
> +test_expect_success 'very long name in the index handled sanely' '
> +
> +	a=a && # 1
> +	a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 16
> +	a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 256
> +	a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 4096

I'd expect it to fail on some systems (everywindowsthing up to w2k,
maybe some commercial unices).

Re: [PATCH] index: be careful when handling long names

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Junio C Hamano, Sun, Jan 13, 2008 23:36:34 +0100:
>> +test_expect_success 'very long name in the index handled sanely' '
>> +
>> +	a=a && # 1
>> +	a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 16
>> +	a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 256
>> +	a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 4096
>
> I'd expect it to fail on some systems (everywindowsthing up to w2k,
> maybe some commercial unices).

My understanding is that Everywindowsthing do not come with any
(POSIX compliant) shell that we support by default, so if you
are talking about a limit of shell variable value, I do not
think it is an issue to begin with.  It is just the matter of
picking a sensible shell (I understand both Cygwin and msys
ports use a shell that supports more than 4k bytes in value
given to a variable).

I would agree that it might overflow the argument limit when
this is given to "echo", though.  We cannot do much about it,
but you may have cleverer ideas.

Re: [PATCH] index: be careful when handling long names

[Alex Riesen]

Junio C Hamano, Mon, Jan 14, 2008 00:08:07 +0100:
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
> > Junio C Hamano, Sun, Jan 13, 2008 23:36:34 +0100:
> >> +test_expect_success 'very long name in the index handled sanely' '
> >> +
> >> +	a=a && # 1
> >> +	a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 16
> >> +	a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 256
> >> +	a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 4096
> >
> > I'd expect it to fail on some systems (everywindowsthing up to w2k,
> > maybe some commercial unices).
> 
> My understanding is that Everywindowsthing do not come with any
> (POSIX compliant) shell that we support by default, so if you
> are talking about a limit of shell variable value, I do not
> think it is an issue to begin with.  It is just the matter of

Oh, right. The file system wont even see it, it is passed directly to
update-index.

> picking a sensible shell (I understand both Cygwin and msys
> ports use a shell that supports more than 4k bytes in value
> given to a variable).

can't check right now, but I believe it is so

> I would agree that it might overflow the argument limit when
> this is given to "echo", though.  We cannot do much about it,
> but you may have cleverer ideas.

I thought about conditionally disabling the test, like it was done
when the tabs in filenames had to be tested. Wont be needed for this
particular case.

Re: [PATCH] index: be careful when handling long names

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Junio C Hamano, Mon, Jan 14, 2008 00:08:07 +0100:
> ...
>> I would agree that it might overflow the argument limit when
>> this is given to "echo", though.  We cannot do much about it,
>> but you may have cleverer ideas.
>
> I thought about conditionally disabling the test, like it was done
> when the tabs in filenames had to be tested.

Yes, we can and should do that when somebody reports this (or
any other tests) a real issue, as we have done for other tests.

Updated in-memory index cleanup

[Linus Torvalds]

This is a slightly updated version of the patch I sent out a few days ago, 
which make the index use separate in-memory and on-disk representations 
(with the in-memory format being host-endian and having an extended flags 
field).

Not only has the patch been updated for current 'master', it also now 
handles the "remove entry" condition with a ce_flags value (CE_REMOVE) to 
mirror our old use of "update entry" (CE_UPDATE).

It also makes both CE_REMOVE and CE_UPDATE be in the "in-memory only" part 
of ce_flags, so that they don't use up precios bits from the on-disk 
format.

I have been using some version of this patch since I sent it out 
originally, so it has actually gotten some testing. The CE_REMOVE parts 
are new, but are pretty obvious cleanups and not subtle. It passes the 
test-suite and yadda-yadda.

The patch isn't trivial, but the bulk of it is (still) of the obvious 
cleanup kind, ie things like the removal of ntohl/htonl (since it's now 
done by the disk-format conversion at read/write time):

	-	if (S_ISGITLINK(ntohl(ce->ce_mode))) {
	+	if (S_ISGITLINK(ce->ce_mode)) {

and using CE_REMOVE instead of setting mode and stage to 0:

	-			ce->ce_mode = 0;
	-			ce->ce_flags &= ~htons(CE_STAGEMASK);
	+			ce->ce_flags |= CE_REMOVE;

with the fact that there are about 45 more lines added than removed coming 
mainly from the new definition of the on-disk/in-memory structures and the 
conversion routines to convert from one to the other.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 builtin-apply.c        |   10 +-
 builtin-blame.c        |    2 +-
 builtin-commit.c       |    2 +-
 builtin-fsck.c         |    2 +-
 builtin-grep.c         |    4 +-
 builtin-ls-files.c     |   10 +-
 builtin-read-tree.c    |    3 +-
 builtin-rerere.c       |    4 +-
 builtin-update-index.c |   18 ++--
 cache-tree.c           |    4 +-
 cache.h                |   46 ++++++++----
 diff-lib.c             |   32 ++++-----
 dir.c                  |    2 +-
 entry.c                |    6 +-
 merge-index.c          |    2 +-
 merge-recursive.c      |    2 +-
 reachable.c            |    2 +-
 read-cache.c           |  195 ++++++++++++++++++++++++++++--------------------
 sha1_name.c            |    2 +-
 tree.c                 |    4 +-
 unpack-trees.c         |   29 ++++----
 21 files changed, 214 insertions(+), 167 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 15432b6..30d86f2 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1946,7 +1946,7 @@ static int read_file_or_gitlink(struct cache_entry *ce, struct strbuf *buf)
 	if (!ce)
 		return 0;
 
-	if (S_ISGITLINK(ntohl(ce->ce_mode))) {
+	if (S_ISGITLINK(ce->ce_mode)) {
 		strbuf_grow(buf, 100);
 		strbuf_addf(buf, "Subproject commit %s\n", sha1_to_hex(ce->sha1));
 	} else {
@@ -2023,7 +2023,7 @@ static int check_to_create_blob(const char *new_name, int ok_if_exists)
 
 static int verify_index_match(struct cache_entry *ce, struct stat *st)
 {
-	if (S_ISGITLINK(ntohl(ce->ce_mode))) {
+	if (S_ISGITLINK(ce->ce_mode)) {
 		if (!S_ISDIR(st->st_mode))
 			return -1;
 		return 0;
@@ -2082,12 +2082,12 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)
 				return error("%s: does not match index",
 					     old_name);
 			if (cached)
-				st_mode = ntohl(ce->ce_mode);
+				st_mode = ce->ce_mode;
 		} else if (stat_ret < 0)
 			return error("%s: %s", old_name, strerror(errno));
 
 		if (!cached)
-			st_mode = ntohl(ce_mode_from_stat(ce, st.st_mode));
+			st_mode = ce_mode_from_stat(ce, st.st_mode);
 
 		if (patch->is_new < 0)
 			patch->is_new = 0;
@@ -2388,7 +2388,7 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned
 	ce = xcalloc(1, ce_size);
 	memcpy(ce->name, path, namelen);
 	ce->ce_mode = create_ce_mode(mode);
-	ce->ce_flags = htons(namelen);
+	ce->ce_flags = namelen;
 	if (S_ISGITLINK(mode)) {
 		const char *s = buf;
 
diff --git a/builtin-blame.c b/builtin-blame.c
index 9b4c02e..c7e6887 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -2092,7 +2092,7 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con
 	if (!mode) {
 		int pos = cache_name_pos(path, len);
 		if (0 <= pos)
-			mode = ntohl(active_cache[pos]->ce_mode);
+			mode = active_cache[pos]->ce_mode;
 		else
 			/* Let's not bother reading from HEAD tree */
 			mode = S_IFREG | 0644;
diff --git a/builtin-commit.c b/builtin-commit.c
index 0227936..c63ff82 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -156,7 +156,7 @@ static int list_paths(struct path_list *list, const char *with_tree,
 
 	for (i = 0; i < active_nr; i++) {
 		struct cache_entry *ce = active_cache[i];
-		if (ce->ce_flags & htons(CE_UPDATE))
+		if (ce->ce_flags & CE_UPDATE)
 			continue;
 		if (!pathspec_match(pattern, m, ce->name, 0))
 			continue;
diff --git a/builtin-fsck.c b/builtin-fsck.c
index 2a6e94d..6fc9525 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -765,7 +765,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 			struct blob *blob;
 			struct object *obj;
 
-			mode = ntohl(active_cache[i]->ce_mode);
+			mode = active_cache[i]->ce_mode;
 			if (S_ISGITLINK(mode))
 				continue;
 			blob = lookup_blob(active_cache[i]->sha1);
diff --git a/builtin-grep.c b/builtin-grep.c
index 0d6cc73..9180b39 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -331,7 +331,7 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)
 		struct cache_entry *ce = active_cache[i];
 		char *name;
 		int kept;
-		if (!S_ISREG(ntohl(ce->ce_mode)))
+		if (!S_ISREG(ce->ce_mode))
 			continue;
 		if (!pathspec_matches(paths, ce->name))
 			continue;
@@ -387,7 +387,7 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
 
 	for (nr = 0; nr < active_nr; nr++) {
 		struct cache_entry *ce = active_cache[nr];
-		if (!S_ISREG(ntohl(ce->ce_mode)))
+		if (!S_ISREG(ce->ce_mode))
 			continue;
 		if (!pathspec_matches(paths, ce->name))
 			continue;
diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index 0f0ab2d..d56e33e 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -189,7 +189,7 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)
 		return;
 
 	if (tag && *tag && show_valid_bit &&
-	    (ce->ce_flags & htons(CE_VALID))) {
+	    (ce->ce_flags & CE_VALID)) {
 		static char alttag[4];
 		memcpy(alttag, tag, 3);
 		if (isalpha(tag[0]))
@@ -210,7 +210,7 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)
 	} else {
 		printf("%s%06o %s %d\t",
 		       tag,
-		       ntohl(ce->ce_mode),
+		       ce->ce_mode,
 		       abbrev ? find_unique_abbrev(ce->sha1,abbrev)
 				: sha1_to_hex(ce->sha1),
 		       ce_stage(ce));
@@ -242,7 +242,7 @@ static void show_files(struct dir_struct *dir, const char *prefix)
 				continue;
 			if (show_unmerged && !ce_stage(ce))
 				continue;
-			if (ce->ce_flags & htons(CE_UPDATE))
+			if (ce->ce_flags & CE_UPDATE)
 				continue;
 			show_ce_entry(ce_stage(ce) ? tag_unmerged : tag_cached, ce);
 		}
@@ -350,7 +350,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
 		struct cache_entry *ce = active_cache[i];
 		if (!ce_stage(ce))
 			continue;
-		ce->ce_flags |= htons(CE_STAGEMASK);
+		ce->ce_flags |= CE_STAGEMASK;
 	}
 
 	if (prefix) {
@@ -379,7 +379,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
 			 */
 			if (last_stage0 &&
 			    !strcmp(last_stage0->name, ce->name))
-				ce->ce_flags |= htons(CE_UPDATE);
+				ce->ce_flags |= CE_UPDATE;
 		}
 	}
 }
diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index c0ea034..5785401 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -45,8 +45,7 @@ static int read_cache_unmerged(void)
 				continue;
 			cache_tree_invalidate_path(active_cache_tree, ce->name);
 			last = ce;
-			ce->ce_mode = 0;
-			ce->ce_flags &= ~htons(CE_STAGEMASK);
+			ce->ce_flags |= CE_REMOVE;
 		}
 		*dst++ = ce;
 	}
diff --git a/builtin-rerere.c b/builtin-rerere.c
index a9e3ebc..b0c17bd 100644
--- a/builtin-rerere.c
+++ b/builtin-rerere.c
@@ -149,8 +149,8 @@ static int find_conflict(struct path_list *conflict)
 		if (ce_stage(e2) == 2 &&
 		    ce_stage(e3) == 3 &&
 		    ce_same_name(e2, e3) &&
-		    S_ISREG(ntohl(e2->ce_mode)) &&
-		    S_ISREG(ntohl(e3->ce_mode))) {
+		    S_ISREG(e2->ce_mode) &&
+		    S_ISREG(e3->ce_mode)) {
 			path_list_insert((const char *)e2->name, conflict);
 			i++; /* skip over both #2 and #3 */
 		}
diff --git a/builtin-update-index.c b/builtin-update-index.c
index c3a14c7..a8795d3 100644
--- a/builtin-update-index.c
+++ b/builtin-update-index.c
@@ -47,10 +47,10 @@ static int mark_valid(const char *path)
 	if (0 <= pos) {
 		switch (mark_valid_only) {
 		case MARK_VALID:
-			active_cache[pos]->ce_flags |= htons(CE_VALID);
+			active_cache[pos]->ce_flags |= CE_VALID;
 			break;
 		case UNMARK_VALID:
-			active_cache[pos]->ce_flags &= ~htons(CE_VALID);
+			active_cache[pos]->ce_flags &= ~CE_VALID;
 			break;
 		}
 		cache_tree_invalidate_path(active_cache_tree, path);
@@ -95,7 +95,7 @@ static int add_one_path(struct cache_entry *old, const char *path, int len, stru
 	size = cache_entry_size(len);
 	ce = xcalloc(1, size);
 	memcpy(ce->name, path, len);
-	ce->ce_flags = htons(len);
+	ce->ce_flags = len;
 	fill_stat_cache_info(ce, st);
 	ce->ce_mode = ce_mode_from_stat(old, st->st_mode);
 
@@ -139,7 +139,7 @@ static int process_directory(const char *path, int len, struct stat *st)
 	/* Exact match: file or existing gitlink */
 	if (pos >= 0) {
 		struct cache_entry *ce = active_cache[pos];
-		if (S_ISGITLINK(ntohl(ce->ce_mode))) {
+		if (S_ISGITLINK(ce->ce_mode)) {
 
 			/* Do nothing to the index if there is no HEAD! */
 			if (resolve_gitlink_ref(path, "HEAD", sha1) < 0)
@@ -183,7 +183,7 @@ static int process_file(const char *path, int len, struct stat *st)
 	int pos = cache_name_pos(path, len);
 	struct cache_entry *ce = pos < 0 ? NULL : active_cache[pos];
 
-	if (ce && S_ISGITLINK(ntohl(ce->ce_mode)))
+	if (ce && S_ISGITLINK(ce->ce_mode))
 		return error("%s is already a gitlink, not replacing", path);
 
 	return add_one_path(ce, path, len, st);
@@ -226,7 +226,7 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
 	ce->ce_flags = create_ce_flags(len, stage);
 	ce->ce_mode = create_ce_mode(mode);
 	if (assume_unchanged)
-		ce->ce_flags |= htons(CE_VALID);
+		ce->ce_flags |= CE_VALID;
 	option = allow_add ? ADD_CACHE_OK_TO_ADD : 0;
 	option |= allow_replace ? ADD_CACHE_OK_TO_REPLACE : 0;
 	if (add_cache_entry(ce, option))
@@ -246,14 +246,14 @@ static void chmod_path(int flip, const char *path)
 	if (pos < 0)
 		goto fail;
 	ce = active_cache[pos];
-	mode = ntohl(ce->ce_mode);
+	mode = ce->ce_mode;
 	if (!S_ISREG(mode))
 		goto fail;
 	switch (flip) {
 	case '+':
-		ce->ce_mode |= htonl(0111); break;
+		ce->ce_mode |= 0111; break;
 	case '-':
-		ce->ce_mode &= htonl(~0111); break;
+		ce->ce_mode &= ~0111; break;
 	default:
 		goto fail;
 	}
diff --git a/cache-tree.c b/cache-tree.c
index 50b3526..bfc95d2 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -320,13 +320,13 @@ static int update_one(struct cache_tree *it,
 		}
 		else {
 			sha1 = ce->sha1;
-			mode = ntohl(ce->ce_mode);
+			mode = ce->ce_mode;
 			entlen = pathlen - baselen;
 		}
 		if (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1))
 			return error("invalid object %s", sha1_to_hex(sha1));
 
-		if (!ce->ce_mode)
+		if (ce->ce_flags & CE_REMOVE)
 			continue; /* entry being removed */
 
 		strbuf_grow(&buffer, entlen + 100);
diff --git a/cache.h b/cache.h
index 549f4bb..4a054c5 100644
--- a/cache.h
+++ b/cache.h
@@ -94,48 +94,66 @@ struct cache_time {
  * We save the fields in big-endian order to allow using the
  * index file over NFS transparently.
  */
+struct ondisk_cache_entry {
+	struct cache_time ctime;
+	struct cache_time mtime;
+	unsigned int dev;
+	unsigned int ino;
+	unsigned int mode;
+	unsigned int uid;
+	unsigned int gid;
+	unsigned int size;
+	unsigned char sha1[20];
+	unsigned short flags;
+	char name[FLEX_ARRAY]; /* more */
+};
+
 struct cache_entry {
-	struct cache_time ce_ctime;
-	struct cache_time ce_mtime;
+	unsigned int ce_ctime;
+	unsigned int ce_mtime;
 	unsigned int ce_dev;
 	unsigned int ce_ino;
 	unsigned int ce_mode;
 	unsigned int ce_uid;
 	unsigned int ce_gid;
 	unsigned int ce_size;
+	unsigned int ce_flags;
 	unsigned char sha1[20];
-	unsigned short ce_flags;
 	char name[FLEX_ARRAY]; /* more */
 };
 
 #define CE_NAMEMASK  (0x0fff)
 #define CE_STAGEMASK (0x3000)
-#define CE_UPDATE    (0x4000)
 #define CE_VALID     (0x8000)
 #define CE_STAGESHIFT 12
 
-#define create_ce_flags(len, stage) htons((len) | ((stage) << CE_STAGESHIFT))
-#define ce_namelen(ce) (CE_NAMEMASK & ntohs((ce)->ce_flags))
+/* In-memory only */
+#define CE_UPDATE    (0x10000)
+#define CE_REMOVE    (0x20000)
+
+#define create_ce_flags(len, stage) ((len) | ((stage) << CE_STAGESHIFT))
+#define ce_namelen(ce) (CE_NAMEMASK & (ce)->ce_flags)
 #define ce_size(ce) cache_entry_size(ce_namelen(ce))
-#define ce_stage(ce) ((CE_STAGEMASK & ntohs((ce)->ce_flags)) >> CE_STAGESHIFT)
+#define ondisk_ce_size(ce) ondisk_cache_entry_size(ce_namelen(ce))
+#define ce_stage(ce) ((CE_STAGEMASK & (ce)->ce_flags) >> CE_STAGESHIFT)
 
 #define ce_permissions(mode) (((mode) & 0100) ? 0755 : 0644)
 static inline unsigned int create_ce_mode(unsigned int mode)
 {
 	if (S_ISLNK(mode))
-		return htonl(S_IFLNK);
+		return S_IFLNK;
 	if (S_ISDIR(mode) || S_ISGITLINK(mode))
-		return htonl(S_IFGITLINK);
-	return htonl(S_IFREG | ce_permissions(mode));
+		return S_IFGITLINK;
+	return S_IFREG | ce_permissions(mode);
 }
 static inline unsigned int ce_mode_from_stat(struct cache_entry *ce, unsigned int mode)
 {
 	extern int trust_executable_bit, has_symlinks;
 	if (!has_symlinks && S_ISREG(mode) &&
-	    ce && S_ISLNK(ntohl(ce->ce_mode)))
+	    ce && S_ISLNK(ce->ce_mode))
 		return ce->ce_mode;
 	if (!trust_executable_bit && S_ISREG(mode)) {
-		if (ce && S_ISREG(ntohl(ce->ce_mode)))
+		if (ce && S_ISREG(ce->ce_mode))
 			return ce->ce_mode;
 		return create_ce_mode(0666);
 	}
@@ -146,14 +164,14 @@ static inline unsigned int ce_mode_from_stat(struct cache_entry *ce, unsigned in
 	S_ISLNK(mode) ? S_IFLNK : S_ISDIR(mode) ? S_IFDIR : S_IFGITLINK)
 
 #define cache_entry_size(len) ((offsetof(struct cache_entry,name) + (len) + 8) & ~7)
+#define ondisk_cache_entry_size(len) ((offsetof(struct ondisk_cache_entry,name) + (len) + 8) & ~7)
 
 struct index_state {
 	struct cache_entry **cache;
 	unsigned int cache_nr, cache_alloc, cache_changed;
 	struct cache_tree *cache_tree;
 	time_t timestamp;
-	void *mmap;
-	size_t mmap_size;
+	void *alloc;
 };
 
 extern struct index_state the_index;
diff --git a/diff-lib.c b/diff-lib.c
index d85d8f3..4a05b02 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -37,7 +37,7 @@ static int get_mode(const char *path, int *mode)
 	if (!path || !strcmp(path, "/dev/null"))
 		*mode = 0;
 	else if (!strcmp(path, "-"))
-		*mode = ntohl(create_ce_mode(0666));
+		*mode = create_ce_mode(0666);
 	else if (stat(path, &st))
 		return error("Could not access '%s'", path);
 	else
@@ -384,7 +384,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 					continue;
 			}
 			else
-				dpath->mode = ntohl(ce_mode_from_stat(ce, st.st_mode));
+				dpath->mode = ce_mode_from_stat(ce, st.st_mode);
 
 			while (i < entries) {
 				struct cache_entry *nce = active_cache[i];
@@ -398,10 +398,10 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 				 */
 				stage = ce_stage(nce);
 				if (2 <= stage) {
-					int mode = ntohl(nce->ce_mode);
+					int mode = nce->ce_mode;
 					num_compare_stages++;
 					hashcpy(dpath->parent[stage-2].sha1, nce->sha1);
-					dpath->parent[stage-2].mode = ntohl(ce_mode_from_stat(nce, mode));
+					dpath->parent[stage-2].mode = ce_mode_from_stat(nce, mode);
 					dpath->parent[stage-2].status =
 						DIFF_STATUS_MODIFIED;
 				}
@@ -442,15 +442,15 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			}
 			if (silent_on_removed)
 				continue;
-			diff_addremove(&revs->diffopt, '-', ntohl(ce->ce_mode),
+			diff_addremove(&revs->diffopt, '-', ce->ce_mode,
 				       ce->sha1, ce->name, NULL);
 			continue;
 		}
 		changed = ce_match_stat(ce, &st, ce_option);
 		if (!changed && !DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))
 			continue;
-		oldmode = ntohl(ce->ce_mode);
-		newmode = ntohl(ce_mode_from_stat(ce, st.st_mode));
+		oldmode = ce->ce_mode;
+		newmode = ce_mode_from_stat(ce, st.st_mode);
 		diff_change(&revs->diffopt, oldmode, newmode,
 			    ce->sha1, (changed ? null_sha1 : ce->sha1),
 			    ce->name, NULL);
@@ -471,7 +471,7 @@ static void diff_index_show_file(struct rev_info *revs,
 				 struct cache_entry *ce,
 				 unsigned char *sha1, unsigned int mode)
 {
-	diff_addremove(&revs->diffopt, prefix[0], ntohl(mode),
+	diff_addremove(&revs->diffopt, prefix[0], mode,
 		       sha1, ce->name, NULL);
 }
 
@@ -550,14 +550,14 @@ static int show_modified(struct rev_info *revs,
 		p->len = pathlen;
 		memcpy(p->path, new->name, pathlen);
 		p->path[pathlen] = 0;
-		p->mode = ntohl(mode);
+		p->mode = mode;
 		hashclr(p->sha1);
 		memset(p->parent, 0, 2 * sizeof(struct combine_diff_parent));
 		p->parent[0].status = DIFF_STATUS_MODIFIED;
-		p->parent[0].mode = ntohl(new->ce_mode);
+		p->parent[0].mode = new->ce_mode;
 		hashcpy(p->parent[0].sha1, new->sha1);
 		p->parent[1].status = DIFF_STATUS_MODIFIED;
-		p->parent[1].mode = ntohl(old->ce_mode);
+		p->parent[1].mode = old->ce_mode;
 		hashcpy(p->parent[1].sha1, old->sha1);
 		show_combined_diff(p, 2, revs->dense_combined_merges, revs);
 		free(p);
@@ -569,9 +569,6 @@ static int show_modified(struct rev_info *revs,
 	    !DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))
 		return 0;
 
-	mode = ntohl(mode);
-	oldmode = ntohl(oldmode);
-
 	diff_change(&revs->diffopt, oldmode, mode,
 		    old->sha1, sha1, old->name, NULL);
 	return 0;
@@ -628,7 +625,7 @@ static int diff_cache(struct rev_info *revs,
 					   cached, match_missing))
 				break;
 			diff_unmerge(&revs->diffopt, ce->name,
-				     ntohl(ce->ce_mode), ce->sha1);
+				     ce->ce_mode, ce->sha1);
 			break;
 		case 3:
 			diff_unmerge(&revs->diffopt, ce->name,
@@ -664,7 +661,7 @@ static void mark_merge_entries(void)
 		struct cache_entry *ce = active_cache[i];
 		if (!ce_stage(ce))
 			continue;
-		ce->ce_flags |= htons(CE_STAGEMASK);
+		ce->ce_flags |= CE_STAGEMASK;
 	}
 }
 
@@ -722,8 +719,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
 			cache_tree_invalidate_path(active_cache_tree,
 						   ce->name);
 			last = ce;
-			ce->ce_mode = 0;
-			ce->ce_flags &= ~htons(CE_STAGEMASK);
+			ce->ce_flags |= CE_REMOVE;
 		}
 		*dst++ = ce;
 	}
diff --git a/dir.c b/dir.c
index 3e345c2..1b9cc7a 100644
--- a/dir.c
+++ b/dir.c
@@ -391,7 +391,7 @@ static enum exist_status directory_exists_in_index(const char *dirname, int len)
 			break;
 		if (endchar == '/')
 			return index_directory;
-		if (!endchar && S_ISGITLINK(ntohl(ce->ce_mode)))
+		if (!endchar && S_ISGITLINK(ce->ce_mode))
 			return index_gitdir;
 	}
 	return index_nonexistent;
diff --git a/entry.c b/entry.c
index 257ab46..44f4b89 100644
--- a/entry.c
+++ b/entry.c
@@ -103,7 +103,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 	int fd;
 	long wrote;
 
-	switch (ntohl(ce->ce_mode) & S_IFMT) {
+	switch (ce->ce_mode & S_IFMT) {
 		char *new;
 		struct strbuf buf;
 		unsigned long size;
@@ -129,7 +129,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 			strcpy(path, ".merge_file_XXXXXX");
 			fd = mkstemp(path);
 		} else
-			fd = create_file(path, ntohl(ce->ce_mode));
+			fd = create_file(path, ce->ce_mode);
 		if (fd < 0) {
 			free(new);
 			return error("git-checkout-index: unable to create file %s (%s)",
@@ -221,7 +221,7 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t
 		unlink(path);
 		if (S_ISDIR(st.st_mode)) {
 			/* If it is a gitlink, leave it alone! */
-			if (S_ISGITLINK(ntohl(ce->ce_mode)))
+			if (S_ISGITLINK(ce->ce_mode))
 				return 0;
 			if (!state->force)
 				return error("%s is a directory", path);
diff --git a/merge-index.c b/merge-index.c
index fa719cb..bbb700b 100644
--- a/merge-index.c
+++ b/merge-index.c
@@ -48,7 +48,7 @@ static int merge_entry(int pos, const char *path)
 			break;
 		found++;
 		strcpy(hexbuf[stage], sha1_to_hex(ce->sha1));
-		sprintf(ownbuf[stage], "%o", ntohl(ce->ce_mode));
+		sprintf(ownbuf[stage], "%o", ce->ce_mode);
 		arguments[stage] = hexbuf[stage];
 		arguments[stage + 4] = ownbuf[stage];
 	} while (++pos < active_nr);
diff --git a/merge-recursive.c b/merge-recursive.c
index c292a77..bdf03b1 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -333,7 +333,7 @@ static struct path_list *get_unmerged(void)
 			item->util = xcalloc(1, sizeof(struct stage_data));
 		}
 		e = item->util;
-		e->stages[ce_stage(ce)].mode = ntohl(ce->ce_mode);
+		e->stages[ce_stage(ce)].mode = ce->ce_mode;
 		hashcpy(e->stages[ce_stage(ce)].sha, ce->sha1);
 	}
 
diff --git a/reachable.c b/reachable.c
index 6383401..00f289f 100644
--- a/reachable.c
+++ b/reachable.c
@@ -176,7 +176,7 @@ static void add_cache_refs(struct rev_info *revs)
 		 * lookup_blob() on them, to avoid populating the hash table
 		 * with invalid information
 		 */
-		if (S_ISGITLINK(ntohl(active_cache[i]->ce_mode)))
+		if (S_ISGITLINK(active_cache[i]->ce_mode))
 			continue;
 
 		lookup_blob(active_cache[i]->sha1);
diff --git a/read-cache.c b/read-cache.c
index 7db5588..f5f9c3d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -30,20 +30,16 @@ struct index_state the_index;
  */
 void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
 {
-	ce->ce_ctime.sec = htonl(st->st_ctime);
-	ce->ce_mtime.sec = htonl(st->st_mtime);
-#ifdef USE_NSEC
-	ce->ce_ctime.nsec = htonl(st->st_ctim.tv_nsec);
-	ce->ce_mtime.nsec = htonl(st->st_mtim.tv_nsec);
-#endif
-	ce->ce_dev = htonl(st->st_dev);
-	ce->ce_ino = htonl(st->st_ino);
-	ce->ce_uid = htonl(st->st_uid);
-	ce->ce_gid = htonl(st->st_gid);
-	ce->ce_size = htonl(st->st_size);
+	ce->ce_ctime = st->st_ctime;
+	ce->ce_mtime = st->st_mtime;
+	ce->ce_dev = st->st_dev;
+	ce->ce_ino = st->st_ino;
+	ce->ce_uid = st->st_uid;
+	ce->ce_gid = st->st_gid;
+	ce->ce_size = st->st_size;
 
 	if (assume_unchanged)
-		ce->ce_flags |= htons(CE_VALID);
+		ce->ce_flags |= CE_VALID;
 }
 
 static int ce_compare_data(struct cache_entry *ce, struct stat *st)
@@ -116,7 +112,7 @@ static int ce_modified_check_fs(struct cache_entry *ce, struct stat *st)
 			return DATA_CHANGED;
 		break;
 	case S_IFDIR:
-		if (S_ISGITLINK(ntohl(ce->ce_mode)))
+		if (S_ISGITLINK(ce->ce_mode))
 			return 0;
 	default:
 		return TYPE_CHANGED;
@@ -128,14 +124,14 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 {
 	unsigned int changed = 0;
 
-	switch (ntohl(ce->ce_mode) & S_IFMT) {
+	switch (ce->ce_mode & S_IFMT) {
 	case S_IFREG:
 		changed |= !S_ISREG(st->st_mode) ? TYPE_CHANGED : 0;
 		/* We consider only the owner x bit to be relevant for
 		 * "mode changes"
 		 */
 		if (trust_executable_bit &&
-		    (0100 & (ntohl(ce->ce_mode) ^ st->st_mode)))
+		    (0100 & (ce->ce_mode ^ st->st_mode)))
 			changed |= MODE_CHANGED;
 		break;
 	case S_IFLNK:
@@ -152,29 +148,17 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 	case 0: /* Special case: unmerged file in index */
 		return MODE_CHANGED | DATA_CHANGED | TYPE_CHANGED;
 	default:
-		die("internal error: ce_mode is %o", ntohl(ce->ce_mode));
+		die("internal error: ce_mode is %o", ce->ce_mode);
 	}
-	if (ce->ce_mtime.sec != htonl(st->st_mtime))
+	if (ce->ce_mtime != (unsigned int) st->st_mtime)
 		changed |= MTIME_CHANGED;
-	if (ce->ce_ctime.sec != htonl(st->st_ctime))
+	if (ce->ce_ctime != (unsigned int) st->st_ctime)
 		changed |= CTIME_CHANGED;
 
-#ifdef USE_NSEC
-	/*
-	 * nsec seems unreliable - not all filesystems support it, so
-	 * as long as it is in the inode cache you get right nsec
-	 * but after it gets flushed, you get zero nsec.
-	 */
-	if (ce->ce_mtime.nsec != htonl(st->st_mtim.tv_nsec))
-		changed |= MTIME_CHANGED;
-	if (ce->ce_ctime.nsec != htonl(st->st_ctim.tv_nsec))
-		changed |= CTIME_CHANGED;
-#endif
-
-	if (ce->ce_uid != htonl(st->st_uid) ||
-	    ce->ce_gid != htonl(st->st_gid))
+	if (ce->ce_uid != (unsigned int) st->st_uid ||
+	    ce->ce_gid != (unsigned int) st->st_gid)
 		changed |= OWNER_CHANGED;
-	if (ce->ce_ino != htonl(st->st_ino))
+	if (ce->ce_ino != (unsigned int) st->st_ino)
 		changed |= INODE_CHANGED;
 
 #ifdef USE_STDEV
@@ -183,11 +167,11 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 	 * clients will have different views of what "device"
 	 * the filesystem is on
 	 */
-	if (ce->ce_dev != htonl(st->st_dev))
+	if (ce->ce_dev != (unsigned int) st->st_dev)
 		changed |= INODE_CHANGED;
 #endif
 
-	if (ce->ce_size != htonl(st->st_size))
+	if (ce->ce_size != (unsigned int) st->st_size)
 		changed |= DATA_CHANGED;
 
 	return changed;
@@ -205,7 +189,7 @@ int ie_match_stat(struct index_state *istate,
 	 * If it's marked as always valid in the index, it's
 	 * valid whatever the checked-out copy says.
 	 */
-	if (!ignore_valid && (ce->ce_flags & htons(CE_VALID)))
+	if (!ignore_valid && (ce->ce_flags & CE_VALID))
 		return 0;
 
 	changed = ce_match_stat_basic(ce, st);
@@ -228,7 +212,7 @@ int ie_match_stat(struct index_state *istate,
 	 */
 	if (!changed &&
 	    istate->timestamp &&
-	    istate->timestamp <= ntohl(ce->ce_mtime.sec)) {
+	    istate->timestamp <= ce->ce_mtime) {
 		if (assume_racy_is_modified)
 			changed |= DATA_CHANGED;
 		else
@@ -257,7 +241,7 @@ int ie_modified(struct index_state *istate,
 	 * the length field is zero.  For other cases the ce_size
 	 * should match the SHA1 recorded in the index entry.
 	 */
-	if ((changed & DATA_CHANGED) && ce->ce_size != htonl(0))
+	if ((changed & DATA_CHANGED) && ce->ce_size != 0)
 		return changed;
 
 	changed_fs = ce_modified_check_fs(ce, st);
@@ -320,7 +304,7 @@ int index_name_pos(struct index_state *istate, const char *name, int namelen)
 	while (last > first) {
 		int next = (last + first) >> 1;
 		struct cache_entry *ce = istate->cache[next];
-		int cmp = cache_name_compare(name, namelen, ce->name, ntohs(ce->ce_flags));
+		int cmp = cache_name_compare(name, namelen, ce->name, ce->ce_flags);
 		if (!cmp)
 			return next;
 		if (cmp < 0) {
@@ -405,7 +389,7 @@ int add_file_to_index(struct index_state *istate, const char *path, int verbose)
 	size = cache_entry_size(namelen);
 	ce = xcalloc(1, size);
 	memcpy(ce->name, path, namelen);
-	ce->ce_flags = htons(namelen);
+	ce->ce_flags = namelen;
 	fill_stat_cache_info(ce, &st);
 
 	if (trust_executable_bit && has_symlinks)
@@ -583,7 +567,7 @@ static int has_file_name(struct index_state *istate,
 			continue;
 		if (p->name[len] != '/')
 			continue;
-		if (!ce_stage(p) && !p->ce_mode)
+		if (p->ce_flags & CE_REMOVE)
 			continue;
 		retval = -1;
 		if (!ok_to_replace)
@@ -616,7 +600,7 @@ static int has_dir_name(struct index_state *istate,
 		}
 		len = slash - name;
 
-		pos = index_name_pos(istate, name, ntohs(create_ce_flags(len, stage)));
+		pos = index_name_pos(istate, name, create_ce_flags(len, stage));
 		if (pos >= 0) {
 			/*
 			 * Found one, but not so fast.  This could
@@ -679,7 +663,7 @@ static int check_file_directory_conflict(struct index_state *istate,
 	/*
 	 * When ce is an "I am going away" entry, we allow it to be added
 	 */
-	if (!ce_stage(ce) && !ce->ce_mode)
+	if (ce->ce_flags & CE_REMOVE)
 		return 0;
 
 	/*
@@ -704,7 +688,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
 	int skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;
 
 	cache_tree_invalidate_path(istate->cache_tree, ce->name);
-	pos = index_name_pos(istate, ce->name, ntohs(ce->ce_flags));
+	pos = index_name_pos(istate, ce->name, ce->ce_flags);
 
 	/* existing match? Just replace it. */
 	if (pos >= 0) {
@@ -736,7 +720,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
 		if (!ok_to_replace)
 			return error("'%s' appears as both a file and as a directory",
 				     ce->name);
-		pos = index_name_pos(istate, ce->name, ntohs(ce->ce_flags));
+		pos = index_name_pos(istate, ce->name, ce->ce_flags);
 		pos = -pos-1;
 	}
 	return pos + 1;
@@ -810,7 +794,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 		 * valid again, under "assume unchanged" mode.
 		 */
 		if (ignore_valid && assume_unchanged &&
-		    !(ce->ce_flags & htons(CE_VALID)))
+		    !(ce->ce_flags & CE_VALID))
 			; /* mark this one VALID again */
 		else
 			return ce;
@@ -826,7 +810,6 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 	updated = xmalloc(size);
 	memcpy(updated, ce, size);
 	fill_stat_cache_info(updated, &st);
-
 	/*
 	 * If ignore_valid is not set, we should leave CE_VALID bit
 	 * alone.  Otherwise, paths marked with --no-assume-unchanged
@@ -834,8 +817,8 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 	 * automatically, which is not really what we want.
 	 */
 	if (!ignore_valid && assume_unchanged &&
-	    !(ce->ce_flags & htons(CE_VALID)))
-		updated->ce_flags &= ~htons(CE_VALID);
+	    !(ce->ce_flags & CE_VALID))
+		updated->ce_flags &= ~CE_VALID;
 
 	return updated;
 }
@@ -880,7 +863,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 				/* If we are doing --really-refresh that
 				 * means the index is not valid anymore.
 				 */
-				ce->ce_flags &= ~htons(CE_VALID);
+				ce->ce_flags &= ~CE_VALID;
 				istate->cache_changed = 1;
 			}
 			if (quiet)
@@ -942,16 +925,34 @@ int read_index(struct index_state *istate)
 	return read_index_from(istate, get_index_file());
 }
 
+static void convert_from_disk(struct ondisk_cache_entry *ondisk, struct cache_entry *ce)
+{
+	ce->ce_ctime = ntohl(ondisk->ctime.sec);
+	ce->ce_mtime = ntohl(ondisk->mtime.sec);
+	ce->ce_dev   = ntohl(ondisk->dev);
+	ce->ce_ino   = ntohl(ondisk->ino);
+	ce->ce_mode  = ntohl(ondisk->mode);
+	ce->ce_uid   = ntohl(ondisk->uid);
+	ce->ce_gid   = ntohl(ondisk->gid);
+	ce->ce_size  = ntohl(ondisk->size);
+	/* On-disk flags are just 16 bits */
+	ce->ce_flags = ntohs(ondisk->flags);
+	hashcpy(ce->sha1, ondisk->sha1);
+	memcpy(ce->name, ondisk->name, ce_namelen(ce)+1);
+}
+
 /* remember to discard_cache() before reading a different cache! */
 int read_index_from(struct index_state *istate, const char *path)
 {
 	int fd, i;
 	struct stat st;
-	unsigned long offset;
+	unsigned long src_offset, dst_offset;
 	struct cache_header *hdr;
+	void *mmap;
+	size_t mmap_size;
 
 	errno = EBUSY;
-	if (istate->mmap)
+	if (istate->alloc)
 		return istate->cache_nr;
 
 	errno = ENOENT;
@@ -967,31 +968,47 @@ int read_index_from(struct index_state *istate, const char *path)
 		die("cannot stat the open index (%s)", strerror(errno));
 
 	errno = EINVAL;
-	istate->mmap_size = xsize_t(st.st_size);
-	if (istate->mmap_size < sizeof(struct cache_header) + 20)
+	mmap_size = xsize_t(st.st_size);
+	if (mmap_size < sizeof(struct cache_header) + 20)
 		die("index file smaller than expected");
 
-	istate->mmap = xmmap(NULL, istate->mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
+	mmap = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
 	close(fd);
+	if (mmap == MAP_FAILED)
+		die("unable to map index file");
 
-	hdr = istate->mmap;
-	if (verify_hdr(hdr, istate->mmap_size) < 0)
+	hdr = mmap;
+	if (verify_hdr(hdr, mmap_size) < 0)
 		goto unmap;
 
 	istate->cache_nr = ntohl(hdr->hdr_entries);
 	istate->cache_alloc = alloc_nr(istate->cache_nr);
 	istate->cache = xcalloc(istate->cache_alloc, sizeof(struct cache_entry *));
 
-	offset = sizeof(*hdr);
+	/*
+	 * The disk format is actually larger than the in-memory format,
+	 * due to space for nsec etc, so even though the in-memory one
+	 * has room for a few  more flags, we can allocate using the same
+	 * index size
+	 */
+	istate->alloc = xmalloc(mmap_size);
+
+	src_offset = sizeof(*hdr);
+	dst_offset = 0;
 	for (i = 0; i < istate->cache_nr; i++) {
+		struct ondisk_cache_entry *disk_ce;
 		struct cache_entry *ce;
 
-		ce = (struct cache_entry *)((char *)(istate->mmap) + offset);
-		offset = offset + ce_size(ce);
+		disk_ce = (struct ondisk_cache_entry *)((char *)mmap + src_offset);
+		ce = (struct cache_entry *)((char *)istate->alloc + dst_offset);
+		convert_from_disk(disk_ce, ce);
 		istate->cache[i] = ce;
+
+		src_offset += ondisk_ce_size(ce);
+		dst_offset += ce_size(ce);
 	}
 	istate->timestamp = st.st_mtime;
-	while (offset <= istate->mmap_size - 20 - 8) {
+	while (src_offset <= mmap_size - 20 - 8) {
 		/* After an array of active_nr index entries,
 		 * there can be arbitrary number of extended
 		 * sections, each of which is prefixed with
@@ -999,40 +1016,36 @@ int read_index_from(struct index_state *istate, const char *path)
 		 * in 4-byte network byte order.
 		 */
 		unsigned long extsize;
-		memcpy(&extsize, (char *)(istate->mmap) + offset + 4, 4);
+		memcpy(&extsize, (char *)mmap + src_offset + 4, 4);
 		extsize = ntohl(extsize);
 		if (read_index_extension(istate,
-					 ((const char *) (istate->mmap)) + offset,
-					 (char *) (istate->mmap) + offset + 8,
+					 (const char *) mmap + src_offset,
+					 (char *) mmap + src_offset + 8,
 					 extsize) < 0)
 			goto unmap;
-		offset += 8;
-		offset += extsize;
+		src_offset += 8;
+		src_offset += extsize;
 	}
+	munmap(mmap, mmap_size);
 	return istate->cache_nr;
 
 unmap:
-	munmap(istate->mmap, istate->mmap_size);
+	munmap(mmap, mmap_size);
 	errno = EINVAL;
 	die("index file corrupt");
 }
 
 int discard_index(struct index_state *istate)
 {
-	int ret;
-
 	istate->cache_nr = 0;
 	istate->cache_changed = 0;
 	istate->timestamp = 0;
 	cache_tree_free(&(istate->cache_tree));
-	if (istate->mmap == NULL)
-		return 0;
-	ret = munmap(istate->mmap, istate->mmap_size);
-	istate->mmap = NULL;
-	istate->mmap_size = 0;
+	free(istate->alloc);
+	istate->alloc = NULL;
 
 	/* no need to throw away allocated active_cache */
-	return ret;
+	return 0;
 }
 
 #define WRITE_BUFFER_SIZE 8192
@@ -1144,10 +1157,32 @@ static void ce_smudge_racily_clean_entry(struct cache_entry *ce)
 		 * file, and never calls us, so the cached size information
 		 * for "frotz" stays 6 which does not match the filesystem.
 		 */
-		ce->ce_size = htonl(0);
+		ce->ce_size = 0;
 	}
 }
 
+static int ce_write_entry(SHA_CTX *c, int fd, struct cache_entry *ce)
+{
+	int size = ondisk_ce_size(ce);
+	struct ondisk_cache_entry *ondisk = xcalloc(1, size);
+
+	ondisk->ctime.sec = htonl(ce->ce_ctime);
+	ondisk->ctime.nsec = 0;
+	ondisk->mtime.sec = htonl(ce->ce_mtime);
+	ondisk->mtime.nsec = 0;
+	ondisk->dev  = htonl(ce->ce_dev);
+	ondisk->ino  = htonl(ce->ce_ino);
+	ondisk->mode = htonl(ce->ce_mode);
+	ondisk->uid  = htonl(ce->ce_uid);
+	ondisk->gid  = htonl(ce->ce_gid);
+	ondisk->size = htonl(ce->ce_size);
+	hashcpy(ondisk->sha1, ce->sha1);
+	ondisk->flags = htons(ce->ce_flags);
+	memcpy(ondisk->name, ce->name, ce_namelen(ce));
+
+	return ce_write(c, fd, ondisk, size);
+}
+
 int write_index(struct index_state *istate, int newfd)
 {
 	SHA_CTX c;
@@ -1157,7 +1192,7 @@ int write_index(struct index_state *istate, int newfd)
 	int entries = istate->cache_nr;
 
 	for (i = removed = 0; i < entries; i++)
-		if (!cache[i]->ce_mode)
+		if (cache[i]->ce_flags & CE_REMOVE)
 			removed++;
 
 	hdr.hdr_signature = htonl(CACHE_SIGNATURE);
@@ -1170,12 +1205,12 @@ int write_index(struct index_state *istate, int newfd)
 
 	for (i = 0; i < entries; i++) {
 		struct cache_entry *ce = cache[i];
-		if (!ce->ce_mode)
+		if (ce->ce_flags & CE_REMOVE)
 			continue;
 		if (istate->timestamp &&
-		    istate->timestamp <= ntohl(ce->ce_mtime.sec))
+		    istate->timestamp <= ce->ce_mtime)
 			ce_smudge_racily_clean_entry(ce);
-		if (ce_write(&c, newfd, ce, ce_size(ce)) < 0)
+		if (ce_write_entry(&c, newfd, ce) < 0)
 			return -1;
 	}
 
diff --git a/sha1_name.c b/sha1_name.c
index 13e1164..be8489e 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -695,7 +695,7 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
 				break;
 			if (ce_stage(ce) == stage) {
 				hashcpy(sha1, ce->sha1);
-				*mode = ntohl(ce->ce_mode);
+				*mode = ce->ce_mode;
 				return 0;
 			}
 			pos++;
diff --git a/tree.c b/tree.c
index 8c0819f..87708ef 100644
--- a/tree.c
+++ b/tree.c
@@ -142,8 +142,8 @@ static int cmp_cache_name_compare(const void *a_, const void *b_)
 
 	ce1 = *((const struct cache_entry **)a_);
 	ce2 = *((const struct cache_entry **)b_);
-	return cache_name_compare(ce1->name, ntohs(ce1->ce_flags),
-				  ce2->name, ntohs(ce2->ce_flags));
+	return cache_name_compare(ce1->name, ce1->ce_flags,
+				  ce2->name, ce2->ce_flags);
 }
 
 int read_tree(struct tree *tree, int stage, const char **match)
diff --git a/unpack-trees.c b/unpack-trees.c
index aa2513e..ff46fd6 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -289,7 +289,6 @@ static struct checkout state;
 static void check_updates(struct cache_entry **src, int nr,
 			struct unpack_trees_options *o)
 {
-	unsigned short mask = htons(CE_UPDATE);
 	unsigned cnt = 0, total = 0;
 	struct progress *progress = NULL;
 	char last_symlink[PATH_MAX];
@@ -297,7 +296,7 @@ static void check_updates(struct cache_entry **src, int nr,
 	if (o->update && o->verbose_update) {
 		for (total = cnt = 0; cnt < nr; cnt++) {
 			struct cache_entry *ce = src[cnt];
-			if (!ce->ce_mode || ce->ce_flags & mask)
+			if (ce->ce_flags & (CE_UPDATE | CE_REMOVE))
 				total++;
 		}
 
@@ -310,15 +309,15 @@ static void check_updates(struct cache_entry **src, int nr,
 	while (nr--) {
 		struct cache_entry *ce = *src++;
 
-		if (!ce->ce_mode || ce->ce_flags & mask)
+		if (ce->ce_flags & (CE_UPDATE | CE_REMOVE))
 			display_progress(progress, ++cnt);
-		if (!ce->ce_mode) {
+		if (ce->ce_flags & CE_REMOVE) {
 			if (o->update)
 				unlink_entry(ce->name, last_symlink);
 			continue;
 		}
-		if (ce->ce_flags & mask) {
-			ce->ce_flags &= ~mask;
+		if (ce->ce_flags & CE_UPDATE) {
+			ce->ce_flags &= ~CE_UPDATE;
 			if (o->update) {
 				checkout_entry(ce, &state, NULL);
 				*last_symlink = '\0';
@@ -408,7 +407,7 @@ static void verify_uptodate(struct cache_entry *ce,
 		 * submodules that are marked to be automatically
 		 * checked out.
 		 */
-		if (S_ISGITLINK(ntohl(ce->ce_mode)))
+		if (S_ISGITLINK(ce->ce_mode))
 			return;
 		errno = 0;
 	}
@@ -450,7 +449,7 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,
 	int cnt = 0;
 	unsigned char sha1[20];
 
-	if (S_ISGITLINK(ntohl(ce->ce_mode)) &&
+	if (S_ISGITLINK(ce->ce_mode) &&
 	    resolve_gitlink_ref(ce->name, "HEAD", sha1) == 0) {
 		/* If we are not going to update the submodule, then
 		 * we don't care.
@@ -481,7 +480,7 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,
 		 */
 		if (!ce_stage(ce)) {
 			verify_uptodate(ce, o);
-			ce->ce_mode = 0;
+			ce->ce_flags |= CE_REMOVE;
 		}
 		cnt++;
 	}
@@ -568,7 +567,7 @@ static void verify_absent(struct cache_entry *ce, const char *action,
 		cnt = cache_name_pos(ce->name, strlen(ce->name));
 		if (0 <= cnt) {
 			struct cache_entry *ce = active_cache[cnt];
-			if (!ce_stage(ce) && !ce->ce_mode)
+			if (ce->ce_flags & CE_REMOVE)
 				return;
 		}
 
@@ -580,7 +579,7 @@ static void verify_absent(struct cache_entry *ce, const char *action,
 static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 		struct unpack_trees_options *o)
 {
-	merge->ce_flags |= htons(CE_UPDATE);
+	merge->ce_flags |= CE_UPDATE;
 	if (old) {
 		/*
 		 * See if we can re-use the old CE directly?
@@ -601,7 +600,7 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 		invalidate_ce_path(merge);
 	}
 
-	merge->ce_flags &= ~htons(CE_STAGEMASK);
+	merge->ce_flags &= ~CE_STAGEMASK;
 	add_cache_entry(merge, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);
 	return 1;
 }
@@ -613,7 +612,7 @@ static int deleted_entry(struct cache_entry *ce, struct cache_entry *old,
 		verify_uptodate(old, o);
 	else
 		verify_absent(ce, "removed", o);
-	ce->ce_mode = 0;
+	ce->ce_flags |= CE_REMOVE;
 	add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);
 	invalidate_ce_path(ce);
 	return 1;
@@ -634,7 +633,7 @@ static void show_stage_entry(FILE *o,
 	else
 		fprintf(o, "%s%06o %s %d\t%s\n",
 			label,
-			ntohl(ce->ce_mode),
+			ce->ce_mode,
 			sha1_to_hex(ce->sha1),
 			ce_stage(ce),
 			ce->name);
@@ -920,7 +919,7 @@ int oneway_merge(struct cache_entry **src,
 			struct stat st;
 			if (lstat(old->name, &st) ||
 			    ce_match_stat(old, &st, CE_MATCH_IGNORE_VALID))
-				old->ce_flags |= htons(CE_UPDATE);
+				old->ce_flags |= CE_UPDATE;
 		}
 		return keep_entry(old, o);
 	}

[PATCH] index: be careful when handling long names

[Junio C Hamano]

We currently use lower 12-bit (masked with CE_NAMEMASK) in the
ce_flags field to store the length of the name in cache_entry,
without checking the length parameter given to
create_ce_flags().  This can make us store incorrect length.

Currently we are mostly protected by the fact that many
codepaths first copy the path in a variable of size PATH_MAX,
which typically is 4096 that happens to match the limit, but
that feels like a bug waiting to happen.  Besides, that would
not allow us to shorten the width of CE_NAMEMASK to use the bits
for new flags.

This redefines the meaning of the name length stored in the
cache_entry.  A name that does not fit is represented by storing
CE_NAMEMASK in the field, and the actual length needs to be
computed by actually counting the bytes in the name[] field.
This way, only the unusually long paths need to suffer.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is rebased on your "Updated in-memory index" patch.

   When we read on-disk index, I fear that we trust the name
   length a bit too much.  With or without this patch, you could
   craft a malicious index file that records a namelen that is
   longer than the real string name length for the last entry to
   cause us to go beyond the mmaped area.  Maybe we would want
   to make sure that (1) the name lengths are sane; (2) names
   have NUL at the place we expect them to be; and (3) names are
   sorted in the proper cache order, while we iterate over the
   ondisk structure and convert it to incore structure.

 cache.h          |   17 +++++++++++++++--
 read-cache.c     |   12 +++++++++++-
 t/t0000-basic.sh |   20 ++++++++++++++++++++
 3 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index 4a054c5..9eaffde 100644
--- a/cache.h
+++ b/cache.h
@@ -131,8 +131,21 @@ struct cache_entry {
 #define CE_UPDATE    (0x10000)
 #define CE_REMOVE    (0x20000)
 
-#define create_ce_flags(len, stage) ((len) | ((stage) << CE_STAGESHIFT))
-#define ce_namelen(ce) (CE_NAMEMASK & (ce)->ce_flags)
+static inline unsigned create_ce_flags(size_t len, unsigned stage)
+{
+	if (len >= CE_NAMEMASK)
+		len = CE_NAMEMASK;
+	return (len | (stage << CE_STAGESHIFT));
+}
+
+static inline size_t ce_namelen(const struct cache_entry *ce)
+{
+	size_t len = ce->ce_flags & CE_NAMEMASK;
+	if (len < CE_NAMEMASK)
+		return len;
+	return strlen(ce->name + CE_NAMEMASK) + CE_NAMEMASK;
+}
+
 #define ce_size(ce) cache_entry_size(ce_namelen(ce))
 #define ondisk_ce_size(ce) ondisk_cache_entry_size(ce_namelen(ce))
 #define ce_stage(ce) ((CE_STAGEMASK & (ce)->ce_flags) >> CE_STAGESHIFT)
diff --git a/read-cache.c b/read-cache.c
index f5f9c3d..f98f57c 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -927,6 +927,8 @@ int read_index(struct index_state *istate)
 
 static void convert_from_disk(struct ondisk_cache_entry *ondisk, struct cache_entry *ce)
 {
+	size_t len;
+
 	ce->ce_ctime = ntohl(ondisk->ctime.sec);
 	ce->ce_mtime = ntohl(ondisk->mtime.sec);
 	ce->ce_dev   = ntohl(ondisk->dev);
@@ -938,7 +940,15 @@ static void convert_from_disk(struct ondisk_cache_entry *ondisk, struct cache_en
 	/* On-disk flags are just 16 bits */
 	ce->ce_flags = ntohs(ondisk->flags);
 	hashcpy(ce->sha1, ondisk->sha1);
-	memcpy(ce->name, ondisk->name, ce_namelen(ce)+1);
+
+	len = ce->ce_flags & CE_NAMEMASK;
+	if (len == CE_NAMEMASK)
+		len = strlen(ondisk->name);
+	/*
+	 * NEEDSWORK: If the original index is crafted, this copy could
+	 * go unchecked.
+	 */
+	memcpy(ce->name, ondisk->name, len + 1);
 }
 
 /* remember to discard_cache() before reading a different cache! */
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 4e49d59..9f84b8d 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -297,4 +297,24 @@ test_expect_success 'absolute path works as expected' '
 	test "$sym" = "$(test-absolute-path $dir2/syml)"
 '
 
+test_expect_success 'very long name in the index handled sanely' '
+
+	a=a && # 1
+	a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 16
+	a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 256
+	a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 4096
+	a=${a}q &&
+
+	>path4 &&
+	git update-index --add path4 &&
+	(
+		git ls-files -s path4 |
+		sed -e "s/	.*/	/" |
+		tr -d "\012"
+		echo "$a"
+	) | git update-index --index-info &&
+	len=$(git ls-files "a*" | wc -c) &&
+	test $len = 4098
+'
+
 test_done
-- 
1.5.4.rc3.37.gfdcf3

[PATCH] Avoid running lstat(2) on the same cache entry.

[Junio C Hamano]

Aside from the lstat(2) done for work tree files, there are
quite many lstat(2) calls in refname dwimming codepath.  This
patch is not about reducing them.

 * It adds a new ce_flag, CE_UPTODATE, that is meant to mark the
   cache entries that record a regular file blob that is up to
   date in the work tree.  If somebody later walks the index and
   wants to see if the work tree has changes, they do not have
   to be checked with lstat(2) again.

 * fill_stat_cache_info() marks the cache entry it just added
   with CE_UPTODATE.  This has the effect of marking the paths
   we write out of the index and lstat(2) immediately as "no
   need to lstat -- we know it is up-to-date", from quite a lot
   fo callers:

    - git-apply --index
    - git-update-index
    - git-checkout-index
    - git-add (uses add_file_to_index())
    - git-commit (ditto)
    - git-mv (ditto)

 * refresh_cache_ent() also marks the cache entry that are clean
   with CE_UPTODATE.

 * write_index is changed not to write CE_UPTODATE out to the
   index file, because CE_UPTODATE is meant to be transient only
   in core.  For the same reason, CE_UPDATE is not written to
   prevent an accident from happening.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Again, this is a rebase on top of your "Updated in-memory
   index".  "echo >>Makefile && git commit -m test Makefile" in
   the kernel repository does 23k lstat(2) with this patch, 46k
   without.

 cache.h      |    3 +++
 diff.c       |   23 ++++++++++++++---------
 read-cache.c |   16 +++++++++++++++-
 3 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/cache.h b/cache.h
index 9eaffde..3a47cdc 100644
--- a/cache.h
+++ b/cache.h
@@ -130,6 +130,7 @@ struct cache_entry {
 /* In-memory only */
 #define CE_UPDATE    (0x10000)
 #define CE_REMOVE    (0x20000)
+#define CE_UPTODATE  (0x40000)
 
 static inline unsigned create_ce_flags(size_t len, unsigned stage)
 {
@@ -149,6 +150,8 @@ static inline size_t ce_namelen(const struct cache_entry *ce)
 #define ce_size(ce) cache_entry_size(ce_namelen(ce))
 #define ondisk_ce_size(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_mark_uptodate(ce) ((ce)->ce_flags |= CE_UPTODATE)
 
 #define ce_permissions(mode) (((mode) & 0100) ? 0755 : 0644)
 static inline unsigned int create_ce_mode(unsigned int mode)
diff --git a/diff.c b/diff.c
index 5b8afdc..d464fe3 100644
--- a/diff.c
+++ b/diff.c
@@ -1510,17 +1510,22 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int
 	if (pos < 0)
 		return 0;
 	ce = active_cache[pos];
-	if ((lstat(name, &st) < 0) ||
-	    !S_ISREG(st.st_mode) || /* careful! */
-	    ce_match_stat(ce, &st, 0) ||
-	    hashcmp(sha1, ce->sha1))
+
+	/*
+	 * This is not the sha1 we are looking for, or
+	 * unreusable because it is not a regular file.
+	 */
+	if (hashcmp(sha1, ce->sha1) || !S_ISREG(ce->ce_mode))
 		return 0;
-	/* we return 1 only when we can stat, it is a regular file,
-	 * stat information matches, and sha1 recorded in the cache
-	 * matches.  I.e. we know the file in the work tree really is
-	 * the same as the <name, sha1> pair.
+
+	/*
+	 * If ce matches the file in the work tree, we can reuse it.
 	 */
-	return 1;
+	if (ce_uptodate(ce) ||
+	    (!lstat(name, &st) && !ce_match_stat(ce, &st, 0)))
+		return 1;
+
+	return 0;
 }
 
 static int populate_from_stdin(struct diff_filespec *s)
diff --git a/read-cache.c b/read-cache.c
index f98f57c..6165885 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -40,6 +40,9 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
 
 	if (assume_unchanged)
 		ce->ce_flags |= CE_VALID;
+
+	if (S_ISREG(st->st_mode))
+		ce_mark_uptodate(ce);
 }
 
 static int ce_compare_data(struct cache_entry *ce, struct stat *st)
@@ -411,6 +414,7 @@ int add_file_to_index(struct index_state *istate, const char *path, int verbose)
 	    !ie_match_stat(istate, istate->cache[pos], &st, ce_option)) {
 		/* Nothing changed, really */
 		free(ce);
+		ce_mark_uptodate(istate->cache[pos]);
 		return 0;
 	}
 
@@ -778,6 +782,9 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 	int changed, size;
 	int ignore_valid = options & CE_MATCH_IGNORE_VALID;
 
+	if (ce_uptodate(ce))
+		return ce;
+
 	if (lstat(ce->name, &st) < 0) {
 		if (err)
 			*err = errno;
@@ -796,8 +803,15 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 		if (ignore_valid && assume_unchanged &&
 		    !(ce->ce_flags & CE_VALID))
 			; /* mark this one VALID again */
-		else
+		else {
+			/*
+			 * We do not mark the index itself "modified"
+			 * because CE_UPTODATE flag is in-core only;
+			 * we are not going to write this change out.
+			 */
+			ce_mark_uptodate(ce);
 			return ce;
+		}
 	}
 
 	if (ie_modified(istate, ce, &st, options)) {
-- 
1.5.4.rc3.37.gfdcf3

Re: [PATCH] Avoid running lstat(2) on the same cache entry.

[Linus Torvalds]

On Fri, 18 Jan 2008, Junio C Hamano wrote:
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Acked-by: Linus Torvalds <torvalds@linux-foundation.org>

>  * Again, this is a rebase on top of your "Updated in-memory
>    index".  "echo >>Makefile && git commit -m test Makefile" in
>    the kernel repository does 23k lstat(2) with this patch, 46k
>    without.

Sadly, doing a simple "git commit" without anything else, will still 
apparently get us 69230 lstat() calls, with or without this patch. So we 
get 3 lstat() calls per path.

The call trace for the first one is

 - builtin-commit.c: line 786
	prepare_index
	  refresh_index
	    refresh_cache_ent
	      lstat

while the second one is

 - builtin-commit.c: line 793
	prepare_log_message
	  run_status
	    wt_status_print
	      wt_status_print_changed
	        run_diff_files
	          lstat

and the third one is

 - builtin-commit.c: line 795
	run_status
	  wt_status_print
	    wt_status_print_changed
	      run_diff_files
	        lstat

and the reason is that 
 - "run_diff_files()" doesn't honor ce_uptodate
 - ..but even if it did, wt_status_print_changed() does a wt_read_cache() 
   which will invalidate and re-read the index!

The first problem is trivially fixed (appended), the second one is notas 
trivial.

		Linus
---
 diff-lib.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 4a05b02..5d5a950 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -435,6 +435,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 				continue;
 		}
 
+		if (ce_uptodate(ce))
+			continue;
 		if (lstat(ce->name, &st) < 0) {
 			if (errno != ENOENT && errno != ENOTDIR) {
 				perror(ce->name);

Re: [PATCH] Avoid running lstat(2) on the same cache entry.

[Linus Torvalds]

On Sat, 19 Jan 2008, Linus Torvalds wrote:
> 
> The call trace for the first one is

Btw, the easiest way to do this on Linux isn't totally obvious, because 
you cannot do a simple breakpoint on "lstat()", since GNU libc internally 
uses other names.

So if anybody wants to work on this and runs Linux, the way to do this 
trivially (once you know how) is to do

	gdb git
	..
	.. run the startup to wait for the libraries to be loaded 
	..	
	(gdb) b main
	(gdb) run commit
	..
	.. modern glibc low-level lstat() call is __lxstat64
	.. at least on x86-64
	..
	(gdb) b __lxstat64
	..
	.. Ignore the first few ones (we have 23000+ files in the index, 
	.. so don't worry about getting the *first* one)
	..
	(gdb) ignore 2 1000
	(gdb) c
	..
	.. Ok, look at that backtrace, then go to the next one by
	.. just knowing that we'll be doing 23,000+ for each iteration
	.. over the index.
	..
	.. Rinse and repeat this as required:
	..
	(gdb) where
	(gdb) ignore 2 24000
	(gdb) c

which gets you the backtraces I showed you, without having to think too 
much about all the odd lstat() calls we do for resolving refs etc.

			Linus

Re: [PATCH] Avoid running lstat(2) on the same cache entry.

[Linus Torvalds]

On Sat, 19 Jan 2008, Linus Torvalds wrote:
> 
> and the reason is that 
>  - "run_diff_files()" doesn't honor ce_uptodate
>  - ..but even if it did, wt_status_print_changed() does a wt_read_cache() 
>    which will invalidate and re-read the index!
> 
> The first problem is trivially fixed (appended), the second one is not as 
> trivial.

Ok, I have worked on that not-as-trivial thing here, and indeed, it was 
quite painful and needed major cleanups elsewhere.

The reason why wt-status.c ended up invalidating and re-reading the cache 
multiple times was that it uses "run_diff_index()", which in turn uses 
"read_tree()" to populate the index with *both* the old index and the tree 
we want to compare against.

So this patch re-writes run_diff_index() to not use read_tree(), but 
instead use "unpack_trees()" to diff the index to a tree. That, in turn, 
means that we don't need to modify the index itself, which then means that 
we don't need to invalidate it and re-read it!

This, together with the lstat() optimizations, means that "git commit" on 
the kernel tree really only needs to lstat() the index entries once. That 
noticeably cuts down on the cached timings.

Best time before:

	[torvalds@woody linux]$ time git commit > /dev/null
	real    0m0.399s
	user    0m0.232s
	sys     0m0.164s

Best time after:

	[torvalds@woody linux]$ time git commit > /dev/null
	real    0m0.254s
	user    0m0.140s
	sys     0m0.112s

so it's a noticeable improvement in addition to being a nice conceptual 
cleanup (it's really not that pretty that "run_diff_index()" dirties the 
index!)

Doing an "strace -c" on it also shows that as it cuts the number of 
lstat() calls by two thirds, it goes from being lstat()-limited to being 
limited by getdents() (which is the readdir system call):

Before:
	% time     seconds  usecs/call     calls    errors syscall
	------ ----------- ----------- --------- --------- ----------------
	 60.69    0.000704           0     69230        31 lstat
	 23.62    0.000274           0      5522           getdents
	  8.36    0.000097           0      5508      2638 open
	  2.59    0.000030           0      2869           close
	  2.50    0.000029           0       274           write
	  1.47    0.000017           0      2844           fstat

After:
	% time     seconds  usecs/call     calls    errors syscall
	------ ----------- ----------- --------- --------- ----------------
	 45.17    0.000276           0      5522           getdents
	 26.51    0.000162           0     23112        31 lstat
	 19.80    0.000121           0      5503      2638 open
	  4.91    0.000030           0      2864           close
	  1.48    0.000020           0       274           write
	  1.34    0.000018           0      2844           fstat
	...

It passes the test-suite for me, but this is another of one of those 
really core functions, and certainly pretty subtle, so..

NOTE! The Linux lstat() system call is really quite cheap when everything 
is cached, so the fact that this is quite noticeable on Linux is likely to 
mean that it is *much* more noticeable on other operating systems. I bet 
you'll see a much bigger performance improvement from this on Windows in 
particular.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

This patch also includes the already sent-out trivial fix to 
run_diff_files to take advantage of ce_uptodate().


 diff-lib.c     |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 unpack-trees.h |    1 +
 wt-status.c    |    4 ---
 3 files changed, 59 insertions(+), 10 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 4a05b02..23d0fa6 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -9,6 +9,7 @@
 #include "revision.h"
 #include "cache-tree.h"
 #include "path-list.h"
+#include "unpack-trees.h"
 
 /*
  * diff-files
@@ -435,6 +436,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 				continue;
 		}
 
+		if (ce_uptodate(ce))
+			continue;
 		if (lstat(ce->name, &st) < 0) {
 			if (errno != ENOENT && errno != ENOTDIR) {
 				perror(ce->name);
@@ -665,12 +668,54 @@ static void mark_merge_entries(void)
 	}
 }
 
+static int oneway_diff(struct cache_entry **src,
+	struct unpack_trees_options *o,
+	int remove)
+{
+	struct cache_entry *idx = src[0];
+	struct cache_entry *tree = src[1];
+	struct rev_info *revs = o->unpack_data;
+
+	/*
+	 * Unpack-trees generates a DF/conflict entry if
+	 * there was a directory in the index and a tree
+	 * in the tree. From a diff standpoint, that's a
+	 * delete of the tree and a create of the file.
+	 */
+	if (tree == o->df_conflict_entry)
+		tree = NULL;
+
+	/*
+	 * Something added to the tree?
+	 */
+	if (!tree) {
+		if (ce_path_match(idx, revs->prune_data))
+			show_new_file(revs, idx, o->index_only, 0);
+		return 1;
+	}
+
+	/*
+	 * Something removed from the tree?
+	 */
+	if (!idx) {
+		if (ce_path_match(tree, revs->prune_data))
+			diff_index_show_file(revs, "-", tree, tree->sha1, tree->ce_mode);
+		return 0;
+	}
+
+	/* Show difference between old and new */
+	if (ce_path_match(idx, revs->prune_data))
+		show_modified(revs, tree, idx, 1, o->index_only, 0);
+	return 1;
+}
+
 int run_diff_index(struct rev_info *revs, int cached)
 {
-	int ret;
 	struct object *ent;
 	struct tree *tree;
 	const char *tree_name;
+	struct unpack_trees_options opts;
+	struct tree_desc t;
 	int match_missing = 0;
 
 	/*
@@ -687,13 +732,20 @@ int run_diff_index(struct rev_info *revs, int cached)
 	tree = parse_tree_indirect(ent->sha1);
 	if (!tree)
 		return error("bad tree object %s", tree_name);
-	if (read_tree(tree, 1, revs->prune_data))
-		return error("unable to read tree object %s", tree_name);
-	ret = diff_cache(revs, active_cache, active_nr, revs->prune_data,
-			 cached, match_missing);
+
+	memset(&opts, 0, sizeof(opts));
+	opts.head_idx = 1;
+	opts.index_only = cached;
+	opts.merge = 1;
+	opts.fn = oneway_diff;
+	opts.unpack_data = revs;
+
+	init_tree_desc(&t, tree->buffer, tree->size);
+	unpack_trees(1, &t, &opts);
+	
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
-	return ret;
+	return 0;
 }
 
 int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
diff --git a/unpack-trees.h b/unpack-trees.h
index 5517faa..197a004 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -25,6 +25,7 @@ struct unpack_trees_options {
 	int merge_size;
 
 	struct cache_entry *df_conflict_entry;
+	void *unpack_data;
 };
 
 extern int unpack_trees(unsigned n, struct tree_desc *t,
diff --git a/wt-status.c b/wt-status.c
index c0c2472..a18ed11 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -229,7 +229,6 @@ static void wt_status_print_initial(struct wt_status *s)
 	struct strbuf buf;
 
 	strbuf_init(&buf, 0);
-	wt_read_cache(s);
 	if (active_nr) {
 		s->commitable = 1;
 		wt_status_print_cached_header(s);
@@ -256,7 +255,6 @@ static void wt_status_print_updated(struct wt_status *s)
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.rename_limit = 100;
 	rev.diffopt.break_opt = 0;
-	wt_read_cache(s);
 	run_diff_index(&rev, 1);
 }
 
@@ -268,7 +266,6 @@ static void wt_status_print_changed(struct wt_status *s)
 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = wt_status_print_changed_cb;
 	rev.diffopt.format_callback_data = s;
-	wt_read_cache(s);
 	run_diff_files(&rev, 0);
 }
 
@@ -335,7 +332,6 @@ static void wt_status_print_verbose(struct wt_status *s)
 	setup_revisions(0, NULL, &rev, s->reference);
 	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
 	rev.diffopt.detect_rename = 1;
-	wt_read_cache(s);
 	run_diff_index(&rev, 1);
 
 	fflush(stdout);

Re: [PATCH] Avoid running lstat(2) on the same cache entry.

[Linus Torvalds]

On Sat, 19 Jan 2008, Linus Torvalds wrote:
> 
> It passes the test-suite for me, but this is another of one of those 
> really core functions, and certainly pretty subtle, so..

By this I don't mean that I don't think this patch is good, but it does 
mean that while I think it's a great patch (along with all the other index 
cleanups and the lstat-avoidance this one depends on), it's probably not 
appropriate for master without some more people looking at it, and 
probably cooking in a next for a while.

		Linus

Re: [PATCH] Avoid running lstat(2) on the same cache entry.

[Johannes Schindelin]

Hi,

On Sat, 19 Jan 2008, Linus Torvalds wrote:


> Best time before:
> 
> 	[torvalds@woody linux]$ time git commit > /dev/null
> 	real    0m0.399s
> 	user    0m0.232s
> 	sys     0m0.164s
> 
> Best time after:
> 
> 	[torvalds@woody linux]$ time git commit > /dev/null
> 	real    0m0.254s
> 	user    0m0.140s
> 	sys     0m0.112s

Wow.

> I bet you'll see a much bigger performance improvement from this on 
> Windows in particular.

I bet so, too.  Traditionally, filesystem calls are painfully slow on 
Windows.

But I cannot test before Monday, so I would not be mad if somebody else 
could perform some tests on Windows.

Now, the changes you made are quite a ways off of my current knowledge of 
git codepaths, so I cannot really comment.  Although they look sane 
enough.

> @@ -687,13 +732,20 @@ int run_diff_index(struct rev_info *revs, int cached)
>  	tree = parse_tree_indirect(ent->sha1);
>  	if (!tree)
>  		return error("bad tree object %s", tree_name);
> -	if (read_tree(tree, 1, revs->prune_data))
> -		return error("unable to read tree object %s", tree_name);
> -	ret = diff_cache(revs, active_cache, active_nr, revs->prune_data,
> -			 cached, match_missing);
> +
> +	memset(&opts, 0, sizeof(opts));
> +	opts.head_idx = 1;
> +	opts.index_only = cached;
> +	opts.merge = 1;
> +	opts.fn = oneway_diff;
> +	opts.unpack_data = revs;
> +
> +	init_tree_desc(&t, tree->buffer, tree->size);
> +	unpack_trees(1, &t, &opts);
> +	
>  	diffcore_std(&revs->diffopt);
>  	diff_flush(&revs->diffopt);
> -	return ret;
> +	return 0;
>  }
>  
>  int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)

What about do_diff_cache()?  Shouldn't it get exactly the same treatment 
as run_diff_index()?

In which case, do_diff_cache() would not be needed anymore, too.

> diff --git a/unpack-trees.h b/unpack-trees.h
> index 5517faa..197a004 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -25,6 +25,7 @@ struct unpack_trees_options {
>  	int merge_size;
>  
>  	struct cache_entry *df_conflict_entry;
> +	void *unpack_data;

Maybe this should be called "cb_data", in line with similar places in 
git.git?

> diff --git a/wt-status.c b/wt-status.c
> index c0c2472..a18ed11 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -229,7 +229,6 @@ static void wt_status_print_initial(struct wt_status *s)
>  	struct strbuf buf;
>  
>  	strbuf_init(&buf, 0);
> -	wt_read_cache(s);

If I'm not mistaken, you removed all four call sites of wt_read_cache(), 
and it is static anyway, so it should be removed, no?

Ciao,
Dscho "who will try to wrap his head around what read_tree() && 
diff_cache() are supposed to do"

Re: [PATCH] Avoid running lstat(2) on the same cache entry.

[Linus Torvalds]

On Sun, 20 Jan 2008, Johannes Schindelin wrote:
> 
> If I'm not mistaken, you removed all four call sites of wt_read_cache(), 
> and it is static anyway, so it should be removed, no?

Yes.

The wt_read_cache() changes are really independent, and while removing 
them was my original goal, that didn't end up being the interesting part 
of the patch, which is why I then left that part half-way..

		Linus

Re: [PATCH] Avoid running lstat(2) on the same cache entry.

[Johannes Schindelin]

Hi,

On Sat, 19 Jan 2008, Linus Torvalds wrote:

> @@ -687,13 +732,20 @@ int run_diff_index(struct rev_info *revs, int cached)
>  	tree = parse_tree_indirect(ent->sha1);
>  	if (!tree)
>  		return error("bad tree object %s", tree_name);
> -	if (read_tree(tree, 1, revs->prune_data))
> -		return error("unable to read tree object %s", tree_name);
> -	ret = diff_cache(revs, active_cache, active_nr, revs->prune_data,
> -			 cached, match_missing);
> +
> +	memset(&opts, 0, sizeof(opts));
> +	opts.head_idx = 1;
> +	opts.index_only = cached;
> +	opts.merge = 1;
> +	opts.fn = oneway_diff;
> +	opts.unpack_data = revs;
> +
> +	init_tree_desc(&t, tree->buffer, tree->size);
> +	unpack_trees(1, &t, &opts);
> +	
>  	diffcore_std(&revs->diffopt);
>  	diff_flush(&revs->diffopt);
> -	return ret;
> +	return 0;
>  }
>

Two problems I see (before I go to bed): match_missing is now ignored, and 
the return value is set to 0, whereas it was the return value of 
diff_cache() before.

The first is easily fixed, by replacing the two "0"s in the calls to 
show_new_file() and show_modified() with "!revs->ignore_merges".  Unless I 
am missing something, of course.

And the second is not _that_ bad, as it seems that diff_cache() has only 
one return statement, and it returns 0, AFAICS.  But it had to be said 
somewhere. 

Ciao,
Dscho

Re: [PATCH] Avoid running lstat(2) on the same cache entry.

[Steffen Prohaska]

On Jan 20, 2008, at 2:48 AM, Johannes Schindelin wrote:

>> I bet you'll see a much bigger performance improvement from this on
>> Windows in particular.
>
> I bet so, too.  Traditionally, filesystem calls are painfully slow on
> Windows.
>
> But I cannot test before Monday, so I would not be mad if somebody  
> else
> could perform some tests on Windows.

Has someone collected the whole series on a topic branch?  I did
not follow the discussion closely and apparently more than Linus'
patch is needed.  I couldn't immediately figure out which of the
patches from the thread should be applied in what order.

	Steffen

Re: [PATCH] Avoid running lstat(2) on the same cache entry.

[Johannes Schindelin]

Hi,

On Sun, 20 Jan 2008, Steffen Prohaska wrote:

> On Jan 20, 2008, at 2:48 AM, Johannes Schindelin wrote:
> 
> > > I bet you'll see a much bigger performance improvement from this on 
> > > Windows in particular.
> > 
> > I bet so, too.  Traditionally, filesystem calls are painfully slow on 
> > Windows.
> > 
> > But I cannot test before Monday, so I would not be mad if somebody 
> > else could perform some tests on Windows.
> 
> Has someone collected the whole series on a topic branch?  I did not 
> follow the discussion closely and apparently more than Linus' patch is 
> needed.  I couldn't immediately figure out which of the patches from the 
> thread should be applied in what order.

AFAICT Linus' patch relies on Junio's lstat() patch that was the OP in 
this thread.

FWIW I applied both patches to current "master" and pushed to the "lstat" 
branch of http://repo.or.cz/w/git/dscho.git

Hth,
Dscho

Re: [PATCH] Avoid running lstat(2) on the same cache entry.

[Steffen Prohaska]

On Jan 20, 2008, at 2:48 AM, Johannes Schindelin wrote:

> On Sat, 19 Jan 2008, Linus Torvalds wrote:
>
>
>> Best time before:
>>
>> 	[torvalds@woody linux]$ time git commit > /dev/null
>> 	real    0m0.399s
>> 	user    0m0.232s
>> 	sys     0m0.164s
>>
>> Best time after:
>>
>> 	[torvalds@woody linux]$ time git commit > /dev/null
>> 	real    0m0.254s
>> 	user    0m0.140s
>> 	sys     0m0.112s
>
> Wow.
>
>> I bet you'll see a much bigger performance improvement from this on
>> Windows in particular.
>
> I bet so, too.  Traditionally, filesystem calls are painfully slow on
> Windows.
>
> But I cannot test before Monday, so I would not be mad if somebody  
> else
> could perform some tests on Windows.

Here are timings for Windows XP running in a virtual machine on a
Laptop.  The work tree contains 7k files.  I stripped user and
sys times because they are apparently meaningless for MINGW.

Best time before:

     $ time git commit >/dev/null
     real    0m1.662s

Best time after:

     $ time git commit >/dev/null
     real    0m1.196s

The absolute time improvement is obviously larger, although the
relative improvement is slightly smaller than in Linus' example.


And here are the timings for the host system, which is Mac OS X,
on the same work tree.

Best time before:

     $ time git commit >/dev/null
     real    0m0.571s
     user    0m0.332s
     sys     0m0.237s

Best time after:

     $ time git commit >/dev/null
     real    0m0.463s
     user    0m0.273s
     sys     0m0.186s

Interestingly, the relative improvements are even smaller on Mac
OS X.

	Steffen

Re: [PATCH] Avoid running lstat(2) on the same cache entry.

[Johannes Schindelin]

Hi,

On Sun, 20 Jan 2008, Steffen Prohaska wrote:

> On Jan 20, 2008, at 2:48 AM, Johannes Schindelin wrote:
> 
> > On Sat, 19 Jan 2008, Linus Torvalds wrote:
> > 
> > > Best time before:
> > > 
> > > 	[torvalds@woody linux]$ time git commit > /dev/null
> > > 	real    0m0.399s
> > > 	user    0m0.232s
> > > 	sys     0m0.164s
> > > 
> > > Best time after:
> > > 
> > > 	[torvalds@woody linux]$ time git commit > /dev/null
> > > 	real    0m0.254s
> > > 	user    0m0.140s
> > > 	sys     0m0.112s
> > 
> > Wow.
> > 
> > > I bet you'll see a much bigger performance improvement from this on
> > > Windows in particular.
> > 
> > I bet so, too.  Traditionally, filesystem calls are painfully slow on
> > Windows.
> > 
> > But I cannot test before Monday, so I would not be mad if somebody else
> > could perform some tests on Windows.
> 
> Here are timings for Windows XP running in a virtual machine on a
> Laptop.  The work tree contains 7k files.  I stripped user and
> sys times because they are apparently meaningless for MINGW.
> 
> Best time before:
> 
>    $ time git commit >/dev/null
>    real    0m1.662s
> 
> Best time after:
> 
>    $ time git commit >/dev/null
>    real    0m1.196s
> 
> The absolute time improvement is obviously larger, although the
> relative improvement is slightly smaller than in Linus' example.
> 
> 
> And here are the timings for the host system, which is Mac OS X,
> on the same work tree.
> 
> Best time before:
> 
>    $ time git commit >/dev/null
>    real    0m0.571s
>    user    0m0.332s
>    sys     0m0.237s
> 
> Best time after:
> 
>    $ time git commit >/dev/null
>    real    0m0.463s
>    user    0m0.273s
>    sys     0m0.186s
> 
> Interestingly, the relative improvements are even smaller on Mac
> OS X.

Still, nothing to spit at.

FWIW I have made two patches which should be probably folded into Linus' 
patch; I will post them as a reply.

Ciao,
Dscho

[PATCH] Also use unpack_trees() in do_diff_cache()

[Johannes Schindelin]

As in run_diff_index(), we call unpack_trees() with the oneway_diff()
function in do_diff_cache() now.  This makes the function diff_cache()
obsolete.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

Obviously on top of Junio's + Linus' patch.

 diff-lib.c |   92 ++++++++---------------------------------------------------
 1 files changed, 13 insertions(+), 79 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index dc7b514..7941486 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -580,81 +580,6 @@ static int show_modified(struct rev_info *revs,
 	return 0;
 }
 
-static int diff_cache(struct rev_info *revs,
-		      struct cache_entry **ac, int entries,
-		      const char **pathspec,
-		      int cached, int match_missing)
-{
-	while (entries) {
-		struct cache_entry *ce = *ac;
-		int same = (entries > 1) && ce_same_name(ce, ac[1]);
-
-		if (DIFF_OPT_TST(&revs->diffopt, QUIET) &&
-			DIFF_OPT_TST(&revs->diffopt, HAS_CHANGES))
-			break;
-
-		if (!ce_path_match(ce, pathspec))
-			goto skip_entry;
-
-		switch (ce_stage(ce)) {
-		case 0:
-			/* No stage 1 entry? That means it's a new file */
-			if (!same) {
-				show_new_file(revs, ce, cached, match_missing);
-				break;
-			}
-			/* Show difference between old and new */
-			show_modified(revs, ac[1], ce, 1,
-				      cached, match_missing);
-			break;
-		case 1:
-			/* No stage 3 (merge) entry?
-			 * That means it's been deleted.
-			 */
-			if (!same) {
-				diff_index_show_file(revs, "-", ce,
-						     ce->sha1, ce->ce_mode);
-				break;
-			}
-			/* We come here with ce pointing at stage 1
-			 * (original tree) and ac[1] pointing at stage
-			 * 3 (unmerged).  show-modified with
-			 * report-missing set to false does not say the
-			 * file is deleted but reports true if work
-			 * tree does not have it, in which case we
-			 * fall through to report the unmerged state.
-			 * Otherwise, we show the differences between
-			 * the original tree and the work tree.
-			 */
-			if (!cached &&
-			    !show_modified(revs, ce, ac[1], 0,
-					   cached, match_missing))
-				break;
-			diff_unmerge(&revs->diffopt, ce->name,
-				     ntohl(ce->ce_mode), ce->sha1);
-			break;
-		case 3:
-			diff_unmerge(&revs->diffopt, ce->name,
-				     0, null_sha1);
-			break;
-
-		default:
-			die("impossible cache entry stage");
-		}
-
-skip_entry:
-		/*
-		 * Ignore all the different stages for this file,
-		 * we've handled the relevant cases now.
-		 */
-		do {
-			ac++;
-			entries--;
-		} while (entries && ce_same_name(ce, ac[0]));
-	}
-	return 0;
-}
-
 /*
  * This turns all merge entries into "stage 3". That guarantees that
  * when we read in the new tree (into "stage 1"), we won't lose sight
@@ -758,6 +683,8 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
 	int i;
 	struct cache_entry **dst;
 	struct cache_entry *last = NULL;
+	struct unpack_trees_options opts;
+	struct tree_desc t;
 
 	/*
 	 * This is used by git-blame to run diff-cache internally;
@@ -786,8 +713,15 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
 	tree = parse_tree_indirect(tree_sha1);
 	if (!tree)
 		die("bad tree object %s", sha1_to_hex(tree_sha1));
-	if (read_tree(tree, 1, opt->paths))
-		return error("unable to read tree %s", sha1_to_hex(tree_sha1));
-	return diff_cache(&revs, active_cache, active_nr, revs.prune_data,
-			  1, 0);
+
+	memset(&opts, 0, sizeof(opts));
+	opts.head_idx = 1;
+	opts.index_only = 1;
+	opts.merge = 1;
+	opts.fn = oneway_diff;
+	opts.unpack_data = &revs;
+
+	init_tree_desc(&t, tree->buffer, tree->size);
+	unpack_trees(1, &t, &opts);
+	return 0;
 }
-- 
1.5.4.rc3.44.g6cd4

[PATCH] Try to resurrect the handling for 'diff-index -m'

[Johannes Schindelin]

run_diff_index() had special handling for 'diff-index -m', which
was lost in the conversion to use unpack_trees() instead of
diff_cache().

---

Also on top of the patches of Junio and Linus.

I am not too certain about this; I just tried to reproduce what the old
code did.

 diff-lib.c |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 7941486..27032a9 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -614,11 +614,19 @@ static int oneway_diff(struct cache_entry **src,
 		tree = NULL;
 
 	/*
+	 * Backward compatibility wart - "diff-index -m" does
+	 * not mean "do not ignore merges", but totally different.
+	 * Therefore, the "match_missing" argument is set to
+	 * "!revs->ignore_merges".
+	 */
+
+	/*
 	 * Something added to the tree?
 	 */
 	if (!tree) {
 		if (ce_path_match(idx, revs->prune_data))
-			show_new_file(revs, idx, o->index_only, 0);
+			show_new_file(revs, idx, o->index_only,
+					!revs->ignore_merges);
 		return 1;
 	}
 
@@ -627,13 +635,15 @@ static int oneway_diff(struct cache_entry **src,
 	 */
 	if (!idx) {
 		if (ce_path_match(tree, revs->prune_data))
-			diff_index_show_file(revs, "-", tree, tree->sha1, tree->ce_mode);
+			diff_index_show_file(revs, "-",
+					tree, tree->sha1, tree->ce_mode);
 		return 0;
 	}
 
 	/* Show difference between old and new */
 	if (ce_path_match(idx, revs->prune_data))
-		show_modified(revs, tree, idx, 1, o->index_only, 0);
+		show_modified(revs, tree, idx, 1, o->index_only,
+				!revs->ignore_merges);
 	return 1;
 }
 
@@ -644,14 +654,6 @@ int run_diff_index(struct rev_info *revs, int cached)
 	const char *tree_name;
 	struct unpack_trees_options opts;
 	struct tree_desc t;
-	int match_missing = 0;
-
-	/*
-	 * Backward compatibility wart - "diff-index -m" does
-	 * not mean "do not ignore merges", but totally different.
-	 */
-	if (!revs->ignore_merges)
-		match_missing = 1;
 
 	mark_merge_entries();
 
-- 
1.5.4.rc3.44.g6cd4

Re: [PATCH] Avoid running lstat(2) on the same cache entry.

[Linus Torvalds]

On Sun, 20 Jan 2008, Steffen Prohaska wrote:
> 
> Here are timings for Windows XP running in a virtual machine on a
> Laptop.  The work tree contains 7k files.  I stripped user and
> sys times because they are apparently meaningless for MINGW.
> 
> Best time before:
> 
>    $ time git commit >/dev/null
>    real    0m1.662s
> 
> Best time after:
> 
>    $ time git commit >/dev/null
>    real    0m1.196s

Ok, very interesting.

That's a 28% reduction in running time (or, alternatively, if you want to 
make it sound better, a 39% performance improvement ;), but quite frankly, 
I was hoping for even more. On my tests (with the kernel tree), the patch 
series literally removed half of the system calls, and I was really hoping 
that performance would be almost linear in system calls, especially on 
Windows where they are more expensive.

But I guess that was a bit optimistic. On Linux, we still had about 50% 
user time, and I guess the same is _largely_ true on Windows. Cutting the 
system time in half only gives you about a quarter performance 
improvement, and I was just incorrect in expecting system calls to be the 
limiting factor.

That's still a nice performance improvement, of course, but I was really 
hoping for more, since people have claimed that git is slow on windows. 
Maybe that's simply not true any more now that (a) people use the thinner 
mingw layers and (b) many things - like git-commit - are built-in, so that 
there simply isn't the horrible process creation overhead.

I have absolutely no idea how to do performance analysis or even something 
simple as getting a list of system calls from Windows (even if I had a 
Windows machine, which I obviously don't ;), so I'm afraid I have no clue 
why git might be considered slow there. I was hoping this was it

> And here are the timings for the host system, which is Mac OS X,
> on the same work tree.
> 
> Best time before:
> 
>    $ time git commit >/dev/null
>    real    0m0.571s
>    user    0m0.332s
>    sys     0m0.237s
> 
> Best time after:
> 
>    $ time git commit >/dev/null
>    real    0m0.463s
>    user    0m0.273s
>    sys     0m0.186s
> 
> Interestingly, the relative improvements are even smaller on Mac
> OS X.

Well, you're testing something with 7k files, and I was testing something 
with 23k files. The changes in question only affect the number of lstat's 
on those files, they don't affect things like the basic setup overhead we 
have for doign things like checking that refs are unique.

So your test-case does have relatively more overhead (compared to what got 
optimized) than the numbers I quoted. 

We also do know that while Linux has a very low-overhead lstat(), the real 
problem on OS X has been the memory management, not the filesystem. We had 
some case where the page fault overhead was something like two orders of 
magnitude bigger, didn't we?

(Yeah - just checked. Commit 6d2fa7f1b489c65e677c18eda5c144dbc5d614ab: 
"index-pack usage of mmap() is unacceptably slower.."). That took a minute 
on Linux, and an hour on OS X)

It would be good to have a system-level profile of something like this. On 
Linux, it's now mostly in user space with "git commit", and oprofile 
shows:

	samples  %        image name               app name                 symbol name
	23318    11.5388  git                      git                      cache_name_compare
	11910     5.8936  git                      git                      excluded
	9775      4.8371  libcrypto.so.0.9.8b      libcrypto.so.0.9.8b      (no symbols)
	7990      3.9538  libc-2.7.so              libc-2.7.so              strlen
	7468      3.6955  vmlinux                  vmlinux                  __d_lookup
	7137      3.5317  libc-2.7.so              libc-2.7.so              internal_fnmatch
	7047      3.4872  libz.so.1.2.3            libz.so.1.2.3            (no symbols)
	6317      3.1259  git                      git                      index_name_pos
	5537      2.7400  git                      git                      unpack_trees_rec
	5079      2.5133  vmlinux                  vmlinux                  ext3fs_dirhash
	4774      2.3624  libc-2.7.so              libc-2.7.so              strcmp
	4005      1.9819  vmlinux                  vmlinux                  __link_path_walk
	3436      1.7003  vmlinux                  vmlinux                  ext3_htree_store_dirent
	3421      1.6929  vmlinux                  vmlinux                  __kmalloc
	3188      1.5776  vmlinux                  vmlinux                  _atomic_dec_and_lock
	3041      1.5048  libc-2.7.so              libc-2.7.so              _int_malloc
	2904      1.4370  libc-2.7.so              libc-2.7.so              fnmatch@@GLIBC_2.2.5
	2727      1.3494  vmlinux                  vmlinux                  str2hashbuf
	...

and one thing to look out for would be that some of these things might be 
relatively much more costly on other systems.

fnmatch? It's certainly visible on Linux, I wonder if the Windows/OSX 
version is more expensive due to trying to be case-insensitive or 
something?

				Linus

Re: [PATCH] Avoid running lstat(2) on the same cache entry.

[Brian Downing]

On Sun, Jan 20, 2008 at 10:20:46AM -0800, Linus Torvalds wrote:
> But I guess that was a bit optimistic. On Linux, we still had about 50% 
> user time, and I guess the same is _largely_ true on Windows. Cutting the 
> system time in half only gives you about a quarter performance 
> improvement, and I was just incorrect in expecting system calls to be the 
> limiting factor.
> 
> [...]
> 
> fnmatch? It's certainly visible on Linux, I wonder if the Windows/OSX 
> version is more expensive due to trying to be case-insensitive or 
> something?

Windows (msys) Git, with the lstat patch and on my 15433-file test case,
is at least 90-95% system time.  I don't have any good tools for
measuring this, but just using the silly performance monitor you get
with ctrl-alt-del and enabling the "View -> Show Kernel Times" curve on
the graph shows this fact pretty clearly.

I think this shows that:

1. There's not much overhead in msysGit's stat wrapper at all.  It gets
   converted quite efficiently to native Windows calls.

2. Windows really sucks at filesystem ops.  Film at 11.

One thing I dimly recall is that the nightmare interface of
FindFirstFile/FindNextFile (which I assume is what is actually getting
called for opendir/readdir) actually returns some stat information with
each file.  Would it be possible to use that directly in some cases
rather than taking an extra stat call to get it?

-bcd

Re: [PATCH] Also use unpack_trees() in do_diff_cache()

[Linus Torvalds]

On Sun, 20 Jan 2008, Johannes Schindelin wrote:
> 
> As in run_diff_index(), we call unpack_trees() with the oneway_diff()
> function in do_diff_cache() now.  This makes the function diff_cache()
> obsolete.

Sadly, this doesn't work yet.

diiff_cache() does magic things with different stages. 

The new unpack_trees() model *should* make it possible to do an even 
better job at this (we should really approximate the diff_files() 
behaviour), but oneway_diff() currently doesn't do it right.

Try

	git diff-index [-p] [--cached] -m HEAD

in a tree with unmerged entries (or just "git diff HEAD", which does the 
same thing).

The following patch gets us closer, but really only for the "--cached" 
case (for the non-cached case, it should use the working tree entry ratehr 
than generate a unmerged entry), and I also suspect there's a case it gets 
wrong for the case of the unmerged path not existign in the tree at all 
(the "if (tree)" entry basically ends up being a stand-in for the "is this 
the first index entry for this path we see" case).

The really nice thing would be to do what "diff_files()" does for this 
all, something that the old model couldn't do at all (that's why it 
currently prints out that "* Unmerged path xyzzy" thing - the *correct* 
thing to do would be to generate the same combined diff that diff_files() 
does).

So some more work is needed - either to get back the old (arguably 
quite broken) behaviour, or to go the whole way and do it right.

Anyway, my point with this all is that even without any of this set of 
fixes, git doesn't do this optimally. Try this with the current stable git 
master:

	git diff --cached HEAD

with a unmerged file, and compare it to the much more useful "git diff" 
output. But right now, this patch series makes this case behave even 
worse, I think.

		Linus

---
 diff-lib.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 23d0fa6..3ac4d1f 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -685,6 +685,12 @@ static int oneway_diff(struct cache_entry **src,
 	if (tree == o->df_conflict_entry)
 		tree = NULL;
 
+	if (ce_stage(idx)) {
+		if (tree)
+			diff_unmerge(&revs->diffopt, idx->name, idx->ce_mode, idx->sha1);
+		return 1;
+	}
+
 	/*
 	 * Something added to the tree?
 	 */

Re: [PATCH] Avoid running lstat(2) on the same cache entry.

[Steffen Prohaska]

On Jan 20, 2008, at 7:20 PM, Linus Torvalds wrote:

>
> On Sun, 20 Jan 2008, Steffen Prohaska wrote:
>
> So your test-case does have relatively more overhead (compared to  
> what got
> optimized) than the numbers I quoted.

I cloned the kernel tree and measured these numbers:

Best time before:

     $ time git commit >/dev/null
     real    0m1.487s
     user    0m0.786s
     sys     0m0.696s

Best time after:

     $ time git commit >/dev/null
     real    0m1.087s
     user    0m0.578s
     sys     0m0.508s


> We also do know that while Linux has a very low-overhead lstat(),  
> the real
> problem on OS X has been the memory management, not the filesystem.  
> We had
> some case where the page fault overhead was something like two  
> orders of
> magnitude bigger, didn't we?
>
> (Yeah - just checked. Commit 6d2fa7f1b489c65e677c18eda5c144dbc5d614ab:
> "index-pack usage of mmap() is unacceptably slower.."). That took a  
> minute
> on Linux, and an hour on OS X)
>
> It would be good to have a system-level profile of something like  
> this. On
> Linux, it's now mostly in user space with "git commit", and oprofile
> shows:

[...]

I used Mac OS X's Sampler to collect some statistics.

It took me some time to find out about and work around an
annoying limitation.  Sampler is not capable of simply sampling a
full run of a process but needs to be manually stopped before the
process terminates.  You literally have to push a button before
the process exits--what a crazy tool.  I worked around this by
adding a sleep(100) instruction.  Sampler allows you to prune the
samples collected in mach_wait_until() and the relative numbers
should be fine.

Here is what I have.

Before Junio's and your patch, lstat dominates with 25%:

# Report - Timed Samples for git-commit
# Samples < 20 have been filtered
# Generated from the visible portion of the outline view
+ 25% lstat
| + 25% lstat
| | - 17% wt_status_print
| | - 8.4% refresh_index
+ 16% getdirentries
| + 16% getdirentries
| | - 16% read_directory_recursive
+ 10% fnmatch
| + 10% fnmatch
| | + 10% excluded
| | | - 10% read_directory_recursive
+ 9.7% sha1_block_data_order
| + 9.7% sha1_block_data_order
| | + 9.7% SHA1_Update
| | | + 8.0% read_index_from
| | | | - 6.8% wt_read_cache
| | | - 1.6% ce_write
+ 8.6% cache_name_compare
| + 8.6% cache_name_compare
| | - 6.9% cmp_cache_name_compare
| | - 1.6% index_name_pos
- 5.2% __mmap
- 3.2% open
+ 2.0% excluded
| + 2.0% excluded
| | - 2.0% read_directory_recursive
- 1.6% mbrtowc_l
- 1.5% find_pack_entry_one
- 1.4% cmp_cache_name_compare
- 1.3% qsort



After your patches getdirentries and fnmatch dominate:

# Report - Timed Samples for git-commit
# Samples < 20 have been filtered
# Generated from the visible portion of the outline view
+ 29% getdirentries
| + 29% getdirentries
| | - 29% read_directory_recursive
+ 19% fnmatch
| + 19% fnmatch
| | + 19% excluded
| | | - 19% read_directory_recursive
+ 9.6% lstat
| + 9.6% lstat
| | - 9.5% refresh_index
+ 5.5% open
| + 5.5% open
| | + 3.9% excluded
| | | - 3.9% read_directory_recursive
| | - 1.6% read_directory_recursive
+ 3.9% cache_name_compare
| + 3.9% cache_name_compare
| | - 3.8% index_name_pos
+ 3.0% sha1_block_data_order
| + 3.0% sha1_block_data_order
| | + 3.0% SHA1_Update
| | | - 1.6% read_index_from
+ 2.8% excluded
| + 2.8% excluded
| | - 2.8% read_directory_recursive
- 2.5% __memcpy
- 1.9% strcmp
- 1.6% close
- 1.4% fnmatch1


The reports above were created with Sampler's "Invert call tree"
option, while the next one is generated without this option.

# Report - Timed Samples for git-commit
# Samples < 20 have been filtered
# Generated from the visible portion of the outline view
+ 85% run_status
| + 85% wt_status_print
| | + 73% read_directory
| | | + 73% read_directory_recursive
| | | | + 73% read_directory_recursive
| | | | | + 71% read_directory_recursive
| | | | | | + 44% read_directory_recursive
| | | | | | | + 15% excluded
| | | | | | | | - 12% fnmatch
| | | | | | | | - 1.6% open
| | | | | | | - 12% getdirentries
| | | | | | | + 10% read_directory_recursive
| | | | | | | | - 4.0% getdirentries
| | | | | | | | - 2.6% excluded
| | | | | | | |   1.7% read_directory_recursive
| | | | | | | - 2.6% dir_add_name
| | | | | | + 14% excluded
| | | | | | | - 11% fnmatch
| | | | | | - 8.0% getdirentries
| | | | | | - 1.6% dir_add_name
| | - 11% run_diff_index
+ 15% prepare_index
| + 10% refresh_index
| | - 9.7% lstat
| - 2.5% write_index
| - 1.9% read_index

With this kind of report you can see how the running time is
distributed over the different functions called by cmd_commit().


> and one thing to look out for would be that some of these things  
> might be
> relatively much more costly on other systems.
>
> fnmatch? It's certainly visible on Linux, I wonder if the Windows/OSX
> version is more expensive due to trying to be case-insensitive or
> something?

So roughly 50% of the running time is spent in getdirentries
and fnmatch on the MacBook Pro I used to run these tests.

	Steffen

Re: [PATCH] Also use unpack_trees() in do_diff_cache()

[Linus Torvalds]

On Sun, 20 Jan 2008, Linus Torvalds wrote:
> 
> The following patch gets us closer, but really only for the "--cached" 
> case (for the non-cached case, it should use the working tree entry ratehr 
> than generate a unmerged entry), and I also suspect there's a case it gets 
> wrong for the case of the unmerged path not existign in the tree at all 
> (the "if (tree)" entry basically ends up being a stand-in for the "is this 
> the first index entry for this path we see" case).

This cleanup patch really looks big and fairly ugly, but it splits up the 
magic rules for "unpack_trees()" and adds a lot of comments about what is 
going on, and in the presense lays the groundwork for doing a much better 
job on unmerged entries - it now keeps track of how many index entries we 
have that match the tree entry we found, so it should be pretty trivial 
to now add the logic to do a combined diff..

It replaces the previous patch, and should fix both of the issues I 
mention above. It adds a lot more lines than it removes, but much of that 
is due to some comments and a lot cleaner abstractions, so that the 
resulting code is, I think, quite a bit more obvious.

The unpack_trees() interface really isn't trivial (it can't be: the things 
we need to do with the index for a merge - at the same time as traversing 
it! - are quite involved). So clarifying all that is definitely worth it.

With this patch in place, I think Dscho's two other patches are now good 
to go, and we now generate the same output we used to. In addition, we now 
have the infrastructure to generate *better* output, so if we want to make 
"git diff HEAD" generate a nice combined diff, this sets the stage for 
that.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 diff-lib.c |  112 ++++++++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 90 insertions(+), 22 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 23d0fa6..2a3a9ff 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -668,45 +668,113 @@ static void mark_merge_entries(void)
 	}
 }
 
-static int oneway_diff(struct cache_entry **src,
-	struct unpack_trees_options *o,
-	int remove)
+/*
+ * This gets a mix of an existing index and a tree, one pathname entry
+ * at a time. The index entry may be a single stage-0 one, but it could
+ * also be multiple unmerged entries (in which case you idx_pos/idx_nr
+ * will give you the position and number of entries in the index).
+ */
+static void do_oneway_diff(struct unpack_trees_options *o,
+	struct cache_entry *idx,
+	struct cache_entry *tree,
+	int idx_pos, int idx_nr)
 {
-	struct cache_entry *idx = src[0];
-	struct cache_entry *tree = src[1];
 	struct rev_info *revs = o->unpack_data;
 
-	/*
-	 * Unpack-trees generates a DF/conflict entry if
-	 * there was a directory in the index and a tree
-	 * in the tree. From a diff standpoint, that's a
-	 * delete of the tree and a create of the file.
-	 */
-	if (tree == o->df_conflict_entry)
-		tree = NULL;
+	if (o->index_only && idx && ce_stage(idx)) {
+		if (tree)
+			diff_unmerge(&revs->diffopt, idx->name, idx->ce_mode, idx->sha1);
+		return;
+	}
 
 	/*
 	 * Something added to the tree?
 	 */
 	if (!tree) {
-		if (ce_path_match(idx, revs->prune_data))
-			show_new_file(revs, idx, o->index_only, 0);
-		return 1;
+		show_new_file(revs, idx, o->index_only, 0);
+		return;
 	}
 
 	/*
 	 * Something removed from the tree?
 	 */
 	if (!idx) {
-		if (ce_path_match(tree, revs->prune_data))
-			diff_index_show_file(revs, "-", tree, tree->sha1, tree->ce_mode);
-		return 0;
+		diff_index_show_file(revs, "-", tree, tree->sha1, tree->ce_mode);
+		return;
 	}
 
 	/* Show difference between old and new */
-	if (ce_path_match(idx, revs->prune_data))
-		show_modified(revs, tree, idx, 1, o->index_only, 0);
-	return 1;
+	show_modified(revs, tree, idx, 1, o->index_only, 0);
+}
+
+/*
+ * Count how many index entries go with the first one
+ */
+static inline int count_skip(const struct cache_entry *src, int pos)
+{
+	int skip = 1;
+
+	/* We can only have multiple entries if the first one is not stage-0 */
+	if (ce_stage(src)) {
+		struct cache_entry **p = active_cache + pos;
+		int namelen = ce_namelen(src);
+
+		for (;;) {
+			const struct cache_entry *ce;
+			pos++;
+			if (pos >= active_nr)
+				break;
+			ce = *++p;
+			if (ce_namelen(ce) != namelen)
+				break;
+			if (memcmp(ce->name, src->name, namelen))
+				break;
+			skip++;
+		}
+	}
+	return skip;
+}
+
+/*
+ * The unpack_trees() interface is designed for merging, so
+ * the different source entries are designed primarily for
+ * the source trees, with the old index being really mainly
+ * used for being replaced by the result.
+ *
+ * For diffing, the index is more important, and we only have a
+ * single tree.  
+ *
+ * We're supposed to return how many index entries we want to skip.
+ *
+ * This wrapper makes it all more readable, and takes care of all
+ * the fairly complex unpack_trees() semantic requirements, including
+ * the skipping, the path matching, the type conflict cases etc.
+ */
+static int oneway_diff(struct cache_entry **src,
+	struct unpack_trees_options *o,
+	int index_pos)
+{
+	int skip = 0;
+	struct cache_entry *idx = src[0];
+	struct cache_entry *tree = src[1];
+	struct rev_info *revs = o->unpack_data;
+
+	if (index_pos >= 0)
+		skip = count_skip(idx, index_pos);
+
+	/*
+	 * Unpack-trees generates a DF/conflict entry if
+	 * there was a directory in the index and a tree
+	 * in the tree. From a diff standpoint, that's a
+	 * delete of the tree and a create of the file.
+	 */
+	if (tree == o->df_conflict_entry)
+		tree = NULL;
+
+	if (ce_path_match(idx ? idx : tree, revs->prune_data))
+		do_oneway_diff(o, idx, tree, index_pos, skip);
+
+	return skip;
 }
 
 int run_diff_index(struct rev_info *revs, int cached)

Re: [PATCH] Avoid running lstat(2) on the same cache entry.

[Linus Torvalds]

On Sun, 20 Jan 2008, Steffen Prohaska wrote:
> > 
> > fnmatch? It's certainly visible on Linux, I wonder if the Windows/OSX
> > version is more expensive due to trying to be case-insensitive or
> > something?
> 
> So roughly 50% of the running time is spent in getdirentries
> and fnmatch on the MacBook Pro I used to run these tests.

Ok. That seems to explain it (I have some trouble reading/comparing your 
profiles against mine, but yeah, fnmatch and getdirentries seem to be 
excessive from that "invert call tree" thing.

On Linux, the fnmatch and readdir overhead is way down in the noise both 
before and after the lstat removal. So removing the lstat's - which were 
fairly cheap, but there were _lots_ of them, made more of a relative 
difference on Linux.

I don't know what can be done about that on OS X. We really can't avoid 
reading the directory tree and matching it against ignore files. That's 
very integral for the status generation of "git commit", after all.

Maybe there are better fnmatch libraries for OS X? But getdirentries() was 
the bigger cost, and I don't see any alternative for that.

			Linus

Re: [PATCH] Also use unpack_trees() in do_diff_cache()

[Johannes Schindelin]

Hi,

On Sun, 20 Jan 2008, Linus Torvalds wrote:

> In addition, we now have the infrastructure to generate *better* output, 
> so if we want to make "git diff HEAD" generate a nice combined diff, 
> this sets the stage for that.

Note: "git diff HEAD" as it is now still holds value; it tells you about 
_all_ changes -- staged and unstaged -- and I have to admit that I used it 
a lot (as "git diff HEAD -- file | git apply -R"; note the absence of 
"--index" to the call to apply, otherwise "git checkout file" would have 
done the same).

So if we ever do combined diffs for "git diff HEAD", I want an option to 
turn them off, too.

Ciao,
Dscho

Re: [PATCH] Also use unpack_trees() in do_diff_cache()

[Linus Torvalds]

On Sun, 20 Jan 2008, Johannes Schindelin wrote:
> 
> Note: "git diff HEAD" as it is now still holds value;

Oh, absolutely.

It's not "git diff HEAD" that is broken.

It's "git diff --cached HEAD" that doesn't work. The "--cached" means that 
it's supposed to diff the index against HEAD, but since it cannot handle 
unmerged entries, instead of getting a diff, you get just a line saying

	* Unmerged path xyzzy

and no diff at all.

That's the thing we should think about improving on (although it's not 
100% clear that a combined diff is the rigth thing either).

Anyway, to make it easier for people to follow along with this, I've put 
my whole series now on kernel.org in a "new-lstat" branch at:

	git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/git.git new-lstat

(That git repo is a bit odd, it doesn't have any "master" branch at all, 
just that new-lstat one. I didn't want anybody to think that it's 
anything but a temporary git tree for this one series of patches. It 
will clone into an odd kind of repo that only has a "origin/new-lstat" 
remote branch, nothing else).

I'll probably rebase that thing if I keep working on it, but I think it's 
pretty good.

		Linus

Re: [PATCH] Avoid running lstat(2) on the same cache entry.

[Junio C Hamano]

Steffen Prohaska <prohaska@zib.de> writes:

> On Jan 20, 2008, at 2:48 AM, Johannes Schindelin wrote:
>
>>> I bet you'll see a much bigger performance improvement from this on
>>> Windows in particular.
>>
>> I bet so, too.  Traditionally, filesystem calls are painfully slow on
>> Windows.
>>
>> But I cannot test before Monday, so I would not be mad if somebody
>> else
>> could perform some tests on Windows.
>
> Has someone collected the whole series on a topic branch?

Yes, I have.

Haven't pushed it out, though.

Re: [PATCH] Also use unpack_trees() in do_diff_cache()

[Johannes Schindelin]

Hi,

On Sun, 20 Jan 2008, Linus Torvalds wrote:

> On Sun, 20 Jan 2008, Johannes Schindelin wrote:
> > 
> > Note: "git diff HEAD" as it is now still holds value;
> 
> Oh, absolutely.
> 
> It's not "git diff HEAD" that is broken.
> 
> It's "git diff --cached HEAD" that doesn't work. The "--cached" means 
> that it's supposed to diff the index against HEAD, but since it cannot 
> handle unmerged entries, instead of getting a diff, you get just a line 
> saying
> 
> 	* Unmerged path xyzzy
> 
> and no diff at all.

Silly me.  Unmerged paths can only have problems when you look at the 
index, of course.

Sorry for the noise,
Dscho

[PATCH] index: be careful when handling long names

[Junio C Hamano]

This is a follow-up to 7fec10b (index: be careful when handling long
names, 2008-01-18) where we fixed handling of index entries with long
names.  There still remained a few remaining unsafe codepaths.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-apply.c        |    2 +-
 builtin-update-index.c |    2 +-
 read-cache.c           |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index e15471b..f2377fe 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2758,7 +2758,7 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned
 	ce = xcalloc(1, ce_size);
 	memcpy(ce->name, path, namelen);
 	ce->ce_mode = create_ce_mode(mode);
-	ce->ce_flags = namelen;
+	ce->ce_flags = create_ce_flags(namelen, 0);
 	if (S_ISGITLINK(mode)) {
 		const char *s = buf;
 
diff --git a/builtin-update-index.c b/builtin-update-index.c
index 38eb53c..003cb98 100644
--- a/builtin-update-index.c
+++ b/builtin-update-index.c
@@ -95,7 +95,7 @@ static int add_one_path(struct cache_entry *old, const char *path, int len, stru
 	size = cache_entry_size(len);
 	ce = xcalloc(1, size);
 	memcpy(ce->name, path, len);
-	ce->ce_flags = len;
+	ce->ce_flags = create_ce_flags(len, 0);
 	fill_stat_cache_info(ce, st);
 	ce->ce_mode = ce_mode_from_stat(old, st->st_mode);
 
diff --git a/read-cache.c b/read-cache.c
index 1648428..2dd8131 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -498,7 +498,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 	size = cache_entry_size(namelen);
 	ce = xcalloc(1, size);
 	memcpy(ce->name, path, namelen);
-	ce->ce_flags = namelen;
+	ce->ce_flags = create_ce_flags(namelen, 0);
 	fill_stat_cache_info(ce, st);
 
 	if (trust_executable_bit && has_symlinks)