Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 1, 2014, 22:45 UTC
Message-ID: <xmqqlhscvgts.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1404242075-7068-3-git-send-email-dturner@twitter.com>

David Turner <dturner@twopensource.com> writes:

Show 22 quoted lines

> During the commit process, the cache-tree is updated. We need to write
> this updated cache-tree so that it's ready for subsequent commands.
>
> Add test code which demonstrates that git commit now writes the cache
> tree.  Also demonstrate that cache-tree invalidation is correct.
>
> Signed-off-by: David Turner <dturner@twitter.com>
> ---
>  builtin/commit.c      | 15 ++++++------
>  t/t0090-cache-tree.sh | 63 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 67 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 9cfef6c..dbd4f4b 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -342,6 +342,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
>  
>  		discard_cache();
>  		read_cache_from(index_lock.filename);
> +		if (update_main_cache_tree(WRITE_TREE_SILENT) >= 0)
> +			write_cache(fd, active_cache, active_nr);

OK, interactive-add may leave the cache-tree not quite populated; we are going to write out a tree from the cache so we need to update the in-core cache tree anyway, so calling update-main-cache-tree here would not hurt (it will speed up the write-cache-as-tree we will eventually call).

We might want to see if we are really changing anything, though. What happens if the interactive-add gave us an index with fully valid cache-tree? Is that rare enough not to matter (not a rhetorical question)?

Show 16 quoted lines

> @@ -383,14 +385,10 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
>  	if (!only && !pathspec.nr) {
>  		fd = hold_locked_index(&index_lock, 1);
>  		refresh_cache_or_die(refresh_flags);
> -		if (active_cache_changed) {
> -			update_main_cache_tree(WRITE_TREE_SILENT);
> -			if (write_cache(fd, active_cache, active_nr) ||
> -			    commit_locked_index(&index_lock))
> -				die(_("unable to write new_index file"));
> -		} else {
> -			rollback_lock_file(&index_lock);
> -		}
> +		update_main_cache_tree(WRITE_TREE_SILENT);
> +		if (write_cache(fd, active_cache, active_nr) ||
> +		    commit_locked_index(&index_lock))
> +			die(_("unable to write new_index file"));

How about doing this part like the following instead, so that we can avoid the overhead of uselessly rewriting the index file when we do not have to?

diff --git a/builtin/commit.c b/builtin/commit.c
index 5e2221c..7d730a5 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -383,8 +383,11 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 	if (!only && !pathspec.nr) {
 		fd = hold_locked_index(&index_lock, 1);
 		refresh_cache_or_die(refresh_flags);
-		if (active_cache_changed) {
+		if (active_cache_changed || !cache_tree_fully_valid(active_cache_tree)) {
 			update_main_cache_tree(WRITE_TREE_SILENT);
+			active_cache_changed = 1;
+		}
+		if (active_cache_changed) {
 			if (write_cache(fd, active_cache, active_nr) ||
 			    commit_locked_index(&index_lock))
 				die(_("unable to write new_index file"));

It makes me wonder if we should teach update_main_cache_tree() to
somehow smudge active_cache_changed bit as necessary.  Then we do
not have to make the call to update-main-cache-tree conditional.

> @@ -435,6 +433,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
>  	fd = hold_locked_index(&index_lock, 1);
>  	add_remove_files(&partial);
>  	refresh_cache(REFRESH_QUIET);
> +	update_main_cache_tree(WRITE_TREE_SILENT);
>  	if (write_cache(fd, active_cache, active_nr) ||
>  	    close_lock_file(&index_lock))
>  		die(_("unable to write new_index file"));

This is the index that will be used after we create the commit
(which will be created from a temporary index that will be discarded
immediately after we create the commit).  As we _know_ we are
changing something in this code path by calling add_remote_files(),
it is fine to call update-main-cache-tree here unconditionally.

I didn't notice it when I gave the previous review comment but while
reviewing this round, we already do the cache-tree population for
"commit -a" in this function, which suggests me that it is the right
place to do these changes.  Modulo minor niggles, I like this
iteration much better than the previous one.

Thanks for working on this.