@matheusbsbs / matheus-bagliano / commits / 5cdb38458e

2.36 show regression fix

This only surfaced as a regression after 2.36 release, but the breakage was already there with us for at least a year. e900d494 (diff: add an API for deferred freeing, 2021-02-11) introduced a mechanism to delay freeing resources held in diff_options struct that need to be kept as long as the struct will be reused to compute diff. "git log -p" was taught to utilize the mechanism but it was done with an incorrect assumption that the underlying helper function, cmd_log_walk(), is called only once, and it is OK to do the freeing at the end of it. Alas, for "git show A B", the function is called once for each commit given, so it is not OK to free the resources until we finish calling it for all the commits given from the command line. During 2.36 release cycle, we started clearing the <pathspec> as part of this freeing, which made the bug a lot more visible. Fix this breakage by tweaking how cmd_log_walk() frees the resources at the end and using a variant of it that does not immediately free the resources to show each commit object from the command line in "git show". Protect the fix with a few new tests. Reported-by: Daniel Li <dan@danielyli.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Junio C Hamano committed Apr 29, 2022 at 22:29 UTC 5cdb38458ee5b53daada9cb767ae51a08650bd6a

2 files changed +37 -5

builtin/log.c

+18 -5

@@ -417,7 +417,7 @@ static void finish_early_output(struct rev_info *rev)

417 show_early_header(rev, "done", n);

418 }

419

420 -static int cmd_log_walk(struct rev_info *rev)

420 +static int cmd_log_walk_no_free(struct rev_info *rev)

421 {

422 struct commit *commit;

423 int saved_nrl = 0;

@@ -444,7 +444,6 @@ static int cmd_log_walk(struct rev_info *rev)

444 * and HAS_CHANGES being accumulated in rev->diffopt, so be careful to

445 * retain that state information if replacing rev->diffopt in this loop

446 */

447 - rev->diffopt.no_free = 1;

447 while ((commit = get_revision(rev)) != NULL) {

448 if (!log_tree_commit(rev, commit) && rev->max_count >= 0)

449 /*

@@ -469,8 +468,6 @@ static int cmd_log_walk(struct rev_info *rev)

468 }

469 rev->diffopt.degraded_cc_to_c = saved_dcctc;

470 rev->diffopt.needed_rename_limit = saved_nrl;

472 - rev->diffopt.no_free = 0;

473 - diff_free(&rev->diffopt);

471

472 if (rev->remerge_diff) {

473 tmp_objdir_destroy(rev->remerge_objdir);

@@ -484,6 +481,17 @@ static int cmd_log_walk(struct rev_info *rev)

481 return diff_result_code(&rev->diffopt, 0);

482 }

483

484 +static int cmd_log_walk(struct rev_info *rev)

485 +{

486 + int retval;

487 +

488 + rev->diffopt.no_free = 1;

489 + retval = cmd_log_walk_no_free(rev);

490 + rev->diffopt.no_free = 0;

491 + diff_free(&rev->diffopt);

492 + return retval;

493 +}

494 +

495 static int git_log_config(const char *var, const char *value, void *cb)

496 {

497 const char *slot_name;

@@ -680,6 +688,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)

688

689 count = rev.pending.nr;

690 objects = rev.pending.objects;

691 + rev.diffopt.no_free = 1;

692 for (i = 0; i < count && !ret; i++) {

693 struct object *o = objects[i].item;

694 const char *name = objects[i].name;

@@ -725,12 +734,16 @@ int cmd_show(int argc, const char **argv, const char *prefix)

734 rev.pending.nr = rev.pending.alloc = 0;

735 rev.pending.objects = NULL;

736 add_object_array(o, name, &rev.pending);

728 - ret = cmd_log_walk(&rev);

737 + ret = cmd_log_walk_no_free(&rev);

738 break;

739 default:

740 ret = error(_("unknown type: %d"), o->type);

741 }

742 }

743 +

744 + rev.diffopt.no_free = 0;

745 + diff_free(&rev.diffopt);

746 +

747 free(objects);

748 return ret;

749 }

t/t4013-diff-various.sh

+19

@@ -542,6 +542,25 @@ test_expect_success 'diff-tree --stdin with log formatting' '

542 test_cmp expect actual

543 '

544

545 +test_expect_success 'show A B ... -- <pathspec>' '

546 + # side touches dir/sub, file0, and file3

547 + # master^ touches dir/sub, and file1

548 + # master^^ touches dir/sub, file0, and file2

549 + git show --name-only --format="<%s>" side master^ master^^ -- dir >actual &&

550 + cat >expect <<-\EOF &&

551 + <Side>

552 +

553 + dir/sub

554 + <Third>

555 +

556 + dir/sub

557 + <Second>

558 +

559 + dir/sub

560 + EOF

561 + test_cmp expect actual

562 +'

563 +

564 test_expect_success 'diff -I<regex>: setup' '

565 git checkout master &&

566 test_seq 50 >file0 &&