# [PATCH] commit: write out cache-tree information 4 messages from 2011-08-02 to 2011-08-02. Participants: trast@student.ethz.ch, Junio C Hamano, Thomas Rast. Thread: https://gitlist.dev/t/27996 ## trast@student.ethz.ch, 2011-08-02 16:36 Subject: [PATCH] commit: write out cache-tree information Message-ID: <9f5e031045b55c4738de8bc9aae290827e128cd3.1312302862.git.trast@student.ethz.ch> URL: https://gitlist.dev/e/9f5e031045b55c4738de8bc9aae290827e128cd3.1312302862.git.trast%40student.ethz.ch ``` From: Thomas Rast While write-tree has code to write out the cache-tree information (since we have to compute it anyway if the cache is stale), commit lost this capability when it became a builtin and moved away from using write-tree. Add the necessary code to write out the cache. This is extremely similar to what write_cache_as_tree() does. Reported-by: Carlos Martín Nieto Signed-off-by: Thomas Rast --- Reported on IRC. There's a similar disparity between read-tree and checkout, but I first have to understand the exact conditions in or around unpack_trees() [checkout.c:417] where the read-tree reasoning applies. builtin/commit.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index e1af9b1..47b0eea 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -637,6 +637,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix, const char *hook_arg2 = NULL; int ident_shown = 0; int clean_message_contents = (cleanup_mode != CLEANUP_NONE); + int fd; + struct lock_file *lock_file; if (!no_verify && run_hook(index_file, "pre-commit", NULL)) return 0; @@ -861,6 +863,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix, * the editor and after we invoke run_status above. */ discard_cache(); + lock_file = xcalloc(1, sizeof(struct lock_file)); + fd = hold_lock_file_for_update(lock_file, index_file, LOCK_DIE_ON_ERROR); read_cache_from(index_file); if (!active_cache_tree) active_cache_tree = cache_tree(); @@ -869,6 +873,11 @@ static int prepare_to_commit(const char *index_file, const char *prefix, error(_("Error building trees")); return 0; } + if (0 <= fd) { + if (write_cache(fd, active_cache, active_nr) || + commit_lock_file(lock_file)) + rollback_lock_file(lock_file); + } if (run_hook(index_file, "prepare-commit-msg", git_path(commit_editmsg), hook_arg1, hook_arg2, NULL)) -- 1.7.6.668.g17b0a ``` ## Junio C Hamano, 2011-08-02 18:13 Subject: Re: [PATCH] commit: write out cache-tree information Message-ID: <7vmxfrel63.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmxfrel63.fsf%40alter.siamese.dyndns.org In-Reply-To: <9f5e031045b55c4738de8bc9aae290827e128cd3.1312302862.git.trast@student.ethz.ch> ``` writes: > From: Thomas Rast > > While write-tree has code to write out the cache-tree information > (since we have to compute it anyway if the cache is stale), commit > lost this capability when it became a builtin and moved away from > using write-tree. Earlier the code read from the index, made sure that it is not unmerged by running cache_tere_update(), before running prepare-commit-msg hook. The hook used to see the index that was read in this codepath which is the same as what pre-commit left us. Why run an extra I/O here? The index file could be quite large, and I do not want people to writing it out without good reason. The tree object that is committed is taken from active_cache_tree->sha1 in cmd_commit() and not from the cache tree you are writing out to the index file. ``` ## Junio C Hamano, 2011-08-02 21:15 Subject: Re: [PATCH] commit: write out cache-tree information Message-ID: <7vfwljecqv.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vfwljecqv.fsf%40alter.siamese.dyndns.org In-Reply-To: <7vmxfrel63.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: >> While write-tree has code to write out the cache-tree information >> (since we have to compute it anyway if the cache is stale), commit >> lost this capability when it became a builtin and moved away from >> using write-tree. > > Earlier the code read from the index, made sure that it is not unmerged by > running cache_tere_update(), before running prepare-commit-msg hook. The > hook used to see the index that was read in this codepath which is the > same as what pre-commit left us. > > Why run an extra I/O here? The index file could be quite large, and I do > not want people to writing it out without good reason. One possible good reason is if you did this only when we are committing the whole index. Then theoretically the next operation after commit could start from a fully populated cache-tree in the index. When we are running a partial commit, the index file you are writing back is a temporary index only to build a tree object to record in the commit, which we already have done, and the temporary will be discarded. ``` ## Thomas Rast, 2011-08-02 22:01 Subject: Re: [PATCH] commit: write out cache-tree information Message-ID: <201108030001.53476.trast@student.ethz.ch> URL: https://gitlist.dev/e/201108030001.53476.trast%40student.ethz.ch In-Reply-To: <7vmxfrel63.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano wrote: > writes: > > > From: Thomas Rast > > > > While write-tree has code to write out the cache-tree information > > (since we have to compute it anyway if the cache is stale), commit > > lost this capability when it became a builtin and moved away from > > using write-tree. > > Earlier the code read from the index, made sure that it is not unmerged by > running cache_tere_update(), before running prepare-commit-msg hook. The > hook used to see the index that was read in this codepath which is the > same as what pre-commit left us. > > Why run an extra I/O here? The index file could be quite large, and I do > not want people to writing it out without good reason. Ok, so let's run some numbers. With the first test script below I'm seeing: before patch: $ time ./commit-in-large-tree.sh Initialized empty Git repository in /dev/shm/commit-in-large-tree.tmp/.git/ 6.9M .git/index real 1m31.607s user 0m57.604s sys 0m29.976s after patch: 14% speedup $ time ./commit-in-large-tree.sh Initialized empty Git repository in /dev/shm/commit-in-large-tree.tmp/.git/ 7.0M .git/index real 1m18.521s user 0m53.430s sys 0m22.138s On the other hand if you touch every file as in the second script: before patch: $ time ./commit-in-large-tree-2.sh Initialized empty Git repository in /dev/shm/commit-in-large-tree.tmp/.git/ 6.9M .git/index real 1m40.910s user 0m58.731s sys 0m38.011s after patch: 5% slowdown $ time ./commit-in-large-tree-2.sh Initialized empty Git repository in /dev/shm/commit-in-large-tree.tmp/.git/ 7.0M .git/index real 1m45.465s user 1m2.329s sys 0m38.849s I also ran the latter test where it only touches one file in 100 (instead of all 1000) subdirs, and there the patch is still a speedup. So I guess it depends whether we expect users to mostly modify a small part or the whole tree. Regarding your other email > When we are running a partial commit, the index file you are writing back > is a temporary index only to build a tree object to record in the commit, > which we already have done, and the temporary will be discarded. that's a valid point that I need to address. -- 8< -- commit-in-large-tree.sh #!/bin/sh set -e git init /dev/shm/commit-in-large-tree.tmp cd /dev/shm/commit-in-large-tree.tmp for i in $(seq 1 1000); do mkdir $i ( cd $i for j in $(seq 1 100); do echo $j > $j done ) git add $i done git commit -q -m initial du -h .git/index for i in $(seq 1 100); do echo "$i changed" > $i/$i git add $i/$i git commit -q -m $i done rm -rf /dev/shm/commit-in-large-tree.tmp -- >8 -- -- 8< -- commit-in-large-tree-2.sh #!/bin/sh set -e git init /dev/shm/commit-in-large-tree.tmp cd /dev/shm/commit-in-large-tree.tmp for i in $(seq 1 1000); do mkdir $i ( cd $i for j in $(seq 1 100); do echo $j > $j done ) git add $i done git commit -q -m initial du -h .git/index for i in $(seq 1 100); do for j in $(seq 1 1000); do echo "$i changed" > $j/$i done git add -u git commit -q -m $i done rm -rf /dev/shm/commit-in-large-tree.tmp -- >3 -- -- Thomas Rast trast@{inf,student}.ethz.ch ```