Skip to content

Commit 84132b1

Browse files
committed
line-log: fix crash when combined with pickaxe options
queue_diffs() passes the caller's diff_options—which may carry user-specified pickaxe state—to diff_tree_oid() and diffcore_std() when detecting renames for line-level history tracking. 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 a2bb801 (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() so that filter_diffs_for_paths(), which previously always discarded stray deletion pairs from the diff queue, now only runs when diff_might_be_rename() signals a potential rename. When pickaxe breaks rename following at one commit, a later commit may produce a deletion pair that is no longer cleaned up, reaching process_diff_filepair() with an invalid filespec and triggering an assertion failure. Fix this by building a private diff_options for the rename-detection path inside queue_diffs(), following the same pattern used by blame's find_rename(). This isolates the rename machinery from unrelated user-specified options, and calls diffcore_rename() directly since 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>
1 parent 67ad421 commit 84132b1

File tree

2 files changed

+66
-4
lines changed

2 files changed

+66
-4
lines changed

line-log.c

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -858,15 +858,28 @@ static void queue_diffs(struct line_log_data *range,
858858
diff_queue_clear(&diff_queued_diff);
859859
diff_tree_oid(parent_tree_oid, tree_oid, "", opt);
860860
if (opt->detect_rename && diff_might_be_rename()) {
861+
struct diff_options rename_opts;
862+
863+
/*
864+
* Build a private diff_options for rename detection so
865+
* that any user-specified options on the original opts
866+
* (e.g. pickaxe) cannot discard diff pairs needed for
867+
* rename tracking. Similar to blame's find_rename().
868+
*/
869+
repo_diff_setup(opt->repo, &rename_opts);
870+
rename_opts.flags.recursive = 1;
871+
rename_opts.detect_rename = opt->detect_rename;
872+
rename_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
873+
diff_setup_done(&rename_opts);
874+
861875
/* must look at the full tree diff to detect renames */
862-
clear_pathspec(&opt->pathspec);
863876
diff_queue_clear(&diff_queued_diff);
864-
865-
diff_tree_oid(parent_tree_oid, tree_oid, "", opt);
877+
diff_tree_oid(parent_tree_oid, tree_oid, "", &rename_opts);
866878

867879
filter_diffs_for_paths(range, 1);
868-
diffcore_std(opt);
880+
diffcore_rename(&rename_opts);
869881
filter_diffs_for_paths(range, 0);
882+
diff_free(&rename_opts);
870883
}
871884
move_diff_queue(queue, &diff_queued_diff);
872885
}

t/t4211-line-log.sh

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -367,4 +367,53 @@ test_expect_success 'show line-log with graph' '
367367
test_cmp expect actual
368368
'
369369

370+
test_expect_success 'setup for -L with -G/-S/--find-object and a merge with rename' '
371+
git checkout --orphan pickaxe-rename &&
372+
git reset --hard &&
373+
374+
echo content >file &&
375+
git add file &&
376+
git commit -m "add file" &&
377+
378+
git checkout -b pickaxe-rename-side &&
379+
git mv file renamed-file &&
380+
git commit -m "rename file" &&
381+
382+
git checkout pickaxe-rename &&
383+
git commit --allow-empty -m "diverge" &&
384+
git merge --no-edit pickaxe-rename-side &&
385+
386+
git mv renamed-file file &&
387+
git commit -m "rename back"
388+
'
389+
390+
test_expect_success '-L -G does not crash with merge and rename' '
391+
git log --format="%s" --no-patch -L 1,1:file -G "." >actual
392+
'
393+
394+
test_expect_success '-L -S does not crash with merge and rename' '
395+
git log --format="%s" --no-patch -L 1,1:file -S content >actual
396+
'
397+
398+
test_expect_success '-L --find-object does not crash with merge and rename' '
399+
git log --format="%s" --no-patch -L 1,1:file \
400+
--find-object=$(git rev-parse HEAD:file) >actual
401+
'
402+
403+
test_expect_failure '-L -G should filter commits by pattern' '
404+
git log --format="%s" --no-patch -L 1,1:file -G "nomatch" >actual &&
405+
test_must_be_empty actual
406+
'
407+
408+
test_expect_failure '-L -S should filter commits by pattern' '
409+
git log --format="%s" --no-patch -L 1,1:file -S "nomatch" >actual &&
410+
test_must_be_empty actual
411+
'
412+
413+
test_expect_failure '-L --find-object should filter commits by object' '
414+
git log --format="%s" --no-patch -L 1,1:file \
415+
--find-object=$ZERO_OID >actual &&
416+
test_must_be_empty actual
417+
'
418+
370419
test_done

0 commit comments

Comments
 (0)