WIP: GSoC Git Support - Support semantic pull merge without conflicts #714

wanling0000 · 2025-06-18T18:29:42Z

Steps to test

This PR implements the first part of Git sync support by enabling automatic semantic merges when there are no conflicts. The scenario is tested via TDD in GitSyncServiceTest, simulating the following steps:

Alice creates an initial commit and pushes it to the remote.
Bob clones the repo and modifies entry b, then pushes.
Alice modifies entry a, then pulls.

Since they changed different entries, we expect a clean merge without user intervention. The orchestrator is GitSyncService, and helper utilities and value objects are temporarily located in org.jabref.logic.git.util.

Mandatory checks

I own the copyright of the code submitted and I license it under the MIT license
[.] Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
[.] Screenshots added in PR description (if change is visible to the user)
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.

subhramit

This was not easy - really beautiful work, Wanling. For a WIP, this is exceptionally good!
I am leaving some comments - the earlier we get habituated to some idiomatic practices (some JabRef-specific, some general), the easier it will be to stick to them during the rest of the project.

Please don't be overwhelmed by the number of comments - most of them are minor. Your code is already good, so the only way I could help make it better was by nit-picking.

subhramit · 2025-06-18T19:26:18Z

jablib/src/main/java/org/jabref/logic/git/GitSyncService.java

+    private static final Logger LOGGER = LoggerFactory.getLogger(GitSyncService.class);
+    private final ImportFormatPreferences importFormatPreferences;
+    private GitHandler gitHandler;


Nitpick: I usually separate the logger from the other variables with a newline

Suggested change

private static final Logger LOGGER = LoggerFactory.getLogger(GitSyncService.class);

private final ImportFormatPreferences importFormatPreferences;

private GitHandler gitHandler;

private static final Logger LOGGER = LoggerFactory.getLogger(GitSyncService.class);

private final ImportFormatPreferences importFormatPreferences;

private GitHandler gitHandler;

subhramit · 2025-06-18T19:29:07Z

jablib/src/main/java/org/jabref/logic/git/GitSyncService.java

