@matheusbsbs / matheus-bagliano / commits / 611c7785e8

checkout: fix two bugs on the final count of updated entries

At the end of `git checkout <pathspec>`, we get a message informing how many entries were updated in the working tree. However, this number can be inaccurate for two reasons: 1) Delayed entries currently get counted twice. 2) Failed entries are included in the count. The first problem happens because the counter is first incremented before inserting the entry in the delayed checkout queue, and once again when finish_delayed_checkout() calls checkout_entry(). And the second happens because the counter is incremented too early in checkout_entry(), before the entry was in fact checked out. Fix that by moving the count increment further down in the call stack and removing the duplicate increment on delayed entries. Note that we have to keep a per-entry reference for the counter (both on parallel checkout and delayed checkout) because not all entries are always accumulated at the same counter. See checkout_worktree(), at builtin/checkout.c for an example. Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Matheus Tavares committed Jul 14, 2022 at 08:49 UTC 611c7785e8e22637e183333c54ed266e6e83e163

9 files changed +40 -25

builtin/checkout.c

+1 -1

@@ -417,7 +417,7 @@ static int checkout_worktree(const struct checkout_opts *opts,

417 mem_pool_discard(&ce_mem_pool, should_validate_cache_entries());

418 remove_marked_cache_entries(&the_index, 1);

419 remove_scheduled_dirs();

420 - errs |= finish_delayed_checkout(&state, &nr_checkouts, opts->show_progress);

420 + errs |= finish_delayed_checkout(&state, opts->show_progress);

421

422 if (opts->count_checkout_paths) {

423 if (nr_unmerged)

convert.h

+5 -1

@@ -53,7 +53,11 @@ struct delayed_checkout {

53 enum ce_delay_state state;

54 /* List of filter drivers that signaled delayed blobs. */

55 struct string_list filters;

56 - /* List of delayed blobs identified by their path. */

56 + /*

57 + * List of delayed blobs identified by their path. The `util` member

58 + * holds a counter pointer which must be incremented when/if the

59 + * associated blob gets checked out.

60 + */

61 struct string_list paths;

62 };

entry.c

+20 -14

@@ -157,12 +157,11 @@ static int remove_available_paths(struct string_list_item *item, void *cb_data)

157

158 available = string_list_lookup(available_paths, item->string);

159 if (available)

160 - available->util = (void *)item->string;

160 + available->util = item->util;

161 return !available;

162 }

163

164 -int finish_delayed_checkout(struct checkout *state, int *nr_checkouts,

165 - int show_progress)

164 +int finish_delayed_checkout(struct checkout *state, int show_progress)

165 {

166 int errs = 0;

167 unsigned processed_paths = 0;

@@ -227,7 +226,7 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts,

226 strlen(path->string), 0);

227 if (ce) {

228 display_progress(progress, ++processed_paths);

230 - errs |= checkout_entry(ce, state, NULL, nr_checkouts);

229 + errs |= checkout_entry(ce, state, NULL, path->util);

230 filtered_bytes += ce->ce_stat_data.sd_size;

231 display_throughput(progress, filtered_bytes);

232 } else

@@ -266,7 +265,8 @@ void update_ce_after_write(const struct checkout *state, struct cache_entry *ce,

265

266 /* Note: ca is used (and required) iff the entry refers to a regular file. */

267 static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca,

269 - const struct checkout *state, int to_tempfile)

268 + const struct checkout *state, int to_tempfile,

269 + int *nr_checkouts)

270 {

271 unsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;

272 struct delayed_checkout *dco = state->delayed_checkout;

@@ -279,6 +279,7 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca

279 struct stat st;

280 const struct submodule *sub;

281 struct checkout_metadata meta;

282 + static int scratch_nr_checkouts;

283

284 clone_checkout_metadata(&meta, &state->meta, &ce->oid);

285

@@ -333,9 +334,15 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca

334 ret = async_convert_to_working_tree_ca(ca, ce->name,

335 new_blob, size,

336 &buf, &meta, dco);

336 - if (ret && string_list_has_string(&dco->paths, ce->name)) {

337 - free(new_blob);

338 - goto delayed;

337 + if (ret) {

338 + struct string_list_item *item =

339 + string_list_lookup(&dco->paths, ce->name);

340 + if (item) {

341 + item->util = nr_checkouts ? nr_checkouts

342 + : &scratch_nr_checkouts;

343 + free(new_blob);

344 + goto delayed;

345 + }

346 }

347 } else {

348 ret = convert_to_working_tree_ca(ca, ce->name, new_blob,

@@ -392,6 +399,8 @@ finish:

399 ce->name);

400 update_ce_after_write(state, ce , &st);

401 }

402 + if (nr_checkouts)

403 + (*nr_checkouts)++;

404 delayed:

405 return 0;

406 }

@@ -476,7 +485,7 @@ int checkout_entry_ca(struct cache_entry *ce, struct conv_attrs *ca,

485 convert_attrs(state->istate, &ca_buf, ce->name);

486 ca = &ca_buf;

487 }

479 - return write_entry(ce, topath, ca, state, 1);

488 + return write_entry(ce, topath, ca, state, 1, nr_checkouts);

489 }

490

491 strbuf_reset(&path);

