Skip to content

Commit

Permalink
Update message in the blame view
Browse files Browse the repository at this point in the history 
* Show the commit the line came from in the title window.

* Show the commit where annotating starts in the status window.
  • Loading branch information
@koutcher
koutcher committed Mar 18, 2024
1 parent 5b03f69 commit b1cda8c
Show file tree
Hide file tree
Showing 6 changed files with 22 additions and 13 deletions.
13 changes: 10 additions & 3 deletions src/blame.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,10 @@ blame_read(struct view *view, struct buffer *buf, bool force_stop)
update_view_title(view);
redraw_view_from(view, 0);
}

if (view->env->ref[0])
update_status("Start annotating from %s", view->env->ref);

This comment has been minimized.

Sign in to view

Copy link
@ilyagr

ilyagr Mar 23, 2024

Thank you for working on this!

I think this specific message is, unfortunately, not very helpful as is. Perhaps it'd be more helpful if you mentioned the parent commit, e.g. "Start annotating from parent(s) of 395611"? Perhaps we can think of something else?

Here's why I think it is confusing. I did tig src/blame.c while on this wip-gh-1315 branch, and then pressed , on the first line. This results in a message "Start annotating from 0a7f...". However, the commit starting with 0a7f is nowhere to be seen on screen, nor was it on screen before I pressed ,. (It's actually the commit before 395611 that was selected before you pressed ,, but is nowhere on screen after you press ,. This makes sense, in a way, but I had to quit tig and reopen it with the log view to find it.).

A message like "Start annotating from parent(s) of 395611" still has the downside that once you see the message, that commit is no longer visible on screen, but it will be clear for people who looked at what the selected commit was before they pressed ,.

This comment has been minimized.

Sign in to view

Copy link
@ilyagr

ilyagr Mar 23, 2024 •
edited
Loading

Maybe "Start annotating from 0a7f, the parent of previously selected 395611" (with longer prefixes)?

It's also a bit unfortunate that these commit ids disappear from screen and can't be easily recovered. I'm not sure how to help with this, though, apart from showing this message in the command bar (the line below the status bar).

This comment has been minimized.

Sign in to view

Copy link
@koutcher

koutcher Mar 23, 2024

Author Owner

Thanks for your feedback. The newly opened blamed view has no idea whether it was opened from the parent of another commit, the only information it has are the filename and the commit to start the blaming from. Beside knowing that you are not blaming against HEAD anymore, the value itself has very little interest, we are more interested in the id of the commit changing a specific line. As it is also divergent from other views behaviour, I'll leave this part aside for now.

return true;
}

Expand Down Expand Up @@ -355,7 +359,7 @@ blame_go_forward(struct view *view, struct blame *blame, bool parent)
const char *filename = parent ? commit->parent_filename : commit->filename;

if (!*id && parent) {
report("The selected commit has no parents");
report("The selected commit has no parents with this file");
return;
}

Expand Down Expand Up @@ -474,10 +478,13 @@ blame_select(struct view *view, struct line *line)
if (!commit)
return;

if (string_rev_is_null(commit->id))
if (string_rev_is_null(commit->id)) {
string_ncopy(view->env->commit, "HEAD", 4);
else
string_format(view->ref, "%s", commit->filename);
} else {
string_copy_rev(view->env->commit, commit->id);
string_format(view->ref, "%s changed %s", commit->id, commit->filename);
}

if (strcmp(commit->filename, view->env->file))
string_format(view->env->file_old, "%s", commit->filename);
Expand Down
8 changes: 4 additions & 4 deletions test/blame/default-test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ assert_equals 'blame-default.screen' <<EOF
90286e0 Jonas Fonseca 2013-10-14 13:15 -0400 26|
90286e0 Jonas Fonseca 2013-10-14 13:15 -0400 27| lazy val parent: Projec
90286e0 Jonas Fonseca 2013-10-14 13:15 -0400 28| id = "parent",
[blame] project/Build.scala - line 1 of 64 43%
[blame] 90286e0752016a6bca30dfa7ca236d1f99345eb8 changed project/Build.scala 43%

This comment has been minimized.

Sign in to view

Copy link
@ilyagr

ilyagr Mar 23, 2024 •
edited
Loading

I think this is very much an improvement, thanks!

Currently, it shows:

[blame] 395611ebf13cbcebee1791b5c9d81ef2c6823475 changed src/blame.c - line 1 of 511

