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

Fix **partial** 578 - Git integration #10422

Closed
wants to merge 20 commits into from

Conversation

lbenicio
Copy link

@lbenicio lbenicio commented Sep 26, 2023

This is a partial pull request just to track down the development progress and to any suggestions.

Here we want to outline:

  1. We implemented both SSH and Username/Password auth method
  2. Added support to local and remote git repositories
  3. Stills needs to add support for GPG signing
  4. Stills needs to add user interface messages on git errors
  5. Stills must implement unit testing
  6. Stills has to update documentation
  7. The Credentials input dialog box should be updated. Currently, we open two dialog boxes, asking for username and then password

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@Siedlerchr Siedlerchr changed the title Fix partial 578 Fix partial 578 - Git integration Sep 26, 2023
@lbenicio lbenicio changed the title Fix partial 578 - Git integration Fix **partial** 578 - Git integration Sep 26, 2023
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Small comments.

I strongly do advice you use JUnit for development, too.

IntelliJ offers a play button at each test case. Thus, debugging is very easy!

String gitPassword = "";

if (dialogService != null) {
gitUsername = dialogService.showInputDialogAndWait(Localization.lang("Git credentials"), Localization.lang("git username")).get();
Copy link
Member

Choose a reason for hiding this comment

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

Please check how JabRef handles proxy credentials. Username/password stored in preferences. You should do that, too,

@@ -243,6 +243,11 @@ private boolean save(Path targetPath, SaveDatabaseMode mode) {
libraryTab.resetChangedProperties();
}
dialogService.notify(Localization.lang("Library saved"));

if (success) {
SaveGitDatabaseAction saveGit = new SaveGitDatabaseAction(targetPath.getParent(), dialogService);
Copy link
Member

Choose a reason for hiding this comment

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

Please commit and push the current file only. Nothing else!

build.gradle Outdated
@@ -128,7 +128,7 @@ dependencies {
// required for reading write-protected PDFs - see https://github.com/JabRef/jabref/pull/942#issuecomment-209252635
implementation 'org.bouncycastle:bcprov-jdk18on:1.76'

implementation 'commons-cli:commons-cli:1.5.0'
implementation ('commons-cli:commons-cli:1.5.0')
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the style of the build.gradle file. --> no braces here.

Comment on lines 31 to 33
String gitUsername = Optional.ofNullable(System.getenv("GIT_EMAIL")).orElse("");
String gitPassword = Optional.ofNullable(System.getenv("GIT_PW")).orElse("");
final CredentialsProvider credentialsProvider = new UsernamePasswordCredentialsProvider(gitUsername, gitPassword);
Copy link
Member

Choose a reason for hiding this comment

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

This change will break the SLR feature, doesn't it?

Maybe, you can also rely that a user provided username and password in the environment variables GIT_EMAIL and GIT_PW.

}
}
}

/**
* Get currently checked out branch.
* If checking out fails, it fails silently.
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any checkout call there. -- Maybe you mean something else? Maybe demonstrate using a @Test?

@lbenicio lbenicio closed this Oct 8, 2023
@Siedlerchr
Copy link
Member

Why did you close the PR?

@koppor
Copy link
Member

koppor commented Oct 22, 2023

This PR implements koppor#578

@koppor
Copy link
Member

koppor commented Oct 25, 2023

A new PR was submitted --> #10586

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.

None yet

4 participants