Fix: Decouple GitHandler creation via registry and extend semantic conflict detection #13666

wanling0000 · 2025-08-06T11:06:23Z

Closes #12350

This PR includes two major improvements:

Decoupled GitHandler creation using a new GitHandlerRegistry
- Ensures a single GitHandler instance per Git repository root.
Improved semantic conflict detection logic
- Updated SemanticConflictDetector to better handle edge cases in entry-level conflicts

Steps to test

All changes are covered by new or updated unit tests:

SemanticConflictDetectorTest covers the new edge cases for entry-level conflict detection.
GitHandlerRegistryTest verifies consistent handler retrieval and repo root resolution.

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.

Decouple GitHandler creation via GitHandlerRegistry

Update SemanticConflictDetector

Apply OpenRewrite auto-fixes for code style compliance

Case comments are consistent with actual logic.

subhramit · 2025-08-06T15:54:40Z

jablib/src/main/java/org/jabref/logic/git/conflicts/SemanticConflictDetector.java

+            boolean localDeleted = local == null;
+            boolean remoteDeleted = remote == null;


Suggested change

boolean localDeleted = local == null;

boolean remoteDeleted = remote == null;

boolean localDeleted = (local == null);

boolean remoteDeleted = (remote == null);

It seems like each time it is changed by OpenRewrite

Can skip then

subhramit · 2025-08-06T16:09:39Z

jablib/src/main/java/org/jabref/logic/git/conflicts/SemanticConflictDetector.java

+            boolean localChanged = !localDeleted && !Objects.equals(base.getFieldMap(), local.getFieldMap());
+            boolean remoteChanged = !remoteDeleted && !Objects.equals(base.getFieldMap(), remote.getFieldMap());


I think since we have already checked for base != null and also since short circuiting guarantees that the respective second conditions are evaluated only if the first is not true (i.e, not null), we can use the plain equals method in these cases instead of Objects.equals?

Suggested change

boolean localChanged = !localDeleted && !Objects.equals(base.getFieldMap(), local.getFieldMap());

boolean remoteChanged = !remoteDeleted && !Objects.equals(base.getFieldMap(), remote.getFieldMap());

boolean localChanged = !localDeleted && !base.getFieldMap().equals(local.getFieldMap());

boolean remoteChanged = !remoteDeleted && !base.getFieldMap().equals(remote.getFieldMap());

You're right, no risk of NPE here. Changed to use equals() as suggested

subhramit · 2025-08-06T16:10:19Z

jablib/src/main/java/org/jabref/logic/git/conflicts/SemanticConflictDetector.java

+            boolean localChanged = !Objects.equals(base.getFieldMap(), local.getFieldMap());
+            boolean remoteChanged = !Objects.equals(base.getFieldMap(), remote.getFieldMap());


Similar comment as https://github.com/JabRef/jabref/pull/13666/files#r2257660938 here

subhramit · 2025-08-06T16:11:03Z

jablib/src/main/java/org/jabref/logic/git/conflicts/SemanticConflictDetector.java

+        boolean localChanged = !Objects.equals(base.getType(), local.getType());
+        boolean remoteChanged = !Objects.equals(base.getType(), remote.getType());
+
+        return localChanged && remoteChanged && !Objects.equals(local.getType(), remote.getType());


subhramit · 2025-08-06T16:33:18Z

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

+    public GitHandler get(Path repoPath) {
+        if (repoPath == null) {
+            throw new IllegalArgumentException("Path must not be null");
+        }
+
+        Path normalized = repoPath.toAbsolutePath().normalize();
+        return handlerCache.computeIfAbsent(normalized, GitHandler::new);
+    }


This is debatable, but I am not a huge fan of using logical branching to throw exceptions. If there is a logical branch, it should not deal with an exceptional/unexpected case. Catch blocks are there for that.
How about

Suggested change

public GitHandler get(Path repoPath) {

if (repoPath == null) {

throw new IllegalArgumentException("Path must not be null");

}

Path normalized = repoPath.toAbsolutePath().normalize();

return handlerCache.computeIfAbsent(normalized, GitHandler::new);

}

public GitHandler get(Path repoPath) {

Path normalized = Objects.requireNonNull(repoPath, "Path must not be null")

.toAbsolutePath().normalize();

return handlerCache.computeIfAbsent(normalized, GitHandler::new);

}

Note that if you take this suggestion, you'll need the Objects import here.

