unpack-trees: also allow get_progress() to work on a different index
PATCH 1/1 unpack-trees: also allow get_progress() to work on a different index
From: Elijah Newren <[email protected]>
commit b0a5a12a60 ("unpack-trees: allow check_updates() to work on a different index", 2020-03-27) allowed check_updates() to work on a different index, but it called get_progress() which was hardcoded to work on o->result much like check_updates() had been. Update it to also accept an index parameter and have check_updates() pass that parameter along so that both are working on the same index.
---
unpack-trees: also allow get_progress() to work on a different index
This is a fix to a minor 2.27 regression, from the en/sparse-checkout series.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-785%2Fnewren%2Frecent-sparse-checkout-bugfix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-785/newren/recent-sparse-checkout-bugfix-v1
Pull-Request: https://github.com/git/git/pull/785
unpack-trees.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
unpack-trees.c +3 -3
@@ -333,10 +333,10 @@ static void load_gitmodules_file(struct index_state *index, } } -static struct progress *get_progress (struct unpack_trees_options *o) +static struct progress *get_progress (struct unpack_trees_options *o, +struct index_state *index) {unsigned cnt =0 , total =0 ; -struct index_state *index = &o->result;if (!o->update || !o->verbose_update)return NULL ;
@@ -415,7 +415,7 @@ static int check_updates(struct unpack_trees_options *o,if (o->clone)setup_collided_checkout_detection (&state, index); - progress =get_progress (o); + progress =get_progress (o, index);git_attr_set_direction (GIT_ATTR_CHECKOUT );
base-commit: b994622632154fc3b17fb40a38819ad954a5fb88
Signature · 1 line
gitgitgadget
Quoted 9 lines · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
commit
b0a5a12a60("unpack-trees: allowcheck_updates()to work on a different index", 2020-03-27) allowedcheck_updates()to work on a different index, but it calledget_progress()which was hardcoded to work ono->resultmuch likecheck_updates()had been. Update it to also accept an index parameter and havecheck_updates()pass that parameter along so that both are working on the same index.
Quoted 7 lines
Noticed-by Jeff Hostetler <[email protected]>Signed-off-by Elijah Newren <[email protected]>---
unpack-trees: also allowget_progress()to work on a different indexThis is a fix to a minor 2.27 regression, from the en/sparse-checkout series.
OK, so update_sparsity() calls the function with o->src_index, get_progress() without this fix would iterate over an unrelated o->result to count the cache entries that are marked for update or removal and show the progress based on that number without this patch? The other caller in unpack_trees() uses o->result so there is no change in the behaviour with or without the patch.
The flag bits on cache entries used to count the entries in the index by the get_progress() function are applied only on the o->result side when merged_entry() and friends call do_add_entry(), no?
Do we see these CE_UPDATE|CE_WT_REMOVE bits attached to the cache entries in the o->src_index array when get_progress() is fed the src_index in the first place?
Quoted 36 lines
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-785%2Fnewren%2Frecent-sparse-checkout-bugfix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-785/newren/recent-sparse-checkout-bugfix-v1
Pull-Request: https://github.com/git/git/pull/785unpack-trees.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)unpack-trees.c +3
-3@@ -333,10 +333,10 @@ static void load_gitmodules_file(struct index_state *index, } } -static struct progress *get_progress (struct unpack_trees_options *o) +static struct progress *get_progress (struct unpack_trees_options *o, +struct index_state *index) {unsigned cnt =0 , total =0 ; -struct index_state *index = &o->result;if (!o->update || !o->verbose_update)return NULL ;@@ -415,7 +415,7 @@ static int check_updates(struct unpack_trees_options *o,if (o->clone)setup_collided_checkout_detection (&state, index); - progress =get_progress (o); + progress =get_progress (o, index);git_attr_set_direction (GIT_ATTR_CHECKOUT );base-commit:
b994622632154fc3b17fb40a38819ad954a5fb88
Quoted 20 lines · Junio C Hamano
Quoted 9 lines · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
commit
b0a5a12a60("unpack-trees: allowcheck_updates()to work on a different index", 2020-03-27) allowedcheck_updates()to work on a different index, but it calledget_progress()which was hardcoded to work ono->resultmuch likecheck_updates()had been. Update it to also accept an index parameter and havecheck_updates()pass that parameter along so that both are working on the same index.Quoted 7 lines
Noticed-by Jeff Hostetler <[email protected]>Signed-off-by Elijah Newren <[email protected]>---
unpack-trees: also allowget_progress()to work on a different indexThis is a fix to a minor 2.27 regression, from the en/sparse-checkout series.
Quoted 6 lines
OK, so
update_sparsity()calls the function witho->src_index,get_progress()without this fix would iterate over an unrelatedo->resultto count the cache entries that are marked for update or removal and show the progress based on that number without this patch? The other caller inunpack_trees()useso->resultso there is no change in the behaviour with or without the patch.
Yes.
Quoted 4 lines
The flag bits on cache entries used to count the entries in the index by the
get_progress()function are applied only on theo->resultside whenmerged_entry()and friends calldo_add_entry(), no?
Not quite; unpack_trees() also calls apply_sparse_checkout(), passing it o->result, and apply_sparse_checkout() also sets those bits.
Quoted 3 lines
Do we see these CE_UPDATE|CE_WT_REMOVE bits attached to the cache entries in the
o->src_indexarray whenget_progress()is fed thesrc_indexin the first place?
Yes, before calling check_updates(o, o->src_index), update_sparsity() loops over o->src_index and calls apply_sparse_checkout() on each of the non-conflicted cache entries. apply_sparse_checkout() will set either CE_UPDATE or CE_WT_REMOVE whenever items flip from or to having the SKIP_WORKTREE bit set.
Quoted 9 lines · Elijah Newren
Quoted 3 lines
Do we see these CE_UPDATE|CE_WT_REMOVE bits attached to the cache entries in the
o->src_indexarray whenget_progress()is fed thesrc_indexin the first place?Yes, before calling
check_updates(o, o->src_index),update_sparsity()loops overo->src_indexand callsapply_sparse_checkout()on each of the non-conflicted cache entries.apply_sparse_checkout()will set eitherCE_UPDATEorCE_WT_REMOVEwhenever items flip from or to having theSKIP_WORKTREEbit set.
Hmph.
I thought that the whole point of splitting o->result from o->src_index we did long time ago was to allow us to treat o->src_index constant. I hope we haven't broken anything by starting to do things like that X-<.
Anyway, if that is the case, this change won't make things any worse. Let's queue it.
Thanks.
Quoted 13 lines · Junio C Hamano
Quoted 9 lines · Elijah Newren
Quoted 3 lines
Do we see these CE_UPDATE|CE_WT_REMOVE bits attached to the cache entries in the
o->src_indexarray whenget_progress()is fed thesrc_indexin the first place?Yes, before calling
check_updates(o, o->src_index),update_sparsity()loops overo->src_indexand callsapply_sparse_checkout()on each of the non-conflicted cache entries.apply_sparse_checkout()will set eitherCE_UPDATEorCE_WT_REMOVEwhenever items flip from or to having theSKIP_WORKTREEbit set.
Quoted 6 lines
Hmph.
I thought that the whole point of splitting
o->resultfromo->src_indexwe did long time ago was to allow us to treato->src_indexconstant. I hope we haven't broken anything by starting to do things like that X-<.
I think we're safe there. No function started modifying o->src_index directly; they just modify the index they are passed in. The only place that passes o->src_index to functions for modification is update_sparsity(), which unpack_trees() never calls. In fact, update_sparsity() was split out from unpack_trees(). But perhaps I can address your concerns a bit better with the history and from a high level instead of the low level details...
commit 34110cd4e394e3f92c01a4709689b384c34645d8
Author: Linus Torvalds <[email protected]> Date: Thu Mar 6 18:12:28 2008 -0800
Make 'unpack_trees()' have a separate source and destination index
...
This ends up removing a fair number more lines than it adds, for the simple reason that we can now skip all the crud that tried to be oh-so-careful about maintaining our position in the index as we were traversing and modifying it. Since we don't actually modify the source index any more, we can just update the 'o->pos' pointer without worrying about whether an index entry got removed or replaced or added to.
unpack_trees() has to do lots of work, including worrying about adding and removing many cache entries. Not having to worry about the position pointer as you're traversing the index makes sense. update_sparsity(), on the other hand, knows going in that no cache entries will be added or removed; it will at most toggle some flags on existing cache entries. The same concerns thus just aren't relevant. While we could make it use o->result instead of o->src_index, it would imply cloning all the index entries before toggling some flags on the existing entries, which seems like a waste of effort. (In fact, update_sparsity() never even initializes o->result, unlike unpack_trees(), so any attempts to access it are undefined behavior.)
I agree we don't want the two mixed up, but isn't that achieved by the fact that update_sparsity() was split out of unpack_trees() instead of making update_sparsity() some kind of extra mode flag passed to unpack_trees() (and which would have complicated unpack_trees() even further)?
One thing that could possibly be improved is using o->dst_index instead of o->src_index to make things slightly clearer, although it wouldn't actually change things because of this sanity check from the beginning of update_sparsity():
if (o->src_index != o->dst_index || o->fn)
BUG("update_sparsity() called wrong");Do any of those comments help? Are there still other changes you'd like to see (separate from this patch)?
Quoted 2 lines
Anyway, if that is the case, this change won't make things any worse. Let's queue it.
Thanks.
Quoted 21 lines · Elijah Newren
Quoted 13 lines · Junio C Hamano
Quoted 9 lines · Elijah Newren
Quoted 3 lines
Do we see these CE_UPDATE|CE_WT_REMOVE bits attached to the cache entries in the
o->src_indexarray whenget_progress()is fed thesrc_indexin the first place?Yes, before calling
check_updates(o, o->src_index),update_sparsity()loops overo->src_indexand callsapply_sparse_checkout()on each of the non-conflicted cache entries.apply_sparse_checkout()will set eitherCE_UPDATEorCE_WT_REMOVEwhenever items flip from or to having theSKIP_WORKTREEbit set.Quoted 6 lines
Hmph.
I thought that the whole point of splitting
o->resultfromo->src_indexwe did long time ago was to allow us to treato->src_indexconstant. I hope we haven't broken anything by starting to do things like that X-<.
Quoted 4 lines
I think we're safe there. No function started modifying
o->src_indexdirectly; they just modify the index they are passed in. The only place that passeso->src_indexto functions for modification isupdate_sparsity(), whichunpack_trees()never calls.
OK. Thanks for an additional detail. Let's merge it down by -rc1.