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 Files] Add Git Files package #1126

Merged
merged 106 commits into from
Mar 20, 2018
Merged

[Git Files] Add Git Files package #1126

merged 106 commits into from
Mar 20, 2018

Conversation

ryboe
Copy link
Contributor

@ryboe ryboe commented Aug 28, 2017

I've taken a stab at implementing support for gitconfig files, which I believe should be supported natively by Sublime.

Here's what my gitconfig looks like in Monokai now:

gitconfig_highlighting

Here are the features:

  • small 207 line gitconfig.sublime-syntax definition
  • doesn't trigger Oniguruma
  • recognizes these files: /etc/gitconfig, ~/.gitconfig, myproject/.git/config
  • there is no spec for gitconfig, so it's based on the official documentation here
  • tested on ~100 .gitconfig files I found on GitHub. lots of bugs and edge cases fixed.
  • catches stray brackets: [user]] <- whoops
  • includes a syntax test
  • Comments and Symbol List support
  • includes a single snippet for [section]
  • resolves [Git Files] Add a package for highlighting git files #1125

I erred on the side of Keeping It Simple:tm:. My initial implementation had special highlighting for placeholders like %h and %(author), but I found that added too much complexity. The line count was cut in half when I ripped it out. I'm happy to bring it back if you think it's important, though.

If you think this is a good idea, I'd appreciate it if people tried it out. I've done a lot of testing, so I'm reasonable confident there aren't many edge cases or weird bugs remaining. I'd also appreciate some feedback on my scope name choices. I've followed the official guidelines as best as I can, but scope naming is a bit of an art form 😀.

UPDATE (09/02/2017): I've implemented more sublime-syntaxes for commit messages, merge commits, tags, and interactive rebases. This is now a full Git package, not just a gitconfig package. You can try it out by setting core.editor to whatever Sublime alias you're using (e.g. subl, st).

$ ls -l /usr/local/bin/subl
lrwxr-xr-x 1 ryan admin 62 Apr 22  2016 /usr/local/bin/subl -> '/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl'
$ cat ~/.gitconfig
...
[core]
    editor = subl -w

NOTE: You must call subl with the -w (wait) flag, or commits won't work.

I took inspiration from vim's highlighting of commit messages. My Sublime implementation is basically identical. It even does cool things like encouraging a 50 char subject line, and enforcing an blank line after the subject line.

commit_msg_newline_enforcement

Why I rolled my own instead of using a sublime-syntax from an existing package?

I looked at some of the git packages on Package Control. I was going to use them if they were Good Enough:tm: and had permissible licenses. Unfortunately, the code quality was kinda meh 😬 and it was too easy to trigger highlighting mistakes. Git support is really important. I'm hoping this can eventually be merged as Sublime's default git package, so it needs to be bulletproof. If you think this is a good idea, I'd sincerely appreciate the help testing it 🙏. Thanks!!!

@ryboe
Copy link
Contributor Author

ryboe commented Aug 28, 2017

One possible objection to adding a gitconfig package would be that we should have a git package instead. The git package would include highlighting for commit messages. Atom has this with its language-git package.

@rwols
Copy link
Contributor

rwols commented Aug 28, 2017

I think a standardized syntax for all things git-related is a good idea.

scope: support.constant.color.gitconfig

