Implement breaking lints in the LSP #3404
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
private/buf/buflsp/file.go
Outdated
| if errors.Is(err, git.ErrRemoteNotFound) { | ||
| // This is an invalid remote, so it means gitRemote should be | ||
| // interpreted as a local branch name instead. | ||
| f.gitRemoteBranch = f.gitRemote | ||
| f.gitRemote = "" | ||
| f.lsp.logger.Debug( | ||
| "found local branch", | ||
| slog.String("uri", string(f.uri)), | ||
| slog.String("ref", f.gitRemoteBranch), | ||
| ) | ||
| } else if err != nil { |
There was a problem hiding this comment.
This behaviour is somewhat reasonable, but I wonder if branch + remote should just be different config fields with sane defaults for clarity. Even if this is documented behaviour, the config field being named gitRemote but pointing to a local branch might cause some confusion. This would have cascading changes through the fileOpener, etc.
There was a problem hiding this comment.
I think I'm going to change the semantics as follows:
- againstKind will be
againstGitoragainstSaved. - There will be a single config knob for the ref to use for
againstGit, with a default oforigin/HEAD.
private/pkg/git/git.go
Outdated
| for { | ||
| _, err := os.Stat(filepath.Join(dir, ".git")) | ||
| if err == nil { | ||
| break | ||
| } else if errors.Is(err, os.ErrNotExist) { | ||
| parent := filepath.Dir(dir) | ||
| if parent == dir { | ||
| return nil, fmt.Errorf("could not find .git directory for %s: %w", orig, ErrNotARepo) | ||
| } | ||
| dir = parent | ||
| } else { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
We could use CheckDirectoryIsValidGitCheckout to check if this is a git repository:
Line 215 in b886118
The call-site will need to handle ErrInvalidGitCheckout instead.
There was a problem hiding this comment.
That's not quite what we want. We want to get the repository root, which requires traversing upwards (vs IsValidCheckout, which just kicks rev-parse).
This change adds breaking lints to the LSP as part of the general lint pass.
Breaking lints are interesting because they need an "against". I've chosen to expose two configuration knobs (which are set in the settings for an LSP in the standard way):
buf.againstsets the against strategy. For the LSP, there are three that reasonably make sense: against some remote's HEAD, against your local HEAD, and against whatever is on disk. This means that lints go away when pushing to main, committing working changes, or saving in the editor, respectively.buf.gitRemote, the remote to use for the above. "origin" is the default for almost everyone, but it need not be that, so it's a config knob.Also, in the process of standing up loading config settings from the client, I found a deadlock in the
jsonrpc2library. To work around it, every request runs on its own goroutine (although they are still serialized wrt to the big LSP lock). This ensures that the single goroutinejsonrpc2spawns to actually poll the IPC socket doesn't deadlock. Why do they only spawn one goroutine? Presumably because they expect users to spawn a goroutine to handle each request, but this does not appear to be visibly documented. (Much debugging of deadlocked goroutines happened today.)