Fix external file modification warnings during Git merge by wanling0000 · Pull Request #13946 · JabRef/jabref

wanling0000 · 2025-09-22T13:03:06Z

Closes #12350

Previously, JabRef sometimes showed file has been externally altered warnings after a Git pull.
Possible causes:

Database writes were not triggered by the GUI.
Side-effects from GitHandler.MergeGuard modifying the index or working tree.

This PR changes the workflow:

Logic layer: only performs semantic analysis and orchestrates Git operations.
GUI layer: responsible for writing the merged database to disk.
Removed MergeCommand usage; now commits are created explicitly with specified parent commits.

Next steps

Review and refine test classes for coverage and stability.

Steps to test

Use “Share to GitHub” to create a remote BibTeX repository
Modify the file on the remote side
In JabRef, go to File → Git → Pull
Verify that the GUI no longer shows file being externally altered warnings

Mandatory checks

I own the copyright of the code submitted and I license it under the MIT license
I manually tested my changes in running JabRef (always required)
I added JUnit tests for changes (if applicable)
[/] I added screenshots in the PR description (if change is visible to the user)
[/] I described the change in CHANGELOG.md in a way that is understandable for the average user (if change is visible to the user)
I checked the user 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 updating file(s) in https://github.com/JabRef/user-documentation/tree/main/en.

…xternally

…d add unit tests

…xternally

wanling0000 · 2025-09-22T13:30:57Z

Sorry, this PR has become too large...
Although most of the changes are class moves and bulk test case migrations, I will split it into smaller PRs for easier review.
I will push the split versions later.

…xternally

trag-bot · 2025-09-24T16:21:22Z