I wonder if it could be more clear about the fact that the commit changed the line, not the file. (Without clarification, I'd expect the latter in the status bar).

Perhaps

[blame] 395611ebf13 changed line 1 of 511 of src/blame.c

I think this also shouldn't show the full commit id, perhaps 7-10 characters. Otherwise, people will not see the end of the message.

Finally, I wish it was clearer that this is the "selected commit" other messages discuss. I haven't had an amazing idea for this, though.

This comment has been minimized.

Sign in to view

Copy link
@koutcher

koutcher Mar 23, 2024

Author Owner

We are constrained by the view framework for the layout of the line and all views show the full commit ids in the title window (we don't have a way to control the abbrev level here).
EOF

assert_equals 'blame-with-diff.screen' <<EOF
Expand All @@ -69,7 +69,7 @@ assert_equals 'blame-with-diff.screen' <<EOF
90286e0 Jonas Fonseca 2013-10-14 13:15 -0400 7|
4779f9b Jonas Fonseca 2013-11-26 20:13 -0500 8| object ScalaJSBenchmarks
90286e0 Jonas Fonseca 2013-10-14 13:15 -0400 9|
[blame] project/Build.scala - line 4 of 64 14%
[blame] 74537d9b257954056d3caa19eb3837500aded883 changed project/Build.scala 14%
commit 74537d9b257954056d3caa19eb3837500aded883
Author: Sébastien Doeraene <[email protected]>
AuthorDate: Tue Oct 29 18:46:52 2013 +0100
Expand Down Expand Up @@ -101,7 +101,7 @@ assert_equals 'blame-with-diff-no-file-filter.screen' <<EOF
90286e0 Jonas Fonseca 2013-10-14 13:15 -0400 7|
4779f9b Jonas Fonseca 2013-11-26 20:13 -0500 8| object ScalaJSBenchmarks
90286e0 Jonas Fonseca 2013-10-14 13:15 -0400 9|
[blame] project/Build.scala - line 4 of 64 14%
[blame] 74537d9b257954056d3caa19eb3837500aded883 changed project/Build.scala 14%
commit 74537d9b257954056d3caa19eb3837500aded883
Author: Sébastien Doeraene <[email protected]>
AuthorDate: Tue Oct 29 18:46:52 2013 +0100
Expand Down Expand Up @@ -152,5 +152,5 @@ assert_equals 'blame-parent-of-74537d9.screen' <<EOF
90286e0 Jonas Fonseca 2013-10-14 13:15 -0400 26|
90286e0 Jonas Fonseca 2013-10-14 13:15 -0400 27| lazy val benchmarkSettings =
90286e0 Jonas Fonseca 2013-10-14 13:15 -0400 28| unmanagedSources in (Com
[blame] project/Build.scala - line 4 of 66 42%
[blame] 90286e0752016a6bca30dfa7ca236d1f99345eb8 changed project/Build.scala 42%
EOF
2 changes: 1 addition & 1 deletion test/blame/initial-diff-test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,5 +63,5 @@ assert_equals 'blame-deleted-line.screen' <<EOF
0500 19x
0500 20x def add(benchmark: Benchmark) {
0500 21x benchmarks.push {
[blame] common/Benchmark.scala - line 17 of 99 21%
[blame] 34d6868cfae60a7cc2494d282eaee27efa900059 changed common/Benchmark.sc 21%
EOF
4 changes: 2 additions & 2 deletions test/blame/revargs-test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ assert_equals 'limit.screen' <<EOF
74537d9 Sébastien Doeraene 2013-10-29 18:46 +0100 26|
74537d9 Sébastien Doeraene 2013-10-29 18:46 +0100 27| lazy val parent: Projec
74537d9 Sébastien Doeraene 2013-10-29 18:46 +0100 28| id = "parent",
[blame] project/Build.scala - line 1 of 64 43%
[blame] 74537d9b257954056d3caa19eb3837500aded883 changed project/Build.scala 43%
EOF

# confirm that we kept our lower bound
Expand Down Expand Up @@ -82,5 +82,5 @@ assert_equals 'parent-of-4779f9b.screen' <<EOF
74537d9 Sébastien Doeraene 2013-10-29 18:46 +0100 26|
74537d9 Sébastien Doeraene 2013-10-29 18:46 +0100 27| lazy val benchmarkSetti
74537d9 Sébastien Doeraene 2013-10-29 18:46 +0100 28| unmanagedSources in
[blame] project/Build.scala - line 8 of 69 40%
[blame] 74537d9b257954056d3caa19eb3837500aded883 changed project/Build.scala 40%
EOF
2 changes: 1 addition & 1 deletion test/blame/start-on-line-test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,5 @@ ee91287 deltablue/src/main/scala/org/scalajs/benchmark/deltablue/DeltaBlue.scala
b103989 deltablue/DeltaBlue.scala Jonas Fonseca 2013-10-18 18:00 -0400 53|
b103989 deltablue/DeltaBlue.scala Jonas Fonseca 2013-10-18 18:00 -0400 54| override def prefix = "DeltaBlue"
b103989 deltablue/DeltaBlue.scala Jonas Fonseca 2013-10-18 18:00 -0400 55|
[blame] deltablue/src/main/scala/org/scalajs/benchmark/deltablue/DeltaBlue.scala - line 42 of 711 7%
[blame] b103989d59edab3adc312ff5408fa3d344ea0201 changed deltablue/DeltaBlue.scala - line 42 of 711 7%
EOF
6 changes: 4 additions & 2 deletions test/blame/stash-test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ test_setup_work_dir()
{
echo "original line" > file
git add file
export GIT_AUTHOR_DATE="1486403695"
export GIT_COMMITTER_DATE="$GIT_AUTHOR_DATE"
git commit -m "Initial commit"
echo "changed line" > file
git stash
Expand All @@ -40,10 +42,10 @@ EOF

assert_equals 'blame1.screen' <<EOF
original line
[blame] file - line 1 of 1 100%
[blame] 973a46fa50e2704ee2d550a931ad8edb1c50731c changed file - line 1 of 1 100%
EOF

assert_equals 'blame2.screen' <<EOF
original line
[blame] file - line 1 of 1 100%
[blame] 973a46fa50e2704ee2d550a931ad8edb1c50731c changed file - line 1 of 1 100%
EOF

0 comments on commit b1cda8c

Please sign in to comment.