[PATCH 0/2] line-log: fix -L with pickaxe options

[PATCH 0/2] line-log: fix -L with pickaxe options

[Michael Montalbo via GitGitGadget]

This series fixes a crash in git log -L when combined with pickaxe options
(-G, -S, or --find-object) on a history involving renames, reported in [1].

The crash bisects to a2bb801f6a (line-log: avoid unnecessary full tree
diffs, 2019-08-21), which made the diffcore_std() call in queue_diffs()
unconditional. Before that commit, the same combination silently truncated
history at rename boundaries rather than crashing. The root cause is that
diffcore_std() runs diffcore_pickaxe(), which may discard diff pairs needed
for rename detection.

Patch 1 fixes the crash by calling diffcore_rename() directly instead of
diffcore_std(), and adds tests including known-breakage markers showing that
the pickaxe options are silently ignored by -L.

Patch 2 explicitly rejects the unsupported combination with die(), replacing
the known-breakage tests with rejection tests.

[1] https://lore.kernel.org/git/aac-QdjY1ohAqgw_@desktop/

Michael Montalbo (2):
  line-log: fix crash when combined with pickaxe options
  log: reject pickaxe options when combined with -L

 builtin/log.c       |  4 ++++
 line-log.c          |  8 +++++++-
 t/t4211-line-log.sh | 15 +++++++++++++++
 3 files changed, 26 insertions(+), 1 deletion(-)


base-commit: 67ad42147a7acc2af6074753ebd03d904476118f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2061%2Fmmontalbo%2Ffix-line-log-G-crash-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2061/mmontalbo/fix-line-log-G-crash-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2061
-- 
gitgitgadget

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

[Michael Montalbo via GitGitGadget]

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.

Before a2bb801f6a (line-log: avoid unnecessary full tree diffs,
2019-08-21), diffcore_std() was only invoked when a rename was already
suspected, so the pickaxe interference was unlikely in practice.  That
commit made the diffcore_std() call unconditional, and with
filter_diffs_for_paths() now framing that call, a queue pruned by
pickaxe violates filter_diffs_for_paths()'s expectation that diff
pairs correspond to tracked paths, triggering an assertion failure.

Fix this by calling diffcore_rename() directly instead of
diffcore_std().  The line-log machinery only needs rename detection
from this call site; the other stages run by diffcore_std() (pickaxe,
order, break/rewrite) are unnecessary here.

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.
Add tests that verify the crash is fixed and mark the silent-ignore
behavior as known breakage for all three options.

Reported-by: Matthew Hughes <matthewhughes934@gmail.com>
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
---
 line-log.c          |  8 +++++++-
 t/t4211-line-log.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/line-log.c b/line-log.c
index 8bd422148d..8a404f5c22 100644
--- a/line-log.c
+++ b/line-log.c
@@ -865,7 +865,13 @@ static void queue_diffs(struct line_log_data *range,
 		diff_tree_oid(parent_tree_oid, tree_oid, "", opt);
 
 		filter_diffs_for_paths(range, 1);
-		diffcore_std(opt);
+		/*
+		 * Call diffcore_rename() directly, as only rename
+		 * detection is needed.  diffcore_std() would also run
+		 * pickaxe, which may discard pairs needed for rename
+		 * detection and break rename following.
+		 */
+		diffcore_rename(opt);
 		filter_diffs_for_paths(range, 0);
 	}
 	move_diff_queue(queue, &diff_queued_diff);
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 0a7c3ca42f..7acc38f72d 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -367,4 +367,53 @@ test_expect_success 'show line-log with graph' '
 	test_cmp expect actual
 '
 
