On Wed, Jun 08, 2022 at 09:27:43PM -0400, Kyle Meyer wrote: > > I think the code could be written to realize that none of those features > > are in use, and disable the diff entirely in favor of checking whether > > the two trees has the same object id. That would yield _mostly_ the same > > behavior, though there are probably corner cases (e.g., a tree with an > > odd mode entry, say, may get parsed so as to produce an empty diff, even > > though it's not byte for byte identical). That may be an acceptable > > tradeoff. But I think the code would be a bit brittle (it needs to know > > about all the cases where a diff might matter, and we may add more > > later). > > Do you think it'd be safe to make --no-patch imply --diff-merges=off, as > Tao suggested elsewhere in this thread? > > https://lore.kernel.org/git/CAPMMpog-7eDOrgSU9GjV4G9rk5RkL-PJhaUAO3_0p2YxfRf=LA@xxxxxxxxxxxxxx I'm not sure. If I do: diff --git a/diff-merges.c b/diff-merges.c index 7f64156b8b..f05a585dfb 100644 --- a/diff-merges.c +++ b/diff-merges.c @@ -164,6 +164,9 @@ void diff_merges_set_dense_combined_if_unset(struct rev_info *revs) void diff_merges_setup_revs(struct rev_info *revs) { + if (revs->diffopt.output_format == DIFF_FORMAT_NO_OUTPUT && + !revs->explicit_diff_merges) + diff_merges_suppress(revs); if (revs->combine_merges == 0) revs->dense_combined_merges = 0; if (revs->separate_merges == 0) then the test suite passes, but that may just be because we are not invoking the right corner case. It does change the output with something like: git show --diff-filter=D -s a6434bc6f7a1 Without the patch above, it always shows the commit. With it, it shows nothing. That's a bit far-fetched, but it is a regression, and I'm also not sure if it's just the tip of the iceberg. It also doesn't solve problem completely. Regular commits can have expensive diffs, too. I think you'd do better to have a mode specific to git-show that skips the diff if we're not showing it, but makes sure to always show the commit anyway. Perhaps something like the hunk above, but put into cmd_show(), and then setting revs->always_show_header. But it would require somebody verifying that this does the right thing in all cases. -Peff
