Blame: improve error messages for the , operation
#1316
Conversation
ilyagr
changed the title
, operation, operation
ilyagr
force-pushed
the
blame
branch
3 times, most recently
from
d387a0b to
1444a54
Compare
src/blame.c
Outdated
| return; | ||
| } | ||
|
|
||
| if (!strcmp(history_state->id, id) && !strcmp(history_state->filename, filename)) { | ||
| report("The selected commit is already displayed"); | ||
| report("Commit %.8s is already beind displayed", commit->id); |
There was a problem hiding this comment.
typo: being.
Also I wonder how to hit this case?
I would still appreciate pointers on how to create a help section
The existing help text is generated for commands, I guess we don't want to add commands.
Probably we can add some lines that are not associated with a command. Not completely trivial but doable.
I'm also
curious whether you think that the additional command I describe in the "update" in #1315 (comment) is a good idea.
Having a command that shows the previous version of a file doesn't seem useful very often, unlike recursive the commands that use the commit that added the line at cursor.
There was a problem hiding this comment.
Thanks for looking this over!
typo: being.
Fixed, thanks for catching it!
Also I wonder how to hit this case?
No idea, I was wondering that myself.
The existing help text is generated for commands, I guess we don't want to add commands.
I imagine we could add commands with the same keybindings only for the blame view. I'm not sure whether they could have the same names (for people who reconfigured things), though.
Probably we can add some lines that are not associated with a command. Not completely trivial but doable.
This would be great! Also, is the man page generated from the commands as well?
Having a command that shows the previous version of a file doesn't seem useful very often, unlike recursive the commands that use the commit that added the line at cursor.
I think it's useful for a few reasons:
- To browse the history of one file. I am not sure whether tig has that functionality already, the closest I found is the "blob view", but it doesn't support the
,command. - As I'm about to post in help/docs bug: blame view keybindings are hard to discover #1315, together with
bsuch a command can replace,'s current behavior, but not vice-versa. - For consistency with what the command does in other screens (I assume, I haven't double-checked)
- For consistency with how the command is described in help (unless we can think of a better description)
I agree that it sounds a bit weird in theory, but I find that I want it in practice when I see a blame view. Maybe I'll get used to not having it...
There was a problem hiding this comment.
Having a command that shows the previous version of a file doesn't seem useful very often, unlike recursive the commands that use the commit that added the line at cursor.
In #1315, you pointed out that you use the diff view for this. Perhaps the reason I want this command is that I don't know how to do that effectively, especially if I want to focus on one file.
There was a problem hiding this comment.
I usually don't really care about the history of a full file1 but about a single line (and the related changes).
I usually alternate between blame and diff view for recursive blaming.
I'm realizing I'm not sure how to do this,
Often a line has been modified by several (uninteresting) commits.
So what I (used to) do is
- select the interesting line and press "b" to show the blame-view
- press Enter to show the diff that introduced the line
- use "/foo" and/or "n" to select the old version of the line (the one prefixed with a "-")
- go back to step 1 (until I have found the interesting commit)
There are many alternatives that can help like the view-parent, using an
ignore-revs-file using git blame -w to ignore whitespace changes, using
tig -Gfoo.. but I find the above steps to be the most general one and
least likely to break (assuming your Git history has some level of continuity).
I've recently switched to the dark side and implemented this workflow in my editor, with one neat speedup:
My step 2 (pressing Enter to show the blamed commit) will not move the viewport to the top of the commit.
Instead it jumps directly to the line that is being blamed.
Since in most cases, the old version of the line is right next to the new version (which is the blamed one), this usually makes step 3 trivial.
I'd like to have this in Tig as well (I still use it for heavy-duty Git operations).
Perhaps a B shortcut (view-blame-jump) that's like view-blame but moves to the blamed line. Or change view-blame
For consistency with what the command does in other screens (I assume, I haven't double-checked)
Normally, view-parent goes to the parent commit so that's not the same as "youngest ancestor that touched this file" anyway.
And that's hardly more intuitive than "youngest ancestor that touched this line" even though I agree that one is confusing too
Footnotes
-
If I care about the history of a file, I don't think I'd want to look
at it by eyeballing blob views. Rather I'd look at the diffs fromtig my-files.c. ↩
There was a problem hiding this comment.
Thanks for explaining, makes sense.
My step 2 (pressing Enter to show the blamed commit) will not move the viewport to the top of the commit.
...
I'd like to have this in Tig as well (I still use it for heavy-duty Git operations).
That would be very nice! I think the reason that I didn't think of this is precisely that tig does not move to the correct line by itself. That'd be more discoverable that ,.
I appreciate you adding this to Kakoune, too! 👍 I'll try it out once the new release comes out. I do usually use VS Code + Dance plugin, though, so I'd have to open Kakoune specially just for this in most cases.
Also, I'm currently wishing not just for a tool I can use, but a tool I can recommend to others. Kakoune is not it, unfortunately, at least not for this purpose.
Normally, view-parent goes to the parent commit so that's not the same as "youngest ancestor that touched this file" anyway.
I'm not sure how valid your argument is; it depends on whether in the rest of tig, view-parent goes to the parent commit of the current view or of the currently selected item. I'm hoping it's the former, in which case I think my suggested behavior is consistent.
But in any case, the strongest argument for changing its behavior, IMO, is that you can easily get the line behavior from the "this file" behavior by combining it with b, you would just need to do b, instead of , like you'd do now. You can't get the behavior I want at all, I think, in the current tig. I want it one way or another (I don't think it's the most important thing in the world, but I do think it'd come useful), and the other (current) behavior could either be b, or it could be a separate key (I think B would work great, but anything is fine).
There was a problem hiding this comment.
Thanks for your proposal. I don't think the problem really lies in the error message but rather in the lack of information in the title window (to which the error message is supposed to refer). I tried to improve this in koutcher@b1cda8c.
Both keys are properly described in the man page, but I agree it is not always easy to understand where the annotating starts after using b or ,. To make it clearer, I also added a notification in the status window when the starting point changes. HTH
There was a problem hiding this comment.
Thank you for working on this! I posted some thoughts at koutcher@b1cda8c. I certainly think your commit is an improvement, but I think it'd be nice to clarify its messages further, if possible.
The message is now: Commit f4657f0 has no ancestors modifying the seleted line instead of The selected commit has no parents This addresses a portion of jonas#1315. Apart from being a bit clearer, the user will be able to learn what `,` is meant to do from it. In particular, they should notice that commit `f4657f00` is the commit on the selected *line*, not the commit that is currently displayed in general. Trying to press `,` a few times is a natural thing to try to do after reading the help while trying to figure out how blame works in tig.
This addresses a portion of #1315.
The message is now:
Commit f4657f0 has no ancestors modifying the selected line
instead of
The selected commit has no parents
Apart from being a bit clearer, the user will be able to learn what
,is meant to do from it. In particular, they should notice that commit
f4657f00is the commit on the selected line, not the commit that iscurrently displayed in general.
Trying to press
,a few times is a natural thing to try to do afterreading the help while trying to figure out how blame works in tig.
I would still appreciate pointers on how to create a help section. I'm also
curious whether you think that the additional command I describe in the "update" in #1315 (comment) is a good idea.
I'd also need a pointer if you'd like me to add the test. Changing the message didn't seem to break any tests.