chore(git): Fix failing unit test #12350

wanling0000 · wanling0000 · commit bfd8bb437667 · 2025-07-09T14:20:18.000+01:00
diff --git a/jabgui/src/main/java/org/jabref/gui/git/GitPullViewModel.java b/jabgui/src/main/java/org/jabref/gui/git/GitPullViewModel.java
@@ -29,6 +29,10 @@
 import org.eclipse.jgit.api.errors.GitAPIException;
 import org.eclipse.jgit.revwalk.RevCommit;
 
+/**
+ * ViewModel responsible for coordinating UI-bound Git Pull workflow,
+ * including conflict resolution.
+ */
 public class GitPullViewModel extends AbstractViewModel {
     private final ImportFormatPreferences importFormatPreferences;
     private final GitConflictResolver conflictResolver;
diff --git a/jablib/src/main/java/org/jabref/logic/git/GitHandler.java b/jablib/src/main/java/org/jabref/logic/git/GitHandler.java
@@ -234,4 +234,8 @@ public static Optional<GitHandler> fromAnyPath(Path anyPathInsideRepo) {
     public File getRepositoryPathAsFile() {
         return repositoryPathAsFile;
     }
+
+    public Git open() throws IOException {
+        return Git.open(this.repositoryPathAsFile);
+    }
 }
diff --git a/jablib/src/main/java/org/jabref/logic/git/GitSyncService.java b/jablib/src/main/java/org/jabref/logic/git/GitSyncService.java
@@ -16,6 +16,9 @@
 import org.jabref.logic.git.merge.MergePlan;
 import org.jabref.logic.git.merge.SemanticMerger;
 import org.jabref.logic.git.model.MergeResult;
+import org.jabref.logic.git.status.GitStatusChecker;
+import org.jabref.logic.git.status.GitStatusSnapshot;
+import org.jabref.logic.git.status.SyncStatus;
 import org.jabref.logic.importer.ImportFormatPreferences;
 import org.jabref.model.database.BibDatabaseContext;
 import org.jabref.model.entry.BibEntry;
@@ -27,11 +30,23 @@
 import org.slf4j.LoggerFactory;
 
 /**
- * Orchestrator for git sync service
+ * GitSyncService currently serves as an orchestrator for Git pull/push logic.
  * if (hasConflict)
  *     → UI merge;
  * else
  *     → autoMerge := local + remoteDiff
+ *
+ * NOTICE：
+ * - TODO：This class will be **deprecated** in the near future to avoid architecture violation (logic → gui)!
+ * - The underlying business logic will not change significantly.
+ * - Only the coordination responsibilities will shift to GUI/ViewModel layer.
+ *
+ * PLAN:
+ * - All orchestration logic (pull/push, merge, resolve, commit)
+ *   will be **moved into corresponding ViewModels**, such as:
+ *     - GitPullViewModel
+ *     - GitPushViewModel
+ *     - GitStatusViewModel
  */
 public class GitSyncService {
     private static final Logger LOGGER = LoggerFactory.getLogger(GitSyncService.class);
@@ -49,24 +64,42 @@ public GitSyncService(ImportFormatPreferences importFormatPreferences, GitHandle
      * Called when user clicks Pull
      */
     public MergeResult fetchAndMerge(Path bibFilePath) throws GitAPIException, IOException, JabRefException {
-        Git git = Git.open(bibFilePath.getParent().toFile());
+        GitStatusSnapshot status = GitStatusChecker.checkStatus(bibFilePath);
 
-        // 1. fetch latest remote branch
-        gitHandler.fetchOnCurrentBranch();
-
-        // 2. Locating the base / local / remote versions
-        GitRevisionLocator locator = new GitRevisionLocator();
-        RevisionTriple triple = locator.locateMergeCommits(git);
+        if (!status.tracking()) {
+            LOGGER.warn("Pull aborted: The file is not under Git version control.");
+            return MergeResult.failure();
+        }
 
-        // 3. Calling semantic merge logic
-        MergeResult result = performSemanticMerge(git, triple.base(), triple.local(), triple.remote(), bibFilePath);
+        if (status.conflict()) {
+            LOGGER.warn("Pull aborted: Local repository has unresolved merge conflicts.");
+            return MergeResult.failure();
+        }
 
-        // 4. Automatic merge
-        if (result.isSuccessful()) {
-            gitHandler.createCommitOnCurrentBranch("Auto-merged by JabRef", !AMEND);
+        if (status.syncStatus() == SyncStatus.UP_TO_DATE || status.syncStatus() == SyncStatus.AHEAD) {
+            LOGGER.info("Pull skipped: Local branch is already up to date with remote.");
+            return MergeResult.success();
         }
 
-        return result;
+        // Status is BEHIND or DIVERGED
+        try (Git git = gitHandler.open()) {
+            // 1. Fetch latest remote branch
+            gitHandler.fetchOnCurrentBranch();
+
+            // 2. Locate base / local / remote commits
+            GitRevisionLocator locator = new GitRevisionLocator();
+            RevisionTriple triple = locator.locateMergeCommits(git);
+
+            // 3. Perform semantic merge
+            MergeResult result = performSemanticMerge(git, triple.base(), triple.local(), triple.remote(), bibFilePath);
+
+            // 4. Auto-commit merge result if successful
+            if (result.isSuccessful()) {
+                gitHandler.createCommitOnCurrentBranch("Auto-merged by JabRef", !AMEND);
+            }
+
+            return result;
+        }
     }
 
     public MergeResult performSemanticMerge(Git git,
@@ -80,7 +113,6 @@ public MergeResult performSemanticMerge(Git git,
         Path relativePath;
 
         // TODO: Validate that the .bib file is inside the Git repository earlier in the workflow.
-        // This check might be better placed before calling performSemanticMerge.
         if (!bibPath.startsWith(workTree)) {
             throw new IllegalStateException("Given .bib file is not inside repository");
         }
@@ -122,43 +154,61 @@ public MergeResult performSemanticMerge(Git git,
         return MergeResult.success();
     }
 
-    // WIP
-    public void push(Path bibFilePath) throws GitAPIException, IOException, JabRefException{
-        // 1. 1: Fetch remote changes
-        gitHandler.fetchOnCurrentBranch();
-
-        // 2: Check if local branch is behind remote
-        if (gitHandler.isBehindRemote()) {
-            LOGGER.info("Remote changes detected — performing semantic merge");
-
-            // 2.1: Resolve commit identifiers
-            RevCommit baseCommit = gitHandler.findMergeBaseWithRemote();
-            RevCommit localCommit = gitHandler.resolveHead();
-            RevCommit remoteCommit = gitHandler.resolveRemoteHead();
-
-            // 2.2: Perform semantic merge of the .bib file
-            MergeResult mergeResult = performSemanticMerge(
-                    gitHandler.getGit(),
-                    baseCommit,
-                    localCommit,
-                    remoteCommit,
-                    bibFilePath
-            );
-
-            if (!mergeResult.isSuccessful()) {
-                LOGGER.warn("Semantic merge failed — aborting push");
-                return;
-            }
+    public void push(Path bibFilePath) throws GitAPIException, IOException, JabRefException {
+        GitStatusSnapshot status = GitStatusChecker.checkStatus(bibFilePath);
+
+        if (!status.tracking()) {
+            LOGGER.warn("Push aborted: file is not tracked by Git");
+            return;
         }
 
-        // 3: Commit changes if there are any
-        boolean committed = gitHandler.createCommitOnCurrentBranch("Changes committed by JabRef", !AMEND);
+        switch (status.syncStatus()) {
+            case UP_TO_DATE -> {
+                boolean committed = gitHandler.createCommitOnCurrentBranch("Changes committed by JabRef", !AMEND);
+                if (committed) {
+                    gitHandler.pushCommitsToRemoteRepository();
+                } else {
+                    LOGGER.info("No changes to commit — skipping push");
+                }
+            }
 
-        // 4: Push to remote only if a commit was made
-        if (committed) {
-            gitHandler.pushCommitsToRemoteRepository();
-        } else {
-            LOGGER.info("No changes to commit — skipping push");
+            case AHEAD -> {
+                gitHandler.pushCommitsToRemoteRepository();
+            }
+
+            case BEHIND -> {
+                LOGGER.warn("Push aborted: Local branch is behind remote. Please pull first.");
+            }
+
+            case DIVERGED -> {
+                try (Git git = gitHandler.open()) {
+                    GitRevisionLocator locator = new GitRevisionLocator();
+                    RevisionTriple triple = locator.locateMergeCommits(git);
+
+                    MergeResult mergeResult = performSemanticMerge(git, triple.base(), triple.local(), triple.remote(), bibFilePath);
+
+                    if (!mergeResult.isSuccessful()) {
+                        LOGGER.warn("Semantic merge failed — aborting push");
+                        return;
+                    }
+
+                    boolean committed = gitHandler.createCommitOnCurrentBranch("Merged changes", !AMEND);
+
+                    if (committed) {
+                        gitHandler.pushCommitsToRemoteRepository();
+                    } else {
+                        LOGGER.info("Nothing to commit after semantic merge — skipping push");
+                    }
+                }
+            }
+
+            case CONFLICT -> {
+                LOGGER.warn("Push aborted: Local repository has unresolved merge conflicts.");
+            }
+
+            case UNTRACKED, UNKNOWN -> {
+                LOGGER.warn("Push aborted: Untracked or unknown Git status.");
+            }
         }
     }
 }
diff --git a/jablib/src/main/java/org/jabref/logic/git/io/GitFileWriter.java b/jablib/src/main/java/org/jabref/logic/git/io/GitFileWriter.java
@@ -6,8 +6,8 @@
 import java.nio.file.Path;
 
 import org.jabref.logic.exporter.AtomicFileWriter;
+import org.jabref.logic.exporter.BibDatabaseWriter;
 import org.jabref.logic.exporter.BibWriter;
-import org.jabref.logic.exporter.BibtexDatabaseWriter;
 import org.jabref.logic.exporter.SelfContainedSaveConfiguration;
 import org.jabref.logic.importer.ImportFormatPreferences;
 import org.jabref.model.database.BibDatabaseContext;
@@ -21,7 +21,7 @@ public static void write(Path file, BibDatabaseContext bibDatabaseContext, Impor
         synchronized (bibDatabaseContext) {
             try (AtomicFileWriter fileWriter = new AtomicFileWriter(file, encoding, saveConfiguration.shouldMakeBackup())) {
                 BibWriter bibWriter = new BibWriter(fileWriter, bibDatabaseContext.getDatabase().getNewLineSeparator());
-                BibtexDatabaseWriter writer = new BibtexDatabaseWriter(
+                BibDatabaseWriter writer = new BibDatabaseWriter(
                         bibWriter,
                         saveConfiguration,
                         importPrefs.fieldPreferences(),
diff --git a/jablib/src/main/java/org/jabref/logic/git/io/GitRevisionLocator.java b/jablib/src/main/java/org/jabref/logic/git/io/GitRevisionLocator.java
@@ -7,8 +7,10 @@
 import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.api.errors.GitAPIException;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.revwalk.filter.RevFilter;
 
 /**
  * Find the base/local/remote three commits:
@@ -21,26 +23,31 @@ public class GitRevisionLocator {
     private static final String REMOTE = "refs/remotes/origin/main";
 
     public RevisionTriple locateMergeCommits(Git git) throws GitAPIException, IOException, JabRefException {
+        Repository repo = git.getRepository();
         // assumes the remote branch is 'origin/main'
-        ObjectId headId = git.getRepository().resolve(HEAD);
+        ObjectId headId = repo.resolve(HEAD);
         // and uses the default remote tracking reference
         // does not support multiple remotes or custom remote branch names so far
-        ObjectId remoteId = git.getRepository().resolve(REMOTE);
+        ObjectId remoteId = repo.resolve(REMOTE);
         if (remoteId == null) {
             throw new IllegalStateException("Remote branch missing origin/main.");
         }
 
         try (RevWalk walk = new RevWalk(git.getRepository())) {
             RevCommit local = walk.parseCommit(headId);
             RevCommit remote = walk.parseCommit(remoteId);
-
-            walk.setRevFilter(org.eclipse.jgit.revwalk.filter.RevFilter.MERGE_BASE);
-            walk.markStart(local);
-            walk.markStart(remote);
-
-            RevCommit base = walk.next();
+            RevCommit base = findMergeBase(repo, local, remote);
 
             return new RevisionTriple(base, local, remote);
         }
     }
+
+    public static RevCommit findMergeBase(Repository repo, RevCommit a, RevCommit b) throws IOException {
+        try (RevWalk walk = new RevWalk(repo)) {
+            walk.setRevFilter(RevFilter.MERGE_BASE);
+            walk.markStart(a);
+            walk.markStart(b);
+            return walk.next();
+        }
+    }
 }
diff --git a/jablib/src/main/java/org/jabref/logic/git/model/MergeResult.java b/jablib/src/main/java/org/jabref/logic/git/model/MergeResult.java
@@ -14,6 +14,10 @@ public static MergeResult success() {
         return new MergeResult(SUCCESS, List.of());
     }
 
+    public static MergeResult failure() {
+        return new MergeResult(false, List.of());
+    }
+
     public boolean hasConflicts() {
         return !conflicts.isEmpty();
     }
diff --git a/jablib/src/main/java/org/jabref/logic/git/status/GitStatusChecker.java b/jablib/src/main/java/org/jabref/logic/git/status/GitStatusChecker.java
@@ -18,9 +18,9 @@
 import org.slf4j.LoggerFactory;
 
 /**
- * GitStatusChecker 用于从任意路径检测当前 Git 仓库状态。
- * 如果找不到仓库，返回 tracking = false 的 GitStatusSnapshot；
- * 否则返回完整状态（tracking + syncStatus + conflict）。
+ * This class is used to determine the status of a Git repository from any given path inside it.
+ * If no repository is found, it returns a {@link GitStatusSnapshot} with tracking = false.
+ * Otherwise, it returns a full snapshot including tracking status, sync status, and conflict state.
  */
 public class GitStatusChecker {
     private static final Logger LOGGER = LoggerFactory.getLogger(GitStatusChecker.class);
@@ -39,7 +39,7 @@ public static GitStatusSnapshot checkStatus(Path anyPathInsideRepo) {
             boolean hasConflict = !status.getConflicting().isEmpty();
 
             ObjectId localHead = repo.resolve("HEAD");
-            ObjectId remoteHead = repo.resolve("@{u}");
+            ObjectId remoteHead = repo.resolve("refs/remotes/origin/main");
             SyncStatus syncStatus = determineSyncStatus(repo, localHead, remoteHead);
 
             return new GitStatusSnapshot(
diff --git a/jablib/src/main/java/org/jabref/logic/git/status/GitStatusSnapshot.java b/jablib/src/main/java/org/jabref/logic/git/status/GitStatusSnapshot.java
@@ -0,0 +1,9 @@
+package org.jabref.logic.git.status;
+
+import java.util.Optional;
+
+public record GitStatusSnapshot(
+    boolean tracking,
+    SyncStatus syncStatus,
+    boolean conflict,
+    Optional<String> lastPulledCommit) { }
diff --git a/jablib/src/test/java/org/jabref/logic/git/GitSyncServiceTest.java b/jablib/src/test/java/org/jabref/logic/git/GitSyncServiceTest.java
@@ -118,16 +118,13 @@ void aliceBobSimple(@TempDir Path tempDir) throws Exception {
         aliceCommit = writeAndCommit(aliceUpdatedContent, "Fix author of a", alice, library, aliceGit);
         git.fetch().setRemote("origin").call();
 
-        // ToDo: Replace by call to GitSyncService crafting a merge commit
-//      git.merge().include(aliceCommit).include(bobCommit).call(); // Will throw exception bc of merge conflict
-
         // Debug hint: Show the created git graph on the command line
         //   git log --graph --oneline --decorate --all --reflog
     }
 
     @Test
     void pullTriggersSemanticMergeWhenNoConflicts() throws Exception {
-        GitHandler gitHandler = mock(GitHandler.class);
+        GitHandler gitHandler = new GitHandler(library.getParent());
         GitSyncService syncService = new GitSyncService(importFormatPreferences, gitHandler);
         MergeResult result = syncService.fetchAndMerge(library);
 
@@ -151,7 +148,7 @@ void pullTriggersSemanticMergeWhenNoConflicts() throws Exception {
 
     @Test
     void pushTriggersMergeAndPushWhenNoConflicts() throws Exception {
-        GitHandler gitHandler = mock(GitHandler.class);
+        GitHandler gitHandler = new GitHandler(library.getParent());
         GitSyncService syncService = new GitSyncService(importFormatPreferences, gitHandler);
         syncService.push(library);
 
@@ -171,7 +168,6 @@ void pushTriggersMergeAndPushWhenNoConflicts() throws Exception {
         assertEquals(normalize(expected), normalize(pushedContent));
     }
 
-
     @Test
     void readFromCommits() throws Exception {
         String base = GitFileReader.readFileFromCommit(git, baseCommit, Path.of("library.bib"));
diff --git a/jablib/src/test/java/org/jabref/logic/git/merge/GitSemanticMergeExecutorTest.java b/jablib/src/test/java/org/jabref/logic/git/merge/GitSemanticMergeExecutorTest.java
@@ -4,6 +4,8 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 
+import javafx.collections.FXCollections;
+
 import org.jabref.logic.git.model.MergeResult;
 import org.jabref.logic.importer.ImportFormatPreferences;
 import org.jabref.model.database.BibDatabaseContext;
@@ -12,9 +14,11 @@
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.mockito.Answers;
 
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 public class GitSemanticMergeExecutorTest {
 
@@ -40,7 +44,10 @@ public void setup() throws IOException {
         local.getDatabase().insertEntry(localEntry);
         remote.getDatabase().insertEntry(remoteEntry);
 
-        preferences = mock(ImportFormatPreferences.class);
+        preferences = mock(ImportFormatPreferences.class, Answers.RETURNS_DEEP_STUBS);
+        when(preferences.fieldPreferences().getNonWrappableFields())
+                .thenReturn(FXCollections.emptyObservableList());
+
         executor = new GitSemanticMergeExecutorImpl(preferences);
 
         tempFile = Files.createTempFile("merged", ".bib");
diff --git a/jablib/src/test/java/org/jabref/logic/git/status/GitStatusCheckerTest.java b/jablib/src/test/java/org/jabref/logic/git/status/GitStatusCheckerTest.java

Original file line number	Diff line number	Diff line change
`@@ -234,4 +234,8 @@ public static Optional<GitHandler> fromAnyPath(Path anyPathInsideRepo) {`
`234`	`234`	`public File getRepositoryPathAsFile() {`
`235`	`235`	`return repositoryPathAsFile;`
`236`	`236`	`}`
	`237`	`+`
	`238`	`+ public Git open() throws IOException {`
	`239`	`+ return Git.open(this.repositoryPathAsFile);`
	`240`	`+ }`
`237`	`241`	`}`
Original file line number	Diff line number	Diff line change
`@@ -14,6 +14,10 @@ public static MergeResult success() {`
`14`	`14`	`return new MergeResult(SUCCESS, List.of());`
`15`	`15`	`}`
`16`	`16`
	`17`	`+ public static MergeResult failure() {`
	`18`	`+ return new MergeResult(false, List.of());`
	`19`	`+ }`
	`20`	`+`
`17`	`21`	`public boolean hasConflicts() {`
`18`	`22`	`return !conflicts.isEmpty();`
`19`	`23`	`}`