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   ...
[ message continues ]

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_...
[ message continues ]

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);

-

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     call...
[ message continues ]

Hi,
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
-

Hi,
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 
What about do_diff_cache()?  Shouldn't it get exactly the same treatment

as run_diff_index()?
Maybe this should be called "cb_data", in line with similar places in 
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"

-

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

-

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 
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. Comm...
[ message continues ]

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
[...]
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_di...
[ message continues ]

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

-

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

-

Hi,
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
-

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;
 ...
[ message continues ]

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

-			 * t...
[ message continues ]

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_unm...
[ message continues ]

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 ...
[ message continues ]

Hi,
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
-

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

-

Hi,
Silly me.  Unmerged paths can only have problems when you look at the

index, of course.
Sorry for the noise,

Dscho

-

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
-

Yes, I have.
Haven't pushed it out, though.
-

Hi,
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
-

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

-

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

-

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

-

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))

...
[ message continues ]