-
Notifications
You must be signed in to change notification settings - Fork 158
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
Deprecate the CommandRunner class #442
Comments
This is quite an epic issue and in the interest of reporting my progress so far, here are some notes on my attempt to perform this migration. Potential replacementsThe following is a table of the various CommandRunner usages and their potential replacements in JGit:
Resources
EvaluationPros
Cons
Notes JGit has quite some potential in replacing our use of CommandRunner and also provides better error handling in general. However, as it requires us to be interacting with the API directly, we also need to replace the regex "hacks" that were developed over time with the proper API function calls, which is not trivial especially without the understanding of why certain information is needed in the first place. The "new" exceptions also need to be handled, which will involve knowing how the various parts of the software interact with one another. Our implementation of CommandRunner also features running commands asynchronously, which at present is only used for cloning repositories. However, it does not seem that JGit is capable of providing such a functionality, which can be a blocker in replacing the existing Git cloning functionality. JGit itself is not a complete implementation of Git yet, so there may be some features that are not available, if we ever need them in the future. This will also increase our project's dependency on this plugin. It is also worth checking out libgit2, which is an alternative implementation of Git core and has an experimental binding in Java called jagged. If this is a step in the right direction, I will try to replace the various usages of CommandRunner incrementally. This task would track that progress. I have also provided an example of what JGit can potentially do in PR #1454, starting with the GitBlame.java file. |
@damithc Your input for this change? It is quite a big change though. We should not worry about the feature of JGit can provide as it is still being maintained quite often. I also agree that we may need to do some regex hack to make it work for JGit. Maybe we can pack a Git binary and use it if the user does not have local git installation? |
Given that we already has a working implementation, we'll need a pretty strong reason to migrate i.e., either something very negative about the current choice or a big value added by the alternative. |
I think if we manage to migrate to JGit, we would be able to avoid using Git itself, so that we can skip the step of having the user to install Git on their local machine.
Currently, the main selling point is better error handling, as the environment is better controlled compared to using the existing CommandRunner approach which could have many different possible Git versions and configurations being used. The main cost is having to redevelop the Git backend (on a similar scale as the projectify frontend issue), although I believe it can be done incrementally without significant disruptions to other aspects of the codebase. I personally would prefer if we can start the switch over to JGit, as I feel that we would eventually reap the benefits of better stability in the long run. However, it also depends on whether it is aligned to the long-term goals/roadmap of this project, and whether there are other more pressing issues to work on. |
Leaving this task up for grabs. This task is ideal for an FYP project. |
Thanks for the initial investigation @dcshzj Yes, let's put it on hold for the time being given that it requires rewriting a lot of code and we are not 100% sure if all the current features can be supported with JGit. We can revisit this later. |
@dcshzj Thanks for your effort on this issue. |
(Rest omitted for brevity) The latest JGit version as of today is That being said, JGit |
Our tool has the pre-requisite of existing git installation and command line dependency which makes it more tedious to set up compare to a general tool.
Additionally, this reduce the maintainability due to security issues such as command injection, and may also prevent us from developing it into a integrate-able tool such as gradle plugin.
Let's move towards alternative such as jgit and deprecate the use of terminal / command prompt.
The text was updated successfully, but these errors were encountered: