Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout
David Turner <dturner@twopensource.com> writes:
Show 26 quoted lines
> When git checkout checks out a branch, create or update the
> cache-tree so that subsequent operations are faster.
>
> Signed-off-by: David Turner <dturner@twitter.com>
> ---
> builtin/checkout.c | 8 ++++++++
> cache-tree.c | 5 +++--
> t/t0090-cache-tree.sh | 15 ++++++++++++++-
> 3 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 07cf555..a023a86 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -553,6 +553,14 @@ static int merge_working_tree(const struct checkout_opts *opts,
> }
> }
>
> + if (!active_cache_tree)
> + active_cache_tree = cache_tree();
> +
> + if (!cache_tree_fully_valid(active_cache_tree))
> + cache_tree_update(active_cache_tree,
> + (const struct cache_entry * const *)active_cache,
> + active_nr, 0);
> +
This looks much better than the previous round, but it still does allow verify_cache() to throw noises against unmerged entries in the cache, as WRITE_TREE_SILENT flag is not passed down, no?
$ git checkout master^0
$ git am $this_message
$ make
$ edit builtin/checkout.c ;# make changes to the above lines
$ ./git checkout -m master^0
x builtin/checkout.c: unmerged (972c8a7b28f16f88475268f9a714048c228e69db)
x builtin/checkout.c: unmerged (f1dc56e55f7b2200412142b10517458ccfda2952)
x builtin/checkout.c: unmerged (3b9753ba8c19e7dfe6e922f30eb85c83a92a4596)
M builtin/checkout.c
Warning: you are leaving 1 commit behind, not connected to
any of your branches: 25fab54 cache-tree: Create/update cache-tree on checkout
Switched to branch 'master'
Passing WRITE_TREE_SILENT in the flags parameter will get rid of the conflict notice output from the above.
The user is not interested in writing a brand new tree object at all in this case, so it feels wrong to actually let the call chain go down to update_one() and create new tree objects.
Side note. And passing WRITE_TREE_DRY_RUN is not a good
solution either, because a later write_cache_as_tree() will
not create the necessary tree object once you stuff a tree
object name in the cache-tree.
What we want in this code path is a way to repair a sub cache_tree if it can be repaired without creating a new tree object and otherwise leave that part invalid. The existing cache-tree framework is not prepared to do that kind of thing. It wants to start from the bottom and percolate things up, computing levels nearer to the top-level only after it fully created the trees for deeper levels, because it is meant to be used only when we really want to write out trees. We may want to reuse update_one() but
I am not convinced that doing an equivalent of write-tree when you switch branches is the right approach in the first place. You will eventually write it out as a tree, and having a relatively undamaged cache-tree will help you when you do so, but spending the cycles necessary to compute a fully populated cache-tree, only to let it degrade over time until you are ready to write it out as a tree, somehow sounds like asking for a duplicated work upfront.
By the way, you seem to have touched write_cache_as_tree() in the same patch, but I am not sure what makes the change necessary. I do not see a new caller to it that passes a NULL to its sha1 parameter.
Show 45 quoted lines
> diff --git a/cache-tree.c b/cache-tree.c
> index 7fa524a..28d2356 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -612,9 +612,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
> cache_tree_find(active_cache_tree, prefix);
> if (!subtree)
> return WRITE_TREE_PREFIX_ERROR;
> - hashcpy(sha1, subtree->sha1);
> + if (sha1)
> + hashcpy(sha1, subtree->sha1);
> }
> - else
> + else if (sha1)
> hashcpy(sha1, active_cache_tree->sha1);
>
> if (0 <= newfd)
> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
> index 6c33e28..7c60675 100755
> --- a/t/t0090-cache-tree.sh
> +++ b/t/t0090-cache-tree.sh
> @@ -85,9 +85,22 @@ test_expect_success 'reset --hard without index gives cache-tree' '
> test_shallow_cache_tree
> '
>
> -test_expect_failure 'checkout gives cache-tree' '
> +test_expect_success 'checkout gives cache-tree' '
> + git tag current
> git checkout HEAD^ &&
> test_shallow_cache_tree
> '
>
> +test_expect_success 'checkout -b gives cache-tree' '
> + git checkout current &&
> + git checkout -b prev HEAD^ &&
> + test_shallow_cache_tree
> +'
> +
> +test_expect_success 'checkout -B gives cache-tree' '
> + git checkout current &&
> + git checkout -B prev HEAD^ &&
> + test_shallow_cache_tree
> +'
> +
> test_done
David Turner
