cli: remove author name from default annotate template, reorder fields #4653
Conversation
There was a problem hiding this comment.
If this is the easiest fix in the short term, I think it's fine. I'll let others look at this before approving.
I'm hoping this won't be the final state though.
I have some thoughts about the future, but they are not blocking. The most actionable one is the one about dates, but I don't think it is mandatory.
How hard would it be to implement padding?
There was a problem hiding this comment.
If this is the easiest fix in the short term, I think it's fine.
I believe so. I just want to make the default usable.
How hard would it be to implement padding?
I don't think it's difficult to implement a padding-only function, but we'll need to come up with a nicer syntax.
A padding function would have to support optional min/max widths, padding character, left/right/center alignment. For readability, it's nice if multiple texts can be inlined in a string literal (just like Rust's format!().) This means padding function will take another template mini language.
yuja
force-pushed
the
push-kywpuxuyxqrq
branch
from
6332562
to
4f6bb06
Compare
|
Just to catch myself up, is the purpose of this PR to remove the author because it messes with alignment? |
Yes, that's the main reason. Another reason is that author name can be long enough to make file contents invisible. So even if we include author, I want to truncate it up to 4-8 characters. BTW, I've implemented padding/truncating function. If we'd like to include author in the default template, it can be something like |
Nice, I see no reason not to merge this PR as is. I can either add author manually with padding as mentioned or discuss in an issue if people feel like it is default-worthy |
|
Do we like to include author name in the default annotation template? |
|
I vote yes for showing the author and if anything needs to be cut, omit the commit ID. |
|
My personal preference would be to include the email, trimmed to (say) 8-10 chars. We could try the trimmed name, but that will be annoying if we have more than one Alexander or Jean-Jacques one day. We could also consider various ways to abbreviate names, but probably not in the first version. It'd also be cool to consider de-duplication, as in #4653 (comment), but that is also complicated. |
The problem is that author names are variable-length by nature, and format_*() can be customized in that way. I also made committer.timestamp() shorter because I'm more interested in file contents than precise timestamps. We could add some format_short_fixed_length_*() hook points, but it would probably be easier to just customize the annotation template at all.
yuja
force-pushed
the
push-kywpuxuyxqrq
branch
from
4f6bb06
to
0debffe
Compare
|
Ok, updated the template to include hard-coded (change_id, commit_id, author, timestamp). PTAL. |
+1. What do the rest of you think about removing the commit id? |
|
I'm torn about the commit id. Showing only the change id is a bit problematic if there are multiple commits with the same commit id, or if we blame hidden revisions. I'm not sure how to address this in a blame view. Update: One option would be to show the commit id instead of the change id in these cases, but that might be confusing. I think it would furthermore be convenient for me to keep the change id, since I'd guess that if I'm blaming, I'm likely to be using other tools at the same time (GitHub, VS Code) that might be more familiar with commit ids. That said, I agree that it's the least important of the things we currently show. |
| format_timestamp(committer.timestamp()), | ||
| change_id.shortest(8), | ||
| commit_id.shortest(8), | ||
| pad_end(8, truncate_end(8, author.username())), |
There was a problem hiding this comment.
Perhaps author.email() would be better (even though I had just the username in my screenshot), in case we have somebody with [email protected] and another person with me@another-cool-username.
If you disagree, I'd use pad(truncate(author.username() ++ '@')), to make it clearer what this field is. (The @ might not always show up, but even if it shows up occasionally, I hope it'll make things clear enough).
More subjectively, after looking at the tests, I'm not sure whether 8 characters is enough. I think we could try 10. We can also keep it as is and change it later, if needed.
There was a problem hiding this comment.
This is not solely related to this PR, but I dislike how we have an entire field in commits that represents an appropriate human‐readable identifier to use for their authors and committers but ignore it to just show email addresses instead, and then run into situations like this where we’re hoping that we can use the local‐part as a sensible global identifier. It makes sense in a corporate context where everyone is ‹username›@google.com, but not for collaborative projects in general.
Of course I’m biased since the local‐parts of my email addresses don’t identify me at all.
There was a problem hiding this comment.
username() (which is sometimes shorter than 8 chars) looked nicer than full 8 or 10 email() characters truncated at arbitrary place. I don't have a strong opinion, though.
I wouldn't add @ suffix unless we do the same for builtin_log_onenline.
There was a problem hiding this comment.
For reference, here is the log with emails trimmed to 10 chars. I also make the @ white somewhat indulgently (it'd be easier if we supported signature.domain()), though it seems to help readability to me.
There was a problem hiding this comment.
I'm not sure what people other than me prefer. Is a trimmed email a worthwhile compromise between a trimmed username and a full name trimmed to, say, 20 characters?
I wouldn't add @ suffix unless we do the same for builtin_log_onenline.
I wouldn't mind adding @ to builtin_log_onenline as well, especially if we don't go with the full email here and adding it to the username was conditional on adding it to that template. But again, this might be my personal aesthetics.
There was a problem hiding this comment.
I also don't feel very strongly about this, by the way. I can always configure it and, more importantly, we can try something and see how/whether people react.
There was a problem hiding this comment.
Tig supports an abbreviated author name that seems to split the name by spaces, use initials, and then the full last name. I'm not sure whether it's the default, but it's what I picked. Examples from tig blame Cargo.toml:
- ASeipp
- MvZweigbergk
- EMesterhazy
- IGrigoriev
- db (from
dependabot[bot]) - SJennings
- YNishihara
- BTan
- JOkoński
- STardieu
I'm not sure we're lacking in options, but I thought this is worth mentioning. Update: OTOH, I'm not sure how well this works with non-Western names. I wonder whether tig does anything special with other scripts.
It also has an option for relative dates collapsed to two-three letters: 11M, 2Y, 3D, 6H. (I do remember that Yuya said he isn't a fan of relative dates)
Finally, it colors the commit ids by age, which is nice.
Here's my config: https://asciinema.org/a/kpHpSIWREIYJ2gQvVEhWrhWDt
The default tig uses is very verbose, though: https://asciinema.org/a/LYVeJuCy73snrboJKGDgK6HFh
Unfortunately, tig doesn't have keybindings for toggling these options as I wanted in #4653 (comment).
Update: Pressing o in tig allows you to toggle options. After o, you press up and down to pick which option to switch. That menu also seems to show keybindings for toggling options quicker; it seems that you can even press A to toggle the author.
Pointless rant: tig is a wonderful piece of software, but its choice of keybindings follows some logic that is completely alien to me. Even when I'm reading its docs, I can never find what I'm looking for by searching since the operations listed are not the ones I'd think of and have different names than I'd think of. I have to read every line to find something. For example, the o menu is the only place I found where the A binding is documented. If it's documented elsewhere, it's nowhere near where I looked (I searched for "blame" in man tigrc and man tigmanual).
| change_id.shortest(8), | ||
| commit_id.shortest(8), | ||
| pad_end(8, truncate_end(8, author.username())), | ||
| committer.timestamp().local().format("%Y-%m-%d"), |
There was a problem hiding this comment.
I would include the hour and minute, in case I'm looking at a draft PR stack I just wrote today.
There was a problem hiding this comment.
I would add %H:%M:%S then. Consistency is more important than saving 3 characters.
There was a problem hiding this comment.
I'm not sure. Would you ever want to see the seconds in the blame view? I'd only want that if I did several commits in less than a minute, which is rare.
But I'll leave it up to you and others.
There was a problem hiding this comment.
I personally don't want to see %H:%M:%S at all. I wouldn't even be interested in %d (and author as I said before.) I'm just saying that we should stick to the default formatting than adding a couple of variants that look quite similar.
There was a problem hiding this comment.
It's an interesting question about showing the author. We decided to not call it jj [file] blame because we don't want it to seem like a tool for blaming people (at least I think that's why). Removing the author from the output seems compatible with that line of thinking.
OTOH, it does seem a bit useful to help remind you of the context in which a line change. I personally often remember (think I) remember who added a feature, so filtering by user is sometimes helpful at least for finding a commit I'm looking for. But it probably is much less helpful in jj file annotate output. I think I just look for the change id (or commit id in git/hg) and date (in addition to the line contents, of course).
So, I think I would personally be happy to try including only change id and date (not even time). But that's just my feeling.
There was a problem hiding this comment.
Yuya, you just want to see the year and the month? So, you only care whether some lines are much older than others?
Yes. I just want to see some identifier, age (but I don't like relative timestamp fwiw), and line content. I would then run jj show <id> to see more details. It could be a TUI opened by e.g. jj file annotate --interactive.
BTW, author name is more visually distinct than change/commit ids and dates, so it will help separate lines originating from different commits.
There was a problem hiding this comment.
Where are you "filtering by user" if not in the jj file annotate output? The log?
Yes, that's what I meant. Sorry that I wasn't clear.
There was a problem hiding this comment.
BTW, author name is more visually distinct than change/commit ids and dates, so it will help separate lines originating from different commits.
OTOH, that may make it harder to notice when some nearby lines are from different commits if they have the same author.
There was a problem hiding this comment.
If this command has machine output, all the details can be output (JSON ?).
There was a problem hiding this comment.
If this command has machine output, all the details can be output (JSON ?).
No, the output is meant for humans. It could also be used by scripts, but they probably only need the commit id and the line number.
It could be a TUI opened by e.g. jj file annotate --interactive.
Yes, a TUI would be very nice. Among other nice things, it could have a binding to hide/show author or date, which would make the defaults less important.
I generally use tig blame for annotations, which is quite a nice UI, though I'd change a bunch of things about it (easier and more discoverable key bindings, add the show/hide author or date binding, ...). Update: More on tig's tricks
(Also, that's one more reason I should let others decide on the default: I'm not sure I'll actually be using the jj file annotate UI much, except for debugging)
LGTM, thank you very much! I have some minor suggestions above, but we can experiment with them later as well. This is certainly better than what we had before. |
| format_short_id(commit_id), | ||
| format_short_signature(author), | ||
| format_timestamp(committer.timestamp()), | ||
| change_id.shortest(8), |
There was a problem hiding this comment.
This will not show whether the commit is hidden or divergent.
I'm not sure what to do about it. The simplest things would be to change the color, but it would make the template complicated. Adding ? for divergent makes sense, but we don't have a short syntax for hidden.
So, this might not be actionable, but I thought I'd put it out there in case people have ideas.
There was a problem hiding this comment.
Is the goal of annotate to give all the gory details, or to show the last change that modified the line?
There was a problem hiding this comment.
If the revision is hidden or divergent, it can't be referred to easily by its change id. This seems worth indicating somehow when you show this (potentially useless) change id.
It might be OK to keep things as they are if we also show the commit id. Maybe I'd add color to the change id at some point.
BTW, this keeps reminding me that we might want a syntax like zzx^a to refer to a revision with change id zzx (that could be divergent) and commit id starting with a. I'm not sure how appropriate this is for hidden commits, but if it is and if we decided to usually omit commit ids, it could be a way out. But again, at least for now, I think just showing the commit id might be easier and more convenient (though I'm not certain about it).
There was a problem hiding this comment.
I feel like it's going to be very rare to want to annotate a hidden or divergent file. Probably rare enough that the user can override the template when they want to do that. I can't remember ever running hg blame or git blame on a hidden commit.
|
By the way, there is nothing stopping us from providing a few prefabricated annotate template aliases that the user could swap between, is there? (Probably in a separate PR) Or do we not want the user to configure Also, we probably want to allow something like The one issue I see is that, ideally, the whole line including the line number and content would be templated, but I don't think it's a huge deal if it isn't. |
Checklist
If applicable:
CHANGELOG.md