Skip to content
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

git commit message highlighting #1338

Merged

Conversation

the-mikedavis
Copy link
Member

@the-mikedavis the-mikedavis commented Dec 22, 2021

Git stuff was my last bastion of kakoune and I like the way kakoune highlighted new files, deletions, etc. So I made this!

helix-commit

I've got the playground up here if you wanna play with it ahead of time: https://the-mikedavis.github.io/tree-sitter-git-commit/

I was curious how we could choose language based on exact filename instead of extension, but according to this block, that's already possible!

@the-mikedavis the-mikedavis marked this pull request as ready for review December 22, 2021 15:27
@the-mikedavis
Copy link
Member Author

got a screenshot with the default theme, although my terminal colors might be throwing off how it looks

helix-default-theme-commit

@the-mikedavis
Copy link
Member Author

example of configuring a theme to differentiate the diff.delta and diff.delta.moved: the-mikedavis/dotfiles@0506035

@the-mikedavis
Copy link
Member Author

Would I be stepping on theme-maintainers' toes by updating existing themes to add the new scopes?

(and wow the themes dir has been growing! :D)

@Omnikar
Copy link
Contributor

Omnikar commented Dec 22, 2021

This seems to be missing highlighting for untracked file names that are not staged.

# Untracked files:
#	foo.bar

@the-mikedavis
Copy link
Member Author

Ah yeah I thought about untracked files but it would be a bit more difficult to parse them with how the grammar is currently written. I was thinking it'd be fine to leave them unhighlighted (and hence highlighted as comment) since they're unstaged and aren't really important to the commit anyways

It might be nice to at least have the option to highlight those paths though, especially for the similar case of conflicts in a merge/rebase like so:

add submodule on tree-sitter-rebase, add to languages

# Conflicts:
#	languages.toml

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# interactive rebase in progress; onto 34766e2
# Last command done (1 command done):
#    pick 1bc0b2c add submodule on tree-sitter-rebase, add to languages
# Next commands to do (5 remaining commands):
#    pick e339083 add basic highlights query
#    pick 00c5a26 inject bash in execute statements
# You are currently rebasing branch 'md-tree-sitter-rebase' on '34766e2'.
#
# Changes to be committed:
#	modified:   .gitmodules
#	new file:   helix-syntax/languages/tree-sitter-rebase
#	modified:   languages.toml
#
# Untracked files:
#	COMMIT_EDITMSG
#

@Omnikar
Copy link
Contributor

Omnikar commented Dec 22, 2021

I also think that there should be some highlighting for the headers, like Changes to be committed:.

the-mikedavis added a commit to the-mikedavis/tree-sitter-git-commit that referenced this pull request Dec 22, 2021
@the-mikedavis
Copy link
Member Author

the-mikedavis commented Dec 22, 2021

Ok @Omnikar I wrangled the grammar a bit and now there's highlighting for summary headers (Conflicts/Untracked files/Changes to be committed) and paths under those headers. That rebase-commit from above now looks pretty:

rebase-commit-highlighted

(and for the online playground, I mucked with the setup a bit so you'll have to clear local storage or open in a private window to kick the playground into a good state, if you want to try these new changes there)

@Omnikar
Copy link
Contributor

Omnikar commented Dec 23, 2021

Ok @Omnikar I wrangled the grammar a bit and now there's highlighting for summary headers (Conflicts/Untracked files/Changes to be committed) and paths under those headers. That rebase-commit from above now looks pretty:

Looks great!

@archseer
Copy link
Member

Would I be stepping on theme-maintainers' toes by updating existing themes to add the new scopes?

Sure, if you don't mind! Can just copy-paste the defaults if you don't know which colors to use

@archseer
Copy link
Member

Also, could you mark everything below as a diff block?

# ------------------------ >8 ------------------------
# Do not modify or remove the line above.
# Everything below it will be ignored.

Then we could inject a diff grammar that can color the diff with diff.plus/diff.minus & highlight the headers

(Here's an example from my old vim config, when doing a git commit --amend)
pos

@Omnikar
Copy link
Contributor

Omnikar commented Dec 23, 2021

I just want to say that this is a very exciting PR for me because git commit messages are the last thing that I still use Vim for, so I'll finally be able to completely switch over to Helix once this is merged. 😄

the-mikedavis added a commit to the-mikedavis/tree-sitter-git-commit that referenced this pull request Dec 23, 2021
@the-mikedavis
Copy link
Member Author

the-mikedavis commented Dec 23, 2021

@archseer w.r.t. the scissors, I updated the grammar to parse out the scissors comment as a node within (comment), which allows us to write a query to capture those trailing messages after the scissors (see the new commented-out injections.scm). For now I have that pattern setup to mark trailing messages after the scissors as comments so they don't look like they'll end up in the commit message.

I think you're using git commit --verbose which uses the scissors to show the diff but there's also a (probably less-widely used) git commit --cleanup=scissors that looks like so:

tmp

# ------------------------ >8 ------------------------
# Do not modify or remove the line above.
# Everything below it will be ignored.
#
# Date:      Thu Dec 23 08:06:48 2021 -0600
#
# On branch master
# Your branch is ahead of 'origin/master' by 1 commit.
#   (use "git push" to publish your local commits)
#
# Changes to be committed:
#	new file:   tmp.txt
#

So I'd like to support both and leave it up to the query to figure out what is diff and what isn't.

(btw I didn't know about verbose commit, I'm definitely stealing that in my dotfiles now :D the-mikedavis/dotfiles@8280144)

also I think a diff grammar is up next for me :)

@the-mikedavis
Copy link
Member Author

Ok so I took a crack at a diff parser based on those injections.scm queries (repo, integration branch) and it's looking good, albeit it's a little buggy: line-25 is picking up as an addition but only when injected 🤔

commit-verbose-diff

I'll poke around the grammar and see if that's the grammar or maybe a bug in tree-sitter.

@archseer
Copy link
Member

(btw I didn't know about verbose commit, I'm definitely stealing that in my dotfiles now :D the-mikedavis/dotfiles@8280144)

Huh, I completely forgot about that: https://github.com/archseer/snowflake/blob/4079405214666163c50cfbf71d798e63895814ae/users/profiles/git/default.nix#L60

also I think a diff grammar is up next for me :)