comments:
- match: (#|;).*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you absolutely sure that things like var=foo#bar also start a comment? Or does var have the value foo#bar now? Is a space between the comment token required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, the only time when # isn't starting a comment is in hex colors like #efefef. I looked at ~100 gitconfigs on GitHub and didn't see any cases of # being a value, but it's possible there's an obscure edge case I missed.

# [section-name "subsection"]
section:
- match: \[
scope: punctuation.definition.section.begin.gitconfig
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

punctuation.section.brackets.begin ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with the scopes in the existing packages, I think punctuation.definition.section. makes more sense. For example, the $ in PHP vars like $foo is scoped as punctuation.definition.variable, not punctuation.definition.dollar. But I'm happy to change it to brackets if you think it's wise. I'm 100% not attached to any of my scope names 🙂.

push:
- meta_scope: meta.brackets.gitconfig
- match: \]
scope: punctuation.definition.section.end.gitconfig
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

punctuation.section.brackets.end ?

# ^ punctuation.definition.string.begin
# ^ punctuation.definition.string.end
foo = \"foo\"
# ^^^^^^^ meta.value string.quoted.double
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be a double quoted string. It's an unquoted string with escaped double quotes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this 👍. I'll update the PR shortly with the improved scopes.

# ^ punctuation.definition.string.begin
# ^ punctuation.definition.string.end
foo = \'foo\'
# ^^^^^^^ meta.value string.quoted.single
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@rwols
Copy link
Contributor

rwols commented Aug 28, 2017

What about numbers? We have these funky suffixes k, m and g for 1024, 1024 * 1024 and 1024 * 1024 * 1024. See: https://git-scm.com/docs/git-config#git-config-integer

1: punctuation.definition.comment.gitconfig
scope: comment.line.gitconfig

# '\b', '\n', '\t', '\"', '\\'
Copy link
Contributor

@jrappen jrappen Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it would make sense here to add a match for invalid escape characters (with tests)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not sure how to do that 🤔. If someone tried to enter \r as an escape character, I could match that, but \r, when interpreted as two characters, is valid text.

C:\Users\rob <-- would mismatch

The current behavior might be the best we can do. Only valid escapes will change color.

@ryboe ryboe changed the title [gitconfig] Add gitconfig package [gitconfig] Add Git package Sep 3, 2017
@ryboe
Copy link
Contributor Author

ryboe commented Sep 3, 2017

@rwols First, thank you for the thorough review!
I tried to implement support for matching integers, but I found it caused too many mismatches. For example, log -1 would mismatch as an integer. I definitely think it would be good thing to add, but it's super hard 😬.

# everything left that isn't a comment is the commit message
- match: '[^#]*'
scope: string.unquoted.commit-message.gitcommit
push:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When do we pop from this push?

Copy link
Contributor Author

@ryboe ryboe Sep 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! This is kinda hacky. The goal is to enforce the empty-line-after-subject rule This is done with the previous match: \n context, which matches text between two newlines. This will actually highlight all even-numbered lines bright red. But we only want to highlight the second line red (if there's any text there). The solution I came up with is to push a context on the stack that matches all remaining text in the commit message, before match: \n has a chance to match again. To ensure that match: \n can't match any more text, I push a context on the stack and never pop it. I actually use this trick in the parent context too, which is also never popped.

If there's a better, less hacky, way of achieving this, please let me know!

vim's highlighting of commits

vim_commit

# this absurd regex is necessary to:
# 1. match comments in the subject line
# 2. only style the first 50 chars in the subject line
- match: ([^#\n]{,50})[^#\n]*((#).*)?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can leave out the newline characters here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I can't remove them. My (limited) understanding is that without the newlines, the regex is matching up to and including the newline char. This prevents the first pushed context (\n) from matching and highlighting non-empty lines as invalid.

With

working

Without

broken

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

scope: string.unquoted.commit-message.gitcommit
push:
- match: '[^#]*'
scope: string.unquoted.commit-message.gitcommit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the string.unquoted scope is overkill here -- we all know it's just plaintext except for comment lines.

scope: support.constant.color.gitconfig

comments:
- match: (#|;).*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use a push with a meta_scope of comment.line.gitconfig here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would work too. I'm not sure what the advantage would be. Since they're line comments, I kinda like the added guarantee that the context won't escape the current line.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the advantage of @rwols suggestion is that it becomes more likely that you will get correct highlighting if the syntax is ever embedded in another syntax: #757 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Should the pop match be $\r?\n? to handle Windows line endings, or does it matter?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ST normalizes line endings to \n internally, so the \r would never match, so no :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Good to know. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should still remove the .* part of the match.

key-value-pair:
- match: ^\s*({{variable_name}})\s*(\=)
captures:
1: meta.key.gitconfig variable.language.gitconfig # TODO: correct scope?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be a variable.language, maybe variable.other.readwrite is more suitable (although most color schemes don't highlight that, but they'll have to catch up). You could also drop the meta.key scope.

- match: ^\s*({{variable_name}})\s*(\=)
captures:
1: meta.key.gitconfig variable.language.gitconfig # TODO: correct scope?
2: punctuation.separator.key-value.gitconfig
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keyword.operator.assignment

# [section-name "subsection"]
section:
- match: \[
scope: punctuation.definition.section.begin.gitconfig
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with punctuation.definition if you think it suits better.

- include: escape

# some values use escaped quotes, e.g. --pretty=format:\"%h %ad | %s%d [%an]\"
string-double-escaped-quotes:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this context.

Copy link
Contributor Author

@ryboe ryboe Sep 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um, I'm not sure how to do that without increasing the potential for mismatches. With values (everything to the right of the = sign), I'm matching strings separately, at a high precedence. That way string text doesn't get mismatched as a color or boolean.

[foo]
    boolVar = true  <- match as bool
    color   = blue  <- match as color
    bar     = --flag=\"true blue\"  <- don't want to match as bool or color

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I see your point and I also see that it's been changed to an unquoted string so I'm happy. On the other hand, can something like this happen?

[foosection]
bar = \"  # a string of length one consisting of a double-quote character.

- include: escape

# \'foo\'
string-single-escaped-quotes:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this context too.

- TAG_EDITMSG
- syntax_test_git_commit
- syntax_test_git_merge
- syntax_test_git_tag
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These syntax_test_* file extensions are temporary, right? I believe ST picks up the syntax automatically for syntax tests anyway, I think you can remove these even when developing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to remove these! Sublime will open the syntax tests as plain text, but it's easy to switch the syntax. The problem is that the COMMIT_EDITMSG, MERGE_MSG, and TAG_EDITMSG files don't have extensions and don't have a predictable first line of text for first_line_match.

I think you're totally right that these shouldn't be in the shipped version, though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe ST picks up the syntax automatically for syntax tests anyway

That's a feature of PackageDev, actually.

captures:
1: support.type.branch-name.gitcommit

change:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting context, but can be improved. For example, highlight the ++++++------ lines accordingly. Take a look at how GitSavvy does it.

@rwols
Copy link
Contributor

rwols commented Sep 3, 2017

Ugh, I should really use the review function next time, sorry for the noise!

@ryboe ryboe changed the title [gitconfig] Add Git package [Git] Add Git package Sep 4, 2017
push:
- match: \n
pop: true
- match: '.'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with things like this, I wonder if it is more efficient to use .+$, as then the lexer doesn't have to match each character individually, which I would imagine has a slightly larger overhead.

@ryboe
Copy link
Contributor Author

ryboe commented Sep 11, 2017

I've made another pass over the source. I think I've addressed all the comments. Let me know if I missed anything 🙂

I've been using subl -w as my core.editor since last week and it's been working great so far.

Copy link
Contributor

@rwols rwols left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's starting to look good.

scope: support.constant.color.gitconfig

comments:
- match: (#|;).*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should still remove the .* part of the match.

- include: escape

# some values use escaped quotes, e.g. --pretty=format:\"%h %ad | %s%d [%an]\"
string-double-escaped-quotes:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I see your point and I also see that it's been changed to an unquoted string so I'm happy. On the other hand, can something like this happen?

[foosection]
bar = \"  # a string of length one consisting of a double-quote character.

- include: color-named # red, blue, green
- include: color-attribute # bold, italic, underline
- include: boolean # true, yes, on, 1
- match: '\S' # match most values
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use \S+ here?

# ^^^^^^^ meta.commit constant.numeric.hex.hash
# ^^^^^^^^^^^^^ meta.commit text.plain.commit-msg

# Rebase 9e73d21..6746220 onto 9e73d21 (2 commands)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe scope the two dots as punctuation.separator?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in a manner of speaking the .. is a range, not a separator. RegExp uses constant.other.range for character ranges, but maybe punctuation is better here.

Also, there's a ... in Git (although it should never appear here). Whoever decides on the scope might want to take that into account.

Copy link
Collaborator

@FichteFoll FichteFoll Sep 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keyword.operator maybe? That's what I'd use.

# ^^^^^ string.unquoted.value

# ESCAPES TEST
foo = "\\ \" \b \n \t"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also add invalid.illegal for any other escapes like @jrappen suggested?

Copy link
Contributor Author

@ryboe ryboe Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could make an invalid.illegal rule like this:

- match: \\[rfv]
  scope: invalid.illegal.bad-escape.gitconfig

Unfortunately, this would mismatch valid text like C:\Users\rob.

If the user intends \rto mean a single char, that's a mistake. It would be nice if we could catch it. However, it might also be intended as two chars: a \ followed by an r. In that case it would be valid text. I can't think of a way to distinguish the two cases. The current behavior might be the best we can do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This:

[x]
    a = C:\Users\rob

prints

\fatal: bad config line 14 in file /Users/raoulwols/.gitconfig

while this

[x]
    a = C:\\Users\\rob

prints:

C:\Users\rob

but this is on macOS, not sure how git treats this on Windows?

1: punctuation.definition.comment.gitrebase
push:
- meta_scope: meta.rebase-msg.gitrebase
- match: '(?=\n)'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can replace this with just a single dollar sign ($)

rebase-msg:
- match: '\s*Rebase\s+'
captures:
1: punctuation.definition.comment.gitrebase
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

- meta_scope: meta.commit.gitrebase
- match: \n
pop: true
- match: \b({{hash}})\s(.*) # e.g. d284bb2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always weary of these "match anything" regexes. Maybe just match \b{{hash}}\b and then push onto a new context?


# e.g. pick d284bb2 Initial commit
commit:
- match: ^\s*(squash|reword|pick|fixup|exec|edit|drop|x|s|r|p|f|e|d)\s
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe replace the right-most \s with \b?

Copy link
Collaborator

@michaelblyons michaelblyons Sep 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If pick'0123abcd commit text isn't valid, do you want a \b instead of \s? This is a case where \s [\t ] makes sense to me.

1: punctuation.separator.change-list.gitcommit

comments:
- match: '#'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you thought about this?

[foosection]
bar = baz#qux
one = two #three

Does bar have the value baz or does it have the value baz#qux?

Copy link
Contributor Author

@ryboe ryboe Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Hmm, currently it's matching as a value, ever since I changed this match from \S to \S+.

comment_or_value

I'm not sure if this is the correct behavior, or if it should match as a comment. If it should be a comment, I could change \S+ to [^#;\s]+.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and tested this here (git version 2.11). With

[x]
    a = b#c

the output of

git config --get x.a

is

b

Copy link
Contributor

@rwols rwols Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover,

"b#c"    ---> b#c
\"b#c\"  ---> "b
\"       ---> "

Add sublime-syntax definitions for gitconfig, commit, tag, merge, rebase.
The new syntax definitions don't trigger Oniguruma.
Add syntax tests.
Add Comments and Symbol List.tmPreferences.
Add snippet for creating a gitconfig [section].
@wbond
Copy link
Member

wbond commented Sep 15, 2017

I think this definitely has some merit. I'll run it by Jon, but I am inclined to include it.

deathaxe and others added 3 commits February 8, 2018 18:38
All .gitconfig files use indented key-value pairs. This commit adds indention rules to automatically adapt to this behavior.
An little more exhaustive escape pattern in combination with dropping support for comments starting with `;` after values, it is possible to fix a couple of highlighting issues reported during the last few days.
Git Config: add fixes for embedded multiline shell scripts and indention rules
@michaelblyons
Copy link
Collaborator

It is back in a working state, so far as my purposes are concerned.

@wbond
Copy link
Member

wbond commented Feb 9, 2018

You guys have done an amazing amount of work and collaboration here.

Rather than trying to digest everything that has happened, how does everyone feel about the current state of this PR?

@deathaxe
Copy link
Collaborator

deathaxe commented Feb 10, 2018

I did not find any issues while working with this package during the last few months nor did I find things to optimize much. With the embedded shellscript issue being fixed, I'd call it stable.

The syntaxes have been tested against a couple of large .gitattributes and .gitignore collections found on github and various .gitconfig files.

One remaining regression to fix is the commit bcd2798 to have placed the indention rules into the wrong directory.

@ryboe
Copy link
Contributor Author

ryboe commented Feb 11, 2018

I've been using this for months with zero issues 😀. I don't think it makes sense to keep these improvements from users for much longer. They're more likely to be missing syntax highlighting of their gitconfigs than they are to experience bugs from edge cases. Thanks to @deathaxe's heroic work, and the testing and reviews from everyone in this thread, I feel this package has gotten extremely solid.

@jrappen
Copy link
Contributor

jrappen commented Feb 28, 2018

Same here, no issues for months. Merge this for next build?

/cc @wbond

@wbond
Copy link
Member

wbond commented Mar 17, 2018

I'm inclined to name this package "Git Editor" - thoughts?

I think Git Files feels odd, whereas it seems most of these syntaxes are related to using Sublime Text as the git editor.

@FichteFoll
Copy link
Collaborator

FichteFoll commented Mar 17, 2018

I don't see "Git Files" as odd, considering package names usually represent what kind of syntax they are used for (e.g. "Rust", "Java"). Since "Git" is already taken on PC, "Git Files" is a good alternative as it declares the intent of the package providing syntax highighting for files used by git, albeit some of temporary nature like interactive rebases.

"Git Editor" to me seems to indicate the package provides more than just syntax highlighting and I don't think this is within the scope of a default package?

(Granted, it does include completions, but that's still pretty basic.)

@deathaxe
Copy link
Collaborator

I guess the name depends on the possible future use of that package. All default packages are named by the language they are meant to be used for and include only one or two syntaxes where "Git Files" contains several related but different syntaxes/languages.

Here are my 2 cents about it:

  1. "Git Files" or "Git Editor" both contain a whitespace in the name. The only other default package doing so is "Regular Expressions". In case the package may contain python files which may be imported by 3rd party packages, the names should possibly not contain spaces. "GitFiles" or "GitEditor" even look better next to packages like GitSavvy or GitGutter. ;-)

  2. "GitEditor" sounds like a sophisticated 3rd party package with a couple of advanced functions to help working with git which opposes the intention of default packages to provide only basic support for syntax highlighting and maybe some completions. By adding some helper functions for handling git-rebase-todo or others the package could on the other hand quickly add some editor functionality, which would turn the name into an alternative to think about.

  3. The package contains many syntaxes compared to the other default packages. To keep future proof each syntax should possibly get its own folder/(sub-)package like PackageDev does with all the different ST related files. So each function being added in the future could be placed into the folder of the syntax it targets. I did some experiments about it here. But I am not sure about introducing another level of sub-packages to be a good idea. An alternative would be to split "Git Files" into several packages like "GitConfig", "GitIgnore", "GitAttributes", ... with "GitCommon" as the only internal library like package. This way each package would target one syntax as all the others, but the common scope of the package could get lost. Caution: I did not check for existing packages already using these names.

In the end this PR is a suggestion only. The final decision about he name and (file-)structure is up to @jps and @wbond as the main maintainers who know about the future direction of the journey.

I hope not to arise too many new questions ;-) with those of my thoughts.

@wbond
Copy link
Member

wbond commented Mar 18, 2018

I like the point about Git Editor. I don’t think at the current time we are planning any plugin functionality for this package.

I’m not keen on Git Files because it sounds like “Git Stuff” to me.

Perhaps “Git Formats”?

@jrappen
Copy link
Contributor

jrappen commented Mar 18, 2018

What about using „Git“ and asking @kemayo as the maintainer of the „Git“ package to contribute to core going forward?

I believe having strong support for the default packages benefits everyone at the end of the day, this should include top third party developers merging their work into the default packages.

I’d even go as far as merging this as a first step for the default “Git” package and then building on top of this to make it a replacement for the current third packages:

Yes, I’m aware of the differences between the packages.

This is the start of making the core experience while working with git repositories so much better, though. It is a good first step. We should use the name “Git” and all focus our work on core going forward.

@kemayo
Copy link

kemayo commented Mar 18, 2018

To wbond's point, "Git Files" sounded to me like it was going to be a package focused on showing the files-tracked-by-git, rather than a collection of syntax stuff. I also agree that "Git Editor" sounds like a package with actual repo-management behavior. "Git Formats" or "Git Syntax"? My sole objection to "Git" as a name is that it'd cause a bunch of namespace conflicts with people who're using the existing package, and since it's fairly popular that's a big pile of support requests to deal with.

As for merging in the various other git packages, as one of those maintainers I'm not conceptually opposed to it. (Though mine has a certain been-around-for-ages level of cruft, and might benefit from a fresh reimplementation to take proper advantage of the last 7 years of API improvements, and the dropping of ST2 compatibility.)

That said, one benefit of the current arrangement is that a lot of "why doesn't git work???" support request stuff winds up going to third-party developers, rather than causing more work for the core developers. If it goes into core, I feel there's a higher standard of working everywhere that needs to be met, as opposed to my current level of "it sounds like git isn't configured properly; you should look into that".

@rwols
Copy link
Contributor

rwols commented Mar 18, 2018

That said, one benefit of the current arrangement is that a lot of "why doesn't git work???" support request stuff winds up going to third-party developers, rather than causing more work for the core developers.

Right now this only does syntax highlighting. I doubt you'd get any technical nagging from users.

We should use the name “Git” and all focus our work on core going forward.

This feels like a step that's too much ahead. As it stands this does syntax highlighting and I think lots of people would be fine with that. I would be hesitant to include more functionality. Let's keep that to the Git(?:Savvy|Gutter)? plugins.

@kemayo
Copy link

kemayo commented Mar 18, 2018

Right now this only does syntax highlighting. I doubt you'd get any technical nagging from users.

Yeah, sorry, I was directing that comment at the "we should have more full-featured Git support in core" line of discussion, not the actual current state of this request.

@wbond
Copy link
Member

wbond commented Mar 19, 2018

So does anyone feel that "Git Formats" is a bad name?

@FichteFoll
Copy link
Collaborator

FichteFoll commented Mar 19, 2018

Works for me. Though the "formats" is kind of weird, it should be obvious that it can't be anything else since "git format" isn't something that exists.

@ghost
Copy link

ghost commented Mar 20, 2018

@wbond @kemayo How about renaming Git to something like GitIntegration, GitExtra or GitAwesome and notify the users about the renaming decision. And name default package as Git. That scenario could be repeated in the future if there will be another new default syntax, e.g. SCSS support out of the box.

@wbond
Copy link
Member

wbond commented Mar 20, 2018

@ihodev Unfortunately that would be a lot of work and coordination, and some users don't have Package Control set to do automatic upgrades. I'd rather choose an option with less work and disruption so we can continue to make progress on other core issues.

@wbond wbond merged commit d23339e into sublimehq:master Mar 20, 2018
@wbond
Copy link
Member

wbond commented Mar 20, 2018

Thanks for the work on this @y0ssar1an and @deathaxe, and the reviews @rwols, @FichteFoll, @keith-hall, @michaelblyons and @jrappen!

@ryboe ryboe deleted the add_gitconfig_pkg branch March 21, 2018 18:36
@deathaxe
Copy link
Collaborator

deathaxe commented Mar 22, 2018

I’d even go as far as merging this as a first step for the default “Git” package and then building on top of this to make it a replacement for the current third packages

@jrappen: According to Package Control, there are about 12M users, 0,8M use Git package, 0,6M use GitGutter. With other words: Only about 6% use git. Not enough to merge it to core, I think, compared to the complexity the and limited scope of use of the packages.

Such packages should keep standalone.

Instead of merging packages which are made for one single VCS and try to create some kind of text based GUI (GitSavvy), a more general approach to enable plugin developers to integrate any VCS would be nice. Highlighting files in the tree, adding function icons next to tree items or dedicated lists for modified files could help. But all of it needs to be designed carefully. Even though VS Code's approach of integrating VCS is quite good, I find it too much in some cases.

@hkdobrev
Copy link

hkdobrev commented Apr 3, 2018

What's the difference between the git configuration file and an ini file?

@deathaxe
Copy link
Collaborator

deathaxe commented Apr 3, 2018

  1. .gitconfig supports subsections like
[section "subsection"]
  1. .gitconfig supports highlighting the color values and it embeds the bash syntax to highlight aliases etc.

  2. Not to forget: ST doesn't provide support for INI-files out of the box ;-)

deathaxe pushed a commit to deathaxe/sublime-packages that referenced this pull request Jun 9, 2019
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.

[Git Files] Add a package for highlighting git files
10 participants