From 973e955ead1a9bf41efb3168baf5b68527e78023 Mon Sep 17 00:00:00 2001 From: Andre Bossert Date: Sun, 19 Jan 2020 20:56:28 +0100 Subject: [PATCH 1/3] Add availability check of pre-defined tools see: https://git-scm.com/docs/git-difftool see: https://git-scm.com/docs/git-mergetool * now all available tools are printed with "--tool-help" * if no diff.tool or merge.tool is defined the first available pre-defined tool is used TODO: - add mergetools to difftools --> extra change or merge to this - return the exit-code of the tool to jgit / java runtime Bug: 356832 Change-Id: I20fb04e71ced981f5625020f461bbac24e6cec70 Signed-off-by: Andre Bossert --- .../org/eclipse/jgit/pgm/DiffToolTest.java | 30 ++++++-- .../org/eclipse/jgit/pgm/MergeToolTest.java | 30 ++++++-- .../jgit/pgm/internal/CLIText.properties | 4 + .../src/org/eclipse/jgit/pgm/DiffTool.java | 77 ++++++++++++------- .../src/org/eclipse/jgit/pgm/MergeTool.java | 46 ++++++++--- .../eclipse/jgit/pgm/internal/CLIText.java | 4 + .../diffmergetool/ExternalDiffToolTest.java | 13 +--- .../diffmergetool/ExternalMergeToolTest.java | 17 +--- .../diffmergetool/CommandExecutor.java | 51 +++++++++++- .../diffmergetool/CommandLineDiffTool.java | 4 +- .../internal/diffmergetool/DiffTools.java | 38 +++++++-- .../diffmergetool/ExternalDiffTool.java | 6 ++ .../diffmergetool/ExternalToolUtils.java | 64 +++++++++++++-- .../internal/diffmergetool/MergeTools.java | 41 ++++++++-- .../diffmergetool/PreDefinedDiffTool.java | 2 +- .../diffmergetool/PreDefinedMergeTool.java | 2 +- .../internal/diffmergetool/ToolException.java | 6 ++ .../diffmergetool/UserDefinedDiffTool.java | 19 +++++ 18 files changed, 357 insertions(+), 97 deletions(-) diff --git a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/DiffToolTest.java b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/DiffToolTest.java index 8daaa6ad9..a258821f0 100644 --- a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/DiffToolTest.java +++ b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/DiffToolTest.java @@ -20,8 +20,10 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Map; -import org.eclipse.jgit.internal.diffmergetool.CommandLineDiffTool; +import org.eclipse.jgit.internal.diffmergetool.DiffTools; +import org.eclipse.jgit.internal.diffmergetool.ExternalDiffTool; import org.eclipse.jgit.lib.StoredConfig; import org.junit.Before; import org.junit.Test; @@ -149,20 +151,38 @@ expectedOutput, runAndCaptureUsingInitRaw(DIFF_TOOL, @Test public void testToolHelp() throws Exception { - CommandLineDiffTool[] defaultTools = CommandLineDiffTool.values(); List expectedOutput = new ArrayList<>(); + + DiffTools diffTools = new DiffTools(db); + Map predefinedTools = diffTools + .getPredefinedTools(true); + List availableTools = new ArrayList<>(); + List notAvailableTools = new ArrayList<>(); + for (ExternalDiffTool tool : predefinedTools.values()) { + if (tool.isAvailable()) { + availableTools.add(tool); + } else { + notAvailableTools.add(tool); + } + } + expectedOutput.add( "'git difftool --tool=' may be set to one of the following:"); - for (CommandLineDiffTool defaultTool : defaultTools) { - String toolName = defaultTool.name(); + for (ExternalDiffTool tool : availableTools) { + String toolName = tool.getName(); expectedOutput.add(toolName); } String customToolHelpLine = TOOL_NAME + "." + CONFIG_KEY_CMD + " " + getEchoCommand(); expectedOutput.add("user-defined:"); expectedOutput.add(customToolHelpLine); + expectedOutput.add( + "The following tools are valid, but not currently available:"); + for (ExternalDiffTool tool : notAvailableTools) { + String toolName = tool.getName(); + expectedOutput.add(toolName); + } String[] userDefinedToolsHelp = { - "The following tools are valid, but not currently available:", "Some of the tools listed above only work in a windowed", "environment. If run in a terminal-only session, they will fail.", }; diff --git a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/MergeToolTest.java b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/MergeToolTest.java index 2e50f0908..e9d559e70 100644 --- a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/MergeToolTest.java +++ b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/MergeToolTest.java @@ -19,8 +19,10 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Map; -import org.eclipse.jgit.internal.diffmergetool.CommandLineMergeTool; +import org.eclipse.jgit.internal.diffmergetool.ExternalMergeTool; +import org.eclipse.jgit.internal.diffmergetool.MergeTools; import org.eclipse.jgit.lib.StoredConfig; import org.junit.Before; import org.junit.Test; @@ -157,20 +159,38 @@ expectedOutput, runAndCaptureUsingInitRaw(MERGE_TOOL, @Test public void testToolHelp() throws Exception { - CommandLineMergeTool[] defaultTools = CommandLineMergeTool.values(); List expectedOutput = new ArrayList<>(); + + MergeTools diffTools = new MergeTools(db); + Map predefinedTools = diffTools + .getPredefinedTools(true); + List availableTools = new ArrayList<>(); + List notAvailableTools = new ArrayList<>(); + for (ExternalMergeTool tool : predefinedTools.values()) { + if (tool.isAvailable()) { + availableTools.add(tool); + } else { + notAvailableTools.add(tool); + } + } + expectedOutput.add( "'git mergetool --tool=' may be set to one of the following:"); - for (CommandLineMergeTool defaultTool : defaultTools) { - String toolName = defaultTool.name(); + for (ExternalMergeTool tool : availableTools) { + String toolName = tool.getName(); expectedOutput.add(toolName); } String customToolHelpLine = TOOL_NAME + "." + CONFIG_KEY_CMD + " " + getEchoCommand(); expectedOutput.add("user-defined:"); expectedOutput.add(customToolHelpLine); + expectedOutput.add( + "The following tools are valid, but not currently available:"); + for (ExternalMergeTool tool : notAvailableTools) { + String toolName = tool.getName(); + expectedOutput.add(toolName); + } String[] userDefinedToolsHelp = { - "The following tools are valid, but not currently available:", "Some of the tools listed above only work in a windowed", "environment. If run in a terminal-only session, they will fail.", }; expectedOutput.addAll(Arrays.asList(userDefinedToolsHelp)); diff --git a/org.eclipse.jgit.pgm/resources/org/eclipse/jgit/pgm/internal/CLIText.properties b/org.eclipse.jgit.pgm/resources/org/eclipse/jgit/pgm/internal/CLIText.properties index 674185df2..b14531a1b 100644 --- a/org.eclipse.jgit.pgm/resources/org/eclipse/jgit/pgm/internal/CLIText.properties +++ b/org.eclipse.jgit.pgm/resources/org/eclipse/jgit/pgm/internal/CLIText.properties @@ -61,6 +61,8 @@ deletedRemoteBranch=Deleted remote branch {0} diffToolHelpSetToFollowing=''git difftool --tool='' may be set to one of the following:\n{0}\n\tuser-defined:\n{1}\nThe following tools are valid, but not currently available:\n{2}\nSome of the tools listed above only work in a windowed\nenvironment. If run in a terminal-only session, they will fail. diffToolLaunch=Viewing ({0}/{1}): ''{2}''\nLaunch ''{3}'' [Y/n]? diffToolDied=external diff died, stopping at path ''{0}'' due to exception: {1} +diffToolPromptToolName=This message is displayed because 'diff.tool' is not configured.\nSee 'git difftool --tool-help' or 'git help config' for more details.\n'git difftool' will now attempt to use one of the following tools:\n{0}\n +diffToolUnknownToolName=Unknown diff tool '{0}' doesNotExist={0} does not exist dontOverwriteLocalChanges=error: Your local changes to the following file would be overwritten by merge: everythingUpToDate=Everything up-to-date @@ -107,6 +109,8 @@ mergeToolDeletedConflictByThem= {local}: modified file\n {remote}: deleted mergeToolContinueUnresolvedPaths=\nContinue merging other unresolved paths [y/n]? mergeToolWasMergeSuccessfull=Was the merge successful [y/n]? mergeToolDeletedMergeDecision=Use (m)odified or (d)eleted file, or (a)bort? +mergeToolPromptToolName=This message is displayed because 'merge.tool' is not configured.\nSee 'git mergetool --tool-help' or 'git help config' for more details.\n'git mergetool' will now attempt to use one of the following tools:\n{0}\n +mergeToolUnknownToolName=Unknown merge tool '{0}' mergeFailed=Automatic merge failed; fix conflicts and then commit the result mergeCheckoutFailed=Please, commit your changes or stash them before you can merge. mergeMadeBy=Merge made by the ''{0}'' strategy. diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/DiffTool.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/DiffTool.java index 74d91cd3d..e5a3c53e3 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/DiffTool.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/DiffTool.java @@ -23,29 +23,30 @@ import java.util.List; import java.util.Map; import java.util.concurrent.TimeUnit; + import org.eclipse.jgit.diff.ContentSource; import org.eclipse.jgit.diff.ContentSource.Pair; import org.eclipse.jgit.diff.DiffEntry; import org.eclipse.jgit.diff.DiffEntry.Side; -import org.eclipse.jgit.internal.diffmergetool.ToolException; -import org.eclipse.jgit.internal.diffmergetool.DiffTools; -import org.eclipse.jgit.internal.diffmergetool.FileElement; -import org.eclipse.jgit.internal.diffmergetool.ExternalDiffTool; import org.eclipse.jgit.diff.DiffFormatter; import org.eclipse.jgit.dircache.DirCacheCheckout; -import org.eclipse.jgit.dircache.DirCacheIterator; import org.eclipse.jgit.dircache.DirCacheCheckout.CheckoutMetadata; +import org.eclipse.jgit.dircache.DirCacheIterator; import org.eclipse.jgit.errors.AmbiguousObjectException; import org.eclipse.jgit.errors.CorruptObjectException; import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.errors.NoWorkTreeException; import org.eclipse.jgit.errors.RevisionSyntaxException; +import org.eclipse.jgit.internal.diffmergetool.DiffTools; +import org.eclipse.jgit.internal.diffmergetool.ExternalDiffTool; +import org.eclipse.jgit.internal.diffmergetool.FileElement; +import org.eclipse.jgit.internal.diffmergetool.ToolException; import org.eclipse.jgit.lib.Constants; +import org.eclipse.jgit.lib.CoreConfig.EolStreamType; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.TextProgressMonitor; -import org.eclipse.jgit.lib.CoreConfig.EolStreamType; import org.eclipse.jgit.lib.internal.BooleanTriState; import org.eclipse.jgit.pgm.internal.CLIText; import org.eclipse.jgit.pgm.opt.PathTreeFilterHandler; @@ -58,8 +59,8 @@ import org.eclipse.jgit.treewalk.WorkingTreeOptions; import org.eclipse.jgit.treewalk.filter.PathFilterGroup; import org.eclipse.jgit.treewalk.filter.TreeFilter; -import org.eclipse.jgit.util.StringUtils; import org.eclipse.jgit.util.FS.ExecutionResult; +import org.eclipse.jgit.util.StringUtils; import org.kohsuke.args4j.Argument; import org.kohsuke.args4j.Option; @@ -144,19 +145,14 @@ protected void run() { if (prompt != BooleanTriState.UNSET) { showPrompt = prompt == BooleanTriState.TRUE; } - String toolNamePrompt = toolName; - if (showPrompt) { - if (StringUtils.isEmptyOrNull(toolNamePrompt)) { - toolNamePrompt = diffTools.getDefaultToolName(gui); - } - } + // get passed or default tool name + String toolNameToUse = promptToolName(); // get the changed files List files = getFiles(); if (files.size() > 0) { - compare(files, showPrompt, toolNamePrompt); + compare(files, showPrompt, toolNameToUse); } } - outw.flush(); } catch (RevisionSyntaxException | IOException e) { throw die(e.getMessage(), e); } finally { @@ -164,8 +160,32 @@ protected void run() { } } + private String promptToolName() throws IOException { + String toolNameToUse = toolName; + if (StringUtils.isEmptyOrNull(toolNameToUse)) { + toolNameToUse = diffTools.getDefaultToolName(gui); + } + if (StringUtils.isEmptyOrNull(toolNameToUse)) { + Map predefTools = diffTools + .getPredefinedTools(false); + StringBuilder toolNames = new StringBuilder(); + for (String name : predefTools.keySet()) { + toolNames.append(name + " "); //$NON-NLS-1$ + } + outw.println(MessageFormat.format( + CLIText.get().diffToolPromptToolName, toolNames)); + outw.flush(); + toolNameToUse = diffTools.getFirstAvailableTool(); + } + if (StringUtils.isEmptyOrNull(toolNameToUse)) { + throw new IOException(MessageFormat + .format(CLIText.get().diffToolUnknownToolName, toolName)); + } + return toolNameToUse; + } + private void compare(List files, boolean showPrompt, - String toolNamePrompt) throws IOException { + String toolNameToUse) throws IOException { ContentSource.Pair sourcePair = new ContentSource.Pair(source(oldTree), source(newTree)); try { @@ -179,7 +199,7 @@ private void compare(List files, boolean showPrompt, boolean launchCompare = true; if (showPrompt) { launchCompare = isLaunchCompare(fileIndex + 1, files.size(), - mergedFilePath, toolNamePrompt); + mergedFilePath, toolNameToUse); } if (launchCompare) { try { @@ -195,17 +215,21 @@ private void compare(List files, boolean showPrompt, // to jgit / java runtime ? // int rc =... ExecutionResult result = diffTools.compare(local, - remote, merged, toolName, prompt, gui, + remote, merged, toolNameToUse, prompt, gui, trustExitCode); outw.println(new String(result.getStdout().toByteArray())); + outw.flush(); errw.println( new String(result.getStderr().toByteArray())); + errw.flush(); } catch (ToolException e) { outw.println(e.getResultStdout()); outw.flush(); errw.println(e.getMessage()); + errw.flush(); throw die(MessageFormat.format( - CLIText.get().diffToolDied, mergedFilePath), e); + CLIText.get().diffToolDied, mergedFilePath, e), + e); } } else { break; @@ -232,16 +256,17 @@ private boolean isLaunchCompare(int fileIndex, int fileCount, } return launchCompare; } - private void showToolHelp() throws IOException { + Map predefTools = diffTools + .getPredefinedTools(true); StringBuilder availableToolNames = new StringBuilder(); - for (String name : diffTools.getAvailableTools().keySet()) { - availableToolNames.append(MessageFormat.format("\t\t{0}\n", name)); //$NON-NLS-1$ - } StringBuilder notAvailableToolNames = new StringBuilder(); - for (String name : diffTools.getNotAvailableTools().keySet()) { - notAvailableToolNames - .append(MessageFormat.format("\t\t{0}\n", name)); //$NON-NLS-1$ + for (String name : predefTools.keySet()) { + if (predefTools.get(name).isAvailable()) { + availableToolNames.append(MessageFormat.format("\t\t{0}\n", name)); //$NON-NLS-1$ + } else { + notAvailableToolNames.append(MessageFormat.format("\t\t{0}\n", name)); //$NON-NLS-1$ + } } StringBuilder userToolNames = new StringBuilder(); Map userTools = diffTools diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/MergeTool.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/MergeTool.java index 971277075..f5884c44d 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/MergeTool.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/MergeTool.java @@ -48,6 +48,7 @@ import org.eclipse.jgit.treewalk.filter.PathFilterGroup; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.util.StringUtils; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.internal.BooleanTriState; import org.eclipse.jgit.lib.CoreConfig.EolStreamType; @@ -121,14 +122,11 @@ protected void run() { showPrompt = prompt == BooleanTriState.TRUE; } // get passed or default tool name - String toolNameSelected = toolName; - if ((toolNameSelected == null) || toolNameSelected.isEmpty()) { - toolNameSelected = mergeTools.getDefaultToolName(gui); - } + String toolNameToUse = promptToolName(); // get the changed files Map files = getFiles(); if (files.size() > 0) { - merge(files, showPrompt, toolNameSelected); + merge(files, showPrompt, toolNameToUse); } else { outw.println(CLIText.get().mergeToolNoFiles); } @@ -139,6 +137,30 @@ protected void run() { } } + private String promptToolName() throws IOException { + String toolNameToUse = toolName; + if (StringUtils.isEmptyOrNull(toolNameToUse)) { + toolNameToUse = mergeTools.getDefaultToolName(gui); + } + if (StringUtils.isEmptyOrNull(toolNameToUse)) { + Map predefTools = mergeTools + .getPredefinedTools(false); + StringBuilder toolNames = new StringBuilder(); + for (String name : predefTools.keySet()) { + toolNames.append(name + " "); //$NON-NLS-1$ + } + outw.println(MessageFormat + .format(CLIText.get().mergeToolPromptToolName, toolNames)); + outw.flush(); + toolNameToUse = mergeTools.getFirstAvailableTool(); + } + if (StringUtils.isEmptyOrNull(toolNameToUse)) { + throw new IOException(MessageFormat + .format(CLIText.get().mergeToolUnknownToolName, toolName)); + } + return toolNameToUse; + } + private void merge(Map files, boolean showPrompt, String toolNamePrompt) throws Exception { // sort file names @@ -420,14 +442,16 @@ private int getDeletedMergeDecision() throws IOException { } private void showToolHelp() throws IOException { + Map predefTools = mergeTools + .getPredefinedTools(true); StringBuilder availableToolNames = new StringBuilder(); - for (String name : mergeTools.getAvailableTools().keySet()) { - availableToolNames.append(MessageFormat.format("\t\t{0}\n", name)); //$NON-NLS-1$ - } StringBuilder notAvailableToolNames = new StringBuilder(); - for (String name : mergeTools.getNotAvailableTools().keySet()) { - notAvailableToolNames - .append(MessageFormat.format("\t\t{0}\n", name)); //$NON-NLS-1$ + for (String name : predefTools.keySet()) { + if (predefTools.get(name).isAvailable()) { + availableToolNames.append(MessageFormat.format("\t\t{0}\n", name)); //$NON-NLS-1$ + } else { + notAvailableToolNames.append(MessageFormat.format("\t\t{0}\n", name)); //$NON-NLS-1$ + } } StringBuilder userToolNames = new StringBuilder(); Map userTools = mergeTools diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/internal/CLIText.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/internal/CLIText.java index 989e649b7..e06f150e5 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/internal/CLIText.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/internal/CLIText.java @@ -139,6 +139,8 @@ public static String fatalError(String message) { /***/ public String diffToolHelpSetToFollowing; /***/ public String diffToolLaunch; /***/ public String diffToolDied; + /***/ public String diffToolPromptToolName; + /***/ public String diffToolUnknownToolName; /***/ public String doesNotExist; /***/ public String dontOverwriteLocalChanges; /***/ public String everythingUpToDate; @@ -185,6 +187,8 @@ public static String fatalError(String message) { /***/ public String mergeToolContinueUnresolvedPaths; /***/ public String mergeToolWasMergeSuccessfull; /***/ public String mergeToolDeletedMergeDecision; + /***/ public String mergeToolPromptToolName; + /***/ public String mergeToolUnknownToolName; /***/ public String mergeFailed; /***/ public String mergeCheckoutFailed; /***/ public String mergeMadeBy; diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalDiffToolTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalDiffToolTest.java index 4fd55c6ca..1b501c25b 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalDiffToolTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalDiffToolTest.java @@ -97,7 +97,7 @@ public void testToolNames() { @Test public void testAllTools() { DiffTools manager = new DiffTools(db); - Set actualToolNames = manager.getAvailableTools().keySet(); + Set actualToolNames = manager.getPredefinedTools(true).keySet(); Set expectedToolNames = new LinkedHashSet<>(); CommandLineDiffTool[] defaultTools = CommandLineDiffTool.values(); for (CommandLineDiffTool defaultTool : defaultTools) { @@ -152,15 +152,6 @@ public void testUserDefinedTools() { actualToolNames); } - @Test - public void testNotAvailableTools() { - DiffTools manager = new DiffTools(db); - Set actualToolNames = manager.getNotAvailableTools().keySet(); - Set expectedToolNames = Collections.emptySet(); - assertEquals("Incorrect set of not available external diff tools", - expectedToolNames, actualToolNames); - } - @Test public void testCompare() throws ToolException { String toolName = "customTool"; @@ -239,7 +230,7 @@ public void testOverridePreDefinedToolPath() { DiffTools manager = new DiffTools(db); Map availableTools = manager - .getAvailableTools(); + .getPredefinedTools(true); ExternalDiffTool externalDiffTool = availableTools .get(overridenToolName); String actualDiffToolPath = externalDiffTool.getPath(); diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalMergeToolTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalMergeToolTest.java index 50576682e..305f2b470 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalMergeToolTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalMergeToolTest.java @@ -9,14 +9,14 @@ */ package org.eclipse.jgit.internal.diffmergetool; -import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_MERGETOOL_SECTION; -import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_MERGE_SECTION; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_CMD; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_GUITOOL; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_PATH; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_PROMPT; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_TOOL; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_TRUST_EXIT_CODE; +import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_MERGETOOL_SECTION; +import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_MERGE_SECTION; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; @@ -95,7 +95,7 @@ public void testToolNames() { @Test public void testAllTools() { MergeTools manager = new MergeTools(db); - Set actualToolNames = manager.getAvailableTools().keySet(); + Set actualToolNames = manager.getPredefinedTools(true).keySet(); Set expectedToolNames = new LinkedHashSet<>(); CommandLineMergeTool[] defaultTools = CommandLineMergeTool.values(); for (CommandLineMergeTool defaultTool : defaultTools) { @@ -150,15 +150,6 @@ public void testUserDefinedTools() { actualToolNames); } - @Test - public void testNotAvailableTools() { - MergeTools manager = new MergeTools(db); - Set actualToolNames = manager.getNotAvailableTools().keySet(); - Set expectedToolNames = Collections.emptySet(); - assertEquals("Incorrect set of not available external merge tools", - expectedToolNames, actualToolNames); - } - @Test public void testCompare() throws ToolException { String toolName = "customTool"; @@ -236,7 +227,7 @@ public void testOverridePreDefinedToolPath() { MergeTools manager = new MergeTools(db); Map availableTools = manager - .getAvailableTools(); + .getPredefinedTools(true); ExternalMergeTool externalMergeTool = availableTools .get(overridenToolName); String actualMergeToolPath = externalMergeTool.getPath(); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/CommandExecutor.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/CommandExecutor.java index ad79fe8fc..668adeab6 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/CommandExecutor.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/CommandExecutor.java @@ -14,13 +14,19 @@ import java.io.FileOutputStream; import java.io.IOException; import java.io.OutputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; import java.util.Arrays; import java.util.Map; + +import org.eclipse.jgit.errors.NoWorkTreeException; import org.eclipse.jgit.util.FS; import org.eclipse.jgit.util.FS.ExecutionResult; import org.eclipse.jgit.util.FS_POSIX; import org.eclipse.jgit.util.FS_Win32; import org.eclipse.jgit.util.FS_Win32_Cygwin; +import org.eclipse.jgit.util.StringUtils; /** * Runs a command with help of FS. @@ -91,6 +97,49 @@ public ExecutionResult run(String command, File workingDir, } } + /** + * @param path + * the executable path + * @param workingDir + * the working directory + * @param env + * the environment + * @return the execution result + * @throws ToolException + * @throws InterruptedException + * @throws IOException + */ + public boolean checkExecutable(String path, File workingDir, + Map env) + throws ToolException, IOException, InterruptedException { + checkUseMsys2(path); + String command = null; + if (fs instanceof FS_Win32 && !useMsys2) { + Path p = Paths.get(path); + // Win32 (and not cygwin or MSYS2) where accepts only command / exe + // name as parameter + // so check if exists and executable in this case + if (p.isAbsolute() && Files.isExecutable(p)) { + return true; + } + // try where command for all other cases + command = "where " + ExternalToolUtils.quotePath(path); //$NON-NLS-1$ + } else { + command = "which " + ExternalToolUtils.quotePath(path); //$NON-NLS-1$ + } + boolean available = true; + try { + ExecutionResult rc = run(command, workingDir, env); + if (rc.getRc() != 0) { + available = false; + } + } catch (IOException | InterruptedException | NoWorkTreeException + | ToolException e) { + // no op: is true to not hide possible tools from user + } + return available; + } + private void deleteCommandArray() { deleteCommandFile(); } @@ -127,7 +176,7 @@ private String[] createCommandArray(String command) private void checkUseMsys2(String command) { useMsys2 = false; String useMsys2Str = System.getProperty("jgit.usemsys2bash"); //$NON-NLS-1$ - if (useMsys2Str != null && !useMsys2Str.isEmpty()) { + if (!StringUtils.isEmptyOrNull(useMsys2Str)) { if (useMsys2Str.equalsIgnoreCase("auto")) { //$NON-NLS-1$ useMsys2 = command.contains(".sh"); //$NON-NLS-1$ } else { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/CommandLineDiffTool.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/CommandLineDiffTool.java index 509515c37..00dec3271 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/CommandLineDiffTool.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/CommandLineDiffTool.java @@ -111,7 +111,7 @@ public enum CommandLineDiffTool { * See: http://vimdoc.sourceforge.net/htmldoc/diff.html */ - gvimdiff("gviewdiff", "\"$LOCAL\" \"$REMOTE\""), + gvimdiff("gvimdiff", "\"$LOCAL\" \"$REMOTE\""), /** * See: http://vimdoc.sourceforge.net/htmldoc/diff.html @@ -160,7 +160,7 @@ public enum CommandLineDiffTool { * See: http://vimdoc.sourceforge.net/htmldoc/diff.html */ - vimdiff("viewdiff", gvimdiff), + vimdiff("vimdiff", gvimdiff), /** * See: http://vimdoc.sourceforge.net/htmldoc/diff.html diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/DiffTools.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/DiffTools.java index 1dcc523bf..6c1a780a3 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/DiffTools.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/DiffTools.java @@ -111,17 +111,38 @@ public Map getUserDefinedTools() { } /** - * @return the available predefined tools + * @param checkAvailability + * true: for checking if tools can be executed; ATTENTION: this + * check took some time, do not execute often (store the map for + * other actions); false: availability is NOT checked: + * isAvailable() returns default false is this case! + * @return the predefined tools with optionally checked availability (long + * running operation) */ - public Map getAvailableTools() { + public Map getPredefinedTools( + boolean checkAvailability) { + if (checkAvailability) { + for (ExternalDiffTool tool : predefinedTools.values()) { + PreDefinedDiffTool predefTool = (PreDefinedDiffTool) tool; + predefTool.setAvailable(ExternalToolUtils.isToolAvailable(repo, + predefTool.getPath())); + } + } return Collections.unmodifiableMap(predefinedTools); } /** - * @return the NOT available predefined tools + * @return the name of first available predefined tool or null */ - public Map getNotAvailableTools() { - return Collections.unmodifiableMap(new TreeMap<>()); + public String getFirstAvailableTool() { + String name = null; + for (ExternalDiffTool tool : predefinedTools.values()) { + if (ExternalToolUtils.isToolAvailable(repo, tool.getPath())) { + name = tool.getName(); + break; + } + } + return name; } /** @@ -146,9 +167,12 @@ private ExternalDiffTool guessTool(String toolName, BooleanTriState gui) if (StringUtils.isEmptyOrNull(toolName)) { toolName = getDefaultToolName(gui); } - ExternalDiffTool tool = getTool(toolName); + ExternalDiffTool tool = null; + if (!StringUtils.isEmptyOrNull(toolName)) { + tool = getTool(toolName); + } if (tool == null) { - throw new ToolException("Unknown diff tool " + toolName); //$NON-NLS-1$ + throw new ToolException("Unknown diff tool '" + toolName + "'"); //$NON-NLS-1$ //$NON-NLS-2$ } return tool; } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ExternalDiffTool.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ExternalDiffTool.java index f2d7e828c..e01b892a5 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ExternalDiffTool.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ExternalDiffTool.java @@ -30,4 +30,10 @@ public interface ExternalDiffTool { */ String getCommand(); + /** + * @return availability of the tool: true if tool can be executed and false + * if not + */ + boolean isAvailable(); + } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ExternalToolUtils.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ExternalToolUtils.java index 3efb90c49..e10607d2f 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ExternalToolUtils.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ExternalToolUtils.java @@ -39,9 +39,15 @@ public class ExternalToolUtils { public static String prepareCommand(String command, FileElement localFile, FileElement remoteFile, FileElement mergedFile, FileElement baseFile) throws IOException { - command = localFile.replaceVariable(command); - command = remoteFile.replaceVariable(command); - command = mergedFile.replaceVariable(command); + if (localFile != null) { + command = localFile.replaceVariable(command); + } + if (remoteFile != null) { + command = remoteFile.replaceVariable(command); + } + if (mergedFile != null) { + command = mergedFile.replaceVariable(command); + } if (baseFile != null) { command = baseFile.replaceVariable(command); } @@ -69,13 +75,59 @@ public static Map prepareEnvironment(Repository repo, FileElement mergedFile, FileElement baseFile) throws IOException { Map env = new TreeMap<>(); env.put(Constants.GIT_DIR_KEY, repo.getDirectory().getAbsolutePath()); - localFile.addToEnv(env); - remoteFile.addToEnv(env); - mergedFile.addToEnv(env); + if (localFile != null) { + localFile.addToEnv(env); + } + if (remoteFile != null) { + remoteFile.addToEnv(env); + } + if (mergedFile != null) { + mergedFile.addToEnv(env); + } if (baseFile != null) { baseFile.addToEnv(env); } return env; } + /** + * @param path + * the path to be quoted + * @return quoted path if it contains spaces + */ + @SuppressWarnings("nls") + public static String quotePath(String path) { + // handling of spaces in path + if (path.contains(" ")) { + // add quotes before if needed + if (!path.startsWith("\"")) { + path = "\"" + path; + } + // add quotes after if needed + if (!path.endsWith("\"")) { + path = path + "\""; + } + } + return path; + } + + /** + * @param repo + * the repository + * @param path + * the tool path + * @return true if tool available and false otherwise + */ + public static boolean isToolAvailable(Repository repo, String path) { + boolean available = true; + try { + CommandExecutor cmdExec = new CommandExecutor(repo.getFS(), false); + available = cmdExec.checkExecutable(path, repo.getWorkTree(), + prepareEnvironment(repo, null, null, null, null)); + } catch (Exception e) { + available = false; + } + return available; + } + } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/MergeTools.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/MergeTools.java index 9a2a8304e..d91d57f1a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/MergeTools.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/MergeTools.java @@ -23,6 +23,7 @@ import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.internal.BooleanTriState; import org.eclipse.jgit.util.FS.ExecutionResult; +import org.eclipse.jgit.util.StringUtils; /** * Manages merge tools. @@ -161,17 +162,38 @@ public Map getUserDefinedTools() { } /** - * @return the available predefined tools + * @param checkAvailability + * true: for checking if tools can be executed; ATTENTION: this + * check took some time, do not execute often (store the map for + * other actions); false: availability is NOT checked: + * isAvailable() returns default false is this case! + * @return the predefined tools with optionally checked availability (long + * running operation) */ - public Map getAvailableTools() { + public Map getPredefinedTools( + boolean checkAvailability) { + if (checkAvailability) { + for (ExternalMergeTool tool : predefinedTools.values()) { + PreDefinedMergeTool predefTool = (PreDefinedMergeTool) tool; + predefTool.setAvailable(ExternalToolUtils.isToolAvailable(repo, + predefTool.getPath())); + } + } return predefinedTools; } /** - * @return the NOT available predefined tools + * @return the name of first available predefined tool or null */ - public Map getNotAvailableTools() { - return new TreeMap<>(); + public String getFirstAvailableTool() { + String name = null; + for (ExternalMergeTool tool : predefinedTools.values()) { + if (ExternalToolUtils.isToolAvailable(repo, tool.getPath())) { + name = tool.getName(); + break; + } + } + return name; } /** @@ -193,12 +215,15 @@ public boolean isInteractive() { private ExternalMergeTool guessTool(String toolName, BooleanTriState gui) throws ToolException { - if ((toolName == null) || toolName.isEmpty()) { + if (StringUtils.isEmptyOrNull(toolName)) { toolName = getDefaultToolName(gui); } - ExternalMergeTool tool = getTool(toolName); + ExternalMergeTool tool = null; + if (!StringUtils.isEmptyOrNull(toolName)) { + tool = getTool(toolName); + } if (tool == null) { - throw new ToolException("Unknown diff tool " + toolName); //$NON-NLS-1$ + throw new ToolException("Unknown merge tool '" + toolName + "'"); //$NON-NLS-1$ //$NON-NLS-2$ } return tool; } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/PreDefinedDiffTool.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/PreDefinedDiffTool.java index 092cb605b..e1169a2d6 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/PreDefinedDiffTool.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/PreDefinedDiffTool.java @@ -56,7 +56,7 @@ public void setPath(String path) { */ @Override public String getCommand() { - return getPath() + " " + super.getCommand(); //$NON-NLS-1$ + return ExternalToolUtils.quotePath(getPath()) + " " + super.getCommand(); //$NON-NLS-1$ } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/PreDefinedMergeTool.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/PreDefinedMergeTool.java index 2c64c1666..7b28d3282 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/PreDefinedMergeTool.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/PreDefinedMergeTool.java @@ -84,7 +84,7 @@ public String getCommand() { */ @Override public String getCommand(boolean withBase) { - return getPath() + " " //$NON-NLS-1$ + return ExternalToolUtils.quotePath(getPath()) + " " //$NON-NLS-1$ + (withBase ? super.getCommand() : parametersWithoutBase); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ToolException.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ToolException.java index 27f7d12e6..7cc5bb50d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ToolException.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ToolException.java @@ -110,6 +110,9 @@ public boolean isCommandExecutionError() { * @return the result Stderr */ public String getResultStderr() { + if (result == null) { + return ""; //$NON-NLS-1$ + } try { return new String(result.getStderr().toByteArray()); } catch (Exception e) { @@ -122,6 +125,9 @@ public String getResultStderr() { * @return the result Stdout */ public String getResultStdout() { + if (result == null) { + return ""; //$NON-NLS-1$ + } try { return new String(result.getStdout().toByteArray()); } catch (Exception e) { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/UserDefinedDiffTool.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/UserDefinedDiffTool.java index 012296eb3..eb72d01cd 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/UserDefinedDiffTool.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/UserDefinedDiffTool.java @@ -15,6 +15,8 @@ */ public class UserDefinedDiffTool implements ExternalDiffTool { + private boolean available; + /** * the diff tool name */ @@ -98,6 +100,23 @@ public String getCommand() { return cmd; } + /** + * @return availability of the tool: true if tool can be executed and false + * if not + */ + @Override + public boolean isAvailable() { + return available; + } + + /** + * @param available + * true if tool can be found and false if not + */ + public void setAvailable(boolean available) { + this.available = available; + } + /** * Overrides the path for the given tool. Equivalent to setting * {@code difftool..path}. From ff77d412a9f1a956b8d76b139489038ee3b0870c Mon Sep 17 00:00:00 2001 From: Andre Bossert Date: Sun, 19 Jan 2020 20:57:23 +0100 Subject: [PATCH 2/3] Adapt diff- and merge tool code for PGM and EGit usage see: https://git-scm.com/docs/git-mergetool * DiffTools and MergeTools * store FS, gitDir and workTree for usage without git repository (for EGit preferences) * add getUserDefinedToolNames() and getPredefinedToolNames() * replace getToolNames() with getAllToolNames() that combines the two lists and put default tool name (diff.tool or merge.tool) as first element (for EGit preferences) * FileElement: refactoring of getFile() and friends to have midName (LOCAL, REMOTE etc.) always added to the temp file name (also for EGit) * FileElement: added directory attribute that is used in getFile() to return path with workDir as parent * DiffTool and MergeTool * added errw.flush(), because sometimes stderr is not printed in case of die() * print e.getMessage() always to stderr * Moved toolname and prompt logic into managers * Exported internal packages required for egit.ui Bug: 356832 Change-Id: I71e7f4dc362169a7612ca4f6546a021bc4b2b5f4 Signed-off-by: Andre Bossert Signed-off-by: Tim Neumann --- .../org/eclipse/jgit/pgm/DiffToolTest.java | 114 ++++++++- .../org/eclipse/jgit/pgm/MergeToolTest.java | 77 +++++- .../org/eclipse/jgit/pgm/ToolTestCase.java | 55 +++- .../src/org/eclipse/jgit/pgm/DiffTool.java | 175 +++++++------ .../src/org/eclipse/jgit/pgm/MergeTool.java | 137 +++++----- .../diffmergetool/ExternalDiffToolTest.java | 153 ++++++++---- .../diffmergetool/ExternalMergeToolTest.java | 187 +++++++++++--- .../diffmergetool/ExternalToolTestCase.java | 37 +++ org.eclipse.jgit/META-INF/MANIFEST.MF | 6 +- .../internal/diffmergetool/DiffTools.java | 230 ++++++++++++----- .../diffmergetool/ExternalToolUtils.java | 66 ++++- .../internal/diffmergetool/FileElement.java | 185 +++++++------- .../diffmergetool/InformNoToolHandler.java | 28 +++ .../diffmergetool/MergeToolConfig.java | 2 +- .../internal/diffmergetool/MergeTools.java | 236 +++++++++++++----- .../diffmergetool/PromptContinueHandler.java | 27 ++ 16 files changed, 1222 insertions(+), 493 deletions(-) create mode 100644 org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/InformNoToolHandler.java create mode 100644 org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/PromptContinueHandler.java diff --git a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/DiffToolTest.java b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/DiffToolTest.java index a258821f0..ce4c004c0 100644 --- a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/DiffToolTest.java +++ b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/DiffToolTest.java @@ -16,11 +16,14 @@ import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_TOOL; import static org.junit.Assert.fail; +import java.io.File; import java.io.InputStream; +import java.nio.file.Path; import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Map; +import java.util.regex.Pattern; import org.eclipse.jgit.internal.diffmergetool.DiffTools; import org.eclipse.jgit.internal.diffmergetool.ExternalDiffTool; @@ -42,6 +45,58 @@ public void setUp() throws Exception { configureEchoTool(TOOL_NAME); } + @Test(expected = Die.class) + public void testUndefinedTool() throws Exception { + String toolName = "undefined"; + String[] conflictingFilenames = createUnstagedChanges(); + + List expectedErrors = new ArrayList<>(); + for (String changedFilename : conflictingFilenames) { + expectedErrors.add("External diff tool is not defined: " + toolName); + expectedErrors.add("compare of " + changedFilename + " failed"); + } + + runAndCaptureUsingInitRaw(expectedErrors, DIFF_TOOL, "--no-prompt", + "--tool", toolName); + fail("Expected exception to be thrown due to undefined external tool"); + } + + @Test(expected = Die.class) + public void testUserToolWithCommandNotFoundError() throws Exception { + String toolName = "customTool"; + + int errorReturnCode = 127; // command not found + String command = "exit " + errorReturnCode; + + StoredConfig config = db.getConfig(); + config.setString(CONFIG_DIFFTOOL_SECTION, toolName, CONFIG_KEY_CMD, + command); + + createMergeConflict(); + runAndCaptureUsingInitRaw(DIFF_TOOL, "--no-prompt", "--tool", toolName); + + fail("Expected exception to be thrown due to external tool exiting with error code: " + + errorReturnCode); + } + + @Test + public void testEmptyToolName() throws Exception { + String emptyToolName = ""; + + StoredConfig config = db.getConfig(); + // the default diff tool is configured without a subsection + String subsection = null; + config.setString(CONFIG_DIFF_SECTION, subsection, CONFIG_KEY_TOOL, + emptyToolName); + + createUnstagedChanges(); + + String araxisErrorLine = "compare: unrecognized option `-wait' @ error/compare.c/CompareImageCommand/1123."; + String[] expectedErrorOutput = { araxisErrorLine, araxisErrorLine, }; + runAndCaptureUsingInitRaw(Arrays.asList(expectedErrorOutput), DIFF_TOOL, + "--no-prompt"); + } + @Test public void testToolWithPrompt() throws Exception { String[] inputLines = { @@ -138,12 +193,12 @@ expectedOutput, runAndCaptureUsingInitRaw(DIFF_TOOL, @Test public void testToolCached() throws Exception { String[] conflictingFilenames = createStagedChanges(); - String[] expectedOutput = getExpectedToolOutputNoPrompt(conflictingFilenames); + Pattern[] expectedOutput = getExpectedCachedToolOutputNoPrompt(conflictingFilenames); String[] options = { "--cached", "--staged", }; for (String option : options) { - assertArrayOfLinesEquals("Incorrect output for option: " + option, + assertArrayOfMatchingLines("Incorrect output for option: " + option, expectedOutput, runAndCaptureUsingInitRaw(DIFF_TOOL, option, "--tool", TOOL_NAME)); } @@ -213,43 +268,76 @@ private void configureEchoTool(String toolName) { String.valueOf(false)); } - private static String[] getExpectedToolOutputNoPrompt(String[] conflictingFilenames) { + private String[] getExpectedToolOutputNoPrompt(String[] conflictingFilenames) { String[] expectedToolOutput = new String[conflictingFilenames.length]; for (int i = 0; i < conflictingFilenames.length; ++i) { String newPath = conflictingFilenames[i]; - String expectedLine = newPath; - expectedToolOutput[i] = expectedLine; + Path fullPath = getFullPath(newPath); + expectedToolOutput[i] = fullPath.toString(); } return expectedToolOutput; } - private static String[] getExpectedCompareOutput(String[] conflictingFilenames) { + private Pattern[] getExpectedCachedToolOutputNoPrompt(String[] conflictingFilenames) { + String tmpDir = System.getProperty("java.io.tmpdir"); + if (tmpDir.endsWith(File.separator)) { + tmpDir = tmpDir.substring(0, tmpDir.length() - 1); + } + Pattern emptyPattern = Pattern.compile(""); + List expectedToolOutput = new ArrayList<>(); + for (int i = 0; i < conflictingFilenames.length; ++i) { + String changedFilename = conflictingFilenames[i]; + Path fullPath = getFullPath(changedFilename); + String filename = fullPath.getFileName().toString(); + String regexp = tmpDir + File.separatorChar + filename + + "_REMOTE_.*"; + Pattern pattern = Pattern.compile(regexp); + expectedToolOutput.add(pattern); + expectedToolOutput.add(emptyPattern); + } + expectedToolOutput.add(emptyPattern); + return expectedToolOutput.toArray(new Pattern[0]); + } + + private String[] getExpectedCompareOutput(String[] conflictingFilenames) { List expected = new ArrayList<>(); int n = conflictingFilenames.length; for (int i = 0; i < n; ++i) { - String newPath = conflictingFilenames[i]; + String changedFilename = conflictingFilenames[i]; expected.add( - "Viewing (" + (i + 1) + "/" + n + "): '" + newPath + "'"); + "Viewing (" + (i + 1) + "/" + n + "): '" + changedFilename + + "'"); expected.add("Launch '" + TOOL_NAME + "' [Y/n]?"); - expected.add(newPath); + Path fullPath = getFullPath(changedFilename); + expected.add(fullPath.toString()); } return expected.toArray(new String[0]); } - private static String[] getExpectedAbortOutput(String[] conflictingFilenames, + private String[] getExpectedAbortOutput(String[] conflictingFilenames, int abortIndex) { List expected = new ArrayList<>(); int n = conflictingFilenames.length; for (int i = 0; i < n; ++i) { - String newPath = conflictingFilenames[i]; + String changedFilename = conflictingFilenames[i]; expected.add( - "Viewing (" + (i + 1) + "/" + n + "): '" + newPath + "'"); + "Viewing (" + (i + 1) + "/" + n + "): '" + changedFilename + + "'"); expected.add("Launch '" + TOOL_NAME + "' [Y/n]?"); if (i == abortIndex) { break; } - expected.add(newPath); + Path fullPath = getFullPath(changedFilename); + expected.add(fullPath.toString()); } return expected.toArray(new String[0]); } + + private static String getEchoCommand() { + /* + * use 'REMOTE' placeholder, as it will be replaced by a file path + * within the repository. + */ + return "(echo \"$REMOTE\")"; + } } diff --git a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/MergeToolTest.java b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/MergeToolTest.java index e9d559e70..1236dd30d 100644 --- a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/MergeToolTest.java +++ b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/MergeToolTest.java @@ -14,8 +14,10 @@ import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_TOOL; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_MERGETOOL_SECTION; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_MERGE_SECTION; +import static org.junit.Assert.fail; import java.io.InputStream; +import java.nio.file.Path; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -41,6 +43,58 @@ public void setUp() throws Exception { configureEchoTool(TOOL_NAME); } + @Test + public void testUndefinedTool() throws Exception { + String toolName = "undefined"; + String[] conflictingFilenames = createMergeConflict(); + + List expectedErrors = new ArrayList<>(); + for (String conflictingFilename : conflictingFilenames) { + expectedErrors.add("External merge tool is not defined: " + toolName); + expectedErrors.add("merge of " + conflictingFilename + " failed"); + } + + runAndCaptureUsingInitRaw(expectedErrors, MERGE_TOOL, + "--no-prompt", "--tool", toolName); + } + + @Test(expected = Die.class) + public void testUserToolWithCommandNotFoundError() throws Exception { + String toolName = "customTool"; + + int errorReturnCode = 127; // command not found + String command = "exit " + errorReturnCode; + + StoredConfig config = db.getConfig(); + config.setString(CONFIG_MERGETOOL_SECTION, toolName, CONFIG_KEY_CMD, + command); + + createMergeConflict(); + runAndCaptureUsingInitRaw(MERGE_TOOL, "--no-prompt", "--tool", + toolName); + + fail("Expected exception to be thrown due to external tool exiting with error code: " + + errorReturnCode); + } + + @Test + public void testEmptyToolName() throws Exception { + String emptyToolName = ""; + + StoredConfig config = db.getConfig(); + // the default merge tool is configured without a subsection + String subsection = null; + config.setString(CONFIG_MERGE_SECTION, subsection, CONFIG_KEY_TOOL, + emptyToolName); + + createMergeConflict(); + + String araxisErrorLine = "compare: unrecognized option `-wait' @ error/compare.c/CompareImageCommand/1123."; + String[] expectedErrorOutput = { araxisErrorLine, araxisErrorLine, }; + runAndCaptureUsingInitRaw(Arrays.asList(expectedErrorOutput), + MERGE_TOOL, "--no-prompt"); + } + @Test public void testAbortMerge() throws Exception { String[] inputLines = { @@ -220,7 +274,7 @@ private void configureEchoTool(String toolName) { String.valueOf(false)); } - private static String[] getExpectedMergeConflictOutputNoPrompt( + private String[] getExpectedMergeConflictOutputNoPrompt( String[] conflictFilenames) { List expected = new ArrayList<>(); expected.add("Merging:"); @@ -232,7 +286,8 @@ private static String[] getExpectedMergeConflictOutputNoPrompt( + "':"); expected.add("{local}: modified file"); expected.add("{remote}: modified file"); - expected.add(conflictFilename); + Path filePath = getFullPath(conflictFilename); + expected.add(filePath.toString()); expected.add(conflictFilename + " seems unchanged."); } return expected.toArray(new String[0]); @@ -257,7 +312,7 @@ private static String[] getExpectedAbortLaunchOutput( return expected.toArray(new String[0]); } - private static String[] getExpectedAbortMergeOutput( + private String[] getExpectedAbortMergeOutput( String[] conflictFilenames, int abortIndex) { List expected = new ArrayList<>(); expected.add("Merging:"); @@ -274,8 +329,9 @@ private static String[] getExpectedAbortMergeOutput( "Normal merge conflict for '" + conflictFilename + "':"); expected.add("{local}: modified file"); expected.add("{remote}: modified file"); + Path fullPath = getFullPath(conflictFilename); expected.add("Hit return to start merge resolution tool (" - + TOOL_NAME + "): " + conflictFilename); + + TOOL_NAME + "): " + fullPath); expected.add(conflictFilename + " seems unchanged."); expected.add("Was the merge successful [y/n]?"); if (i < conflictFilenames.length - 1) { @@ -286,7 +342,7 @@ private static String[] getExpectedAbortMergeOutput( return expected.toArray(new String[0]); } - private static String[] getExpectedMergeConflictOutput( + private String[] getExpectedMergeConflictOutput( String[] conflictFilenames) { List expected = new ArrayList<>(); expected.add("Merging:"); @@ -299,8 +355,9 @@ private static String[] getExpectedMergeConflictOutput( + "':"); expected.add("{local}: modified file"); expected.add("{remote}: modified file"); + Path filePath = getFullPath(conflictFilename); expected.add("Hit return to start merge resolution tool (" - + TOOL_NAME + "): " + conflictFilename); + + TOOL_NAME + "): " + filePath); expected.add(conflictFilename + " seems unchanged."); expected.add("Was the merge successful [y/n]?"); if (i < conflictFilenames.length - 1) { @@ -327,4 +384,12 @@ private static String[] getExpectedDeletedConflictOutput( } return expected.toArray(new String[0]); } + + private static String getEchoCommand() { + /* + * use 'MERGED' placeholder, as both 'LOCAL' and 'REMOTE' will be + * replaced with full paths to a temporary file during some of the tests + */ + return "(echo \"$MERGED\")"; + } } diff --git a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/ToolTestCase.java b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/ToolTestCase.java index 933f19bcc..a3c41f0fe 100644 --- a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/ToolTestCase.java +++ b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/ToolTestCase.java @@ -10,13 +10,18 @@ package org.eclipse.jgit.pgm; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import java.io.ByteArrayInputStream; +import java.io.IOException; import java.io.InputStream; +import java.nio.file.Path; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import java.util.stream.Collectors; import org.eclipse.jgit.api.Git; @@ -29,6 +34,7 @@ import org.eclipse.jgit.treewalk.TreeWalk; import org.junit.Before; import org.kohsuke.args4j.Argument; +import org.kohsuke.args4j.CmdLineException; /** * Base test case for the {@code difftool} and {@code mergetool} commands. @@ -64,8 +70,23 @@ protected String[] runAndCaptureUsingInitRaw(String... args) return runAndCaptureUsingInitRaw(inputStream, args); } + protected String[] runAndCaptureUsingInitRaw( + List expectedErrorOutput, String... args) throws Exception { + InputStream inputStream = null; // no input stream + return runAndCaptureUsingInitRaw(inputStream, expectedErrorOutput, + args); + } + protected String[] runAndCaptureUsingInitRaw(InputStream inputStream, String... args) throws Exception { + List expectedErrorOutput = Collections.emptyList(); + return runAndCaptureUsingInitRaw(inputStream, expectedErrorOutput, + args); + } + + protected String[] runAndCaptureUsingInitRaw(InputStream inputStream, + List expectedErrorOutput, String... args) + throws CmdLineException, Exception, IOException { CLIGitCommand.Result result = new CLIGitCommand.Result(); GitCliJGitWrapperParser bean = new GitCliJGitWrapperParser(); @@ -86,7 +107,7 @@ protected String[] runAndCaptureUsingInitRaw(InputStream inputStream, .filter(l -> !l.isBlank()) // we care only about error messages .collect(Collectors.toList()); assertEquals("Expected no standard error output from tool", - Collections.EMPTY_LIST.toString(), errLines.toString()); + expectedErrorOutput.toString(), errLines.toString()); return result.outLines().toArray(new String[0]); } @@ -177,6 +198,13 @@ protected List getRepositoryChanges(RevCommit commit) return changes; } + protected Path getFullPath(String repositoryFilename) { + Path dotGitPath = db.getDirectory().toPath(); + Path repositoryRoot = dotGitPath.getParent(); + Path repositoryFilePath = repositoryRoot.resolve(repositoryFilename); + return repositoryFilePath; + } + protected static InputStream createInputStream(String[] inputLines) { return createInputStream(Arrays.asList(inputLines)); } @@ -192,11 +220,24 @@ protected static void assertArrayOfLinesEquals(String failMessage, assertEquals(failMessage, toString(expected), toString(actual)); } - protected static String getEchoCommand() { - /* - * use 'MERGED' placeholder, as both 'LOCAL' and 'REMOTE' will be - * replaced with full paths to a temporary file during some of the tests - */ - return "(echo \"$MERGED\")"; + protected static void assertArrayOfMatchingLines(String failMessage, + Pattern[] expected, String[] actual) { + assertEquals(failMessage + System.lineSeparator() + + "Expected and actual lines count don't match. Expected: " + + Arrays.asList(expected) + ", actual: " + + Arrays.asList(actual), expected.length, actual.length); + int n = expected.length; + for (int i = 0; i < n; ++i) { + Pattern expectedPattern = expected[i]; + String actualLine = actual[i]; + Matcher matcher = expectedPattern.matcher(actualLine); + boolean matches = matcher.matches(); + assertTrue(failMessage + System.lineSeparator() + "Line " + i + " '" + + actualLine + "' doesn't match expected pattern: " + + expectedPattern + System.lineSeparator() + "Expected: " + + Arrays.asList(expected) + ", actual: " + + Arrays.asList(actual), + matches); + } } } diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/DiffTool.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/DiffTool.java index e5a3c53e3..3e6042afe 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/DiffTool.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/DiffTool.java @@ -1,5 +1,6 @@ /* * Copyright (C) 2018-2022, Andre Bossert + * Copyright (C) 2019, Tim Neumann * * This program and the accompanying materials are made available under the * terms of the Eclipse Distribution License v. 1.0 which is available at @@ -22,6 +23,7 @@ import java.text.MessageFormat; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.concurrent.TimeUnit; import org.eclipse.jgit.diff.ContentSource; @@ -40,6 +42,7 @@ import org.eclipse.jgit.internal.diffmergetool.DiffTools; import org.eclipse.jgit.internal.diffmergetool.ExternalDiffTool; import org.eclipse.jgit.internal.diffmergetool.FileElement; +import org.eclipse.jgit.internal.diffmergetool.PromptContinueHandler; import org.eclipse.jgit.internal.diffmergetool.ToolException; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.CoreConfig.EolStreamType; @@ -60,7 +63,6 @@ import org.eclipse.jgit.treewalk.filter.PathFilterGroup; import org.eclipse.jgit.treewalk.filter.TreeFilter; import org.eclipse.jgit.util.FS.ExecutionResult; -import org.eclipse.jgit.util.StringUtils; import org.kohsuke.args4j.Argument; import org.kohsuke.args4j.Option; @@ -76,9 +78,13 @@ class DiffTool extends TextBuiltin { @Argument(index = 1, metaVar = "metaVar_treeish") private AbstractTreeIterator newTree; + private Optional toolName = Optional.empty(); + @Option(name = "--tool", aliases = { "-t" }, metaVar = "metaVar_tool", usage = "usage_ToolForDiff") - private String toolName; + void setToolName(String name) { + toolName = Optional.of(name); + } @Option(name = "--cached", aliases = { "--staged" }, usage = "usage_cached") private boolean cached; @@ -98,16 +104,16 @@ void noPrompt(@SuppressWarnings("unused") boolean on) { @Option(name = "--tool-help", usage = "usage_toolHelp") private boolean toolHelp; - private BooleanTriState gui = BooleanTriState.UNSET; + private boolean gui = false; @Option(name = "--gui", aliases = { "-g" }, usage = "usage_DiffGuiTool") void setGui(@SuppressWarnings("unused") boolean on) { - gui = BooleanTriState.TRUE; + gui = true; } @Option(name = "--no-gui", usage = "usage_noGui") void noGui(@SuppressWarnings("unused") boolean on) { - gui = BooleanTriState.FALSE; + gui = false; } private BooleanTriState trustExitCode = BooleanTriState.UNSET; @@ -141,16 +147,10 @@ protected void run() { if (toolHelp) { showToolHelp(); } else { - boolean showPrompt = diffTools.isInteractive(); - if (prompt != BooleanTriState.UNSET) { - showPrompt = prompt == BooleanTriState.TRUE; - } - // get passed or default tool name - String toolNameToUse = promptToolName(); // get the changed files List files = getFiles(); if (files.size() > 0) { - compare(files, showPrompt, toolNameToUse); + compare(files); } } } catch (RevisionSyntaxException | IOException e) { @@ -160,79 +160,103 @@ protected void run() { } } - private String promptToolName() throws IOException { - String toolNameToUse = toolName; - if (StringUtils.isEmptyOrNull(toolNameToUse)) { - toolNameToUse = diffTools.getDefaultToolName(gui); - } - if (StringUtils.isEmptyOrNull(toolNameToUse)) { - Map predefTools = diffTools - .getPredefinedTools(false); + private void informUserNoTool(List tools) { + try { StringBuilder toolNames = new StringBuilder(); - for (String name : predefTools.keySet()) { + for (String name : tools) { toolNames.append(name + " "); //$NON-NLS-1$ } outw.println(MessageFormat.format( CLIText.get().diffToolPromptToolName, toolNames)); outw.flush(); - toolNameToUse = diffTools.getFirstAvailableTool(); + } catch (IOException e) { + throw new IllegalStateException("Cannot output text", e); //$NON-NLS-1$ } - if (StringUtils.isEmptyOrNull(toolNameToUse)) { - throw new IOException(MessageFormat - .format(CLIText.get().diffToolUnknownToolName, toolName)); - } - return toolNameToUse; } - private void compare(List files, boolean showPrompt, - String toolNameToUse) throws IOException { + private class CountingPromptContinueHandler + implements PromptContinueHandler { + private final int fileIndex; + + private final int fileCount; + + private final String fileName; + + public CountingPromptContinueHandler(int fileIndex, int fileCount, + String fileName) { + this.fileIndex = fileIndex; + this.fileCount = fileCount; + this.fileName = fileName; + } + + @SuppressWarnings("boxing") + @Override + public boolean prompt(String toolToLaunchName) { + try { + boolean launchCompare = true; + outw.println(MessageFormat.format(CLIText.get().diffToolLaunch, + fileIndex, fileCount, fileName, toolToLaunchName) + + " "); //$NON-NLS-1$ + outw.flush(); + BufferedReader br = inputReader; + String line = null; + if ((line = br.readLine()) != null) { + if (!line.equalsIgnoreCase("Y")) { //$NON-NLS-1$ + launchCompare = false; + } + } + return launchCompare; + } catch (IOException e) { + throw new IllegalStateException("Cannot output text", e); //$NON-NLS-1$ + } + } + } + + private void compare(List files) throws IOException { ContentSource.Pair sourcePair = new ContentSource.Pair(source(oldTree), source(newTree)); try { for (int fileIndex = 0; fileIndex < files.size(); fileIndex++) { DiffEntry ent = files.get(fileIndex); - String mergedFilePath = ent.getNewPath(); - if (mergedFilePath.equals(DiffEntry.DEV_NULL)) { - mergedFilePath = ent.getOldPath(); + + String filePath = ent.getNewPath(); + if (filePath.equals(DiffEntry.DEV_NULL)) { + filePath = ent.getOldPath(); } - // check if user wants to launch compare - boolean launchCompare = true; - if (showPrompt) { - launchCompare = isLaunchCompare(fileIndex + 1, files.size(), - mergedFilePath, toolNameToUse); - } - if (launchCompare) { - try { - FileElement local = createFileElement( - FileElement.Type.LOCAL, sourcePair, Side.OLD, - ent); - FileElement remote = createFileElement( - FileElement.Type.REMOTE, sourcePair, Side.NEW, - ent); - FileElement merged = new FileElement(mergedFilePath, - FileElement.Type.MERGED); + + try { + FileElement local = createFileElement( + FileElement.Type.LOCAL, sourcePair, Side.OLD, ent); + FileElement remote = createFileElement( + FileElement.Type.REMOTE, sourcePair, Side.NEW, ent); + + PromptContinueHandler promptContinueHandler = new CountingPromptContinueHandler( + fileIndex + 1, files.size(), filePath); + + Optional optionalResult = diffTools + .compare(local, remote, toolName, prompt, gui, + trustExitCode, promptContinueHandler, + this::informUserNoTool); + + if (optionalResult.isPresent()) { + ExecutionResult result = optionalResult.get(); // TODO: check how to return the exit-code of the tool // to jgit / java runtime ? // int rc =... - ExecutionResult result = diffTools.compare(local, - remote, merged, toolNameToUse, prompt, gui, - trustExitCode); - outw.println(new String(result.getStdout().toByteArray())); + outw.println( + new String(result.getStdout().toByteArray())); outw.flush(); errw.println( new String(result.getStderr().toByteArray())); errw.flush(); - } catch (ToolException e) { - outw.println(e.getResultStdout()); - outw.flush(); - errw.println(e.getMessage()); - errw.flush(); - throw die(MessageFormat.format( - CLIText.get().diffToolDied, mergedFilePath, e), - e); } - } else { - break; + } catch (ToolException e) { + outw.println(e.getResultStdout()); + outw.flush(); + errw.println(e.getMessage()); + errw.flush(); + throw die(MessageFormat.format( + CLIText.get().diffToolDied, filePath, e), e); } } } finally { @@ -240,22 +264,6 @@ private void compare(List files, boolean showPrompt, } } - @SuppressWarnings("boxing") - private boolean isLaunchCompare(int fileIndex, int fileCount, - String fileName, String toolNamePrompt) throws IOException { - boolean launchCompare = true; - outw.println(MessageFormat.format(CLIText.get().diffToolLaunch, - fileIndex, fileCount, fileName, toolNamePrompt) + " "); //$NON-NLS-1$ - outw.flush(); - BufferedReader br = inputReader; - String line = null; - if ((line = br.readLine()) != null) { - if (!line.equalsIgnoreCase("Y")) { //$NON-NLS-1$ - launchCompare = false; - } - } - return launchCompare; - } private void showToolHelp() throws IOException { Map predefTools = diffTools .getPredefinedTools(true); @@ -314,12 +322,12 @@ private List getFiles() } private FileElement createFileElement(FileElement.Type elementType, - Pair pair, Side side, DiffEntry entry) - throws NoWorkTreeException, CorruptObjectException, IOException, - ToolException { + Pair pair, Side side, DiffEntry entry) throws NoWorkTreeException, + CorruptObjectException, IOException, ToolException { String entryPath = side == Side.NEW ? entry.getNewPath() : entry.getOldPath(); - FileElement fileElement = new FileElement(entryPath, elementType); + FileElement fileElement = new FileElement(entryPath, elementType, + db.getWorkTree()); if (!pair.isWorkingTreeSource(side) && !fileElement.isNullPath()) { try (RevWalk revWalk = new RevWalk(db); TreeWalk treeWalk = new TreeWalk(db, @@ -348,7 +356,8 @@ private FileElement createFileElement(FileElement.Type elementType, fileElement.createTempFile(null))); } else { throw new ToolException("Cannot find path '" + entryPath //$NON-NLS-1$ - + "' in staging area!", null); //$NON-NLS-1$ + + "' in staging area!", //$NON-NLS-1$ + null); } } } diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/MergeTool.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/MergeTool.java index f5884c44d..2a411b81f 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/MergeTool.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/MergeTool.java @@ -1,5 +1,6 @@ /* * Copyright (C) 2018-2022, Andre Bossert + * Copyright (C) 2019, Tim Neumann * * This program and the accompanying materials are made available under the * terms of the Eclipse Distribution License v. 1.0 which is available at @@ -22,6 +23,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.TreeMap; import org.eclipse.jgit.api.Git; @@ -29,30 +31,29 @@ import org.eclipse.jgit.api.StatusCommand; import org.eclipse.jgit.api.errors.GitAPIException; import org.eclipse.jgit.diff.ContentSource; -import org.eclipse.jgit.internal.diffmergetool.FileElement.Type; import org.eclipse.jgit.dircache.DirCache; import org.eclipse.jgit.dircache.DirCacheCheckout; +import org.eclipse.jgit.dircache.DirCacheCheckout.CheckoutMetadata; import org.eclipse.jgit.dircache.DirCacheEntry; import org.eclipse.jgit.dircache.DirCacheIterator; -import org.eclipse.jgit.dircache.DirCacheCheckout.CheckoutMetadata; import org.eclipse.jgit.errors.NoWorkTreeException; import org.eclipse.jgit.errors.RevisionSyntaxException; import org.eclipse.jgit.internal.diffmergetool.ExternalMergeTool; import org.eclipse.jgit.internal.diffmergetool.FileElement; +import org.eclipse.jgit.internal.diffmergetool.FileElement.Type; import org.eclipse.jgit.internal.diffmergetool.MergeTools; import org.eclipse.jgit.internal.diffmergetool.ToolException; +import org.eclipse.jgit.lib.Constants; +import org.eclipse.jgit.lib.CoreConfig.EolStreamType; import org.eclipse.jgit.lib.IndexDiff.StageState; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.lib.internal.BooleanTriState; +import org.eclipse.jgit.pgm.internal.CLIText; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.treewalk.TreeWalk; import org.eclipse.jgit.treewalk.WorkingTreeOptions; import org.eclipse.jgit.treewalk.filter.PathFilterGroup; -import org.eclipse.jgit.lib.Constants; -import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.util.StringUtils; -import org.eclipse.jgit.lib.Repository; -import org.eclipse.jgit.lib.internal.BooleanTriState; -import org.eclipse.jgit.lib.CoreConfig.EolStreamType; -import org.eclipse.jgit.pgm.internal.CLIText; import org.eclipse.jgit.util.FS.ExecutionResult; import org.kohsuke.args4j.Argument; import org.kohsuke.args4j.Option; @@ -62,9 +63,13 @@ class MergeTool extends TextBuiltin { private MergeTools mergeTools; + private Optional toolName = Optional.empty(); + @Option(name = "--tool", aliases = { "-t" }, metaVar = "metaVar_tool", usage = "usage_ToolForMerge") - private String toolName; + void setToolName(String name) { + toolName = Optional.of(name); + } private BooleanTriState prompt = BooleanTriState.UNSET; @@ -81,16 +86,16 @@ void noPrompt(@SuppressWarnings("unused") boolean on) { @Option(name = "--tool-help", usage = "usage_toolHelp") private boolean toolHelp; - private BooleanTriState gui = BooleanTriState.UNSET; + private boolean gui = false; @Option(name = "--gui", aliases = { "-g" }, usage = "usage_MergeGuiTool") void setGui(@SuppressWarnings("unused") boolean on) { - gui = BooleanTriState.TRUE; + gui = true; } @Option(name = "--no-gui", usage = "usage_noGui") void noGui(@SuppressWarnings("unused") boolean on) { - gui = BooleanTriState.FALSE; + gui = false; } @Argument(required = false, index = 0, metaVar = "metaVar_paths") @@ -116,17 +121,10 @@ protected void run() { if (toolHelp) { showToolHelp(); } else { - // get prompt - boolean showPrompt = mergeTools.isInteractive(); - if (prompt != BooleanTriState.UNSET) { - showPrompt = prompt == BooleanTriState.TRUE; - } - // get passed or default tool name - String toolNameToUse = promptToolName(); // get the changed files Map files = getFiles(); if (files.size() > 0) { - merge(files, showPrompt, toolNameToUse); + merge(files); } else { outw.println(CLIText.get().mergeToolNoFiles); } @@ -137,32 +135,21 @@ protected void run() { } } - private String promptToolName() throws IOException { - String toolNameToUse = toolName; - if (StringUtils.isEmptyOrNull(toolNameToUse)) { - toolNameToUse = mergeTools.getDefaultToolName(gui); - } - if (StringUtils.isEmptyOrNull(toolNameToUse)) { - Map predefTools = mergeTools - .getPredefinedTools(false); + private void informUserNoTool(List tools) { + try { StringBuilder toolNames = new StringBuilder(); - for (String name : predefTools.keySet()) { + for (String name : tools) { toolNames.append(name + " "); //$NON-NLS-1$ } outw.println(MessageFormat .format(CLIText.get().mergeToolPromptToolName, toolNames)); outw.flush(); - toolNameToUse = mergeTools.getFirstAvailableTool(); + } catch (IOException e) { + throw new IllegalStateException("Cannot output text", e); //$NON-NLS-1$ } - if (StringUtils.isEmptyOrNull(toolNameToUse)) { - throw new IOException(MessageFormat - .format(CLIText.get().mergeToolUnknownToolName, toolName)); - } - return toolNameToUse; } - private void merge(Map files, boolean showPrompt, - String toolNamePrompt) throws Exception { + private void merge(Map files) throws Exception { // sort file names List mergedFilePaths = new ArrayList<>(files.keySet()); Collections.sort(mergedFilePaths); @@ -174,6 +161,10 @@ private void merge(Map files, boolean showPrompt, outw.println(MessageFormat.format(CLIText.get().mergeToolMerging, mergedFiles)); outw.flush(); + boolean showPrompt = mergeTools.isInteractive(); + if (prompt != BooleanTriState.UNSET) { + showPrompt = prompt == BooleanTriState.TRUE; + } // merge the files MergeResult mergeResult = MergeResult.SUCCESSFUL; for (String mergedFilePath : mergedFilePaths) { @@ -191,8 +182,7 @@ private void merge(Map files, boolean showPrompt, // get file stage state and merge StageState fileState = files.get(mergedFilePath); if (fileState == StageState.BOTH_MODIFIED) { - mergeResult = mergeModified(mergedFilePath, showPrompt, - toolNamePrompt); + mergeResult = mergeModified(mergedFilePath, showPrompt); } else if ((fileState == StageState.DELETED_BY_US) || (fileState == StageState.DELETED_BY_THEM)) { mergeResult = mergeDeleted(mergedFilePath, @@ -206,19 +196,11 @@ private void merge(Map files, boolean showPrompt, } } - private MergeResult mergeModified(String mergedFilePath, boolean showPrompt, - String toolNamePrompt) throws Exception { + private MergeResult mergeModified(String mergedFilePath, boolean showPrompt) + throws Exception { outw.println(MessageFormat.format(CLIText.get().mergeToolNormalConflict, mergedFilePath)); outw.flush(); - // check if user wants to launch merge resolution tool - boolean launch = true; - if (showPrompt) { - launch = isLaunch(toolNamePrompt); - } - if (!launch) { - return MergeResult.ABORTED; // abort - } boolean isMergeSuccessful = true; ContentSource baseSource = ContentSource.create(db.newObjectReader()); ContentSource localSource = ContentSource.create(db.newObjectReader()); @@ -232,8 +214,8 @@ private MergeResult mergeModified(String mergedFilePath, boolean showPrompt, FileElement base = null; FileElement local = null; FileElement remote = null; - FileElement merged = new FileElement(mergedFilePath, - Type.MERGED); + FileElement merged = new FileElement(mergedFilePath, Type.MERGED, + db.getWorkTree()); DirCache cache = db.readDirCache(); try (RevWalk revWalk = new RevWalk(db); TreeWalk treeWalk = new TreeWalk(db, @@ -255,7 +237,8 @@ private MergeResult mergeModified(String mergedFilePath, boolean showPrompt, .get(WorkingTreeOptions.KEY); CheckoutMetadata checkoutMetadata = new CheckoutMetadata( eolStreamType, filterCommand); - DirCacheEntry entry = treeWalk.getTree(DirCacheIterator.class).getDirCacheEntry(); + DirCacheEntry entry = treeWalk + .getTree(DirCacheIterator.class).getDirCacheEntry(); if (entry == null) { continue; } @@ -297,23 +280,27 @@ private MergeResult mergeModified(String mergedFilePath, boolean showPrompt, // TODO: check how to return the exit-code of the // tool to jgit / java runtime ? // int rc =... - ExecutionResult executionResult = mergeTools.merge(local, - remote, merged, base, tempDir, toolName, prompt, gui); - outw.println( - new String(executionResult.getStdout().toByteArray())); - outw.flush(); - errw.println( - new String(executionResult.getStderr().toByteArray())); - errw.flush(); + Optional optionalResult = mergeTools.merge( + local, remote, merged, base, tempDir, toolName, prompt, + gui, this::promptForLaunch, this::informUserNoTool); + if (optionalResult.isPresent()) { + ExecutionResult result = optionalResult.get(); + outw.println(new String(result.getStdout().toByteArray())); + outw.flush(); + errw.println(new String(result.getStderr().toByteArray())); + errw.flush(); + } else { + return MergeResult.ABORTED; + } } catch (ToolException e) { isMergeSuccessful = false; outw.println(e.getResultStdout()); outw.flush(); + errw.println(e.getMessage()); errw.println(MessageFormat.format( CLIText.get().mergeToolMergeFailed, mergedFilePath)); errw.flush(); if (e.isCommandExecutionError()) { - errw.println(e.getMessage()); throw die(CLIText.get().mergeToolExecutionError, e); } } @@ -402,19 +389,23 @@ private boolean isMergeSuccessful() throws IOException { return hasUserAccepted(CLIText.get().mergeToolWasMergeSuccessfull); } - private boolean isLaunch(String toolNamePrompt) throws IOException { - boolean launch = true; - outw.print(MessageFormat.format(CLIText.get().mergeToolLaunch, - toolNamePrompt) + " "); //$NON-NLS-1$ - outw.flush(); - BufferedReader br = inputReader; - String line = null; - if ((line = br.readLine()) != null) { - if (!line.equalsIgnoreCase("y") && !line.equalsIgnoreCase("")) { //$NON-NLS-1$ //$NON-NLS-2$ - launch = false; + private boolean promptForLaunch(String toolNamePrompt) { + try { + boolean launch = true; + outw.print(MessageFormat.format(CLIText.get().mergeToolLaunch, + toolNamePrompt) + " "); //$NON-NLS-1$ + outw.flush(); + BufferedReader br = inputReader; + String line = null; + if ((line = br.readLine()) != null) { + if (!line.equalsIgnoreCase("y") && !line.equalsIgnoreCase("")) { //$NON-NLS-1$ //$NON-NLS-2$ + launch = false; + } } + return launch; + } catch (IOException e) { + throw new IllegalStateException("Cannot output text", e); //$NON-NLS-1$ } - return launch; } private int getDeletedMergeDecision() throws IOException { diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalDiffToolTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalDiffToolTest.java index 1b501c25b..222608e31 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalDiffToolTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalDiffToolTest.java @@ -18,13 +18,20 @@ import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_TOOL; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_TRUST_EXIT_CODE; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import java.io.IOException; +import java.nio.file.Files; +import java.util.Arrays; import java.util.Collections; import java.util.LinkedHashSet; +import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import org.eclipse.jgit.lib.internal.BooleanTriState; @@ -48,14 +55,7 @@ public void testUserToolWithError() throws Exception { config.setString(CONFIG_DIFFTOOL_SECTION, toolName, CONFIG_KEY_CMD, command); - DiffTools manager = new DiffTools(db); - - BooleanTriState prompt = BooleanTriState.UNSET; - BooleanTriState gui = BooleanTriState.UNSET; - BooleanTriState trustExitCode = BooleanTriState.TRUE; - - manager.compare(local, remote, merged, toolName, prompt, gui, - trustExitCode); + invokeCompare(toolName); fail("Expected exception to be thrown due to external tool exiting with error code: " + errorReturnCode); @@ -72,33 +72,84 @@ public void testUserToolWithCommandNotFoundError() throws Exception { config.setString(CONFIG_DIFFTOOL_SECTION, toolName, CONFIG_KEY_CMD, command); - DiffTools manager = new DiffTools(db); - - BooleanTriState prompt = BooleanTriState.UNSET; - BooleanTriState gui = BooleanTriState.UNSET; - BooleanTriState trustExitCode = BooleanTriState.FALSE; - - manager.compare(local, remote, merged, toolName, prompt, gui, - trustExitCode); - + invokeCompare(toolName); fail("Expected exception to be thrown due to external tool exiting with error code: " + errorReturnCode); } @Test - public void testToolNames() { + public void testUserDefinedTool() throws Exception { + String command = getEchoCommand(); + + FileBasedConfig config = db.getConfig(); + String customToolName = "customTool"; + config.setString(CONFIG_DIFFTOOL_SECTION, customToolName, + CONFIG_KEY_CMD, command); + DiffTools manager = new DiffTools(db); - Set actualToolNames = manager.getToolNames(); - Set expectedToolNames = Collections.emptySet(); - assertEquals("Incorrect set of external diff tool names", - expectedToolNames, actualToolNames); + + Map tools = manager.getUserDefinedTools(); + ExternalDiffTool externalTool = tools.get(customToolName); + boolean trustExitCode = true; + manager.compare(local, remote, externalTool, trustExitCode); + + assertEchoCommandHasCorrectOutput(); + } + + @Test + public void testUserDefinedToolWithPrompt() throws Exception { + String command = getEchoCommand(); + + FileBasedConfig config = db.getConfig(); + String customToolName = "customTool"; + config.setString(CONFIG_DIFFTOOL_SECTION, customToolName, + CONFIG_KEY_CMD, command); + + DiffTools manager = new DiffTools(db); + + PromptHandler promptHandler = PromptHandler.acceptPrompt(); + MissingToolHandler noToolHandler = new MissingToolHandler(); + + manager.compare(local, remote, Optional.of(customToolName), + BooleanTriState.TRUE, false, BooleanTriState.TRUE, + promptHandler, noToolHandler); + + assertEchoCommandHasCorrectOutput(); + + List actualToolPrompts = promptHandler.toolPrompts; + List expectedToolPrompts = Arrays.asList("customTool"); + assertEquals("Expected a user prompt for custom tool call", + expectedToolPrompts, actualToolPrompts); + + assertEquals("Expected to no informing about missing tools", + Collections.EMPTY_LIST, noToolHandler.missingTools); + } + + @Test + public void testUserDefinedToolWithCancelledPrompt() throws Exception { + DiffTools manager = new DiffTools(db); + + PromptHandler promptHandler = PromptHandler.cancelPrompt(); + MissingToolHandler noToolHandler = new MissingToolHandler(); + + Optional result = manager.compare(local, remote, + Optional.empty(), BooleanTriState.TRUE, false, + BooleanTriState.TRUE, promptHandler, noToolHandler); + assertFalse("Expected no result if user cancels the operation", + result.isPresent()); } @Test public void testAllTools() { + FileBasedConfig config = db.getConfig(); + String customToolName = "customTool"; + config.setString(CONFIG_DIFFTOOL_SECTION, customToolName, + CONFIG_KEY_CMD, "echo"); + DiffTools manager = new DiffTools(db); - Set actualToolNames = manager.getPredefinedTools(true).keySet(); + Set actualToolNames = manager.getAllToolNames(); Set expectedToolNames = new LinkedHashSet<>(); + expectedToolNames.add(customToolName); CommandLineDiffTool[] defaultTools = CommandLineDiffTool.values(); for (CommandLineDiffTool defaultTool : defaultTools) { String toolName = defaultTool.name(); @@ -166,18 +217,12 @@ public void testCompare() throws ToolException { config.setString(CONFIG_DIFFTOOL_SECTION, toolName, CONFIG_KEY_CMD, command); - - BooleanTriState prompt = BooleanTriState.UNSET; - BooleanTriState gui = BooleanTriState.UNSET; - BooleanTriState trustExitCode = BooleanTriState.UNSET; - - DiffTools manager = new DiffTools(db); - + Optional result = invokeCompare(toolName); + assertTrue("Expected external diff tool result to be available", + result.isPresent()); int expectedCompareResult = 0; - ExecutionResult compareResult = manager.compare(local, remote, merged, - toolName, prompt, gui, trustExitCode); assertEquals("Incorrect compare result for external diff tool", - expectedCompareResult, compareResult.getRc()); + expectedCompareResult, result.get().getRc()); } @Test @@ -192,17 +237,16 @@ public void testDefaultTool() throws Exception { toolName); DiffTools manager = new DiffTools(db); - BooleanTriState gui = BooleanTriState.UNSET; + boolean gui = false; String defaultToolName = manager.getDefaultToolName(gui); assertEquals( "Expected configured difftool to be the default external diff tool", toolName, defaultToolName); - gui = BooleanTriState.TRUE; + gui = true; String defaultGuiToolName = manager.getDefaultToolName(gui); - assertEquals( - "Expected configured difftool to be the default external diff tool", - "my_gui_tool", defaultGuiToolName); + assertNull("Expected default difftool to not be set", + defaultGuiToolName); config.setString(CONFIG_DIFF_SECTION, subsection, CONFIG_KEY_GUITOOL, guiToolName); @@ -210,7 +254,7 @@ public void testDefaultTool() throws Exception { defaultGuiToolName = manager.getDefaultToolName(gui); assertEquals( "Expected configured difftool to be the default external diff guitool", - "my_gui_tool", defaultGuiToolName); + guiToolName, defaultGuiToolName); } @Test @@ -247,20 +291,39 @@ public void testOverridePreDefinedToolPath() { @Test(expected = ToolException.class) public void testUndefinedTool() throws Exception { + String toolName = "undefined"; + invokeCompare(toolName); + fail("Expected exception to be thrown due to not defined external diff tool"); + } + + private Optional invokeCompare(String toolName) + throws ToolException { DiffTools manager = new DiffTools(db); - String toolName = "undefined"; BooleanTriState prompt = BooleanTriState.UNSET; - BooleanTriState gui = BooleanTriState.UNSET; - BooleanTriState trustExitCode = BooleanTriState.UNSET; + boolean gui = false; + BooleanTriState trustExitCode = BooleanTriState.TRUE; + PromptHandler promptHandler = PromptHandler.acceptPrompt(); + MissingToolHandler noToolHandler = new MissingToolHandler(); - manager.compare(local, remote, merged, toolName, prompt, gui, - trustExitCode); - fail("Expected exception to be thrown due to not defined external diff tool"); + Optional result = manager.compare(local, remote, + Optional.of(toolName), prompt, gui, trustExitCode, + promptHandler, noToolHandler); + return result; } private String getEchoCommand() { return "(echo \"$LOCAL\" \"$REMOTE\") > " + commandResult.getAbsolutePath(); } + + private void assertEchoCommandHasCorrectOutput() throws IOException { + List actualLines = Files.readAllLines(commandResult.toPath()); + String actualContent = String.join(System.lineSeparator(), actualLines); + actualLines = Arrays.asList(actualContent.split(" ")); + List expectedLines = Arrays.asList(localFile.getAbsolutePath(), + remoteFile.getAbsolutePath()); + assertEquals("Dummy test tool called with unexpected arguments", + expectedLines, actualLines); + } } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalMergeToolTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalMergeToolTest.java index 305f2b470..130b42a92 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalMergeToolTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalMergeToolTest.java @@ -18,13 +18,21 @@ import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_MERGETOOL_SECTION; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_MERGE_SECTION; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.junit.Assume.assumeTrue; +import java.io.IOException; +import java.nio.file.Files; +import java.util.Arrays; import java.util.Collections; import java.util.LinkedHashSet; +import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import org.eclipse.jgit.lib.internal.BooleanTriState; @@ -50,12 +58,7 @@ public void testUserToolWithError() throws Exception { config.setString(CONFIG_MERGETOOL_SECTION, toolName, CONFIG_KEY_TRUST_EXIT_CODE, String.valueOf(Boolean.TRUE)); - MergeTools manager = new MergeTools(db); - - BooleanTriState prompt = BooleanTriState.UNSET; - BooleanTriState gui = BooleanTriState.UNSET; - - manager.merge(local, remote, merged, base, null, toolName, prompt, gui); + invokeMerge(toolName); fail("Expected exception to be thrown due to external tool exiting with error code: " + errorReturnCode); @@ -72,31 +75,112 @@ public void testUserToolWithCommandNotFoundError() throws Exception { config.setString(CONFIG_MERGETOOL_SECTION, toolName, CONFIG_KEY_CMD, command); - MergeTools manager = new MergeTools(db); - - BooleanTriState prompt = BooleanTriState.UNSET; - BooleanTriState gui = BooleanTriState.UNSET; - - manager.merge(local, remote, merged, base, null, toolName, prompt, gui); + invokeMerge(toolName); fail("Expected exception to be thrown due to external tool exiting with error code: " + errorReturnCode); } @Test - public void testToolNames() { + public void testKdiff3() throws Exception { + assumePosixPlatform(); + + CommandLineMergeTool autoMergingTool = CommandLineMergeTool.kdiff3; + assumeMergeToolIsAvailable(autoMergingTool); + + CommandLineMergeTool tool = autoMergingTool; + PreDefinedMergeTool externalTool = new PreDefinedMergeTool(tool.name(), + tool.getPath(), tool.getParameters(true), + tool.getParameters(false), + tool.isExitCodeTrustable() ? BooleanTriState.TRUE + : BooleanTriState.FALSE); + MergeTools manager = new MergeTools(db); - Set actualToolNames = manager.getToolNames(); - Set expectedToolNames = Collections.emptySet(); - assertEquals("Incorrect set of external merge tool names", - expectedToolNames, actualToolNames); + ExecutionResult result = manager.merge(local, remote, merged, null, + null, externalTool); + assertEquals("Expected merge tool to succeed", 0, result.getRc()); + + List actualLines = Files.readAllLines(mergedFile.toPath()); + String actualMergeResult = String.join(System.lineSeparator(), + actualLines); + String expectedMergeResult = DEFAULT_CONTENT; + assertEquals( + "Failed to merge equal local and remote versions with pre-defined tool: " + + tool.getPath(), + expectedMergeResult, actualMergeResult); + } + + @Test + public void testUserDefinedTool() throws Exception { + String customToolName = "customTool"; + String command = getEchoCommand(); + + FileBasedConfig config = db.getConfig(); + config.setString(CONFIG_MERGETOOL_SECTION, customToolName, + CONFIG_KEY_CMD, command); + + MergeTools manager = new MergeTools(db); + Map tools = manager.getUserDefinedTools(); + ExternalMergeTool externalTool = tools.get(customToolName); + manager.merge(local, remote, merged, base, null, externalTool); + + assertEchoCommandHasCorrectOutput(); + } + + @Test + public void testUserDefinedToolWithPrompt() throws Exception { + String customToolName = "customTool"; + String command = getEchoCommand(); + + FileBasedConfig config = db.getConfig(); + config.setString(CONFIG_MERGETOOL_SECTION, customToolName, + CONFIG_KEY_CMD, command); + + MergeTools manager = new MergeTools(db); + + PromptHandler promptHandler = PromptHandler.acceptPrompt(); + MissingToolHandler noToolHandler = new MissingToolHandler(); + + manager.merge(local, remote, merged, base, null, + Optional.of(customToolName), BooleanTriState.TRUE, false, + promptHandler, noToolHandler); + + assertEchoCommandHasCorrectOutput(); + + List actualToolPrompts = promptHandler.toolPrompts; + List expectedToolPrompts = Arrays.asList("customTool"); + assertEquals("Expected a user prompt for custom tool call", + expectedToolPrompts, actualToolPrompts); + + assertEquals("Expected to no informing about missing tools", + Collections.EMPTY_LIST, noToolHandler.missingTools); + } + + @Test + public void testUserDefinedToolWithCancelledPrompt() throws Exception { + MergeTools manager = new MergeTools(db); + + PromptHandler promptHandler = PromptHandler.cancelPrompt(); + MissingToolHandler noToolHandler = new MissingToolHandler(); + + Optional result = manager.merge(local, remote, merged, + base, null, Optional.empty(), BooleanTriState.TRUE, false, + promptHandler, noToolHandler); + assertFalse("Expected no result if user cancels the operation", + result.isPresent()); } @Test public void testAllTools() { + FileBasedConfig config = db.getConfig(); + String customToolName = "customTool"; + config.setString(CONFIG_MERGETOOL_SECTION, customToolName, + CONFIG_KEY_CMD, "echo"); + MergeTools manager = new MergeTools(db); - Set actualToolNames = manager.getPredefinedTools(true).keySet(); + Set actualToolNames = manager.getAllToolNames(); Set expectedToolNames = new LinkedHashSet<>(); + expectedToolNames.add(customToolName); CommandLineMergeTool[] defaultTools = CommandLineMergeTool.values(); for (CommandLineMergeTool defaultTool : defaultTools) { String toolName = defaultTool.name(); @@ -165,16 +249,12 @@ public void testCompare() throws ToolException { config.setString(CONFIG_MERGETOOL_SECTION, toolName, CONFIG_KEY_CMD, command); - BooleanTriState prompt = BooleanTriState.UNSET; - BooleanTriState gui = BooleanTriState.UNSET; - - MergeTools manager = new MergeTools(db); - + Optional result = invokeMerge(toolName); + assertTrue("Expected external merge tool result to be available", + result.isPresent()); int expectedCompareResult = 0; - ExecutionResult compareResult = manager.merge(local, remote, merged, - base, null, toolName, prompt, gui); assertEquals("Incorrect compare result for external merge tool", - expectedCompareResult, compareResult.getRc()); + expectedCompareResult, result.get().getRc()); } @Test @@ -189,17 +269,16 @@ public void testDefaultTool() throws Exception { toolName); MergeTools manager = new MergeTools(db); - BooleanTriState gui = BooleanTriState.UNSET; + boolean gui = false; String defaultToolName = manager.getDefaultToolName(gui); assertEquals( "Expected configured mergetool to be the default external merge tool", toolName, defaultToolName); - gui = BooleanTriState.TRUE; + gui = true; String defaultGuiToolName = manager.getDefaultToolName(gui); - assertEquals( - "Expected configured mergetool to be the default external merge tool", - "my_gui_tool", defaultGuiToolName); + assertNull("Expected default mergetool to not be set", + defaultGuiToolName); config.setString(CONFIG_MERGE_SECTION, subsection, CONFIG_KEY_GUITOOL, guiToolName); @@ -207,7 +286,7 @@ public void testDefaultTool() throws Exception { defaultGuiToolName = manager.getDefaultToolName(gui); assertEquals( "Expected configured mergetool to be the default external merge guitool", - "my_gui_tool", defaultGuiToolName); + guiToolName, defaultGuiToolName); } @Test @@ -245,18 +324,48 @@ public void testOverridePreDefinedToolPath() { @Test(expected = ToolException.class) public void testUndefinedTool() throws Exception { - MergeTools manager = new MergeTools(db); - String toolName = "undefined"; - BooleanTriState prompt = BooleanTriState.UNSET; - BooleanTriState gui = BooleanTriState.UNSET; - - manager.merge(local, remote, merged, base, null, toolName, prompt, gui); + invokeMerge(toolName); fail("Expected exception to be thrown due to not defined external merge tool"); } + private Optional invokeMerge(String toolName) + throws ToolException { + BooleanTriState prompt = BooleanTriState.UNSET; + boolean gui = false; + + MergeTools manager = new MergeTools(db); + + PromptHandler promptHandler = PromptHandler.acceptPrompt(); + MissingToolHandler noToolHandler = new MissingToolHandler(); + + Optional result = manager.merge(local, remote, merged, + base, null, Optional.of(toolName), prompt, gui, promptHandler, + noToolHandler); + return result; + } + + private void assumeMergeToolIsAvailable( + CommandLineMergeTool autoMergingTool) { + boolean isAvailable = ExternalToolUtils.isToolAvailable(db.getFS(), + db.getDirectory(), db.getWorkTree(), autoMergingTool.getPath()); + assumeTrue("Assuming external tool is available: " + + autoMergingTool.name(), isAvailable); + } + private String getEchoCommand() { - return "(echo \"$LOCAL\" \"$REMOTE\") > " + return "(echo $LOCAL $REMOTE $MERGED $BASE) > " + commandResult.getAbsolutePath(); } + + private void assertEchoCommandHasCorrectOutput() throws IOException { + List actualLines = Files.readAllLines(commandResult.toPath()); + String actualContent = String.join(System.lineSeparator(), actualLines); + actualLines = Arrays.asList(actualContent.split(" ")); + List expectedLines = Arrays.asList(localFile.getAbsolutePath(), + remoteFile.getAbsolutePath(), mergedFile.getAbsolutePath(), + baseFile.getAbsolutePath()); + assertEquals("Dummy test tool called with unexpected arguments", + expectedLines, actualLines); + } } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalToolTestCase.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalToolTestCase.java index 0fd85cb45..7a6ff4657 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalToolTestCase.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalToolTestCase.java @@ -11,6 +11,8 @@ import java.io.File; import java.nio.file.Files; +import java.util.ArrayList; +import java.util.List; import org.eclipse.jgit.junit.RepositoryTestCase; import org.eclipse.jgit.util.FS; @@ -88,4 +90,39 @@ protected static void assumePosixPlatform() { "This test can run only in Linux tests", FS.DETECTED instanceof FS_POSIX); } + + protected static class PromptHandler implements PromptContinueHandler { + + private final boolean promptResult; + + final List toolPrompts = new ArrayList<>(); + + private PromptHandler(boolean promptResult) { + this.promptResult = promptResult; + } + + static PromptHandler acceptPrompt() { + return new PromptHandler(true); + } + + static PromptHandler cancelPrompt() { + return new PromptHandler(false); + } + + @Override + public boolean prompt(String toolName) { + toolPrompts.add(toolName); + return promptResult; + } + } + + protected static class MissingToolHandler implements InformNoToolHandler { + + final List missingTools = new ArrayList<>(); + + @Override + public void inform(List toolNames) { + missingTools.addAll(toolNames); + } + } } diff --git a/org.eclipse.jgit/META-INF/MANIFEST.MF b/org.eclipse.jgit/META-INF/MANIFEST.MF index e72f00f0b..a25685073 100644 --- a/org.eclipse.jgit/META-INF/MANIFEST.MF +++ b/org.eclipse.jgit/META-INF/MANIFEST.MF @@ -73,7 +73,8 @@ Export-Package: org.eclipse.jgit.annotations;version="6.2.0", org.eclipse.jgit.internal.diffmergetool;version="6.2.0"; x-friends:="org.eclipse.jgit.test, org.eclipse.jgit.pgm.test, - org.eclipse.jgit.pgm", + org.eclipse.jgit.pgm, + org.eclipse.egit.ui", org.eclipse.jgit.internal.fsck;version="6.2.0"; x-friends:="org.eclipse.jgit.test", org.eclipse.jgit.internal.revwalk;version="6.2.0"; @@ -133,7 +134,8 @@ Export-Package: org.eclipse.jgit.annotations;version="6.2.0", org.eclipse.jgit.util.time", org.eclipse.jgit.lib.internal;version="6.2.0"; x-friends:="org.eclipse.jgit.test, - org.eclipse.jgit.pgm", + org.eclipse.jgit.pgm, + org.eclipse.egit.ui", org.eclipse.jgit.logging;version="6.2.0", org.eclipse.jgit.merge;version="6.2.0"; uses:="org.eclipse.jgit.dircache, diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/DiffTools.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/DiffTools.java index 6c1a780a3..5fe263a4e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/DiffTools.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/DiffTools.java @@ -1,5 +1,6 @@ /* * Copyright (C) 2018-2022, Andre Bossert + * Copyright (C) 2019, Tim Neumann * * This program and the accompanying materials are made available under the * terms of the Eclipse Distribution License v. 1.0 which is available at @@ -10,23 +11,31 @@ package org.eclipse.jgit.internal.diffmergetool; -import java.util.TreeMap; -import java.util.Collections; +import java.io.File; import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; import java.util.Map; +import java.util.Optional; import java.util.Set; +import java.util.TreeMap; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.lib.StoredConfig; import org.eclipse.jgit.lib.internal.BooleanTriState; +import org.eclipse.jgit.util.FS; import org.eclipse.jgit.util.FS.ExecutionResult; -import org.eclipse.jgit.util.StringUtils; /** * Manages diff tools. */ public class DiffTools { - private final Repository repo; + private final FS fs; + + private final File gitDir; + + private final File workTree; private final DiffToolConfig config; @@ -41,10 +50,103 @@ public class DiffTools { * the repository */ public DiffTools(Repository repo) { - this.repo = repo; - config = repo.getConfig().get(DiffToolConfig.KEY); + this(repo, repo.getConfig()); + } + + /** + * Creates the external merge-tools manager for given configuration. + * + * @param config + * the git configuration + */ + public DiffTools(StoredConfig config) { + this(null, config); + } + + private DiffTools(Repository repo, StoredConfig config) { + this.config = config.get(DiffToolConfig.KEY); + this.gitDir = repo == null ? null : repo.getDirectory(); + this.fs = repo == null ? FS.DETECTED : repo.getFS(); + this.workTree = repo == null ? null : repo.getWorkTree(); predefinedTools = setupPredefinedTools(); - userDefinedTools = setupUserDefinedTools(config, predefinedTools); + userDefinedTools = setupUserDefinedTools(predefinedTools); + } + + /** + * Compare two versions of a file. + * + * @param localFile + * The local/left version of the file. + * @param remoteFile + * The remote/right version of the file. + * @param toolName + * Optionally the name of the tool to use. If not given the + * default tool will be used. + * @param prompt + * Optionally a flag whether to prompt the user before compare. + * If not given the default will be used. + * @param gui + * A flag whether to prefer a gui tool. + * @param trustExitCode + * Optionally a flag whether to trust the exit code of the tool. + * If not given the default will be used. + * @param promptHandler + * The handler to use when needing to prompt the user if he wants + * to continue. + * @param noToolHandler + * The handler to use when needing to inform the user, that no + * tool is configured. + * @return the optioanl result of executing the tool if it was executed + * @throws ToolException + * when the tool fails + */ + public Optional compare(FileElement localFile, + FileElement remoteFile, Optional toolName, + BooleanTriState prompt, boolean gui, BooleanTriState trustExitCode, + PromptContinueHandler promptHandler, + InformNoToolHandler noToolHandler) throws ToolException { + + String toolNameToUse; + + if (toolName.isPresent()) { + toolNameToUse = toolName.get(); + } else { + toolNameToUse = getDefaultToolName(gui); + + if (toolNameToUse == null || toolNameToUse.isEmpty()) { + noToolHandler.inform(new ArrayList<>(predefinedTools.keySet())); + toolNameToUse = getFirstAvailableTool(); + } + } + + boolean doPrompt; + if (prompt != BooleanTriState.UNSET) { + doPrompt = prompt == BooleanTriState.TRUE; + } else { + doPrompt = isInteractive(); + } + + if (doPrompt) { + if (!promptHandler.prompt(toolNameToUse)) { + return Optional.empty(); + } + } + + boolean trust; + if (trustExitCode != BooleanTriState.UNSET) { + trust = trustExitCode == BooleanTriState.TRUE; + } else { + trust = config.isTrustExitCode(); + } + + ExternalDiffTool tool = getTool(toolNameToUse); + if (tool == null) { + throw new ToolException( + "External diff tool is not defined: " + toolNameToUse); //$NON-NLS-1$ + } + + return Optional.of( + compare(localFile, remoteFile, tool, trust)); } /** @@ -54,56 +156,70 @@ public DiffTools(Repository repo) { * the local file element * @param remoteFile * the remote file element - * @param mergedFile - * the merged file element, it's path equals local or remote - * element path - * @param toolName - * the selected tool name (can be null) - * @param prompt - * the prompt option - * @param gui - * the GUI option + * @param tool + * the selected tool * @param trustExitCode * the "trust exit code" option * @return the execution result from tool * @throws ToolException */ public ExecutionResult compare(FileElement localFile, - FileElement remoteFile, FileElement mergedFile, String toolName, - BooleanTriState prompt, BooleanTriState gui, - BooleanTriState trustExitCode) throws ToolException { + FileElement remoteFile, ExternalDiffTool tool, + boolean trustExitCode) throws ToolException { try { // prepare the command (replace the file paths) - String command = ExternalToolUtils.prepareCommand( - guessTool(toolName, gui).getCommand(), localFile, - remoteFile, mergedFile, null); + String command = ExternalToolUtils.prepareCommand(tool.getCommand(), + localFile, remoteFile, null, null); // prepare the environment - Map env = ExternalToolUtils.prepareEnvironment(repo, - localFile, remoteFile, mergedFile, null); - boolean trust = config.isTrustExitCode(); - if (trustExitCode != BooleanTriState.UNSET) { - trust = trustExitCode == BooleanTriState.TRUE; - } + Map env = ExternalToolUtils.prepareEnvironment( + gitDir, localFile, remoteFile, null, null); // execute the tool - CommandExecutor cmdExec = new CommandExecutor(repo.getFS(), trust); - return cmdExec.run(command, repo.getWorkTree(), env); + CommandExecutor cmdExec = new CommandExecutor(fs, trustExitCode); + return cmdExec.run(command, workTree, env); } catch (IOException | InterruptedException e) { throw new ToolException(e); } finally { localFile.cleanTemporaries(); remoteFile.cleanTemporaries(); - mergedFile.cleanTemporaries(); } } /** - * @return the tool names + * Get user defined tool names. + * + * @return the user defined tool names */ - public Set getToolNames() { - return config.getToolNames(); + public Set getUserDefinedToolNames() { + return userDefinedTools.keySet(); } /** + * Get predefined tool names. + * + * @return the predefined tool names + */ + public Set getPredefinedToolNames() { + return predefinedTools.keySet(); + } + + /** + * Get all tool names. + * + * @return the all tool names (default or available tool name is the first + * in the set) + */ + public Set getAllToolNames() { + String defaultName = getDefaultToolName(false); + if (defaultName == null) { + defaultName = getFirstAvailableTool(); + } + return ExternalToolUtils.createSortedToolSet(defaultName, + getUserDefinedToolNames(), getPredefinedToolNames()); + } + + /** + * Get user defined tools map. + * * @return the user defined tools */ public Map getUserDefinedTools() { @@ -111,6 +227,8 @@ public Map getUserDefinedTools() { } /** + * Get predefined tools map. + * * @param checkAvailability * true: for checking if tools can be executed; ATTENTION: this * check took some time, do not execute often (store the map for @@ -124,59 +242,49 @@ public Map getPredefinedTools( if (checkAvailability) { for (ExternalDiffTool tool : predefinedTools.values()) { PreDefinedDiffTool predefTool = (PreDefinedDiffTool) tool; - predefTool.setAvailable(ExternalToolUtils.isToolAvailable(repo, - predefTool.getPath())); + predefTool.setAvailable(ExternalToolUtils.isToolAvailable(fs, + gitDir, workTree, predefTool.getPath())); } } return Collections.unmodifiableMap(predefinedTools); } /** + * Get first available tool name. + * * @return the name of first available predefined tool or null */ public String getFirstAvailableTool() { - String name = null; for (ExternalDiffTool tool : predefinedTools.values()) { - if (ExternalToolUtils.isToolAvailable(repo, tool.getPath())) { - name = tool.getName(); - break; + if (ExternalToolUtils.isToolAvailable(fs, gitDir, workTree, + tool.getPath())) { + return tool.getName(); } } - return name; + return null; } /** + * Get default (gui-)tool name. + * * @param gui * use the diff.guitool setting ? * @return the default tool name */ - public String getDefaultToolName(BooleanTriState gui) { - return gui != BooleanTriState.UNSET ? "my_gui_tool" //$NON-NLS-1$ + public String getDefaultToolName(boolean gui) { + return gui ? config.getDefaultGuiToolName() : config.getDefaultToolName(); } /** + * Is interactive diff (prompt enabled) ? + * * @return is interactive (config prompt enabled) ? */ public boolean isInteractive() { return config.isPrompt(); } - private ExternalDiffTool guessTool(String toolName, BooleanTriState gui) - throws ToolException { - if (StringUtils.isEmptyOrNull(toolName)) { - toolName = getDefaultToolName(gui); - } - ExternalDiffTool tool = null; - if (!StringUtils.isEmptyOrNull(toolName)) { - tool = getTool(toolName); - } - if (tool == null) { - throw new ToolException("Unknown diff tool '" + toolName + "'"); //$NON-NLS-1$ //$NON-NLS-2$ - } - return tool; - } - private ExternalDiffTool getTool(final String name) { ExternalDiffTool tool = userDefinedTools.get(name); if (tool == null) { @@ -193,10 +301,10 @@ private static Map setupPredefinedTools() { return tools; } - private static Map setupUserDefinedTools( - DiffToolConfig cfg, Map predefTools) { + private Map setupUserDefinedTools( + Map predefTools) { Map tools = new TreeMap<>(); - Map userTools = cfg.getTools(); + Map userTools = config.getTools(); for (String name : userTools.keySet()) { ExternalDiffTool userTool = userTools.get(name); // if difftool..cmd is defined we have user defined tool diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ExternalToolUtils.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ExternalToolUtils.java index e10607d2f..9a6968113 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ExternalToolUtils.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ExternalToolUtils.java @@ -10,10 +10,14 @@ package org.eclipse.jgit.internal.diffmergetool; import java.util.TreeMap; +import java.io.File; import java.io.IOException; +import java.util.LinkedHashSet; import java.util.Map; +import java.util.Set; + import org.eclipse.jgit.lib.Constants; -import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.util.FS; /** * Utilities for diff- and merge-tools. @@ -57,8 +61,8 @@ public static String prepareCommand(String command, FileElement localFile, /** * Prepare environment needed for execution. * - * @param repo - * the repository + * @param gitDir + * the .git directory * @param localFile * the local file (ours) * @param remoteFile @@ -70,11 +74,13 @@ public static String prepareCommand(String command, FileElement localFile, * @return the environment map with variables and values (file paths) * @throws IOException */ - public static Map prepareEnvironment(Repository repo, + public static Map prepareEnvironment(File gitDir, FileElement localFile, FileElement remoteFile, FileElement mergedFile, FileElement baseFile) throws IOException { Map env = new TreeMap<>(); - env.put(Constants.GIT_DIR_KEY, repo.getDirectory().getAbsolutePath()); + if (gitDir != null) { + env.put(Constants.GIT_DIR_KEY, gitDir.getAbsolutePath()); + } if (localFile != null) { localFile.addToEnv(env); } @@ -112,22 +118,60 @@ public static String quotePath(String path) { } /** - * @param repo - * the repository + * @param fs + * the file system abstraction + * @param gitDir + * the .git directory + * @param directory + * the working directory * @param path * the tool path * @return true if tool available and false otherwise */ - public static boolean isToolAvailable(Repository repo, String path) { + public static boolean isToolAvailable(FS fs, File gitDir, File directory, + String path) { boolean available = true; try { - CommandExecutor cmdExec = new CommandExecutor(repo.getFS(), false); - available = cmdExec.checkExecutable(path, repo.getWorkTree(), - prepareEnvironment(repo, null, null, null, null)); + CommandExecutor cmdExec = new CommandExecutor(fs, false); + available = cmdExec.checkExecutable(path, directory, + prepareEnvironment(gitDir, null, null, null, null)); } catch (Exception e) { available = false; } return available; } + /** + * @param defaultName + * the default tool name + * @param userDefinedNames + * the user defined tool names + * @param preDefinedNames + * the pre defined tool names + * @return the sorted tool names set: first element is default tool name if + * valid, then user defined tool names and then pre defined tool + * names + */ + public static Set createSortedToolSet(String defaultName, + Set userDefinedNames, Set preDefinedNames) { + Set names = new LinkedHashSet<>(); + if (defaultName != null) { + // remove defaultName from both sets + Set namesPredef = new LinkedHashSet<>(); + Set namesUser = new LinkedHashSet<>(); + namesUser.addAll(userDefinedNames); + namesUser.remove(defaultName); + namesPredef.addAll(preDefinedNames); + namesPredef.remove(defaultName); + // add defaultName as first in set + names.add(defaultName); + names.addAll(namesUser); + names.addAll(namesPredef); + } else { + names.addAll(userDefinedNames); + names.addAll(preDefinedNames); + } + return names; + } + } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/FileElement.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/FileElement.java index 5902c1e1b..ba8ca54c5 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/FileElement.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/FileElement.java @@ -57,6 +57,8 @@ public enum Type { private final Type type; + private final File workDir; + private InputStream stream; private File tempFile; @@ -70,7 +72,7 @@ public enum Type { * the element type */ public FileElement(String path, Type type) { - this(path, type, null, null); + this(path, type, null); } /** @@ -80,17 +82,31 @@ public FileElement(String path, Type type) { * the file path * @param type * the element type - * @param tempFile - * the temporary file to be used (can be null and will be created - * then) - * @param stream - * the object stream to load instead of file + * @param workDir + * the working directory of the path (can be null, then current + * working dir is used) */ - public FileElement(String path, Type type, File tempFile, + public FileElement(String path, Type type, File workDir) { + this(path, type, workDir, null); + } + + /** + * @param path + * the file path + * @param type + * the element type + * @param workDir + * the working directory of the path (can be null, then current + * working dir is used) + * @param stream + * the object stream to load and write on demand, @see getFile(), + * to tempFile once (can be null) + */ + public FileElement(String path, Type type, File workDir, InputStream stream) { this.path = path; this.type = type; - this.tempFile = tempFile; + this.workDir = workDir; this.stream = stream; } @@ -109,41 +125,39 @@ public Type getType() { } /** - * Return a temporary file within passed directory and fills it with stream - * if valid. - * - * @param directory - * the directory where the temporary file is created - * @param midName - * name added in the middle of generated temporary file name - * @return the object stream - * @throws IOException - */ - public File getFile(File directory, String midName) throws IOException { - if ((tempFile != null) && (stream == null)) { - return tempFile; - } - tempFile = getTempFile(path, directory, midName); - return copyFromStream(tempFile, stream); - } - - /** - * Return a real file from work tree or a temporary file with content if - * stream is valid or if path is "/dev/null" + * Return + *
    + *
  • a temporary file if already created and stream is not valid
    • + *
  • OR a real file from work tree: if no temp file was created (@see + * createTempFile()) and if no stream was set
    • + *
  • OR an empty temporary file if path is "/dev/null"
    • + *
  • OR a temporary file with stream content if stream is valid (not + * null); stream is closed and invalidated (set to null) after write to temp + * file, so stream is used only once during first call!
    • + *