+test_expect_success 'setup for -L with -G/-S/--find-object and a merge with rename' '
+	git checkout --orphan pickaxe-rename &&
+	git reset --hard &&
+
+	echo content >file &&
+	git add file &&
+	git commit -m "add file" &&
+
+	git checkout -b pickaxe-rename-side &&
+	git mv file renamed-file &&
+	git commit -m "rename file" &&
+
+	git checkout pickaxe-rename &&
+	git commit --allow-empty -m "diverge" &&
+	git merge --no-edit pickaxe-rename-side &&
+
+	git mv renamed-file file &&
+	git commit -m "rename back"
+'
+
+test_expect_success '-L -G does not crash with merge and rename' '
+	git log --format="%s" --no-patch -L 1,1:file -G "." >actual
+'
+
+test_expect_success '-L -S does not crash with merge and rename' '
+	git log --format="%s" --no-patch -L 1,1:file -S content >actual
+'
+
+test_expect_success '-L --find-object does not crash with merge and rename' '
+	git log --format="%s" --no-patch -L 1,1:file \
+		--find-object=$(git rev-parse HEAD:file) >actual
+'
+
+test_expect_failure '-L -G should filter commits by pattern' '
+	git log --format="%s" --no-patch -L 1,1:file -G "nomatch" >actual &&
+	test_must_be_empty actual
+'
+
+test_expect_failure '-L -S should filter commits by pattern' '
+	git log --format="%s" --no-patch -L 1,1:file -S "nomatch" >actual &&
+	test_must_be_empty actual
+'
+
+test_expect_failure '-L --find-object should filter commits by object' '
+	git log --format="%s" --no-patch -L 1,1:file \
+		--find-object=$ZERO_OID >actual &&
+	test_must_be_empty actual
+'
+
 test_done
-- 
gitgitgadget

[PATCH 2/2] log: reject pickaxe options when combined with -L

[Michael Montalbo via GitGitGadget]

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"));
+
 	memset(&w, 0, sizeof(w));
 	userformat_find_requirements(NULL, &w);
 
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 7acc38f72d..8ebc73d2d9 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -367,53 +367,19 @@ test_expect_success 'show line-log with graph' '
 	test_cmp expect actual
 '
 
-test_expect_success 'setup for -L with -G/-S/--find-object and a merge with rename' '
-	git checkout --orphan pickaxe-rename &&
-	git reset --hard &&
-
-	echo content >file &&
-	git add file &&
-	git commit -m "add file" &&
-
-	git checkout -b pickaxe-rename-side &&
-	git mv file renamed-file &&
-	git commit -m "rename file" &&
-
-	git checkout pickaxe-rename &&
-	git commit --allow-empty -m "diverge" &&
-	git merge --no-edit pickaxe-rename-side &&
-
-	git mv renamed-file file &&
-	git commit -m "rename back"
-'
-
-test_expect_success '-L -G does not crash with merge and rename' '
-	git log --format="%s" --no-patch -L 1,1:file -G "." >actual
-'
-
-test_expect_success '-L -S does not crash with merge and rename' '
-	git log --format="%s" --no-patch -L 1,1:file -S content >actual
-'
-
-test_expect_success '-L --find-object does not crash with merge and rename' '
-	git log --format="%s" --no-patch -L 1,1:file \
-		--find-object=$(git rev-parse HEAD:file) >actual
-'
-
-test_expect_failure '-L -G should filter commits by pattern' '
-	git log --format="%s" --no-patch -L 1,1:file -G "nomatch" >actual &&
-	test_must_be_empty actual
+test_expect_success '-L with -G is rejected' '
+	test_must_fail git log -L 1,1:a.c -G "pattern" 2>err &&
+	test_grep "does not yet support" err
 '
 
-test_expect_failure '-L -S should filter commits by pattern' '
-	git log --format="%s" --no-patch -L 1,1:file -S "nomatch" >actual &&
-	test_must_be_empty actual
+test_expect_success '-L with -S is rejected' '
+	test_must_fail git log -L 1,1:a.c -S "pattern" 2>err &&
+	test_grep "does not yet support" err
 '
 
-test_expect_failure '-L --find-object should filter commits by object' '
-	git log --format="%s" --no-patch -L 1,1:file \
-		--find-object=$ZERO_OID >actual &&
-	test_must_be_empty actual
+test_expect_success '-L with --find-object is rejected' '
+	test_must_fail git log -L 1,1:a.c --find-object=HEAD 2>err &&
+	test_grep "does not yet support" err
 '
 
 test_done
-- 
gitgitgadget

[PATCH v2 0/2] line-log: fix -L with pickaxe options

[Michael Montalbo via GitGitGadget]

This series fixes a crash in git log -L when combined with pickaxe options
(-G, -S, or --find-object) on a history involving renames, reported in [1].

The crash bisects to a2bb801f6a (line-log: avoid unnecessary full tree
diffs, 2019-08-21). Before that commit, the same combination silently
truncated history at rename boundaries rather than crashing. The root cause
is that diffcore_std() runs diffcore_pickaxe(), which may discard diff pairs
needed for rename detection.

Patch 1 fixes the crash by calling diffcore_rename() directly instead of
diffcore_std(), and adds tests including known-breakage markers showing that
the pickaxe options are silently ignored by -L.

Patch 2 explicitly rejects the unsupported combination with die(), replacing
the known-breakage tests with rejection tests.

