{"thread":{"id":"11601","subject":"performance problem: \"git commit filename\"","startedAt":"2008-01-12T22:46:08Z","lastAt":"2008-01-15T01:13:52Z","messageCount":33,"participants":["Linus Torvalds","Daniel Barkalow","Junio C Hamano","Alex Riesen","しらいしななこ","Kristian Høgsberg"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"65221","messageId":"alpine.LFD.1.00.0801121426510.2806@woody.linux-foundation.org","threadId":"11601","inReplyTo":null,"subject":"performance problem: \"git commit filename\"","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-12T22:46:08Z","receivedAt":"2008-01-12T22:46:08Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nI thought we had fixed this long long ago, but if we did, it has \nre-surfaced.\n\nUsing an explicit filename with \"git commit\" is _extremely_ slow. Lookie \nhere:\n\n\t[torvalds@woody linux]$ time git commit fs/exec.c\n\tno changes added to commit (use \"git add\" and/or \"git commit -a\")\n\n\treal    0m1.671s\n\tuser    0m1.200s\n\tsys     0m0.328s\n\nthat's closer to two seconds on a fast machine, with the whole tree \ncached!\n\nAnd for the uncached case, it's just unbearably slow: two and a half \n*minutes*.\n\nIn contrast, without the filename, it's much faster:\n\n\t[torvalds@woody linux]$ time git commit\n\tno changes added to commit (use \"git add\" and/or \"git commit -a\")\n\t\n\treal    0m0.387s\n\tuser    0m0.220s\n\tsys     0m0.168s\n\nwith the cold-cache case now being \"just\" 18s (which is still long, but \nwe're talking eight times faster, and certainly not unbearable!)\n\nDoing an \"strace -c\" on the thing shows why. In the filename case, we \nhave:\n\n\t% time     seconds  usecs/call     calls    errors syscall\n\t------ ----------- ----------- --------- --------- ----------------\n\t 32.69    0.000868           0     92299        37 lstat\n\t 17.40    0.000462           0     29958      3993 open\n\t 15.78    0.000419           0      5522           getdents\n\t 15.56    0.000413           0     23165           mmap\n\t 11.37    0.000302           0     23118           munmap\n\t  5.76    0.000153           0     25966         2 close\n\t  1.43    0.000038           0      2845           fstat\n\t...\n\nand in the non-filename case we have\n\n\t% time     seconds  usecs/call     calls    errors syscall\n\t------ ----------- ----------- --------- --------- ----------------\n\t 53.67    0.000600           0     69227        31 lstat\n\t 23.35    0.000261           0      5522           getdents\n\t 11.09    0.000124           2        55           munmap\n\t  4.20    0.000047           0       285           write\n\t  3.31    0.000037           0      5537      2638 open\n\t  2.33    0.000026           0      2899         1 close\n\t  2.06    0.000023           0      2844           fstat\n\t...\n\nnotice how the expensive case has a lot of successful open/mmap/munmap \ncalls: it is *literally* ignoring the valid entries in the old index \nentirely, and re-hashing every single file in the tree! No wonder it is \nslow!\n\nJust counting \"lstat()\" calls, it's worth noticing that the non-filename \ncase seems to do three lstat's for each index entry (and yes, that's two\ntoo many), but the named file case has upped that to *four* lstats per \nentry, and then added the one open/mmap/munmap/close on top of that!\n\nI'm pretty sure we didn't use to do things this badly. And if this is a \nregression like I think it is, it should be fixed before a real 1.5.4 \nrelease.\n\nI'll try to see if I can see what's up, but I thought I'd better let \nothers know too, in case I don't have time. I *suspect* (but have nothing \nwhat-so-ever to back that up) that this happened as part of making commit \na builtin.\n\n\t\t\tLinus\n"},{"id":"65224","messageId":"alpine.LFD.1.00.0801121735020.2806@woody.linux-foundation.org","threadId":"11601","inReplyTo":"alpine.LFD.1.00.0801121426510.2806@woody.linux-foundation.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-13T01:46:20Z","receivedAt":"2008-01-13T01:46:20Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 12 Jan 2008, Linus Torvalds wrote:\n> \n> I thought we had fixed this long long ago, but if we did, it has \n> re-surfaced.\n\nIt's new, and yes, it seems to be due to the new builtin-commit.c.\n\nI think I know what is going on.\n\nIn the old git-commit.sh, this case used to be handled with\n\n\tTMP_INDEX=\"$GIT_DIR/tmp-index$$\"\n\n\tGIT_INDEX_FILE=\"$THIS_INDEX\" \\\n\tgit read-tree --index-output=\"$TMP_INDEX\" -i -m HEAD\n\nwhich is a one-way merge of the *old* index and HEAD, taking the index \ninformation from the old index, but the actual file information from HEAD \n(to then later be updated by the named files).\n\nThis logic is implemented by builtin-read-tree.c with\n\n\tstruct unpack_trees_options opts;\n\t..\n\topts.fn = oneway_merge;\n\t..\n\tunpack_trees(nr_trees, t, &opts);\n\nwhere all the magic is done by that \"oneway_merge()\" function being called \nfor each entry by unpack_trees(). This does everything right, and the \nresult is that any index entry that was up-to-date in the old index and \nunchanged in the base tree will be up-to-date in the new index too\n\nHOWEVER. When that logic was converted from that shell-script into a \nbuiltin-commit.c, that conversion was not done correctly. The old \"git \nread-tree -i -m\" was not translated as a \"unpack_trees()\" call, but as \nthis in prepare_index():\n\n\tdiscard_cache()\n\t..\n\ttree = parse_tree_indirect(head_sha1);\n\t..\n\tread_tree(tree, 0, NULL)\n\nwhich is very wrong, because it replaces the old index entirely, and \ndoesn't do that stat information merging.\n\nAs a result, the index that is created by read-tree is totally bogus in \nthe stat cache, and yes, everything will have to be re-computed.\n\nKristian?\n\n\t\t\tLinus\n"},{"id":"65225","messageId":"alpine.LFD.1.00.0801121949180.2806@woody.linux-foundation.org","threadId":"11601","inReplyTo":"alpine.LFD.1.00.0801121735020.2806@woody.linux-foundation.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-13T04:04:22Z","receivedAt":"2008-01-13T04:04:22Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 12 Jan 2008, Linus Torvalds wrote:\n> \n> HOWEVER. When that logic was converted from that shell-script into a \n> builtin-commit.c, that conversion was not done correctly. The old \"git \n> read-tree -i -m\" was not translated as a \"unpack_trees()\" call, but as \n> this in prepare_index():\n> \n> \tdiscard_cache()\n> \t..\n> \ttree = parse_tree_indirect(head_sha1);\n> \t..\n> \tread_tree(tree, 0, NULL)\n> \n> which is very wrong, because it replaces the old index entirely, and \n> doesn't do that stat information merging.\n\nThis patch may or may not fix it.\n\nIt makes builtin-commit.c use the same logic that \"git read-tree -i -m\" \ndoes (which is what the old shell script did), and it seems to pass the \ntest-suite, and it looks pretty obvious.\n\nIt also brings down the number of open/mmap/munmap/close calls to where it \nshould be, although it still does *way* too many \"lstat()\" operations (ie \nit does 4*lstat for each file in the index - one more than the \nnon-filename one does).\n\nWith that fixed, performance is also roughly where it should be (ie the \n17-18s for the cold-cache case), because it no longer needs to rehash all \nthe files!\n\nHOWEVER. This was just a quick hack, and while it all looks sane, this is \nsome damn core code. Somebody else should double- and triple-check this.\n\n[ That 4x lstat thing bothers me. I think we should add a flag to the \n  index saying \"we checked this once already, it's clean\", so that if we \n  do multiple passes over the index, we can still do just a single lstat() \n  on just the first pass. But that's a separate issue.\n\n  On Linux, a cached lstat() is almost free. Well, at least compared to \n  all the crap operating systems out there. And obviously, if you do \n  multiple lstat's per file, all but the first one *will* be cached.\n\n  However, \"almost free\" still isn't zero, and with the kernel having 23k \n  files in it, doing almost a hundred thousand lstat's is still something \n  that only takes about half a second or so for me. We _really_ should do \n  only ~23k or so of them, and the cached cache should take on the order \n  of 0.15s, rather than half a second!\n\n  So this is worth optimizing. With bigger repositories, it's going to be \n  more noticeable, and with other operating systems, all those lstat()'s \n  will cost much _much_ more. Of course, any IO overhead will be much \n  bigger, so this is mostly a cached-case issue, but cached-case is still \n  important.. ]\n\nAnyway, consider this being conditionally signed-off-by: me, assuming \na few other people spend a bit of time double-checking all my logic.\n\nPlease?\n\n\t\t\tLinus\n\n---\n builtin-commit.c |   37 ++++++++++++++++++++++++++++---------\n 1 files changed, 28 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 73f1e35..cc5134e 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -21,6 +21,7 @@\n #include \"utf8.h\"\n #include \"parse-options.h\"\n #include \"path-list.h\"\n+#include \"unpack-trees.h\"\n \n static const char * const builtin_commit_usage[] = {\n \t\"git-commit [options] [--] <filepattern>...\",\n@@ -177,10 +178,34 @@ static void add_remove_files(struct path_list *list)\n \t}\n }\n \n+static void create_base_index(void)\n+{\n+\tstruct tree *tree;\n+\tstruct unpack_trees_options opts;\n+\tstruct tree_desc t;\n+\n+\tif (initial_commit) {\n+\t\tdiscard_cache();\n+\t\treturn;\n+\t}\n+\n+\tmemset(&opts, 0, sizeof(opts));\n+\topts.head_idx = 1;\n+\topts.index_only = 1;\n+\topts.merge = 1;\n+\t\n+\topts.fn = oneway_merge;\n+\ttree = parse_tree_indirect(head_sha1);\n+\tif (!tree)\n+\t\tdie(\"failed to unpack HEAD tree object\");\n+\tparse_tree(tree);\n+\tinit_tree_desc(&t, tree->buffer, tree->size);\n+\tunpack_trees(1, &t, &opts);\n+}\n+\n static char *prepare_index(int argc, const char **argv, const char *prefix)\n {\n \tint fd;\n-\tstruct tree *tree;\n \tstruct path_list partial;\n \tconst char **pathspec = NULL;\n \n@@ -278,14 +303,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)\n \n \tfd = hold_lock_file_for_update(&false_lock,\n \t\t\t\t       git_path(\"next-index-%d\", getpid()), 1);\n-\tdiscard_cache();\n-\tif (!initial_commit) {\n-\t\ttree = parse_tree_indirect(head_sha1);\n-\t\tif (!tree)\n-\t\t\tdie(\"failed to unpack HEAD tree object\");\n-\t\tif (read_tree(tree, 0, NULL))\n-\t\t\tdie(\"failed to read HEAD tree object\");\n-\t}\n+\n+\tcreate_base_index();\n \tadd_remove_files(&partial);\n \trefresh_cache(REFRESH_QUIET);\n \n"},{"id":"65228","messageId":"alpine.LNX.1.00.0801130028460.13593@iabervon.org","threadId":"11601","inReplyTo":"alpine.LFD.1.00.0801121949180.2806@woody.linux-foundation.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-01-13T05:38:51Z","receivedAt":"2008-01-13T05:38:51Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 12 Jan 2008, Linus Torvalds wrote:\n\n> It makes builtin-commit.c use the same logic that \"git read-tree -i -m\" \n> does (which is what the old shell script did), and it seems to pass the \n> test-suite, and it looks pretty obvious.\n\nThe only issue I know about with using unpack_trees in C as a replacement \nfor read-tree in shell is that unpack_trees leaves \"deletion\" index \nentries in memory which are not written to disk, but may surprise some \ncode (these are used to allow -u to remove the files from the working \ntree). So you may want to make sure that you don't get any weird results \nout of a commit of particular files that involves not committing some \nnewly-added files:\n\n$ git add new-file\n$ (edit old-file)\n$ git commit old-file\n\nThis may cause the unpack_trees to leave a misleading entry for new-file \nthat the code doesn't expect. I've got a patch to make it saner as part of \nmy builtin-checkout series, but I can't say for sure that that change \nwon't either confuse something else or have performance problems without a \nbunch of analysis I haven't done recently.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"65232","messageId":"7vtzliqh3u.fsf@gitster.siamese.dyndns.org","threadId":"11601","inReplyTo":"alpine.LFD.1.00.0801121949180.2806@woody.linux-foundation.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-13T08:12:21Z","receivedAt":"2008-01-13T08:12:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> HOWEVER. This was just a quick hack, and while it all looks sane, this is \n> some damn core code. Somebody else should double- and triple-check this.\n\nDouble-checked.  The patch looks sane.\n\n> [ That 4x lstat thing bothers me. I think we should add a flag to the \n>   index saying \"we checked this once already, it's clean\", so that if we \n>   do multiple passes over the index, we can still do just a single lstat() \n>   on just the first pass. But that's a separate issue.\n\nI've thought about this in a different context before, but it\nseemed quite tricky, as some codepaths in more complex commands\n(commit being one of them) tend to use the cache and discard to\nuse it for different purpose (like creating temporary index and\nthen reading the real index).  Besides, I had an impression that\nwe ran out of the bits of ce_flags in the cache entry, although\nwe could shorten the maximum path we support from 4k down to 2k\nbytes.  I'll have to think about this a bit more.\n"},{"id":"65233","messageId":"7vprw6qh02.fsf@gitster.siamese.dyndns.org","threadId":"11601","inReplyTo":"alpine.LNX.1.00.0801130028460.13593@iabervon.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-13T08:14:37Z","receivedAt":"2008-01-13T08:14:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> On Sat, 12 Jan 2008, Linus Torvalds wrote:\n>\n>> It makes builtin-commit.c use the same logic that \"git read-tree -i -m\" \n>> does (which is what the old shell script did), and it seems to pass the \n>> test-suite, and it looks pretty obvious.\n>\n> The only issue I know about with using unpack_trees in C as a replacement \n> for read-tree in shell is that unpack_trees leaves \"deletion\" index \n> entries in memory which are not written to disk,...\n\nI do not think you have to worry about that one.  That \"to be\ndeleted\" was a Linus invention and he surely remembers it.\n\nwrite_index() function of course knows about skipping them (they\nare marked as !ce->ce_mode).  I think the patch is safe.\n"},{"id":"65235","messageId":"7vd4s6qal0.fsf@gitster.siamese.dyndns.org","threadId":"11601","inReplyTo":"7vtzliqh3u.fsf@gitster.siamese.dyndns.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-13T10:33:15Z","receivedAt":"2008-01-13T10:33:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Linus Torvalds <torvalds@linux-foundation.org> writes:\n>\n>> HOWEVER. This was just a quick hack, and while it all looks sane, this is \n>> some damn core code. Somebody else should double- and triple-check this.\n>\n> Double-checked.  The patch looks sane.\n>\n>> [ That 4x lstat thing bothers me. I think we should add a flag to the \n>>   index saying \"we checked this once already, it's clean\", so that if we \n>>   do multiple passes over the index, we can still do just a single lstat() \n>>   on just the first pass. But that's a separate issue.\n>\n> I've thought about this in a different context before, but it\n> seemed quite tricky, as some codepaths in more complex commands\n> (commit being one of them) tend to use the cache and discard to\n> use it for different purpose (like creating temporary index and\n> then reading the real index).  Besides, I had an impression that\n> we ran out of the bits of ce_flags in the cache entry, although\n> we could shorten the maximum path we support from 4k down to 2k\n> bytes.  I'll have to think about this a bit more.\n\nAside from the lstat(2) done for work tree files, there are\nquite many lstat(2) calls in refname dwimming codepath.  I am\nnot currently looking into reducing them.\n\nSome observations.\n\n * In the partial commit codepath, we run one file_exists() and\n   then one add_file_to_index(), each of which has lstat(2) on\n   the same pathname, for the paths to be committed.  This can\n   be reduced to one trivially by changing the calling\n   convention of add_file_to_index().  This is done in order to\n   build the temporary index file to be written out as a tree\n   for committing (and the index file needs to be written to be\n   shown to the pre-commit hook).\n\n * Then refresh_cache_ent(), which has one lstat(2), is called\n   for everybody in the temporary index, in order to refresh the\n   temporary index.  Again this cannot be avoided because we\n   promise to show the pre-commit hook a refreshed index.\n\n * Then we run file_exists() and add_file_to_index() to update\n   the real index.\n\n * And refresh_cache_ent() is run for everybody in the real\n   index.\n\n * The final diffstat phase runs lstat(2) from\n   reuse_worktree_file() codepath to see if the work tree is up\n   to date, and read from the work tree files instead of having\n   to explode them from the object store.\n\nThe attached is a quick and dirty hack which may or may not\nhelp.  It all looks sane, this also is some core code, and meant\nonly for discussion and not application.\n\n * It adds a new ce_flag, CE_UPTODATE, that is meant to mark the\n   cache entries that record a regular file blob that is up to\n   date in the work tree.\n\n * I had to reduce the maximum length of allowed pathname from\n   4k down to 2k for the above.  Incidentally I noticed that we\n   do not check the length of pathname does not exceed what we\n   can express with CE_NAMEMASK, which we may want to fix (and\n   make it barf if somebody tries to add too long a path)\n   independently from this issue.  A low hanging fruit for\n   janitors -- hint, hint.\n\n * fill_stat_cache_info() marks the cache entry it just added\n   with CE_UPTODATE.  This has the effect of marking the paths\n   we write out of the index and lstat(2) immediately as \"no\n   need to lstat -- we know it is up-to-date\", from quite a lot\n   fo callers:\n\n    - git-apply --index\n    - git-update-index\n    - git-checkout-index\n    - git-add (uses add_file_to_index())\n    - git-commit (ditto)\n    - git-mv (ditto)\n\n * write_index is changed not to write CE_UPTODATE out to the\n   index file, because CE_UPTODATE is meant to be transient only\n   in core.  For the same reason, CE_UPDATE is not written to\n   prevent an accident from happening.\n\nThe fact that we write out a temporary index and then rebuild\nthe real index means CE_UPTODATE flag we populate in the\ntemporary index is lost and we still need to lstat(2) while\nbuilding the real index, which is a bit unfortunate.  I suspect\nthat we can use the one-way merge to reset the index when\nbuilding the real index after we are done building the temporary\nindex, instead of discarding the in-core temporary index and\nre-reading the real index.\n\n---\n cache.h      |    3 ++-\n diff.c       |    4 ++++\n read-cache.c |   10 ++++++++++\n 3 files changed, 16 insertions(+), 1 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 39331c2..9b950fc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -108,7 +108,8 @@ struct cache_entry {\n \tchar name[FLEX_ARRAY]; /* more */\n };\n \n-#define CE_NAMEMASK  (0x0fff)\n+#define CE_NAMEMASK  (0x07ff)\n+#define CE_UPTODATE  (0x0800)\n #define CE_STAGEMASK (0x3000)\n #define CE_UPDATE    (0x4000)\n #define CE_VALID     (0x8000)\ndiff --git a/diff.c b/diff.c\nindex b18c140..41847ae 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1510,6 +1510,10 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int\n \tif (pos < 0)\n \t\treturn 0;\n \tce = active_cache[pos];\n+\n+\tif (ce->ce_flags & htons(CE_UPTODATE))\n+\t\treturn 1;\n+\n \tif ((lstat(name, &st) < 0) ||\n \t    !S_ISREG(st.st_mode) || /* careful! */\n \t    ce_match_stat(ce, &st, 0) ||\ndiff --git a/read-cache.c b/read-cache.c\nindex 7db5588..3a90db1 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -44,6 +44,9 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n \n \tif (assume_unchanged)\n \t\tce->ce_flags |= htons(CE_VALID);\n+\n+\tif (S_ISREG(st->st_mode))\n+\t\tce->ce_flags |= htons(CE_UPTODATE);\n }\n \n static int ce_compare_data(struct cache_entry *ce, struct stat *st)\n@@ -794,6 +797,9 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,\n \tint changed, size;\n \tint ignore_valid = options & CE_MATCH_IGNORE_VALID;\n \n+\tif (ce->ce_flags & htons(CE_UPTODATE))\n+\t\treturn ce;\n+\n \tif (lstat(ce->name, &st) < 0) {\n \t\tif (err)\n \t\t\t*err = errno;\n@@ -1170,13 +1176,17 @@ int write_index(struct index_state *istate, int newfd)\n \n \tfor (i = 0; i < entries; i++) {\n \t\tstruct cache_entry *ce = cache[i];\n+\t\tunsigned short ce_flags;\n \t\tif (!ce->ce_mode)\n \t\t\tcontinue;\n \t\tif (istate->timestamp &&\n \t\t    istate->timestamp <= ntohl(ce->ce_mtime.sec))\n \t\t\tce_smudge_racily_clean_entry(ce);\n+\t\tce_flags = ce->ce_flags;\n+\t\tce->ce_flags &= htons(~(CE_UPDATE | CE_UPTODATE));\n \t\tif (ce_write(&c, newfd, ce, ce_size(ce)) < 0)\n \t\t\treturn -1;\n+\t\tce->ce_flags = ce_flags;\n \t}\n \n \t/* Write extension data here */\n"},{"id":"65236","messageId":"7v8x2uqabt.fsf_-_@gitster.siamese.dyndns.org","threadId":"11601","inReplyTo":"7vtzliqh3u.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] builtin-commit.c: remove useless check added by faulty cut and paste","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-13T10:38:46Z","receivedAt":"2008-01-13T10:38:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When I did 2888605c649ccd423232161186d72c0e6c458a48\n(builtin-commit: fix partial-commit support), I mindlessly cut\nand pasted from builtin-ls-files.c, and included the part that\nwas meant to exclude redundant path after \"ls-files --with-tree\"\noverlayed the HEAD commit on top of the index.  This logic does\nnot apply to what git-commit does and should not have been\ncopied, even though it would not hurt.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-commit.c |    2 --\n 1 files changed, 0 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 6d2ca80..265ba6b 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -156,8 +156,6 @@ static int list_paths(struct path_list *list, const char *with_tree,\n \n \tfor (i = 0; i < active_nr; i++) {\n \t\tstruct cache_entry *ce = active_cache[i];\n-\t\tif (ce->ce_flags & htons(CE_UPDATE))\n-\t\t\tcontinue;\n \t\tif (!pathspec_match(pattern, m, ce->name, 0))\n \t\t\tcontinue;\n \t\tpath_list_insert(ce->name, list);\n"},{"id":"65237","messageId":"7v3at2q9ks.fsf_-_@gitster.siamese.dyndns.org","threadId":"11601","inReplyTo":"7vd4s6qal0.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] builtin-commit.c: do not lstat(2) partially committed paths twice.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-13T10:54:59Z","receivedAt":"2008-01-13T10:54:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>  * In the partial commit codepath, we run one file_exists() and\n>    then one add_file_to_index(), each of which has lstat(2) on\n>    the same pathname, for the paths to be committed.  This can\n>    be reduced to one trivially by changing the calling\n>    convention of add_file_to_index()....\n\nAnd this is the \"trivial\" lstat(2) reduction.\n\nI do not know if it is worth it, though.  Majority of lstat(2)\nmust be coming from the paths in the index (and not committed),\nnot from the paths that are listed on the command line to be\npartially committed.\n\n---\n builtin-commit.c |    6 ++++--\n cache.h          |    2 ++\n read-cache.c     |   34 ++++++++++++++++++++++------------\n 3 files changed, 28 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 6d2ca80..770bd25 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -170,9 +170,11 @@ static void add_remove_files(struct path_list *list)\n {\n \tint i;\n \tfor (i = 0; i < list->nr; i++) {\n+\t\tstruct stat st;\n \t\tstruct path_list_item *p = &(list->items[i]);\n-\t\tif (file_exists(p->path))\n-\t\t\tadd_file_to_cache(p->path, 0);\n+\n+\t\tif (!lstat(p->path, &st))\n+\t\t\tadd_file_to_cache_with_stat(p->path, &st, 0);\n \t\telse\n \t\t\tremove_file_from_cache(p->path);\n \t}\ndiff --git a/cache.h b/cache.h\nindex 39331c2..73cc83b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -174,6 +174,7 @@ extern struct index_state the_index;\n #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos))\n #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path))\n #define add_file_to_cache(path, verbose) add_file_to_index(&the_index, (path), (verbose))\n+#define add_file_to_cache_with_stat(path, st, verbose) add_file_to_index_with_stat(&the_index, (path), (st), (verbose))\n #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL)\n #define ce_match_stat(ce, st, options) ie_match_stat(&the_index, (ce), (st), (options))\n #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))\n@@ -273,6 +274,7 @@ extern struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int reall\n extern int remove_index_entry_at(struct index_state *, int pos);\n extern int remove_file_from_index(struct index_state *, const char *path);\n extern int add_file_to_index(struct index_state *, const char *path, int verbose);\n+extern int add_file_to_index_with_stat(struct index_state *, const char *path, struct stat *, int verbose);\n extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, int refresh);\n extern int ce_same_name(struct cache_entry *a, struct cache_entry *b);\n \ndiff --git a/read-cache.c b/read-cache.c\nindex 7db5588..928f49b 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -384,21 +384,22 @@ static int index_name_pos_also_unmerged(struct index_state *istate,\n \treturn pos;\n }\n \n-int add_file_to_index(struct index_state *istate, const char *path, int verbose)\n+int add_file_to_index_with_stat(struct index_state *istate,\n+\t\t\t\tconst char *path,\n+\t\t\t\tstruct stat *st,\n+\t\t\t\tint verbose)\n {\n \tint size, namelen, pos;\n-\tstruct stat st;\n \tstruct cache_entry *ce;\n \tunsigned ce_option = CE_MATCH_IGNORE_VALID|CE_MATCH_RACY_IS_DIRTY;\n \n-\tif (lstat(path, &st))\n-\t\tdie(\"%s: unable to stat (%s)\", path, strerror(errno));\n-\n-\tif (!S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode) && !S_ISDIR(st.st_mode))\n+\tif (!S_ISREG(st->st_mode) &&\n+\t    !S_ISLNK(st->st_mode) &&\n+\t    !S_ISDIR(st->st_mode))\n \t\tdie(\"%s: can only add regular files, symbolic links or git-directories\", path);\n \n \tnamelen = strlen(path);\n-\tif (S_ISDIR(st.st_mode)) {\n+\tif (S_ISDIR(st->st_mode)) {\n \t\twhile (namelen && path[namelen-1] == '/')\n \t\t\tnamelen--;\n \t}\n@@ -406,10 +407,10 @@ int add_file_to_index(struct index_state *istate, const char *path, int verbose)\n \tce = xcalloc(1, size);\n \tmemcpy(ce->name, path, namelen);\n \tce->ce_flags = htons(namelen);\n-\tfill_stat_cache_info(ce, &st);\n+\tfill_stat_cache_info(ce, st);\n \n \tif (trust_executable_bit && has_symlinks)\n-\t\tce->ce_mode = create_ce_mode(st.st_mode);\n+\t\tce->ce_mode = create_ce_mode(st->st_mode);\n \telse {\n \t\t/* If there is an existing entry, pick the mode bits and type\n \t\t * from it, otherwise assume unexecutable regular file.\n@@ -418,19 +419,19 @@ int add_file_to_index(struct index_state *istate, const char *path, int verbose)\n \t\tint pos = index_name_pos_also_unmerged(istate, path, namelen);\n \n \t\tent = (0 <= pos) ? istate->cache[pos] : NULL;\n-\t\tce->ce_mode = ce_mode_from_stat(ent, st.st_mode);\n+\t\tce->ce_mode = ce_mode_from_stat(ent, st->st_mode);\n \t}\n \n \tpos = index_name_pos(istate, ce->name, namelen);\n \tif (0 <= pos &&\n \t    !ce_stage(istate->cache[pos]) &&\n-\t    !ie_match_stat(istate, istate->cache[pos], &st, ce_option)) {\n+\t    !ie_match_stat(istate, istate->cache[pos], st, ce_option)) {\n \t\t/* Nothing changed, really */\n \t\tfree(ce);\n \t\treturn 0;\n \t}\n \n-\tif (index_path(ce->sha1, path, &st, 1))\n+\tif (index_path(ce->sha1, path, st, 1))\n \t\tdie(\"unable to index file %s\", path);\n \tif (add_index_entry(istate, ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))\n \t\tdie(\"unable to add %s to index\",path);\n@@ -439,6 +440,15 @@ int add_file_to_index(struct index_state *istate, const char *path, int verbose)\n \treturn 0;\n }\n \n+int add_file_to_index(struct index_state *istate, const char *path, int verbose)\n+{\n+\tstruct stat st;\n+\tif (lstat(path, &st))\n+\t\tdie(\"%s: unable to stat (%s)\", path, strerror(errno));\n+\n+\treturn add_file_to_index_with_stat(istate, path, &st, verbose);\n+}\n+\n struct cache_entry *make_cache_entry(unsigned int mode,\n \t\tconst unsigned char *sha1, const char *path, int stage,\n \t\tint refresh)\n"},{"id":"65238","messageId":"7vy7auoucg.fsf@gitster.siamese.dyndns.org","threadId":"11601","inReplyTo":"7vd4s6qal0.fsf@gitster.siamese.dyndns.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-13T11:09:19Z","receivedAt":"2008-01-13T11:09:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> The fact that we write out a temporary index and then rebuild\n> the real index means CE_UPTODATE flag we populate in the\n> temporary index is lost and we still need to lstat(2) while\n> building the real index, which is a bit unfortunate.  I suspect\n> that we can use the one-way merge to reset the index when\n> building the real index after we are done building the temporary\n> index, instead of discarding the in-core temporary index and\n> re-reading the real index.\n\nThis comment is completely bogus.  With your earlier one-way\nmerge fix, as the way CE_UPTODATE patch was written we preserve\nin-core CE_UPTODATE bit across write_index(), the code already\nshould be taking advantage of an earlier lstat(2).\n"},{"id":"65257","messageId":"alpine.LFD.1.00.0801130850460.2806@woody.linux-foundation.org","threadId":"11601","inReplyTo":"alpine.LNX.1.00.0801130028460.13593@iabervon.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-13T16:57:03Z","receivedAt":"2008-01-13T16:57:03Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 13 Jan 2008, Daniel Barkalow wrote:\n> \n> The only issue I know about with using unpack_trees in C as a replacement \n> for read-tree in shell is that unpack_trees leaves \"deletion\" index \n> entries in memory which are not written to disk, but may surprise some \n> code (these are used to allow -u to remove the files from the working \n> tree).\n\nI certainly agree that this patch should be double-checked. I'm pretty \nsure the issue you mention wouldn't be an issue, since the end result is \nonly used for actually updating the index and writing it out as a tree \n(both of which should handle the magic zero ce_mode case ok), but it would \ncertainly be good to walk through all cases.\n\n\t\t\tLinus\n"},{"id":"65259","messageId":"alpine.LFD.1.00.0801130922030.2806@woody.linux-foundation.org","threadId":"11601","inReplyTo":"7vd4s6qal0.fsf@gitster.siamese.dyndns.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-13T17:24:02Z","receivedAt":"2008-01-13T17:24:02Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 13 Jan 2008, Junio C Hamano wrote:\n> \n> The attached is a quick and dirty hack which may or may not\n> help.  It all looks sane, this also is some core code, and meant\n> only for discussion and not application.\n\nI don't think this will help.\n\nYou never set CE_UPTODATE, except in the \"fill_stat_cache_info()\" \nfunction, but that one will never be called for an old file that already \nmatched the stat.\n\nSo at a minimum, you should also make ie_match_stat() set CE_UPTODATE if \nit matches. Or something.\n\n\t\t\tLinus\n"},{"id":"65268","messageId":"alpine.LNX.1.00.0801131416540.13593@iabervon.org","threadId":"11601","inReplyTo":"alpine.LFD.1.00.0801130850460.2806@woody.linux-foundation.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-01-13T19:31:32Z","receivedAt":"2008-01-13T19:31:32Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 13 Jan 2008, Linus Torvalds wrote:\n\n> On Sun, 13 Jan 2008, Daniel Barkalow wrote:\n> > \n> > The only issue I know about with using unpack_trees in C as a replacement \n> > for read-tree in shell is that unpack_trees leaves \"deletion\" index \n> > entries in memory which are not written to disk, but may surprise some \n> > code (these are used to allow -u to remove the files from the working \n> > tree).\n> \n> I certainly agree that this patch should be double-checked. I'm pretty \n> sure the issue you mention wouldn't be an issue, since the end result is \n> only used for actually updating the index and writing it out as a tree \n> (both of which should handle the magic zero ce_mode case ok), but it would \n> certainly be good to walk through all cases.\n\nYeah, I didn't think it would be an actual problem, but verifying that \nrequires looking outside of the context of the patch. It may even be worth \nputting in a comment for now, since I bet wt_status_print and run_status \ncould be optimized in a way that would look perfectly reasonable (use \nthe in-memory index, instead of reading a file) but would expose the \nmagic case to the diff machinary, which (IIRC) doesn't handle it. But I \nagree (having now looked at the rest of builtin-commit) that the odd index \nentries can't escape, and this should be fine for 1.5.4.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"65269","messageId":"7vsl11plbe.fsf@gitster.siamese.dyndns.org","threadId":"11601","inReplyTo":"alpine.LFD.1.00.0801130922030.2806@woody.linux-foundation.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-13T19:39:01Z","receivedAt":"2008-01-13T19:39:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Sun, 13 Jan 2008, Junio C Hamano wrote:\n>> \n>> The attached is a quick and dirty hack which may or may not\n>> help.  It all looks sane, this also is some core code, and meant\n>> only for discussion and not application.\n>\n> I don't think this will help.\n>\n> You never set CE_UPTODATE, except in the \"fill_stat_cache_info()\" \n> function, but that one will never be called for an old file that already \n> matched the stat.\n>\n> So at a minimum, you should also make ie_match_stat() set CE_UPTODATE if \n> it matches. Or something.\n\nUnfortunately ie_match_stat() is too late.  The caller is\nsupposed to have already called lstat(2) and give the result to\nthat function.\n\nWhen refresh_cache_ent() finds the entry actually matched, we\ncould mark the path with CE_UPTODATE.  That would be a\nrelatively contained and safe optimization that might help\ngit-commit.\n\nAbout the CE_NAMEMASK limitation (and currently we do not check\nit, so I think we would be screwed when a pathname that is\nlonger than (CE_NAMEMASK+1) and still fits under PATH_MAX is\ngiven), I think we do not have to limit the maximum pathname\nlength.  Instead we can teach create_ce_flags() and ce_namelen()\nthat a name longer than 2k (or 4k) has the NAMEMASK bits that\nare all 1 and ce->name[] must be counted if so (with an obvious\noptimization to start counting at byte position 2k or 4k in\nce_namelen()).\n"},{"id":"65277","messageId":"7vhchhpd3h.fsf_-_@gitster.siamese.dyndns.org","threadId":"11601","inReplyTo":"7vsl11plbe.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] index: be careful when handling long names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-13T22:36:34Z","receivedAt":"2008-01-13T22:36:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We currently use lower 12-bit (masked with CE_NAMEMASK) in the\nce_flags field to store the length of the name in cache_entry,\nwithout checking the length parameter given to\ncreate_ce_flags().  This can make us store incorrect length.\n\nCurrently we are mostly protected by the fact that many\ncodepaths first copy the path in a variable of size PATH_MAX,\nwhich typically is 4096 that happens to match the limit, but\nthat feels like a bug waiting to happen.  Besides, that would\nnot allow us to shorten the width of CE_NAMEMASK to use the bits\nfor new flags.\n\nThis redefines the meaning of the name length stored in the\ncache_entry.  A name that does not fit is represented by storing\nCE_NAMEMASK in the field, and the actual length needs to be\ncomputed by actually counting the bytes in the name[] field.\nThis way, only the unusually long paths need to suffer.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n Junio C Hamano <gitster@pobox.com> writes:\n\n > About the CE_NAMEMASK limitation (and currently we do not check\n > it, so I think we would be screwed when a pathname that is\n > longer than (CE_NAMEMASK+1) and still fits under PATH_MAX is\n > given), I think we do not have to limit the maximum pathname\n > length.  Instead we can teach create_ce_flags() and ce_namelen()\n > that a name longer than 2k (or 4k) has the NAMEMASK bits that\n > are all 1 and ce->name[] must be counted if so (with an obvious\n > optimization to start counting at byte position 2k or 4k in\n > ce_namelen()).\n\n This should fix it.  Passes tests including the new test that\n the existing code fails.\n\n cache.h          |   17 +++++++++++++++--\n t/t0000-basic.sh |   18 ++++++++++++++++++\n 2 files changed, 33 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 39331c2..ad53acc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -114,8 +114,21 @@ struct cache_entry {\n #define CE_VALID     (0x8000)\n #define CE_STAGESHIFT 12\n \n-#define create_ce_flags(len, stage) htons((len) | ((stage) << CE_STAGESHIFT))\n-#define ce_namelen(ce) (CE_NAMEMASK & ntohs((ce)->ce_flags))\n+static inline unsigned create_ce_flags(size_t len, unsigned stage)\n+{\n+\tif (len >= CE_NAMEMASK)\n+\t\tlen = CE_NAMEMASK;\n+\treturn htons(len | (stage << CE_STAGESHIFT));\n+}\n+\n+static inline size_t ce_namelen(const struct cache_entry *ce)\n+{\n+\tsize_t len = ntohs((ce)->ce_flags) & CE_NAMEMASK;\n+\tif (len < CE_NAMEMASK) /* likely */\n+\t\treturn len;\n+\treturn strlen(ce->name + CE_NAMEMASK) + CE_NAMEMASK;\n+}\n+\n #define ce_size(ce) cache_entry_size(ce_namelen(ce))\n #define ce_stage(ce) ((CE_STAGEMASK & ntohs((ce)->ce_flags)) >> CE_STAGESHIFT)\n \ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 4e49d59..40551a3 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -297,4 +297,22 @@ test_expect_success 'absolute path works as expected' '\n \ttest \"$sym\" = \"$(test-absolute-path $dir2/syml)\"\n '\n \n+test_expect_success 'very long name in the index handled sanely' '\n+\n+\ta=a && # 1\n+\ta=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 16\n+\ta=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 256\n+\ta=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 4096\n+\ta=${a}q\n+\n+\t>path4 &&\n+\tgit update-index --add path4 &&\n+\t(\n+\t\tgit ls-files -s path4 |\n+\t\tsed -e \"s/\t.*/\t/\" |\n+\t\ttr -d \"\\012\"\n+\t\techo \"$a\"\n+\t) | git update-index --index-info\n+'\n+\n test_done\n"},{"id":"65278","messageId":"20080113225321.GA19970@steel.home","threadId":"11601","inReplyTo":"7vhchhpd3h.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: [PATCH] index: be careful when handling long names","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-01-13T22:53:21Z","receivedAt":"2008-01-13T22:53:21Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Sun, Jan 13, 2008 23:36:34 +0100:\n> +test_expect_success 'very long name in the index handled sanely' '\n> +\n> +\ta=a && # 1\n> +\ta=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 16\n> +\ta=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 256\n> +\ta=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 4096\n\nI'd expect it to fail on some systems (everywindowsthing up to w2k,\nmaybe some commercial unices).\n"},{"id":"65281","messageId":"7v4pdhpbmw.fsf@gitster.siamese.dyndns.org","threadId":"11601","inReplyTo":"20080113225321.GA19970@steel.home","subject":"Re: [PATCH] index: be careful when handling long names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-13T23:08:07Z","receivedAt":"2008-01-13T23:08:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> writes:\n\n> Junio C Hamano, Sun, Jan 13, 2008 23:36:34 +0100:\n>> +test_expect_success 'very long name in the index handled sanely' '\n>> +\n>> +\ta=a && # 1\n>> +\ta=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 16\n>> +\ta=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 256\n>> +\ta=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 4096\n>\n> I'd expect it to fail on some systems (everywindowsthing up to w2k,\n> maybe some commercial unices).\n\nMy understanding is that Everywindowsthing do not come with any\n(POSIX compliant) shell that we support by default, so if you\nare talking about a limit of shell variable value, I do not\nthink it is an issue to begin with.  It is just the matter of\npicking a sensible shell (I understand both Cygwin and msys\nports use a shell that supports more than 4k bytes in value\ngiven to a variable).\n\nI would agree that it might overflow the argument limit when\nthis is given to \"echo\", though.  We cannot do much about it,\nbut you may have cleverer ideas.\n"},{"id":"65283","messageId":"20080113233323.GB19970@steel.home","threadId":"11601","inReplyTo":"7v4pdhpbmw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] index: be careful when handling long names","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-01-13T23:33:23Z","receivedAt":"2008-01-13T23:33:23Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Mon, Jan 14, 2008 00:08:07 +0100:\n> Alex Riesen <raa.lkml@gmail.com> writes:\n> \n> > Junio C Hamano, Sun, Jan 13, 2008 23:36:34 +0100:\n> >> +test_expect_success 'very long name in the index handled sanely' '\n> >> +\n> >> +\ta=a && # 1\n> >> +\ta=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 16\n> >> +\ta=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 256\n> >> +\ta=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 4096\n> >\n> > I'd expect it to fail on some systems (everywindowsthing up to w2k,\n> > maybe some commercial unices).\n> \n> My understanding is that Everywindowsthing do not come with any\n> (POSIX compliant) shell that we support by default, so if you\n> are talking about a limit of shell variable value, I do not\n> think it is an issue to begin with.  It is just the matter of\n\nOh, right. The file system wont even see it, it is passed directly to\nupdate-index.\n\n> picking a sensible shell (I understand both Cygwin and msys\n> ports use a shell that supports more than 4k bytes in value\n> given to a variable).\n\ncan't check right now, but I believe it is so\n\n> I would agree that it might overflow the argument limit when\n> this is given to \"echo\", though.  We cannot do much about it,\n> but you may have cleverer ideas.\n\nI thought about conditionally disabling the test, like it was done\nwhen the tabs in filenames had to be tested. Wont be needed for this\nparticular case.\n"},{"id":"65289","messageId":"7vr6glnrvp.fsf@gitster.siamese.dyndns.org","threadId":"11601","inReplyTo":"alpine.LFD.1.00.0801130922030.2806@woody.linux-foundation.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-14T01:00:10Z","receivedAt":"2008-01-14T01:00:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> So at a minimum, you should also make ie_match_stat() set CE_UPTODATE if \n> it matches. Or something.\n\nI've reworked the patch, and in the kernel repository, a\nsingle-path commit after touching that path now calls 23k\nlstat(2).  It used to call 46k lstat(2) after your fix.\n\nThis depends on the \"index: be careful when handling long names\"\npatch I sent earlier.\n\n-- >8 --\n[PATCH] Avoid running lstat(2) on the same cache entry.\n\nAside from the lstat(2) done for work tree files, there are\nquite many lstat(2) calls in refname dwimming codepath.  This\npatch is not about reducing them.\n\n * It adds a new ce_flag, CE_UPTODATE, that is meant to mark the\n   cache entries that record a regular file blob that is up to\n   date in the work tree.  If somebody later walks the index and\n   wants to see if the work tree has changes, they do not have\n   to be checked with lstat(2) again.\n\n * This reduces the maximum length cached from 4k down to 2k to\n   introduce the new flag.  People with an index that records\n   paths longer than that needs to first commit their changes\n   with the old git, discard the cache with \"rm -f .git/index\"\n   and re-read with updated git by running \"git-read-tree HEAD\"\n   followed by \"git update-index --refresh\".\n\n * fill_stat_cache_info() marks the cache entry it just added\n   with CE_UPTODATE.  This has the effect of marking the paths\n   we write out of the index and lstat(2) immediately as \"no\n   need to lstat -- we know it is up-to-date\", from quite a lot\n   fo callers:\n\n    - git-apply --index\n    - git-update-index\n    - git-checkout-index\n    - git-add (uses add_file_to_index())\n    - git-commit (ditto)\n    - git-mv (ditto)\n\n * refresh_cache_ent() also marks the cache entry that are clean\n   with CE_UPTODATE.\n\n * write_index is changed not to write CE_UPTODATE out to the\n   index file, because CE_UPTODATE is meant to be transient only\n   in core.  For the same reason, CE_UPDATE is not written to\n   prevent an accident from happening.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n cache.h      |    4 +++-\n diff.c       |    4 ++++\n read-cache.c |   20 +++++++++++++++++++-\n 3 files changed, 26 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 7f1457a..3e1cdf9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -108,7 +108,8 @@ struct cache_entry {\n \tchar name[FLEX_ARRAY]; /* more */\n };\n \n-#define CE_NAMEMASK  (0x0fff)\n+#define CE_NAMEMASK  (0x07ff)\n+#define CE_UPTODATE  (0x0800)\n #define CE_STAGEMASK (0x3000)\n #define CE_UPDATE    (0x4000)\n #define CE_VALID     (0x8000)\n@@ -129,6 +130,7 @@ static inline size_t ce_namelen(const struct cache_entry *ce)\n \treturn strlen(ce->name + CE_NAMEMASK) + CE_NAMEMASK;\n }\n \n+#define ce_uptodate(ce) (!!((ce)->ce_flags & htons(CE_UPTODATE)))\n #define ce_size(ce) cache_entry_size(ce_namelen(ce))\n #define ce_stage(ce) ((CE_STAGEMASK & ntohs((ce)->ce_flags)) >> CE_STAGESHIFT)\n \ndiff --git a/diff.c b/diff.c\nindex b18c140..62d0c06 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1510,6 +1510,10 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int\n \tif (pos < 0)\n \t\treturn 0;\n \tce = active_cache[pos];\n+\n+\tif (ce_uptodate(ce))\n+\t\treturn 1;\n+\n \tif ((lstat(name, &st) < 0) ||\n \t    !S_ISREG(st.st_mode) || /* careful! */\n \t    ce_match_stat(ce, &st, 0) ||\ndiff --git a/read-cache.c b/read-cache.c\nindex 928f49b..6e40e37 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -44,6 +44,9 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n \n \tif (assume_unchanged)\n \t\tce->ce_flags |= htons(CE_VALID);\n+\n+\tif (S_ISREG(st->st_mode))\n+\t\tce->ce_flags |= htons(CE_UPTODATE);\n }\n \n static int ce_compare_data(struct cache_entry *ce, struct stat *st)\n@@ -428,6 +431,7 @@ int add_file_to_index_with_stat(struct index_state *istate,\n \t    !ie_match_stat(istate, istate->cache[pos], st, ce_option)) {\n \t\t/* Nothing changed, really */\n \t\tfree(ce);\n+\t\tistate->cache[pos]->ce_flags |= htons(CE_UPTODATE);\n \t\treturn 0;\n \t}\n \n@@ -804,6 +808,9 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,\n \tint changed, size;\n \tint ignore_valid = options & CE_MATCH_IGNORE_VALID;\n \n+\tif (ce_uptodate(ce))\n+\t\treturn ce;\n+\n \tif (lstat(ce->name, &st) < 0) {\n \t\tif (err)\n \t\t\t*err = errno;\n@@ -822,8 +829,15 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,\n \t\tif (ignore_valid && assume_unchanged &&\n \t\t    !(ce->ce_flags & htons(CE_VALID)))\n \t\t\t; /* mark this one VALID again */\n-\t\telse\n+\t\telse {\n+\t\t\t/*\n+\t\t\t * We do not mark the index itself \"modified\"\n+\t\t\t * because CE_UPTODATE flag is in-core only;\n+\t\t\t * we are not going to write this change out.\n+\t\t\t */\n+\t\t\tce->ce_flags |= htons(CE_UPTODATE);\n \t\t\treturn ce;\n+\t\t}\n \t}\n \n \tif (ie_modified(istate, ce, &st, options)) {\n@@ -1180,13 +1194,17 @@ int write_index(struct index_state *istate, int newfd)\n \n \tfor (i = 0; i < entries; i++) {\n \t\tstruct cache_entry *ce = cache[i];\n+\t\tunsigned short ce_flags;\n \t\tif (!ce->ce_mode)\n \t\t\tcontinue;\n \t\tif (istate->timestamp &&\n \t\t    istate->timestamp <= ntohl(ce->ce_mtime.sec))\n \t\t\tce_smudge_racily_clean_entry(ce);\n+\t\tce_flags = ce->ce_flags;\n+\t\tce->ce_flags &= htons(~(CE_UPDATE | CE_UPTODATE));\n \t\tif (ce_write(&c, newfd, ce, ce_size(ce)) < 0)\n \t\t\treturn -1;\n+\t\tce->ce_flags = ce_flags;\n \t}\n \n \t/* Write extension data here */\n-- \n1.5.4.rc3.4.g16335\n"},{"id":"65323","messageId":"alpine.LFD.1.00.0801140902140.2806@woody.linux-foundation.org","threadId":"11601","inReplyTo":"7vr6glnrvp.fsf@gitster.siamese.dyndns.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-14T17:07:27Z","receivedAt":"2008-01-14T17:07:27Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 13 Jan 2008, Junio C Hamano wrote:\n> \n> I've reworked the patch, and in the kernel repository, a\n> single-path commit after touching that path now calls 23k\n> lstat(2).  It used to call 46k lstat(2) after your fix.\n\nOk, I really like what the patch does, and how it looks.\n\nAt the same time, I *really* hate how we now edit the cache entries in \nplace for these kinds of things that really have nothing to do with the \non-disk format. That's not a new thing (CE_UPDATE is the same), but it \ndefinitely got uglier.\n\nSo I think this patch is good, but I think it would be even better if we \njust bit the bullet and started looking at having a different in-memory \nrepresentation from the on-disk one. Possibly not *that* much different: \nperhaps just keeping a pointer to the on-disk one along with a flags \nvalue.\n\nThat would be a fairly painful change, though (and quite independent from \nthis particular one - apart from the fact that CE_UPTODATE is one of the \nusers that could be cleaned up if we did that).\n\nComments?\n\n\t\t\tLinus\n"},{"id":"65327","messageId":"7v63xwl0c6.fsf@gitster.siamese.dyndns.org","threadId":"11601","inReplyTo":"alpine.LFD.1.00.0801140902140.2806@woody.linux-foundation.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-14T18:38:01Z","receivedAt":"2008-01-14T18:38:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> ... I think it would be even better if we \n> just bit the bullet and started looking at having a different in-memory \n> representation from the on-disk one. Possibly not *that* much different: \n> perhaps just keeping a pointer to the on-disk one along with a flags \n> value.\n\nWe have two things we currently do that are not about on-disk\nindex file ('-'), and this patch adds another ('+'):\n\n - Update the work tree file that corresponds to this entry\n   (CE_UPDATE);\n\n - This entry is to be removed (ce_mode == 0);\n\n + The work tree file that corresponds to this entry is known to\n   be unchanged (CE_UPTODATE).\n\nWe could introduce \"struct in_core_cache_entry\" that has these\ninformation, indexed and sorted by name, and has a pointer to\nwhat we read from the on-disk index.\n\n\tstruct in_core_cache_entry {\n\t\tstruct cache_entry *e;\n                unsigned is_up_to_date : 1,\n                \t to_be_updated : 1,\n                         to_be_removed : 1;\n\t};\n\nThe code that iterate over active_cache[] will instead iterate\nover this.  The number of the entries in this array will be the\nnew active_nr.\n\nIn the existing code, we reference \"ce->name\" and \"ce->sha1\"\neverywhere. When we check and update flags we do bitops between\n\"ce->ce_flags\" and \"htons(CE_BLAH)\" in many places.  Converting\nthem would adds another indirection and be quite painful.  But\nthe compiler can reliably spot the places we fail to find, so it\nat least is not so risky.  It will just be a lot of work.\n"},{"id":"65329","messageId":"alpine.LFD.1.00.0801141132250.2806@woody.linux-foundation.org","threadId":"11601","inReplyTo":"alpine.LFD.1.00.0801140902140.2806@woody.linux-foundation.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-14T19:39:17Z","receivedAt":"2008-01-14T19:39:17Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 14 Jan 2008, Linus Torvalds wrote:\n> \n> So I think this patch is good, but I think it would be even better if we \n> just bit the bullet and started looking at having a different in-memory \n> representation from the on-disk one.\n\nOk, so here's a possible patch.\n\nIt passes all the tests for me, and looks fairly ok, but it's also a bit \nbig.\n\nWhat makes it big is that I made the in-memory format be in host order, so \nthat we can remove a *lot* of the \"htonl/ntohl\" switcheroo, and do it just \non index file read/write.\n\nThe nice thing about this patch is that it would make it a lot easier to \ndo any index handling changes,  because it makes a clear difference \nbetween the on-disk and the in-memory formats.\n\nI realize that the patch looks big (195 lines inserted and 148 lines \nremoved), but *most* of the lines are literally those ntohl() \nsimplifications, ie stuff like\n\n\t-       if (S_ISGITLINK(ntohl(ce->ce_mode))) {\n\t+       if (S_ISGITLINK(ce->ce_mode)) {\n\nso while it adds lines (for the \"convert from disk\" and \"convert to disk\" \nformat conversions), in  many ways it really simplifies the source code \ntoo.\n\nComments?\n\nThis is on top of current master, so it's *before* junios thing that adds \na CE_UPTODATE.\n\nWith this, the high 16 bits of \"ce_flags\" are in-memory only, so you could \njust make CE_UPTODATE be 0x10000, and it automatically ends up never being \nwritten to disk (and always \"reads\" as zero).\n\n\t\tLinus\n\n---\n builtin-apply.c        |   10 ++--\n builtin-blame.c        |    2 +-\n builtin-fsck.c         |    2 +-\n builtin-grep.c         |    4 +-\n builtin-ls-files.c     |   10 ++--\n builtin-read-tree.c    |    2 +-\n builtin-rerere.c       |    4 +-\n builtin-update-index.c |   18 +++---\n cache-tree.c           |    2 +-\n cache.h                |   41 +++++++----\n diff-lib.c             |   31 ++++-----\n dir.c                  |    2 +-\n entry.c                |    6 +-\n merge-index.c          |    2 +-\n merge-recursive.c      |    2 +-\n reachable.c            |    2 +-\n read-cache.c           |  183 ++++++++++++++++++++++++++++-------------------\n sha1_name.c            |    2 +-\n tree.c                 |    4 +-\n unpack-trees.c         |   14 ++--\n 20 files changed, 195 insertions(+), 148 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex d57bb6e..bd7cc37 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1946,7 +1946,7 @@ static int read_file_or_gitlink(struct cache_entry *ce, struct strbuf *buf)\n \tif (!ce)\n \t\treturn 0;\n \n-\tif (S_ISGITLINK(ntohl(ce->ce_mode))) {\n+\tif (S_ISGITLINK(ce->ce_mode)) {\n \t\tstrbuf_grow(buf, 100);\n \t\tstrbuf_addf(buf, \"Subproject commit %s\\n\", sha1_to_hex(ce->sha1));\n \t} else {\n@@ -2023,7 +2023,7 @@ static int check_to_create_blob(const char *new_name, int ok_if_exists)\n \n static int verify_index_match(struct cache_entry *ce, struct stat *st)\n {\n-\tif (S_ISGITLINK(ntohl(ce->ce_mode))) {\n+\tif (S_ISGITLINK(ce->ce_mode)) {\n \t\tif (!S_ISDIR(st->st_mode))\n \t\t\treturn -1;\n \t\treturn 0;\n@@ -2082,12 +2082,12 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)\n \t\t\t\treturn error(\"%s: does not match index\",\n \t\t\t\t\t     old_name);\n \t\t\tif (cached)\n-\t\t\t\tst_mode = ntohl(ce->ce_mode);\n+\t\t\t\tst_mode = ce->ce_mode;\n \t\t} else if (stat_ret < 0)\n \t\t\treturn error(\"%s: %s\", old_name, strerror(errno));\n \n \t\tif (!cached)\n-\t\t\tst_mode = ntohl(ce_mode_from_stat(ce, st.st_mode));\n+\t\t\tst_mode = ce_mode_from_stat(ce, st.st_mode);\n \n \t\tif (patch->is_new < 0)\n \t\t\tpatch->is_new = 0;\n@@ -2388,7 +2388,7 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned\n \tce = xcalloc(1, ce_size);\n \tmemcpy(ce->name, path, namelen);\n \tce->ce_mode = create_ce_mode(mode);\n-\tce->ce_flags = htons(namelen);\n+\tce->ce_flags = namelen;\n \tif (S_ISGITLINK(mode)) {\n \t\tconst char *s = buf;\n \ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 9b4c02e..c7e6887 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -2092,7 +2092,7 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con\n \tif (!mode) {\n \t\tint pos = cache_name_pos(path, len);\n \t\tif (0 <= pos)\n-\t\t\tmode = ntohl(active_cache[pos]->ce_mode);\n+\t\t\tmode = active_cache[pos]->ce_mode;\n \t\telse\n \t\t\t/* Let's not bother reading from HEAD tree */\n \t\t\tmode = S_IFREG | 0644;\ndiff --git a/builtin-fsck.c b/builtin-fsck.c\nindex e4874f6..8876d34 100644\n--- a/builtin-fsck.c\n+++ b/builtin-fsck.c\n@@ -762,7 +762,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t\tstruct blob *blob;\n \t\t\tstruct object *obj;\n \n-\t\t\tmode = ntohl(active_cache[i]->ce_mode);\n+\t\t\tmode = active_cache[i]->ce_mode;\n \t\t\tif (S_ISGITLINK(mode))\n \t\t\t\tcontinue;\n \t\t\tblob = lookup_blob(active_cache[i]->sha1);\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex 0d6cc73..9180b39 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -331,7 +331,7 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)\n \t\tstruct cache_entry *ce = active_cache[i];\n \t\tchar *name;\n \t\tint kept;\n-\t\tif (!S_ISREG(ntohl(ce->ce_mode)))\n+\t\tif (!S_ISREG(ce->ce_mode))\n \t\t\tcontinue;\n \t\tif (!pathspec_matches(paths, ce->name))\n \t\t\tcontinue;\n@@ -387,7 +387,7 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached)\n \n \tfor (nr = 0; nr < active_nr; nr++) {\n \t\tstruct cache_entry *ce = active_cache[nr];\n-\t\tif (!S_ISREG(ntohl(ce->ce_mode)))\n+\t\tif (!S_ISREG(ce->ce_mode))\n \t\t\tcontinue;\n \t\tif (!pathspec_matches(paths, ce->name))\n \t\t\tcontinue;\ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex 0f0ab2d..d56e33e 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -189,7 +189,7 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)\n \t\treturn;\n \n \tif (tag && *tag && show_valid_bit &&\n-\t    (ce->ce_flags & htons(CE_VALID))) {\n+\t    (ce->ce_flags & CE_VALID)) {\n \t\tstatic char alttag[4];\n \t\tmemcpy(alttag, tag, 3);\n \t\tif (isalpha(tag[0]))\n@@ -210,7 +210,7 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)\n \t} else {\n \t\tprintf(\"%s%06o %s %d\\t\",\n \t\t       tag,\n-\t\t       ntohl(ce->ce_mode),\n+\t\t       ce->ce_mode,\n \t\t       abbrev ? find_unique_abbrev(ce->sha1,abbrev)\n \t\t\t\t: sha1_to_hex(ce->sha1),\n \t\t       ce_stage(ce));\n@@ -242,7 +242,7 @@ static void show_files(struct dir_struct *dir, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\tif (show_unmerged && !ce_stage(ce))\n \t\t\t\tcontinue;\n-\t\t\tif (ce->ce_flags & htons(CE_UPDATE))\n+\t\t\tif (ce->ce_flags & CE_UPDATE)\n \t\t\t\tcontinue;\n \t\t\tshow_ce_entry(ce_stage(ce) ? tag_unmerged : tag_cached, ce);\n \t\t}\n@@ -350,7 +350,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)\n \t\tstruct cache_entry *ce = active_cache[i];\n \t\tif (!ce_stage(ce))\n \t\t\tcontinue;\n-\t\tce->ce_flags |= htons(CE_STAGEMASK);\n+\t\tce->ce_flags |= CE_STAGEMASK;\n \t}\n \n \tif (prefix) {\n@@ -379,7 +379,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)\n \t\t\t */\n \t\t\tif (last_stage0 &&\n \t\t\t    !strcmp(last_stage0->name, ce->name))\n-\t\t\t\tce->ce_flags |= htons(CE_UPDATE);\n+\t\t\t\tce->ce_flags |= CE_UPDATE;\n \t\t}\n \t}\n }\ndiff --git a/builtin-read-tree.c b/builtin-read-tree.c\nindex 43cd56a..eb879e1 100644\n--- a/builtin-read-tree.c\n+++ b/builtin-read-tree.c\n@@ -46,7 +46,7 @@ static int read_cache_unmerged(void)\n \t\t\tcache_tree_invalidate_path(active_cache_tree, ce->name);\n \t\t\tlast = ce;\n \t\t\tce->ce_mode = 0;\n-\t\t\tce->ce_flags &= ~htons(CE_STAGEMASK);\n+\t\t\tce->ce_flags &= ~CE_STAGEMASK;\n \t\t}\n \t\t*dst++ = ce;\n \t}\ndiff --git a/builtin-rerere.c b/builtin-rerere.c\nindex 37e6248..df1a55d 100644\n--- a/builtin-rerere.c\n+++ b/builtin-rerere.c\n@@ -149,8 +149,8 @@ static int find_conflict(struct path_list *conflict)\n \t\tif (ce_stage(e2) == 2 &&\n \t\t    ce_stage(e3) == 3 &&\n \t\t    ce_same_name(e2, e3) &&\n-\t\t    S_ISREG(ntohl(e2->ce_mode)) &&\n-\t\t    S_ISREG(ntohl(e3->ce_mode))) {\n+\t\t    S_ISREG(e2->ce_mode) &&\n+\t\t    S_ISREG(e3->ce_mode)) {\n \t\t\tpath_list_insert((const char *)e2->name, conflict);\n \t\t\ti++; /* skip over both #2 and #3 */\n \t\t}\ndiff --git a/builtin-update-index.c b/builtin-update-index.c\nindex e1a938d..b5a34ed 100644\n--- a/builtin-update-index.c\n+++ b/builtin-update-index.c\n@@ -47,10 +47,10 @@ static int mark_valid(const char *path)\n \tif (0 <= pos) {\n \t\tswitch (mark_valid_only) {\n \t\tcase MARK_VALID:\n-\t\t\tactive_cache[pos]->ce_flags |= htons(CE_VALID);\n+\t\t\tactive_cache[pos]->ce_flags |= CE_VALID;\n \t\t\tbreak;\n \t\tcase UNMARK_VALID:\n-\t\t\tactive_cache[pos]->ce_flags &= ~htons(CE_VALID);\n+\t\t\tactive_cache[pos]->ce_flags &= ~CE_VALID;\n \t\t\tbreak;\n \t\t}\n \t\tcache_tree_invalidate_path(active_cache_tree, path);\n@@ -95,7 +95,7 @@ static int add_one_path(struct cache_entry *old, const char *path, int len, stru\n \tsize = cache_entry_size(len);\n \tce = xcalloc(1, size);\n \tmemcpy(ce->name, path, len);\n-\tce->ce_flags = htons(len);\n+\tce->ce_flags = len;\n \tfill_stat_cache_info(ce, st);\n \tce->ce_mode = ce_mode_from_stat(old, st->st_mode);\n \n@@ -139,7 +139,7 @@ static int process_directory(const char *path, int len, struct stat *st)\n \t/* Exact match: file or existing gitlink */\n \tif (pos >= 0) {\n \t\tstruct cache_entry *ce = active_cache[pos];\n-\t\tif (S_ISGITLINK(ntohl(ce->ce_mode))) {\n+\t\tif (S_ISGITLINK(ce->ce_mode)) {\n \n \t\t\t/* Do nothing to the index if there is no HEAD! */\n \t\t\tif (resolve_gitlink_ref(path, \"HEAD\", sha1) < 0)\n@@ -183,7 +183,7 @@ static int process_file(const char *path, int len, struct stat *st)\n \tint pos = cache_name_pos(path, len);\n \tstruct cache_entry *ce = pos < 0 ? NULL : active_cache[pos];\n \n-\tif (ce && S_ISGITLINK(ntohl(ce->ce_mode)))\n+\tif (ce && S_ISGITLINK(ce->ce_mode))\n \t\treturn error(\"%s is already a gitlink, not replacing\", path);\n \n \treturn add_one_path(ce, path, len, st);\n@@ -226,7 +226,7 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,\n \tce->ce_flags = create_ce_flags(len, stage);\n \tce->ce_mode = create_ce_mode(mode);\n \tif (assume_unchanged)\n-\t\tce->ce_flags |= htons(CE_VALID);\n+\t\tce->ce_flags |= CE_VALID;\n \toption = allow_add ? ADD_CACHE_OK_TO_ADD : 0;\n \toption |= allow_replace ? ADD_CACHE_OK_TO_REPLACE : 0;\n \tif (add_cache_entry(ce, option))\n@@ -246,14 +246,14 @@ static void chmod_path(int flip, const char *path)\n \tif (pos < 0)\n \t\tgoto fail;\n \tce = active_cache[pos];\n-\tmode = ntohl(ce->ce_mode);\n+\tmode = ce->ce_mode;\n \tif (!S_ISREG(mode))\n \t\tgoto fail;\n \tswitch (flip) {\n \tcase '+':\n-\t\tce->ce_mode |= htonl(0111); break;\n+\t\tce->ce_mode |= 0111; break;\n \tcase '-':\n-\t\tce->ce_mode &= htonl(~0111); break;\n+\t\tce->ce_mode &= ~0111; break;\n \tdefault:\n \t\tgoto fail;\n \t}\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 50b3526..3ef5f87 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -320,7 +320,7 @@ static int update_one(struct cache_tree *it,\n \t\t}\n \t\telse {\n \t\t\tsha1 = ce->sha1;\n-\t\t\tmode = ntohl(ce->ce_mode);\n+\t\t\tmode = ce->ce_mode;\n \t\t\tentlen = pathlen - baselen;\n \t\t}\n \t\tif (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1))\ndiff --git a/cache.h b/cache.h\nindex 39331c2..0aed11e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -94,17 +94,31 @@ struct cache_time {\n  * We save the fields in big-endian order to allow using the\n  * index file over NFS transparently.\n  */\n+struct ondisk_cache_entry {\n+\tstruct cache_time ctime;\n+\tstruct cache_time mtime;\n+\tunsigned int dev;\n+\tunsigned int ino;\n+\tunsigned int mode;\n+\tunsigned int uid;\n+\tunsigned int gid;\n+\tunsigned int size;\n+\tunsigned char sha1[20];\n+\tunsigned short flags;\n+\tchar name[FLEX_ARRAY]; /* more */\n+};\n+\n struct cache_entry {\n-\tstruct cache_time ce_ctime;\n-\tstruct cache_time ce_mtime;\n+\tunsigned int ce_ctime;\n+\tunsigned int ce_mtime;\n \tunsigned int ce_dev;\n \tunsigned int ce_ino;\n \tunsigned int ce_mode;\n \tunsigned int ce_uid;\n \tunsigned int ce_gid;\n \tunsigned int ce_size;\n+\tunsigned int ce_flags;\n \tunsigned char sha1[20];\n-\tunsigned short ce_flags;\n \tchar name[FLEX_ARRAY]; /* more */\n };\n \n@@ -114,28 +128,29 @@ struct cache_entry {\n #define CE_VALID     (0x8000)\n #define CE_STAGESHIFT 12\n \n-#define create_ce_flags(len, stage) htons((len) | ((stage) << CE_STAGESHIFT))\n-#define ce_namelen(ce) (CE_NAMEMASK & ntohs((ce)->ce_flags))\n+#define create_ce_flags(len, stage) ((len) | ((stage) << CE_STAGESHIFT))\n+#define ce_namelen(ce) (CE_NAMEMASK & (ce)->ce_flags)\n #define ce_size(ce) cache_entry_size(ce_namelen(ce))\n-#define ce_stage(ce) ((CE_STAGEMASK & ntohs((ce)->ce_flags)) >> CE_STAGESHIFT)\n+#define ondisk_ce_size(ce) ondisk_cache_entry_size(ce_namelen(ce))\n+#define ce_stage(ce) ((CE_STAGEMASK & (ce)->ce_flags) >> CE_STAGESHIFT)\n \n #define ce_permissions(mode) (((mode) & 0100) ? 0755 : 0644)\n static inline unsigned int create_ce_mode(unsigned int mode)\n {\n \tif (S_ISLNK(mode))\n-\t\treturn htonl(S_IFLNK);\n+\t\treturn S_IFLNK;\n \tif (S_ISDIR(mode) || S_ISGITLINK(mode))\n-\t\treturn htonl(S_IFGITLINK);\n-\treturn htonl(S_IFREG | ce_permissions(mode));\n+\t\treturn S_IFGITLINK;\n+\treturn S_IFREG | ce_permissions(mode);\n }\n static inline unsigned int ce_mode_from_stat(struct cache_entry *ce, unsigned int mode)\n {\n \textern int trust_executable_bit, has_symlinks;\n \tif (!has_symlinks && S_ISREG(mode) &&\n-\t    ce && S_ISLNK(ntohl(ce->ce_mode)))\n+\t    ce && S_ISLNK(ce->ce_mode))\n \t\treturn ce->ce_mode;\n \tif (!trust_executable_bit && S_ISREG(mode)) {\n-\t\tif (ce && S_ISREG(ntohl(ce->ce_mode)))\n+\t\tif (ce && S_ISREG(ce->ce_mode))\n \t\t\treturn ce->ce_mode;\n \t\treturn create_ce_mode(0666);\n \t}\n@@ -146,14 +161,14 @@ static inline unsigned int ce_mode_from_stat(struct cache_entry *ce, unsigned in\n \tS_ISLNK(mode) ? S_IFLNK : S_ISDIR(mode) ? S_IFDIR : S_IFGITLINK)\n \n #define cache_entry_size(len) ((offsetof(struct cache_entry,name) + (len) + 8) & ~7)\n+#define ondisk_cache_entry_size(len) ((offsetof(struct ondisk_cache_entry,name) + (len) + 8) & ~7)\n \n struct index_state {\n \tstruct cache_entry **cache;\n \tunsigned int cache_nr, cache_alloc, cache_changed;\n \tstruct cache_tree *cache_tree;\n \ttime_t timestamp;\n-\tvoid *mmap;\n-\tsize_t mmap_size;\n+\tvoid *alloc;\n };\n \n extern struct index_state the_index;\ndiff --git a/diff-lib.c b/diff-lib.c\nindex d85d8f3..c20adaa 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -37,7 +37,7 @@ static int get_mode(const char *path, int *mode)\n \tif (!path || !strcmp(path, \"/dev/null\"))\n \t\t*mode = 0;\n \telse if (!strcmp(path, \"-\"))\n-\t\t*mode = ntohl(create_ce_mode(0666));\n+\t\t*mode = create_ce_mode(0666);\n \telse if (stat(path, &st))\n \t\treturn error(\"Could not access '%s'\", path);\n \telse\n@@ -384,7 +384,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\t\tcontinue;\n \t\t\t}\n \t\t\telse\n-\t\t\t\tdpath->mode = ntohl(ce_mode_from_stat(ce, st.st_mode));\n+\t\t\t\tdpath->mode = ce_mode_from_stat(ce, st.st_mode);\n \n \t\t\twhile (i < entries) {\n \t\t\t\tstruct cache_entry *nce = active_cache[i];\n@@ -398,10 +398,10 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\t */\n \t\t\t\tstage = ce_stage(nce);\n \t\t\t\tif (2 <= stage) {\n-\t\t\t\t\tint mode = ntohl(nce->ce_mode);\n+\t\t\t\t\tint mode = nce->ce_mode;\n \t\t\t\t\tnum_compare_stages++;\n \t\t\t\t\thashcpy(dpath->parent[stage-2].sha1, nce->sha1);\n-\t\t\t\t\tdpath->parent[stage-2].mode = ntohl(ce_mode_from_stat(nce, mode));\n+\t\t\t\t\tdpath->parent[stage-2].mode = ce_mode_from_stat(nce, mode);\n \t\t\t\t\tdpath->parent[stage-2].status =\n \t\t\t\t\t\tDIFF_STATUS_MODIFIED;\n \t\t\t\t}\n@@ -442,15 +442,15 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t}\n \t\t\tif (silent_on_removed)\n \t\t\t\tcontinue;\n-\t\t\tdiff_addremove(&revs->diffopt, '-', ntohl(ce->ce_mode),\n+\t\t\tdiff_addremove(&revs->diffopt, '-', ce->ce_mode,\n \t\t\t\t       ce->sha1, ce->name, NULL);\n \t\t\tcontinue;\n \t\t}\n \t\tchanged = ce_match_stat(ce, &st, ce_option);\n \t\tif (!changed && !DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))\n \t\t\tcontinue;\n-\t\toldmode = ntohl(ce->ce_mode);\n-\t\tnewmode = ntohl(ce_mode_from_stat(ce, st.st_mode));\n+\t\toldmode = ce->ce_mode;\n+\t\tnewmode = ce_mode_from_stat(ce, st.st_mode);\n \t\tdiff_change(&revs->diffopt, oldmode, newmode,\n \t\t\t    ce->sha1, (changed ? null_sha1 : ce->sha1),\n \t\t\t    ce->name, NULL);\n@@ -471,7 +471,7 @@ static void diff_index_show_file(struct rev_info *revs,\n \t\t\t\t struct cache_entry *ce,\n \t\t\t\t unsigned char *sha1, unsigned int mode)\n {\n-\tdiff_addremove(&revs->diffopt, prefix[0], ntohl(mode),\n+\tdiff_addremove(&revs->diffopt, prefix[0], mode,\n \t\t       sha1, ce->name, NULL);\n }\n \n@@ -550,14 +550,14 @@ static int show_modified(struct rev_info *revs,\n \t\tp->len = pathlen;\n \t\tmemcpy(p->path, new->name, pathlen);\n \t\tp->path[pathlen] = 0;\n-\t\tp->mode = ntohl(mode);\n+\t\tp->mode = mode;\n \t\thashclr(p->sha1);\n \t\tmemset(p->parent, 0, 2 * sizeof(struct combine_diff_parent));\n \t\tp->parent[0].status = DIFF_STATUS_MODIFIED;\n-\t\tp->parent[0].mode = ntohl(new->ce_mode);\n+\t\tp->parent[0].mode = new->ce_mode;\n \t\thashcpy(p->parent[0].sha1, new->sha1);\n \t\tp->parent[1].status = DIFF_STATUS_MODIFIED;\n-\t\tp->parent[1].mode = ntohl(old->ce_mode);\n+\t\tp->parent[1].mode = old->ce_mode;\n \t\thashcpy(p->parent[1].sha1, old->sha1);\n \t\tshow_combined_diff(p, 2, revs->dense_combined_merges, revs);\n \t\tfree(p);\n@@ -569,9 +569,6 @@ static int show_modified(struct rev_info *revs,\n \t    !DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))\n \t\treturn 0;\n \n-\tmode = ntohl(mode);\n-\toldmode = ntohl(oldmode);\n-\n \tdiff_change(&revs->diffopt, oldmode, mode,\n \t\t    old->sha1, sha1, old->name, NULL);\n \treturn 0;\n@@ -628,7 +625,7 @@ static int diff_cache(struct rev_info *revs,\n \t\t\t\t\t   cached, match_missing))\n \t\t\t\tbreak;\n \t\t\tdiff_unmerge(&revs->diffopt, ce->name,\n-\t\t\t\t     ntohl(ce->ce_mode), ce->sha1);\n+\t\t\t\t     ce->ce_mode, ce->sha1);\n \t\t\tbreak;\n \t\tcase 3:\n \t\t\tdiff_unmerge(&revs->diffopt, ce->name,\n@@ -664,7 +661,7 @@ static void mark_merge_entries(void)\n \t\tstruct cache_entry *ce = active_cache[i];\n \t\tif (!ce_stage(ce))\n \t\t\tcontinue;\n-\t\tce->ce_flags |= htons(CE_STAGEMASK);\n+\t\tce->ce_flags |= CE_STAGEMASK;\n \t}\n }\n \n@@ -723,7 +720,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)\n \t\t\t\t\t\t   ce->name);\n \t\t\tlast = ce;\n \t\t\tce->ce_mode = 0;\n-\t\t\tce->ce_flags &= ~htons(CE_STAGEMASK);\n+\t\t\tce->ce_flags &= ~CE_STAGEMASK;\n \t\t}\n \t\t*dst++ = ce;\n \t}\ndiff --git a/dir.c b/dir.c\nindex 3e345c2..1b9cc7a 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -391,7 +391,7 @@ static enum exist_status directory_exists_in_index(const char *dirname, int len)\n \t\t\tbreak;\n \t\tif (endchar == '/')\n \t\t\treturn index_directory;\n-\t\tif (!endchar && S_ISGITLINK(ntohl(ce->ce_mode)))\n+\t\tif (!endchar && S_ISGITLINK(ce->ce_mode))\n \t\t\treturn index_gitdir;\n \t}\n \treturn index_nonexistent;\ndiff --git a/entry.c b/entry.c\nindex 257ab46..44f4b89 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -103,7 +103,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \tint fd;\n \tlong wrote;\n \n-\tswitch (ntohl(ce->ce_mode) & S_IFMT) {\n+\tswitch (ce->ce_mode & S_IFMT) {\n \t\tchar *new;\n \t\tstruct strbuf buf;\n \t\tunsigned long size;\n@@ -129,7 +129,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \t\t\tstrcpy(path, \".merge_file_XXXXXX\");\n \t\t\tfd = mkstemp(path);\n \t\t} else\n-\t\t\tfd = create_file(path, ntohl(ce->ce_mode));\n+\t\t\tfd = create_file(path, ce->ce_mode);\n \t\tif (fd < 0) {\n \t\t\tfree(new);\n \t\t\treturn error(\"git-checkout-index: unable to create file %s (%s)\",\n@@ -221,7 +221,7 @@ int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *t\n \t\tunlink(path);\n \t\tif (S_ISDIR(st.st_mode)) {\n \t\t\t/* If it is a gitlink, leave it alone! */\n-\t\t\tif (S_ISGITLINK(ntohl(ce->ce_mode)))\n+\t\t\tif (S_ISGITLINK(ce->ce_mode))\n \t\t\t\treturn 0;\n \t\t\tif (!state->force)\n \t\t\t\treturn error(\"%s is a directory\", path);\ndiff --git a/merge-index.c b/merge-index.c\nindex fa719cb..bbb700b 100644\n--- a/merge-index.c\n+++ b/merge-index.c\n@@ -48,7 +48,7 @@ static int merge_entry(int pos, const char *path)\n \t\t\tbreak;\n \t\tfound++;\n \t\tstrcpy(hexbuf[stage], sha1_to_hex(ce->sha1));\n-\t\tsprintf(ownbuf[stage], \"%o\", ntohl(ce->ce_mode));\n+\t\tsprintf(ownbuf[stage], \"%o\", ce->ce_mode);\n \t\targuments[stage] = hexbuf[stage];\n \t\targuments[stage + 4] = ownbuf[stage];\n \t} while (++pos < active_nr);\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex b34177d..0db0b3a 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -333,7 +333,7 @@ static struct path_list *get_unmerged(void)\n \t\t\titem->util = xcalloc(1, sizeof(struct stage_data));\n \t\t}\n \t\te = item->util;\n-\t\te->stages[ce_stage(ce)].mode = ntohl(ce->ce_mode);\n+\t\te->stages[ce_stage(ce)].mode = ce->ce_mode;\n \t\thashcpy(e->stages[ce_stage(ce)].sha, ce->sha1);\n \t}\n \ndiff --git a/reachable.c b/reachable.c\nindex 6383401..00f289f 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -176,7 +176,7 @@ static void add_cache_refs(struct rev_info *revs)\n \t\t * lookup_blob() on them, to avoid populating the hash table\n \t\t * with invalid information\n \t\t */\n-\t\tif (S_ISGITLINK(ntohl(active_cache[i]->ce_mode)))\n+\t\tif (S_ISGITLINK(active_cache[i]->ce_mode))\n \t\t\tcontinue;\n \n \t\tlookup_blob(active_cache[i]->sha1);\ndiff --git a/read-cache.c b/read-cache.c\nindex 7db5588..4414a40 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -30,20 +30,16 @@ struct index_state the_index;\n  */\n void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n {\n-\tce->ce_ctime.sec = htonl(st->st_ctime);\n-\tce->ce_mtime.sec = htonl(st->st_mtime);\n-#ifdef USE_NSEC\n-\tce->ce_ctime.nsec = htonl(st->st_ctim.tv_nsec);\n-\tce->ce_mtime.nsec = htonl(st->st_mtim.tv_nsec);\n-#endif\n-\tce->ce_dev = htonl(st->st_dev);\n-\tce->ce_ino = htonl(st->st_ino);\n-\tce->ce_uid = htonl(st->st_uid);\n-\tce->ce_gid = htonl(st->st_gid);\n-\tce->ce_size = htonl(st->st_size);\n+\tce->ce_ctime = st->st_ctime;\n+\tce->ce_mtime = st->st_mtime;\n+\tce->ce_dev = st->st_dev;\n+\tce->ce_ino = st->st_ino;\n+\tce->ce_uid = st->st_uid;\n+\tce->ce_gid = st->st_gid;\n+\tce->ce_size = st->st_size;\n \n \tif (assume_unchanged)\n-\t\tce->ce_flags |= htons(CE_VALID);\n+\t\tce->ce_flags |= CE_VALID;\n }\n \n static int ce_compare_data(struct cache_entry *ce, struct stat *st)\n@@ -116,7 +112,7 @@ static int ce_modified_check_fs(struct cache_entry *ce, struct stat *st)\n \t\t\treturn DATA_CHANGED;\n \t\tbreak;\n \tcase S_IFDIR:\n-\t\tif (S_ISGITLINK(ntohl(ce->ce_mode)))\n+\t\tif (S_ISGITLINK(ce->ce_mode))\n \t\t\treturn 0;\n \tdefault:\n \t\treturn TYPE_CHANGED;\n@@ -128,14 +124,14 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)\n {\n \tunsigned int changed = 0;\n \n-\tswitch (ntohl(ce->ce_mode) & S_IFMT) {\n+\tswitch (ce->ce_mode & S_IFMT) {\n \tcase S_IFREG:\n \t\tchanged |= !S_ISREG(st->st_mode) ? TYPE_CHANGED : 0;\n \t\t/* We consider only the owner x bit to be relevant for\n \t\t * \"mode changes\"\n \t\t */\n \t\tif (trust_executable_bit &&\n-\t\t    (0100 & (ntohl(ce->ce_mode) ^ st->st_mode)))\n+\t\t    (0100 & (ce->ce_mode ^ st->st_mode)))\n \t\t\tchanged |= MODE_CHANGED;\n \t\tbreak;\n \tcase S_IFLNK:\n@@ -152,29 +148,17 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)\n \tcase 0: /* Special case: unmerged file in index */\n \t\treturn MODE_CHANGED | DATA_CHANGED | TYPE_CHANGED;\n \tdefault:\n-\t\tdie(\"internal error: ce_mode is %o\", ntohl(ce->ce_mode));\n+\t\tdie(\"internal error: ce_mode is %o\", ce->ce_mode);\n \t}\n-\tif (ce->ce_mtime.sec != htonl(st->st_mtime))\n+\tif (ce->ce_mtime != (unsigned int) st->st_mtime)\n \t\tchanged |= MTIME_CHANGED;\n-\tif (ce->ce_ctime.sec != htonl(st->st_ctime))\n+\tif (ce->ce_ctime != (unsigned int) st->st_ctime)\n \t\tchanged |= CTIME_CHANGED;\n \n-#ifdef USE_NSEC\n-\t/*\n-\t * nsec seems unreliable - not all filesystems support it, so\n-\t * as long as it is in the inode cache you get right nsec\n-\t * but after it gets flushed, you get zero nsec.\n-\t */\n-\tif (ce->ce_mtime.nsec != htonl(st->st_mtim.tv_nsec))\n-\t\tchanged |= MTIME_CHANGED;\n-\tif (ce->ce_ctime.nsec != htonl(st->st_ctim.tv_nsec))\n-\t\tchanged |= CTIME_CHANGED;\n-#endif\n-\n-\tif (ce->ce_uid != htonl(st->st_uid) ||\n-\t    ce->ce_gid != htonl(st->st_gid))\n+\tif (ce->ce_uid != (unsigned int) st->st_uid ||\n+\t    ce->ce_gid != (unsigned int) st->st_gid)\n \t\tchanged |= OWNER_CHANGED;\n-\tif (ce->ce_ino != htonl(st->st_ino))\n+\tif (ce->ce_ino != (unsigned int) st->st_ino)\n \t\tchanged |= INODE_CHANGED;\n \n #ifdef USE_STDEV\n@@ -183,11 +167,11 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)\n \t * clients will have different views of what \"device\"\n \t * the filesystem is on\n \t */\n-\tif (ce->ce_dev != htonl(st->st_dev))\n+\tif (ce->ce_dev != (unsigned int) st->st_dev)\n \t\tchanged |= INODE_CHANGED;\n #endif\n \n-\tif (ce->ce_size != htonl(st->st_size))\n+\tif (ce->ce_size != (unsigned int) st->st_size)\n \t\tchanged |= DATA_CHANGED;\n \n \treturn changed;\n@@ -205,7 +189,7 @@ int ie_match_stat(struct index_state *istate,\n \t * If it's marked as always valid in the index, it's\n \t * valid whatever the checked-out copy says.\n \t */\n-\tif (!ignore_valid && (ce->ce_flags & htons(CE_VALID)))\n+\tif (!ignore_valid && (ce->ce_flags & CE_VALID))\n \t\treturn 0;\n \n \tchanged = ce_match_stat_basic(ce, st);\n@@ -228,7 +212,7 @@ int ie_match_stat(struct index_state *istate,\n \t */\n \tif (!changed &&\n \t    istate->timestamp &&\n-\t    istate->timestamp <= ntohl(ce->ce_mtime.sec)) {\n+\t    istate->timestamp <= ce->ce_mtime) {\n \t\tif (assume_racy_is_modified)\n \t\t\tchanged |= DATA_CHANGED;\n \t\telse\n@@ -320,7 +304,7 @@ int index_name_pos(struct index_state *istate, const char *name, int namelen)\n \twhile (last > first) {\n \t\tint next = (last + first) >> 1;\n \t\tstruct cache_entry *ce = istate->cache[next];\n-\t\tint cmp = cache_name_compare(name, namelen, ce->name, ntohs(ce->ce_flags));\n+\t\tint cmp = cache_name_compare(name, namelen, ce->name, ce->ce_flags);\n \t\tif (!cmp)\n \t\t\treturn next;\n \t\tif (cmp < 0) {\n@@ -405,7 +389,7 @@ int add_file_to_index(struct index_state *istate, const char *path, int verbose)\n \tsize = cache_entry_size(namelen);\n \tce = xcalloc(1, size);\n \tmemcpy(ce->name, path, namelen);\n-\tce->ce_flags = htons(namelen);\n+\tce->ce_flags = namelen;\n \tfill_stat_cache_info(ce, &st);\n \n \tif (trust_executable_bit && has_symlinks)\n@@ -616,7 +600,7 @@ static int has_dir_name(struct index_state *istate,\n \t\t}\n \t\tlen = slash - name;\n \n-\t\tpos = index_name_pos(istate, name, ntohs(create_ce_flags(len, stage)));\n+\t\tpos = index_name_pos(istate, name, create_ce_flags(len, stage));\n \t\tif (pos >= 0) {\n \t\t\t/*\n \t\t\t * Found one, but not so fast.  This could\n@@ -704,7 +688,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \tint skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;\n \n \tcache_tree_invalidate_path(istate->cache_tree, ce->name);\n-\tpos = index_name_pos(istate, ce->name, ntohs(ce->ce_flags));\n+\tpos = index_name_pos(istate, ce->name, ce->ce_flags);\n \n \t/* existing match? Just replace it. */\n \tif (pos >= 0) {\n@@ -736,7 +720,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \t\tif (!ok_to_replace)\n \t\t\treturn error(\"'%s' appears as both a file and as a directory\",\n \t\t\t\t     ce->name);\n-\t\tpos = index_name_pos(istate, ce->name, ntohs(ce->ce_flags));\n+\t\tpos = index_name_pos(istate, ce->name, ce->ce_flags);\n \t\tpos = -pos-1;\n \t}\n \treturn pos + 1;\n@@ -810,7 +794,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,\n \t\t * valid again, under \"assume unchanged\" mode.\n \t\t */\n \t\tif (ignore_valid && assume_unchanged &&\n-\t\t    !(ce->ce_flags & htons(CE_VALID)))\n+\t\t    !(ce->ce_flags & CE_VALID))\n \t\t\t; /* mark this one VALID again */\n \t\telse\n \t\t\treturn ce;\n@@ -826,7 +810,6 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,\n \tupdated = xmalloc(size);\n \tmemcpy(updated, ce, size);\n \tfill_stat_cache_info(updated, &st);\n-\n \t/*\n \t * If ignore_valid is not set, we should leave CE_VALID bit\n \t * alone.  Otherwise, paths marked with --no-assume-unchanged\n@@ -834,8 +817,8 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,\n \t * automatically, which is not really what we want.\n \t */\n \tif (!ignore_valid && assume_unchanged &&\n-\t    !(ce->ce_flags & htons(CE_VALID)))\n-\t\tupdated->ce_flags &= ~htons(CE_VALID);\n+\t    !(ce->ce_flags & CE_VALID))\n+\t\tupdated->ce_flags &= ~CE_VALID;\n \n \treturn updated;\n }\n@@ -880,7 +863,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p\n \t\t\t\t/* If we are doing --really-refresh that\n \t\t\t\t * means the index is not valid anymore.\n \t\t\t\t */\n-\t\t\t\tce->ce_flags &= ~htons(CE_VALID);\n+\t\t\t\tce->ce_flags &= ~CE_VALID;\n \t\t\t\tistate->cache_changed = 1;\n \t\t\t}\n \t\t\tif (quiet)\n@@ -942,16 +925,34 @@ int read_index(struct index_state *istate)\n \treturn read_index_from(istate, get_index_file());\n }\n \n+static void convert_from_disk(struct ondisk_cache_entry *ondisk, struct cache_entry *ce)\n+{\n+\tce->ce_ctime = ntohl(ondisk->ctime.sec);\n+\tce->ce_mtime = ntohl(ondisk->mtime.sec);\n+\tce->ce_dev   = ntohl(ondisk->dev);\n+\tce->ce_ino   = ntohl(ondisk->ino);\n+\tce->ce_mode  = ntohl(ondisk->mode);\n+\tce->ce_uid   = ntohl(ondisk->uid);\n+\tce->ce_gid   = ntohl(ondisk->gid);\n+\tce->ce_size  = ntohl(ondisk->size);\n+\t/* On-disk flags are just 16 bits */\n+\tce->ce_flags = ntohs(ondisk->flags);\n+\thashcpy(ce->sha1, ondisk->sha1);\n+\tmemcpy(ce->name, ondisk->name, ce_namelen(ce)+1);\n+}\n+\n /* remember to discard_cache() before reading a different cache! */\n int read_index_from(struct index_state *istate, const char *path)\n {\n \tint fd, i;\n \tstruct stat st;\n-\tunsigned long offset;\n+\tunsigned long src_offset, dst_offset;\n \tstruct cache_header *hdr;\n+\tvoid *mmap;\n+\tsize_t mmap_size;\n \n \terrno = EBUSY;\n-\tif (istate->mmap)\n+\tif (istate->alloc)\n \t\treturn istate->cache_nr;\n \n \terrno = ENOENT;\n@@ -967,31 +968,47 @@ int read_index_from(struct index_state *istate, const char *path)\n \t\tdie(\"cannot stat the open index (%s)\", strerror(errno));\n \n \terrno = EINVAL;\n-\tistate->mmap_size = xsize_t(st.st_size);\n-\tif (istate->mmap_size < sizeof(struct cache_header) + 20)\n+\tmmap_size = xsize_t(st.st_size);\n+\tif (mmap_size < sizeof(struct cache_header) + 20)\n \t\tdie(\"index file smaller than expected\");\n \n-\tistate->mmap = xmmap(NULL, istate->mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n+\tmmap = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n+\tif (mmap == MAP_FAILED)\n+\t\tdie(\"unable to map index file\");\n \n-\thdr = istate->mmap;\n-\tif (verify_hdr(hdr, istate->mmap_size) < 0)\n+\thdr = mmap;\n+\tif (verify_hdr(hdr, mmap_size) < 0)\n \t\tgoto unmap;\n \n \tistate->cache_nr = ntohl(hdr->hdr_entries);\n \tistate->cache_alloc = alloc_nr(istate->cache_nr);\n \tistate->cache = xcalloc(istate->cache_alloc, sizeof(struct cache_entry *));\n \n-\toffset = sizeof(*hdr);\n+\t/*\n+\t * The disk format is actually larger than the in-memory format,\n+\t * due to space for nsec etc, so even though the in-memory one\n+\t * has room for a few  more flags, we can allocate using the same\n+\t * index size\n+\t */\n+\tistate->alloc = xmalloc(mmap_size);\n+\n+\tsrc_offset = sizeof(*hdr);\n+\tdst_offset = 0;\n \tfor (i = 0; i < istate->cache_nr; i++) {\n+\t\tstruct ondisk_cache_entry *disk_ce;\n \t\tstruct cache_entry *ce;\n \n-\t\tce = (struct cache_entry *)((char *)(istate->mmap) + offset);\n-\t\toffset = offset + ce_size(ce);\n+\t\tdisk_ce = (struct ondisk_cache_entry *)((char *)mmap + src_offset);\n+\t\tce = (struct cache_entry *)((char *)istate->alloc + dst_offset);\n+\t\tconvert_from_disk(disk_ce, ce);\n \t\tistate->cache[i] = ce;\n+\n+\t\tsrc_offset += ondisk_ce_size(ce);\n+\t\tdst_offset += ce_size(ce);\n \t}\n \tistate->timestamp = st.st_mtime;\n-\twhile (offset <= istate->mmap_size - 20 - 8) {\n+\twhile (src_offset <= mmap_size - 20 - 8) {\n \t\t/* After an array of active_nr index entries,\n \t\t * there can be arbitrary number of extended\n \t\t * sections, each of which is prefixed with\n@@ -999,40 +1016,36 @@ int read_index_from(struct index_state *istate, const char *path)\n \t\t * in 4-byte network byte order.\n \t\t */\n \t\tunsigned long extsize;\n-\t\tmemcpy(&extsize, (char *)(istate->mmap) + offset + 4, 4);\n+\t\tmemcpy(&extsize, (char *)mmap + src_offset + 4, 4);\n \t\textsize = ntohl(extsize);\n \t\tif (read_index_extension(istate,\n-\t\t\t\t\t ((const char *) (istate->mmap)) + offset,\n-\t\t\t\t\t (char *) (istate->mmap) + offset + 8,\n+\t\t\t\t\t (const char *) mmap + src_offset,\n+\t\t\t\t\t (char *) mmap + src_offset + 8,\n \t\t\t\t\t extsize) < 0)\n \t\t\tgoto unmap;\n-\t\toffset += 8;\n-\t\toffset += extsize;\n+\t\tsrc_offset += 8;\n+\t\tsrc_offset += extsize;\n \t}\n+\tmunmap(mmap, mmap_size);\n \treturn istate->cache_nr;\n \n unmap:\n-\tmunmap(istate->mmap, istate->mmap_size);\n+\tmunmap(mmap, mmap_size);\n \terrno = EINVAL;\n \tdie(\"index file corrupt\");\n }\n \n int discard_index(struct index_state *istate)\n {\n-\tint ret;\n-\n \tistate->cache_nr = 0;\n \tistate->cache_changed = 0;\n \tistate->timestamp = 0;\n \tcache_tree_free(&(istate->cache_tree));\n-\tif (istate->mmap == NULL)\n-\t\treturn 0;\n-\tret = munmap(istate->mmap, istate->mmap_size);\n-\tistate->mmap = NULL;\n-\tistate->mmap_size = 0;\n+\tfree(istate->alloc);\n+\tistate->alloc = NULL;\n \n \t/* no need to throw away allocated active_cache */\n-\treturn ret;\n+\treturn 0;\n }\n \n #define WRITE_BUFFER_SIZE 8192\n@@ -1148,6 +1161,28 @@ static void ce_smudge_racily_clean_entry(struct cache_entry *ce)\n \t}\n }\n \n+static int ce_write_entry(SHA_CTX *c, int fd, struct cache_entry *ce)\n+{\n+\tint size = ondisk_ce_size(ce);\n+\tstruct ondisk_cache_entry *ondisk = xcalloc(1, size);\n+\n+\tondisk->ctime.sec = htonl(ce->ce_ctime);\n+\tondisk->ctime.nsec = 0;\n+\tondisk->mtime.sec = htonl(ce->ce_mtime);\n+\tondisk->mtime.nsec = 0;\n+\tondisk->dev  = htonl(ce->ce_dev);\n+\tondisk->ino  = htonl(ce->ce_ino);\n+\tondisk->mode = htonl(ce->ce_mode);\n+\tondisk->uid  = htonl(ce->ce_uid);\n+\tondisk->gid  = htonl(ce->ce_gid);\n+\tondisk->size = htonl(ce->ce_size);\n+\thashcpy(ondisk->sha1, ce->sha1);\n+\tondisk->flags = htons(ce->ce_flags);\n+\tmemcpy(ondisk->name, ce->name, ce_namelen(ce));\n+\n+\treturn ce_write(c, fd, ondisk, size);\n+}\n+\n int write_index(struct index_state *istate, int newfd)\n {\n \tSHA_CTX c;\n@@ -1173,9 +1208,9 @@ int write_index(struct index_state *istate, int newfd)\n \t\tif (!ce->ce_mode)\n \t\t\tcontinue;\n \t\tif (istate->timestamp &&\n-\t\t    istate->timestamp <= ntohl(ce->ce_mtime.sec))\n+\t\t    istate->timestamp <= ce->ce_mtime)\n \t\t\tce_smudge_racily_clean_entry(ce);\n-\t\tif (ce_write(&c, newfd, ce, ce_size(ce)) < 0)\n+\t\tif (ce_write_entry(&c, newfd, ce) < 0)\n \t\t\treturn -1;\n \t}\n \ndiff --git a/sha1_name.c b/sha1_name.c\nindex 13e1164..be8489e 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -695,7 +695,7 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)\n \t\t\t\tbreak;\n \t\t\tif (ce_stage(ce) == stage) {\n \t\t\t\thashcpy(sha1, ce->sha1);\n-\t\t\t\t*mode = ntohl(ce->ce_mode);\n+\t\t\t\t*mode = ce->ce_mode;\n \t\t\t\treturn 0;\n \t\t\t}\n \t\t\tpos++;\ndiff --git a/tree.c b/tree.c\nindex 8c0819f..87708ef 100644\n--- a/tree.c\n+++ b/tree.c\n@@ -142,8 +142,8 @@ static int cmp_cache_name_compare(const void *a_, const void *b_)\n \n \tce1 = *((const struct cache_entry **)a_);\n \tce2 = *((const struct cache_entry **)b_);\n-\treturn cache_name_compare(ce1->name, ntohs(ce1->ce_flags),\n-\t\t\t\t  ce2->name, ntohs(ce2->ce_flags));\n+\treturn cache_name_compare(ce1->name, ce1->ce_flags,\n+\t\t\t\t  ce2->name, ce2->ce_flags);\n }\n \n int read_tree(struct tree *tree, int stage, const char **match)\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex aa2513e..9205320 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -289,7 +289,7 @@ static struct checkout state;\n static void check_updates(struct cache_entry **src, int nr,\n \t\t\tstruct unpack_trees_options *o)\n {\n-\tunsigned short mask = htons(CE_UPDATE);\n+\tunsigned short mask = CE_UPDATE;\n \tunsigned cnt = 0, total = 0;\n \tstruct progress *progress = NULL;\n \tchar last_symlink[PATH_MAX];\n@@ -408,7 +408,7 @@ static void verify_uptodate(struct cache_entry *ce,\n \t\t * submodules that are marked to be automatically\n \t\t * checked out.\n \t\t */\n-\t\tif (S_ISGITLINK(ntohl(ce->ce_mode)))\n+\t\tif (S_ISGITLINK(ce->ce_mode))\n \t\t\treturn;\n \t\terrno = 0;\n \t}\n@@ -450,7 +450,7 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,\n \tint cnt = 0;\n \tunsigned char sha1[20];\n \n-\tif (S_ISGITLINK(ntohl(ce->ce_mode)) &&\n+\tif (S_ISGITLINK(ce->ce_mode) &&\n \t    resolve_gitlink_ref(ce->name, \"HEAD\", sha1) == 0) {\n \t\t/* If we are not going to update the submodule, then\n \t\t * we don't care.\n@@ -580,7 +580,7 @@ static void verify_absent(struct cache_entry *ce, const char *action,\n static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n \t\tstruct unpack_trees_options *o)\n {\n-\tmerge->ce_flags |= htons(CE_UPDATE);\n+\tmerge->ce_flags |= CE_UPDATE;\n \tif (old) {\n \t\t/*\n \t\t * See if we can re-use the old CE directly?\n@@ -601,7 +601,7 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n \t\tinvalidate_ce_path(merge);\n \t}\n \n-\tmerge->ce_flags &= ~htons(CE_STAGEMASK);\n+\tmerge->ce_flags &= ~CE_STAGEMASK;\n \tadd_cache_entry(merge, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n \treturn 1;\n }\n@@ -634,7 +634,7 @@ static void show_stage_entry(FILE *o,\n \telse\n \t\tfprintf(o, \"%s%06o %s %d\\t%s\\n\",\n \t\t\tlabel,\n-\t\t\tntohl(ce->ce_mode),\n+\t\t\tce->ce_mode,\n \t\t\tsha1_to_hex(ce->sha1),\n \t\t\tce_stage(ce),\n \t\t\tce->name);\n@@ -920,7 +920,7 @@ int oneway_merge(struct cache_entry **src,\n \t\t\tstruct stat st;\n \t\t\tif (lstat(old->name, &st) ||\n \t\t\t    ce_match_stat(old, &st, CE_MATCH_IGNORE_VALID))\n-\t\t\t\told->ce_flags |= htons(CE_UPDATE);\n+\t\t\t\told->ce_flags |= CE_UPDATE;\n \t\t}\n \t\treturn keep_entry(old, o);\n \t}\n"},{"id":"65331","messageId":"7vodbojhkj.fsf@gitster.siamese.dyndns.org","threadId":"11601","inReplyTo":"alpine.LFD.1.00.0801141132250.2806@woody.linux-foundation.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-14T20:08:44Z","receivedAt":"2008-01-14T20:08:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Mon, 14 Jan 2008, Linus Torvalds wrote:\n>> \n>> So I think this patch is good, but I think it would be even better if we \n>> just bit the bullet and started looking at having a different in-memory \n>> representation from the on-disk one.\n>\n> Ok, so here's a possible patch.\n>\n> It passes all the tests for me, and looks fairly ok, but it's also a bit \n> big.\n>\n> What makes it big is that I made the in-memory format be in host order, so \n> that we can remove a *lot* of the \"htonl/ntohl\" switcheroo, and do it just \n> on index file read/write.\n>\n> The nice thing about this patch is that it would make it a lot easier to \n> do any index handling changes,  because it makes a clear difference \n> between the on-disk and the in-memory formats.\n>\n> I realize that the patch looks big (195 lines inserted and 148 lines \n> removed), but *most* of the lines are literally those ntohl() \n> simplifications, ie stuff like\n>\n> \t-       if (S_ISGITLINK(ntohl(ce->ce_mode))) {\n> \t+       if (S_ISGITLINK(ce->ce_mode)) {\n>\n> so while it adds lines (for the \"convert from disk\" and \"convert to disk\" \n> format conversions), in  many ways it really simplifies the source code \n> too.\n>\n> Comments?\n>\n> This is on top of current master, so it's *before* junios thing that adds \n> a CE_UPTODATE.\n>\n> With this, the high 16 bits of \"ce_flags\" are in-memory only, so you could \n> just make CE_UPTODATE be 0x10000, and it automatically ends up never being \n> written to disk (and always \"reads\" as zero).\n>\n> \t\tLinus\n\n> diff --git a/cache.h b/cache.h\n> index 39331c2..0aed11e 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -94,17 +94,31 @@ struct cache_time {\n>   * We save the fields in big-endian order to allow using the\n>   * index file over NFS transparently.\n>   */\n> +struct ondisk_cache_entry {\n> +\tstruct cache_time ctime;\n> +\tstruct cache_time mtime;\n> +\tunsigned int dev;\n> +\tunsigned int ino;\n> +\tunsigned int mode;\n> +\tunsigned int uid;\n> +\tunsigned int gid;\n> +\tunsigned int size;\n> +\tunsigned char sha1[20];\n> +\tunsigned short flags;\n> +\tchar name[FLEX_ARRAY]; /* more */\n> +};\n> +\n>  struct cache_entry {\n> -\tstruct cache_time ce_ctime;\n> -\tstruct cache_time ce_mtime;\n> +\tunsigned int ce_ctime;\n> +\tunsigned int ce_mtime;\n>  \tunsigned int ce_dev;\n>  \tunsigned int ce_ino;\n>  \tunsigned int ce_mode;\n>  \tunsigned int ce_uid;\n>  \tunsigned int ce_gid;\n>  \tunsigned int ce_size;\n> +\tunsigned int ce_flags;\n>  \tunsigned char sha1[20];\n> -\tunsigned short ce_flags;\n>  \tchar name[FLEX_ARRAY]; /* more */\n>  };\n\nIf we are using different types anyway, we might want to start\nusing time_t (a worse alternative is ulong which we use for\ntimestamps everywhere else, which we probably want to convert to\ntime_t as well).\n\nIs there still a reason to insist that ce_flags should be a\nsingle field that is multi-purposed for storing stage, namelen\nand other flags?  Wouldn't the code become even simpler and\nsafer if we separated them into individual fields?  For example,\na piece like this:\n\n@@ -2388,7 +2388,7 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned\n \tce = xcalloc(1, ce_size);\n \tmemcpy(ce->name, path, namelen);\n \tce->ce_mode = create_ce_mode(mode);\n-\tce->ce_flags = htons(namelen);\n+\tce->ce_flags = namelen;\n \tif (S_ISGITLINK(mode)) {\n \t\tconst char *s = buf;\n \nstill has that \"names longer than 4096 bytes go unchecked,\ncorrupting stage information\" issue.\n\n+static void convert_from_disk(struct ondisk_cache_entry *ondisk, struct cache_entry *ce)\n+{\n+\tce->ce_ctime = ntohl(ondisk->ctime.sec);\n+\tce->ce_mtime = ntohl(ondisk->mtime.sec);\n+\tce->ce_dev   = ntohl(ondisk->dev);\n+\tce->ce_ino   = ntohl(ondisk->ino);\n+\tce->ce_mode  = ntohl(ondisk->mode);\n+\tce->ce_uid   = ntohl(ondisk->uid);\n+\tce->ce_gid   = ntohl(ondisk->gid);\n+\tce->ce_size  = ntohl(ondisk->size);\n+\t/* On-disk flags are just 16 bits */\n+\tce->ce_flags = ntohs(ondisk->flags);\n+\thashcpy(ce->sha1, ondisk->sha1);\n+\tmemcpy(ce->name, ondisk->name, ce_namelen(ce)+1);\n+}\n\nI presume that the fix to handle names that are longer than 4096\nbytes naturally fits here.  We can make the low 12-bits of\nondisk->ce_flags all 1 for such names and we actually\ncount the strlen to populate ce->ce_namelen.\n\n> +\t/*\n> +\t * The disk format is actually larger than the in-memory format,\n> +\t * due to space for nsec etc, so even though the in-memory one\n> +\t * has room for a few  more flags, we can allocate using the same\n> +\t * index size\n> +\t */\n> +\tistate->alloc = xmalloc(mmap_size);\n> +\n> +\tsrc_offset = sizeof(*hdr);\n> +\tdst_offset = 0;\n>  \tfor (i = 0; i < istate->cache_nr; i++) {\n> +\t\tstruct ondisk_cache_entry *disk_ce;\n>  \t\tstruct cache_entry *ce;\n>  \n> -\t\tce = (struct cache_entry *)((char *)(istate->mmap) + offset);\n> -\t\toffset = offset + ce_size(ce);\n> +\t\tdisk_ce = (struct ondisk_cache_entry *)((char *)mmap + src_offset);\n> +\t\tce = (struct cache_entry *)((char *)istate->alloc + dst_offset);\n> +\t\tconvert_from_disk(disk_ce, ce);\n>  \t\tistate->cache[i] = ce;\n> +\n> +\t\tsrc_offset += ondisk_ce_size(ce);\n> +\t\tdst_offset += ce_size(ce);\n>  \t}\n>  \tistate->timestamp = st.st_mtime;\n\nI somehow had this impression that it was a huge deal to you\nthat we do not have to read and populate each cache entry when\nreading from the existing index file, and thought that was the\nreason why we mmap and access the fields in network byte order.\nIf that was my misconception, then I agree this is a good change\nto make everything else easier to write and much less error\nprone.\n"},{"id":"65336","messageId":"alpine.LFD.1.00.0801141250340.2806@woody.linux-foundation.org","threadId":"11601","inReplyTo":"7vodbojhkj.fsf@gitster.siamese.dyndns.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-14T21:00:38Z","receivedAt":"2008-01-14T21:00:38Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 14 Jan 2008, Junio C Hamano wrote:\n> \n> If we are using different types anyway, we might want to start\n> using time_t (a worse alternative is ulong which we use for\n> timestamps everywhere else, which we probably want to convert to\n> time_t as well).\n\nCareful.\n\nThere are two issues, one trivial one and one important one:\n (a) trivially, right now, the code depends on the fact that the in-memory \n     structure is actually smaller than the on-disk one, to avoid having \n     to estimate the size of the allocation for the in-memory array. That \n     was a matter of gettign a quickly working and efficient patch (we do \n     *not* want to allocate those initial \"struct cache_entry\" entries one \n     by one, we want to allocate one big block!)\n\n     This should be pretty easy to fix up, by just taking the sizes and \n     number of entries (which we do know) into account of the initial \n     allocation. However, it's made a bit more interesting by the \n     differing alignment of the \"name\" part (and the fact that we align \n     each individual on-disk and in-memory structure).\n\n (b) More importantly, the on-disk structures DO NOT CONTAIN the whole \n     stat information! The classic example of this is \"ce_size\": it's \n     32-bit, but it works even if you have a file that is larger than 32 \n     bits in size! It just means that from a stat comparison standpoint, \n     we only compare the low 32 bits!\n\n     This means that if you make \"ce_size\" be a \"loff_t\", for example, you \n     still need to then *compare* it in just an \"unsigned int\",  because \n     the upper bits aren't zero - they are \"nonexistent\".\n\nthat (b) is important, and is why some of the code changed from\n\n\t-       if (ce->ce_ino != htonl(st->st_ino))\n\t+       if (ce->ce_ino != (unsigned int) st->st_ino)\n\nie note how this didn't just remove the \"htonl()\", it replaced it by a \n\"truncate to 'unsigned int'\"!\n\nSo the fact that the types aren't necessarily the \"native\" types is \nactually *important*.\n\n> Is there still a reason to insist that ce_flags should be a\n> single field that is multi-purposed for storing stage, namelen\n> and other flags?  Wouldn't the code become even simpler and\n> safer if we separated them into individual fields?  For example,\n> a piece like this:\n\nNo reason for that part, except I wanted to make this particular initial \npatch be as minimal as possible.\n\n> I somehow had this impression that it was a huge deal to you\n> that we do not have to read and populate each cache entry when\n> reading from the existing index file, and thought that was the\n> reason why we mmap and access the fields in network byte order.\n> If that was my misconception, then I agree this is a good change\n> to make everything else easier to write and much less error\n> prone.\n\nI was a bit worried about it, but I did make sure that the allocation is \ndone as one single allocation, and I did time it. Doing a \n\n\tgit update-index --refresh\n\nseems to be identical before and after, so the costs of conversion are \neither very small or are possibly counteracted by the fact that we then \ncan avoid the byte-order conversion of individual words less at run-time.\n\n\t\tLinus\n"},{"id":"65335","messageId":"7vejckjf1o.fsf@gitster.siamese.dyndns.org","threadId":"11601","inReplyTo":"20080113233323.GB19970@steel.home","subject":"Re: [PATCH] index: be careful when handling long names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-14T21:03:15Z","receivedAt":"2008-01-14T21:03:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> writes:\n\n> Junio C Hamano, Mon, Jan 14, 2008 00:08:07 +0100:\n> ...\n>> I would agree that it might overflow the argument limit when\n>> this is given to \"echo\", though.  We cannot do much about it,\n>> but you may have cleverer ideas.\n>\n> I thought about conditionally disabling the test, like it was done\n> when the tabs in filenames had to be tested.\n\nYes, we can and should do that when somebody reports this (or\nany other tests) a real issue, as we have done for other tests.\n"},{"id":"65338","messageId":"200801142128.m0ELSBoE030025@mi0.bluebottle.com","threadId":"11601","inReplyTo":"7v8x2uqabt.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-commit.c: remove useless check added by faulty cut and paste","fromName":"しらいしななこ","fromEmail":"nanako3@bluebottle.com","sentAt":"2008-01-14T21:23:25Z","receivedAt":"2008-01-14T21:23:25Z","isPatch":true,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Quoting Junio C Hamano <gitster@pobox.com>:\n\n> When I did 2888605c649ccd423232161186d72c0e6c458a48\n> (builtin-commit: fix partial-commit support), I mindlessly cut\n> and pasted from builtin-ls-files.c, and included the part that\n> was meant to exclude redundant path after \"ls-files --with-tree\"\n> overlayed the HEAD commit on top of the index.  This logic does\n> not apply to what git-commit does and should not have been\n> copied, even though it would not hurt.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  builtin-commit.c |    2 --\n>  1 files changed, 0 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin-commit.c b/builtin-commit.c\n> index 6d2ca80..265ba6b 100644\n> --- a/builtin-commit.c\n> +++ b/builtin-commit.c\n> @@ -156,8 +156,6 @@ static int list_paths(struct path_list *list, const char *with_tree,\n>  \n>  \tfor (i = 0; i < active_nr; i++) {\n>  \t\tstruct cache_entry *ce = active_cache[i];\n> -\t\tif (ce->ce_flags & htons(CE_UPDATE))\n> -\t\t\tcontinue;\n>  \t\tif (!pathspec_match(pattern, m, ce->name, 0))\n>  \t\t\tcontinue;\n>  \t\tpath_list_insert(ce->name, list);\n\nI think I may be wrong, but is this change really correct?\n\nYou are calling overlay_tree_on_cache() which does use CE_UPDATE flag to mark\nduplicate entries.  And that is the same algorithm as used when git-ls-files\nis called with its --with-tree option.  I think this if statement is not\nmindless but is the right thing to have the same logic as you have in\ngit-ls-files.\n\n-- \nNanako Shiraishi\nhttp://ivory.ap.teacup.com/nanako3/\n\n----------------------------------------------------------------------\nFinally - A spam blocker that actually works.\nhttp://www.bluebottle.com/tag/4\n"},{"id":"65342","messageId":"7vsl10hy4c.fsf@gitster.siamese.dyndns.org","threadId":"11601","inReplyTo":"200801142128.m0ELSBoE030025@mi0.bluebottle.com","subject":"Re: [PATCH] builtin-commit.c: remove useless check added by faulty cut and paste","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-14T21:54:11Z","receivedAt":"2008-01-14T21:54:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"しらいしななこ  <nanako3@bluebottle.com> writes:\n\n> You are calling overlay_tree_on_cache() which does use CE_UPDATE flag to mark\n> duplicate entries.  And that is the same algorithm as used when git-ls-files\n> is called with its --with-tree option.  I think this if statement is not\n> mindless but is the right thing to have the same logic as you have in\n> git-ls-files.\n\nYou are right and I was stupid.\n\nBecause the pathname ce->name is given to path_list_insert()\nwhich does not allow duplicates, there is no breakage either way\nfrom the correctness point of view in this codepath, unlike the\none in ls-files.  But avoiding unnecessary processing with a\nsingle bit check is certainly better.\n\nWill revert, but I first have to go find a brown paper bag.\n"},{"id":"65344","messageId":"1200352558.488.10.camel@gaara.boston.redhat.com","threadId":"11601","inReplyTo":"alpine.LFD.1.00.0801121735020.2806@woody.linux-foundation.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2008-01-14T23:15:58Z","receivedAt":"2008-01-14T23:15:58Z","isPatch":false,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"On Sat, 2008-01-12 at 17:46 -0800, Linus Torvalds wrote:\n\n> HOWEVER. When that logic was converted from that shell-script into a \n> builtin-commit.c, that conversion was not done correctly. The old \"git \n> read-tree -i -m\" was not translated as a \"unpack_trees()\" call, but as \n> this in prepare_index():\n> \n> \tdiscard_cache()\n> \t..\n> \ttree = parse_tree_indirect(head_sha1);\n> \t..\n> \tread_tree(tree, 0, NULL)\n> \n> which is very wrong, because it replaces the old index entirely, and \n> doesn't do that stat information merging.\n> \n> As a result, the index that is created by read-tree is totally bogus in \n> the stat cache, and yes, everything will have to be re-computed.\n> \n> Kristian?\n\nSorry for being late to the game, and yes, it's a bug I introduced with\nthe rewrite.  When doing the rewrite I was a bit puzzled by the\n\n  git-read-tree --index-output=\"$TMP_INDEX\" -i -m HEAD\n\npart of the shell script.  I carried a FIXME around in the patch for a\nwhile, as can be seen here:\n\n  http://marc.info/?l=git&m=118478660425992&w=2\n\nsince I couldn't figure out what the difference in behavior was between\njust using read_tree(), which did exactly what I wanted and the more\ncomplicated unpack_tree().  I guess it fell through the cracks,\nespecially since it never caused the test suite to fail :/\n\nKristian\n"},{"id":"65348","messageId":"1200354379.488.17.camel@gaara.boston.redhat.com","threadId":"11601","inReplyTo":"alpine.LFD.1.00.0801121949180.2806@woody.linux-foundation.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2008-01-14T23:46:19Z","receivedAt":"2008-01-14T23:46:19Z","isPatch":false,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"On Sat, 2008-01-12 at 20:04 -0800, Linus Torvalds wrote:\n\n> It makes builtin-commit.c use the same logic that \"git read-tree -i -m\" \n> does (which is what the old shell script did), and it seems to pass the \n> test-suite, and it looks pretty obvious.\n> \n> It also brings down the number of open/mmap/munmap/close calls to where it \n> should be, although it still does *way* too many \"lstat()\" operations (ie \n> it does 4*lstat for each file in the index - one more than the \n> non-filename one does).\n> \n> With that fixed, performance is also roughly where it should be (ie the \n> 17-18s for the cold-cache case), because it no longer needs to rehash all \n> the files!\n> \n> HOWEVER. This was just a quick hack, and while it all looks sane, this is \n> some damn core code. Somebody else should double- and triple-check this.\n\nI took a look too, and it looks to me like the it's the exact same code\npath in builtin-read-tree.c that the old\n\n        git read-tree --index-output=\"$TMP_INDEX\" -i -m HEAD\n\npart of the shell script would trigger.  So yes, this look like the\nright fix to me.\n\nSigned-off-by: Kristian Høgsberg <krh@redhat.com>\n"},{"id":"65349","messageId":"7vodbohstl.fsf@gitster.siamese.dyndns.org","threadId":"11601","inReplyTo":"1200352558.488.10.camel@gaara.boston.redhat.com","subject":"Re: performance problem: \"git commit filename\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-14T23:48:38Z","receivedAt":"2008-01-14T23:48:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristian Høgsberg <krh@redhat.com> writes:\n\n> ...  I guess it fell through the cracks,\n> especially since it never caused the test suite to fail :/\n\nYeah, the breakage was not about the correctness, and because I\nalmost never do partial commit I did not notice it until Linus\nbrought it up.\n"},{"id":"65350","messageId":"alpine.LFD.1.00.0801141552090.2806@woody.linux-foundation.org","threadId":"11601","inReplyTo":"7vodbohstl.fsf@gitster.siamese.dyndns.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-14T23:53:35Z","receivedAt":"2008-01-14T23:53:35Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 14 Jan 2008, Junio C Hamano wrote:\n> \n> Yeah, the breakage was not about the correctness, and because I\n> almost never do partial commit I did not notice it until Linus\n> brought it up.\n\nI wouldn't have noticed it either, if it wasn't for the fact that my \nkernel tree was out-of-cache for other testing reasons. When cached, the \ndifference was still quite noticeable, but I would probably not have \nnoticed the difference between half a second and a second and a half.\n\nBut a commit that took over a minute due to IO was very noticeable \nindeed..\n\n\t\tLinus\n"},{"id":"65352","messageId":"alpine.LFD.1.00.0801141611560.2806@woody.linux-foundation.org","threadId":"11601","inReplyTo":"7vr6glnrvp.fsf@gitster.siamese.dyndns.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-15T00:18:06Z","receivedAt":"2008-01-15T00:18:06Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 13 Jan 2008, Junio C Hamano wrote:\n> \n> I've reworked the patch, and in the kernel repository, a\n> single-path commit after touching that path now calls 23k\n> lstat(2).  It used to call 46k lstat(2) after your fix.\n\nHmm. This part of it looks incorrect:\n\n> diff --git a/diff.c b/diff.c\n> index b18c140..62d0c06 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1510,6 +1510,10 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int\n>  \tif (pos < 0)\n>  \t\treturn 0;\n>  \tce = active_cache[pos];\n> +\n> +\tif (ce_uptodate(ce))\n> +\t\treturn 1;\n> +\n>  \tif ((lstat(name, &st) < 0) ||\n>  \t    !S_ISREG(st.st_mode) || /* careful! */\n>  \t    ce_match_stat(ce, &st, 0) ||\n\nIsn't this wrong? I think it also needs to check that ce->sha1 matches the \nright SHA1, because even if the lstat() information may be fine, if the \nSHA1 doesn't match what we want, we still shouldn't use the checked-out \ncopy, of course.\n\nThe old code continues with a\n\n\t   hashcmp(sha1, ce->sha1))\n\t\treturn 0;\n\nin that if-statement that is partially visible in the context, and it's \nthat hashcmp() that got incorrectly cut off from the logic.\n\n(Of course, maybe we never call this function unless we've already checked \nthat the cache-entry SHA1 matches, but if so, that subsequent hashcmp \nshould just be removed instead).\n\n\t\tLinus\n"},{"id":"65355","messageId":"7vd4s3j3fz.fsf@gitster.siamese.dyndns.org","threadId":"11601","inReplyTo":"alpine.LFD.1.00.0801141611560.2806@woody.linux-foundation.org","subject":"Re: performance problem: \"git commit filename\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-15T01:13:52Z","receivedAt":"2008-01-15T01:13:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Sun, 13 Jan 2008, Junio C Hamano wrote:\n>> \n>> I've reworked the patch, and in the kernel repository, a\n>> single-path commit after touching that path now calls 23k\n>> lstat(2).  It used to call 46k lstat(2) after your fix.\n>\n> Hmm. This part of it looks incorrect:\n>\n>> diff --git a/diff.c b/diff.c\n>> index b18c140..62d0c06 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -1510,6 +1510,10 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int\n>>  \tif (pos < 0)\n>>  \t\treturn 0;\n>>  \tce = active_cache[pos];\n>> +\n>> +\tif (ce_uptodate(ce))\n>> +\t\treturn 1;\n>> +\n>>  \tif ((lstat(name, &st) < 0) ||\n>>  \t    !S_ISREG(st.st_mode) || /* careful! */\n>>  \t    ce_match_stat(ce, &st, 0) ||\n>\n> Isn't this wrong?\n\nYou are right.  We call this with sha1 that may not necessarily\nbe the same as ce->sha1.\n\nThe code should probably be something like:\n\n\tce = active_cache[pos];\n\n\t/*\n         * Even if ce matches the work tree, it is not what we can\n\t * reuse for sha1, if the hash is different or not a\n         * regular blob.\n         */\n\tif (hashcmp(sha1, ce->sha1) || !S_ISREG(ntohl(ce->st_mode))\n\t\treturn 0;\n\t/*\n         * Does ce actually match the work tree?  If so we can reuse.\n         */\n\tif (ce_uptodate(ce) ||\n\t    (!lstat(name, &st) && !ce_match_stat(ce, &st, 0)))\n\t\treturn 1;\n\treturn 0;\n\nThe expression inside the latter if () condition should probably\nbe the new abstraction at the level of ce_modified().\n\nCurrently ce_modified() assumes that lstat(2) is cheap and the\ncallers have called it on paths they are interested in already.\n"}]}