+            if (localIsAncestorOfRemote) { // BEHIND (we know remote is ahead of local)
+                if (bibEqualsRemote) {
+                    gitHandler.fastForwardTo(remote);
+                    return FinalizeResult.fastForward();


The method should not return null. Consider using java.util.Optional to handle cases where a result might not be present.

trag-bot · 2025-09-24T16:21:26Z

+
+    private static boolean blobEqualsCommitPath(Git git, RevCommit commit, String relPath, Path file) {
+        try {
+            Optional<String> content = GitFileReader.readFileFromCommit(git, commit, Path.of(relPath));


Use modern Java best practices such as Path.of() instead of Paths.get() to improve readability and maintainability.

trag-bot · 2025-09-24T16:33:45Z

+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;


While Optional is used correctly, ensure that new public methods do not return null and instead use Optional consistently.

trag-bot · 2025-09-24T16:39:13Z

+import java.util.List;
+import java.util.Map;


The imports for List and Map should be replaced with modern Java data structures like Set.of() for better readability and maintainability.

trag-bot · 2025-09-24T16:39:16Z

+    public static MergePlan empty() {
+        return new MergePlan(Map.of(), List.of(), List.of());
+    }


The method 'empty' should not return null and should use java.util.Optional to handle empty states more effectively.

I think, here it returns a valid MergePlan instance with empty fields rather than a missing value, which does not refer to Optional...?

trag-bot · 2025-09-24T16:42:17Z

-    }
-
+public record PushResult(boolean successful, boolean noop) {
+    /// Created when local branch has commits to push and the push succeeded.


The comment is trivial and restates the obvious, which goes against the guideline that comments should add new information and not be plainly derived from the code itself.

trag-bot · 2025-09-24T16:42:20Z

        return new PushResult(true, false);
    }

+    /// Created when there is nothing to push (local is up to date with remote).


The comment is trivial and restates the obvious, which goes against the guideline that comments should add new information and not be plainly derived from the code itself.

…xternally

…/jabref into fix/files-changed-externally

trag-bot · 2025-09-29T20:45:38Z

@trag-bot didn't find any issues in the code! ✅✨

JabRef/jabref#13946 jablib/src/main/java/org/jabref/logic/git/conflicts/GitConflictResolverStrategy.java

koppor

Some small comments

I think, this really needs to be tested manually 😅. Maybe now - but if no one has time, we merge it into main and see what users say.

koppor · 2025-10-02T21:28:56Z

+                                                  Localization.lang("Fast-forwarded to remote.")
+                                          );
+                                      } else {
+                                          String stats = Localization.lang(


Normally, one uses StringJoiner with " ") as separator.

koppor · 2025-10-02T21:29:22Z


-    private PullResult doPull(BibDatabaseContext databaseContext, Path bibPath, GitHandlerRegistry registry) throws IOException, GitAPIException, JabRefException {
-        GitSyncService syncService = buildSyncService(bibPath, registry);
+    private Optional<PullPlan> prepareMergeResult(BibDatabaseContext databaseContext, Path bibPath, GitHandlerRegistry registry) throws IOException, GitAPIException, JabRefException {


Please add JavaDoc when the optional is empty.

koppor · 2025-10-02T21:29:59Z

+                                              PullPlan pullPlan)
+            throws IOException, GitAPIException, JabRefException {
+        GitFileWriter.write(bibPath, databaseContext, guiPreferences.getImportFormatPreferences());
+        // Git bookkeeping


Comment can be remoed, can't it? --> The line is self-explanatory, isn't it?

koppor · 2025-10-02T21:35:27Z

+        return gitSyncService.finalizeMerge(bibPath, pullPlan);
+    }
+
+    // ------------------- helpers: memory mutations -------------------


Not sure what this comment means.

Since the class is not long - just remove it.

(Otherwise: // region: xyz and then // endregion would be thing to group methods)

koppor · 2025-10-02T21:35:41Z

-        return new GitSyncService(guiPreferences.getImportFormatPreferences(), handlerRegistry, resolver, mergeExecutor);
+    /// Apply (remote - base) patches safely into the in-memory DB, plus safe new/deleted entries.
+    private static void applyAutoPlan(BibDatabaseContext bibDatabaseContext, MergePlan plan) {
+        // new entries


Remove the comment - the loop says it.

koppor · 2025-10-02T21:57:12Z

+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+
+public final class DefaultMergeBookkeeper implements MergeBookkeeper {


Please add JavaDoc about the intention of this class.

Since there is only one implementation of "MergeBookkeeper", I would merge these two classes - or document at "MergeBookkeeper" about the possible implementation variants.

koppor · 2025-10-02T22:05:30Z

+    /**
+     * Record the GUI-produced merge result into Git history.
+     * Creates the right commit shape based on the merge graph:
+     *  - BEHIND: fast-forward if content equals remote;
+     *            otherwise create a new commit on top of `remote`.
+     *  - DIVERGED: create a merge commit with parents [localHead, remote].
+     *
+     * Preconditions:
+     *  - GUI has already saved the final .bib file to disk.
+     *  - No unrelated unstaged changes (defensive check recommended).
+     */


Convert to /// because the content is Markdown

koppor · 2025-10-02T22:11:32Z

+                    author = {author-a},
+                    doi = {xya},


Please indent by two to have it consistently formatted. - a non-indent is very strange and seems like an issue

(at "normalize", you do remove the spaces, therefore, the tests work))

koppor · 2025-10-02T22:21:16Z

+     * Apply user-resolved entries into MEMORY: replace or insert by citation key.
+     * (Aligned with MergeEntriesAction’s “edit the in-memory database first” philosophy.)
+     */
+    private static void applyResolved(BibDatabaseContext bibDatabaseContext, List<BibEntry> resolved) {


Please do not duplice methods - you can use "package private" - meaning just leaving out "private" in the method to make it usable for for tests (Annote with @VisibleForTesting)

koppor · 2025-10-02T22:21:24Z

    }
+
+    /// Apply (remote - base) patches safely into the in-memory DB, plus safe new/deleted entries.
+    private static void applyAutoPlan(BibDatabaseContext bibDatabaseContext, MergePlan plan) {


Please do not duplice methods - you can use "package private" - meaning just leaving out "private" in the method to make it usable for for tests (Annote with @VisibleForTesting)

…xternally

subhramit

Tiny suggestions, code reads very pleasant.

rename variables Co-authored-by: Subhramit Basu <subhramit.bb@live.in>

subhramit

Thanks a looot for continuing :)

calixtus

Just five small comments. Please forgive me if some of them were already discussed, I did not follow the reviews before.

calixtus · 2025-10-15T15:34:15Z

+                                  .onSuccess(finalizeResult -> {
+                                      gitStatusViewModel.refresh(bibFilePath);
+                                      if (finalizeResult.isFastForward()) {
+                                          dialogService.showInformationDialogAndWait(
+                                                  Localization.lang("Git Pull"),
+                                                  Localization.lang("Fast-forwarded to remote.")
+                                          );
+                                      } else {
+                                          StringJoiner joiner = new StringJoiner(" ");
+                                          joiner.add(Localization.lang(
+                                                  "Auto-applied changes: %0 new, %1 modified, %2 deleted.",
+                                                  String.valueOf(autoNewCount),
+                                                  String.valueOf(autoModifiedCount),
+                                                  String.valueOf(autoDeletedCount)
+                                          ));
+                                          if (manualResolvedCount > 0) {
+                                              joiner.add(Localization.lang(
+                                                      "%0 conflicts resolved.",
+                                                      String.valueOf(manualResolvedCount)
+                                              ));
+                                          }
+                                          String stats = joiner.toString();
+
+                                          dialogService.showInformationDialogAndWait(
+                                                  Localization.lang("Git Pull"),
+                                                  Localization.lang("Merged and updated.") + " " + stats
+                                          );
+                                      }
+                                  })


For the sake of clean code this code be extracted to a separate method.

calixtus · 2025-10-15T15:36:28Z

-            boolean inMerging = (state == RepositoryState.MERGING) || (state == RepositoryState.MERGING_RESOLVED);

-            if (dirty) {
+            if (!status.isClean()) {


For clean code nesting could be reduced by failing fast - meaning `if (status.isClean()) { return false; }

calixtus · 2025-10-15T16:03:45Z

Looks really great so far. Very excited to have this merged.

comments by carl to be addressed

…/jabref into fix/files-changed-externally

addressed

* Changes by koppor * Changes by subhramit * update the JabRef code style * fix git pull action * fix git push action * test: migrate semantic conflict cases to SemanticMergeAnalyzerTest and add unit tests * fix: remove incorrect mention of `computeMergePlan` * fix: remove incorrect mention of `computeMergePlan` * Revert unintended renaming caused by IDE auto-formatting * Address part of trag-bot's suggestions in JabRef#13946 * Add missing English localization keys * remove obsolete inMerging branch in createCommitOnCurrentBranch * test: replace assertTrue with assertEquals * Clean up comments and variable names + Modify a test assertion * Change prepareMerge() to return Optional<PullPlan> for no-op cases * Fix failed tests * Update comments * remove exception control flow, assert presence of PullPlan instead * Handle the optional PullPlan * refactor(git): address review feedback * chore: fix OpenRewrite config and apply code style cleanup * Apply suggestions from code review rename variables Co-authored-by: Subhramit Basu <subhramit.bb@live.in> * fix:remove unnecessary annotation and avoid throwing RuntimeException --------- Co-authored-by: Subhramit Basu <subhramit.bb@live.in>

JabRef/jabref#13946 jablib/src/main/java/org/jabref/logic/git/conflicts/GitConflictResolverStrategy.java

wanling0000 added 9 commits August 29, 2025 23:53

Changes by koppor

8ac665a

Changes by subhramit

111d787

update the JabRef code style

dfdafb3

Merge remote-tracking branch 'upstream/main' into fix/files-changed-e…

3d2340c

…xternally

fix git pull action

d446f35

fix git push action

40158f4

test: migrate semantic conflict cases to SemanticMergeAnalyzerTest an…

891e94e

…d add unit tests

Merge remote-tracking branch 'upstream/main' into fix/files-changed-e…

c718c65

…xternally

fix: remove incorrect mention of computeMergePlan

63c6641

wanling0000 closed this Sep 22, 2025

wanling0000 added 2 commits September 22, 2025 21:41

fix: remove incorrect mention of computeMergePlan

06d7362

Merge remote-tracking branch 'upstream/main' into fix/files-changed-e…

935900f

…xternally

wanling0000 reopened this Sep 24, 2025

koppor mentioned this pull request Sep 24, 2025

Part 1/5: introduce SemanticMergeAnalyzer; prep fix for external-modification error #13970

Merged

2 tasks

trag-bot Bot reviewed Sep 24, 2025

View reviewed changes

Comment thread jablib/src/main/java/org/jabref/logic/git/merge/execution/MergeBookkeeper.java

trag-bot Bot reviewed Sep 24, 2025

View reviewed changes

Comment thread jablib/src/main/java/org/jabref/logic/git/merge/execution/MergeBookkeeper.java

trag-bot Bot reviewed Sep 24, 2025

View reviewed changes

Comment thread jablib/src/main/java/org/jabref/logic/git/merge/MergeBookkeeper.java Outdated

trag-bot Bot reviewed Sep 24, 2025

View reviewed changes

wanling0000 added 3 commits September 24, 2025 17:45

Merge remote-tracking branch 'upstream/main' into fix/files-changed-e…

ca11474

…xternally

Revert unintended renaming caused by IDE auto-formatting

a3db6fc

Merge branch 'fix/files-changed-externally' of github.com:wanling0000…

c86766d

…/jabref into fix/files-changed-externally

This was referenced Sep 24, 2025

Part 2/5: wire SemanticMergeAnalyzer into sync flow #13971

Closed

Part 3/5: add Merge Bookkeeper to record GUI-applied semantic merges #13972

Closed

tsantalis added a commit to tsantalis/RefactoringMiner that referenced this pull request Oct 1, 2025

Improve comment distribution for top-level type declarations

3e2e174

JabRef/jabref#13946 jablib/src/main/java/org/jabref/logic/git/conflicts/GitConflictResolverStrategy.java

koppor previously requested changes Oct 2, 2025

View reviewed changes

wanling0000 added 3 commits October 14, 2025 20:01

refactor(git): address review feedback

b7acdec

Merge remote-tracking branch 'upstream/main' into fix/files-changed-e…

1f9b0e0

…xternally

chore: fix OpenRewrite config and apply code style cleanup

342b2d5

subhramit reviewed Oct 14, 2025

View reviewed changes

Comment thread jablib/src/main/java/org/jabref/logic/git/merge/execution/MergeBookkeeper.java Outdated

Comment thread jablib/src/main/java/org/jabref/logic/git/merge/execution/MergeBookkeeper.java Outdated

subhramit requested a review from koppor October 14, 2025 23:03

Apply suggestions from code review

dbb3723

rename variables Co-authored-by: Subhramit Basu <subhramit.bb@live.in>

subhramit previously approved these changes Oct 15, 2025

View reviewed changes

calixtus reviewed Oct 15, 2025

View reviewed changes

wanling0000 added 2 commits October 16, 2025 00:15

fix:remove unnecessary annotation and avoid throwing RuntimeException

e4a3f1f

Merge branch 'fix/files-changed-externally' of github.com:wanling0000…

c26c76e

…/jabref into fix/files-changed-externally

calixtus approved these changes Oct 16, 2025

View reviewed changes

subhramit approved these changes Oct 16, 2025

View reviewed changes

Siedlerchr enabled auto-merge October 16, 2025 08:50

Siedlerchr added this pull request to the merge queue Oct 16, 2025

Merged via the queue into JabRef:main with commit 5a91a7f Oct 16, 2025
52 checks passed

github-actions Bot mentioned this pull request Oct 16, 2025

Add (simple) git support #12350

Closed

subhramit added component: external-files component: external-changes component: git labels Oct 16, 2025

ThiloteE mentioned this pull request Oct 17, 2025

[Git] Exception: Nothing to commit wanling0000/jabref#13

Open

maorethians pushed a commit to maorethians/RefactoringMiner that referenced this pull request Jun 8, 2026

Improve comment distribution for top-level type declarations

e824cf7

JabRef/jabref#13946 jablib/src/main/java/org/jabref/logic/git/conflicts/GitConflictResolverStrategy.java

		import java.util.List;
		import java.util.Map;

Uh oh!

Conversation

wanling0000 commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Steps to test

Mandatory checks

Uh oh!

wanling0000 commented Sep 22, 2025

Uh oh!

Uh oh!

Uh oh!

trag-bot Bot Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot Bot Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

trag-bot Bot Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot Bot Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot Bot Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trag-bot Bot Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot Bot Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot Bot commented Sep 29, 2025

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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

subhramit left a comment

Choose a reason for hiding this comment

Uh oh!

calixtus 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!

Uh oh!

Uh oh!

Uh oh!

wanling0000 commented Sep 22, 2025 •

edited

Loading