[1] https://lore.kernel.org/git/aac-QdjY1ohAqgw_@desktop/

Michael Montalbo (2):
  line-log: fix crash when combined with pickaxe options
  log: reject pickaxe options when combined with -L

 builtin/log.c       |  4 ++++
 line-log.c          |  8 +++++++-
 t/t4211-line-log.sh | 15 +++++++++++++++
 3 files changed, 26 insertions(+), 1 deletion(-)


base-commit: 67ad42147a7acc2af6074753ebd03d904476118f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2061%2Fmmontalbo%2Ffix-line-log-G-crash-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2061/mmontalbo/fix-line-log-G-crash-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2061

Range-diff vs v1:

 1:  6e97d88993 ! 1:  273ebf640d line-log: fix crash when combined with pickaxe options
     @@ Commit message
          Before a2bb801f6a (line-log: avoid unnecessary full tree diffs,
          2019-08-21), diffcore_std() was only invoked when a rename was already
          suspected, so the pickaxe interference was unlikely in practice.  That
     -    commit made the diffcore_std() call unconditional, and with
     -    filter_diffs_for_paths() now framing that call, a queue pruned by
     -    pickaxe violates filter_diffs_for_paths()'s expectation that diff
     -    pairs correspond to tracked paths, triggering an assertion failure.
     +    commit restructured queue_diffs() to gate both diffcore_std() and the
     +    surrounding filter_diffs_for_paths() calls behind
     +    diff_might_be_rename().  When pickaxe breaks rename following at one
     +    commit, a later commit may produce a deletion pair that bypasses this
     +    gate entirely, reaching process_diff_filepair() with an invalid
     +    filespec and triggering an assertion failure.
      
          Fix this by calling diffcore_rename() directly instead of
          diffcore_std().  The line-log machinery only needs rename detection
 2:  ae5269af0b = 2:  81cb521401 log: reject pickaxe options when combined with -L

-- 
gitgitgadget

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

[Michael Montalbo via GitGitGadget]

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.

Before a2bb801f6a (line-log: avoid unnecessary full tree diffs,
2019-08-21), diffcore_std() was only invoked when a rename was already
suspected, so the pickaxe interference was unlikely in practice.  That
commit restructured queue_diffs() to gate both diffcore_std() and the
surrounding filter_diffs_for_paths() calls behind
diff_might_be_rename().  When pickaxe breaks rename following at one
commit, a later commit may produce a deletion pair that bypasses this
gate entirely, reaching process_diff_filepair() with an invalid
filespec and triggering an assertion failure.

Fix this by calling diffcore_rename() directly instead of
diffcore_std().  The line-log machinery only needs rename detection
from this call site; the other stages run by diffcore_std() (pickaxe,
order, break/rewrite) are unnecessary here.

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.
Add tests that verify the crash is fixed and mark the silent-ignore
behavior as known breakage for all three options.

Reported-by: Matthew Hughes <matthewhughes934@gmail.com>
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
---
 line-log.c          |  8 +++++++-
 t/t4211-line-log.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/line-log.c b/line-log.c
index 8bd422148d..8a404f5c22 100644
--- a/line-log.c
+++ b/line-log.c
@@ -865,7 +865,13 @@ static void queue_diffs(struct line_log_data *range,
 		diff_tree_oid(parent_tree_oid, tree_oid, "", opt);
 
 		filter_diffs_for_paths(range, 1);
-		diffcore_std(opt);
+		/*
+		 * Call diffcore_rename() directly, as only rename
+		 * detection is needed.  diffcore_std() would also run
+		 * pickaxe, which may discard pairs needed for rename
+		 * detection and break rename following.
+		 */
+		diffcore_rename(opt);
 		filter_diffs_for_paths(range, 0);
 	}
 	move_diff_queue(queue, &diff_queued_diff);
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 0a7c3ca42f..7acc38f72d 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -367,4 +367,53 @@ test_expect_success 'show line-log with graph' '
 	test_cmp expect actual
 '
 
