{"thread":{"id":"27996","subject":"[PATCH] commit: write out cache-tree information","startedAt":"2011-08-02T16:36:34Z","lastAt":"2011-08-02T22:01:53Z","messageCount":4,"participants":["trast@student.ethz.ch","Junio C Hamano","Thomas Rast"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"172668","messageId":"9f5e031045b55c4738de8bc9aae290827e128cd3.1312302862.git.trast@student.ethz.ch","threadId":"27996","inReplyTo":null,"subject":"[PATCH] commit: write out cache-tree information","fromName":"","fromEmail":"trast@student.ethz.ch","sentAt":"2011-08-02T16:36:34Z","receivedAt":"2011-08-02T16:36:34Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"From: Thomas Rast \n\nWhile write-tree has code to write out the cache-tree information\n(since we have to compute it anyway if the cache is stale), commit\nlost this capability when it became a builtin and moved away from\nusing write-tree.\n\nAdd the necessary code to write out the cache. This is extremely\nsimilar to what write_cache_as_tree() does.\n\nReported-by: Carlos Martín Nieto \nSigned-off-by: Thomas Rast \n---\n\nReported on IRC.\n\nThere's a similar disparity between read-tree and checkout, but I\nfirst have to understand the exact conditions in or around\nunpack_trees() [checkout.c:417] where the read-tree reasoning applies.\n\n builtin/commit.c | 9 +++++++++\n 1 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e1af9b1..47b0eea 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -637,6 +637,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tconst char *hook_arg2 = NULL;\n \tint ident_shown = 0;\n \tint clean_message_contents = (cleanup_mode != CLEANUP_NONE);\n+\tint fd;\n+\tstruct lock_file *lock_file;\n \n \tif (!no_verify && run_hook(index_file, \"pre-commit\", NULL))\n \t\treturn 0;\n@@ -861,6 +863,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t * the editor and after we invoke run_status above.\n \t */\n \tdiscard_cache();\n+\tlock_file = xcalloc(1, sizeof(struct lock_file));\n+\tfd = hold_lock_file_for_update(lock_file, index_file, LOCK_DIE_ON_ERROR);\n \tread_cache_from(index_file);\n \tif (!active_cache_tree)\n \t\tactive_cache_tree = cache_tree();\n@@ -869,6 +873,11 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\terror(_(\"Error building trees\"));\n \t\treturn 0;\n \t}\n+\tif (0 <= fd) {\n+\t\tif (write_cache(fd, active_cache, active_nr) ||\n+\t\t commit_lock_file(lock_file))\n+\t\t\trollback_lock_file(lock_file);\n+\t}\n \n \tif (run_hook(index_file, \"prepare-commit-msg\",\n \t\t git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))\n-- \n1.7.6.668.g17b0a\n"},{"id":"172681","messageId":"7vmxfrel63.fsf@alter.siamese.dyndns.org","threadId":"27996","inReplyTo":"9f5e031045b55c4738de8bc9aae290827e128cd3.1312302862.git.trast@student.ethz.ch","subject":"Re: [PATCH] commit: write out cache-tree information","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-02T18:13:40Z","receivedAt":"2011-08-02T18:13:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":" writes:\n\n> From: Thomas Rast \n>\n> While write-tree has code to write out the cache-tree information\n> (since we have to compute it anyway if the cache is stale), commit\n> lost this capability when it became a builtin and moved away from\n> using write-tree.\n\nEarlier the code read from the index, made sure that it is not unmerged by\nrunning cache_tere_update(), before running prepare-commit-msg hook. The\nhook used to see the index that was read in this codepath which is the\nsame as what pre-commit left us.\n\nWhy run an extra I/O here? The index file could be quite large, and I do\nnot want people to writing it out without good reason.\n\nThe tree object that is committed is taken from active_cache_tree->sha1 in\ncmd_commit() and not from the cache tree you are writing out to the index\nfile.\n"},{"id":"172697","messageId":"7vfwljecqv.fsf@alter.siamese.dyndns.org","threadId":"27996","inReplyTo":"7vmxfrel63.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] commit: write out cache-tree information","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-02T21:15:36Z","receivedAt":"2011-08-02T21:15:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> While write-tree has code to write out the cache-tree information\n>> (since we have to compute it anyway if the cache is stale), commit\n>> lost this capability when it became a builtin and moved away from\n>> using write-tree.\n>\n> Earlier the code read from the index, made sure that it is not unmerged by\n> running cache_tere_update(), before running prepare-commit-msg hook. The\n> hook used to see the index that was read in this codepath which is the\n> same as what pre-commit left us.\n>\n> Why run an extra I/O here? The index file could be quite large, and I do\n> not want people to writing it out without good reason.\n\nOne possible good reason is if you did this only when we are committing\nthe whole index. Then theoretically the next operation after commit could\nstart from a fully populated cache-tree in the index.\n\nWhen we are running a partial commit, the index file you are writing back\nis a temporary index only to build a tree object to record in the commit,\nwhich we already have done, and the temporary will be discarded.\n"},{"id":"172703","messageId":"201108030001.53476.trast@student.ethz.ch","threadId":"27996","inReplyTo":"7vmxfrel63.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] commit: write out cache-tree information","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-08-02T22:01:53Z","receivedAt":"2011-08-02T22:01:53Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano wrote:\n> writes:\n>\n> > From: Thomas Rast \n> >\n> > While write-tree has code to write out the cache-tree information\n> > (since we have to compute it anyway if the cache is stale), commit\n> > lost this capability when it became a builtin and moved away from\n> > using write-tree.\n>\n> Earlier the code read from the index, made sure that it is not unmerged by\n> running cache_tere_update(), before running prepare-commit-msg hook. The\n> hook used to see the index that was read in this codepath which is the\n> same as what pre-commit left us.\n>\n> Why run an extra I/O here? The index file could be quite large, and I do\n> not want people to writing it out without good reason.\n\nOk, so let's run some numbers. With the first test script below I'm\nseeing:\n\n before patch:\n $ time ./commit-in-large-tree.sh\n Initialized empty Git repository in /dev/shm/commit-in-large-tree.tmp/.git/\n 6.9M .git/index\n\n real 1m31.607s\n user 0m57.604s\n sys 0m29.976s\n\n after patch: 14% speedup\n $ time ./commit-in-large-tree.sh\n Initialized empty Git repository in /dev/shm/commit-in-large-tree.tmp/.git/\n 7.0M .git/index\n\n real 1m18.521s\n user 0m53.430s\n sys 0m22.138s\n\nOn the other hand if you touch every file as in the second script:\n\n before patch:\n $ time ./commit-in-large-tree-2.sh \n Initialized empty Git repository in /dev/shm/commit-in-large-tree.tmp/.git/\n 6.9M .git/index\n\n real 1m40.910s\n user 0m58.731s\n sys 0m38.011s\n\n after patch: 5% slowdown\n $ time ./commit-in-large-tree-2.sh \n Initialized empty Git repository in /dev/shm/commit-in-large-tree.tmp/.git/\n 7.0M .git/index\n\n real 1m45.465s\n user 1m2.329s\n sys 0m38.849s\n\nI also ran the latter test where it only touches one file in 100\n(instead of all 1000) subdirs, and there the patch is still a speedup.\n\nSo I guess it depends whether we expect users to mostly modify a small\npart or the whole tree.\n\nRegarding your other email\n\n> When we are running a partial commit, the index file you are writing back\n> is a temporary index only to build a tree object to record in the commit,\n> which we already have done, and the temporary will be discarded.\n\nthat's a valid point that I need to address.\n\n\n\n-- 8< -- commit-in-large-tree.sh\n#!/bin/sh\n\nset -e\n\ngit init /dev/shm/commit-in-large-tree.tmp\ncd /dev/shm/commit-in-large-tree.tmp\nfor i in $(seq 1 1000); do\n mkdir $i\n (\n\tcd $i\n\tfor j in $(seq 1 100); do\n\t echo $j > $j\n\tdone\n )\n git add $i\ndone\ngit commit -q -m initial\ndu -h .git/index\n\nfor i in $(seq 1 100); do\n echo \"$i changed\" > $i/$i\n git add $i/$i\n git commit -q -m $i\ndone\n\nrm -rf /dev/shm/commit-in-large-tree.tmp\n-- >8 --\n\n-- 8< -- commit-in-large-tree-2.sh\n#!/bin/sh\n\nset -e\n\ngit init /dev/shm/commit-in-large-tree.tmp\ncd /dev/shm/commit-in-large-tree.tmp\nfor i in $(seq 1 1000); do\n mkdir $i\n (\n\tcd $i\n\tfor j in $(seq 1 100); do\n\t echo $j > $j\n\tdone\n )\n git add $i\ndone\ngit commit -q -m initial\ndu -h .git/index\n\nfor i in $(seq 1 100); do\n for j in $(seq 1 1000); do\n\techo \"$i changed\" > $j/$i\n done\n git add -u\n git commit -q -m $i\ndone\n\nrm -rf /dev/shm/commit-in-large-tree.tmp\n-- >3 --\n\n--\nThomas Rast\ntrast@{inf,student}.ethz.ch\n"}]}