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

[#442] Replace CommandRunner with JGit for GitBranch #1454

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dcshzj
Copy link
Member

@dcshzj dcshzj commented Feb 13, 2021

Part of #442.

CommandRunner should be deprecated and replaced by JGit as it
requires Git to be installed and has poor exception handling.
JGit is able to handle Git operations natively, which makes it
more robust to use.

Let's replace usages of CommandRunner with their equivalents in
JGit, starting with GitBranch.

This PR is a proof-of-concept of what JGit can potentially do, instead of relying on CommandRunner.

CommandRunner should be deprecated and replaced by JGit as it
requires Git to be installed and has poor exception handling.
JGit is able to handle Git operations natively, which makes it
more robust to use.

Let's replace usages of CommandRunner with their equivalents in
JGit, starting with GitBranch.
@@ -68,11 +68,18 @@ public RepoLocation getClonedRepoLocation() {
String bareRepoPath = FileUtil.getBareRepoPath(configs[currentIndex]).toString();
currentRepoDefaultBranch = GitBranch.getCurrentBranch(bareRepoPath);
} catch (GitBranchException gbe) {
// GitBranch will throw this exception when repository is empty
logger.log(Level.WARNING, String.format(MESSAGE_ERROR_GETTING_BRANCH,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this given you have added code for this already? Or did you forget to remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't remove this exception handling as I think that there might be some other errors that may occur. For instance, GitBranchException is thrown because an IOException occurred, which probably happened if the directory given is not a Git repository (which shouldn't happen).

@damithc
Copy link
Collaborator

damithc commented Feb 15, 2021

Good to do a light exploration of this direction at first before going hard at it. For example, we can try to get it working first before deciding whether to merge (hence, no need for a detailed review of the PR first)
I'm particularly worried about not being able to use the all/latest features/improvements of Git and not particularly worried about having to install Git first. It's not unreasonable to expect users to have Git if they want to run RepoSense locally.
If we decide not to go forward, we should still document pros and cons found.

@dcshzj
Copy link
Member Author

dcshzj commented Feb 15, 2021

I have updated my original comment in #442 with a pros and cons list based on the research that I have done on JGit, so that we can make a more informed decision.

@fzdy1914
Copy link
Member

As the discussion, I will make it DoNotMerge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants