Add git support #10586

lbenicio · 2023-10-25T22:04:55Z

Fixes https://github.com/koppor/jabref/issues/578

SSH auth method
Username/password dialog box auth method
Username/password env variables auth method
Store username/password in JabRef preferences
Fail silently
Support for local and remote git repositories

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.

lbenicio · 2023-11-25T14:53:31Z

We are migrating our implementation to use java key ring, we were not using it. Now we see the problem. The diff dialog is still under heavy issues (I added the files from the database diff but still difficulty to navigate how to change it to git diffs). It seems more challenging them what we have thought.

koppor · 2023-11-27T09:07:34Z

@lbenicio you can replace the whole diff dialog by using JabRefs own feature to compare two libraries (bib files). Remember, the is the "file changed on disk" feature of JabRef. Maybe, you simply write out the file of the remote to the disk and then let JabRef do it's magic. The magic includes a) displaying all diffs and b) guiding the user through accepting, rejecting, and merging changes.

lbenicio · 2023-11-28T15:47:31Z

@koppor We thought, as a v1, we could just create a text dialog with the diff output from the user and let the user decide

koppor · 2023-11-28T19:50:01Z

@lbenicio As I understand you, the dialog does not work. What I say: Remove that code, add approx. three lines of code and let JabRef do its magic. For sure, you can train your JavaFX skills with your dialog. Then please add screenshots!

lbenicio · 2023-11-29T20:21:03Z

I don't quite understand how that will work as the class uses bib files, but i will try.

marcelojunior1 · 2023-11-30T01:32:41Z

@lbenicio you can replace the whole diff dialog by using JabRefs own feature to compare two libraries (bib files). Remember, the is the "file changed on disk" feature of JabRef. Maybe, you simply write out the file of the remote to the disk and then let JabRef do it's magic. The magic includes a) displaying all diffs and b) guiding the user through accepting, rejecting, and merging changes.

I tried to pull before save the bib file, but i think file monitor is more slow than save action. Is it possible to check in file monitor if a conflict happened and if user resolved it ?

koppor · 2023-12-04T12:33:15Z

I tried to pull before save the bib file, but i think file monitor is more slow than save action.

The file monitor monitors the file on the disk. Thus, it cannot be slower than the save action.

Is it possible to check in file monitor if a conflict happened and if user resolved it ?

No. There are other listeners taking care of the work. For the first "MVP", it is enough to save the pulled file and let the user to the work - without going back the path of the save call. The handling of conflicts will be done in another independent thread then.

koppor · 2024-01-02T21:00:05Z

Happy new year! 🎉

This PR could take longer to be finished. Do you still have some time?

If yes, I would ask you to check the CI output: Checkstyle complains and some other checkers, too. Please check their output. Especially reconfigure IntelliJ to follow JabRef's code style. See https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace/intellij-13-code-style.html for details. You can then use Ctrl+Alt+L to reformat your classes.

marcelojunior1 · 2024-01-14T01:22:44Z

I make the changes, but I don't understand this changelog.md error.

koppor · 2024-01-14T16:27:30Z

I make the changes, but I don't understand this changelog.md error.

Not sure, how I should explain.

You know software releases? You know that for each version, there is a section in CHANGELOG.md. Thus, for each released version, there is a section.

What if updates to the software come? Are older releases changed? No, new releases are made! - What about the CHANGELOG.md sections of older releases? Do new features go into these sections? No, a new section unreleased is used.

Now, let's check your patch:

The patch is in section of version 5.11 of JabRef. It adds a new feature to 5.11. BUT - hey, JabRef 5.11 is already released. See the release date in the heading and the releases at https://github.com/JabRef/jabref/releases.

Thus, this patch should NOT go into 5.11, but into the unreleased section of CHANGELOG.md

marcelojunior1 · 2024-01-14T23:35:31Z

All right, and this last one, is it related to what we did?

koppor · 2024-01-22T17:09:05Z

All right, and this last one, is it related to what we did?

I don't understand, where "last one" refers to...

marcelojunior1 · 2024-01-22T20:25:46Z

These Fetcher tests.

Siedlerchr · 2024-01-22T22:30:36Z

You can ignore the fetcher tests Marcelo Nascimento ***@***.***> schrieb am Mo., 22. Jan. 2024, 21:25:

…

