From: Michael Montalbo Date: Wed, 04 Mar 2026 22:33:15 GMT Subject: Re: [PATCH 1/2] line-log: fix crash when combined with pickaxe options Message-ID: In-Reply-To: On Wed, Mar 4, 2026 at 12:01 PM Junio C Hamano wrote: > > "Michael Montalbo via GitGitGadget" writes: > > > From: Michael Montalbo > > > > 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. > > 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.