{"thread":{"id":"53471","subject":"[PATCH] unpack-trees: also allow get_progress() to work on a different index","startedAt":"2020-05-14T19:53:29Z","lastAt":"2020-05-15T15:01:17Z","messageCount":6,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"397820","messageId":"pull.785.git.git.1589486002453.gitgitgadget@gmail.com","threadId":"53471","inReplyTo":null,"subject":"[PATCH] unpack-trees: also allow get_progress() to work on a different index","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-14T19:53:22Z","receivedAt":"2020-05-14T19:53:29Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\ncommit b0a5a12a60 (\"unpack-trees: allow check_updates() to work on a\ndifferent index\", 2020-03-27) allowed check_updates() to work on a\ndifferent index, but it called get_progress() which was hardcoded to\nwork on o->result much like check_updates() had been.  Update it to also\naccept an index parameter and have check_updates() pass that parameter\nalong so that both are working on the same index.\n\nNoticed-by: Jeff Hostetler <jeffhost@microsoft.com>\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    unpack-trees: also allow get_progress() to work on a different index\n    \n    This is a fix to a minor 2.27 regression, from the en/sparse-checkout\n    series.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-785%2Fnewren%2Frecent-sparse-checkout-bugfix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-785/newren/recent-sparse-checkout-bugfix-v1\nPull-Request: https://github.com/git/git/pull/785\n\n unpack-trees.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 1fe3764f2b2..2e6e9d5eeb8 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -333,10 +333,10 @@ static void load_gitmodules_file(struct index_state *index,\n \t}\n }\n \n-static struct progress *get_progress(struct unpack_trees_options *o)\n+static struct progress *get_progress(struct unpack_trees_options *o,\n+\t\t\t\t     struct index_state *index)\n {\n \tunsigned cnt = 0, total = 0;\n-\tstruct index_state *index = &o->result;\n \n \tif (!o->update || !o->verbose_update)\n \t\treturn NULL;\n@@ -415,7 +415,7 @@ static int check_updates(struct unpack_trees_options *o,\n \tif (o->clone)\n \t\tsetup_collided_checkout_detection(&state, index);\n \n-\tprogress = get_progress(o);\n+\tprogress = get_progress(o, index);\n \n \tgit_attr_set_direction(GIT_ATTR_CHECKOUT);\n \n\nbase-commit: b994622632154fc3b17fb40a38819ad954a5fb88\n-- \ngitgitgadget\n"},{"id":"397821","messageId":"xmqq4ksiqnbf.fsf@gitster.c.googlers.com","threadId":"53471","inReplyTo":"pull.785.git.git.1589486002453.gitgitgadget@gmail.com","subject":"Re: [PATCH] unpack-trees: also allow get_progress() to work on a different index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-14T20:22:12Z","receivedAt":"2020-05-14T20:22:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Elijah Newren <newren@gmail.com>\n>\n> commit b0a5a12a60 (\"unpack-trees: allow check_updates() to work on a\n> different index\", 2020-03-27) allowed check_updates() to work on a\n> different index, but it called get_progress() which was hardcoded to\n> work on o->result much like check_updates() had been.  Update it to also\n> accept an index parameter and have check_updates() pass that parameter\n> along so that both are working on the same index.\n>\n> Noticed-by: Jeff Hostetler <jeffhost@microsoft.com>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>     unpack-trees: also allow get_progress() to work on a different index\n>     \n>     This is a fix to a minor 2.27 regression, from the en/sparse-checkout\n>     series.\n\nOK, so update_sparsity() calls the function with o->src_index,\nget_progress() without this fix would iterate over an unrelated\no->result to count the cache entries that are marked for update or\nremoval and show the progress based on that number without this\npatch?  The other caller in unpack_trees() uses o->result so there\nis no change in the behaviour with or without the patch.\n\nThe flag bits on cache entries used to count the entries in the\nindex by the get_progress() function are applied only on the\no->result side when merged_entry() and friends call do_add_entry(),\nno?  \n\nDo we see these CE_UPDATE|CE_WT_REMOVE bits attached to the cache\nentries in the o->src_index array when get_progress() is fed the\nsrc_index in the first place?\n\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-785%2Fnewren%2Frecent-sparse-checkout-bugfix-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-785/newren/recent-sparse-checkout-bugfix-v1\n> Pull-Request: https://github.com/git/git/pull/785\n>\n>  unpack-trees.c | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 1fe3764f2b2..2e6e9d5eeb8 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -333,10 +333,10 @@ static void load_gitmodules_file(struct index_state *index,\n>  \t}\n>  }\n>  \n> -static struct progress *get_progress(struct unpack_trees_options *o)\n> +static struct progress *get_progress(struct unpack_trees_options *o,\n> +\t\t\t\t     struct index_state *index)\n>  {\n>  \tunsigned cnt = 0, total = 0;\n> -\tstruct index_state *index = &o->result;\n>  \n>  \tif (!o->update || !o->verbose_update)\n>  \t\treturn NULL;\n> @@ -415,7 +415,7 @@ static int check_updates(struct unpack_trees_options *o,\n>  \tif (o->clone)\n>  \t\tsetup_collided_checkout_detection(&state, index);\n>  \n> -\tprogress = get_progress(o);\n> +\tprogress = get_progress(o, index);\n>  \n>  \tgit_attr_set_direction(GIT_ATTR_CHECKOUT);\n>  \n>\n> base-commit: b994622632154fc3b17fb40a38819ad954a5fb88\n"},{"id":"397834","messageId":"CABPp-BHM8uS1yxAtjE=Frh_V7JF7-EJ8+47e-cr9TpEMZr4nfw@mail.gmail.com","threadId":"53471","inReplyTo":"xmqq4ksiqnbf.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] unpack-trees: also allow get_progress() to work on a different index","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-05-14T21:46:07Z","receivedAt":"2020-05-14T21:46:20Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, May 14, 2020 at 1:22 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > From: Elijah Newren <newren@gmail.com>\n> >\n> > commit b0a5a12a60 (\"unpack-trees: allow check_updates() to work on a\n> > different index\", 2020-03-27) allowed check_updates() to work on a\n> > different index, but it called get_progress() which was hardcoded to\n> > work on o->result much like check_updates() had been.  Update it to also\n> > accept an index parameter and have check_updates() pass that parameter\n> > along so that both are working on the same index.\n> >\n> > Noticed-by: Jeff Hostetler <jeffhost@microsoft.com>\n> > Signed-off-by: Elijah Newren <newren@gmail.com>\n> > ---\n> >     unpack-trees: also allow get_progress() to work on a different index\n> >\n> >     This is a fix to a minor 2.27 regression, from the en/sparse-checkout\n> >     series.\n>\n> OK, so update_sparsity() calls the function with o->src_index,\n> get_progress() without this fix would iterate over an unrelated\n> o->result to count the cache entries that are marked for update or\n> removal and show the progress based on that number without this\n> patch?  The other caller in unpack_trees() uses o->result so there\n> is no change in the behaviour with or without the patch.\n\nYes.\n\n> The flag bits on cache entries used to count the entries in the\n> index by the get_progress() function are applied only on the\n> o->result side when merged_entry() and friends call do_add_entry(),\n> no?\n\nNot quite; unpack_trees() also calls apply_sparse_checkout(), passing\nit o->result, and apply_sparse_checkout() also sets those bits.\n\n> Do we see these CE_UPDATE|CE_WT_REMOVE bits attached to the cache\n> entries in the o->src_index array when get_progress() is fed the\n> src_index in the first place?\n\nYes, before calling check_updates(o, o->src_index), update_sparsity()\nloops over o->src_index and calls apply_sparse_checkout() on each of\nthe non-conflicted cache entries.  apply_sparse_checkout() will set\neither CE_UPDATE or CE_WT_REMOVE whenever items flip from or to having\nthe SKIP_WORKTREE bit set.\n"},{"id":"397839","messageId":"xmqqimgyp3ft.fsf@gitster.c.googlers.com","threadId":"53471","inReplyTo":"CABPp-BHM8uS1yxAtjE=Frh_V7JF7-EJ8+47e-cr9TpEMZr4nfw@mail.gmail.com","subject":"Re: [PATCH] unpack-trees: also allow get_progress() to work on a different index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-14T22:16:54Z","receivedAt":"2020-05-14T22:17:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n>> Do we see these CE_UPDATE|CE_WT_REMOVE bits attached to the cache\n>> entries in the o->src_index array when get_progress() is fed the\n>> src_index in the first place?\n>\n> Yes, before calling check_updates(o, o->src_index), update_sparsity()\n> loops over o->src_index and calls apply_sparse_checkout() on each of\n> the non-conflicted cache entries.  apply_sparse_checkout() will set\n> either CE_UPDATE or CE_WT_REMOVE whenever items flip from or to having\n> the SKIP_WORKTREE bit set.\n\nHmph.\n\nI thought that the whole point of splitting o->result from\no->src_index we did long time ago was to allow us to treat\no->src_index constant.  I hope we haven't broken anything by\nstarting to do things like that X-<.\n\nAnyway, if that is the case, this change won't make things any\nworse.  Let's queue it.\n\nThanks.\n\n"},{"id":"397845","messageId":"CABPp-BFAR=+CxwcZz1v4n3Z53uEas5P6Y7pUT7u9dAL0hULe3w@mail.gmail.com","threadId":"53471","inReplyTo":"xmqqimgyp3ft.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] unpack-trees: also allow get_progress() to work on a different index","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-05-14T23:21:14Z","receivedAt":"2020-05-14T23:21:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, May 14, 2020 at 3:17 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Elijah Newren <newren@gmail.com> writes:\n>\n> >> Do we see these CE_UPDATE|CE_WT_REMOVE bits attached to the cache\n> >> entries in the o->src_index array when get_progress() is fed the\n> >> src_index in the first place?\n> >\n> > Yes, before calling check_updates(o, o->src_index), update_sparsity()\n> > loops over o->src_index and calls apply_sparse_checkout() on each of\n> > the non-conflicted cache entries.  apply_sparse_checkout() will set\n> > either CE_UPDATE or CE_WT_REMOVE whenever items flip from or to having\n> > the SKIP_WORKTREE bit set.\n>\n> Hmph.\n>\n> I thought that the whole point of splitting o->result from\n> o->src_index we did long time ago was to allow us to treat\n> o->src_index constant.  I hope we haven't broken anything by\n> starting to do things like that X-<.\n\nI think we're safe there.  No function started modifying o->src_index\ndirectly; they just modify the index they are passed in.  The only\nplace that passes o->src_index to functions for modification is\nupdate_sparsity(), which unpack_trees() never calls.  In fact,\nupdate_sparsity() was split out from unpack_trees().  But perhaps I\ncan address your concerns a bit better with the history and from a\nhigh level instead of the low level details...\n\ncommit 34110cd4e394e3f92c01a4709689b384c34645d8\nAuthor: Linus Torvalds <torvalds@linux-foundation.org>\nDate:   Thu Mar 6 18:12:28 2008 -0800\n\n    Make 'unpack_trees()' have a separate source and destination index\n...\n    This ends up removing a fair number more lines than it adds, for the\n    simple reason that we can now skip all the crud that tried to be\n    oh-so-careful about maintaining our position in the index as we were\n    traversing and modifying it.  Since we don't actually modify the source\n    index any more, we can just update the 'o->pos' pointer without worrying\n    about whether an index entry got removed or replaced or added to.\n\n\nunpack_trees() has to do lots of work, including worrying about adding\nand removing many cache entries.  Not having to worry about the\nposition pointer as you're traversing the index makes sense.\nupdate_sparsity(), on the other hand, knows going in that no cache\nentries will be added or removed; it will at most toggle some flags on\nexisting cache entries.  The same concerns thus just aren't relevant.\nWhile we could make it use o->result instead of o->src_index, it would\nimply cloning all the index entries before toggling some flags on the\nexisting entries, which seems like a waste of effort.  (In fact,\nupdate_sparsity() never even initializes o->result, unlike\nunpack_trees(), so any attempts to access it are undefined behavior.)\n\nI agree we don't want the two mixed up, but isn't that achieved by the\nfact that update_sparsity() was split out of unpack_trees() instead of\nmaking update_sparsity() some kind of extra mode flag passed to\nunpack_trees() (and which would have complicated unpack_trees() even\nfurther)?\n\nOne thing that could possibly be improved is using o->dst_index\ninstead of o->src_index to make things slightly clearer, although it\nwouldn't actually change things because of this sanity check from the\nbeginning of update_sparsity():\n\nif (o->src_index != o->dst_index || o->fn)\n        BUG(\"update_sparsity() called wrong\");\n\n\nDo any of those comments help?  Are there still other changes you'd\nlike to see (separate from this patch)?\n\n> Anyway, if that is the case, this change won't make things any\n> worse.  Let's queue it.\n\nThanks.\n"},{"id":"397881","messageId":"xmqq8shtnsy1.fsf@gitster.c.googlers.com","threadId":"53471","inReplyTo":"CABPp-BFAR=+CxwcZz1v4n3Z53uEas5P6Y7pUT7u9dAL0hULe3w@mail.gmail.com","subject":"Re: [PATCH] unpack-trees: also allow get_progress() to work on a different index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-15T15:01:10Z","receivedAt":"2020-05-15T15:01:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> On Thu, May 14, 2020 at 3:17 PM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Elijah Newren <newren@gmail.com> writes:\n>>\n>> >> Do we see these CE_UPDATE|CE_WT_REMOVE bits attached to the cache\n>> >> entries in the o->src_index array when get_progress() is fed the\n>> >> src_index in the first place?\n>> >\n>> > Yes, before calling check_updates(o, o->src_index), update_sparsity()\n>> > loops over o->src_index and calls apply_sparse_checkout() on each of\n>> > the non-conflicted cache entries.  apply_sparse_checkout() will set\n>> > either CE_UPDATE or CE_WT_REMOVE whenever items flip from or to having\n>> > the SKIP_WORKTREE bit set.\n>>\n>> Hmph.\n>>\n>> I thought that the whole point of splitting o->result from\n>> o->src_index we did long time ago was to allow us to treat\n>> o->src_index constant.  I hope we haven't broken anything by\n>> starting to do things like that X-<.\n>\n> I think we're safe there.  No function started modifying o->src_index\n> directly; they just modify the index they are passed in.  The only\n> place that passes o->src_index to functions for modification is\n> update_sparsity(), which unpack_trees() never calls.\n\nOK.  Thanks for an additional detail.  Let's merge it down by -rc1.\n\n"}]}