These Fetcher tests. — Reply to this email directly, view it on GitHub <#10586 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACOFZDHVKABZOHT7XPOVNDYP3DNLAVCNFSM6AAAAAA6QCNEUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBUG42DQNBXGM> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

koppor

Some small comments. Still need to try it thoroughly...

koppor · 2024-02-28T22:47:51Z

src/main/java/org/jabref/logic/git/SshTransportConfigCallback.java

+            try {
+                return Files.createDirectories(sshDir).toFile();
+            } catch (IOException e) {
+                throw new RuntimeException(e);


JabRef should not crash completely if something goes wrong when using SSH. Better really throw the exception and handle at the caller.

koppor · 2024-02-28T22:48:37Z

src/main/java/org/jabref/logic/git/GitHandler.java

+            }
+        } catch (GitAPIException | IOException e) {
+            LOGGER.error("Failed to force git pull", e);
+            throw new RuntimeException(e);


JabRef should not be stopped (data loss!!) if some communication goes wrong. Handle differently.

koppor · 2024-02-28T22:49:14Z

src/main/java/org/jabref/logic/git/GitHandler.java

        try (Git git = Git.open(this.repositoryPathAsFile)) {
            return git.getRepository().getBranch();
+        } catch (IOException ex) {
+            LOGGER.info("Failed get current branch");
+            return String.valueOf(Optional.empty());


What is this? You can return the empty string (""), but a string of Optional.empty is really strange.

koppor · 2024-02-28T22:54:35Z

src/main/java/org/jabref/preferences/JabRefPreferences.java

@@ -482,6 +487,7 @@ public class JabRefPreferences implements PreferencesService {
    private ColumnPreferences searchDialogColumnPreferences;
    private JournalAbbreviationPreferences journalAbbreviationPreferences;
    private FieldPreferences fieldPreferences;
+    private GitPreferences gitPreferences;


What I meant that multiple providers should be supported. See #10936 for an example for API keys (which are different for different providers). Maybe, this is a follow-up thing.

Note that GitHub supports both HTTP and SSH:

koppor · 2024-02-28T22:54:59Z

src/main/java/org/jabref/preferences/JabRefPreferences.java

@@ -1614,12 +1626,27 @@ private String getProxyPassword() {
            } catch (PasswordAccessException ex) {
                LOGGER.warn("JabRef uses proxy password from key store but no password is stored");
            } catch (Exception ex) {
-                LOGGER.warn("JabRef could not open the key store");
+                LOGGER.warn("JabRef could not open the git key store");


Wrong change. JabRef also stores other credentials there. Please revert.

koppor · 2024-02-28T22:56:38Z

src/main/resources/l10n/JabRef_en.properties

+Git\ password=Git password
+Git\ username=Git username


Maybe, one can get rid of this and only use "Username" and "Password", which should already exist here?

koppor · 2024-02-28T22:57:59Z

src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java

+            LOGGER.info("Git credentials error");
+        } catch (GitAPIException e) {
+            LOGGER.info("Failed to pull");
+            throw new RuntimeException(e);


Do not crash complete JabRef on an error.

koppor · 2024-02-28T22:58:16Z

src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java

+            LOGGER.info("No remote repository detected");
+        } catch (DetachedHeadException e) {
+            dialogService.showErrorDialogAndWait(Localization.lang("Git"), Localization.lang("Git detached head"));
+            LOGGER.info("Git detached head");


Why not returning "false" if something goes wrong?

koppor · 2024-02-28T22:59:30Z

src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java

+            dialogService.showErrorDialogAndWait(Localization.lang("Git"), Localization.lang("Local repository is out of date, please review the library and save again"));
+            LOGGER.info("Failed to pull");
+            return false;
+        } catch (NoRemoteRepositoryException e) {


I think, all exceptions can be merged into one, because there is no "real" different behavior.

NoRemoteRepositoryException | DetachedHeadException etc..

Please use notifications instead of dialogs. Reason: The flow of the user should not be disturbed while working with git.

koppor · 2024-02-28T23:00:56Z

src/test/java/org/jabref/logic/git/GitHandlerTest.java

+                repository.incrementOpen();
+                return repository;
+            }
+            return null;


Shouldn't there be an exception?

marcelojunior1 · 2024-03-01T03:03:06Z

Unfortunately, my classes started a few days ago and I will not be able to continue this project. If there is someone else who can continue this issue, you can pass it on.

calixtus · 2024-03-01T13:15:02Z

Ok, yes, we will look into it. Thanks for your work on enhancing JabRef.

marcelojunior1 and others added 30 commits September 21, 2023 21:40

test: push

10a4779

fix build error

2c25b5f

test: push

91a12f1

fix build error

1126bc2

refactor

f3ae12e

fix: commit and push

c312817

code refactoring

7ffe7f1

add silent exception on git pull and push

905eda9

add username and password git credentials input

b104b99

fix style and git logic to add one file only

bf33ca0

add git preferences files

375e689

inject dialog service

d3a8014

add initial support to git credentials dialog view

a1e8ca0

fix: git failed silently and style

e4dd331

fix git http push

4c81e54

refactor git credentials dialog to single dialog with two inputs

df3e4bb

user and password menu without Keyring

298d86e

refact credentials view to other file

6109741

fix git pref panel

27b95bf

user git preferences when pushing new commits

e935882

remove custom method used for validation

344775b

git preferences and test

7d0b6db

add unit test for commiting creating

f3313b1

unit test git prefernces and credentials

6686905

push without auth

a102257

add test git push with credentials

a2a5a71

git push without write on disk

fdebffa

fix: start server

45cd433

fix: contextPath and req name

35949a9

fix: pushSingleFile

72c7532

marcelojunior1 added 2 commits November 24, 2023 18:14

fix: git key store

f9abb2c

Style

75bce3c

marcelojunior1 added 3 commits December 14, 2023 15:43

conflict resolution with file monitor

596275a

fix: push when status is clean

ce2e1d3

fix: comments and forceGitPull

18a3373

marcelojunior1 and others added 4 commits January 13, 2024 11:57

fix: set credentials on test and style

4ebfd85

Merge branch 'main' into fix-578

0379fad

fix: keys in language file and mock on SaveDatabaseActionTest

ceab32c

Merge branch 'main' into fix-578

873a5cc

fix: changelog

bc0fd86

Update src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java

6b5d9e1

koppor requested changes Feb 28, 2024

View reviewed changes

koppor marked this pull request as draft March 4, 2024 19:47

lbenicio closed this Mar 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add git support #10586

Add git support #10586

lbenicio commented Oct 25, 2023 •

edited by ThiloteE

Loading

lbenicio commented Nov 25, 2023

koppor commented Nov 27, 2023

lbenicio commented Nov 28, 2023

koppor commented Nov 28, 2023

lbenicio commented Nov 29, 2023

marcelojunior1 commented Nov 30, 2023

koppor commented Dec 4, 2023

koppor commented Jan 2, 2024

marcelojunior1 commented Jan 14, 2024 •

edited

Loading

koppor commented Jan 14, 2024

marcelojunior1 commented Jan 14, 2024

koppor commented Jan 22, 2024

marcelojunior1 commented Jan 22, 2024

Siedlerchr commented Jan 22, 2024 via email

koppor left a comment

koppor Feb 28, 2024

koppor Feb 28, 2024

koppor Feb 28, 2024

koppor Feb 28, 2024

koppor Feb 28, 2024

koppor Feb 28, 2024

koppor Feb 28, 2024

koppor Feb 28, 2024

koppor Feb 28, 2024

koppor Feb 28, 2024

marcelojunior1 commented Mar 1, 2024

calixtus commented Mar 1, 2024

Add git support #10586

Add git support #10586

Conversation

lbenicio commented Oct 25, 2023 • edited by ThiloteE Loading

Mandatory checks

lbenicio commented Nov 25, 2023

koppor commented Nov 27, 2023

lbenicio commented Nov 28, 2023

koppor commented Nov 28, 2023

lbenicio commented Nov 29, 2023

marcelojunior1 commented Nov 30, 2023

koppor commented Dec 4, 2023

koppor commented Jan 2, 2024

marcelojunior1 commented Jan 14, 2024 • edited Loading

koppor commented Jan 14, 2024

marcelojunior1 commented Jan 14, 2024

koppor commented Jan 22, 2024

marcelojunior1 commented Jan 22, 2024

Siedlerchr commented Jan 22, 2024 via email

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelojunior1 commented Mar 1, 2024

calixtus commented Mar 1, 2024

lbenicio commented Oct 25, 2023 •

edited by ThiloteE

Loading

marcelojunior1 commented Jan 14, 2024 •

edited

Loading