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

Project save backed by git #3851

Merged
merged 18 commits into from
Nov 14, 2022
Merged

Conversation

hubertp
Copy link
Contributor

@hubertp hubertp commented Nov 3, 2022

Pull Request Description

This change adds support for Version Controlled projects in language server.
Version Control supports operations:

  • init - initialize VCS for a project
  • save - commit all changes to the project in VCS
  • restore - ability to restore project to some past save
  • status - show the status of the project from VCS' perspective
  • list - show a list of requested saves

Important Notes

Behind the scenes, Enso's VCS uses git (or rather jGit) but nothing stops us from using a different implementation as long as it conforms to the establish API.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@hubertp hubertp marked this pull request as ready for review November 10, 2022 18:59
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Looks like a standard PR for language server. Two things I find strange:

  • Akka is a an actor framework, there should be no blocking waiting - but there is. Why?
  • JGit is added as a dependency of the whole language-server project - that is not modular enough - we should have more compilation units and then just assemble them in runtime.

None of my comments is critical. Hence approving anyway.

import org.enso.jsonrpc.{Error, HasParams, HasResult, Method, Unused}
import org.enso.languageserver.filemanager.Path

object VcsManagerApi {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this whole VCS related functionality be in its own module? Something like engine/language-server-vcs? It looks like a quite independent sub-system with its own dependency needs. Separating it into an independent compilation unit would prevent other parts of language-server code to use JGit API directly when it is supposed to be hidden.

Option1: Define the VcsApi in 'language-servermodule without any dependencies on JGit. Implement it in thelanguage-server-vcsimplmodule that will have dependency on JGit as well aslanguage-server` module. Use ServiceLoader to connect them together.

Option2: Define the VcsApi in a separate module language-server-vcs. Implement it in language-server-vcsimpl module that will have dependency on JGit as well as language-server-vcs module. Change the language-server module to depend on language-server-vcs, use its API. The API module would use ServiceLoader again to find the right implementation during runtime.

Copy link
Contributor Author

@hubertp hubertp Nov 11, 2022

Choose a reason for hiding this comment

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

Seemed like a bit of an overkill at this point. Mostly because we don't use any other VCS implementation right now. If we had more implementations, I would totally agree with you.
Similar thing can be said about other elements of the language server like file manager and all buffer-related stuff.

Copy link
Member

Choose a reason for hiding this comment

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

Similar thing can be said about other elements of the language server

Yes. There are many other violations, but when will we start fixing them if not when introducing completely new library? Or will we continue building such a megalitic monolith forever?

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 sounds to me more like a regular refactoring action that could be applied to different subprojects of language-server. Mixing that work with project save done here, would make review rather difficult.

So how about we schedule that work with @4e6 separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@4e6 4e6 left a comment

Choose a reason for hiding this comment

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

Love the fact that you removed the restriction on unique commit name ❤️

Approved, just don't forget to update the protocol-language-server.md API doc with the new VCS methods before merging.

@hubertp hubertp force-pushed the wip/hubert/git-backed-save-183661786 branch 2 times, most recently from 9d30439 to fd90ffa Compare November 14, 2022 09:22
This change adds VcsManager which is responsible for all version control
operations. All operations were abstracted and jGit usage is well hidden
behind abstractions.

This is an initial support, vcs handlers will likely change once
interaction with filemanager/buffers is figured out.
Bugfixes for various corner cases than can happen. Extended coverage for
those in our test cases.
Added initial json tests for VcsManager.
Ensured that all commit requests, first ensure that all pending saves to
the files of the project are done. That way, we are not left with
pending file modifications **after** the project was explicitly saved.
Added a test to illustrate the scenario.

Note that one occassion we have to carry around the sender explicitly,
because context changes it for the message once we want to forward it.

Added some actor names, so that it is easier to debug potential issues
with messages.
Rather than referring to names, one should rather refer to commit SHAs.
API docs revealed a few inconsistencies. This change resolves both.
Autosave on close was an additional requirement that fits the "save
always" approach. When a request is made, we check if there are any
pending saves from the client and force them, if necessary.
@hubertp hubertp force-pushed the wip/hubert/git-backed-save-183661786 branch from 2cd2945 to df58cdb Compare November 14, 2022 12:36
@hubertp hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Nov 14, 2022
@hubertp hubertp changed the title Save backed by git Project save backed by git Nov 14, 2022
@mergify mergify bot merged commit 85d4337 into develop Nov 14, 2022
@mergify mergify bot deleted the wip/hubert/git-backed-save-183661786 branch November 14, 2022 17:32
hubertp added a commit that referenced this pull request Nov 25, 2022
Rather than hardcoding `.git` in the root of the project, VCS should
save data into Enso's data directory (i.e. `.enso`).
This change reshuffles initialization and configuration to store Git VCS
metadata by default at `.enso/.git`.
This is follow up to #3851
hubertp added a commit that referenced this pull request Nov 28, 2022
Rather than hardcoding `.git` in the root of the project, VCS should
save data into Enso's data directory (i.e. `.enso`).
This change reshuffles initialization and configuration to store Git VCS
metadata by default at `.enso/.git`.
This is follow up to #3851
mergify bot pushed a commit that referenced this pull request Nov 29, 2022
Rather than hard-coding `.git` in the root of the project, VCS should save data into Enso's data directory (i.e. `.enso`).
This change reshuffles initialization and configuration to store Git VCS metadata by default at `.enso/.git`.
This is follow up to #3851

# Important Notes
Apparently a custom Git directory in JGit means that it always creates a `.git` **file** with `gitdir` pointing to the custom location.
This is not necessary in our case since all our commands provide that explicitly.
That is why `init` operation removes `.git` file, which may seem a bit counter-intuitive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants