Re: [PATCH v4 4/4] cache-tree: Write updated cache-tree after commit

[Junio C Hamano <gitster@pobox.com>]

David Turner <dturner@twopensource.com> writes:

> During the commit process, update the cache-tree. 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.  Make all tests test the entire cache-tree, not just the root
> level.
>
> Signed-off-by: David Turner <dturner@twitter.com>
> ---
>  builtin/commit.c      | 31 +++++++++++-------
>  t/t0090-cache-tree.sh | 87 ++++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 91 insertions(+), 27 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 9cfef6c..5981755 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);
>  
>  		commit_style = COMMIT_NORMAL;
>  		return index_lock.filename;
> @@ -363,10 +365,18 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
>  		fd = hold_locked_index(&index_lock, 1);
>  		add_files_to_cache(also ? prefix : NULL, &pathspec, 0);
>  		refresh_cache_or_die(refresh_flags);
> -		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"));
> +		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) ||
> +			    close_lock_file(&index_lock))
> +				die(_("unable to write new_index file"));
> +		} else {
> +			rollback_lock_file(&index_lock);
> +		}
>  		commit_style = COMMIT_NORMAL;
>  		return index_lock.filename;
>  	}
> @@ -383,14 +393,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"));
>  		commit_style = COMMIT_AS_IS;
>  		return get_index_file();
>  	}

Hmph.  The above two hunks somehow feel the other way around.  When
doing a non-partial, non as-is commit, i.e. "git commit $paths", the
original used to call 'write-cache' unconditionally, because it is
very unlikely for us to see !active_cache_changed after calling the
add-files-to-cache with user-supplied pathspec (if there is nothing
to change, the user is being silly to say "git commit $paths" in the
first place).  I would have expected that the patch would have left
that code path alone (it seems to be doing the right thing already).

On the other hand, "git commit" to commit the contents of the index
as-is is being cautious not to write things out unnecessarily, but
as you found out, it would be a good idea to fully populate the
cache-tree in this code path and write the otherwise already-good
index file out, even if we see !active_cache_changed after we called
refresh_cache_or_die().  So I would have expected that the patch
would have kept the "write only necessary" carefulness instead of
calling write-cache unconditionally.

That is, something like:

	fd = hold_locked_index();
        refresh_cache_or_die();
+	if (!cache_tree_fully_valid())
+               active_cache_changed = 1;
	if (active_cache_changed) {
		update_main_cache_tree();

> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
> index 8437c5f..15f1484 100755
> --- a/t/t0090-cache-tree.sh
> +++ b/t/t0090-cache-tree.sh
> @@ -7,8 +7,14 @@ cache-tree extension.
>  "
>   . ./test-lib.sh
>  
> +grep_nonmatch_ok () {
> +    grep $@

dq around it, i.e. "$@".

> +    test "$?" = "2" && return 1

POSIX.1 only specifies >1 not necessarily 2 as an error status.  

> +    return 0
> +}

Having said all that I do not think the helper is unnecessary.  Just
use something like

	sed -e '/#(ref)/d'

>  cmp_cache_tree () {
> -	test-dump-cache-tree >actual &&
> +	test-dump-cache-tree | grep_nonmatch_ok -v \#\(ref\) >actual &&