+
+        // 4. Automatic merge
+        if (result.successful()) {
+            gitHandler.createCommitOnCurrentBranch("Auto-merged by JabRef", false);


Declare a constant called AMEND = true in the class and use !AMEND here to make the code more self-documenting.

subhramit · 2025-06-18T19:37:42Z

jablib/src/main/java/org/jabref/logic/git/GitSyncService.java

+    /**
+     * Called when user clicks Pull
+     */
+    public MergeResult pullAndMerge(Path bibFilePath) throws Exception {


Avoid throwing generic exceptions. Since it might be tedious work to replace them, I will leave suggestions based on how the final exception list should look like, provided you have replaced them for the internal callee methods as well.

This is just to help - do not commit suggestion directly, as you will need some new imports.

Suggested change

public MergeResult pullAndMerge(Path bibFilePath) throws Exception {

public MergeResult pullAndMerge(Path bibFilePath) throws GitAPIException, IOException, JabRefException {

subhramit · 2025-06-18T19:38:36Z

jablib/src/main/java/org/jabref/logic/git/GitSyncService.java

+                                            RevCommit baseCommit,
+                                            RevCommit localCommit,
+                                            RevCommit remoteCommit,
+                                            Path bibFilePath) throws Exception {


Suggested change

Path bibFilePath) throws Exception {

Path bibFilePath) throws IOException, JabRefException {

subhramit · 2025-06-18T20:01:51Z

jablib/src/main/java/org/jabref/logic/git/GitSyncService.java

+        MergeResult result = performSemanticMerge(git, triple.base(), triple.local(), triple.remote(), bibFilePath);
+
+        // 4. Automatic merge
+        if (result.successful()) {


Nitpick: about how code "sounds" when reading.

Hypothetically, if result was of type ResultType, ResultType being an enum for success states, we could do something like if(result == ResultType.SUCCESS) - which sounds "natural".
Since here, result is a MergeResult record instance, and we are using a(n implicit) method to evaluate, can you change the name of the record parameter to isSuccessful so that the implicit method name also changes? if(result.isSuccessful()) will read better.

In general, method names should be imperatives for actions, and predicates for boolean queries.

Thanks! I hadn't thought about how much smoother that reads. Naming really is an art 😄

subhramit · 2025-06-18T22:28:28Z

jablib/src/test/java/org/jabref/logic/git/util/GitBibParserTest.java

+        List<BibEntry> entries = context.getEntries();
+        assertEquals(1, entries.size());
+
+        BibEntry entry = entries.get(0);


Suggested change

BibEntry entry = entries.get(0);

BibEntry entry = entries.getFirst();

subhramit · 2025-06-18T22:30:12Z

jablib/src/test/java/org/jabref/logic/git/util/GitFileWriterTest.java

+        assertEquals(inputEntries.get(0).getCitationKey(), outputEntries.get(0).getCitationKey());
+        assertEquals(inputEntries.get(0).getField(StandardField.AUTHOR), outputEntries.get(0).getField(StandardField.AUTHOR));


Suggested change

assertEquals(inputEntries.get(0).getCitationKey(), outputEntries.get(0).getCitationKey());

assertEquals(inputEntries.get(0).getField(StandardField.AUTHOR), outputEntries.get(0).getField(StandardField.AUTHOR));

assertEquals(inputEntries.getFirst().getCitationKey(), outputEntries.getFirst().getCitationKey());

assertEquals(inputEntries.getFirst().getField(StandardField.AUTHOR), outputEntries.getFirst().getField(StandardField.AUTHOR));

subhramit · 2025-06-18T22:31:33Z

jablib/src/test/java/org/jabref/logic/git/util/SemanticConflictDetectorTest.java

+        BibDatabaseContext baseCtx = parse(baseCommit);
+        BibDatabaseContext remoteCtx = parse(remoteCommit);


baseDatabaseContext, remoteDatabaseContext

subhramit · 2025-06-18T22:32:31Z

jablib/src/test/java/org/jabref/logic/git/util/SemanticMergerTest.java

+        BibDatabaseContext baseCtx = GitBibParser.parseBibFromGit(base, importFormatPreferences);
+        BibDatabaseContext localCtx = GitBibParser.parseBibFromGit(local, importFormatPreferences);
+        BibDatabaseContext remoteCtx = GitBibParser.parseBibFromGit(remote, importFormatPreferences);
+
+        MergePlan plan = SemanticConflictDetector.extractMergePlan(baseCtx, remoteCtx);
+        SemanticMerger.applyMergePlan(localCtx, plan);
+
+        BibEntry patched = localCtx.getDatabase().getEntryByCitationKey("a").orElseThrow();


All Ctx -> DatabaseContext

subhramit · 2025-06-18T22:33:07Z

jablib/src/test/java/org/jabref/logic/git/util/SemanticConflictDetectorTest.java

+        BibDatabaseContext baseCtx = parse(baseCommit);
+        BibDatabaseContext localCtx = parse(localCommit);
+        BibDatabaseContext remoteCtx = parse(remoteCommit);


all Ctx -> DatabaseContext

koppor

Small comment on the process

koppor · 2025-06-18T18:56:41Z

jablib/src/main/java/org/jabref/logic/git/GitSyncService.java

+        Git git = Git.open(bibFilePath.getParent().toFile());
+
+        // 1. fetch latest remote branch
+        gitHandler.pullOnCurrentBranch();


git.fetch().call();

(because pull is fetch+merge, but JabRef should do the merge (see below)

InAnYan · 2025-06-24T16:01:56Z

jablib/src/main/java/org/jabref/logic/git/GitSyncService.java

+public class GitSyncService {
+    private static final Logger LOGGER = LoggerFactory.getLogger(GitSyncService.class);
+    private final ImportFormatPreferences importFormatPreferences;
+    private GitHandler gitHandler;


git handler could be changed? if not, then it's better to add final

InAnYan · 2025-06-25T17:53:13Z

jablib/src/main/java/org/jabref/logic/git/GitSyncService.java

+        // 4. Automatic merge
+        if (result.successful()) {
+            gitHandler.createCommitOnCurrentBranch("Auto-merged by JabRef", false);
+        }


performSemanticMerge is a function that mutates some state? What if the result of semantic merge would be unsuccessful, will there be like part of the work finished and part of the work left untouched?

Oh, as we only handle non-conflicting cases, for now, we can skip this moment

Mhm, I see. If there is a conflict, then it would not change anything, but if there are no conflicts, then the result will be successful

InAnYan · 2025-06-25T17:55:47Z

jablib/src/main/java/org/jabref/logic/git/GitSyncService.java

+        if (bibPath.startsWith(workTree)) {
+            relativePath = workTree.relativize(bibPath);
+        } else {
+            throw new IllegalStateException("Given .bib file is not inside repository");


I think we can leave the exception for now, but in future we will check beforehand if a bib file is inside a repository, right?

InAnYan · 2025-06-25T18:07:56Z

jablib/src/main/java/org/jabref/logic/git/GitSyncService.java

+
+    // WIP
+    public void push(Path bibFilePath) throws Exception {
+        this.gitHandler = new GitHandler(bibFilePath.getParent());


One interesting moment to check for: will this command find a git repository in upper-level directories?

If I use git command, while I'm in some org/jabref/logic/... folders, I still can use git commit. I just don't know whether this is supported on some standard-level, or this is a feature if git CLI application

InAnYan · 2025-06-25T18:23:43Z

jablib/src/main/java/org/jabref/logic/git/util/SemanticMerger.java

+
+    /**
+     * Implementation-only merge logic: applies changes from remote (relative to base) to local.
+     * does not check for "modifications" or "conflicts" — all decisions should be handled in advance by the SemanticConflictDetector


Small moment:

Suggested change

* does not check for "modifications" or "conflicts" — all decisions should be handled in advance by the SemanticConflictDetector

* does not check for "modifications" or "conflicts" — all decisions should be handled in advance by the {@link SemanticConflictDetector}

wanling0000 and others added 6 commits June 18, 2025 18:59

init: create the test case for git sync

b62a1bf

init: create the test case for git sync

d70d347

feat(git): Add SemanticMerger MVP JabRef#12350

4ffd7ba

feat(git): Implement normal sync loop without conflicts JabRef#12350

4f52736

Merge branch 'main' into gsoc-git-support-init

a8eedc4

Try to fix markdown lint issues ^^

f262355

subhramit requested changes Jun 18, 2025

View reviewed changes

koppor reviewed Jun 18, 2025

View reviewed changes

InAnYan reviewed Jun 24, 2025

View reviewed changes

InAnYan reviewed Jun 25, 2025

View reviewed changes

	public MergeResult pullAndMerge(Path bibFilePath) throws Exception {
	public MergeResult pullAndMerge(Path bibFilePath) throws GitAPIException, IOException, JabRefException {

	Path bibFilePath) throws Exception {
	Path bibFilePath) throws IOException, JabRefException {

	BibEntry entry = entries.get(0);
	BibEntry entry = entries.getFirst();

		assertEquals(inputEntries.get(0).getCitationKey(), outputEntries.get(0).getCitationKey());
		assertEquals(inputEntries.get(0).getField(StandardField.AUTHOR), outputEntries.get(0).getField(StandardField.AUTHOR));

		BibDatabaseContext baseCtx = parse(baseCommit);
		BibDatabaseContext remoteCtx = parse(remoteCommit);

	* does not check for "modifications" or "conflicts" — all decisions should be handled in advance by the SemanticConflictDetector
	* does not check for "modifications" or "conflicts" — all decisions should be handled in advance by the {@link SemanticConflictDetector}

WIP: GSoC Git Support - Support semantic pull merge without conflicts #714

Are you sure you want to change the base?

WIP: GSoC Git Support - Support semantic pull merge without conflicts #714

Uh oh!

Conversation

wanling0000 commented Jun 18, 2025

Steps to test

Mandatory checks

Uh oh!

subhramit left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subhramit Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subhramit Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

subhramit left a comment •

edited

Loading

subhramit Jun 18, 2025 •

edited

Loading

subhramit Jun 18, 2025 •

edited

Loading