Re: [RFC PATCH] cache-tree: Teach write_cache_as_tree to discard_cache

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> If the read_cache() call succeeds, the function must call
> discard_cache() before returning to the caller.

Does this mean the callers (current ones and also future ones) cannot
continue using the index after writing it out as a tree?

I would imagine anything that wants to write more than one tree (imagine:
reimplementation of "git stash" in C) would want to do something like:

	read_cache();
        write_cache_as_tree(index_tree_sha1[]);
        create commit to record that tree, with HEAD as the parent,
            and call that i_commit;

	for (all path in work tree different from index)
	        add_file_to_index();
        write_cache_as_tree(worktree_tree_sha1[]);
        create commit to record that tree, with HEAD as the parent;

        create commit to record the same tree, with HEAD and i_commit,
            and return it as the result of "git stash create".

Of course you _could_ force the caller to read_cache() what you wrote out
again immediately, but I do not see why you want to do so.  Especially in
the non-error codepath, I do not see why this could be a good change.

> diff --git a/cache-tree.c b/cache-tree.c
> index f755590..17c5bab 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -573,8 +573,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
>  
>  		if (cache_tree_update(active_cache_tree,
>  				      active_cache, active_nr,
> -				      missing_ok, 0) < 0)
> +				      missing_ok, 0) < 0) {
> +			discard_cache();
>  			return WRITE_TREE_UNMERGED_INDEX;

Also this may not be what the callers want, either.

If your goal is to update the existing code even more reusable by new
callers, I would understand if you introduced a "quiet" mode to
cache_tree_update() codepath (especially, verify_cache()), so that the
callers can choose to re-inspect the index in the error case and give a
better diagnostics, suggestion, or even auto-correction, depending on the
application. That would be a good way to restructure the current API to
give more control to the application.

Running discard_cache() here, however, would forever forbid any such
callers to be written, without reverting this change.

> +		}
>  		if (0 <= newfd) {
>  			if (!write_cache(newfd, active_cache, active_nr) &&
>  			    !commit_lock_file(lock_file))
> @@ -591,8 +593,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
>  	if (prefix) {
>  		struct cache_tree *subtree =
>  			cache_tree_find(active_cache_tree, prefix);
> -		if (!subtree)
> +		if (!subtree) {
> +			discard_cache();
>  			return WRITE_TREE_PREFIX_ERROR;

Same here.

> +		}
>  		hashcpy(sha1, subtree->sha1);
>  	}
>  	else
> @@ -601,6 +605,7 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
>  	if (0 <= newfd)
>  		rollback_lock_file(lock_file);
>  
> +	discard_cache();
>  	return 0;
>  }

I am afraid that I have to say that the approach this patch takes is
totally backwards from the point of view of good API design.  Perhaps
there is an (unstated) other goal you are trying to achieve, but I cannot
read it from the patch.