[PATCH] 2.36 show regression fix
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 30, 2022, 05:29 UTC
- Message-ID
- <xmqqo80j87g0.fsf_-_@gitster.g>
- In-Reply-To
- <xmqqzgk388tt.fsf@gitster.g>
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 <gitster@pobox.com> writes:
Show 20 quoted lines
> Daniel Li <dan@danielyli.com> writes: > >> git version: 2.36.0 >> OS: macOS Monterey 12.2.1 >> Installed via: homebrew > > I think this is the same regression as the recently talked about > "diff-tree --stdin" aka "gitk" regression. > > https://lore.kernel.org/git/xmqq7d7bsu2n.fsf@gitster.g/ > > e900d494 (diff: add an API for deferred freeing, 2021-02-11), broke > cmd_log_walk(), and we started to lose some setting that was parsed > from the command line and stored in the diff_options structure after > cmd_log_walk() runs just once. But "git show A B" runs the function > once for each commit. A recent change in 2.36.0 made it worse by > adding <pathspec> to the set of setting that gets lost after > cmd_log_walk() runs once. > > Thanks for a report.
builtin/log.c | 23 ++++++++++++++++++----- t/t4013-diff-various.sh | 19 +++++++++++++++++++ 2 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c index c211d66d1d..6696c4cfd0 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -417,7 +417,7 @@ static void finish_early_output(struct rev_info *rev) show_early_header(rev, "done", n); } -static int cmd_log_walk(struct rev_info *rev) +static int cmd_log_walk_no_free(struct rev_info *rev) { struct commit *commit; int saved_nrl = 0; @@ -444,7 +444,6 @@ static int cmd_log_walk(struct rev_info *rev) * and HAS_CHANGES being accumulated in rev->diffopt, so be careful to * retain that state information if replacing rev->diffopt in this loop */ - rev->diffopt.no_free = 1; while ((commit = get_revision(rev)) != NULL) { if (!log_tree_commit(rev, commit) && rev->max_count >= 0) /* @@ -469,8 +468,6 @@ static int cmd_log_walk(struct rev_info *rev) } rev->diffopt.degraded_cc_to_c = saved_dcctc; rev->diffopt.needed_rename_limit = saved_nrl; - rev->diffopt.no_free = 0; - diff_free(&rev->diffopt); if (rev->remerge_diff) { tmp_objdir_destroy(rev->remerge_objdir); @@ -484,6 +481,17 @@ static int cmd_log_walk(struct rev_info *rev) return diff_result_code(&rev->diffopt, 0); } +static int cmd_log_walk(struct rev_info *rev) +{ + int retval; + + rev->diffopt.no_free = 1; + retval = cmd_log_walk_no_free(rev); + rev->diffopt.no_free = 0; + diff_free(&rev->diffopt); + return retval; +} + static int git_log_config(const char *var, const char *value, void *cb) { const char *slot_name; @@ -680,6 +688,7 @@ int cmd_show(int argc, const char **argv, const char *prefix) count = rev.pending.nr; objects = rev.pending.objects; + rev.diffopt.no_free = 1; for (i = 0; i < count && !ret; i++) { struct object *o = objects[i].item; const char *name = objects[i].name; @@ -725,12 +734,16 @@ int cmd_show(int argc, const char **argv, const char *prefix) rev.pending.nr = rev.pending.alloc = 0; rev.pending.objects = NULL; add_object_array(o, name, &rev.pending); - ret = cmd_log_walk(&rev); + ret = cmd_log_walk_no_free(&rev); break; default: ret = error(_("unknown type: %d"), o->type); } } + + rev.diffopt.no_free = 0; + diff_free(&rev.diffopt); + free(objects); return ret; } diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh index 750aee17ea..7a44d5d595 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -542,6 +542,25 @@ test_expect_success 'diff-tree --stdin with log formatting' ' test_cmp expect actual ' +test_expect_success 'show A B ... -- <pathspec>' ' + # side touches dir/sub, file0, and file3 + # master^ touches dir/sub, and file1 + # master^^ touches dir/sub, file0, and file2 + git show --name-only --format="<%s>" side master^ master^^ -- dir >actual && + cat >expect <<-\EOF && + <Side> + + dir/sub + <Third> + + dir/sub + <Second> + + dir/sub + EOF + test_cmp expect actual +' + test_expect_success 'diff -I<regex>: setup' ' git checkout master && test_seq 50 >file0 &&
-- 2.36.0-256-g547811d5a1