Use correct line from parent on recursive blame on blob #1370

krobelus · 2025-03-21T00:54:01Z

Running

tig blame +28 tig-2.5.12 -- src/diff.c

and pressing "b" should keep the line containing "const char *diff_argv[]
= }" selected. It actually moves the selection somewhere else.

This regressed in ca0809d (Fix cursor position after "Move to parent"
in blame view, 2019-11-29).

Commit ca0809d was only about "," (":back" or ":parent"). When we press
"b" (":view-blame"), we want to use the old behavior (originally added in
ba7c7d3 (Use file and line number information when loading blame for commit,
2009-02-07)). Do that.

(Note that the behavior fixed by ca0809d regressed in 2280734 (Enable
textconv in the blame view and fix blame -L (#1190), 2022-06-03). I have
not yet figured out what's going on, but this patch shouldn't make that worse.)

Fixes #1369

ilyagr · 2025-03-21T01:23:30Z

Thank you very much! I haven't looked into the details at this point, but I did compile this, and it does seem to solve the problem and work well for me so far.

Adding a test for this also seems very nice.

Note that the behavior fixed by ca0809d regressed in 2280734

Interesting. It'd make sense to me if the behavior of , in 3.5.12 was buggy; it seem sub-optimal in terms of the line it goes to but I didn't think about it long enough to be sure it can be improved.

ilyagr · 2025-03-21T23:13:57Z

I should move this to a separate bug report, but here are a couple of debugging nuggets about the change of the behavior of , in 2280734.

The reproduction of the bug is exactly the same as #1369, except you press , instead of b.

The line

tig/src/blame.c

Line 337 in 2280734

view->pos.lineno = parent_lineno ? parent_lineno - 1 : 0;

never gets executed. It does get executed in the parent commit.
Even though the command in

tig/src/blame.c

Line 311 in 2280734

"git", "diff", encoding_arg, "--no-ext-diff",

changed slightly in that commit, its output seems to remain exactly the same.

In other words, removing --no-textconv in git diff --encoding=UTF-8 --no-textconv --no-ext-diff --no-color -U0 ae8829311b62c1432e2d0b5118804a5b9c629c4a:src/diff.c a1894d17695db1f1b15a296b2423909c3d9e7430:src/diff.c -- makes no difference.

So, the difference is probably caused by a different input to the setup_blame_parent_line function. Its code is effectively unchanged, but its outcome differs. Most likely the result of parsing the blame output (which changed significantly) differs from before.

ilyagr · 2025-03-22T03:15:45Z

After a bit more debugging, I think the fix might be:

diff --git a/src/blame.c b/src/blame.c
index dc2888f8..56278952 100644
--- a/src/blame.c
+++ b/src/blame.c
@@ -332,7 +332,7 @@ setup_blame_parent_line(struct view *view, struct blame *blame)
 				blamed_lineno = atoi(pos + 1);
 
 		} else if (*line == '+' && parent_lineno != -1) {
-			if (blame->lineno == blamed_lineno &&
+			if (blame->lineno == blamed_lineno - 1 &&
 			    !strcmp(blame->text, line + 1)) {
 				view->pos.lineno = parent_lineno ? parent_lineno - 1 : 0;
 				break;

The main question is whether it's better to do this or to shift all the blame->linenos by 1. I haven't gotten to the bottom about why the shift in line numbers happened, whether it was intentional, and whether it affects other parts of code.

Update: The only other place where blame->lineno would be used is this PR. So, the patch above is probably fine.

Update: #1372

@krobelus

@krobelus observed in jonas#1370 that this behavior, fixed in ca0809d, regressed again in 2280734. This bug is similar to, but not identical with, jonas#1369. The easiest way to observe the bug is to run `tig blame +28 tig-2.5.12 -- src/diff.c` and press `,`. The problem is that the view stays on line 28 instead of jumping to the corresponding line.

ilyagr · 2025-03-22T04:04:45Z

test/blame/b-key-navigation-test

+	:save-display recursive-blame.screen
+'
+
+test_tig blame +27 project/Build.scala


These aren't really problems, but I copied this test to make a test for the , key bug in #1372, and we could coordinate the two tests.

If you use line 36 here, it works as an example for both , and b keys

You could rename the file to something like navigation-b-key-test, so that the two tests get placed next to each other. (This seems slightly simpler than combining them into one test).

ilyagr · 2025-03-22T04:07:40Z

test/blame/b-key-navigation-test

+
+test_tig blame +27 project/Build.scala
+
+# This shows that after pressing 'b' we should be positioned the line starting


Also not really a problem, but this confused me at first. It'd help if you at least added the expected line number and the fact that it's shown in the status bar.

Also, there's a typo, "on" is missing after "positioned".

Here's the comment I've come up with for the corresponding spot in #1372:

This shows that after pressing ',' we should be positioned on line 40 (starting with "clean in common"), as shown in the status bar. We should not stay on line 36.

ilyagr · 2025-03-22T04:33:49Z

src/blame.c

Very minor: the commit (and PR) title could say "...from ancestor" instead of "...from parent" to be clearer and less confusable with #1372.

Running tig blame +28 tig-2.5.12 -- src/diff.c and pressing "b" should keep the line containing "const char *diff_argv[] = }" selected. It actually moves the selection somewhere else. This regressed in ca0809d (Fix cursor position after "Move to parent" in blame view, 2019-11-29). That commit only meant to change "," (":back" or ":parent"). When we press "b" (":view-blame"), we want to use the original behavior from ba7c7d3 (Use file and line number information when loading blame for commit, 2009-02-07). Do that. (Note that the behavior fixed by ca0809d regressed in 2280734 (Enable textconv in the blame view and fix blame -L (jonas#1190), 2022-06-03). I have not yet figured out what's going on, but this patch shouldn't make that worse.) Fixes jonas#1369

krobelus mentioned this pull request Mar 21, 2025

blame: The b key in blame view does not go to the correct line since ca0809dbd #1369

Closed

ilyagr mentioned this pull request Mar 21, 2025

FR: jj file annotate should be able to report the line numbers in the original commit to enable nice TUI functionality jj-vcs/jj#6103

Closed

ilyagr mentioned this pull request Mar 22, 2025

Use correct line when using :parent in blame view #1372

Merged

ilyagr reviewed Mar 22, 2025

View reviewed changes

krobelus force-pushed the fix-recursive-blame-lineno branch from 7312d07 to a9f389b Compare March 22, 2025 11:03

krobelus force-pushed the fix-recursive-blame-lineno branch from a9f389b to 5dcfc9c Compare March 22, 2025 11:04

ilyagr mentioned this pull request Apr 3, 2025

Open diff view from blame at the correct line #1375

Merged

koutcher merged commit 9909d55 into jonas:master Apr 11, 2025
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use correct line from parent on recursive blame on blob #1370

Use correct line from parent on recursive blame on blob #1370

Uh oh!

krobelus commented Mar 21, 2025

Uh oh!

ilyagr commented Mar 21, 2025 •

edited

Loading

Uh oh!

ilyagr commented Mar 21, 2025 •

edited

Loading

Uh oh!

ilyagr commented Mar 22, 2025 •

edited

Loading

Uh oh!

ilyagr Mar 22, 2025

Uh oh!

ilyagr Mar 22, 2025

Uh oh!

ilyagr Mar 22, 2025

Uh oh!

Uh oh!

Uh oh!


		test_tig blame +27 project/Build.scala

		# This shows that after pressing 'b' we should be positioned the line starting

Use correct line from parent on recursive blame on blob #1370

Use correct line from parent on recursive blame on blob #1370

Uh oh!

Conversation

krobelus commented Mar 21, 2025

Uh oh!

ilyagr commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilyagr commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilyagr commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilyagr Mar 22, 2025

Choose a reason for hiding this comment

Uh oh!

ilyagr Mar 22, 2025

Choose a reason for hiding this comment

Uh oh!

ilyagr Mar 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ilyagr commented Mar 21, 2025 •

edited

Loading

ilyagr commented Mar 21, 2025 •

edited

Loading

ilyagr commented Mar 22, 2025 •

edited

Loading