Change the blame view to use porcelain #1190
Conversation
ee41e0c to
db4a6b4
Compare
NEWS.adoc
Outdated
| @@ -15,6 +15,7 @@ Bug fixes: | |||
| - Fix cursor behaviour during staging. (#842, #1028) | |||
| - Fix navigation in split tree view. | |||
| - Enable textconv in the stage view. | |||
| - Change the blame view to use porcelain. (#978, #1189) | |||
There was a problem hiding this comment.
I'm not sure if porcelain vs. incremental is relevant for users. I'd probably say something like
The blame view now honors the textconv option and blame flags like -L.
| { | ||
| if (!buf) { | ||
| const char *blame_argv[] = { | ||
| "git", "blame", encoding_arg, "%(blameargs)", "--incremental", |
There was a problem hiding this comment.
The implementation looks solid. I'm wondering though, is it really necessary to drop --incremental to support -L and --textconv?
Probably not a huge loss although incremental mode can be nice.
In theory git blame --incremental -C +123 could try to find the commit that introduced line 123 first, but that's not what happens today.
There was a problem hiding this comment.
Thanks for your review @krobelus. The --incremental and -p (--porcelain) outputs are actually much closer than their names let us presage. With --incremental, only the blame information is present, with --porcelain you have also the lines from the blamed file. The original 2-pass processing doesn't allow to support textconv as Git does not provide any facility to process a random file with textconv. Supporting -L would also be much more difficult.
Despite me breaking it once already, I completely overlooked the blame -C case. Hopefully this time, tig blame -C still works as before. It could be worth adding a test case for it one day.
There was a problem hiding this comment.
I think it's fine as-is but if anyone cares about blame annotations
appearing incrementally (maybe we should ask on the Git list), we could
keep the --incremental code path for cases when neither textconv nor -L
is used. Doesn't seem worth the complexity.
With a hypothetical --incremental --porcelain we could get both features but Git doesn't support that combination of options.
eba74f1 to
d0eb570
Compare
Change the blame view to use the porcelain output rather than loading the file and adding blame information based on the incremental output. Fixes jonas#978 Fixes jonas#1189 Reviewed-by: Johannes Altmanninger <aclopte@gmail.com>
|
I just realized this introduced a regression: we no longer jump to the blamed line To reproduce run and type |
|
Good catch, the goto_lineno has gone with the bathwater, I'll put it back. |
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 (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
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
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
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 (#1190), 2022-06-03). I have not yet figured out what's going on, but this patch shouldn't make that worse.) Fixes #1369
Fixes #978
Fixes #1189