Re: [PATCH v2 2/2] log: reject pickaxe options when combined with -L
- From
Michael Montalbo <mmontalbo@gmail.com>
- Date
- Mar 4, 2026, 22:36 UTC
- Message-ID
- <CAC2Qwm+2pjMk=XFq0cU0Pt1tWkqDy_tOKMtP0jF6JArFX0jmOg@mail.gmail.com>
- In-Reply-To
- <xmqq4imv71g8.fsf@gitster.g>
On Wed, Mar 4, 2026 at 1:02 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 39 quoted lines
>
> "Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Michael Montalbo <mmontalbo@gmail.com>
> >
> > The previous commit fixed a crash when -G, -S, or --find-object was
> > used together with -L and rename detection. However, these options
> > still have no effect on -L output: line-log uses its own
> > commit-filtering logic in line_log_filter() and never consults the
> > pickaxe machinery. Rather than silently ignoring these options, reject
> > the combination with a clear error message.
> >
> > This replaces the known-breakage tests from the previous commit with
> > tests that verify the rejection for all three options. A future series
> > could teach line-log to honor these options and remove this restriction.
> >
> > Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
> > ---
> > builtin/log.c | 4 ++++
> > t/t4211-line-log.sh | 52 ++++++++-------------------------------------
> > 2 files changed, 13 insertions(+), 43 deletions(-)
> >
> > diff --git a/builtin/log.c b/builtin/log.c
> > index 5c9a8ef363..44e2399d59 100644
> > --- a/builtin/log.c
> > +++ b/builtin/log.c
> > @@ -317,6 +317,10 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
> > if (rev->line_level_traverse && rev->prune_data.nr)
> > die(_("-L<range>:<file> cannot be used with pathspec"));
> >
> > + if (rev->line_level_traverse &&
> > + (rev->diffopt.pickaxe_opts & DIFF_PICKAXE_KINDS_MASK))
> > + die(_("-L does not yet support -G, -S, or --find-object"));
>
> I do not think "-L" meant to work well with these features to begin
> with, and I've never used -L with any other options (-L does not
> even work with --stat), so I personally do not mind this change.
>
> But if this is in place, would we still need [1/2]?I went back and forth on whether to keep [1/2]. My main reason for keeping it was as future-proofing if someone removes the die() to implement support for these features working together.
However, I can easily see the argument that whoever does that work would likely rework queue_diffs() anyway and it's simpler to drop it. Happy to do so if it's not worth the churn.