I think you're right, the path is already checked before calling this method, so this is really just a defensive guard (and to keep trag-bot happy). Switched to Objects.requireNonNull(...). Thanks!

subhramit · 2025-08-06T16:33:38Z

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

+    public Optional<GitHandler> fromAnyPath(Path anyPathInsideRepo) {
+        if (anyPathInsideRepo == null) {
+            throw new IllegalArgumentException("Path must not be null");
+        }
+
+        return GitHandler.findRepositoryRoot(anyPathInsideRepo)
+                         .map(this::get);
+    }


Similar comment as https://github.com/JabRef/jabref/pull/13666/files#r2257720507 here

public Optional<GitHandler> fromAnyPath(Path anyPathInsideRepo) { return GitHandler.findRepositoryRoot(Objects.requireNonNull(anyPathInsideRepo, "Path must not be null")) .map(this::get); }

InAnYan

Good!

docs/code-howtos/git.md

trag-bot · 2025-08-07T13:47:46Z

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

subhramit

lgtm

refactor GitHandlerRegistry

d2dc011

Decouple GitHandler creation via GitHandlerRegistry

wanling0000 force-pushed the fix/semantic-conflict-logic branch from da6b7c8 to 267a42f Compare August 6, 2025 11:19

wanling0000 added 5 commits August 6, 2025 12:20

Merge upstream/main into fix/semantic-conflict-logic

ba77395

refactor SemanticConflictDetector

19d098b

Update SemanticConflictDetector

chore(rewrite)

7358c89

Apply OpenRewrite auto-fixes for code style compliance

test

93460f1

fix: add null check

687698c

wanling0000 force-pushed the fix/semantic-conflict-logic branch from 267a42f to 687698c Compare August 6, 2025 11:40

wanling0000 added 3 commits August 6, 2025 12:42

Merge upstream/main into fix/semantic-conflict-logic

09fd2ad

fix: SemanticConflictDetector logic

8a7d1f6

Case comments are consistent with actual logic.

refactor: remove redundant deletion checks

8de1d4f

subhramit reviewed Aug 6, 2025

View reviewed changes

subhramit added project: gsoc component: git labels Aug 6, 2025

wanling0000 added 3 commits August 6, 2025 20:59

Replace redundant null checks and use equals() where safe

88de9df

Merge upstream/main into fix/semantic-conflict-logic

07d9632

chore(rewrite)

3da6327

InAnYan previously approved these changes Aug 7, 2025

View reviewed changes

docs/code-howtos/git.md Show resolved Hide resolved

chore(docs): adjust table formatting

1ef4897

wanling0000 dismissed InAnYan’s stale review via 1ef4897 August 7, 2025 13:46

subhramit approved these changes Aug 7, 2025

View reviewed changes

InAnYan approved these changes Aug 7, 2025

View reviewed changes

Siedlerchr added this pull request to the merge queue Aug 7, 2025

Merged via the queue into JabRef:main with commit 592fd18 Aug 7, 2025
1 check passed

github-actions bot mentioned this pull request Aug 7, 2025

Add (simple) git support #12350

Closed

ThiloteE mentioned this pull request Aug 10, 2025

Citation relations tab doesn't work properly! #12967

Open

2 tasks

		boolean localDeleted = local == null;
		boolean remoteDeleted = remote == null;

		boolean localChanged = !localDeleted && !Objects.equals(base.getFieldMap(), local.getFieldMap());
		boolean remoteChanged = !remoteDeleted && !Objects.equals(base.getFieldMap(), remote.getFieldMap());

		boolean localChanged = !Objects.equals(base.getFieldMap(), local.getFieldMap());
		boolean remoteChanged = !Objects.equals(base.getFieldMap(), remote.getFieldMap());

Uh oh!

Fix: Decouple GitHandler creation via registry and extend semantic conflict detection #13666

Fix: Decouple GitHandler creation via registry and extend semantic conflict detection #13666

Uh oh!

Conversation

wanling0000 commented Aug 6, 2025

Steps to test

Mandatory checks

Uh oh!

subhramit Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

wanling0000 Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

subhramit Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

subhramit Aug 6, 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!

wanling0000 Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

subhramit Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

subhramit Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

subhramit Aug 6, 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!

wanling0000 Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

subhramit Aug 6, 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!

InAnYan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

trag-bot bot commented Aug 7, 2025

Uh oh!

subhramit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

subhramit Aug 6, 2025 •

edited

Loading

subhramit Aug 6, 2025 •

edited

Loading

subhramit Aug 6, 2025 •

edited

Loading