David Turner <dturner@xxxxxxxxxxxxxxxx> writes:
> 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@xxxxxxxxxxx>
> ---
> 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)?
> @@ -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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html