+test_expect_success 'setup for -L with -G/-S/--find-object and a merge with rename' '
+	git checkout --orphan pickaxe-rename &&
+	git reset --hard &&
+
+	echo content >file &&
+	git add file &&
+	git commit -m "add file" &&
+
+	git checkout -b pickaxe-rename-side &&
+	git mv file renamed-file &&
+	git commit -m "rename file" &&
+
+	git checkout pickaxe-rename &&
+	git commit --allow-empty -m "diverge" &&
+	git merge --no-edit pickaxe-rename-side &&
+
+	git mv renamed-file file &&
+	git commit -m "rename back"
+'
+
+test_expect_success '-L -G does not crash with merge and rename' '
+	git log --format="%s" --no-patch -L 1,1:file -G "." >actual
+'
+
+test_expect_success '-L -S does not crash with merge and rename' '
+	git log --format="%s" --no-patch -L 1,1:file -S content >actual
+'
+
+test_expect_success '-L --find-object does not crash with merge and rename' '
+	git log --format="%s" --no-patch -L 1,1:file \
+		--find-object=$(git rev-parse HEAD:file) >actual
+'
+
+test_expect_failure '-L -G should filter commits by pattern' '
+	git log --format="%s" --no-patch -L 1,1:file -G "nomatch" >actual &&
+	test_must_be_empty actual
+'
+
+test_expect_failure '-L -S should filter commits by pattern' '
+	git log --format="%s" --no-patch -L 1,1:file -S "nomatch" >actual &&
+	test_must_be_empty actual
+'
+
+test_expect_failure '-L --find-object should filter commits by object' '
+	git log --format="%s" --no-patch -L 1,1:file \
+		--find-object=$ZERO_OID >actual &&
+	test_must_be_empty actual
+'
+
 test_done
-- 
gitgitgadget

[PATCH v2 2/2] log: reject pickaxe options when combined with -L

[Michael Montalbo via GitGitGadget]

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"));
+
 	memset(&w, 0, sizeof(w));
 	userformat_find_requirements(NULL, &w);
 
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 7acc38f72d..8ebc73d2d9 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -367,53 +367,19 @@ test_expect_success 'show line-log with graph' '
 	test_cmp expect actual
 '
 
-test_expect_success 'setup for -L with -G/-S/--find-object and a merge with rename' '
-	git checkout --orphan pickaxe-rename &&
-	git reset --hard &&
-
-	echo content >file &&
-	git add file &&
-	git commit -m "add file" &&
-
-	git checkout -b pickaxe-rename-side &&
-	git mv file renamed-file &&
-	git commit -m "rename file" &&
-
-	git checkout pickaxe-rename &&
-	git commit --allow-empty -m "diverge" &&
-	git merge --no-edit pickaxe-rename-side &&
-
-	git mv renamed-file file &&
-	git commit -m "rename back"
-'
-
-test_expect_success '-L -G does not crash with merge and rename' '
-	git log --format="%s" --no-patch -L 1,1:file -G "." >actual
-'
-
-test_expect_success '-L -S does not crash with merge and rename' '
-	git log --format="%s" --no-patch -L 1,1:file -S content >actual
-'
-
-test_expect_success '-L --find-object does not crash with merge and rename' '
-	git log --format="%s" --no-patch -L 1,1:file \
-		--find-object=$(git rev-parse HEAD:file) >actual
-'
-
-test_expect_failure '-L -G should filter commits by pattern' '
-	git log --format="%s" --no-patch -L 1,1:file -G "nomatch" >actual &&
-	test_must_be_empty actual
+test_expect_success '-L with -G is rejected' '
+	test_must_fail git log -L 1,1:a.c -G "pattern" 2>err &&
+	test_grep "does not yet support" err
 '
 
-test_expect_failure '-L -S should filter commits by pattern' '
-	git log --format="%s" --no-patch -L 1,1:file -S "nomatch" >actual &&
-	test_must_be_empty actual
+test_expect_success '-L with -S is rejected' '
+	test_must_fail git log -L 1,1:a.c -S "pattern" 2>err &&
+	test_grep "does not yet support" err
 '
 
-test_expect_failure '-L --find-object should filter commits by object' '
-	git log --format="%s" --no-patch -L 1,1:file \
-		--find-object=$ZERO_OID >actual &&
-	test_must_be_empty actual
+test_expect_success '-L with --find-object is rejected' '
+	test_must_fail git log -L 1,1:a.c --find-object=HEAD 2>err &&
+	test_grep "does not yet support" err
 '
 
 test_done
-- 
gitgitgadget

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

[Junio C Hamano]

"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?

> 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.

Thanks.

Re: [PATCH v2 2/2] log: reject pickaxe options when combined with -L

[Junio C Hamano]

"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]?

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

[Michael Montalbo]

On Wed, Mar 4, 2026 at 12:01 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "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.

> > 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.

Re: [PATCH v2 2/2] log: reject pickaxe options when combined with -L

[Michael Montalbo]

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.