New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add config options for length of commit hash displayed in commits view #3505
Add config options for length of commit hash displayed in commits view #3505
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, thanks for making this. It's one of those things that I didn't know I wanted; I'll set this to 4 or 5 to save some space in the commits view.
A few small minor things below.
pkg/config/user_config.go
Outdated
@@ -123,6 +123,8 @@ type GuiConfig struct { | |||
NerdFontsVersion string `yaml:"nerdFontsVersion" jsonschema:"enum=2,enum=3,enum="` | |||
// If true (default), file icons are shown in the file views. Only relevant if NerdFontsVersion is not empty. | |||
ShowFileIcons bool `yaml:"showFileIcons"` | |||
// Length of commit ID/ref (hash) in commits view. | |||
CommitHashLength int `yaml:"commitHashLength" jsonschema:"minimum=1,maximum=12"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why restrict to 12? If someone has a really large monitor and prefers to see full commit hashes, why not let them? I'd either set maximum to 40, or leave it out entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially set it 12 arbitrarily to avoid out of range index errors but that's redundant because of the way I wrote the code to display it. I also wasn't sure if it would mess up anything elsewhere because I'm not very familiar with the repo.
I changed it to maximum 40 here 3816cb9 because I think having it in the schema would be nice to let people know how long hashes are (I didn't know)!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, but this will change in the future. The git folks are working on replacing the 40-characters SHA1 with another hash algorithm that has longer hashes. This won't happen very soon, but some day, and the 40-character maximum will be as arbitrary then as the 12 would be today.
I guess in the light of this it's still best to leave out maximum, and allow users to set arbitrarily large values. And then it would be better to clamp out-of-range values to the length of the hash instead of falling back to ShortHash as you do now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great points!
I removed the schema maximum and the ShortHash call.
I also set the minimum to 0 and made up for the fact that without NF icons you couldn't see the status color by adding a display character (default is ●).
lazygit/pkg/config/user_config.go
Lines 126 to 131 in 8210a79
// Length of commit hash in commits view. | |
// If 0 and NF icons aren't enabled, show 'commitStatusChar' instead of hash. | |
CommitHashLength int `yaml:"commitHashLength" jsonschema:"minimum=0"` | |
// What to show instead of hash if commitHashLength==0 and NF icons aren't on | |
// Empty string to show nothing | |
CommitStatusChar string `yaml:"commitStatusChar"` |
lazygit/pkg/gui/presentation/commits.go
Lines 317 to 325 in 8210a79
hashString := commit.Hash | |
hashColor := getHashColor(commit, diffName, cherryPickedCommitHashSet, bisectStatus, bisectInfo) | |
hashLength := common.UserConfig.Gui.CommitHashLength | |
if hashLength == 0 && !icons.IsIconEnabled() { // if no icons and no hash, show a char so user still sees status color | |
hashString = common.UserConfig.Gui.CommitStatusChar | |
} else if hashLength < len(hashString) { | |
hashString = hashString[:hashLength] | |
} | |
hashString = hashColor.Sprint(hashString) |
} else { | ||
hashString = hashColor.Sprint(commit.Hash[:hashLength]) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I'd set hashString to the uncolored string, and use hashColor.Sprint
in the append call below as before. Seems more logical to me, somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did hashColor.Sprint there to be consistent with most of the rest of that function where Sprint is used to build the strings.
lazygit/pkg/gui/presentation/commits.go
Lines 326 to 399 in 961c3bc
actionString := "" | |
if commit.Action != models.ActionNone { | |
todoString := lo.Ternary(commit.Action == models.ActionConflict, "conflict", commit.Action.String()) | |
actionString = actionColorMap(commit.Action).Sprint(todoString) + " " | |
} | |
tagString := "" | |
if fullDescription { | |
if commit.ExtraInfo != "" { | |
tagString = style.FgMagenta.SetBold().Sprint(commit.ExtraInfo) + " " | |
} | |
} else { | |
if len(commit.Tags) > 0 { | |
tagString = theme.DiffTerminalColor.SetBold().Sprint(strings.Join(commit.Tags, " ")) + " " | |
} | |
if branchHeadsToVisualize.Includes(commit.Hash) && | |
// Don't show branch head on commits that are already merged to a main branch | |
commit.Status != models.StatusMerged && | |
// Don't show branch head on a "pick" todo if the rebase.updateRefs config is on | |
!(commit.IsTODO() && hasRebaseUpdateRefsConfig) { | |
tagString = style.FgCyan.SetBold().Sprint( | |
lo.Ternary(icons.IsIconEnabled(), icons.BRANCH_ICON, "*") + " " + tagString) | |
} | |
} | |
name := commit.Name | |
if commit.Action == todo.UpdateRef { | |
name = strings.TrimPrefix(name, "refs/heads/") | |
} | |
if parseEmoji { | |
name = emoji.Sprint(name) | |
} | |
mark := "" | |
if isYouAreHereCommit { | |
color := lo.Ternary(commit.Action == models.ActionConflict, style.FgRed, style.FgYellow) | |
youAreHere := color.Sprintf("<-- %s ---", common.Tr.YouAreHere) | |
mark = fmt.Sprintf("%s ", youAreHere) | |
} else if isMarkedBaseCommit { | |
rebaseFromHere := style.FgYellow.Sprint(common.Tr.MarkedCommitMarker) | |
mark = fmt.Sprintf("%s ", rebaseFromHere) | |
} else if !willBeRebased { | |
willBeRebased := style.FgYellow.Sprint("✓") | |
mark = fmt.Sprintf("%s ", willBeRebased) | |
} | |
authorFunc := authors.ShortAuthor | |
if fullDescription { | |
authorFunc = authors.LongAuthor | |
} | |
cols := make([]string, 0, 7) | |
if commit.Divergence != models.DivergenceNone { | |
cols = append(cols, hashColor.Sprint(lo.Ternary(commit.Divergence == models.DivergenceLeft, "↑", "↓"))) | |
} else if icons.IsIconEnabled() { | |
cols = append(cols, hashColor.Sprint(icons.IconForCommit(commit))) | |
} | |
cols = append(cols, hashString) | |
cols = append(cols, bisectString) | |
if fullDescription { | |
cols = append(cols, style.FgBlue.Sprint( | |
utils.UnixToDateSmart(now, commit.UnixTimestamp, timeFormat, shortTimeFormat), | |
)) | |
} | |
cols = append( | |
cols, | |
actionString, | |
authorFunc(commit.AuthorName), | |
graphLine+mark+tagString+theme.DefaultTextColor.Sprint(name), | |
) | |
return cols | |
} |
I like this format and would actually go further and make the few strings that were built inside the cols
construction match the rest. 389a8c9.
The cols
construction ends up looking like this
lazygit/pkg/gui/presentation/commits.go
Lines 392 to 405 in 389a8c9
cols := make([]string, 0, 7) | |
cols = append( | |
cols, | |
divergenceString, | |
hashString, | |
bisectString, | |
descriptionString, | |
actionString, | |
authorFunc(commit.AuthorName), | |
graphLine+mark+tagString+theme.DefaultTextColor.Sprint(name), | |
) | |
return cols | |
} |
This commit doesn't change the functionality at all but I think the consistency makes it more readable. I haven't written in go before so maybe there was a reason it was like that. No need to include this commit in the pull, just a suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I don't care too much about which way the code is written here, so I'm ok with your refactoring, but I don't really think it's necessary. One thing to be aware of is that now you are unconditionally adding divergence and description, whereas previously they were only added when needed. So now you are relying on the remove-empty-columns mechanism to remove them again; this works, but is unnecessary work.
My point about the hashString was that I don't like how the logic of assembling the string is mixed with coloring it. Not a huge deal either, I think we're talking about all this for longer than justified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly you'd prefer something like
cols = append(
cols,
hashColor.Sprint(divergenceString),
hashColor.Sprint(hashString),
// etc.
)
I think I agree I prefer that logic as well, but like I said I did it the way I did because all of the rest of the strings in that function are colored and formatted where they are assembled. It felt weird to have just those one or two strings be colored in the append call.
I would also suggest setting the default value to 4; I think most people don't need all those characters and would appreciate the extra space. I only set it to 8 because that's how it was before. |
You might be right about that, but I'd still prefer to leave it at 8 just so that we don't change the behavior unnecessarily. |
Could you please squash the last commit into the first and rebase onto master to get rid of the merge commit? Please never merge master into your branch, always rebase onto master instead. Results in a cleaner commit history. |
d1f97dd
to
8210a79
Compare
8210a79
to
acfd6b5
Compare
acfd6b5
to
0bbf3d9
Compare
Awesome, I'm very happy with this state now. Great work! I pushed a fixup (deeaed0) for a tiny error in a comment; while the schema doesn't allow negative values, we don't enforce the schema, so users can totally put in -1, and it's good to see that we handle it correctly. Now, I'm going to spend a minute on getting the commit history closer to how I prefer it. When I asked you a few days ago to squash things, I didn't mean to squash in the refactoring; it's good to keep behavior changes and refactorings in separate commits (if easily possible). But when you iterate over a change, it's good to squash the iterations, because the history of how the code evolved during review is not interesting to future readers. For the future: it's nice to use Going to force-push the rewritten branch now (the tip is identical) in case you want to have a look before I merge. |
- Add config option `commitHashLength` to to pkg/config/user_config.go - Changed the hash display in pkg/gui/presentation/commits.go
Change `func displayCommit()` so all the individual strings are built first, then the whole thing `cols` is put together. Before, most strings were built prior to constructing `cols`, but a few were built inside the `cols` construction.
deeaed0
to
b63321a
Compare
Thank you for noticing the mistake in the comment! Not sure why I just assumed it was enforced.
I checked out how you fixed my commits and I'll keep this in mind in the future! |
Add a new config option
gui.commitHashLength
to change the length of the commit hash displayed in commits view.default:
With config:
pkg/config/user_config.go
andschema/config.json
docs/Config.md
pkg/gui/presentation/commits.go
go generate ./...
)docs/Config.md
) have been updated if necessary