* * @return the object stream * @throws IOException */ public File getFile() throws IOException { + // if we have already temp file and no stream + // then just return this temp file (it was filled from outside) if ((tempFile != null) && (stream == null)) { return tempFile; } - File file = new File(path); - // if we have a stream or file is missing ("/dev/null") then create - // temporary file + File file = new File(workDir, path); + // if we have a stream or file is missing (path is "/dev/null") + // then optionally create temporary file and fill it with stream content if ((stream != null) || isNullPath()) { - tempFile = getTempFile(file); - return copyFromStream(tempFile, stream); + if (tempFile == null) { + tempFile = getTempFile(file, type.name(), null); + } + if (stream != null) { + copyFromStream(tempFile, stream); + } + // invalidate the stream, because it is used once + stream = null; + return tempFile; } return file; } @@ -158,7 +172,7 @@ public boolean isNullPath() { } /** - * Create temporary file in given or system temporary directory + * Create temporary file in given or system temporary directory. * * @param directory * the directory for the file (can be null); if null system @@ -168,75 +182,23 @@ public boolean isNullPath() { */ public File createTempFile(File directory) throws IOException { if (tempFile == null) { - File file = new File(path); - if (directory != null) { - tempFile = getTempFile(file, directory, type.name()); - } else { - tempFile = getTempFile(file); - } + tempFile = getTempFile(new File(path), type.name(), directory); } return tempFile; } - private static File getTempFile(File file) throws IOException { - return File.createTempFile(".__", "__" + file.getName()); //$NON-NLS-1$ //$NON-NLS-2$ - } - - private static File getTempFile(File file, File directory, String midName) - throws IOException { - String[] fileNameAndExtension = splitBaseFileNameAndExtension(file); - return File.createTempFile( - fileNameAndExtension[0] + "_" + midName + "_", //$NON-NLS-1$ //$NON-NLS-2$ - fileNameAndExtension[1], directory); - } - - private static File getTempFile(String path, File directory, String midName) - throws IOException { - return getTempFile(new File(path), directory, midName); - } - /** * Delete and invalidate temporary file if necessary. */ public void cleanTemporaries() { - if (tempFile != null && tempFile.exists()) - tempFile.delete(); + if (tempFile != null && tempFile.exists()) { + tempFile.delete(); + } tempFile = null; } - private static File copyFromStream(File file, final InputStream stream) - throws IOException, FileNotFoundException { - if (stream != null) { - try (OutputStream outStream = new FileOutputStream(file)) { - int read = 0; - byte[] bytes = new byte[8 * 1024]; - while ((read = stream.read(bytes)) != -1) { - outStream.write(bytes, 0, read); - } - } finally { - // stream can only be consumed once --> close it - stream.close(); - } - } - return file; - } - - private static String[] splitBaseFileNameAndExtension(File file) { - String[] result = new String[2]; - result[0] = file.getName(); - result[1] = ""; //$NON-NLS-1$ - int idx = result[0].lastIndexOf("."); //$NON-NLS-1$ - // if "." was found (>-1) and last-index is not first char (>0), then - // split (same behavior like cgit) - if (idx > 0) { - result[1] = result[0].substring(idx, result[0].length()); - result[0] = result[0].substring(0, idx); - } - return result; - } - /** - * Replace variable in input + * Replace variable in input. * * @param input * the input string @@ -258,4 +220,43 @@ public void addToEnv(Map env) throws IOException { env.put(type.name(), getFile().getPath()); } + private static File getTempFile(final File file, final String midName, + final File workingDir) throws IOException { + String[] fileNameAndExtension = splitBaseFileNameAndExtension(file); + // TODO: avoid long random file name (number generated by + // createTempFile) + return File.createTempFile( + fileNameAndExtension[0] + "_" + midName + "_", //$NON-NLS-1$ //$NON-NLS-2$ + fileNameAndExtension[1], workingDir); + } + + private static void copyFromStream(final File file, + final InputStream stream) + throws IOException, FileNotFoundException { + try (OutputStream outStream = new FileOutputStream(file)) { + int read = 0; + byte[] bytes = new byte[8 * 1024]; + while ((read = stream.read(bytes)) != -1) { + outStream.write(bytes, 0, read); + } + } finally { + // stream can only be consumed once --> close it and invalidate + stream.close(); + } + } + + private static String[] splitBaseFileNameAndExtension(File file) { + String[] result = new String[2]; + result[0] = file.getName(); + result[1] = ""; //$NON-NLS-1$ + int idx = result[0].lastIndexOf("."); //$NON-NLS-1$ + // if "." was found (>-1) and last-index is not first char (>0), then + // split (same behavior like cgit) + if (idx > 0) { + result[1] = result[0].substring(idx, result[0].length()); + result[0] = result[0].substring(0, idx); + } + return result; + } + } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/InformNoToolHandler.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/InformNoToolHandler.java new file mode 100644 index 000000000..36b290d37 --- /dev/null +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/InformNoToolHandler.java @@ -0,0 +1,28 @@ +/* + * Copyright (C) 2018-2019, Tim Neumann + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Distribution License v. 1.0 which is available at + * https://www.eclipse.org/org/documents/edl-v10.php. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +package org.eclipse.jgit.internal.diffmergetool; + +import java.util.List; + +/** + * A handler for when the diff/merge tool manager wants to inform the user that + * no tool has been configured and one of the default tools will be used. + */ +public interface InformNoToolHandler { + /** + * Inform the user, that no tool is configured and that one of the given + * tools is used. + * + * @param toolNames + * The tools which are tried + */ + void inform(List toolNames); +} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/MergeToolConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/MergeToolConfig.java index 9be20b75a..9625d5f10 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/MergeToolConfig.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/MergeToolConfig.java @@ -31,7 +31,7 @@ import org.eclipse.jgit.lib.internal.BooleanTriState; /** - * Keeps track of difftool related configuration options. + * Keeps track of merge tool related configuration options. */ public class MergeToolConfig { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/MergeTools.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/MergeTools.java index d91d57f1a..d2055272e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/MergeTools.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/MergeTools.java @@ -1,5 +1,6 @@ /* * Copyright (C) 2018-2022, Andre Bossert + * Copyright (C) 2019, Tim Neumann * * This program and the accompanying materials are made available under the * terms of the Eclipse Distribution License v. 1.0 which is available at @@ -15,22 +16,30 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.StandardCopyOption; +import java.util.ArrayList; +import java.util.Collections; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.TreeMap; import org.eclipse.jgit.internal.diffmergetool.FileElement.Type; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.lib.StoredConfig; import org.eclipse.jgit.lib.internal.BooleanTriState; +import org.eclipse.jgit.util.FS; import org.eclipse.jgit.util.FS.ExecutionResult; -import org.eclipse.jgit.util.StringUtils; /** * Manages merge tools. */ public class MergeTools { - Repository repo; + private final FS fs; + + private final File gitDir; + + private final File workTree; private final MergeToolConfig config; @@ -39,17 +48,111 @@ public class MergeTools { private final Map userDefinedTools; /** + * Creates the external merge-tools manager for given repository. + * * @param repo * the repository */ public MergeTools(Repository repo) { - this.repo = repo; - config = repo.getConfig().get(MergeToolConfig.KEY); - predefinedTools = setupPredefinedTools(); - userDefinedTools = setupUserDefinedTools(config, predefinedTools); + this(repo, repo.getConfig()); } /** + * Creates the external diff-tools manager for given configuration. + * + * @param config + * the git configuration + */ + public MergeTools(StoredConfig config) { + this(null, config); + } + + private MergeTools(Repository repo, StoredConfig config) { + this.config = config.get(MergeToolConfig.KEY); + this.gitDir = repo == null ? null : repo.getDirectory(); + this.fs = repo == null ? FS.DETECTED : repo.getFS(); + this.workTree = repo == null ? null : repo.getWorkTree(); + predefinedTools = setupPredefinedTools(); + userDefinedTools = setupUserDefinedTools(predefinedTools); + } + + /** + * Merge two versions of a file with optional base file. + * + * @param localFile + * The local/left version of the file. + * @param remoteFile + * The remote/right version of the file. + * @param mergedFile + * The file for the result. + * @param baseFile + * The base version of the file. May be null. + * @param tempDir + * The tmepDir used for the files. May be null. + * @param toolName + * Optionally the name of the tool to use. If not given the + * default tool will be used. + * @param prompt + * Optionally a flag whether to prompt the user before compare. + * If not given the default will be used. + * @param gui + * A flag whether to prefer a gui tool. + * @param promptHandler + * The handler to use when needing to prompt the user if he wants + * to continue. + * @param noToolHandler + * The handler to use when needing to inform the user, that no + * tool is configured. + * @return the optional result of executing the tool if it was executed + * @throws ToolException + * when the tool fails + */ + public Optional merge(FileElement localFile, + FileElement remoteFile, FileElement mergedFile, + FileElement baseFile, File tempDir, Optional toolName, + BooleanTriState prompt, boolean gui, + PromptContinueHandler promptHandler, + InformNoToolHandler noToolHandler) throws ToolException { + + String toolNameToUse; + + if (toolName.isPresent()) { + toolNameToUse = toolName.get(); + } else { + toolNameToUse = getDefaultToolName(gui); + + if (toolNameToUse == null || toolNameToUse.isEmpty()) { + noToolHandler.inform(new ArrayList<>(predefinedTools.keySet())); + toolNameToUse = getFirstAvailableTool(); + } + } + + boolean doPrompt; + if (prompt != BooleanTriState.UNSET) { + doPrompt = prompt == BooleanTriState.TRUE; + } else { + doPrompt = isInteractive(); + } + + if (doPrompt) { + if (!promptHandler.prompt(toolNameToUse)) { + return Optional.empty(); + } + } + + ExternalMergeTool tool = getTool(toolNameToUse); + if (tool == null) { + throw new ToolException( + "External merge tool is not defined: " + toolNameToUse); //$NON-NLS-1$ + } + + return Optional.of(merge(localFile, remoteFile, mergedFile, baseFile, + tempDir, tool)); + } + + /** + * Merge two versions of a file with optional base file. + * * @param localFile * the local file element * @param remoteFile @@ -61,38 +164,31 @@ public MergeTools(Repository repo) { * @param tempDir * the temporary directory (needed for backup and auto-remove, * can be null) - * @param toolName - * the selected tool name (can be null) - * @param prompt - * the prompt option - * @param gui - * the GUI option + * @param tool + * the selected tool * @return the execution result from tool * @throws ToolException */ public ExecutionResult merge(FileElement localFile, FileElement remoteFile, FileElement mergedFile, FileElement baseFile, File tempDir, - String toolName, BooleanTriState prompt, BooleanTriState gui) - throws ToolException { - ExternalMergeTool tool = guessTool(toolName, gui); + ExternalMergeTool tool) throws ToolException { FileElement backup = null; ExecutionResult result = null; try { - File workingDir = repo.getWorkTree(); // create additional backup file (copy worktree file) - backup = createBackupFile(mergedFile.getPath(), - tempDir != null ? tempDir : workingDir); + backup = createBackupFile(mergedFile, + tempDir != null ? tempDir : workTree); // prepare the command (replace the file paths) - boolean trust = tool.getTrustExitCode() == BooleanTriState.TRUE; String command = ExternalToolUtils.prepareCommand( tool.getCommand(baseFile != null), localFile, remoteFile, mergedFile, baseFile); // prepare the environment - Map env = ExternalToolUtils.prepareEnvironment(repo, - localFile, remoteFile, mergedFile, baseFile); + Map env = ExternalToolUtils.prepareEnvironment( + gitDir, localFile, remoteFile, mergedFile, baseFile); + boolean trust = tool.getTrustExitCode() == BooleanTriState.TRUE; // execute the tool - CommandExecutor cmdExec = new CommandExecutor(repo.getFS(), trust); - result = cmdExec.run(command, workingDir, env); + CommandExecutor cmdExec = new CommandExecutor(fs, trust); + result = cmdExec.run(command, workTree, env); // keep backup as .orig file if (backup != null) { keepBackupFile(mergedFile.getPath(), backup); @@ -124,19 +220,21 @@ public ExecutionResult merge(FileElement localFile, FileElement remoteFile, } } - private FileElement createBackupFile(String filePath, File parentDir) + private FileElement createBackupFile(FileElement from, File toParentDir) throws IOException { FileElement backup = null; - Path path = Paths.get(filePath); + Path path = Paths.get(from.getPath()); if (Files.exists(path)) { - backup = new FileElement(filePath, Type.BACKUP); - Files.copy(path, backup.createTempFile(parentDir).toPath(), + backup = new FileElement(from.getPath(), Type.BACKUP); + Files.copy(path, backup.createTempFile(toParentDir).toPath(), StandardCopyOption.REPLACE_EXISTING); } return backup; } /** + * Create temporary directory. + * * @return the created temporary directory if (mergetol.writeToTemp == true) * or null if not configured or false. * @throws IOException @@ -148,20 +246,46 @@ public File createTempDirectory() throws IOException { } /** - * @return the tool names + * Get user defined tool names. + * + * @return the user defined tool names */ - public Set getToolNames() { - return config.getToolNames(); + public Set getUserDefinedToolNames() { + return userDefinedTools.keySet(); + } + + /** + * @return the predefined tool names + */ + public Set getPredefinedToolNames() { + return predefinedTools.keySet(); + } + + /** + * Get all tool names. + * + * @return the all tool names (default or available tool name is the first + * in the set) + */ + public Set getAllToolNames() { + String defaultName = getDefaultToolName(false); + if (defaultName == null) { + defaultName = getFirstAvailableTool(); + } + return ExternalToolUtils.createSortedToolSet(defaultName, + getUserDefinedToolNames(), getPredefinedToolNames()); } /** * @return the user defined tools */ public Map getUserDefinedTools() { - return userDefinedTools; + return Collections.unmodifiableMap(userDefinedTools); } /** + * Get predefined tools map. + * * @param checkAvailability * true: for checking if tools can be executed; ATTENTION: this * check took some time, do not execute often (store the map for @@ -175,20 +299,23 @@ public Map getPredefinedTools( if (checkAvailability) { for (ExternalMergeTool tool : predefinedTools.values()) { PreDefinedMergeTool predefTool = (PreDefinedMergeTool) tool; - predefTool.setAvailable(ExternalToolUtils.isToolAvailable(repo, - predefTool.getPath())); + predefTool.setAvailable(ExternalToolUtils.isToolAvailable(fs, + gitDir, workTree, predefTool.getPath())); } } - return predefinedTools; + return Collections.unmodifiableMap(predefinedTools); } /** + * Get first available tool name. + * * @return the name of first available predefined tool or null */ public String getFirstAvailableTool() { String name = null; for (ExternalMergeTool tool : predefinedTools.values()) { - if (ExternalToolUtils.isToolAvailable(repo, tool.getPath())) { + if (ExternalToolUtils.isToolAvailable(fs, gitDir, workTree, + tool.getPath())) { name = tool.getName(); break; } @@ -197,35 +324,24 @@ public String getFirstAvailableTool() { } /** - * @param gui - * use the diff.guitool setting ? - * @return the default tool name - */ - public String getDefaultToolName(BooleanTriState gui) { - return gui != BooleanTriState.UNSET ? "my_gui_tool" //$NON-NLS-1$ - : config.getDefaultToolName(); - } - - /** + * Is interactive merge (prompt enabled) ? + * * @return is interactive (config prompt enabled) ? */ public boolean isInteractive() { return config.isPrompt(); } - private ExternalMergeTool guessTool(String toolName, BooleanTriState gui) - throws ToolException { - if (StringUtils.isEmptyOrNull(toolName)) { - toolName = getDefaultToolName(gui); - } - ExternalMergeTool tool = null; - if (!StringUtils.isEmptyOrNull(toolName)) { - tool = getTool(toolName); - } - if (tool == null) { - throw new ToolException("Unknown merge tool '" + toolName + "'"); //$NON-NLS-1$ //$NON-NLS-2$ - } - return tool; + /** + * Get the default (gui-)tool name. + * + * @param gui + * use the diff.guitool setting ? + * @return the default tool name + */ + public String getDefaultToolName(boolean gui) { + return gui ? config.getDefaultGuiToolName() + : config.getDefaultToolName(); } private ExternalMergeTool getTool(final String name) { @@ -256,9 +372,9 @@ private Map setupPredefinedTools() { } private Map setupUserDefinedTools( - MergeToolConfig cfg, Map predefTools) { + Map predefTools) { Map tools = new TreeMap<>(); - Map userTools = cfg.getTools(); + Map userTools = config.getTools(); for (String name : userTools.keySet()) { ExternalMergeTool userTool = userTools.get(name); // if mergetool..cmd is defined we have user defined tool diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/PromptContinueHandler.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/PromptContinueHandler.java new file mode 100644 index 000000000..6ad33df2a --- /dev/null +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/PromptContinueHandler.java @@ -0,0 +1,27 @@ +/* + * Copyright (C) 2018-2019, Tim Neumann + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Distribution License v. 1.0 which is available at + * https://www.eclipse.org/org/documents/edl-v10.php. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +package org.eclipse.jgit.internal.diffmergetool; + +/** + * A handler for when the diff/merge tool manager wants to prompt the user + * whether to continue + */ +public interface PromptContinueHandler { + /** + * Prompt the user whether to continue with the next file by opening a given + * tool. + * + * @param toolName + * The name of the tool to open + * @return Whether the user wants to continue + */ + boolean prompt(String toolName); +} From c32694e5ae98e5c1373afcb535a76d37e5d82a29 Mon Sep 17 00:00:00 2001 From: Andre Bossert Date: Tue, 21 Jan 2020 10:13:43 +0100 Subject: [PATCH 3/3] Teach JGit to handle external diff/merge tools defined in .gitattributes Adds API that allows UI to find (and handle) diff/merge tools, specific for the given path. The assumption is that user can specify file type specific diff/merge tools via gitattributes. Bug: 552840 Change-Id: I1daa091e9afa542a9ebb5417853dff0452ed52dd Signed-off-by: Mykola Zakharchuk Signed-off-by: Andrey Loskutov Signed-off-by: Andre Bossert --- .../org/eclipse/jgit/pgm/DiffToolTest.java | 3 +- .../diffmergetool/ExternalDiffToolTest.java | 130 +++++++++++++++++- .../diffmergetool/ExternalMergeToolTest.java | 62 +++++++++ .../eclipse/jgit/internal/JGitText.properties | 5 + .../org/eclipse/jgit/internal/JGitText.java | 5 + .../internal/diffmergetool/DiffTools.java | 70 +++++++++- .../diffmergetool/ExternalToolUtils.java | 65 +++++++++ .../internal/diffmergetool/MergeTools.java | 54 +++++++- 8 files changed, 382 insertions(+), 12 deletions(-) diff --git a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/DiffToolTest.java b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/DiffToolTest.java index ce4c004c0..2b50d4525 100644 --- a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/DiffToolTest.java +++ b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/DiffToolTest.java @@ -79,7 +79,7 @@ public void testUserToolWithCommandNotFoundError() throws Exception { + errorReturnCode); } - @Test + @Test(expected = Die.class) public void testEmptyToolName() throws Exception { String emptyToolName = ""; @@ -95,6 +95,7 @@ public void testEmptyToolName() throws Exception { String[] expectedErrorOutput = { araxisErrorLine, araxisErrorLine, }; runAndCaptureUsingInitRaw(Arrays.asList(expectedErrorOutput), DIFF_TOOL, "--no-prompt"); + fail("Expected exception to be thrown due to external tool exiting with an error"); } @Test diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalDiffToolTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalDiffToolTest.java index 222608e31..f69a1794e 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalDiffToolTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalDiffToolTest.java @@ -24,6 +24,7 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import java.io.File; import java.io.IOException; import java.nio.file.Files; import java.util.Arrays; @@ -34,6 +35,8 @@ import java.util.Optional; import java.util.Set; +import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.internal.BooleanTriState; import org.eclipse.jgit.storage.file.FileBasedConfig; import org.eclipse.jgit.util.FS.ExecutionResult; @@ -127,13 +130,20 @@ public void testUserDefinedToolWithPrompt() throws Exception { @Test public void testUserDefinedToolWithCancelledPrompt() throws Exception { + String command = getEchoCommand(); + + FileBasedConfig config = db.getConfig(); + String customToolName = "customTool"; + config.setString(CONFIG_DIFFTOOL_SECTION, customToolName, + CONFIG_KEY_CMD, command); + DiffTools manager = new DiffTools(db); PromptHandler promptHandler = PromptHandler.cancelPrompt(); MissingToolHandler noToolHandler = new MissingToolHandler(); Optional result = manager.compare(local, remote, - Optional.empty(), BooleanTriState.TRUE, false, + Optional.of(customToolName), BooleanTriState.TRUE, false, BooleanTriState.TRUE, promptHandler, noToolHandler); assertFalse("Expected no result if user cancels the operation", result.isPresent()); @@ -245,8 +255,9 @@ public void testDefaultTool() throws Exception { gui = true; String defaultGuiToolName = manager.getDefaultToolName(gui); - assertNull("Expected default difftool to not be set", - defaultGuiToolName); + assertEquals( + "Expected default gui difftool to be the default tool if no gui tool is set", + toolName, defaultGuiToolName); config.setString(CONFIG_DIFF_SECTION, subsection, CONFIG_KEY_GUITOOL, guiToolName); @@ -296,6 +307,119 @@ public void testUndefinedTool() throws Exception { fail("Expected exception to be thrown due to not defined external diff tool"); } + @Test + public void testDefaultToolExecutionWithPrompt() throws Exception { + FileBasedConfig config = db.getConfig(); + // the default diff tool is configured without a subsection + String subsection = null; + config.setString("diff", subsection, "tool", "customTool"); + + String command = getEchoCommand(); + + config.setString("difftool", "customTool", "cmd", command); + + DiffTools manager = new DiffTools(db); + + PromptHandler promptHandler = PromptHandler.acceptPrompt(); + MissingToolHandler noToolHandler = new MissingToolHandler(); + + manager.compare(local, remote, Optional.empty(), BooleanTriState.TRUE, + false, BooleanTriState.TRUE, promptHandler, noToolHandler); + + assertEchoCommandHasCorrectOutput(); + } + + @Test + public void testNoDefaultToolName() { + DiffTools manager = new DiffTools(db); + boolean gui = false; + String defaultToolName = manager.getDefaultToolName(gui); + assertNull("Expected no default tool when none is configured", + defaultToolName); + + gui = true; + defaultToolName = manager.getDefaultToolName(gui); + assertNull("Expected no default tool when none is configured", + defaultToolName); + } + + @Test + public void testExternalToolInGitAttributes() throws Exception { + String content = "attributes:\n*.txt difftool=customTool"; + File gitattributes = writeTrashFile(".gitattributes", content); + gitattributes.deleteOnExit(); + try (TestRepository testRepository = new TestRepository<>( + db)) { + FileBasedConfig config = db.getConfig(); + config.setString("difftool", "customTool", "cmd", "echo"); + testRepository.git().add().addFilepattern(localFile.getName()) + .call(); + + testRepository.git().add().addFilepattern(".gitattributes").call(); + + testRepository.branch("master").commit().message("first commit") + .create(); + + DiffTools manager = new DiffTools(db); + Optional tool = manager + .getExternalToolFromAttributes(localFile.getName()); + assertTrue("Failed to find user defined tool", tool.isPresent()); + assertEquals("Failed to find user defined tool", "customTool", + tool.get()); + } finally { + Files.delete(gitattributes.toPath()); + } + } + + @Test + public void testNotExternalToolInGitAttributes() throws Exception { + String content = ""; + File gitattributes = writeTrashFile(".gitattributes", content); + gitattributes.deleteOnExit(); + try (TestRepository testRepository = new TestRepository<>( + db)) { + FileBasedConfig config = db.getConfig(); + config.setString("difftool", "customTool", "cmd", "echo"); + testRepository.git().add().addFilepattern(localFile.getName()) + .call(); + + testRepository.git().add().addFilepattern(".gitattributes").call(); + + testRepository.branch("master").commit().message("first commit") + .create(); + + DiffTools manager = new DiffTools(db); + Optional tool = manager + .getExternalToolFromAttributes(localFile.getName()); + assertFalse( + "Expected no external tool if no default tool is specified in .gitattributes", + tool.isPresent()); + } finally { + Files.delete(gitattributes.toPath()); + } + } + + @Test(expected = ToolException.class) + public void testNullTool() throws Exception { + DiffTools manager = new DiffTools(db); + + boolean trustExitCode = true; + ExternalDiffTool tool = null; + manager.compare(local, remote, tool, trustExitCode); + } + + @Test(expected = ToolException.class) + public void testNullToolWithPrompt() throws Exception { + DiffTools manager = new DiffTools(db); + + PromptHandler promptHandler = PromptHandler.cancelPrompt(); + MissingToolHandler noToolHandler = new MissingToolHandler(); + + Optional tool = null; + manager.compare(local, remote, tool, BooleanTriState.TRUE, false, + BooleanTriState.TRUE, promptHandler, noToolHandler); + } + private Optional invokeCompare(String toolName) throws ToolException { DiffTools manager = new DiffTools(db); diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalMergeToolTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalMergeToolTest.java index 130b42a92..94b67b374 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalMergeToolTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/diffmergetool/ExternalMergeToolTest.java @@ -329,6 +329,68 @@ public void testUndefinedTool() throws Exception { fail("Expected exception to be thrown due to not defined external merge tool"); } + @Test + public void testDefaultToolExecutionWithPrompt() throws Exception { + FileBasedConfig config = db.getConfig(); + // the default diff tool is configured without a subsection + String subsection = null; + config.setString("merge", subsection, "tool", "customTool"); + + String command = getEchoCommand(); + + config.setString("mergetool", "customTool", "cmd", command); + + MergeTools manager = new MergeTools(db); + + PromptHandler promptHandler = PromptHandler.acceptPrompt(); + MissingToolHandler noToolHandler = new MissingToolHandler(); + + manager.merge(local, remote, merged, base, null, Optional.empty(), + BooleanTriState.TRUE, false, promptHandler, noToolHandler); + + assertEchoCommandHasCorrectOutput(); + } + + @Test + public void testNoDefaultToolName() { + MergeTools manager = new MergeTools(db); + boolean gui = false; + String defaultToolName = manager.getDefaultToolName(gui); + assertNull("Expected no default tool when none is configured", + defaultToolName); + + gui = true; + defaultToolName = manager.getDefaultToolName(gui); + assertNull("Expected no default tool when none is configured", + defaultToolName); + } + + @Test(expected = ToolException.class) + public void testNullTool() throws Exception { + MergeTools manager = new MergeTools(db); + + PromptHandler promptHandler = null; + MissingToolHandler noToolHandler = null; + + Optional tool = null; + + manager.merge(local, remote, merged, base, null, tool, + BooleanTriState.TRUE, false, promptHandler, noToolHandler); + } + + @Test(expected = ToolException.class) + public void testNullToolWithPrompt() throws Exception { + MergeTools manager = new MergeTools(db); + + PromptHandler promptHandler = PromptHandler.cancelPrompt(); + MissingToolHandler noToolHandler = new MissingToolHandler(); + + Optional tool = null; + + manager.merge(local, remote, merged, base, null, tool, + BooleanTriState.TRUE, false, promptHandler, noToolHandler); + } + private Optional invokeMerge(String toolName) throws ToolException { BooleanTriState prompt = BooleanTriState.UNSET; diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index 9f264fca3..f0bb6c6c9 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -237,6 +237,9 @@ deleteTagUnexpectedResult=Delete tag returned unexpected result {0} deletingNotSupported=Deleting {0} not supported. destinationIsNotAWildcard=Destination is not a wildcard. detachedHeadDetected=HEAD is detached +diffToolNotGivenError=No diff tool provided and no defaults configured. +diffToolNotSpecifiedInGitAttributesError=Diff tool specified in git attributes cannot be found. +diffToolNullError=Parameter for diff tool cannot be null. dirCacheDoesNotHaveABackingFile=DirCache does not have a backing file dirCacheFileIsNotLocked=DirCache {0} not locked dirCacheIsNotLocked=DirCache is not locked @@ -457,6 +460,8 @@ mergeStrategyDoesNotSupportHeads=merge strategy {0} does not support {1} heads t mergeUsingStrategyResultedInDescription=Merge of revisions {0} with base {1} using strategy {2} resulted in: {3}. {4} mergeRecursiveConflictsWhenMergingCommonAncestors=Multiple common ancestors were found and merging them resulted in a conflict: {0}, {1} mergeRecursiveTooManyMergeBasesFor = "More than {0} merge bases for:\n a {1}\n b {2} found:\n count {3}" +mergeToolNotGivenError=No merge tool provided and no defaults configured. +mergeToolNullError=Parameter for merge tool cannot be null. messageAndTaggerNotAllowedInUnannotatedTags = Unannotated tags cannot have a message or tagger minutesAgo={0} minutes ago mismatchOffset=mismatch offset for object {0} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java index b81e605c1..17e359de4 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -265,6 +265,9 @@ public static JGitText get() { /***/ public String deletingNotSupported; /***/ public String destinationIsNotAWildcard; /***/ public String detachedHeadDetected; + /***/ public String diffToolNotGivenError; + /***/ public String diffToolNotSpecifiedInGitAttributesError; + /***/ public String diffToolNullError; /***/ public String dirCacheDoesNotHaveABackingFile; /***/ public String dirCacheFileIsNotLocked; /***/ public String dirCacheIsNotLocked; @@ -485,6 +488,8 @@ public static JGitText get() { /***/ public String mergeUsingStrategyResultedInDescription; /***/ public String mergeRecursiveConflictsWhenMergingCommonAncestors; /***/ public String mergeRecursiveTooManyMergeBasesFor; + /***/ public String mergeToolNotGivenError; + /***/ public String mergeToolNullError; /***/ public String messageAndTaggerNotAllowedInUnannotatedTags; /***/ public String minutesAgo; /***/ public String mismatchOffset; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/DiffTools.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/DiffTools.java index 5fe263a4e..7cedd8299 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/DiffTools.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/DiffTools.java @@ -13,18 +13,22 @@ import java.io.File; import java.io.IOException; -import java.util.ArrayList; import java.util.Collections; +import java.util.LinkedHashSet; import java.util.Map; +import java.util.Map.Entry; import java.util.Optional; import java.util.Set; import java.util.TreeMap; +import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.StoredConfig; import org.eclipse.jgit.lib.internal.BooleanTriState; +import org.eclipse.jgit.treewalk.TreeWalk; import org.eclipse.jgit.util.FS; import org.eclipse.jgit.util.FS.ExecutionResult; +import org.eclipse.jgit.util.StringUtils; /** * Manages diff tools. @@ -39,6 +43,8 @@ public class DiffTools { private final DiffToolConfig config; + private final Repository repo; + private final Map predefinedTools; private final Map userDefinedTools; @@ -64,6 +70,7 @@ public DiffTools(StoredConfig config) { } private DiffTools(Repository repo, StoredConfig config) { + this.repo = repo; this.config = config.get(DiffToolConfig.KEY); this.gitDir = repo == null ? null : repo.getDirectory(); this.fs = repo == null ? FS.DETECTED : repo.getFS(); @@ -108,15 +115,18 @@ public Optional compare(FileElement localFile, String toolNameToUse; + if (toolName == null) { + throw new ToolException(JGitText.get().diffToolNullError); + } + if (toolName.isPresent()) { toolNameToUse = toolName.get(); } else { toolNameToUse = getDefaultToolName(gui); + } - if (toolNameToUse == null || toolNameToUse.isEmpty()) { - noToolHandler.inform(new ArrayList<>(predefinedTools.keySet())); - toolNameToUse = getFirstAvailableTool(); - } + if (StringUtils.isEmptyOrNull(toolNameToUse)) { + throw new ToolException(JGitText.get().diffToolNotGivenError); } boolean doPrompt; @@ -167,6 +177,10 @@ public ExecutionResult compare(FileElement localFile, FileElement remoteFile, ExternalDiffTool tool, boolean trustExitCode) throws ToolException { try { + if (tool == null) { + throw new ToolException(JGitText + .get().diffToolNotSpecifiedInGitAttributesError); + } // prepare the command (replace the file paths) String command = ExternalToolUtils.prepareCommand(tool.getCommand(), localFile, remoteFile, null, null); @@ -217,6 +231,42 @@ public Set getAllToolNames() { getUserDefinedToolNames(), getPredefinedToolNames()); } + /** + * Provides {@link Optional} with the name of an external diff tool if + * specified in git configuration for a path. + * + * The formed git configuration results from global rules as well as merged + * rules from info and worktree attributes. + * + * Triggers {@link TreeWalk} until specified path found in the tree. + * + * @param path + * path to the node in repository to parse git attributes for + * @return name of the difftool if set + * @throws ToolException + */ + public Optional getExternalToolFromAttributes(final String path) + throws ToolException { + return ExternalToolUtils.getExternalToolFromAttributes(repo, path, + ExternalToolUtils.KEY_DIFF_TOOL); + } + + /** + * Checks the availability of the predefined tools in the system. + * + * @return set of predefined available tools + */ + public Set getPredefinedAvailableTools() { + Map defTools = getPredefinedTools(true); + Set availableTools = new LinkedHashSet<>(); + for (Entry elem : defTools.entrySet()) { + if (elem.getValue().isAvailable()) { + availableTools.add(elem.getKey()); + } + } + return availableTools; + } + /** * Get user defined tools map. * @@ -272,8 +322,14 @@ public String getFirstAvailableTool() { * @return the default tool name */ public String getDefaultToolName(boolean gui) { - return gui ? config.getDefaultGuiToolName() - : config.getDefaultToolName(); + String guiToolName; + if (gui) { + guiToolName = config.getDefaultGuiToolName(); + if (guiToolName != null) { + return guiToolName; + } + } + return config.getDefaultToolName(); } /** diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ExternalToolUtils.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ExternalToolUtils.java index 9a6968113..b2dd846d7 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ExternalToolUtils.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/ExternalToolUtils.java @@ -14,9 +14,17 @@ import java.io.IOException; import java.util.LinkedHashSet; import java.util.Map; +import java.util.Optional; import java.util.Set; +import org.eclipse.jgit.attributes.Attributes; +import org.eclipse.jgit.errors.RevisionSyntaxException; import org.eclipse.jgit.lib.Constants; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.treewalk.FileTreeIterator; +import org.eclipse.jgit.treewalk.TreeWalk; +import org.eclipse.jgit.treewalk.WorkingTreeIterator; +import org.eclipse.jgit.treewalk.filter.NotIgnoredFilter; import org.eclipse.jgit.util.FS; /** @@ -24,6 +32,16 @@ */ public class ExternalToolUtils { + /** + * Key for merge tool git configuration section + */ + public static final String KEY_MERGE_TOOL = "mergetool"; //$NON-NLS-1$ + + /** + * Key for diff tool git configuration section + */ + public static final String KEY_DIFF_TOOL = "difftool"; //$NON-NLS-1$ + /** * Prepare command for execution. * @@ -174,4 +192,51 @@ public static Set createSortedToolSet(String defaultName, return names; } + /** + * Provides {@link Optional} with the name of an external tool if specified + * in git configuration for a path. + * + * The formed git configuration results from global rules as well as merged + * rules from info and worktree attributes. + * + * Triggers {@link TreeWalk} until specified path found in the tree. + * + * @param repository + * target repository to traverse into + * @param path + * path to the node in repository to parse git attributes for + * @param toolKey + * config key name for the tool + * @return attribute value for the given tool key if set + * @throws ToolException + */ + public static Optional getExternalToolFromAttributes( + final Repository repository, final String path, + final String toolKey) throws ToolException { + try { + WorkingTreeIterator treeIterator = new FileTreeIterator(repository); + try (TreeWalk walk = new TreeWalk(repository)) { + walk.addTree(treeIterator); + walk.setFilter(new NotIgnoredFilter(0)); + while (walk.next()) { + String treePath = walk.getPathString(); + if (treePath.equals(path)) { + Attributes attrs = walk.getAttributes(); + if (attrs.containsKey(toolKey)) { + return Optional.of(attrs.getValue(toolKey)); + } + } + if (walk.isSubtree()) { + walk.enterSubtree(); + } + } + // no external tool specified + return Optional.empty(); + } + + } catch (RevisionSyntaxException | IOException e) { + throw new ToolException(e); + } + } + } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/MergeTools.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/MergeTools.java index d2055272e..b90320126 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/MergeTools.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/diffmergetool/MergeTools.java @@ -18,16 +18,21 @@ import java.nio.file.StandardCopyOption; import java.util.ArrayList; import java.util.Collections; +import java.util.LinkedHashSet; import java.util.Map; +import java.util.Map.Entry; import java.util.Optional; import java.util.Set; import java.util.TreeMap; +import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.internal.diffmergetool.FileElement.Type; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.StoredConfig; import org.eclipse.jgit.lib.internal.BooleanTriState; +import org.eclipse.jgit.treewalk.TreeWalk; import org.eclipse.jgit.util.FS; +import org.eclipse.jgit.util.StringUtils; import org.eclipse.jgit.util.FS.ExecutionResult; /** @@ -43,6 +48,8 @@ public class MergeTools { private final MergeToolConfig config; + private final Repository repo; + private final Map predefinedTools; private final Map userDefinedTools; @@ -68,6 +75,7 @@ public MergeTools(StoredConfig config) { } private MergeTools(Repository repo, StoredConfig config) { + this.repo = repo; this.config = config.get(MergeToolConfig.KEY); this.gitDir = repo == null ? null : repo.getDirectory(); this.fs = repo == null ? FS.DETECTED : repo.getFS(); @@ -116,17 +124,25 @@ public Optional merge(FileElement localFile, String toolNameToUse; + if (toolName == null) { + throw new ToolException(JGitText.get().diffToolNullError); + } + if (toolName.isPresent()) { toolNameToUse = toolName.get(); } else { toolNameToUse = getDefaultToolName(gui); - if (toolNameToUse == null || toolNameToUse.isEmpty()) { + if (StringUtils.isEmptyOrNull(toolNameToUse)) { noToolHandler.inform(new ArrayList<>(predefinedTools.keySet())); toolNameToUse = getFirstAvailableTool(); } } + if (StringUtils.isEmptyOrNull(toolNameToUse)) { + throw new ToolException(JGitText.get().diffToolNotGivenError); + } + boolean doPrompt; if (prompt != BooleanTriState.UNSET) { doPrompt = prompt == BooleanTriState.TRUE; @@ -276,6 +292,42 @@ public Set getAllToolNames() { getUserDefinedToolNames(), getPredefinedToolNames()); } + /** + * Provides {@link Optional} with the name of an external merge tool if + * specified in git configuration for a path. + * + * The formed git configuration results from global rules as well as merged + * rules from info and worktree attributes. + * + * Triggers {@link TreeWalk} until specified path found in the tree. + * + * @param path + * path to the node in repository to parse git attributes for + * @return name of the difftool if set + * @throws ToolException + */ + public Optional getExternalToolFromAttributes(final String path) + throws ToolException { + return ExternalToolUtils.getExternalToolFromAttributes(repo, path, + ExternalToolUtils.KEY_MERGE_TOOL); + } + + /** + * Checks the availability of the predefined tools in the system. + * + * @return set of predefined available tools + */ + public Set getPredefinedAvailableTools() { + Map defTools = getPredefinedTools(true); + Set availableTools = new LinkedHashSet<>(); + for (Entry elem : defTools.entrySet()) { + if (elem.getValue().isAvailable()) { + availableTools.add(elem.getKey()); + } + } + return availableTools; + } + /** * @return the user defined tools */