@@ -540,18 +549,15 @@ int checkout_entry_ca(struct cache_entry *ce, struct conv_attrs *ca,

549

550 create_directories(path.buf, path.len, state);

551

543 - if (nr_checkouts)

544 - (*nr_checkouts)++;

545 -

552 if (S_ISREG(ce->ce_mode) && !ca) {

553 convert_attrs(state->istate, &ca_buf, ce->name);

554 ca = &ca_buf;

555 }

556

551 - if (!enqueue_checkout(ce, ca))

557 + if (!enqueue_checkout(ce, ca, nr_checkouts))

558 return 0;

559

554 - return write_entry(ce, path.buf, ca, state, 0);

560 + return write_entry(ce, path.buf, ca, state, 0, nr_checkouts);

561 }

562

563 void unlink_entry(const struct cache_entry *ce)

entry.h

+1 -2

@@ -43,8 +43,7 @@ static inline int checkout_entry(struct cache_entry *ce,

43 }

45 void enable_delayed_checkout(struct checkout *state);

46 -int finish_delayed_checkout(struct checkout *state, int *nr_checkouts,

47 - int show_progress);

46 +int finish_delayed_checkout(struct checkout *state, int show_progress);

48 /*

49 * Unlink the last component and schedule the leading directories for

parallel-checkout.c

+7 -3

@@ -143,7 +143,8 @@ static int is_eligible_for_parallel_checkout(const struct cache_entry *ce,

143 }

144 }

145

146 -int enqueue_checkout(struct cache_entry *ce, struct conv_attrs *ca)

146 +int enqueue_checkout(struct cache_entry *ce, struct conv_attrs *ca,

147 + int *checkout_counter)

148 {

149 struct parallel_checkout_item *pc_item;

150

@@ -159,6 +160,7 @@ int enqueue_checkout(struct cache_entry *ce, struct conv_attrs *ca)

160 memcpy(&pc_item->ca, ca, sizeof(pc_item->ca));

161 pc_item->status = PC_ITEM_PENDING;

162 pc_item->id = parallel_checkout.nr;

163 + pc_item->checkout_counter = checkout_counter;

164 parallel_checkout.nr++;

165

166 return 0;

@@ -200,7 +202,8 @@ static int handle_results(struct checkout *state)

202

203 switch(pc_item->status) {

204 case PC_ITEM_WRITTEN:

203 - /* Already handled */

205 + if (pc_item->checkout_counter)

206 + (*pc_item->checkout_counter)++;

207 break;

208 case PC_ITEM_COLLIDED:

209 /*

@@ -225,7 +228,8 @@ static int handle_results(struct checkout *state)

228 * add any extra overhead.

229 */

230 ret |= checkout_entry_ca(pc_item->ce, &pc_item->ca,

228 - state, NULL, NULL);

231 + state, NULL,

232 + pc_item->checkout_counter);

233 advance_progress_meter();

234 break;

235 case PC_ITEM_PENDING:

parallel-checkout.h

+3 -1

@@ -31,7 +31,8 @@ void init_parallel_checkout(void);

31 * entry is not eligible for parallel checkout. Otherwise, enqueue the entry

32 * for later write and return 0.

33 */

34 -int enqueue_checkout(struct cache_entry *ce, struct conv_attrs *ca);

34 +int enqueue_checkout(struct cache_entry *ce, struct conv_attrs *ca,

35 + int *checkout_counter);

36 size_t pc_queue_size(void);

38 /*

@@ -68,6 +69,7 @@ struct parallel_checkout_item {

69 struct cache_entry *ce;

70 struct conv_attrs ca;

71 size_t id; /* position in parallel_checkout.items[] of main process */

72 + int *checkout_counter;

74 /* Output fields, sent from workers. */

75 enum pc_item_status status;

t/t0021-conversion.sh

+1 -1

@@ -1132,7 +1132,7 @@ do

1132 '

1133 done

1134

1135 -test_expect_failure PERL 'delayed checkout correctly reports the number of updated entries' '

1135 +test_expect_success PERL 'delayed checkout correctly reports the number of updated entries' '

1136 rm -rf repo &&

1137 git init repo &&

1138 (

t/t2080-parallel-checkout-basics.sh

+1 -1

@@ -230,7 +230,7 @@ test_expect_success SYMLINKS 'parallel checkout checks for symlinks in leading d

230 # check the final report including sequential, parallel, and delayed entries

231 # all at the same time. So we must have finer control of the parallel checkout

232 # variables.

233 -test_expect_failure PERL '"git checkout ." report should not include failed entries' '

233 +test_expect_success PERL '"git checkout ." report should not include failed entries' '

234 write_script rot13-filter.pl "$PERL_PATH" \

235 <"$TEST_DIRECTORY"/t0021/rot13-filter.pl &&

236

unpack-trees.c

+1 -1

@@ -487,7 +487,7 @@ static int check_updates(struct unpack_trees_options *o,

487 errs |= run_parallel_checkout(&state, pc_workers, pc_threshold,

488 progress, &cnt);

489 stop_progress(&progress);

490 - errs |= finish_delayed_checkout(&state, NULL, o->verbose_update);

490 + errs |= finish_delayed_checkout(&state, o->verbose_update);

491 git_attr_set_direction(GIT_ATTR_CHECKIN);

492

493 if (o->clone)