From: Michael Montalbo <mmontalbo@gmail.com>
Date: Wed, 04 Mar 2026 22:36:30 GMT
Subject: Re: [PATCH v2 2/2] log: reject pickaxe options when combined with -L
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:
>
> "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.