{"thread":{"id":"58148","subject":"[PATCH 0/3] checkout: fix two bugs on count of updated entries","startedAt":"2022-07-13T04:20:23Z","lastAt":"2022-07-14T11:49:42Z","messageCount":11,"participants":["Matheus Tavares","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"458934","messageId":"cover.1657685948.git.matheus.bernardino@usp.br","threadId":"58148","inReplyTo":null,"subject":"[PATCH 0/3] checkout: fix two bugs on count of updated entries","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2022-07-13T04:19:54Z","receivedAt":"2022-07-13T04:20:23Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"This fixes two issues at the \"Updated %d path from the index\" report at\nthe end of a `git checkout <paths>` operation:\n\n  - Delayed checkout entries being counted twice.\n  - Failed entries being included in the count.\n\nThe first two patches add tests and the third implements the fix. I came\nacross this while working at parallel checkout, but only managed to get\nback to it now.\n\nMatheus Tavares (3):\n  checkout: document bug where delayed checkout counts entries twice\n  checkout: show bug about failed entries being included in final report\n  checkout: fix two bugs on the final count of updated entries\n\n builtin/checkout.c                  |  2 +-\n convert.h                           |  6 +++-\n entry.c                             | 34 ++++++++++++--------\n entry.h                             |  3 +-\n parallel-checkout.c                 | 10 ++++--\n parallel-checkout.h                 |  4 ++-\n t/lib-parallel-checkout.sh          |  6 +++-\n t/t0021-conversion.sh               | 22 +++++++++++++\n t/t2080-parallel-checkout-basics.sh | 50 +++++++++++++++++++++++++++++\n unpack-trees.c                      |  2 +-\n 10 files changed, 115 insertions(+), 24 deletions(-)\n\n-- \n2.37.0\n\n"},{"id":"458935","messageId":"694aeb19f57297d9b9d07d47897385bdbedd309c.1657685948.git.matheus.bernardino@usp.br","threadId":"58148","inReplyTo":"cover.1657685948.git.matheus.bernardino@usp.br","subject":"[PATCH 1/3] checkout: document bug where delayed checkout counts entries twice","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2022-07-13T04:19:55Z","receivedAt":"2022-07-13T04:20:25Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"At the end of a `git checkout <pathspec>` operation, git reports how\nmany paths were checked out with a message like \"Updated N paths from\nthe index\". However, entries that end up on the delayed checkout queue\n(as requested by a long-running process filter) get counted twice,\nproducing a wrong number in the final report. We will fix this bug in an\nupcoming commit. For now, only document/demonstrate it with a\ntest_expect_failure.\n\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n t/t0021-conversion.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex bad37abad2..00df9b5c18 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -1132,4 +1132,26 @@ do\n \t'\n done\n \n+test_expect_failure PERL 'delayed checkout correctly reports the number of updated entries' '\n+\trm -rf repo &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config filter.delay.process \"../rot13-filter.pl delayed.log clean smudge delay\" &&\n+\t\tgit config filter.delay.required true &&\n+\n+\t\techo \"*.a filter=delay\" >.gitattributes &&\n+\t\techo a >test-delay10.a &&\n+\t\techo a >test-delay11.a &&\n+\t\tgit add . &&\n+\t\tgit commit -m files &&\n+\n+\t\trm *.a &&\n+\t\tgit checkout . 2>err &&\n+\t\tgrep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delayed.log &&\n+\t\tgrep \"IN: smudge test-delay11.a .* \\\\[DELAYED\\\\]\" delayed.log &&\n+\t\tgrep \"Updated 2 paths from the index\" err\n+\t)\n+'\n+\n test_done\n-- \n2.37.0\n\n"},{"id":"458936","messageId":"8da18a0a8c34a1c10d55bcdda725817db586f763.1657685948.git.matheus.bernardino@usp.br","threadId":"58148","inReplyTo":"cover.1657685948.git.matheus.bernardino@usp.br","subject":"[PATCH 2/3] checkout: show bug about failed entries being included in final report","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2022-07-13T04:19:56Z","receivedAt":"2022-07-13T04:20:26Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"After checkout, git usually reports how many entries were updated at\nthat operation. However, because we count the entries too soon during\nthe checkout process, we may actually include entries that do not get\nproperly checked out in the end. This can lead to an inaccurate final\nreport if the user expects it to show only the *successful* updates.\nThis will be fixed in the next commit, but for now let's document it\nwith a test that cover all checkout modes.\n\nNote that `test_checkout_workers` have to be slightly adjusted in order\nto use the construct `test_checkout_workers ...  test_must_fail git\ncheckout`. The function runs the command given to it with an assignment\nprefix to set the GIT_TRACE2 variable. However, this this assignment has\nan undefined behavior when the command is a shell function (like\n`test_must_fail`). As POSIX specifies:\n\n  If the command name is a function that is not a standard utility\n  implemented as a function, variable assignments shall affect the\n  current execution environment during the execution of the function. It\n  is unspecified:\n\n    - Whether or not the variable assignments persist after the\n      completion of the function\n\n    - Whether or not the variables gain the export attribute during the\n      execution of the function\n\nThus, in order to make sure the GIT_TRACE2 value gets visible to the git\ncommand executed by `test_must_fail`, export the variable and run git in\na subshell.\n\n[1]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html\n     (Section 2.9.1: Simple Commands)\n\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n t/lib-parallel-checkout.sh          |  6 +++-\n t/t2080-parallel-checkout-basics.sh | 50 +++++++++++++++++++++++++++++\n 2 files changed, 55 insertions(+), 1 deletion(-)\n\ndiff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\nindex 83b279a846..acaee9cbb6 100644\n--- a/t/lib-parallel-checkout.sh\n+++ b/t/lib-parallel-checkout.sh\n@@ -25,7 +25,11 @@ test_checkout_workers () {\n \n \tlocal trace_file=trace-test-checkout-workers &&\n \trm -f \"$trace_file\" &&\n-\tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n+\t(\n+\t\tGIT_TRACE2=\"$(pwd)/$trace_file\" &&\n+\t\texport GIT_TRACE2 &&\n+\t\t\"$@\" 2>&8\n+\t) &&\n \n \tlocal workers=\"$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l)\" &&\n \ttest $workers -eq $expected_workers &&\ndiff --git a/t/t2080-parallel-checkout-basics.sh b/t/t2080-parallel-checkout-basics.sh\nindex 3e0f8c675f..6fd7e4c4b2 100755\n--- a/t/t2080-parallel-checkout-basics.sh\n+++ b/t/t2080-parallel-checkout-basics.sh\n@@ -226,4 +226,54 @@ test_expect_success SYMLINKS 'parallel checkout checks for symlinks in leading d\n \t)\n '\n \n+# This test is here (and not in e.g. t2022-checkout-paths.sh), because we\n+# check the final report including sequential, parallel, and delayed entries\n+# all at the same time. So we must have finer control of the parallel checkout\n+# variables.\n+test_expect_failure PERL '\"git checkout .\" report should not include failed entries' '\n+\twrite_script rot13-filter.pl \"$PERL_PATH\" \\\n+\t\t<\"$TEST_DIRECTORY\"/t0021/rot13-filter.pl &&\n+\n+\ttest_config_global filter.delay.process \\\n+\t\t\"\\\"$(pwd)/rot13-filter.pl\\\" --always-delay delayed.log clean smudge delay\" &&\n+\ttest_config_global filter.delay.required true &&\n+\ttest_config_global filter.cat.clean cat  &&\n+\ttest_config_global filter.cat.smudge cat  &&\n+\ttest_config_global filter.cat.required true  &&\n+\n+\tset_checkout_config 2 0 &&\n+\tgit init failed_entries &&\n+\t(\n+\t\tcd failed_entries &&\n+\t\tcat >.gitattributes <<-EOF &&\n+\t\t*delay*              filter=delay\n+\t\tparallel-ineligible* filter=cat\n+\t\tEOF\n+\t\techo a >missing-delay.a &&\n+\t\techo a >parallel-ineligible.a &&\n+\t\techo a >parallel-eligible.a &&\n+\t\techo b >success-delay.b &&\n+\t\techo b >parallel-ineligible.b &&\n+\t\techo b >parallel-eligible.b &&\n+\t\tgit add -A &&\n+\t\tgit commit -m files &&\n+\n+\t\ta_blob=\"$(git rev-parse :parallel-ineligible.a)\" &&\n+\t\tdir=\"$(echo \"$a_blob\" | cut -c 1-2)\" &&\n+\t\tfile=\"$(echo \"$a_blob\" | cut -c 3-)\" &&\n+\t\trm \".git/objects/$dir/$file\" &&\n+\t\trm *.a *.b &&\n+\n+\t\ttest_checkout_workers 2 test_must_fail git checkout . 2>err &&\n+\n+\t\t# All *.b entries should succeed and all *.a entries should fail:\n+\t\t#  - missing-delay.a: the delay filter will drop this path\n+\t\t#  - parallel-*.a: the blob will be missing\n+\t\t#\n+\t\tgrep \"Updated 3 paths from the index\" err &&\n+\t\ttest \"$(ls *.b | wc -l)\" -eq 3 &&\n+\t\ttest \"$(ls *.a | wc -l)\" -eq 0\n+\t)\n+'\n+\n test_done\n-- \n2.37.0\n\n"},{"id":"458937","messageId":"5e9452be66d75e94ca595228e568dce133e388ab.1657685948.git.matheus.bernardino@usp.br","threadId":"58148","inReplyTo":"cover.1657685948.git.matheus.bernardino@usp.br","subject":"[PATCH 3/3] checkout: fix two bugs on the final count of updated entries","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2022-07-13T04:19:57Z","receivedAt":"2022-07-13T04:20:28Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"At the end of `git checkout <pathspec>`, we get a message informing how\nmany entries were updated in the working tree. However, this number can\nbe inaccurate for two reasons:\n\n1) Delayed entries currently get counted twice.\n2) Failed entries are included in the count.\n\nThe first problem happens because the counter is first incremented\nbefore inserting the entry in the delayed checkout queue, and once again\nwhen finish_delayed_checkout() calls checkout_entry(). And the second\nhappens because the counter is incremented too early in\ncheckout_entry(), before the entry was in fact checked out. Fix that by\nmoving the count increment further down in the call stack and removing\nthe duplicate increment on delayed entries. Note that we have to keep\na per-entry reference for the counter (both on parallel checkout and\ndelayed checkout) because not all entries are always accumulated at the\nsame counter. See checkout_worktree(), at builtin/checkout.c for an\nexample.\n\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n builtin/checkout.c                  |  2 +-\n convert.h                           |  6 ++++-\n entry.c                             | 34 +++++++++++++++++------------\n entry.h                             |  3 +--\n parallel-checkout.c                 | 10 ++++++---\n parallel-checkout.h                 |  4 +++-\n t/t0021-conversion.sh               |  2 +-\n t/t2080-parallel-checkout-basics.sh |  2 +-\n unpack-trees.c                      |  2 +-\n 9 files changed, 40 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2eefda81d8..df3f1663d7 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -417,7 +417,7 @@ static int checkout_worktree(const struct checkout_opts *opts,\n \tmem_pool_discard(&ce_mem_pool, should_validate_cache_entries());\n \tremove_marked_cache_entries(&the_index, 1);\n \tremove_scheduled_dirs();\n-\terrs |= finish_delayed_checkout(&state, &nr_checkouts, opts->show_progress);\n+\terrs |= finish_delayed_checkout(&state, opts->show_progress);\n \n \tif (opts->count_checkout_paths) {\n \t\tif (nr_unmerged)\ndiff --git a/convert.h b/convert.h\nindex 5ee1c32205..0a6e4086b8 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -53,7 +53,11 @@ struct delayed_checkout {\n \tenum ce_delay_state state;\n \t/* List of filter drivers that signaled delayed blobs. */\n \tstruct string_list filters;\n-\t/* List of delayed blobs identified by their path. */\n+\t/*\n+\t * List of delayed blobs identified by their path. The `util` member\n+\t * holds a counter pointer which must be incremented when/if the\n+\t * associated blob gets checked out.\n+\t */\n \tstruct string_list paths;\n };\n \ndiff --git a/entry.c b/entry.c\nindex 1c9df62b30..616e4f073c 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -157,12 +157,11 @@ static int remove_available_paths(struct string_list_item *item, void *cb_data)\n \n \tavailable = string_list_lookup(available_paths, item->string);\n \tif (available)\n-\t\tavailable->util = (void *)item->string;\n+\t\tavailable->util = item->util;\n \treturn !available;\n }\n \n-int finish_delayed_checkout(struct checkout *state, int *nr_checkouts,\n-\t\t\t    int show_progress)\n+int finish_delayed_checkout(struct checkout *state, int show_progress)\n {\n \tint errs = 0;\n \tunsigned processed_paths = 0;\n@@ -227,7 +226,7 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts,\n \t\t\t\t\t\t       strlen(path->string), 0);\n \t\t\t\tif (ce) {\n \t\t\t\t\tdisplay_progress(progress, ++processed_paths);\n-\t\t\t\t\terrs |= checkout_entry(ce, state, NULL, nr_checkouts);\n+\t\t\t\t\terrs |= checkout_entry(ce, state, NULL, path->util);\n \t\t\t\t\tfiltered_bytes += ce->ce_stat_data.sd_size;\n \t\t\t\t\tdisplay_throughput(progress, filtered_bytes);\n \t\t\t\t} else\n@@ -266,7 +265,8 @@ void update_ce_after_write(const struct checkout *state, struct cache_entry *ce,\n \n /* Note: ca is used (and required) iff the entry refers to a regular file. */\n static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca,\n-\t\t       const struct checkout *state, int to_tempfile)\n+\t\t       const struct checkout *state, int to_tempfile,\n+\t\t       int *nr_checkouts)\n {\n \tunsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;\n \tstruct delayed_checkout *dco = state->delayed_checkout;\n@@ -279,6 +279,7 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca\n \tstruct stat st;\n \tconst struct submodule *sub;\n \tstruct checkout_metadata meta;\n+\tstatic int scratch_nr_checkouts;\n \n \tclone_checkout_metadata(&meta, &state->meta, &ce->oid);\n \n@@ -333,9 +334,15 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca\n \t\t\tret = async_convert_to_working_tree_ca(ca, ce->name,\n \t\t\t\t\t\t\t       new_blob, size,\n \t\t\t\t\t\t\t       &buf, &meta, dco);\n-\t\t\tif (ret && string_list_has_string(&dco->paths, ce->name)) {\n-\t\t\t\tfree(new_blob);\n-\t\t\t\tgoto delayed;\n+\t\t\tif (ret) {\n+\t\t\t\tstruct string_list_item *item =\n+\t\t\t\t\tstring_list_lookup(&dco->paths, ce->name);\n+\t\t\t\tif (item) {\n+\t\t\t\t\titem->util = nr_checkouts ? nr_checkouts\n+\t\t\t\t\t\t\t: &scratch_nr_checkouts;\n+\t\t\t\t\tfree(new_blob);\n+\t\t\t\t\tgoto delayed;\n+\t\t\t\t}\n \t\t\t}\n \t\t} else {\n \t\t\tret = convert_to_working_tree_ca(ca, ce->name, new_blob,\n@@ -392,6 +399,8 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca\n \t\t\t\t\t   ce->name);\n \t\tupdate_ce_after_write(state, ce , &st);\n \t}\n+\tif (nr_checkouts)\n+\t\t(*nr_checkouts)++;\n delayed:\n \treturn 0;\n }\n@@ -476,7 +485,7 @@ int checkout_entry_ca(struct cache_entry *ce, struct conv_attrs *ca,\n \t\t\tconvert_attrs(state->istate, &ca_buf, ce->name);\n \t\t\tca = &ca_buf;\n \t\t}\n-\t\treturn write_entry(ce, topath, ca, state, 1);\n+\t\treturn write_entry(ce, topath, ca, state, 1, nr_checkouts);\n \t}\n \n \tstrbuf_reset(&path);\n@@ -540,18 +549,15 @@ int checkout_entry_ca(struct cache_entry *ce, struct conv_attrs *ca,\n \n \tcreate_directories(path.buf, path.len, state);\n \n-\tif (nr_checkouts)\n-\t\t(*nr_checkouts)++;\n-\n \tif (S_ISREG(ce->ce_mode) && !ca) {\n \t\tconvert_attrs(state->istate, &ca_buf, ce->name);\n \t\tca = &ca_buf;\n \t}\n \n-\tif (!enqueue_checkout(ce, ca))\n+\tif (!enqueue_checkout(ce, ca, nr_checkouts))\n \t\treturn 0;\n \n-\treturn write_entry(ce, path.buf, ca, state, 0);\n+\treturn write_entry(ce, path.buf, ca, state, 0, nr_checkouts);\n }\n \n void unlink_entry(const struct cache_entry *ce)\ndiff --git a/entry.h b/entry.h\nindex 252fd24c2e..9be4659881 100644\n--- a/entry.h\n+++ b/entry.h\n@@ -43,8 +43,7 @@ static inline int checkout_entry(struct cache_entry *ce,\n }\n \n void enable_delayed_checkout(struct checkout *state);\n-int finish_delayed_checkout(struct checkout *state, int *nr_checkouts,\n-\t\t\t    int show_progress);\n+int finish_delayed_checkout(struct checkout *state, int show_progress);\n \n /*\n  * Unlink the last component and schedule the leading directories for\ndiff --git a/parallel-checkout.c b/parallel-checkout.c\nindex 31a3d0ee1b..4f6819f240 100644\n--- a/parallel-checkout.c\n+++ b/parallel-checkout.c\n@@ -143,7 +143,8 @@ static int is_eligible_for_parallel_checkout(const struct cache_entry *ce,\n \t}\n }\n \n-int enqueue_checkout(struct cache_entry *ce, struct conv_attrs *ca)\n+int enqueue_checkout(struct cache_entry *ce, struct conv_attrs *ca,\n+\t\t     int *checkout_counter)\n {\n \tstruct parallel_checkout_item *pc_item;\n \n@@ -159,6 +160,7 @@ int enqueue_checkout(struct cache_entry *ce, struct conv_attrs *ca)\n \tmemcpy(&pc_item->ca, ca, sizeof(pc_item->ca));\n \tpc_item->status = PC_ITEM_PENDING;\n \tpc_item->id = parallel_checkout.nr;\n+\tpc_item->checkout_counter = checkout_counter;\n \tparallel_checkout.nr++;\n \n \treturn 0;\n@@ -200,7 +202,8 @@ static int handle_results(struct checkout *state)\n \n \t\tswitch(pc_item->status) {\n \t\tcase PC_ITEM_WRITTEN:\n-\t\t\t/* Already handled */\n+\t\t\tif (pc_item->checkout_counter)\n+\t\t\t\t(*pc_item->checkout_counter)++;\n \t\t\tbreak;\n \t\tcase PC_ITEM_COLLIDED:\n \t\t\t/*\n@@ -225,7 +228,8 @@ static int handle_results(struct checkout *state)\n \t\t\t * add any extra overhead.\n \t\t\t */\n \t\t\tret |= checkout_entry_ca(pc_item->ce, &pc_item->ca,\n-\t\t\t\t\t\t state, NULL, NULL);\n+\t\t\t\t\t\t state, NULL,\n+\t\t\t\t\t\t pc_item->checkout_counter);\n \t\t\tadvance_progress_meter();\n \t\t\tbreak;\n \t\tcase PC_ITEM_PENDING:\ndiff --git a/parallel-checkout.h b/parallel-checkout.h\nindex 80f539bcb7..c575284005 100644\n--- a/parallel-checkout.h\n+++ b/parallel-checkout.h\n@@ -31,7 +31,8 @@ void init_parallel_checkout(void);\n  * entry is not eligible for parallel checkout. Otherwise, enqueue the entry\n  * for later write and return 0.\n  */\n-int enqueue_checkout(struct cache_entry *ce, struct conv_attrs *ca);\n+int enqueue_checkout(struct cache_entry *ce, struct conv_attrs *ca,\n+\t\t     int *checkout_counter);\n size_t pc_queue_size(void);\n \n /*\n@@ -68,6 +69,7 @@ struct parallel_checkout_item {\n \tstruct cache_entry *ce;\n \tstruct conv_attrs ca;\n \tsize_t id; /* position in parallel_checkout.items[] of main process */\n+\tint *checkout_counter;\n \n \t/* Output fields, sent from workers. */\n \tenum pc_item_status status;\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 00df9b5c18..1c840348bd 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -1132,7 +1132,7 @@ do\n \t'\n done\n \n-test_expect_failure PERL 'delayed checkout correctly reports the number of updated entries' '\n+test_expect_success PERL 'delayed checkout correctly reports the number of updated entries' '\n \trm -rf repo &&\n \tgit init repo &&\n \t(\ndiff --git a/t/t2080-parallel-checkout-basics.sh b/t/t2080-parallel-checkout-basics.sh\nindex 6fd7e4c4b2..950361d767 100755\n--- a/t/t2080-parallel-checkout-basics.sh\n+++ b/t/t2080-parallel-checkout-basics.sh\n@@ -230,7 +230,7 @@ test_expect_success SYMLINKS 'parallel checkout checks for symlinks in leading d\n # check the final report including sequential, parallel, and delayed entries\n # all at the same time. So we must have finer control of the parallel checkout\n # variables.\n-test_expect_failure PERL '\"git checkout .\" report should not include failed entries' '\n+test_expect_success PERL '\"git checkout .\" report should not include failed entries' '\n \twrite_script rot13-filter.pl \"$PERL_PATH\" \\\n \t\t<\"$TEST_DIRECTORY\"/t0021/rot13-filter.pl &&\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex d561ca01ed..8a454e03bf 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -487,7 +487,7 @@ static int check_updates(struct unpack_trees_options *o,\n \t\terrs |= run_parallel_checkout(&state, pc_workers, pc_threshold,\n \t\t\t\t\t      progress, &cnt);\n \tstop_progress(&progress);\n-\terrs |= finish_delayed_checkout(&state, NULL, o->verbose_update);\n+\terrs |= finish_delayed_checkout(&state, o->verbose_update);\n \tgit_attr_set_direction(GIT_ATTR_CHECKIN);\n \n \tif (o->clone)\n-- \n2.37.0\n\n"},{"id":"458947","messageId":"220713.86a69d461f.gmgdl@evledraar.gmail.com","threadId":"58148","inReplyTo":"8da18a0a8c34a1c10d55bcdda725817db586f763.1657685948.git.matheus.bernardino@usp.br","subject":"Re: [PATCH 2/3] checkout: show bug about failed entries being included in final report","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-13T11:14:55Z","receivedAt":"2022-07-13T11:16:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jul 13 2022, Matheus Tavares wrote:\n\n> After checkout, git usually reports how many entries were updated at\n> that operation. However, because we count the entries too soon during\n> the checkout process, we may actually include entries that do not get\n> properly checked out in the end. This can lead to an inaccurate final\n> report if the user expects it to show only the *successful* updates.\n> This will be fixed in the next commit, but for now let's document it\n> with a test that cover all checkout modes.\n>\n> Note that `test_checkout_workers` have to be slightly adjusted in order\n> to use the construct `test_checkout_workers ...  test_must_fail git\n> checkout`. The function runs the command given to it with an assignment\n> prefix to set the GIT_TRACE2 variable. However, this this assignment has\n> an undefined behavior when the command is a shell function (like\n> `test_must_fail`). As POSIX specifies:\n>\n>   If the command name is a function that is not a standard utility\n>   implemented as a function, variable assignments shall affect the\n>   current execution environment during the execution of the function. It\n>   is unspecified:\n>\n>     - Whether or not the variable assignments persist after the\n>       completion of the function\n>\n>     - Whether or not the variables gain the export attribute during the\n>       execution of the function\n>\n> Thus, in order to make sure the GIT_TRACE2 value gets visible to the git\n> command executed by `test_must_fail`, export the variable and run git in\n> a subshell.\n>\n> [1]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html\n>      (Section 2.9.1: Simple Commands)\n>\n> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n> ---\n>  t/lib-parallel-checkout.sh          |  6 +++-\n>  t/t2080-parallel-checkout-basics.sh | 50 +++++++++++++++++++++++++++++\n>  2 files changed, 55 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\n> index 83b279a846..acaee9cbb6 100644\n> --- a/t/lib-parallel-checkout.sh\n> +++ b/t/lib-parallel-checkout.sh\n> @@ -25,7 +25,11 @@ test_checkout_workers () {\n>  \n>  \tlocal trace_file=trace-test-checkout-workers &&\n>  \trm -f \"$trace_file\" &&\n> -\tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n> +\t(\n> +\t\tGIT_TRACE2=\"$(pwd)/$trace_file\" &&\n> +\t\texport GIT_TRACE2 &&\n> +\t\t\"$@\" 2>&8\n> +\t) &&\n>  \n>  \tlocal workers=\"$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l)\" &&\n>  \ttest $workers -eq $expected_workers &&\n> diff --git a/t/t2080-parallel-checkout-basics.sh b/t/t2080-parallel-checkout-basics.sh\n> index 3e0f8c675f..6fd7e4c4b2 100755\n> --- a/t/t2080-parallel-checkout-basics.sh\n> +++ b/t/t2080-parallel-checkout-basics.sh\n> @@ -226,4 +226,54 @@ test_expect_success SYMLINKS 'parallel checkout checks for symlinks in leading d\n>  \t)\n>  '\n>  \n> +# This test is here (and not in e.g. t2022-checkout-paths.sh), because we\n> +# check the final report including sequential, parallel, and delayed entries\n> +# all at the same time. So we must have finer control of the parallel checkout\n> +# variables.\n> +test_expect_failure PERL '\"git checkout .\" report should not include failed entries' '\n> +\twrite_script rot13-filter.pl \"$PERL_PATH\" \\\n> +\t\t<\"$TEST_DIRECTORY\"/t0021/rot13-filter.pl &&\n> +\n> +\ttest_config_global filter.delay.process \\\n> +\t\t\"\\\"$(pwd)/rot13-filter.pl\\\" --always-delay delayed.log clean smudge delay\" &&\n> +\ttest_config_global filter.delay.required true &&\n> +\ttest_config_global filter.cat.clean cat  &&\n> +\ttest_config_global filter.cat.smudge cat  &&\n> +\ttest_config_global filter.cat.required true  &&\n> +\n> +\tset_checkout_config 2 0 &&\n> +\tgit init failed_entries &&\n> +\t(\n> +\t\tcd failed_entries &&\n> +\t\tcat >.gitattributes <<-EOF &&\n> +\t\t*delay*              filter=delay\n> +\t\tparallel-ineligible* filter=cat\n> +\t\tEOF\n> +\t\techo a >missing-delay.a &&\n> +\t\techo a >parallel-ineligible.a &&\n> +\t\techo a >parallel-eligible.a &&\n> +\t\techo b >success-delay.b &&\n> +\t\techo b >parallel-ineligible.b &&\n> +\t\techo b >parallel-eligible.b &&\n> +\t\tgit add -A &&\n> +\t\tgit commit -m files &&\n> +\n> +\t\ta_blob=\"$(git rev-parse :parallel-ineligible.a)\" &&\n> +\t\tdir=\"$(echo \"$a_blob\" | cut -c 1-2)\" &&\n> +\t\tfile=\"$(echo \"$a_blob\" | cut -c 3-)\" &&\n\nCan't this use test_oid_to_path?\n> +\t\trm \".git/objects/$dir/$file\" &&\n> +\t\trm *.a *.b &&\n> +\n> +\t\ttest_checkout_workers 2 test_must_fail git checkout . 2>err &&\n> +\n> +\t\t# All *.b entries should succeed and all *.a entries should fail:\n> +\t\t#  - missing-delay.a: the delay filter will drop this path\n> +\t\t#  - parallel-*.a: the blob will be missing\n> +\t\t#\n> +\t\tgrep \"Updated 3 paths from the index\" err &&\n> +\t\ttest \"$(ls *.b | wc -l)\" -eq 3 &&\n> +\t\ttest \"$(ls *.a | wc -l)\" -eq 0\n\nAnd this test_stdout_line_count?\n"},{"id":"458953","messageId":"CAHd-oW4rBe5db8RFZa0S4OJd1RT3sXyBEW_QX8QqL8SJtWTm8g@mail.gmail.com","threadId":"58148","inReplyTo":"CAHd-oW6AeOGv=zQ=9Udtzwau=5XbQkhuctVDa0=4PoMTSU20HQ@mail.gmail.com","subject":"Re: [PATCH 2/3] checkout: show bug about failed entries being included in final report","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2022-07-13T13:00:13Z","receivedAt":"2022-07-13T13:00:32Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Wed, Jul 13, 2022 at 8:15 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> On Wed, Jul 13 2022, Matheus Tavares wrote:\n> >\n> > +             a_blob=\"$(git rev-parse :parallel-ineligible.a)\" &&\n> > +             dir=\"$(echo \"$a_blob\" | cut -c 1-2)\" &&\n> > +             file=\"$(echo \"$a_blob\" | cut -c 3-)\" &&\n>\n> Can't this use test_oid_to_path?\n[...]\n> > +             test \"$(ls *.b | wc -l)\" -eq 3 &&\n> > +             test \"$(ls *.a | wc -l)\" -eq 0\n>\n> And this test_stdout_line_count?\n\nSure! Thanks for pointing that out. Will change.\n"},{"id":"458997","messageId":"xmqqfsj4dhfi.fsf@gitster.g","threadId":"58148","inReplyTo":"694aeb19f57297d9b9d07d47897385bdbedd309c.1657685948.git.matheus.bernardino@usp.br","subject":"Re: [PATCH 1/3] checkout: document bug where delayed checkout counts entries twice","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-13T17:57:21Z","receivedAt":"2022-07-13T17:57:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares <matheus.bernardino@usp.br> writes:\n\n> At the end of a `git checkout <pathspec>` operation, git reports how\n> many paths were checked out with a message like \"Updated N paths from\n> the index\". However, entries that end up on the delayed checkout queue\n> (as requested by a long-running process filter) get counted twice,\n> producing a wrong number in the final report. We will fix this bug in an\n> upcoming commit. For now, only document/demonstrate it with a\n> test_expect_failure.\n>\n> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n> ---\n>  t/t0021-conversion.sh | 22 ++++++++++++++++++++++\n>  1 file changed, 22 insertions(+)\n>\n> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> index bad37abad2..00df9b5c18 100755\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -1132,4 +1132,26 @@ do\n>  \t'\n>  done\n>  \n> +test_expect_failure PERL 'delayed checkout correctly reports the number of updated entries' '\n\nIt is unfortunate that we depend on Perl only to run rot13-filter;\nI'll leave a #leftoverbit label here to remind us to write a\n\"test-tool rot13-filter\" someday.  No need to do so in this series.\n\n> +\trm -rf repo &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tgit config filter.delay.process \"../rot13-filter.pl delayed.log clean smudge delay\" &&\n> +\t\tgit config filter.delay.required true &&\n> +\n> +\t\techo \"*.a filter=delay\" >.gitattributes &&\n> +\t\techo a >test-delay10.a &&\n> +\t\techo a >test-delay11.a &&\n> +\t\tgit add . &&\n> +\t\tgit commit -m files &&\n> +\n> +\t\trm *.a &&\n> +\t\tgit checkout . 2>err &&\n> +\t\tgrep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delayed.log &&\n> +\t\tgrep \"IN: smudge test-delay11.a .* \\\\[DELAYED\\\\]\" delayed.log &&\n> +\t\tgrep \"Updated 2 paths from the index\" err\n> +\t)\n> +'\n> +\n>  test_done\n"},{"id":"459038","messageId":"cover.1657799213.git.matheus.bernardino@usp.br","threadId":"58148","inReplyTo":"cover.1657685948.git.matheus.bernardino@usp.br","subject":"[PATCH v2 0/3] checkout: fix two bugs on count of updated entries","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2022-07-14T11:49:09Z","receivedAt":"2022-07-14T11:49:28Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Changed since v1: small simplification at test t2080, to use\ntest_oid_to_path and test_stdout_line_count.\n\n\nv1 cover letter:\n\nThis fixes two issues at the \"Updated %d path from the index\" report at\nthe end of a `git checkout <paths>` operation:\n\n  - Delayed checkout entries being counted twice.\n  - Failed entries being included in the count.\n\nThe first two patches add tests and the third implements the fix. I came\nacross this while working at parallel checkout, but only managed to get\nback to it now.\n\nMatheus Tavares (3):\n  checkout: document bug where delayed checkout counts entries twice\n  checkout: show bug about failed entries being included in final report\n  checkout: fix two bugs on the final count of updated entries\n\n builtin/checkout.c                  |  2 +-\n convert.h                           |  6 +++-\n entry.c                             | 34 +++++++++++---------\n entry.h                             |  3 +-\n parallel-checkout.c                 | 10 ++++--\n parallel-checkout.h                 |  4 ++-\n t/lib-parallel-checkout.sh          |  6 +++-\n t/t0021-conversion.sh               | 22 +++++++++++++\n t/t2080-parallel-checkout-basics.sh | 48 +++++++++++++++++++++++++++++\n unpack-trees.c                      |  2 +-\n 10 files changed, 113 insertions(+), 24 deletions(-)\n\nRange-diff against v1:\n1:  694aeb19f5 = 1:  694aeb19f5 checkout: document bug where delayed checkout counts entries twice\n2:  8da18a0a8c ! 2:  4541e90224 checkout: show bug about failed entries being included in final report\n    @@ t/t2080-parallel-checkout-basics.sh: test_expect_success SYMLINKS 'parallel chec\n     +\t\tgit commit -m files &&\n     +\n     +\t\ta_blob=\"$(git rev-parse :parallel-ineligible.a)\" &&\n    -+\t\tdir=\"$(echo \"$a_blob\" | cut -c 1-2)\" &&\n    -+\t\tfile=\"$(echo \"$a_blob\" | cut -c 3-)\" &&\n    -+\t\trm \".git/objects/$dir/$file\" &&\n    ++\t\trm .git/objects/$(test_oid_to_path $a_blob) &&\n     +\t\trm *.a *.b &&\n     +\n     +\t\ttest_checkout_workers 2 test_must_fail git checkout . 2>err &&\n    @@ t/t2080-parallel-checkout-basics.sh: test_expect_success SYMLINKS 'parallel chec\n     +\t\t#  - parallel-*.a: the blob will be missing\n     +\t\t#\n     +\t\tgrep \"Updated 3 paths from the index\" err &&\n    -+\t\ttest \"$(ls *.b | wc -l)\" -eq 3 &&\n    -+\t\ttest \"$(ls *.a | wc -l)\" -eq 0\n    ++\t\ttest_stdout_line_count = 3 ls *.b &&\n    ++\t\t! ls *.a\n     +\t)\n     +'\n     +\n3:  5e9452be66 = 3:  b182d74e96 checkout: fix two bugs on the final count of updated entries\n-- \n2.37.0\n\n"},{"id":"459039","messageId":"694aeb19f57297d9b9d07d47897385bdbedd309c.1657799213.git.matheus.bernardino@usp.br","threadId":"58148","inReplyTo":"cover.1657799213.git.matheus.bernardino@usp.br","subject":"[PATCH v2 1/3] checkout: document bug where delayed checkout counts entries twice","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2022-07-14T11:49:10Z","receivedAt":"2022-07-14T11:49:31Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"At the end of a `git checkout <pathspec>` operation, git reports how\nmany paths were checked out with a message like \"Updated N paths from\nthe index\". However, entries that end up on the delayed checkout queue\n(as requested by a long-running process filter) get counted twice,\nproducing a wrong number in the final report. We will fix this bug in an\nupcoming commit. For now, only document/demonstrate it with a\ntest_expect_failure.\n\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n t/t0021-conversion.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex bad37abad2..00df9b5c18 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -1132,4 +1132,26 @@ do\n \t'\n done\n \n+test_expect_failure PERL 'delayed checkout correctly reports the number of updated entries' '\n+\trm -rf repo &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config filter.delay.process \"../rot13-filter.pl delayed.log clean smudge delay\" &&\n+\t\tgit config filter.delay.required true &&\n+\n+\t\techo \"*.a filter=delay\" >.gitattributes &&\n+\t\techo a >test-delay10.a &&\n+\t\techo a >test-delay11.a &&\n+\t\tgit add . &&\n+\t\tgit commit -m files &&\n+\n+\t\trm *.a &&\n+\t\tgit checkout . 2>err &&\n+\t\tgrep \"IN: smudge test-delay10.a .* \\\\[DELAYED\\\\]\" delayed.log &&\n+\t\tgrep \"IN: smudge test-delay11.a .* \\\\[DELAYED\\\\]\" delayed.log &&\n+\t\tgrep \"Updated 2 paths from the index\" err\n+\t)\n+'\n+\n test_done\n-- \n2.37.0\n\n"},{"id":"459040","messageId":"4541e90224f5e79b16717b94f37edd10a4fdbbb1.1657799213.git.matheus.bernardino@usp.br","threadId":"58148","inReplyTo":"cover.1657799213.git.matheus.bernardino@usp.br","subject":"[PATCH v2 2/3] checkout: show bug about failed entries being included in final report","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2022-07-14T11:49:11Z","receivedAt":"2022-07-14T11:49:36Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"After checkout, git usually reports how many entries were updated at\nthat operation. However, because we count the entries too soon during\nthe checkout process, we may actually include entries that do not get\nproperly checked out in the end. This can lead to an inaccurate final\nreport if the user expects it to show only the *successful* updates.\nThis will be fixed in the next commit, but for now let's document it\nwith a test that cover all checkout modes.\n\nNote that `test_checkout_workers` have to be slightly adjusted in order\nto use the construct `test_checkout_workers ...  test_must_fail git\ncheckout`. The function runs the command given to it with an assignment\nprefix to set the GIT_TRACE2 variable. However, this this assignment has\nan undefined behavior when the command is a shell function (like\n`test_must_fail`). As POSIX specifies:\n\n  If the command name is a function that is not a standard utility\n  implemented as a function, variable assignments shall affect the\n  current execution environment during the execution of the function. It\n  is unspecified:\n\n    - Whether or not the variable assignments persist after the\n      completion of the function\n\n    - Whether or not the variables gain the export attribute during the\n      execution of the function\n\nThus, in order to make sure the GIT_TRACE2 value gets visible to the git\ncommand executed by `test_must_fail`, export the variable and run git in\na subshell.\n\n[1]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html\n     (Vol. 3: Shell and Utilities, Section 2.9.1: Simple Commands)\n\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n t/lib-parallel-checkout.sh          |  6 +++-\n t/t2080-parallel-checkout-basics.sh | 48 +++++++++++++++++++++++++++++\n 2 files changed, 53 insertions(+), 1 deletion(-)\n\ndiff --git a/t/lib-parallel-checkout.sh b/t/lib-parallel-checkout.sh\nindex 83b279a846..acaee9cbb6 100644\n--- a/t/lib-parallel-checkout.sh\n+++ b/t/lib-parallel-checkout.sh\n@@ -25,7 +25,11 @@ test_checkout_workers () {\n \n \tlocal trace_file=trace-test-checkout-workers &&\n \trm -f \"$trace_file\" &&\n-\tGIT_TRACE2=\"$(pwd)/$trace_file\" \"$@\" 2>&8 &&\n+\t(\n+\t\tGIT_TRACE2=\"$(pwd)/$trace_file\" &&\n+\t\texport GIT_TRACE2 &&\n+\t\t\"$@\" 2>&8\n+\t) &&\n \n \tlocal workers=\"$(grep \"child_start\\[..*\\] git checkout--worker\" \"$trace_file\" | wc -l)\" &&\n \ttest $workers -eq $expected_workers &&\ndiff --git a/t/t2080-parallel-checkout-basics.sh b/t/t2080-parallel-checkout-basics.sh\nindex 3e0f8c675f..7d6d26e1a4 100755\n--- a/t/t2080-parallel-checkout-basics.sh\n+++ b/t/t2080-parallel-checkout-basics.sh\n@@ -226,4 +226,52 @@ test_expect_success SYMLINKS 'parallel checkout checks for symlinks in leading d\n \t)\n '\n \n+# This test is here (and not in e.g. t2022-checkout-paths.sh), because we\n+# check the final report including sequential, parallel, and delayed entries\n+# all at the same time. So we must have finer control of the parallel checkout\n+# variables.\n+test_expect_failure PERL '\"git checkout .\" report should not include failed entries' '\n+\twrite_script rot13-filter.pl \"$PERL_PATH\" \\\n+\t\t<\"$TEST_DIRECTORY\"/t0021/rot13-filter.pl &&\n+\n+\ttest_config_global filter.delay.process \\\n+\t\t\"\\\"$(pwd)/rot13-filter.pl\\\" --always-delay delayed.log clean smudge delay\" &&\n+\ttest_config_global filter.delay.required true &&\n+\ttest_config_global filter.cat.clean cat  &&\n+\ttest_config_global filter.cat.smudge cat  &&\n+\ttest_config_global filter.cat.required true  &&\n+\n+\tset_checkout_config 2 0 &&\n+\tgit init failed_entries &&\n+\t(\n+\t\tcd failed_entries &&\n+\t\tcat >.gitattributes <<-EOF &&\n+\t\t*delay*              filter=delay\n+\t\tparallel-ineligible* filter=cat\n+\t\tEOF\n+\t\techo a >missing-delay.a &&\n+\t\techo a >parallel-ineligible.a &&\n+\t\techo a >parallel-eligible.a &&\n+\t\techo b >success-delay.b &&\n+\t\techo b >parallel-ineligible.b &&\n+\t\techo b >parallel-eligible.b &&\n+\t\tgit add -A &&\n+\t\tgit commit -m files &&\n+\n+\t\ta_blob=\"$(git rev-parse :parallel-ineligible.a)\" &&\n+\t\trm .git/objects/$(test_oid_to_path $a_blob) &&\n+\t\trm *.a *.b &&\n+\n+\t\ttest_checkout_workers 2 test_must_fail git checkout . 2>err &&\n+\n+\t\t# All *.b entries should succeed and all *.a entries should fail:\n+\t\t#  - missing-delay.a: the delay filter will drop this path\n+\t\t#  - parallel-*.a: the blob will be missing\n+\t\t#\n+\t\tgrep \"Updated 3 paths from the index\" err &&\n+\t\ttest_stdout_line_count = 3 ls *.b &&\n+\t\t! ls *.a\n+\t)\n+'\n+\n test_done\n-- \n2.37.0\n\n"},{"id":"459041","messageId":"b182d74e967aaa38d54c502f45a0b94af5845833.1657799213.git.matheus.bernardino@usp.br","threadId":"58148","inReplyTo":"cover.1657799213.git.matheus.bernardino@usp.br","subject":"[PATCH v2 3/3] checkout: fix two bugs on the final count of updated entries","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2022-07-14T11:49:12Z","receivedAt":"2022-07-14T11:49:42Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"At the end of `git checkout <pathspec>`, we get a message informing how\nmany entries were updated in the working tree. However, this number can\nbe inaccurate for two reasons:\n\n1) Delayed entries currently get counted twice.\n2) Failed entries are included in the count.\n\nThe first problem happens because the counter is first incremented\nbefore inserting the entry in the delayed checkout queue, and once again\nwhen finish_delayed_checkout() calls checkout_entry(). And the second\nhappens because the counter is incremented too early in\ncheckout_entry(), before the entry was in fact checked out. Fix that by\nmoving the count increment further down in the call stack and removing\nthe duplicate increment on delayed entries. Note that we have to keep\na per-entry reference for the counter (both on parallel checkout and\ndelayed checkout) because not all entries are always accumulated at the\nsame counter. See checkout_worktree(), at builtin/checkout.c for an\nexample.\n\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n builtin/checkout.c                  |  2 +-\n convert.h                           |  6 ++++-\n entry.c                             | 34 +++++++++++++++++------------\n entry.h                             |  3 +--\n parallel-checkout.c                 | 10 ++++++---\n parallel-checkout.h                 |  4 +++-\n t/t0021-conversion.sh               |  2 +-\n t/t2080-parallel-checkout-basics.sh |  2 +-\n unpack-trees.c                      |  2 +-\n 9 files changed, 40 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2eefda81d8..df3f1663d7 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -417,7 +417,7 @@ static int checkout_worktree(const struct checkout_opts *opts,\n \tmem_pool_discard(&ce_mem_pool, should_validate_cache_entries());\n \tremove_marked_cache_entries(&the_index, 1);\n \tremove_scheduled_dirs();\n-\terrs |= finish_delayed_checkout(&state, &nr_checkouts, opts->show_progress);\n+\terrs |= finish_delayed_checkout(&state, opts->show_progress);\n \n \tif (opts->count_checkout_paths) {\n \t\tif (nr_unmerged)\ndiff --git a/convert.h b/convert.h\nindex 5ee1c32205..0a6e4086b8 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -53,7 +53,11 @@ struct delayed_checkout {\n \tenum ce_delay_state state;\n \t/* List of filter drivers that signaled delayed blobs. */\n \tstruct string_list filters;\n-\t/* List of delayed blobs identified by their path. */\n+\t/*\n+\t * List of delayed blobs identified by their path. The `util` member\n+\t * holds a counter pointer which must be incremented when/if the\n+\t * associated blob gets checked out.\n+\t */\n \tstruct string_list paths;\n };\n \ndiff --git a/entry.c b/entry.c\nindex 1c9df62b30..616e4f073c 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -157,12 +157,11 @@ static int remove_available_paths(struct string_list_item *item, void *cb_data)\n \n \tavailable = string_list_lookup(available_paths, item->string);\n \tif (available)\n-\t\tavailable->util = (void *)item->string;\n+\t\tavailable->util = item->util;\n \treturn !available;\n }\n \n-int finish_delayed_checkout(struct checkout *state, int *nr_checkouts,\n-\t\t\t    int show_progress)\n+int finish_delayed_checkout(struct checkout *state, int show_progress)\n {\n \tint errs = 0;\n \tunsigned processed_paths = 0;\n@@ -227,7 +226,7 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts,\n \t\t\t\t\t\t       strlen(path->string), 0);\n \t\t\t\tif (ce) {\n \t\t\t\t\tdisplay_progress(progress, ++processed_paths);\n-\t\t\t\t\terrs |= checkout_entry(ce, state, NULL, nr_checkouts);\n+\t\t\t\t\terrs |= checkout_entry(ce, state, NULL, path->util);\n \t\t\t\t\tfiltered_bytes += ce->ce_stat_data.sd_size;\n \t\t\t\t\tdisplay_throughput(progress, filtered_bytes);\n \t\t\t\t} else\n@@ -266,7 +265,8 @@ void update_ce_after_write(const struct checkout *state, struct cache_entry *ce,\n \n /* Note: ca is used (and required) iff the entry refers to a regular file. */\n static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca,\n-\t\t       const struct checkout *state, int to_tempfile)\n+\t\t       const struct checkout *state, int to_tempfile,\n+\t\t       int *nr_checkouts)\n {\n \tunsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;\n \tstruct delayed_checkout *dco = state->delayed_checkout;\n@@ -279,6 +279,7 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca\n \tstruct stat st;\n \tconst struct submodule *sub;\n \tstruct checkout_metadata meta;\n+\tstatic int scratch_nr_checkouts;\n \n \tclone_checkout_metadata(&meta, &state->meta, &ce->oid);\n \n@@ -333,9 +334,15 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca\n \t\t\tret = async_convert_to_working_tree_ca(ca, ce->name,\n \t\t\t\t\t\t\t       new_blob, size,\n \t\t\t\t\t\t\t       &buf, &meta, dco);\n-\t\t\tif (ret && string_list_has_string(&dco->paths, ce->name)) {\n-\t\t\t\tfree(new_blob);\n-\t\t\t\tgoto delayed;\n+\t\t\tif (ret) {\n+\t\t\t\tstruct string_list_item *item =\n+\t\t\t\t\tstring_list_lookup(&dco->paths, ce->name);\n+\t\t\t\tif (item) {\n+\t\t\t\t\titem->util = nr_checkouts ? nr_checkouts\n+\t\t\t\t\t\t\t: &scratch_nr_checkouts;\n+\t\t\t\t\tfree(new_blob);\n+\t\t\t\t\tgoto delayed;\n+\t\t\t\t}\n \t\t\t}\n \t\t} else {\n \t\t\tret = convert_to_working_tree_ca(ca, ce->name, new_blob,\n@@ -392,6 +399,8 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca\n \t\t\t\t\t   ce->name);\n \t\tupdate_ce_after_write(state, ce , &st);\n \t}\n+\tif (nr_checkouts)\n+\t\t(*nr_checkouts)++;\n delayed:\n \treturn 0;\n }\n@@ -476,7 +485,7 @@ int checkout_entry_ca(struct cache_entry *ce, struct conv_attrs *ca,\n \t\t\tconvert_attrs(state->istate, &ca_buf, ce->name);\n \t\t\tca = &ca_buf;\n \t\t}\n-\t\treturn write_entry(ce, topath, ca, state, 1);\n+\t\treturn write_entry(ce, topath, ca, state, 1, nr_checkouts);\n \t}\n \n \tstrbuf_reset(&path);\n@@ -540,18 +549,15 @@ int checkout_entry_ca(struct cache_entry *ce, struct conv_attrs *ca,\n \n \tcreate_directories(path.buf, path.len, state);\n \n-\tif (nr_checkouts)\n-\t\t(*nr_checkouts)++;\n-\n \tif (S_ISREG(ce->ce_mode) && !ca) {\n \t\tconvert_attrs(state->istate, &ca_buf, ce->name);\n \t\tca = &ca_buf;\n \t}\n \n-\tif (!enqueue_checkout(ce, ca))\n+\tif (!enqueue_checkout(ce, ca, nr_checkouts))\n \t\treturn 0;\n \n-\treturn write_entry(ce, path.buf, ca, state, 0);\n+\treturn write_entry(ce, path.buf, ca, state, 0, nr_checkouts);\n }\n \n void unlink_entry(const struct cache_entry *ce)\ndiff --git a/entry.h b/entry.h\nindex 252fd24c2e..9be4659881 100644\n--- a/entry.h\n+++ b/entry.h\n@@ -43,8 +43,7 @@ static inline int checkout_entry(struct cache_entry *ce,\n }\n \n void enable_delayed_checkout(struct checkout *state);\n-int finish_delayed_checkout(struct checkout *state, int *nr_checkouts,\n-\t\t\t    int show_progress);\n+int finish_delayed_checkout(struct checkout *state, int show_progress);\n \n /*\n  * Unlink the last component and schedule the leading directories for\ndiff --git a/parallel-checkout.c b/parallel-checkout.c\nindex 31a3d0ee1b..4f6819f240 100644\n--- a/parallel-checkout.c\n+++ b/parallel-checkout.c\n@@ -143,7 +143,8 @@ static int is_eligible_for_parallel_checkout(const struct cache_entry *ce,\n \t}\n }\n \n-int enqueue_checkout(struct cache_entry *ce, struct conv_attrs *ca)\n+int enqueue_checkout(struct cache_entry *ce, struct conv_attrs *ca,\n+\t\t     int *checkout_counter)\n {\n \tstruct parallel_checkout_item *pc_item;\n \n@@ -159,6 +160,7 @@ int enqueue_checkout(struct cache_entry *ce, struct conv_attrs *ca)\n \tmemcpy(&pc_item->ca, ca, sizeof(pc_item->ca));\n \tpc_item->status = PC_ITEM_PENDING;\n \tpc_item->id = parallel_checkout.nr;\n+\tpc_item->checkout_counter = checkout_counter;\n \tparallel_checkout.nr++;\n \n \treturn 0;\n@@ -200,7 +202,8 @@ static int handle_results(struct checkout *state)\n \n \t\tswitch(pc_item->status) {\n \t\tcase PC_ITEM_WRITTEN:\n-\t\t\t/* Already handled */\n+\t\t\tif (pc_item->checkout_counter)\n+\t\t\t\t(*pc_item->checkout_counter)++;\n \t\t\tbreak;\n \t\tcase PC_ITEM_COLLIDED:\n \t\t\t/*\n@@ -225,7 +228,8 @@ static int handle_results(struct checkout *state)\n \t\t\t * add any extra overhead.\n \t\t\t */\n \t\t\tret |= checkout_entry_ca(pc_item->ce, &pc_item->ca,\n-\t\t\t\t\t\t state, NULL, NULL);\n+\t\t\t\t\t\t state, NULL,\n+\t\t\t\t\t\t pc_item->checkout_counter);\n \t\t\tadvance_progress_meter();\n \t\t\tbreak;\n \t\tcase PC_ITEM_PENDING:\ndiff --git a/parallel-checkout.h b/parallel-checkout.h\nindex 80f539bcb7..c575284005 100644\n--- a/parallel-checkout.h\n+++ b/parallel-checkout.h\n@@ -31,7 +31,8 @@ void init_parallel_checkout(void);\n  * entry is not eligible for parallel checkout. Otherwise, enqueue the entry\n  * for later write and return 0.\n  */\n-int enqueue_checkout(struct cache_entry *ce, struct conv_attrs *ca);\n+int enqueue_checkout(struct cache_entry *ce, struct conv_attrs *ca,\n+\t\t     int *checkout_counter);\n size_t pc_queue_size(void);\n \n /*\n@@ -68,6 +69,7 @@ struct parallel_checkout_item {\n \tstruct cache_entry *ce;\n \tstruct conv_attrs ca;\n \tsize_t id; /* position in parallel_checkout.items[] of main process */\n+\tint *checkout_counter;\n \n \t/* Output fields, sent from workers. */\n \tenum pc_item_status status;\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 00df9b5c18..1c840348bd 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -1132,7 +1132,7 @@ do\n \t'\n done\n \n-test_expect_failure PERL 'delayed checkout correctly reports the number of updated entries' '\n+test_expect_success PERL 'delayed checkout correctly reports the number of updated entries' '\n \trm -rf repo &&\n \tgit init repo &&\n \t(\ndiff --git a/t/t2080-parallel-checkout-basics.sh b/t/t2080-parallel-checkout-basics.sh\nindex 7d6d26e1a4..c683e60007 100755\n--- a/t/t2080-parallel-checkout-basics.sh\n+++ b/t/t2080-parallel-checkout-basics.sh\n@@ -230,7 +230,7 @@ test_expect_success SYMLINKS 'parallel checkout checks for symlinks in leading d\n # check the final report including sequential, parallel, and delayed entries\n # all at the same time. So we must have finer control of the parallel checkout\n # variables.\n-test_expect_failure PERL '\"git checkout .\" report should not include failed entries' '\n+test_expect_success PERL '\"git checkout .\" report should not include failed entries' '\n \twrite_script rot13-filter.pl \"$PERL_PATH\" \\\n \t\t<\"$TEST_DIRECTORY\"/t0021/rot13-filter.pl &&\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex d561ca01ed..8a454e03bf 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -487,7 +487,7 @@ static int check_updates(struct unpack_trees_options *o,\n \t\terrs |= run_parallel_checkout(&state, pc_workers, pc_threshold,\n \t\t\t\t\t      progress, &cnt);\n \tstop_progress(&progress);\n-\terrs |= finish_delayed_checkout(&state, NULL, o->verbose_update);\n+\terrs |= finish_delayed_checkout(&state, o->verbose_update);\n \tgit_attr_set_direction(GIT_ATTR_CHECKIN);\n \n \tif (o->clone)\n-- \n2.37.0\n\n"}]}