Re: [BUG?] Major performance issue with some commands on our repo's master branch
On Wed, Jun 08, 2022 at 09:27:43PM -0400, Kyle Meyer wrote:
Show 14 quoted lines
> > 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@mail.gmail.com
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
Jeff King <peff@peff.net>