Re: [PATCH 1/2] line-log: fix crash when combined with pickaxe options

From: Michael Montalbo <mmontalbo@gmail.com>
Date: Mar 4, 2026, 22:33 UTC
Message-ID: <CAC2QwmKh1DFXfDVKDv1xdj7-AqswgEPSDDXcgTn6dLLgQ9ALKw@mail.gmail.com>
In-Reply-To: <xmqqh5qv74a9.fsf@gitster.g>

On Wed, Mar 4, 2026 at 12:01 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

>
> "Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Michael Montalbo <mmontalbo@gmail.com>
> >
> > queue_diffs() calls diffcore_std() to detect renames so that line-level
> > history can follow files across renames.  When pickaxe options are
> > present on the command line (-G and -S to filter by text pattern,
> > --find-object to filter by object identity), diffcore_std() also runs
> > diffcore_pickaxe(), which may discard diff pairs that are relevant for
> > rename detection.  Losing those pairs breaks rename following.
>
> Shouldn't that be solved not by omitting the necessary call to
> diffcore_std(), but by using the "--pickaxe-all" option?
>

I looked into --pickaxe-all but my understanding is that it only preserves pairs when at least one pair matches the pattern. For a pure rename commit with no content change, I believe -G "pattern" would find zero matches, and even with --pickaxe-all the entire queue would still get discarded, losing the rename pair. Just in case I tested this to confirm and it still hits the same assertion failure. I could be wrong about my understanding of the intent though.

Show 13 quoted lines

> > Note that this only fixes the crash.  The -G, -S, and --find-object
> > options still have no effect on -L output because line-log uses its
> > own commit-filtering logic that bypasses the normal pickaxe pipeline.
>
> I do not know exactly what -L really wants to do, but from the look
> at a patch like this, it smells like it is abusing the diffcore
> machinery.  If it wants to follow the rename history for individual
> paths, even if the end-user's top-level command line option included
> pickaxe or other fancy diffcore options, should it be *reusing* the
> diff_options struct, prepared from the end-user request?  Shouldn't
> it rather be using its own diffopt crafted for that rename tracking
> purpose, I have to wonder.
>

Yes I think that makes more sense. I can update v3 to follow the pattern in blame.c::find_rename(), building a private diff_options inside queue_diffs().

> Thanks.

Thank you for the review.