line-log: fix -L with pickaxe options

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

From: Michael Montalbo <[email protected]>

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.

---
 line-log.c          |  8 +++++++-
 t/t4211-line-log.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)

index 8bd422148d..8a404f5c22 100644
@@ -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);

gitgitgadget

Shouldn't that be solved not by omitting the necessary call to diffcore_std(), but by using the "--pickaxe-all" option?

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.

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.

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

Thank you for the review.

From: Michael Montalbo <[email protected]>

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.

---
 builtin/log.c       |  4 ++++
 t/t4211-line-log.sh | 52 ++++++++-------------------------------------
 2 files changed, 13 insertions(+), 43 deletions(-)

index 5c9a8ef363..44e2399d59 100644
@@ -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);
 

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

From: Michael Montalbo <[email protected]>

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.

---
 line-log.c          |  8 +++++++-
 t/t4211-line-log.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)

index 8bd422148d..8a404f5c22 100644
@@ -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);

gitgitgadget

From: Michael Montalbo <[email protected]>

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.

---
 builtin/log.c       |  4 ++++
 t/t4211-line-log.sh | 52 ++++++++-------------------------------------
 2 files changed, 13 insertions(+), 43 deletions(-)

index 5c9a8ef363..44e2399d59 100644
@@ -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);
 

gitgitgadget

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.