That would be great :D Wish we could also syntax highlight the diffs but that will be hard with tree-sitter since we only get partial code fragments, not the whole file.

@the-mikedavis
Copy link
Member Author

Ooo highlighting within diffs would be super sweet. Maybe you can reconstruct the full file by consulting the git index in some cases? Could be a cool feature of a git-porcelain plugin some day

@archseer
Copy link
Member

archseer commented Dec 24, 2021

I use https://github.com/dandavison/delta that does this for git diffs, there's a nixos module for it too

@the-mikedavis the-mikedavis force-pushed the md-git-commit-message-tree-sitter branch from a234d55 to bb8f2da Compare December 24, 2021 18:52
@the-mikedavis
Copy link
Member Author

the-mikedavis commented Dec 24, 2021

rebase-commit

I was thinking it should also be possible to inject tree-sitter-git-rebase under those "last command done" / "next command to do" headers. I'll see if tree-sitter-gitcommit needs any changes to support that

edit: added a rebase_command node that can be used for injection

@the-mikedavis the-mikedavis force-pushed the md-git-commit-message-tree-sitter branch 3 times, most recently from fbdd5b4 to 33d1921 Compare December 24, 2021 22:57
@the-mikedavis the-mikedavis force-pushed the md-git-commit-message-tree-sitter branch from 33d1921 to 5f75e6c Compare December 24, 2021 23:05
@Omnikar
Copy link
Contributor

Omnikar commented Dec 24, 2021

Shouldn't it be named tree-sitter-git_commit to match git_commit in grammar.js? For me, the syntax highlighting doesn't seem to work unless I change everything from git-commit to git_commit.

@the-mikedavis
Copy link
Member Author

Hmm that's weird that highlighting doesn't work with git-commit, it's working for me even if I cargo clean and then cargo build 🤔

I was leaning into some prior art here with that rename:

Let me do a fresh clone of helix to make sure I don't have any weird submodule things hanging around

@Omnikar
Copy link
Contributor

Omnikar commented Dec 24, 2021

Hm, maybe the issue is on my end, I'll try again.

@Omnikar
Copy link
Contributor

Omnikar commented Dec 24, 2021

OK, I tried rebuilding everything and it still didn't work.

@the-mikedavis
Copy link
Member Author

Ok I tried a fresh clone and build

git clone --recurse-submodules --branch md-git-commit-message-tree-sitter git@github.com:the-mikedavis/helix.git
cd helix/
cargo run ~/COMMIT_EDITMSG

And I the scopes and highlights seem to be working for me. Maybe there's something strange going on with the submodules you have? I renamed the repository and I had to do some funky stuff to my git index to make git happy. Maybe if you delete the branch, re-check it out and git submodule update --init --force it'll work?

@Omnikar
Copy link
Contributor

Omnikar commented Dec 24, 2021

Huh, I tried it with a new clone and it did work…

@Omnikar
Copy link
Contributor

Omnikar commented Dec 25, 2021

OK, I was able to get it to work on my main clone by deleting and refreshing the submodule and cargo cleaning.

@archseer
Copy link
Member

Tested, LGTM! 👍

@archseer archseer merged commit 6af0d51 into helix-editor:master Dec 25, 2021
@the-mikedavis the-mikedavis deleted the md-git-commit-message-tree-sitter branch December 25, 2021 16:11
the-mikedavis added a commit to the-mikedavis/tree-sitter-git-commit that referenced this pull request Dec 25, 2021
see helix-editor/helix#1338 (comment)

Leading whitespace is important when injecting diff highlights into
messages trailing the scissors.

Without this change, some adverserial context lines can end up being
mistakenly parsed as non-$.context rules. For example, in the
screenshot of the linked issue comment, a context line is being parsed
by a tricky link that looks like a malformed $.similarity rule. Because
NEWLINE is not a child of $._line but $.source in tree-sitter-git-diff,
part of the line is re-parsed into another valid $._line rule, namely
$.addition in this case.

For an example COMMIT_EDITMSG which has a second diff line 6 characters
to the right of the newline, this commit changes the start column of
the parsed $.message node to include the whitespace:

 (source [0, 0] - [7, 0]
   (subject [0, 0] - [0, 53])
   (comment [2, 0] - [4, 38]
     (scissors [2, 0] - [4, 38]))
   (message [5, 0] - [5, 36])
-  (message [6, 5] - [6, 80]))
+  (message [6, 0] - [6, 80]))

This change probably restricts this grammar to tree-sitter 0.20.1+
because the WHITE_SPACE 'extra' is now used as an extra and within
a rule
(see tree-sitter/tree-sitter#1444 (comment))
but trailing diffs are not meant to be edited anyways, so it's probably
not a big deal.
This was referenced Dec 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants