Skip to content

Commit 5a91a7f

Browse files
wanling0000subhramit
wanling0000
and
subhramit
authored
Fix external file modification warnings during Git merge (#13946)
* 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 #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 <[email protected]> * fix:remove unnecessary annotation and avoid throwing RuntimeException --------- Co-authored-by: Subhramit Basu <[email protected]>
1 parent fcceb73 commit 5a91a7f

37 files changed

+1502
-913
lines changed

‎docs/code-howtos/git.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,8 @@ Therefore, JabRef performs an automatic merge without requiring manual conflict
7777

7878
The semantic conflict detection and merge resolution logic is covered by:
7979

80-
* `org.jabref.logic.git.util.SemanticMergerTest#patchDatabase`
81-
* `org.jabref.logic.git.util.SemanticConflictDetectorTest#semanticConflicts`.
80+
* `org.jabref.logic.git.merge.SemanticMergeAnalyzerTest#semanticEntryLevelConflicts`
81+
* `org.jabref.logic.git.merge.SemanticMergeAnalyzerTest#semanticFieldLevelConflicts`.
8282

8383
## Conflict Scenarios
8484

‎jabgui/src/main/java/org/jabref/gui/git/GitPullAction.java‎

Lines changed: 82 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import java.nio.file.Path;
55
import java.util.List;
66
import java.util.Optional;
7+
import java.util.StringJoiner;
78

89
import org.jabref.gui.DialogService;
910
import org.jabref.gui.StateManager;
@@ -14,9 +15,11 @@
1415
import org.jabref.logic.git.GitHandler;
1516
import org.jabref.logic.git.GitSyncService;
1617
import org.jabref.logic.git.conflicts.GitConflictResolverStrategy;
17-
import org.jabref.logic.git.merge.GitSemanticMergeExecutor;
18-
import org.jabref.logic.git.merge.GitSemanticMergeExecutorImpl;
19-
import org.jabref.logic.git.model.PullResult;
18+
import org.jabref.logic.git.conflicts.ThreeWayEntryConflict;
19+
import org.jabref.logic.git.io.GitFileWriter;
20+
import org.jabref.logic.git.model.BookkeepingResult;
21+
import org.jabref.logic.git.model.MergePlan;
22+
import org.jabref.logic.git.model.PullPlan;
2023
import org.jabref.logic.git.util.GitHandlerRegistry;
2124
import org.jabref.logic.l10n.Localization;
2225
import org.jabref.logic.util.BackgroundTask;
@@ -26,6 +29,9 @@
2629

2730
import org.eclipse.jgit.api.errors.GitAPIException;
2831

32+
import static org.jabref.logic.git.merge.execution.GitMergeApplier.applyAutoPlan;
33+
import static org.jabref.logic.git.merge.execution.GitMergeApplier.applyResolved;
34+
2935
public class GitPullAction extends SimpleCommand {
3036

3137
private final DialogService dialogService;
@@ -74,44 +80,95 @@ public void execute() {
7480
GitStatusViewModel gitStatusViewModel = GitStatusViewModel.fromPathAndContext(stateManager, taskExecutor, gitHandlerRegistry, bibFilePath);
7581

7682
BackgroundTask
77-
.wrap(() -> doPull(activeDatabase, bibFilePath, gitHandlerRegistry))
78-
.onSuccess(result -> {
79-
if (result.noop()) {
83+
.wrap(() -> prepareMergeResult(activeDatabase, bibFilePath, gitHandlerRegistry))
84+
.onSuccess(pullPlanOpt -> {
85+
if (pullPlanOpt.isEmpty()) {
8086
dialogService.showInformationDialogAndWait(
8187
Localization.lang("Git Pull"),
82-
Localization.lang("Already up to date.")
88+
Localization.lang("Already up to date or local branch is ahead.")
8389
);
84-
} else if (result.isSuccessful()) {
85-
try {
86-
replaceWithMergedEntries(result.getMergedEntries(), activeDatabase);
87-
gitStatusViewModel.refresh(bibFilePath);
88-
dialogService.showInformationDialogAndWait(
89-
Localization.lang("Git Pull"),
90-
Localization.lang("Merged and updated."));
91-
} catch (IOException | JabRefException ex) {
92-
showPullError(ex);
90+
return;
91+
}
92+
93+
PullPlan pullPlan = pullPlanOpt.get();
94+
MergePlan autoMergePlan = pullPlan.autoPlan();
95+
List<ThreeWayEntryConflict> conflicts = pullPlan.conflicts();
96+
97+
int autoNewCount = autoMergePlan.newEntries().size();
98+
int autoModifiedCount = autoMergePlan.fieldPatches().size();
99+
int autoDeletedCount = autoMergePlan.deletedEntryKeys().size();
100+
101+
applyAutoPlan(activeDatabase, autoMergePlan);
102+
103+
int manualResolvedCount;
104+
if (!conflicts.isEmpty()) {
105+
// resolve via GUI (strategy jumps to FX thread internally; safe to call from background)
106+
GitConflictResolverStrategy resolver = new GuiGitConflictResolverStrategy(new GitConflictResolverDialog(dialogService, guiPreferences));
107+
List<BibEntry> resolved = resolver.resolveConflicts(conflicts);
108+
if (resolved.isEmpty()) {
109+
dialogService.notify(Localization.lang("Pull canceled."));
110+
return;
93111
}
112+
manualResolvedCount = resolved.size();
113+
applyResolved(activeDatabase, resolved);
114+
} else {
115+
manualResolvedCount = 0;
94116
}
117+
118+
BackgroundTask.wrap(() -> saveAndFinalize(bibFilePath, activeDatabase, pullPlan))
119+
.onSuccess(finalizeResult -> {
120+
gitStatusViewModel.refresh(bibFilePath);
121+
if (finalizeResult.isFastForward()) {
122+
dialogService.showInformationDialogAndWait(
123+
Localization.lang("Git Pull"),
124+
Localization.lang("Fast-forwarded to remote.")
125+
);
126+
} else {
127+
StringJoiner joiner = new StringJoiner(" ");
128+
joiner.add(Localization.lang(
129+
"Auto-applied changes: %0 new, %1 modified, %2 deleted.",
130+
String.valueOf(autoNewCount),
131+
String.valueOf(autoModifiedCount),
132+
String.valueOf(autoDeletedCount)
133+
));
134+
if (manualResolvedCount > 0) {
135+
joiner.add(Localization.lang(
136+
"%0 conflicts resolved.",
137+
String.valueOf(manualResolvedCount)
138+
));
139+
}
140+
String stats = joiner.toString();
141+
142+
dialogService.showInformationDialogAndWait(
143+
Localization.lang("Git Pull"),
144+
Localization.lang("Merged and updated.") + " " + stats
145+
);
146+
}
147+
})
148+
.onFailure(this::showPullError)
149+
.executeWith(taskExecutor);
95150
})
96151
.onFailure(this::showPullError)
97152
.executeWith(taskExecutor);
98153
}
99154

100-
private PullResult doPull(BibDatabaseContext databaseContext, Path bibPath, GitHandlerRegistry registry) throws IOException, GitAPIException, JabRefException {
101-
GitSyncService syncService = buildSyncService(bibPath, registry);
155+
/// Prepares a merge plan for the given library and file path.
156+
///
157+
/// @return An Optional containing the PullPlan if a merge is needed,
158+
/// or Optional.empty() if the local library is already up-to-date or ahead of the remote branch.
159+
private Optional<PullPlan> prepareMergeResult(BibDatabaseContext databaseContext, Path bibPath, GitHandlerRegistry registry) throws IOException, GitAPIException, JabRefException {
160+
GitSyncService gitSyncService = GitSyncService.create(guiPreferences.getImportFormatPreferences(), registry);
102161
GitHandler handler = registry.get(bibPath.getParent());
103162
String user = guiPreferences.getGitPreferences().getUsername();
104163
String pat = guiPreferences.getGitPreferences().getPat();
105164
handler.setCredentials(user, pat);
106-
return syncService.fetchAndMerge(databaseContext, bibPath);
165+
return gitSyncService.prepareMerge(databaseContext, bibPath);
107166
}
108167

109-
private GitSyncService buildSyncService(Path bibPath, GitHandlerRegistry handlerRegistry) {
110-
GitConflictResolverDialog dialog = new GitConflictResolverDialog(dialogService, guiPreferences);
111-
GitConflictResolverStrategy resolver = new GuiGitConflictResolverStrategy(dialog);
112-
GitSemanticMergeExecutor mergeExecutor = new GitSemanticMergeExecutorImpl(guiPreferences.getImportFormatPreferences());
113-
114-
return new GitSyncService(guiPreferences.getImportFormatPreferences(), handlerRegistry, resolver, mergeExecutor);
168+
private BookkeepingResult saveAndFinalize(Path bibPath, BibDatabaseContext databaseContext, PullPlan pullPlan) throws IOException, GitAPIException, JabRefException {
169+
GitFileWriter.write(bibPath, databaseContext, guiPreferences.getImportFormatPreferences());
170+
GitSyncService gitSyncService = GitSyncService.create(guiPreferences.getImportFormatPreferences(), gitHandlerRegistry);
171+
return gitSyncService.finalizeMerge(bibPath, pullPlan);
115172
}
116173

117174
private void showPullError(Throwable exception) {
@@ -141,15 +198,4 @@ private void showPullError(Throwable exception) {
141198
);
142199
}
143200
}
144-
145-
private void replaceWithMergedEntries(List<BibEntry> mergedEntries, BibDatabaseContext databaseContext) throws IOException, JabRefException {
146-
List<BibEntry> currentEntries = List.copyOf(databaseContext.getDatabase().getEntries());
147-
for (BibEntry entry : currentEntries) {
148-
databaseContext.getDatabase().removeEntry(entry);
149-
}
150-
151-
for (BibEntry entry : mergedEntries) {
152-
databaseContext.getDatabase().insertEntry(new BibEntry(entry));
153-
}
154-
}
155201
}

‎jabgui/src/main/java/org/jabref/gui/git/GitPushAction.java‎

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,6 @@
1212
import org.jabref.logic.JabRefException;
1313
import org.jabref.logic.git.GitHandler;
1414
import org.jabref.logic.git.GitSyncService;
15-
import org.jabref.logic.git.conflicts.GitConflictResolverStrategy;
16-
import org.jabref.logic.git.merge.GitSemanticMergeExecutor;
17-
import org.jabref.logic.git.merge.GitSemanticMergeExecutorImpl;
1815
import org.jabref.logic.git.model.PushResult;
1916
import org.jabref.logic.git.util.GitHandlerRegistry;
2017
import org.jabref.logic.l10n.Localization;
@@ -80,7 +77,7 @@ public void execute() {
8077
Localization.lang("Git Push"),
8178
Localization.lang("Nothing to push. Local branch is up to date.")
8279
);
83-
} else if (result.isSuccessful()) {
80+
} else if (result.successful()) {
8481
dialogService.showInformationDialogAndWait(
8582
Localization.lang("Git Push"),
8683
Localization.lang("Pushed successfully.")
@@ -96,27 +93,20 @@ private PushResult doPush(BibDatabaseContext databaseContext,
9693
GitStatusViewModel gitStatusViewModel,
9794
GitHandlerRegistry registry) throws IOException, GitAPIException, JabRefException {
9895

99-
GitSyncService syncService = buildSyncService(bibPath, registry);
96+
GitSyncService syncService = GitSyncService.create(guiPreferences.getImportFormatPreferences(), registry);
10097
GitHandler handler = registry.get(bibPath.getParent());
10198
String user = guiPreferences.getGitPreferences().getUsername();
10299
String pat = guiPreferences.getGitPreferences().getPat();
103100
handler.setCredentials(user, pat);
104101

105102
PushResult result = syncService.push(databaseContext, bibPath);
106103

107-
if (result.isSuccessful()) {
104+
if (result.successful()) {
108105
gitStatusViewModel.refresh(bibPath);
109106
}
110107
return result;
111108
}
112109

113-
private GitSyncService buildSyncService(Path bibPath, GitHandlerRegistry handlerRegistry) throws JabRefException {
114-
GitConflictResolverDialog dialog = new GitConflictResolverDialog(dialogService, guiPreferences);
115-
GitConflictResolverStrategy resolver = new GuiGitConflictResolverStrategy(dialog);
116-
GitSemanticMergeExecutor mergeExecutor = new GitSemanticMergeExecutorImpl(guiPreferences.getImportFormatPreferences());
117-
return new GitSyncService(guiPreferences.getImportFormatPreferences(), handlerRegistry, resolver, mergeExecutor);
118-
}
119-
120110
private void showPushError(Throwable ex) {
121111
if (ex instanceof JabRefException e) {
122112
dialogService.showErrorDialogAndWait(

‎jablib/src/main/java/module-info.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,14 +109,15 @@
109109
exports org.jabref.logic.git;
110110
exports org.jabref.logic.git.conflicts;
111111
exports org.jabref.logic.git.io;
112-
exports org.jabref.logic.git.merge;
113112
exports org.jabref.logic.git.model;
114113
exports org.jabref.logic.git.status;
115114
exports org.jabref.logic.command;
116115
exports org.jabref.logic.git.util;
117116
exports org.jabref.logic.git.preferences;
118117
exports org.jabref.logic.icore;
119118
exports org.jabref.model.icore;
119+
exports org.jabref.logic.git.merge.planning;
120+
exports org.jabref.logic.git.merge.execution;
120121
exports org.jabref.model.sciteTallies;
121122

122123
requires java.base;

0 commit comments

Comments
 (0)