From 540b29bf4266f3ec974cdf230faca32ab712843b Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Mon, 1 Mar 2021 12:17:54 +0100 Subject: Remove ReftableNumbersNotIncreasingException In a distributed setting, one can have multiple datacenters use reftables for serving, while the ground truth for the Ref database is administered centrally. In this setting, replication delays combined with compaction can cause update-index ranges to overlap. Such a setting is used at Google, and the JGit code already handles this correctly (modulo a bugfix that applied in change I8f8215b99a). Remove the restriction that was applied at FileReftableDatabase. Signed-off-by: Han-Wen Nienhuys Change-Id: I6f9ed0fbd7fbc5220083ab808b22a909215f13a9 --- .../internal/storage/file/FileReftableStack.java | 37 ---------------------- 1 file changed, 37 deletions(-) (limited to 'org.eclipse.jgit/src/org/eclipse/jgit') diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableStack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableStack.java index bc2039c56b..71130f0f3b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableStack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableStack.java @@ -128,33 +128,6 @@ public class FileReftableStack implements AutoCloseable { return stats; } - /** Thrown if the update indices in the stack are not monotonic */ - public static class ReftableNumbersNotIncreasingException - extends RuntimeException { - private static final long serialVersionUID = 1L; - - String name; - - long lastMax; - - long min; - - ReftableNumbersNotIncreasingException(String name, long lastMax, - long min) { - this.name = name; - this.lastMax = lastMax; - this.min = min; - } - - @SuppressWarnings({ "nls", "boxing" }) - @Override - public String toString() { - return String.format( - "ReftableNumbersNotIncreasingException %s: min %d, lastMax %d", - name, min, lastMax); - } - } - /** * Reloads the stack, potentially reusing opened reftableReaders. * @@ -173,7 +146,6 @@ public class FileReftableStack implements AutoCloseable { List newTables = new ArrayList<>(); List newStack = new ArrayList<>(stack.size() + 1); try { - ReftableReader last = null; for (String name : names) { StackEntry entry = new StackEntry(); entry.name = name; @@ -191,15 +163,6 @@ public class FileReftableStack implements AutoCloseable { newTables.add(t); } - if (last != null) { - // TODO: move this to MergedReftable - if (last.maxUpdateIndex() >= t.minUpdateIndex()) { - throw new ReftableNumbersNotIncreasingException(name, - last.maxUpdateIndex(), t.minUpdateIndex()); - } - } - last = t; - entry.reftableReader = t; newStack.add(entry); } -- cgit v1.2.3 From ffc1f9b02618a59ee72298e9af15f64fe157fa8a Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Thu, 18 Mar 2021 21:16:48 +0100 Subject: sshd: implement ssh config PubkeyAcceptedAlgorithms Apache MINA sshd 2.6.0 appears to use only the first appropriate public key signature algorithm for a particular key. See [1]. For RSA keys, that is rsa-sha2-512. This breaks authentication at servers that only know the older (and deprecated) ssh-rsa algorithm. With PubkeyAcceptedAlgorithms, users can re-order algorithms in the ssh config file per host, if needed. Setting PubkeyAcceptedAlgorithms ^ssh-rsa will put "ssh-rsa" at the front of the list of algorithms, and then authentication at such servers with RSA keys works again. [1] https://issues.apache.org/jira/browse/SSHD-1105 Bug: 572056 Change-Id: I86c3b93f05960c68936e80642965815926bb2532 Signed-off-by: Thomas Wolf --- .../META-INF/MANIFEST.MF | 1 + org.eclipse.jgit.ssh.apache.test/build.properties | 2 + .../eclipse/jgit/transport/sshd/ApacheSshTest.java | 108 ++++++++++++++++----- .../internal/transport/sshd/SshdText.properties | 3 +- .../internal/transport/sshd/JGitClientSession.java | 107 +++++++++++++------- .../internal/transport/sshd/JGitSshClient.java | 18 ++++ .../jgit/internal/transport/sshd/SshdText.java | 3 +- .../org/eclipse/jgit/transport/SshConstants.java | 8 ++ 8 files changed, 184 insertions(+), 66 deletions(-) (limited to 'org.eclipse.jgit/src/org/eclipse/jgit') diff --git a/org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF b/org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF index 30eb2bf8b6..b035453529 100644 --- a/org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF +++ b/org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF @@ -14,6 +14,7 @@ Import-Package: org.apache.sshd.client.config.hosts;version="[2.6.0,2.7.0)", org.apache.sshd.common.helpers;version="[2.6.0,2.7.0)", org.apache.sshd.common.keyprovider;version="[2.6.0,2.7.0)", org.apache.sshd.common.session;version="[2.6.0,2.7.0)", + org.apache.sshd.common.signature;version="[2.6.0,2.7.0)", org.apache.sshd.common.util.net;version="[2.6.0,2.7.0)", org.apache.sshd.common.util.security;version="[2.6.0,2.7.0)", org.apache.sshd.core;version="[2.6.0,2.7.0)", diff --git a/org.eclipse.jgit.ssh.apache.test/build.properties b/org.eclipse.jgit.ssh.apache.test/build.properties index 9ffa0caf78..406c5a768f 100644 --- a/org.eclipse.jgit.ssh.apache.test/build.properties +++ b/org.eclipse.jgit.ssh.apache.test/build.properties @@ -3,3 +3,5 @@ output.. = bin/ bin.includes = META-INF/,\ .,\ plugin.properties +additional.bundles = org.apache.log4j,\ + org.slf4j.binding.log4j12 diff --git a/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java b/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java index 97f97f9028..09d048b4fa 100644 --- a/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java +++ b/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java @@ -47,7 +47,9 @@ import org.eclipse.jgit.api.Git; import org.eclipse.jgit.api.errors.TransportException; import org.eclipse.jgit.junit.ssh.SshTestBase; import org.eclipse.jgit.lib.Constants; +import org.eclipse.jgit.transport.RemoteSession; import org.eclipse.jgit.transport.SshSessionFactory; +import org.eclipse.jgit.transport.URIish; import org.eclipse.jgit.util.FS; import org.junit.Test; import org.junit.experimental.theories.Theories; @@ -232,64 +234,89 @@ public class ApacheSshTest extends SshTestBase { } /** - * Creates a simple proxy server. Accepts only publickey authentication from - * the given user with the given key, allows all forwardings. Adds the - * proxy's host key to {@link #knownHosts}. + * Creates a simple SSH server without git setup. * * @param user * to accept * @param userKey * public key of that user at this server - * @param report - * single-element array to report back the forwarded address. - * @return the started server + * @return the {@link SshServer}, not yet started * @throws Exception */ - private SshServer createProxy(String user, File userKey, - SshdSocketAddress[] report) throws Exception { - SshServer proxy = SshServer.setUpDefaultServer(); + private SshServer createServer(String user, File userKey) throws Exception { + SshServer srv = SshServer.setUpDefaultServer(); // Give the server its own host key KeyPairGenerator generator = KeyPairGenerator.getInstance("RSA"); generator.initialize(2048); KeyPair proxyHostKey = generator.generateKeyPair(); - proxy.setKeyPairProvider( + srv.setKeyPairProvider( session -> Collections.singletonList(proxyHostKey)); // Allow (only) publickey authentication - proxy.setUserAuthFactories(Collections.singletonList( + srv.setUserAuthFactories(Collections.singletonList( ServerAuthenticationManager.DEFAULT_USER_AUTH_PUBLIC_KEY_FACTORY)); // Install the user's public key PublicKey userProxyKey = AuthorizedKeyEntry .readAuthorizedKeys(userKey.toPath()).get(0) .resolvePublicKey(null, PublicKeyEntryResolver.IGNORING); - proxy.setPublickeyAuthenticator( + srv.setPublickeyAuthenticator( (userName, publicKey, session) -> user.equals(userName) && KeyUtils.compareKeys(userProxyKey, publicKey)); - // Allow forwarding - proxy.setForwardingFilter(new StaticDecisionForwardingFilter(true) { + return srv; + } - @Override - protected boolean checkAcceptance(String request, Session session, - SshdSocketAddress target) { - report[0] = target; - return super.checkAcceptance(request, session, target); - } - }); - proxy.start(); + /** + * Writes the server's host key to our knownhosts file. + * + * @param srv to register + * @throws Exception + */ + private void registerServer(SshServer srv) throws Exception { // Add the proxy's host key to knownhosts try (BufferedWriter writer = Files.newBufferedWriter( knownHosts.toPath(), StandardCharsets.US_ASCII, StandardOpenOption.WRITE, StandardOpenOption.APPEND)) { writer.append('\n'); KnownHostHashValue.appendHostPattern(writer, "localhost", - proxy.getPort()); + srv.getPort()); writer.append(','); KnownHostHashValue.appendHostPattern(writer, "127.0.0.1", - proxy.getPort()); + srv.getPort()); writer.append(' '); PublicKeyEntry.appendPublicKeyEntry(writer, - proxyHostKey.getPublic()); + srv.getKeyPairProvider().loadKeys(null).iterator().next().getPublic()); writer.append('\n'); } + } + + /** + * Creates a simple proxy server. Accepts only publickey authentication from + * the given user with the given key, allows all forwardings. Adds the + * proxy's host key to {@link #knownHosts}. + * + * @param user + * to accept + * @param userKey + * public key of that user at this server + * @param report + * single-element array to report back the forwarded address. + * @return the started server + * @throws Exception + */ + private SshServer createProxy(String user, File userKey, + SshdSocketAddress[] report) throws Exception { + SshServer proxy = createServer(user, userKey); + // Allow forwarding + proxy.setForwardingFilter(new StaticDecisionForwardingFilter(true) { + + @Override + protected boolean checkAcceptance(String request, Session session, + SshdSocketAddress target) { + report[0] = target; + return super.checkAcceptance(request, session, target); + } + }); + proxy.start(); + registerServer(proxy); return proxy; } @@ -606,4 +633,35 @@ public class ApacheSshTest extends SshTestBase { } } } + + /** + * Tests that one can log in to an old server that doesn't handle + * rsa-sha2-512 if one puts ssh-rsa first in the client's list of public key + * signature algorithms. + * + * @see bug + * 572056 + * @throws Exception + * on failure + */ + @Test + public void testConnectAuthSshRsaPubkeyAcceptedAlgorithms() + throws Exception { + try (SshServer oldServer = createServer(TEST_USER, publicKey1)) { + oldServer.setSignatureFactoriesNames("ssh-rsa"); + oldServer.start(); + registerServer(oldServer); + installConfig("Host server", // + "HostName localhost", // + "Port " + oldServer.getPort(), // + "User " + TEST_USER, // + "IdentityFile " + privateKey1.getAbsolutePath(), // + "PubkeyAcceptedAlgorithms ^ssh-rsa"); + RemoteSession session = getSessionFactory().getSession( + new URIish("ssh://server/doesntmatter"), null, FS.DETECTED, + 10000); + assertNotNull(session); + session.disconnect(); + } + } } diff --git a/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties b/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties index f810fd40e4..16b5738332 100644 --- a/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties +++ b/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties @@ -5,8 +5,7 @@ configInvalidPath=Invalid path in ssh config key {0}: {1} configInvalidPattern=Invalid pattern in ssh config key {0}: {1} configInvalidPositive=Ssh config entry {0} must be a strictly positive number but is ''{1}'' configInvalidProxyJump=Ssh config, host ''{0}'': Cannot parse ProxyJump ''{1}'' -configNoKnownHostKeyAlgorithms=No implementations for any of the algorithms ''{0}'' given in HostKeyAlgorithms in the ssh config; using the default. -configNoRemainingHostKeyAlgorithms=Ssh config removed all host key algorithms: HostKeyAlgorithms ''{0}'' +configNoKnownAlgorithms=Ssh config ''{0}'' ''{1}'' resulted in empty list (none known, or all known removed); using default. configProxyJumpNotSsh=Non-ssh URI in ProxyJump ssh config configProxyJumpWithPath=ProxyJump ssh config: jump host specification must not have a path ftpCloseFailed=Closing the SFTP channel failed diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java index 66713ba632..8183a92b9f 100644 --- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java +++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java @@ -21,6 +21,7 @@ import java.security.PublicKey; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashSet; import java.util.List; @@ -45,6 +46,7 @@ import org.eclipse.jgit.fnmatch.FileNameMatcher; import org.eclipse.jgit.internal.transport.sshd.proxy.StatefulProxyConnector; import org.eclipse.jgit.transport.CredentialsProvider; import org.eclipse.jgit.transport.SshConstants; +import org.eclipse.jgit.util.StringUtils; /** * A {@link org.apache.sshd.client.session.ClientSession ClientSession} that can @@ -201,48 +203,23 @@ public class JGitClientSession extends ClientSessionImpl { @Override protected String resolveAvailableSignaturesProposal( FactoryManager manager) { - Set defaultSignatures = new LinkedHashSet<>(); - defaultSignatures.addAll(getSignatureFactoriesNames()); + List defaultSignatures = getSignatureFactoriesNames(); HostConfigEntry config = resolveAttribute( JGitSshClient.HOST_CONFIG_ENTRY); - String hostKeyAlgorithms = config + String algorithms = config .getProperty(SshConstants.HOST_KEY_ALGORITHMS); - if (hostKeyAlgorithms != null && !hostKeyAlgorithms.isEmpty()) { - char first = hostKeyAlgorithms.charAt(0); - switch (first) { - case '+': - // Additions make not much sense -- it's either in - // defaultSignatures already, or we have no implementation for - // it. No point in proposing it. - return String.join(",", defaultSignatures); //$NON-NLS-1$ - case '-': - // This takes wildcard patterns! - removeFromList(defaultSignatures, - SshConstants.HOST_KEY_ALGORITHMS, - hostKeyAlgorithms.substring(1)); - if (defaultSignatures.isEmpty()) { - // Too bad: user config error. Warn here, and then fail - // later. - log.warn(format( - SshdText.get().configNoRemainingHostKeyAlgorithms, - hostKeyAlgorithms)); - } - return String.join(",", defaultSignatures); //$NON-NLS-1$ - default: - // Default is overridden -- only accept the ones for which we do - // have an implementation. - List newNames = filteredList(defaultSignatures, - hostKeyAlgorithms); - if (newNames.isEmpty()) { - log.warn(format( - SshdText.get().configNoKnownHostKeyAlgorithms, - hostKeyAlgorithms)); - // Use the default instead. - } else { - return String.join(",", newNames); //$NON-NLS-1$ + if (!StringUtils.isEmptyOrNull(algorithms)) { + List result = modifyAlgorithmList(defaultSignatures, + algorithms, SshConstants.HOST_KEY_ALGORITHMS); + if (!result.isEmpty()) { + if (log.isDebugEnabled()) { + log.debug(SshConstants.HOST_KEY_ALGORITHMS + ' ' + result); } - break; + return String.join(",", result); //$NON-NLS-1$ } + log.warn(format(SshdText.get().configNoKnownAlgorithms, + SshConstants.HOST_KEY_ALGORITHMS, + algorithms)); } // No HostKeyAlgorithms; using default -- change order to put existing // keys first. @@ -262,11 +239,67 @@ public class JGitClientSession extends ClientSessionImpl { } } reordered.addAll(defaultSignatures); + if (log.isDebugEnabled()) { + log.debug(SshConstants.HOST_KEY_ALGORITHMS + ' ' + reordered); + } return String.join(",", reordered); //$NON-NLS-1$ } + if (log.isDebugEnabled()) { + log.debug( + SshConstants.HOST_KEY_ALGORITHMS + ' ' + defaultSignatures); + } return String.join(",", defaultSignatures); //$NON-NLS-1$ } + /** + * Modifies a given algorithm list according to a list from the ssh config, + * including remove ('-') and reordering ('^') operators. Addition ('+') is + * not handled since we have no way of adding dynamically implementations, + * and the defaultList is supposed to contain all known implementations + * already. + * + * @param defaultList + * to modify + * @param fromConfig + * telling how to modify the {@code defaultList}, must not be + * {@code null} or empty + * @param overrideKey + * ssh config key; used for logging + * @return the modified list or {@code null} if {@code overrideKey} is not + * set + */ + public List modifyAlgorithmList(List defaultList, + String fromConfig, String overrideKey) { + Set defaults = new LinkedHashSet<>(); + defaults.addAll(defaultList); + switch (fromConfig.charAt(0)) { + case '+': + // Additions make not much sense -- it's either in + // defaultList already, or we have no implementation for + // it. No point in proposing it. + return defaultList; + case '-': + // This takes wildcard patterns! + removeFromList(defaults, overrideKey, fromConfig.substring(1)); + return new ArrayList<>(defaults); + case '^': + // Specified entries go to the front of the default list + List allSignatures = filteredList(defaults, + fromConfig.substring(1)); + Set atFront = new HashSet<>(allSignatures); + for (String sig : defaults) { + if (!atFront.contains(sig)) { + allSignatures.add(sig); + } + } + return allSignatures; + default: + // Default is overridden -- only accept the ones for which we do + // have an implementation. + return filteredList(defaults, fromConfig); + } + } + private void removeFromList(Set current, String key, String patterns) { for (String toRemove : patterns.split("\\s*,\\s*")) { //$NON-NLS-1$ diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitSshClient.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitSshClient.java index 74455dc808..071e1979d3 100644 --- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitSshClient.java +++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitSshClient.java @@ -267,6 +267,24 @@ public class JGitSshClient extends SshClient { session.setUsername(username); session.setConnectAddress(address); session.setHostConfigEntry(hostConfig); + // Set signature algorithms for public key authentication + String pubkeyAlgos = hostConfig + .getProperty(SshConstants.PUBKEY_ACCEPTED_ALGORITHMS); + if (!StringUtils.isEmptyOrNull(pubkeyAlgos)) { + List signatures = getSignatureFactoriesNames(); + signatures = session.modifyAlgorithmList(signatures, pubkeyAlgos, + SshConstants.PUBKEY_ACCEPTED_ALGORITHMS); + if (!signatures.isEmpty()) { + if (log.isDebugEnabled()) { + log.debug(SshConstants.PUBKEY_ACCEPTED_ALGORITHMS + ' ' + + signatures); + } + session.setSignatureFactoriesNames(signatures); + } else { + log.warn(format(SshdText.get().configNoKnownAlgorithms, + SshConstants.PUBKEY_ACCEPTED_ALGORITHMS, pubkeyAlgos)); + } + } if (session.getCredentialsProvider() == null) { session.setCredentialsProvider(getCredentialsProvider()); } diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java index 13bb3ebe75..4c4ff5949d 100644 --- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java +++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java @@ -25,8 +25,7 @@ public final class SshdText extends TranslationBundle { /***/ public String configInvalidPattern; /***/ public String configInvalidPositive; /***/ public String configInvalidProxyJump; - /***/ public String configNoKnownHostKeyAlgorithms; - /***/ public String configNoRemainingHostKeyAlgorithms; + /***/ public String configNoKnownAlgorithms; /***/ public String configProxyJumpNotSsh; /***/ public String configProxyJumpWithPath; /***/ public String ftpCloseFailed; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SshConstants.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SshConstants.java index fff2938e5d..be55cd1b81 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SshConstants.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SshConstants.java @@ -114,6 +114,14 @@ public final class SshConstants { /** Key in an ssh config file. */ public static final String PREFERRED_AUTHENTICATIONS = "PreferredAuthentications"; + /** + * Key in an ssh config file; defines signature algorithms for public key + * authentication as a comma-separated list. + * + * @since 5.11 + */ + public static final String PUBKEY_ACCEPTED_ALGORITHMS = "PubkeyAcceptedAlgorithms"; + /** Key in an ssh config file. */ public static final String PROXY_COMMAND = "ProxyCommand"; -- cgit v1.2.3 From 6faee128f8930b851d33f1f06cb77b3e1b9a0cc5 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Fri, 19 Mar 2021 09:24:31 +0100 Subject: sshd: modernize ssh config file parsing OpenSSH has changed some things in ssh config files. Update our parser to implement some of these changes: * ignore trailing comments on a line * rename PubkeyAcceptedKeyTypes to PubkeyAcceptedAlgorithms Note that for the rename, openSSH still accepts both names. We do the same, translating names whenever we get or set values. Change-Id: Icccca060e6a4350a7acf05ff9e260f2c8c60ee1a Signed-off-by: Thomas Wolf --- .../eclipse/jgit/transport/OpenSshConfigTest.java | 30 +++++++++ .../internal/transport/ssh/OpenSshConfigFile.java | 73 +++++++++++++++------- 2 files changed, 81 insertions(+), 22 deletions(-) (limited to 'org.eclipse.jgit/src/org/eclipse/jgit') diff --git a/org.eclipse.jgit.ssh.jsch.test/tst/org/eclipse/jgit/transport/OpenSshConfigTest.java b/org.eclipse.jgit.ssh.jsch.test/tst/org/eclipse/jgit/transport/OpenSshConfigTest.java index af09f499f5..4c7e99ea80 100644 --- a/org.eclipse.jgit.ssh.jsch.test/tst/org/eclipse/jgit/transport/OpenSshConfigTest.java +++ b/org.eclipse.jgit.ssh.jsch.test/tst/org/eclipse/jgit/transport/OpenSshConfigTest.java @@ -467,4 +467,34 @@ public class OpenSshConfigTest extends RepositoryTestCase { new File(new File(home, ".ssh"), localhost + "_id_dsa"), h.getIdentityFile()); } + + @Test + public void testPubKeyAcceptedAlgorithms() throws Exception { + config("Host=orcz\n\tPubkeyAcceptedAlgorithms ^ssh-rsa"); + Host h = osc.lookup("orcz"); + Config c = h.getConfig(); + assertEquals("^ssh-rsa", + c.getValue(SshConstants.PUBKEY_ACCEPTED_ALGORITHMS)); + assertEquals("^ssh-rsa", c.getValue("PubkeyAcceptedKeyTypes")); + } + + @Test + public void testPubKeyAcceptedKeyTypes() throws Exception { + config("Host=orcz\n\tPubkeyAcceptedKeyTypes ^ssh-rsa"); + Host h = osc.lookup("orcz"); + Config c = h.getConfig(); + assertEquals("^ssh-rsa", + c.getValue(SshConstants.PUBKEY_ACCEPTED_ALGORITHMS)); + assertEquals("^ssh-rsa", c.getValue("PubkeyAcceptedKeyTypes")); + } + + @Test + public void testEolComments() throws Exception { + config("#Comment\nHost=orcz #Comment\n\tPubkeyAcceptedAlgorithms ^ssh-rsa # Comment\n#Comment"); + Host h = osc.lookup("orcz"); + assertNotNull(h); + Config c = h.getConfig(); + assertEquals("^ssh-rsa", + c.getValue(SshConstants.PUBKEY_ACCEPTED_ALGORITHMS)); + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFile.java index 98c63cdcdd..c514270f5b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFile.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFile.java @@ -23,7 +23,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; -import java.util.Locale; import java.util.Map; import java.util.Set; import java.util.TreeMap; @@ -224,8 +223,17 @@ public class OpenSshConfigFile implements SshConfigStore { entries.put(DEFAULT_NAME, defaults); while ((line = reader.readLine()) != null) { + // OpenSsh ignores trailing comments on a line. Anything after the + // first # on a line is trimmed away (yes, even if the hash is + // inside quotes). + // + // See https://github.com/openssh/openssh-portable/commit/2bcbf679 + int i = line.indexOf('#'); + if (i >= 0) { + line = line.substring(0, i); + } line = line.trim(); - if (line.isEmpty() || line.startsWith("#")) { //$NON-NLS-1$ + if (line.isEmpty()) { continue; } String[] parts = line.split("[ \t]*[= \t]", 2); //$NON-NLS-1$ @@ -484,12 +492,30 @@ public class OpenSshConfigFile implements SshConfigStore { LIST_KEYS.add(SshConstants.USER_KNOWN_HOSTS_FILE); } + /** + * OpenSSH has renamed some config keys. This maps old names to new + * names. + */ + private static final Map ALIASES = new TreeMap<>( + String.CASE_INSENSITIVE_ORDER); + + static { + // See https://github.com/openssh/openssh-portable/commit/ee9c0da80 + ALIASES.put("PubkeyAcceptedKeyTypes", //$NON-NLS-1$ + SshConstants.PUBKEY_ACCEPTED_ALGORITHMS); + } + private Map options; private Map> multiOptions; private Map> listOptions; + private static String toKey(String key) { + String k = ALIASES.get(key); + return k != null ? k : key; + } + /** * Retrieves the value of a single-valued key, or the first if the key * has multiple values. Keys are case-insensitive, so @@ -501,15 +527,15 @@ public class OpenSshConfigFile implements SshConfigStore { */ @Override public String getValue(String key) { - String result = options != null ? options.get(key) : null; + String k = toKey(key); + String result = options != null ? options.get(k) : null; if (result == null) { // Let's be lenient and return at least the first value from // a list-valued or multi-valued key. - List values = listOptions != null ? listOptions.get(key) + List values = listOptions != null ? listOptions.get(k) : null; if (values == null) { - values = multiOptions != null ? multiOptions.get(key) - : null; + values = multiOptions != null ? multiOptions.get(k) : null; } if (values != null && !values.isEmpty()) { result = values.get(0); @@ -529,10 +555,11 @@ public class OpenSshConfigFile implements SshConfigStore { */ @Override public List getValues(String key) { - List values = listOptions != null ? listOptions.get(key) + String k = toKey(key); + List values = listOptions != null ? listOptions.get(k) : null; if (values == null) { - values = multiOptions != null ? multiOptions.get(key) : null; + values = multiOptions != null ? multiOptions.get(k) : null; } if (values == null || values.isEmpty()) { return new ArrayList<>(); @@ -551,34 +578,35 @@ public class OpenSshConfigFile implements SshConfigStore { * to set or add */ public void setValue(String key, String value) { + String k = toKey(key); if (value == null) { if (multiOptions != null) { - multiOptions.remove(key); + multiOptions.remove(k); } if (listOptions != null) { - listOptions.remove(key); + listOptions.remove(k); } if (options != null) { - options.remove(key); + options.remove(k); } return; } - if (MULTI_KEYS.contains(key)) { + if (MULTI_KEYS.contains(k)) { if (multiOptions == null) { multiOptions = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); } - List values = multiOptions.get(key); + List values = multiOptions.get(k); if (values == null) { values = new ArrayList<>(4); - multiOptions.put(key, values); + multiOptions.put(k, values); } values.add(value); } else { if (options == null) { options = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); } - if (!options.containsKey(key)) { - options.put(key, value); + if (!options.containsKey(k)) { + options.put(k, value); } } } @@ -595,20 +623,21 @@ public class OpenSshConfigFile implements SshConfigStore { if (values.isEmpty()) { return; } + String k = toKey(key); // Check multi-valued keys first; because of the replacement // strategy, they must take precedence over list-valued keys // which always follow the "first occurrence wins" strategy. // // Note that SendEnv is a multi-valued list-valued key. (It's // rather immaterial for JGit, though.) - if (MULTI_KEYS.contains(key)) { + if (MULTI_KEYS.contains(k)) { if (multiOptions == null) { multiOptions = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); } - List items = multiOptions.get(key); + List items = multiOptions.get(k); if (items == null) { items = new ArrayList<>(values); - multiOptions.put(key, items); + multiOptions.put(k, items); } else { items.addAll(values); } @@ -616,8 +645,8 @@ public class OpenSshConfigFile implements SshConfigStore { if (listOptions == null) { listOptions = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); } - if (!listOptions.containsKey(key)) { - listOptions.put(key, values); + if (!listOptions.containsKey(k)) { + listOptions.put(k, values); } } } @@ -630,7 +659,7 @@ public class OpenSshConfigFile implements SshConfigStore { * @return {@code true} if the key is a list-valued key. */ public static boolean isListKey(String key) { - return LIST_KEYS.contains(key.toUpperCase(Locale.ROOT)); + return LIST_KEYS.contains(toKey(key)); } void merge(HostEntry entry) { -- cgit v1.2.3 From f43cb3605c078d0932365c49163f5a8b799fe117 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Sat, 20 Mar 2021 11:15:20 +0100 Subject: Ensure post-commit hook is called after index lock was released Otherwise a post-commit hook cannot modify the index. Bug: 566934 Change-Id: I0093dccd93b2064f243544b516bdce198afdb18b Signed-off-by: Matthias Sohn --- .../org/eclipse/jgit/internal/JGitText.properties | 1 + .../src/org/eclipse/jgit/api/CommitCommand.java | 23 ++++++++++++++++------ .../src/org/eclipse/jgit/internal/JGitText.java | 1 + 3 files changed, 19 insertions(+), 6 deletions(-) (limited to 'org.eclipse.jgit/src/org/eclipse/jgit') 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 9695e5742b..d4bcd9a42b 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -529,6 +529,7 @@ peeledRefIsRequired=Peeled ref is required. peerDidNotSupplyACompleteObjectGraph=peer did not supply a complete object graph personIdentEmailNonNull=E-mail address of PersonIdent must not be null. personIdentNameNonNull=Name of PersonIdent must not be null. +postCommitHookFailed=Execution of post-commit hook failed: {0}. prefixRemote=remote: problemWithResolvingPushRefSpecsLocally=Problem with resolving push ref specs locally: {0} progressMonUploading=Uploading {0} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java index 31f6a31c75..51d5d382cb 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java @@ -67,6 +67,8 @@ import org.eclipse.jgit.treewalk.FileTreeIterator; import org.eclipse.jgit.treewalk.TreeWalk; import org.eclipse.jgit.treewalk.TreeWalk.OperationType; import org.eclipse.jgit.util.ChangeIdUtil; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * A class used to execute a {@code Commit} command. It has setters for all @@ -78,6 +80,9 @@ import org.eclipse.jgit.util.ChangeIdUtil; * >Git documentation about Commit */ public class CommitCommand extends GitCommand { + private static final Logger log = LoggerFactory + .getLogger(CommitCommand.class); + private PersonIdent author; private PersonIdent committer; @@ -212,6 +217,7 @@ public class CommitCommand extends GitCommand { .setCommitMessage(message).call(); } + RevCommit revCommit; // lock the index DirCache index = repo.lockDirCache(); try (ObjectInserter odi = repo.newObjectInserter()) { @@ -267,7 +273,7 @@ public class CommitCommand extends GitCommand { ObjectId commitId = odi.insert(commit); odi.flush(); - RevCommit revCommit = rw.parseCommit(commitId); + revCommit = rw.parseCommit(commitId); RefUpdate ru = repo.updateRef(Constants.HEAD); ru.setNewObjectId(commitId); if (!useDefaultReflogMessage) { @@ -302,11 +308,7 @@ public class CommitCommand extends GitCommand { repo.writeMergeCommitMsg(null); repo.writeRevertHead(null); } - Hooks.postCommit(repo, - hookOutRedirect.get(PostCommitHook.NAME), - hookErrRedirect.get(PostCommitHook.NAME)).call(); - - return revCommit; + break; } case REJECTED: case LOCK_FAILURE: @@ -320,6 +322,15 @@ public class CommitCommand extends GitCommand { } finally { index.unlock(); } + try { + Hooks.postCommit(repo, hookOutRedirect.get(PostCommitHook.NAME), + hookErrRedirect.get(PostCommitHook.NAME)).call(); + } catch (Exception e) { + log.error(MessageFormat.format( + JGitText.get().postCommitHookFailed, e.getMessage()), + e); + } + return revCommit; } catch (UnmergedPathException e) { throw new UnmergedPathsException(e); } catch (IOException e) { 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 95265feb4e..fe8c2d218e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -557,6 +557,7 @@ public class JGitText extends TranslationBundle { /***/ public String peerDidNotSupplyACompleteObjectGraph; /***/ public String personIdentEmailNonNull; /***/ public String personIdentNameNonNull; + /***/ public String postCommitHookFailed; /***/ public String prefixRemote; /***/ public String problemWithResolvingPushRefSpecsLocally; /***/ public String progressMonUploading; -- cgit v1.2.3 From b08c599fb8f7eae831e3b0fd1f9cfd907db4b098 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Sat, 20 Mar 2021 11:19:07 +0100 Subject: CommitCommand: remove unncessary comment Let the code speak for itself. Change-Id: I6a6d6c327ffac23fc607295a7f4fd3131b3d1e58 Signed-off-by: Matthias Sohn --- org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java | 1 - 1 file changed, 1 deletion(-) (limited to 'org.eclipse.jgit/src/org/eclipse/jgit') diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java index 51d5d382cb..4291968b4f 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java @@ -218,7 +218,6 @@ public class CommitCommand extends GitCommand { } RevCommit revCommit; - // lock the index DirCache index = repo.lockDirCache(); try (ObjectInserter odi = repo.newObjectInserter()) { if (!only.isEmpty()) -- cgit v1.2.3 From 18c735c474579d013d7e8deb6570b51ec26af087 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Sat, 20 Mar 2021 11:20:52 +0100 Subject: CommitCommand: fix formatting Change-Id: I5efd1ffee4ebb08b3b5c27e29162493615727840 Signed-off-by: Matthias Sohn --- org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'org.eclipse.jgit/src/org/eclipse/jgit') diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java index 4291968b4f..259a3d2696 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java @@ -178,8 +178,7 @@ public class CommitCommand extends GitCommand { if (all && !repo.isBare()) { try (Git git = new Git(repo)) { - git.add() - .addFilepattern(".") //$NON-NLS-1$ + git.add().addFilepattern(".") //$NON-NLS-1$ .setUpdate(true).call(); } catch (NoFilepatternException e) { // should really not happen -- cgit v1.2.3 From 502bfff7db5c0d91d9c7062fda7a0974df60591a Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Sat, 20 Mar 2021 11:35:27 +0100 Subject: Refactor CommitCommand to improve readability Change-Id: Id3cac81cd32c07f677b7f669d58e32b5290e1790 Signed-off-by: Matthias Sohn --- .../src/org/eclipse/jgit/api/CommitCommand.java | 162 ++++++++++++--------- 1 file changed, 90 insertions(+), 72 deletions(-) (limited to 'org.eclipse.jgit/src/org/eclipse/jgit') diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java index 259a3d2696..7ec36af714 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java @@ -20,6 +20,7 @@ import java.util.LinkedList; import java.util.List; import org.eclipse.jgit.api.errors.AbortedByHookException; +import org.eclipse.jgit.api.errors.CanceledException; import org.eclipse.jgit.api.errors.ConcurrentRefUpdateException; import org.eclipse.jgit.api.errors.EmptyCommitException; import org.eclipse.jgit.api.errors.GitAPIException; @@ -36,6 +37,8 @@ import org.eclipse.jgit.dircache.DirCacheBuildIterator; import org.eclipse.jgit.dircache.DirCacheBuilder; import org.eclipse.jgit.dircache.DirCacheEntry; import org.eclipse.jgit.dircache.DirCacheIterator; +import org.eclipse.jgit.errors.IncorrectObjectTypeException; +import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.errors.UnmergedPathException; import org.eclipse.jgit.hooks.CommitMsgHook; import org.eclipse.jgit.hooks.Hooks; @@ -230,93 +233,25 @@ public class CommitCommand extends GitCommand { if (insertChangeId) insertChangeId(indexTreeId); - // Check for empty commits - if (headId != null && !allowEmpty.booleanValue()) { - RevCommit headCommit = rw.parseCommit(headId); - headCommit.getTree(); - if (indexTreeId.equals(headCommit.getTree())) { - throw new EmptyCommitException( - JGitText.get().emptyCommit); - } - } + checkIfEmpty(rw, headId, indexTreeId); // Create a Commit object, populate it and write it CommitBuilder commit = new CommitBuilder(); commit.setCommitter(committer); commit.setAuthor(author); commit.setMessage(message); - commit.setParentIds(parents); commit.setTreeId(indexTreeId); if (signCommit.booleanValue()) { - if (gpgSigner == null) { - throw new ServiceUnavailableException( - JGitText.get().signingServiceUnavailable); - } - if (gpgSigner instanceof GpgObjectSigner) { - ((GpgObjectSigner) gpgSigner).signObject(commit, - signingKey, committer, credentialsProvider, - gpgConfig); - } else { - if (gpgConfig.getKeyFormat() != GpgFormat.OPENPGP) { - throw new UnsupportedSigningFormatException(JGitText - .get().onlyOpenPgpSupportedForSigning); - } - gpgSigner.sign(commit, signingKey, committer, - credentialsProvider); - } + sign(commit); } ObjectId commitId = odi.insert(commit); odi.flush(); - revCommit = rw.parseCommit(commitId); - RefUpdate ru = repo.updateRef(Constants.HEAD); - ru.setNewObjectId(commitId); - if (!useDefaultReflogMessage) { - ru.setRefLogMessage(reflogComment, false); - } else { - String prefix = amend ? "commit (amend): " //$NON-NLS-1$ - : parents.isEmpty() ? "commit (initial): " //$NON-NLS-1$ - : "commit: "; //$NON-NLS-1$ - ru.setRefLogMessage(prefix + revCommit.getShortMessage(), - false); - } - if (headId != null) - ru.setExpectedOldObjectId(headId); - else - ru.setExpectedOldObjectId(ObjectId.zeroId()); - Result rc = ru.forceUpdate(); - switch (rc) { - case NEW: - case FORCED: - case FAST_FORWARD: { - setCallable(false); - if (state == RepositoryState.MERGING_RESOLVED - || isMergeDuringRebase(state)) { - // Commit was successful. Now delete the files - // used for merge commits - repo.writeMergeCommitMsg(null); - repo.writeMergeHeads(null); - } else if (state == RepositoryState.CHERRY_PICKING_RESOLVED) { - repo.writeMergeCommitMsg(null); - repo.writeCherryPickHead(null); - } else if (state == RepositoryState.REVERTING_RESOLVED) { - repo.writeMergeCommitMsg(null); - repo.writeRevertHead(null); - } - break; - } - case REJECTED: - case LOCK_FAILURE: - throw new ConcurrentRefUpdateException( - JGitText.get().couldNotLockHEAD, ru.getRef(), rc); - default: - throw new JGitInternalException(MessageFormat.format( - JGitText.get().updatingRefFailed, Constants.HEAD, - commitId.toString(), rc)); - } + + updateRef(state, headId, revCommit, commitId); } finally { index.unlock(); } @@ -337,6 +272,89 @@ public class CommitCommand extends GitCommand { } } + private void checkIfEmpty(RevWalk rw, ObjectId headId, ObjectId indexTreeId) + throws EmptyCommitException, MissingObjectException, + IncorrectObjectTypeException, IOException { + if (headId != null && !allowEmpty.booleanValue()) { + RevCommit headCommit = rw.parseCommit(headId); + headCommit.getTree(); + if (indexTreeId.equals(headCommit.getTree())) { + throw new EmptyCommitException(JGitText.get().emptyCommit); + } + } + } + + private void sign(CommitBuilder commit) throws ServiceUnavailableException, + CanceledException, UnsupportedSigningFormatException { + if (gpgSigner == null) { + throw new ServiceUnavailableException( + JGitText.get().signingServiceUnavailable); + } + if (gpgSigner instanceof GpgObjectSigner) { + ((GpgObjectSigner) gpgSigner).signObject(commit, + signingKey, committer, credentialsProvider, + gpgConfig); + } else { + if (gpgConfig.getKeyFormat() != GpgFormat.OPENPGP) { + throw new UnsupportedSigningFormatException(JGitText + .get().onlyOpenPgpSupportedForSigning); + } + gpgSigner.sign(commit, signingKey, committer, + credentialsProvider); + } + } + + private void updateRef(RepositoryState state, ObjectId headId, + RevCommit revCommit, ObjectId commitId) + throws ConcurrentRefUpdateException, IOException { + RefUpdate ru = repo.updateRef(Constants.HEAD); + ru.setNewObjectId(commitId); + if (!useDefaultReflogMessage) { + ru.setRefLogMessage(reflogComment, false); + } else { + String prefix = amend ? "commit (amend): " //$NON-NLS-1$ + : parents.isEmpty() ? "commit (initial): " //$NON-NLS-1$ + : "commit: "; //$NON-NLS-1$ + ru.setRefLogMessage(prefix + revCommit.getShortMessage(), + false); + } + if (headId != null) { + ru.setExpectedOldObjectId(headId); + } else { + ru.setExpectedOldObjectId(ObjectId.zeroId()); + } + Result rc = ru.forceUpdate(); + switch (rc) { + case NEW: + case FORCED: + case FAST_FORWARD: { + setCallable(false); + if (state == RepositoryState.MERGING_RESOLVED + || isMergeDuringRebase(state)) { + // Commit was successful. Now delete the files + // used for merge commits + repo.writeMergeCommitMsg(null); + repo.writeMergeHeads(null); + } else if (state == RepositoryState.CHERRY_PICKING_RESOLVED) { + repo.writeMergeCommitMsg(null); + repo.writeCherryPickHead(null); + } else if (state == RepositoryState.REVERTING_RESOLVED) { + repo.writeMergeCommitMsg(null); + repo.writeRevertHead(null); + } + break; + } + case REJECTED: + case LOCK_FAILURE: + throw new ConcurrentRefUpdateException( + JGitText.get().couldNotLockHEAD, ru.getRef(), rc); + default: + throw new JGitInternalException(MessageFormat.format( + JGitText.get().updatingRefFailed, Constants.HEAD, + commitId.toString(), rc)); + } + } + private void insertChangeId(ObjectId treeId) { ObjectId firstParentId = null; if (!parents.isEmpty()) -- cgit v1.2.3 From 1de2a9fbe73930504b72904539a3d98d9f9c9738 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Sat, 20 Mar 2021 18:46:13 +0100 Subject: ssh config: do environment variable replacement OpenSSH 8.4 has introduced simple environment variable substitution for some keys. Implement that feature in our ssh config file parser, too. Bug: 572103 Change-Id: I360f2c5510eea4ec3329aeedf3d29dfefc9163f0 Signed-off-by: Thomas Wolf --- .../eclipse/jgit/transport/OpenSshConfigTest.java | 20 +++++ .../internal/transport/ssh/OpenSshConfigFile.java | 85 ++++++++++++++-------- 2 files changed, 76 insertions(+), 29 deletions(-) (limited to 'org.eclipse.jgit/src/org/eclipse/jgit') diff --git a/org.eclipse.jgit.ssh.jsch.test/tst/org/eclipse/jgit/transport/OpenSshConfigTest.java b/org.eclipse.jgit.ssh.jsch.test/tst/org/eclipse/jgit/transport/OpenSshConfigTest.java index 4c7e99ea80..4be2271a8c 100644 --- a/org.eclipse.jgit.ssh.jsch.test/tst/org/eclipse/jgit/transport/OpenSshConfigTest.java +++ b/org.eclipse.jgit.ssh.jsch.test/tst/org/eclipse/jgit/transport/OpenSshConfigTest.java @@ -58,6 +58,7 @@ public class OpenSshConfigTest extends RepositoryTestCase { FileUtils.mkdir(configFile.getParentFile()); mockSystemReader.setProperty(Constants.OS_USER_NAME_KEY, "jex_junit"); + mockSystemReader.setProperty("TST_VAR", "TEST"); osc = new OpenSshConfig(home, configFile); } @@ -497,4 +498,23 @@ public class OpenSshConfigTest extends RepositoryTestCase { assertEquals("^ssh-rsa", c.getValue(SshConstants.PUBKEY_ACCEPTED_ALGORITHMS)); } + + @Test + public void testEnVarSubstitution() throws Exception { + config("Host orcz\nIdentityFile /tmp/${TST_VAR}\n" + + "CertificateFile /tmp/${}/foo\nUser ${TST_VAR}\nIdentityAgent /tmp/${TST_VAR/bar"); + Host h = osc.lookup("orcz"); + assertNotNull(h); + Config c = h.getConfig(); + assertEquals("/tmp/TEST", + c.getValue(SshConstants.IDENTITY_FILE)); + // No variable name + assertEquals("/tmp/${}/foo", c.getValue(SshConstants.CERTIFICATE_FILE)); + // User doesn't get env var substitution: + assertEquals("${TST_VAR}", c.getValue(SshConstants.USER)); + assertEquals("${TST_VAR}", h.getUser()); + // Unterminated: + assertEquals("/tmp/${TST_VAR/bar", + c.getValue(SshConstants.IDENTITY_AGENT)); + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFile.java index c514270f5b..de6a346cb2 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFile.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFile.java @@ -708,10 +708,10 @@ public class OpenSshConfigFile implements SshConfigStore { } private List substitute(List values, String allowed, - Replacer r) { + Replacer r, boolean withEnv) { List result = new ArrayList<>(values.size()); for (String value : values) { - result.add(r.substitute(value, allowed)); + result.add(r.substitute(value, allowed, withEnv)); } return result; } @@ -743,7 +743,7 @@ public class OpenSshConfigFile implements SshConfigStore { if (hostName == null || hostName.isEmpty()) { options.put(SshConstants.HOST_NAME, originalHostName); } else { - hostName = r.substitute(hostName, "h"); //$NON-NLS-1$ + hostName = r.substitute(hostName, "h", false); //$NON-NLS-1$ options.put(SshConstants.HOST_NAME, hostName); r.update('h', hostName); } @@ -752,13 +752,13 @@ public class OpenSshConfigFile implements SshConfigStore { List values = multiOptions .get(SshConstants.IDENTITY_FILE); if (values != null) { - values = substitute(values, "dhlru", r); //$NON-NLS-1$ + values = substitute(values, "dhlru", r, true); //$NON-NLS-1$ values = replaceTilde(values, home); multiOptions.put(SshConstants.IDENTITY_FILE, values); } values = multiOptions.get(SshConstants.CERTIFICATE_FILE); if (values != null) { - values = substitute(values, "dhlru", r); //$NON-NLS-1$ + values = substitute(values, "dhlru", r, true); //$NON-NLS-1$ values = replaceTilde(values, home); multiOptions.put(SshConstants.CERTIFICATE_FILE, values); } @@ -775,29 +775,29 @@ public class OpenSshConfigFile implements SshConfigStore { // HOSTNAME already done above String value = options.get(SshConstants.IDENTITY_AGENT); if (value != null) { - value = r.substitute(value, "dhlru"); //$NON-NLS-1$ + value = r.substitute(value, "dhlru", true); //$NON-NLS-1$ value = toFile(value, home).getPath(); options.put(SshConstants.IDENTITY_AGENT, value); } value = options.get(SshConstants.CONTROL_PATH); if (value != null) { - value = r.substitute(value, "ChLlnpru"); //$NON-NLS-1$ + value = r.substitute(value, "ChLlnpru", true); //$NON-NLS-1$ value = toFile(value, home).getPath(); options.put(SshConstants.CONTROL_PATH, value); } value = options.get(SshConstants.LOCAL_COMMAND); if (value != null) { - value = r.substitute(value, "CdhlnprTu"); //$NON-NLS-1$ + value = r.substitute(value, "CdhlnprTu", false); //$NON-NLS-1$ options.put(SshConstants.LOCAL_COMMAND, value); } value = options.get(SshConstants.REMOTE_COMMAND); if (value != null) { - value = r.substitute(value, "Cdhlnpru"); //$NON-NLS-1$ + value = r.substitute(value, "Cdhlnpru", false); //$NON-NLS-1$ options.put(SshConstants.REMOTE_COMMAND, value); } value = options.get(SshConstants.PROXY_COMMAND); if (value != null) { - value = r.substitute(value, "hpr"); //$NON-NLS-1$ + value = r.substitute(value, "hpr", false); //$NON-NLS-1$ options.put(SshConstants.PROXY_COMMAND, value); } } @@ -871,7 +871,7 @@ public class OpenSshConfigFile implements SshConfigStore { replacements.put(Character.valueOf('r'), user == null ? "" : user); //$NON-NLS-1$ replacements.put(Character.valueOf('u'), localUserName); replacements.put(Character.valueOf('C'), - substitute("%l%h%p%r", "hlpr")); //$NON-NLS-1$ //$NON-NLS-2$ + substitute("%l%h%p%r", "hlpr", false)); //$NON-NLS-1$ //$NON-NLS-2$ replacements.put(Character.valueOf('T'), "NONE"); //$NON-NLS-1$ } @@ -879,36 +879,63 @@ public class OpenSshConfigFile implements SshConfigStore { replacements.put(Character.valueOf(key), value); if ("lhpr".indexOf(key) >= 0) { //$NON-NLS-1$ replacements.put(Character.valueOf('C'), - substitute("%l%h%p%r", "hlpr")); //$NON-NLS-1$ //$NON-NLS-2$ + substitute("%l%h%p%r", "hlpr", false)); //$NON-NLS-1$ //$NON-NLS-2$ } } - public String substitute(String input, String allowed) { + public String substitute(String input, String allowed, + boolean withEnv) { if (input == null || input.length() <= 1 - || input.indexOf('%') < 0) { + || input.indexOf('%') < 0 + && (!withEnv || input.indexOf("${") < 0)) { //$NON-NLS-1$ return input; } StringBuilder builder = new StringBuilder(); int start = 0; int length = input.length(); while (start < length) { - int percent = input.indexOf('%', start); - if (percent < 0 || percent + 1 >= length) { - builder.append(input.substring(start)); + char ch = input.charAt(start); + switch (ch) { + case '%': + if (start + 1 >= length) { + break; + } + String replacement = null; + ch = input.charAt(start + 1); + if (ch == '%' || allowed.indexOf(ch) >= 0) { + replacement = replacements.get(Character.valueOf(ch)); + } + if (replacement == null) { + builder.append('%').append(ch); + } else { + builder.append(replacement); + } + start += 2; + continue; + case '$': + if (!withEnv || start + 2 >= length) { + break; + } + ch = input.charAt(start + 1); + if (ch == '{') { + int close = input.indexOf('}', start + 2); + if (close > start + 2) { + String variable = SystemReader.getInstance() + .getenv(input.substring(start + 2, close)); + if (!StringUtils.isEmptyOrNull(variable)) { + builder.append(variable); + } + start = close + 1; + continue; + } + } + ch = '$'; + break; + default: break; } - String replacement = null; - char ch = input.charAt(percent + 1); - if (ch == '%' || allowed.indexOf(ch) >= 0) { - replacement = replacements.get(Character.valueOf(ch)); - } - if (replacement == null) { - builder.append(input.substring(start, percent + 2)); - } else { - builder.append(input.substring(start, percent)) - .append(replacement); - } - start = percent + 2; + builder.append(ch); + start++; } return builder.toString(); } -- cgit v1.2.3 From 0c91bf4e174013f0039f39349e8f83ff0d2e51c3 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Sat, 20 Mar 2021 18:54:17 +0100 Subject: Allow info messages in UsernamePasswordCredentialsProvider o.e.j.ssh.apache produces passphrase prompts containing InformationalMessage items to show the fingerprint of the key the passphrase is being asked for. Allow this so that the credentials provider can be used with o.e.j.ssh.apache. Change-Id: Ibc2ffd3a987d3118952726091b9b80442972dfd8 Signed-off-by: Thomas Wolf --- .../UsernamePasswordCredentialsProvider.java | 24 +++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) (limited to 'org.eclipse.jgit/src/org/eclipse/jgit') diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UsernamePasswordCredentialsProvider.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UsernamePasswordCredentialsProvider.java index 979961f2ae..c0de42cb57 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UsernamePasswordCredentialsProvider.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UsernamePasswordCredentialsProvider.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010, Google Inc. and others + * Copyright (C) 2010, 2021 Google Inc. and others * * This program and the accompanying materials are made available under the * terms of the Eclipse Distribution License v. 1.0 which is available at @@ -58,14 +58,21 @@ public class UsernamePasswordCredentialsProvider extends CredentialsProvider { @Override public boolean supports(CredentialItem... items) { for (CredentialItem i : items) { - if (i instanceof CredentialItem.Username) + if (i instanceof CredentialItem.InformationalMessage) { continue; - - else if (i instanceof CredentialItem.Password) + } + if (i instanceof CredentialItem.Username) { continue; - - else - return false; + } + if (i instanceof CredentialItem.Password) { + continue; + } + if (i instanceof CredentialItem.StringType) { + if (i.getPromptText().equals("Password: ")) { //$NON-NLS-1$ + continue; + } + } + return false; } return true; } @@ -75,6 +82,9 @@ public class UsernamePasswordCredentialsProvider extends CredentialsProvider { public boolean get(URIish uri, CredentialItem... items) throws UnsupportedCredentialItem { for (CredentialItem i : items) { + if (i instanceof CredentialItem.InformationalMessage) { + continue; + } if (i instanceof CredentialItem.Username) { ((CredentialItem.Username) i).setValue(username); continue; -- cgit v1.2.3 From 7ceb61494bba8e3de16c26d1b3c36d4d74fb4975 Mon Sep 17 00:00:00 2001 From: Marija Savtchouk Date: Thu, 1 Apr 2021 15:52:26 +0100 Subject: Allow file mode conflicts in virtual base commit on recursive merge. Similar to https://git.eclipse.org/r/c/jgit/jgit/+/175166, ignore path that have conflicts on attributes, so that the virtual base could be used by RecursiveMerger. Change-Id: I99c95445a305558d55bbb9c9e97446caaf61c154 Signed-off-by: Marija Savtchouk --- .../tst/org/eclipse/jgit/merge/MergerTest.java | 76 ++++++++++++++++++++++ .../src/org/eclipse/jgit/merge/ResolveMerger.java | 21 +++--- 2 files changed, 88 insertions(+), 9 deletions(-) (limited to 'org.eclipse.jgit/src/org/eclipse/jgit') diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/MergerTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/MergerTest.java index eecf25be90..6cbb4a89b2 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/MergerTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/MergerTest.java @@ -1648,6 +1648,82 @@ public class MergerTest extends RepositoryTestCase { indexState(CONTENT)); } + /** + * Merging two commits when files have equal content, but conflicting file mode + * in the virtual ancestor. + * + *

+ * This test has the same set up as + * {@code checkFileDirMergeConflictInVirtualAncestor_NoConflictInChildren}, only + * with the mode conflict in A1 and A2. + */ + @Theory + public void checkModeMergeConflictInVirtualAncestor(MergeStrategy strategy) throws Exception { + if (!strategy.equals(MergeStrategy.RECURSIVE)) { + return; + } + + Git git = Git.wrap(db); + + // master + writeTrashFile("c", "initial file"); + git.add().addFilepattern("c").call(); + RevCommit commitI = git.commit().setMessage("Initial commit").call(); + + File a = writeTrashFile("a", "content in Ancestor"); + git.add().addFilepattern("a").call(); + RevCommit commitA1 = git.commit().setMessage("Ancestor 1").call(); + + a = writeTrashFile("a", "content in Child 1 (commited on master)"); + git.add().addFilepattern("a").call(); + // commit C1M + git.commit().setMessage("Child 1 on master").call(); + + git.checkout().setCreateBranch(true).setStartPoint(commitI).setName("branch-to-merge").call(); + // "a" becomes executable in A2 + a = writeTrashFile("a", "content in Ancestor"); + a.setExecutable(true); + git.add().addFilepattern("a").call(); + RevCommit commitA2 = git.commit().setMessage("Ancestor 2").call(); + + // second branch + git.checkout().setCreateBranch(true).setStartPoint(commitA1).setName("second-branch").call(); + a = writeTrashFile("a", "content in Child 2 (commited on second-branch)"); + git.add().addFilepattern("a").call(); + // commit C2S + git.commit().setMessage("Child 2 on second-branch").call(); + + // Merge branch-to-merge into second-branch + MergeResult mergeResult = git.merge().include(commitA2).setStrategy(strategy).call(); + assertEquals(mergeResult.getNewHead(), null); + assertEquals(mergeResult.getMergeStatus(), MergeStatus.CONFLICTING); + // Resolve the conflict manually, merge "a" as non-executable + a = writeTrashFile("a", "merge conflict resolution"); + a.setExecutable(false); + git.add().addFilepattern("a").call(); + RevCommit commitC3S = git.commit().setMessage("Child 3 on second bug - resolve merge conflict").call(); + + // Merge branch-to-merge into master + git.checkout().setName("master").call(); + mergeResult = git.merge().include(commitA2).setStrategy(strategy).call(); + assertEquals(mergeResult.getNewHead(), null); + assertEquals(mergeResult.getMergeStatus(), MergeStatus.CONFLICTING); + + // Resolve the conflict manually - merge "a" as non-executable + a = writeTrashFile("a", "merge conflict resolution"); + a.setExecutable(false); + git.add().addFilepattern("a").call(); + // commit C4M + git.commit().setMessage("Child 4 on master - resolve merge conflict").call(); + + // Merge C4M (second-branch) into master (C3S) + // Conflict in virtual base should be here, but there are no conflicts in + // children + mergeResult = git.merge().include(commitC3S).call(); + assertEquals(mergeResult.getMergeStatus(), MergeStatus.MERGED); + + } + private void writeSubmodule(String path, ObjectId commit) throws IOException, ConfigInvalidException { addSubmoduleToIndex(path, commit); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java index 4bfb38d286..b011258981 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java @@ -644,15 +644,18 @@ public class ResolveMerger extends ThreeWayMerger { } return true; } - // FileModes are not mergeable. We found a conflict on modes. - // For conflicting entries we don't know lastModified and - // length. - add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, EPOCH, 0); - add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, EPOCH, 0); - add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, EPOCH, 0); - unmergedPaths.add(tw.getPathString()); - mergeResults.put(tw.getPathString(), - new MergeResult<>(Collections. emptyList())); + if (!ignoreConflicts) { + // FileModes are not mergeable. We found a conflict on modes. + // For conflicting entries we don't know lastModified and + // length. + // This path can be skipped on ignoreConflicts, so the caller + // could use virtual commit. + add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, EPOCH, 0); + add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, EPOCH, 0); + add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, EPOCH, 0); + unmergedPaths.add(tw.getPathString()); + mergeResults.put(tw.getPathString(), new MergeResult<>(Collections.emptyList())); + } return true; } -- cgit v1.2.3 From 8210f29fe43ccd35e7d2ed3ed45a84a75b2717c4 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Mon, 12 Apr 2021 23:50:54 +0200 Subject: Implement ours/theirs content conflict resolution Git has different conflict resolution strategies: * There is a tree merge strategy "ours" which just ignores any changes from theirs ("-s ours"). JGit also has the mirror strategy "theirs" ignoring any changes from "ours". (This doesn't exist in C git.) Adapt StashApplyCommand and CherrypickCommand to be able to use those tree merge strategies. * For the resolve/recursive tree merge strategies, there are content conflict resolution strategies "ours" and "theirs", which resolve any conflict hunks by taking the "ours" or "theirs" hunk. In C git those correspond to "-Xours" or -Xtheirs". Implement that in MergeAlgorithm, and add API to set and pass through such a strategy for resolving content conflicts. * The "ours/theirs" content conflict resolution strategies also apply for binary files. Handle these cases in ResolveMerger. Note that the content conflict resolution strategies ("-X ours/theirs") do _not_ apply to modify/delete or delete/modify conflicts. Such conflicts are always reported as conflicts by C git. They do apply, however, if one side completely clears a file's content. Bug: 501111 Change-Id: I2c9c170c61c440a2ab9c387991e7a0c3ab960e07 Signed-off-by: Thomas Wolf Signed-off-by: Matthias Sohn --- .../eclipse/jgit/pgm/internal/CLIText.properties | 3 + .../src/org/eclipse/jgit/pgm/Merge.java | 22 +- .../src/org/eclipse/jgit/pgm/internal/CLIText.java | 1 + .../eclipse/jgit/api/CherryPickCommandTest.java | 93 ++++- .../tst/org/eclipse/jgit/api/MergeCommandTest.java | 376 ++++++++++++++++++++- .../tst/org/eclipse/jgit/api/PullCommandTest.java | 71 ++++ .../eclipse/jgit/api/StashApplyCommandTest.java | 133 +++++++- .../org/eclipse/jgit/api/CherryPickCommand.java | 70 +++- .../src/org/eclipse/jgit/api/MergeCommand.java | 24 +- .../src/org/eclipse/jgit/api/PullCommand.java | 34 +- .../src/org/eclipse/jgit/api/RebaseCommand.java | 34 +- .../org/eclipse/jgit/api/StashApplyCommand.java | 89 +++-- .../eclipse/jgit/merge/ContentMergeStrategy.java | 27 ++ .../src/org/eclipse/jgit/merge/MergeAlgorithm.java | 106 ++++-- .../src/org/eclipse/jgit/merge/ResolveMerger.java | 116 +++++-- 15 files changed, 1085 insertions(+), 114 deletions(-) create mode 100644 org.eclipse.jgit/src/org/eclipse/jgit/merge/ContentMergeStrategy.java (limited to 'org.eclipse.jgit/src/org/eclipse/jgit') 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 83846ee8e9..38deab99a0 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 @@ -115,6 +115,7 @@ metaVar_configFile=FILE metaVar_connProp=conn.prop metaVar_diffAlg=ALGORITHM metaVar_directory=DIRECTORY +metaVar_extraArgument=ours|theirs metaVar_file=FILE metaVar_filepattern=filepattern metaVar_gitDir=GIT_DIR @@ -217,6 +218,7 @@ timeInMilliSeconds={0} ms treeIsRequired=argument tree is required tooManyRefsGiven=Too many refs given unknownIoErrorStdout=An unknown I/O error occurred on standard output +unknownExtraArgument=unknown extra argument -X {0} specified unknownMergeStrategy=unknown merge strategy {0} specified unknownSubcommand=Unknown subcommand: {0} unmergedPaths=Unmerged paths: @@ -226,6 +228,7 @@ updating=Updating {0}..{1} usage_Aggressive=This option will cause gc to more aggressively optimize the repository at the expense of taking much more time usage_AlwaysFallback=Show uniquely abbreviated commit object as fallback usage_bareClone=Make a bare Git repository. That is, instead of creating [DIRECTORY] and placing the administrative files in [DIRECTORY]/.git, make the [DIRECTORY] itself the $GIT_DIR. +usage_extraArgument=Pass an extra argument to a merge driver. Currently supported are "-X ours" and "-X theirs". usage_mirrorClone=Set up a mirror of the source repository. This implies --bare. Compared to --bare, --mirror not only maps \ local branches of the source to local branches of the target, it maps all refs (including remote-tracking branches, notes etc.) \ and sets up a refspec configuration such that all these refs are overwritten by a git remote update in the target repository. diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Merge.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Merge.java index fdc449e063..ca4877fb34 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Merge.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Merge.java @@ -24,6 +24,7 @@ import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.merge.ContentMergeStrategy; import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.merge.ResolveMerger.MergeFailureReason; import org.eclipse.jgit.pgm.internal.CLIText; @@ -69,6 +70,20 @@ class Merge extends TextBuiltin { @Option(name = "-m", usage = "usage_message") private String message; + private ContentMergeStrategy contentStrategy = null; + + @Option(name = "--strategy-option", aliases = { "-X" }, + metaVar = "metaVar_extraArgument", usage = "usage_extraArgument") + void extraArg(String name) { + if (ContentMergeStrategy.OURS.name().equalsIgnoreCase(name)) { + contentStrategy = ContentMergeStrategy.OURS; + } else if (ContentMergeStrategy.THEIRS.name().equalsIgnoreCase(name)) { + contentStrategy = ContentMergeStrategy.THEIRS; + } else { + throw die(MessageFormat.format(CLIText.get().unknownExtraArgument, name)); + } + } + /** {@inheritDoc} */ @Override protected void run() { @@ -96,8 +111,11 @@ class Merge extends TextBuiltin { Ref oldHead = getOldHead(); MergeResult result; try (Git git = new Git(db)) { - MergeCommand mergeCmd = git.merge().setStrategy(mergeStrategy) - .setSquash(squash).setFastForward(ff) + MergeCommand mergeCmd = git.merge() + .setStrategy(mergeStrategy) + .setContentMergeStrategy(contentStrategy) + .setSquash(squash) + .setFastForward(ff) .setCommit(!noCommit); if (srcRef != null) { mergeCmd.include(srcRef); 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 991b3ba58a..8e49a76a33 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 @@ -284,6 +284,7 @@ public class CLIText extends TranslationBundle { /***/ public String tooManyRefsGiven; /***/ public String treeIsRequired; /***/ public char[] unknownIoErrorStdout; + /***/ public String unknownExtraArgument; /***/ public String unknownMergeStrategy; /***/ public String unknownSubcommand; /***/ public String unmergedPaths; diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CherryPickCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CherryPickCommandTest.java index 9dd129c335..f4f0ecd689 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CherryPickCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CherryPickCommandTest.java @@ -34,6 +34,8 @@ import org.eclipse.jgit.lib.FileMode; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ReflogReader; import org.eclipse.jgit.lib.RepositoryState; +import org.eclipse.jgit.merge.ContentMergeStrategy; +import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.merge.ResolveMerger.MergeFailureReason; import org.eclipse.jgit.revwalk.RevCommit; import org.junit.Test; @@ -193,7 +195,7 @@ public class CherryPickCommandTest extends RepositoryTestCase { } @Test - public void testCherryPickConflictResolutionNoCOmmit() throws Exception { + public void testCherryPickConflictResolutionNoCommit() throws Exception { Git git = new Git(db); RevCommit sideCommit = prepareCherryPick(git); @@ -279,6 +281,70 @@ public class CherryPickCommandTest extends RepositoryTestCase { } } + @Test + public void testCherryPickOurs() throws Exception { + try (Git git = new Git(db)) { + RevCommit sideCommit = prepareCherryPick(git); + + CherryPickResult result = git.cherryPick() + .include(sideCommit.getId()) + .setStrategy(MergeStrategy.OURS) + .call(); + assertEquals(CherryPickStatus.OK, result.getStatus()); + + String expected = "a(master)"; + checkFile(new File(db.getWorkTree(), "a"), expected); + } + } + + @Test + public void testCherryPickTheirs() throws Exception { + try (Git git = new Git(db)) { + RevCommit sideCommit = prepareCherryPick(git); + + CherryPickResult result = git.cherryPick() + .include(sideCommit.getId()) + .setStrategy(MergeStrategy.THEIRS) + .call(); + assertEquals(CherryPickStatus.OK, result.getStatus()); + + String expected = "a(side)"; + checkFile(new File(db.getWorkTree(), "a"), expected); + } + } + + @Test + public void testCherryPickXours() throws Exception { + try (Git git = new Git(db)) { + RevCommit sideCommit = prepareCherryPickStrategyOption(git); + + CherryPickResult result = git.cherryPick() + .include(sideCommit.getId()) + .setContentMergeStrategy(ContentMergeStrategy.OURS) + .call(); + assertEquals(CherryPickStatus.OK, result.getStatus()); + + String expected = "a\nmaster\nc\nd\n"; + checkFile(new File(db.getWorkTree(), "a"), expected); + } + } + + @Test + public void testCherryPickXtheirs() throws Exception { + try (Git git = new Git(db)) { + RevCommit sideCommit = prepareCherryPickStrategyOption(git); + + CherryPickResult result = git.cherryPick() + .include(sideCommit.getId()) + .setContentMergeStrategy(ContentMergeStrategy.THEIRS) + .call(); + assertEquals(CherryPickStatus.OK, result.getStatus()); + + String expected = "a\nside\nc\nd\n"; + checkFile(new File(db.getWorkTree(), "a"), expected); + } + } + @Test public void testCherryPickConflictMarkers() throws Exception { try (Git git = new Git(db)) { @@ -384,6 +450,31 @@ public class CherryPickCommandTest extends RepositoryTestCase { return sideCommit; } + private RevCommit prepareCherryPickStrategyOption(Git git) + throws Exception { + // create, add and commit file a + writeTrashFile("a", "a\nb\nc\n"); + git.add().addFilepattern("a").call(); + RevCommit firstMasterCommit = git.commit().setMessage("first master") + .call(); + + // create and checkout side branch + createBranch(firstMasterCommit, "refs/heads/side"); + checkoutBranch("refs/heads/side"); + // modify, add and commit file a + writeTrashFile("a", "a\nside\nc\nd\n"); + git.add().addFilepattern("a").call(); + RevCommit sideCommit = git.commit().setMessage("side").call(); + + // checkout master branch + checkoutBranch("refs/heads/master"); + // modify, add and commit file a + writeTrashFile("a", "a\nmaster\nc\n"); + git.add().addFilepattern("a").call(); + git.commit().setMessage("second master").call(); + return sideCommit; + } + private void doCherryPickAndCheckResult(final Git git, final RevCommit sideCommit, final MergeFailureReason reason) throws Exception { diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/MergeCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/MergeCommandTest.java index 8747c85dec..bc4e9405ea 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/MergeCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/MergeCommandTest.java @@ -14,6 +14,7 @@ import static org.eclipse.jgit.lib.Constants.MASTER; import static org.eclipse.jgit.lib.Constants.R_HEADS; 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; @@ -25,6 +26,7 @@ import java.util.regex.Pattern; import org.eclipse.jgit.api.MergeCommand.FastForwardMode; import org.eclipse.jgit.api.MergeResult.MergeStatus; +import org.eclipse.jgit.api.ResetCommand.ResetType; import org.eclipse.jgit.api.errors.InvalidMergeHeadsException; import org.eclipse.jgit.junit.RepositoryTestCase; import org.eclipse.jgit.junit.TestRepository; @@ -34,6 +36,7 @@ import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.RepositoryState; import org.eclipse.jgit.lib.Sets; +import org.eclipse.jgit.merge.ContentMergeStrategy; import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.merge.ResolveMerger.MergeFailureReason; import org.eclipse.jgit.revwalk.RevCommit; @@ -305,6 +308,200 @@ public class MergeCommandTest extends RepositoryTestCase { } } + @Test + public void testContentMergeXtheirs() throws Exception { + try (Git git = new Git(db)) { + writeTrashFile("a", "1\na\n3\n"); + writeTrashFile("b", "1\nb\n3\n"); + writeTrashFile("c/c/c", "1\nc\n3\n"); + git.add().addFilepattern("a").addFilepattern("b") + .addFilepattern("c/c/c").call(); + RevCommit initialCommit = git.commit().setMessage("initial").call(); + + createBranch(initialCommit, "refs/heads/side"); + checkoutBranch("refs/heads/side"); + + writeTrashFile("a", "1\na(side)\n3\n4\n"); + writeTrashFile("b", "1\nb(side)\n3\n4\n"); + git.add().addFilepattern("a").addFilepattern("b").call(); + RevCommit secondCommit = git.commit().setMessage("side").call(); + + assertEquals("1\nb(side)\n3\n4\n", + read(new File(db.getWorkTree(), "b"))); + checkoutBranch("refs/heads/master"); + assertEquals("1\nb\n3\n", read(new File(db.getWorkTree(), "b"))); + + writeTrashFile("a", "1\na(main)\n3\n"); + writeTrashFile("c/c/c", "1\nc(main)\n3\n"); + git.add().addFilepattern("a").addFilepattern("c/c/c").call(); + git.commit().setMessage("main").call(); + + MergeResult result = git.merge().include(secondCommit.getId()) + .setStrategy(MergeStrategy.RESOLVE) + .setContentMergeStrategy(ContentMergeStrategy.THEIRS) + .call(); + assertEquals(MergeStatus.MERGED, result.getMergeStatus()); + + assertEquals("1\na(side)\n3\n4\n", + read(new File(db.getWorkTree(), "a"))); + assertEquals("1\nb(side)\n3\n4\n", + read(new File(db.getWorkTree(), "b"))); + assertEquals("1\nc(main)\n3\n", + read(new File(db.getWorkTree(), "c/c/c"))); + + assertNull(result.getConflicts()); + + assertEquals(RepositoryState.SAFE, db.getRepositoryState()); + } + } + + @Test + public void testContentMergeXours() throws Exception { + try (Git git = new Git(db)) { + writeTrashFile("a", "1\na\n3\n"); + writeTrashFile("b", "1\nb\n3\n"); + writeTrashFile("c/c/c", "1\nc\n3\n"); + git.add().addFilepattern("a").addFilepattern("b") + .addFilepattern("c/c/c").call(); + RevCommit initialCommit = git.commit().setMessage("initial").call(); + + createBranch(initialCommit, "refs/heads/side"); + checkoutBranch("refs/heads/side"); + + writeTrashFile("a", "1\na(side)\n3\n4\n"); + writeTrashFile("b", "1\nb(side)\n3\n4\n"); + git.add().addFilepattern("a").addFilepattern("b").call(); + RevCommit secondCommit = git.commit().setMessage("side").call(); + + assertEquals("1\nb(side)\n3\n4\n", + read(new File(db.getWorkTree(), "b"))); + checkoutBranch("refs/heads/master"); + assertEquals("1\nb\n3\n", read(new File(db.getWorkTree(), "b"))); + + writeTrashFile("a", "1\na(main)\n3\n"); + writeTrashFile("c/c/c", "1\nc(main)\n3\n"); + git.add().addFilepattern("a").addFilepattern("c/c/c").call(); + git.commit().setMessage("main").call(); + + MergeResult result = git.merge().include(secondCommit.getId()) + .setStrategy(MergeStrategy.RESOLVE) + .setContentMergeStrategy(ContentMergeStrategy.OURS).call(); + assertEquals(MergeStatus.MERGED, result.getMergeStatus()); + + assertEquals("1\na(main)\n3\n4\n", + read(new File(db.getWorkTree(), "a"))); + assertEquals("1\nb(side)\n3\n4\n", + read(new File(db.getWorkTree(), "b"))); + assertEquals("1\nc(main)\n3\n", + read(new File(db.getWorkTree(), "c/c/c"))); + + assertNull(result.getConflicts()); + + assertEquals(RepositoryState.SAFE, db.getRepositoryState()); + } + } + + @Test + public void testBinaryContentMerge() throws Exception { + try (Git git = new Git(db)) { + writeTrashFile(".gitattributes", "a binary"); + writeTrashFile("a", "initial"); + git.add().addFilepattern(".").call(); + RevCommit initialCommit = git.commit().setMessage("initial").call(); + + createBranch(initialCommit, "refs/heads/side"); + checkoutBranch("refs/heads/side"); + + writeTrashFile("a", "side"); + git.add().addFilepattern("a").call(); + RevCommit secondCommit = git.commit().setMessage("side").call(); + + checkoutBranch("refs/heads/master"); + + writeTrashFile("a", "main"); + git.add().addFilepattern("a").call(); + git.commit().setMessage("main").call(); + + MergeResult result = git.merge().include(secondCommit.getId()) + .setStrategy(MergeStrategy.RESOLVE).call(); + assertEquals(MergeStatus.CONFLICTING, result.getMergeStatus()); + + assertEquals("main", read(new File(db.getWorkTree(), "a"))); + + // Hmmm... there doesn't seem to be a way to figure out which files + // had a binary conflict from a MergeResult... + + assertEquals(RepositoryState.MERGING, db.getRepositoryState()); + } + } + + @Test + public void testBinaryContentMergeXtheirs() throws Exception { + try (Git git = new Git(db)) { + writeTrashFile(".gitattributes", "a binary"); + writeTrashFile("a", "initial"); + git.add().addFilepattern(".").call(); + RevCommit initialCommit = git.commit().setMessage("initial").call(); + + createBranch(initialCommit, "refs/heads/side"); + checkoutBranch("refs/heads/side"); + + writeTrashFile("a", "side"); + git.add().addFilepattern("a").call(); + RevCommit secondCommit = git.commit().setMessage("side").call(); + + checkoutBranch("refs/heads/master"); + + writeTrashFile("a", "main"); + git.add().addFilepattern("a").call(); + git.commit().setMessage("main").call(); + + MergeResult result = git.merge().include(secondCommit.getId()) + .setStrategy(MergeStrategy.RESOLVE) + .setContentMergeStrategy(ContentMergeStrategy.THEIRS) + .call(); + assertEquals(MergeStatus.MERGED, result.getMergeStatus()); + + assertEquals("side", read(new File(db.getWorkTree(), "a"))); + + assertNull(result.getConflicts()); + assertEquals(RepositoryState.SAFE, db.getRepositoryState()); + } + } + + @Test + public void testBinaryContentMergeXours() throws Exception { + try (Git git = new Git(db)) { + writeTrashFile(".gitattributes", "a binary"); + writeTrashFile("a", "initial"); + git.add().addFilepattern(".").call(); + RevCommit initialCommit = git.commit().setMessage("initial").call(); + + createBranch(initialCommit, "refs/heads/side"); + checkoutBranch("refs/heads/side"); + + writeTrashFile("a", "side"); + git.add().addFilepattern("a").call(); + RevCommit secondCommit = git.commit().setMessage("side").call(); + + checkoutBranch("refs/heads/master"); + + writeTrashFile("a", "main"); + git.add().addFilepattern("a").call(); + git.commit().setMessage("main").call(); + + MergeResult result = git.merge().include(secondCommit.getId()) + .setStrategy(MergeStrategy.RESOLVE) + .setContentMergeStrategy(ContentMergeStrategy.OURS).call(); + assertEquals(MergeStatus.MERGED, result.getMergeStatus()); + + assertEquals("main", read(new File(db.getWorkTree(), "a"))); + + assertNull(result.getConflicts()); + assertEquals(RepositoryState.SAFE, db.getRepositoryState()); + } + } + @Test public void testMergeTag() throws Exception { try (Git git = new Git(db)) { @@ -774,6 +971,51 @@ public class MergeCommandTest extends RepositoryTestCase { @Test public void testDeletionOnMasterConflict() throws Exception { + try (Git git = new Git(db)) { + writeTrashFile("a", "1\na\n3\n"); + writeTrashFile("b", "1\nb\n3\n"); + git.add().addFilepattern("a").addFilepattern("b").call(); + RevCommit initialCommit = git.commit().setMessage("initial").call(); + + // create side branch and modify "a" + createBranch(initialCommit, "refs/heads/side"); + checkoutBranch("refs/heads/side"); + writeTrashFile("a", "1\na(side)\n3\n"); + git.add().addFilepattern("a").call(); + RevCommit secondCommit = git.commit().setMessage("side").call(); + + // delete a on master to generate conflict + checkoutBranch("refs/heads/master"); + git.rm().addFilepattern("a").call(); + RevCommit thirdCommit = git.commit().setMessage("main").call(); + + for (ContentMergeStrategy contentStrategy : ContentMergeStrategy + .values()) { + // merge side with master + MergeResult result = git.merge().include(secondCommit.getId()) + .setStrategy(MergeStrategy.RESOLVE) + .setContentMergeStrategy(contentStrategy) + .call(); + assertEquals("merge -X " + contentStrategy.name(), + MergeStatus.CONFLICTING, result.getMergeStatus()); + + // result should be 'a' conflicting with workspace content from + // side + assertTrue("merge -X " + contentStrategy.name(), + new File(db.getWorkTree(), "a").exists()); + assertEquals("merge -X " + contentStrategy.name(), + "1\na(side)\n3\n", + read(new File(db.getWorkTree(), "a"))); + assertEquals("merge -X " + contentStrategy.name(), "1\nb\n3\n", + read(new File(db.getWorkTree(), "b"))); + git.reset().setMode(ResetType.HARD).setRef(thirdCommit.name()) + .call(); + } + } + } + + @Test + public void testDeletionOnMasterTheirs() throws Exception { try (Git git = new Git(db)) { writeTrashFile("a", "1\na\n3\n"); writeTrashFile("b", "1\nb\n3\n"); @@ -794,18 +1036,102 @@ public class MergeCommandTest extends RepositoryTestCase { // merge side with master MergeResult result = git.merge().include(secondCommit.getId()) - .setStrategy(MergeStrategy.RESOLVE).call(); - assertEquals(MergeStatus.CONFLICTING, result.getMergeStatus()); + .setStrategy(MergeStrategy.THEIRS) + .call(); + assertEquals(MergeStatus.MERGED, result.getMergeStatus()); - // result should be 'a' conflicting with workspace content from side + // result should be 'a' assertTrue(new File(db.getWorkTree(), "a").exists()); - assertEquals("1\na(side)\n3\n", read(new File(db.getWorkTree(), "a"))); + assertEquals("1\na(side)\n3\n", + read(new File(db.getWorkTree(), "a"))); + assertEquals("1\nb\n3\n", read(new File(db.getWorkTree(), "b"))); + assertTrue(git.status().call().isClean()); + } + } + + @Test + public void testDeletionOnMasterOurs() throws Exception { + try (Git git = new Git(db)) { + writeTrashFile("a", "1\na\n3\n"); + writeTrashFile("b", "1\nb\n3\n"); + git.add().addFilepattern("a").addFilepattern("b").call(); + RevCommit initialCommit = git.commit().setMessage("initial").call(); + + // create side branch and modify "a" + createBranch(initialCommit, "refs/heads/side"); + checkoutBranch("refs/heads/side"); + writeTrashFile("a", "1\na(side)\n3\n"); + git.add().addFilepattern("a").call(); + RevCommit secondCommit = git.commit().setMessage("side").call(); + + // delete a on master to generate conflict + checkoutBranch("refs/heads/master"); + git.rm().addFilepattern("a").call(); + git.commit().setMessage("main").call(); + + // merge side with master + MergeResult result = git.merge().include(secondCommit.getId()) + .setStrategy(MergeStrategy.OURS).call(); + assertEquals(MergeStatus.MERGED, result.getMergeStatus()); + + assertFalse(new File(db.getWorkTree(), "a").exists()); assertEquals("1\nb\n3\n", read(new File(db.getWorkTree(), "b"))); + assertTrue(git.status().call().isClean()); } } @Test public void testDeletionOnSideConflict() throws Exception { + try (Git git = new Git(db)) { + writeTrashFile("a", "1\na\n3\n"); + writeTrashFile("b", "1\nb\n3\n"); + git.add().addFilepattern("a").addFilepattern("b").call(); + RevCommit initialCommit = git.commit().setMessage("initial").call(); + + // create side branch and delete "a" + createBranch(initialCommit, "refs/heads/side"); + checkoutBranch("refs/heads/side"); + git.rm().addFilepattern("a").call(); + RevCommit secondCommit = git.commit().setMessage("side").call(); + + // update a on master to generate conflict + checkoutBranch("refs/heads/master"); + writeTrashFile("a", "1\na(main)\n3\n"); + git.add().addFilepattern("a").call(); + RevCommit thirdCommit = git.commit().setMessage("main").call(); + + for (ContentMergeStrategy contentStrategy : ContentMergeStrategy + .values()) { + // merge side with master + MergeResult result = git.merge().include(secondCommit.getId()) + .setStrategy(MergeStrategy.RESOLVE) + .setContentMergeStrategy(contentStrategy) + .call(); + assertEquals("merge -X " + contentStrategy.name(), + MergeStatus.CONFLICTING, result.getMergeStatus()); + + assertTrue("merge -X " + contentStrategy.name(), + new File(db.getWorkTree(), "a").exists()); + assertEquals("merge -X " + contentStrategy.name(), + "1\na(main)\n3\n", + read(new File(db.getWorkTree(), "a"))); + assertEquals("merge -X " + contentStrategy.name(), "1\nb\n3\n", + read(new File(db.getWorkTree(), "b"))); + + assertNotNull("merge -X " + contentStrategy.name(), + result.getConflicts()); + assertEquals("merge -X " + contentStrategy.name(), 1, + result.getConflicts().size()); + assertEquals("merge -X " + contentStrategy.name(), 3, + result.getConflicts().get("a")[0].length); + git.reset().setMode(ResetType.HARD).setRef(thirdCommit.name()) + .call(); + } + } + } + + @Test + public void testDeletionOnSideTheirs() throws Exception { try (Git git = new Git(db)) { writeTrashFile("a", "1\na\n3\n"); writeTrashFile("b", "1\nb\n3\n"); @@ -826,15 +1152,45 @@ public class MergeCommandTest extends RepositoryTestCase { // merge side with master MergeResult result = git.merge().include(secondCommit.getId()) - .setStrategy(MergeStrategy.RESOLVE).call(); - assertEquals(MergeStatus.CONFLICTING, result.getMergeStatus()); + .setStrategy(MergeStrategy.THEIRS).call(); + assertEquals(MergeStatus.MERGED, result.getMergeStatus()); - assertTrue(new File(db.getWorkTree(), "a").exists()); - assertEquals("1\na(main)\n3\n", read(new File(db.getWorkTree(), "a"))); + assertFalse(new File(db.getWorkTree(), "a").exists()); assertEquals("1\nb\n3\n", read(new File(db.getWorkTree(), "b"))); + assertTrue(git.status().call().isClean()); + } + } - assertEquals(1, result.getConflicts().size()); - assertEquals(3, result.getConflicts().get("a")[0].length); + @Test + public void testDeletionOnSideOurs() throws Exception { + try (Git git = new Git(db)) { + writeTrashFile("a", "1\na\n3\n"); + writeTrashFile("b", "1\nb\n3\n"); + git.add().addFilepattern("a").addFilepattern("b").call(); + RevCommit initialCommit = git.commit().setMessage("initial").call(); + + // create side branch and delete "a" + createBranch(initialCommit, "refs/heads/side"); + checkoutBranch("refs/heads/side"); + git.rm().addFilepattern("a").call(); + RevCommit secondCommit = git.commit().setMessage("side").call(); + + // update a on master to generate conflict + checkoutBranch("refs/heads/master"); + writeTrashFile("a", "1\na(main)\n3\n"); + git.add().addFilepattern("a").call(); + git.commit().setMessage("main").call(); + + // merge side with master + MergeResult result = git.merge().include(secondCommit.getId()) + .setStrategy(MergeStrategy.OURS).call(); + assertEquals(MergeStatus.MERGED, result.getMergeStatus()); + + assertTrue(new File(db.getWorkTree(), "a").exists()); + assertEquals("1\na(main)\n3\n", + read(new File(db.getWorkTree(), "a"))); + assertEquals("1\nb\n3\n", read(new File(db.getWorkTree(), "b"))); + assertTrue(git.status().call().isClean()); } } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/PullCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/PullCommandTest.java index e4af44e6f8..9af77aa3e8 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/PullCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/PullCommandTest.java @@ -34,6 +34,8 @@ import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.RepositoryState; import org.eclipse.jgit.lib.StoredConfig; +import org.eclipse.jgit.merge.ContentMergeStrategy; +import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevSort; import org.eclipse.jgit.revwalk.RevWalk; @@ -153,6 +155,75 @@ public class PullCommandTest extends RepositoryTestCase { .getRepositoryState()); } + @Test + public void testPullConflictTheirs() throws Exception { + PullResult res = target.pull().call(); + // nothing to update since we don't have different data yet + assertTrue(res.getFetchResult().getTrackingRefUpdates().isEmpty()); + assertTrue(res.getMergeResult().getMergeStatus() + .equals(MergeStatus.ALREADY_UP_TO_DATE)); + + assertFileContentsEqual(targetFile, "Hello world"); + + // change the source file + writeToFile(sourceFile, "Source change"); + source.add().addFilepattern("SomeFile.txt").call(); + source.commit().setMessage("Source change in remote").call(); + + // change the target file + writeToFile(targetFile, "Target change"); + target.add().addFilepattern("SomeFile.txt").call(); + target.commit().setMessage("Target change in local").call(); + + res = target.pull().setStrategy(MergeStrategy.THEIRS).call(); + + assertTrue(res.isSuccessful()); + assertFileContentsEqual(targetFile, "Source change"); + assertEquals(RepositoryState.SAFE, + target.getRepository().getRepositoryState()); + assertTrue(target.status().call().isClean()); + } + + @Test + public void testPullConflictXtheirs() throws Exception { + PullResult res = target.pull().call(); + // nothing to update since we don't have different data yet + assertTrue(res.getFetchResult().getTrackingRefUpdates().isEmpty()); + assertTrue(res.getMergeResult().getMergeStatus() + .equals(MergeStatus.ALREADY_UP_TO_DATE)); + + assertFileContentsEqual(targetFile, "Hello world"); + + // change the source file + writeToFile(sourceFile, "a\nHello\nb\n"); + source.add().addFilepattern("SomeFile.txt").call(); + source.commit().setMessage("Multi-line change in remote").call(); + + // Pull again + res = target.pull().call(); + assertTrue(res.isSuccessful()); + assertFileContentsEqual(targetFile, "a\nHello\nb\n"); + + // change the source file + writeToFile(sourceFile, "a\nSource change\nb\n"); + source.add().addFilepattern("SomeFile.txt").call(); + source.commit().setMessage("Source change in remote").call(); + + // change the target file + writeToFile(targetFile, "a\nTarget change\nb\nc\n"); + target.add().addFilepattern("SomeFile.txt").call(); + target.commit().setMessage("Target change in local").call(); + + res = target.pull().setContentMergeStrategy(ContentMergeStrategy.THEIRS) + .call(); + + assertTrue(res.isSuccessful()); + assertFileContentsEqual(targetFile, "a\nSource change\nb\nc\n"); + assertEquals(RepositoryState.SAFE, + target.getRepository().getRepositoryState()); + assertTrue(target.status().call().isClean()); + } + @Test public void testPullWithUntrackedStash() throws Exception { target.pull().call(); diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/StashApplyCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/StashApplyCommandTest.java index f109cbf50f..49b31b1c4c 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/StashApplyCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/StashApplyCommandTest.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2012, GitHub Inc. and others + * Copyright (C) 2012, 2021 GitHub Inc. and others * * This program and the accompanying materials are made available under the * terms of the Eclipse Distribution License v. 1.0 which is available at @@ -28,6 +28,8 @@ import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.junit.RepositoryTestCase; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.merge.ContentMergeStrategy; +import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.util.FileUtils; import org.junit.After; @@ -426,6 +428,135 @@ public class StashApplyCommandTest extends RepositoryTestCase { read(PATH)); } + @Test + public void stashedContentMergeXtheirs() throws Exception { + writeTrashFile(PATH, "content\nmore content\n"); + git.add().addFilepattern(PATH).call(); + git.commit().setMessage("more content").call(); + + writeTrashFile(PATH, "content\nhead change\nmore content\n"); + git.add().addFilepattern(PATH).call(); + git.commit().setMessage("even content").call(); + + writeTrashFile(PATH, "content\nstashed change\nmore content\n"); + + RevCommit stashed = git.stashCreate().call(); + assertNotNull(stashed); + assertEquals("content\nhead change\nmore content\n", + read(committedFile)); + assertTrue(git.status().call().isClean()); + recorder.assertEvent(new String[] { PATH }, ChangeRecorder.EMPTY); + + writeTrashFile(PATH, "content\nmore content\ncommitted change\n"); + git.add().addFilepattern(PATH).call(); + git.commit().setMessage("committed change").call(); + recorder.assertNoEvent(); + + git.stashApply().setContentMergeStrategy(ContentMergeStrategy.THEIRS) + .call(); + recorder.assertEvent(new String[] { PATH }, ChangeRecorder.EMPTY); + Status status = new StatusCommand(db).call(); + assertEquals('[' + PATH + ']', status.getModified().toString()); + assertEquals( + "content\nstashed change\nmore content\ncommitted change\n", + read(PATH)); + } + + @Test + public void stashedContentMergeXours() throws Exception { + writeTrashFile(PATH, "content\nmore content\n"); + git.add().addFilepattern(PATH).call(); + git.commit().setMessage("more content").call(); + + writeTrashFile(PATH, "content\nhead change\nmore content\n"); + git.add().addFilepattern(PATH).call(); + git.commit().setMessage("even content").call(); + + writeTrashFile(PATH, "content\nstashed change\nmore content\n"); + + RevCommit stashed = git.stashCreate().call(); + assertNotNull(stashed); + assertEquals("content\nhead change\nmore content\n", + read(committedFile)); + assertTrue(git.status().call().isClean()); + recorder.assertEvent(new String[] { PATH }, ChangeRecorder.EMPTY); + + writeTrashFile(PATH, + "content\nnew head\nmore content\ncommitted change\n"); + git.add().addFilepattern(PATH).call(); + git.commit().setMessage("committed change").call(); + recorder.assertNoEvent(); + + git.stashApply().setContentMergeStrategy(ContentMergeStrategy.OURS) + .call(); + recorder.assertEvent(new String[] { PATH }, ChangeRecorder.EMPTY); + assertTrue(git.status().call().isClean()); + assertEquals("content\nnew head\nmore content\ncommitted change\n", + read(PATH)); + } + + @Test + public void stashedContentMergeTheirs() throws Exception { + writeTrashFile(PATH, "content\nmore content\n"); + git.add().addFilepattern(PATH).call(); + git.commit().setMessage("more content").call(); + + writeTrashFile(PATH, "content\nhead change\nmore content\n"); + git.add().addFilepattern(PATH).call(); + git.commit().setMessage("even content").call(); + + writeTrashFile(PATH, "content\nstashed change\nmore content\n"); + + RevCommit stashed = git.stashCreate().call(); + assertNotNull(stashed); + assertEquals("content\nhead change\nmore content\n", + read(committedFile)); + assertTrue(git.status().call().isClean()); + recorder.assertEvent(new String[] { PATH }, ChangeRecorder.EMPTY); + + writeTrashFile(PATH, "content\nmore content\ncommitted change\n"); + git.add().addFilepattern(PATH).call(); + git.commit().setMessage("committed change").call(); + recorder.assertNoEvent(); + + git.stashApply().setStrategy(MergeStrategy.THEIRS).call(); + recorder.assertEvent(new String[] { PATH }, ChangeRecorder.EMPTY); + Status status = new StatusCommand(db).call(); + assertEquals('[' + PATH + ']', status.getModified().toString()); + assertEquals("content\nstashed change\nmore content\n", read(PATH)); + } + + @Test + public void stashedContentMergeOurs() throws Exception { + writeTrashFile(PATH, "content\nmore content\n"); + git.add().addFilepattern(PATH).call(); + git.commit().setMessage("more content").call(); + + writeTrashFile(PATH, "content\nhead change\nmore content\n"); + git.add().addFilepattern(PATH).call(); + git.commit().setMessage("even content").call(); + + writeTrashFile(PATH, "content\nstashed change\nmore content\n"); + + RevCommit stashed = git.stashCreate().call(); + assertNotNull(stashed); + assertEquals("content\nhead change\nmore content\n", + read(committedFile)); + assertTrue(git.status().call().isClean()); + recorder.assertEvent(new String[] { PATH }, ChangeRecorder.EMPTY); + + writeTrashFile(PATH, "content\nmore content\ncommitted change\n"); + git.add().addFilepattern(PATH).call(); + git.commit().setMessage("committed change").call(); + recorder.assertNoEvent(); + + // Doesn't make any sense... should be a no-op + git.stashApply().setStrategy(MergeStrategy.OURS).call(); + recorder.assertNoEvent(); + assertTrue(git.status().call().isClean()); + assertEquals("content\nmore content\ncommitted change\n", read(PATH)); + } + @Test public void stashedApplyOnOtherBranch() throws Exception { writeTrashFile(PATH, "content\nmore content\n"); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/CherryPickCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/CherryPickCommand.java index 5d0154c6dc..7922f9e729 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/CherryPickCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/CherryPickCommand.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010, Christian Halstrick and others + * Copyright (C) 2010, 2021 Christian Halstrick and others * * This program and the accompanying materials are made available under the * terms of the Eclipse Distribution License v. 1.0 which is available at @@ -13,6 +13,7 @@ import java.io.IOException; import java.text.MessageFormat; import java.util.LinkedList; import java.util.List; +import java.util.Map; import org.eclipse.jgit.api.errors.ConcurrentRefUpdateException; import org.eclipse.jgit.api.errors.GitAPIException; @@ -35,9 +36,12 @@ import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Ref.Storage; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.merge.ContentMergeStrategy; import org.eclipse.jgit.merge.MergeMessageFormatter; import org.eclipse.jgit.merge.MergeStrategy; +import org.eclipse.jgit.merge.Merger; import org.eclipse.jgit.merge.ResolveMerger; +import org.eclipse.jgit.merge.ResolveMerger.MergeFailureReason; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.treewalk.FileTreeIterator; @@ -61,6 +65,8 @@ public class CherryPickCommand extends GitCommand { private MergeStrategy strategy = MergeStrategy.RECURSIVE; + private ContentMergeStrategy contentStrategy; + private Integer mainlineParentNumber; private boolean noCommit = false; @@ -121,16 +127,30 @@ public class CherryPickCommand extends GitCommand { String cherryPickName = srcCommit.getId().abbreviate(7).name() + " " + srcCommit.getShortMessage(); //$NON-NLS-1$ - ResolveMerger merger = (ResolveMerger) strategy.newMerger(repo); - merger.setWorkingTreeIterator(new FileTreeIterator(repo)); - merger.setBase(srcParent.getTree()); - merger.setCommitNames(new String[] { "BASE", ourName, //$NON-NLS-1$ - cherryPickName }); - if (merger.merge(newHead, srcCommit)) { - if (!merger.getModifiedFiles().isEmpty()) { + Merger merger = strategy.newMerger(repo); + merger.setProgressMonitor(monitor); + boolean noProblems; + Map failingPaths = null; + List unmergedPaths = null; + if (merger instanceof ResolveMerger) { + ResolveMerger resolveMerger = (ResolveMerger) merger; + resolveMerger.setContentMergeStrategy(contentStrategy); + resolveMerger.setCommitNames( + new String[] { "BASE", ourName, cherryPickName }); //$NON-NLS-1$ + resolveMerger + .setWorkingTreeIterator(new FileTreeIterator(repo)); + resolveMerger.setBase(srcParent.getTree()); + noProblems = merger.merge(newHead, srcCommit); + failingPaths = resolveMerger.getFailingPaths(); + unmergedPaths = resolveMerger.getUnmergedPaths(); + if (!resolveMerger.getModifiedFiles().isEmpty()) { repo.fireEvent(new WorkingTreeModifiedEvent( - merger.getModifiedFiles(), null)); + resolveMerger.getModifiedFiles(), null)); } + } else { + noProblems = merger.merge(newHead, srcCommit); + } + if (noProblems) { if (AnyObjectId.isEqual(newHead.getTree().getId(), merger.getResultTreeId())) { continue; @@ -153,24 +173,26 @@ public class CherryPickCommand extends GitCommand { } cherryPickedRefs.add(src); } else { - if (merger.failed()) { - return new CherryPickResult(merger.getFailingPaths()); + if (failingPaths != null && !failingPaths.isEmpty()) { + return new CherryPickResult(failingPaths); } // there are merge conflicts - String message = new MergeMessageFormatter() + String message; + if (unmergedPaths != null) { + message = new MergeMessageFormatter() .formatWithConflicts(srcCommit.getFullMessage(), - merger.getUnmergedPaths()); + unmergedPaths); + } else { + message = srcCommit.getFullMessage(); + } if (!noCommit) { repo.writeCherryPickHead(srcCommit.getId()); } repo.writeMergeCommitMsg(message); - repo.fireEvent(new WorkingTreeModifiedEvent( - merger.getModifiedFiles(), null)); - return CherryPickResult.CONFLICT; } } @@ -290,6 +312,22 @@ public class CherryPickCommand extends GitCommand { return this; } + /** + * Sets the content merge strategy to use if the + * {@link #setStrategy(MergeStrategy) merge strategy} is "resolve" or + * "recursive". + * + * @param strategy + * the {@link ContentMergeStrategy} to be used + * @return {@code this} + * @since 5.12 + */ + public CherryPickCommand setContentMergeStrategy( + ContentMergeStrategy strategy) { + this.contentStrategy = strategy; + return this; + } + /** * Set the (1-based) parent number to diff against * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/MergeCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/MergeCommand.java index d88f4ec561..c611f915ae 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/MergeCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/MergeCommand.java @@ -1,7 +1,7 @@ /* * Copyright (C) 2010, Christian Halstrick - * Copyright (C) 2010-2014, Stefan Lay - * Copyright (C) 2016, Laurent Delaigue and others + * Copyright (C) 2010, 2014, Stefan Lay + * Copyright (C) 2016, 2021 Laurent Delaigue and others * * This program and the accompanying materials are made available under the * terms of the Eclipse Distribution License v. 1.0 which is available at @@ -45,6 +45,7 @@ import org.eclipse.jgit.lib.Ref.Storage; import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.RefUpdate.Result; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.merge.ContentMergeStrategy; import org.eclipse.jgit.merge.MergeConfig; import org.eclipse.jgit.merge.MergeMessageFormatter; import org.eclipse.jgit.merge.MergeStrategy; @@ -71,6 +72,8 @@ public class MergeCommand extends GitCommand { private MergeStrategy mergeStrategy = MergeStrategy.RECURSIVE; + private ContentMergeStrategy contentStrategy; + private List commits = new LinkedList<>(); private Boolean squash; @@ -320,6 +323,7 @@ public class MergeCommand extends GitCommand { List unmergedPaths = null; if (merger instanceof ResolveMerger) { ResolveMerger resolveMerger = (ResolveMerger) merger; + resolveMerger.setContentMergeStrategy(contentStrategy); resolveMerger.setCommitNames(new String[] { "BASE", "HEAD", ref.getName() }); //$NON-NLS-1$ //$NON-NLS-2$ resolveMerger.setWorkingTreeIterator(new FileTreeIterator(repo)); @@ -472,6 +476,22 @@ public class MergeCommand extends GitCommand { return this; } + /** + * Sets the content merge strategy to use if the + * {@link #setStrategy(MergeStrategy) merge strategy} is "resolve" or + * "recursive". + * + * @param strategy + * the {@link ContentMergeStrategy} to be used + * @return {@code this} + * @since 5.12 + */ + public MergeCommand setContentMergeStrategy(ContentMergeStrategy strategy) { + checkCallable(); + this.contentStrategy = strategy; + return this; + } + /** * Reference to a commit to be merged with the current head * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/PullCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/PullCommand.java index 449250890c..281ecfd011 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/PullCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/PullCommand.java @@ -1,7 +1,7 @@ /* * Copyright (C) 2010, Christian Halstrick * Copyright (C) 2010, Mathias Kinzler - * Copyright (C) 2016, Laurent Delaigue and others + * Copyright (C) 2016, 2021 Laurent Delaigue and others * * This program and the accompanying materials are made available under the * terms of the Eclipse Distribution License v. 1.0 which is available at @@ -43,6 +43,7 @@ import org.eclipse.jgit.lib.RefUpdate.Result; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.RepositoryState; import org.eclipse.jgit.lib.SubmoduleConfig.FetchRecurseSubmodulesMode; +import org.eclipse.jgit.merge.ContentMergeStrategy; import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; @@ -69,6 +70,8 @@ public class PullCommand extends TransportCommand { private MergeStrategy strategy = MergeStrategy.RECURSIVE; + private ContentMergeStrategy contentStrategy; + private TagOpt tagOption; private FastForwardMode fastForwardMode; @@ -275,8 +278,7 @@ public class PullCommand extends TransportCommand { JGitText.get().pullTaskName)); // we check the updates to see which of the updated branches - // corresponds - // to the remote branch name + // corresponds to the remote branch name AnyObjectId commitToMerge; if (isRemote) { Ref r = null; @@ -354,8 +356,11 @@ public class PullCommand extends TransportCommand { } RebaseCommand rebase = new RebaseCommand(repo); RebaseResult rebaseRes = rebase.setUpstream(commitToMerge) - .setUpstreamName(upstreamName).setProgressMonitor(monitor) - .setOperation(Operation.BEGIN).setStrategy(strategy) + .setProgressMonitor(monitor) + .setUpstreamName(upstreamName) + .setOperation(Operation.BEGIN) + .setStrategy(strategy) + .setContentMergeStrategy(contentStrategy) .setPreserveMerges( pullRebaseMode == BranchRebaseMode.PRESERVE) .call(); @@ -363,7 +368,9 @@ public class PullCommand extends TransportCommand { } else { MergeCommand merge = new MergeCommand(repo); MergeResult mergeRes = merge.include(upstreamName, commitToMerge) - .setStrategy(strategy).setProgressMonitor(monitor) + .setProgressMonitor(monitor) + .setStrategy(strategy) + .setContentMergeStrategy(contentStrategy) .setFastForward(getFastForwardMode()).call(); monitor.update(1); result = new PullResult(fetchRes, remote, mergeRes); @@ -441,6 +448,21 @@ public class PullCommand extends TransportCommand { return this; } + /** + * Sets the content merge strategy to use if the + * {@link #setStrategy(MergeStrategy) merge strategy} is "resolve" or + * "recursive". + * + * @param strategy + * the {@link ContentMergeStrategy} to be used + * @return {@code this} + * @since 5.12 + */ + public PullCommand setContentMergeStrategy(ContentMergeStrategy strategy) { + this.contentStrategy = strategy; + return this; + } + /** * Set the specification of annotated tag behavior during fetch * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseCommand.java index 836175dcea..a26ffc2e66 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseCommand.java @@ -1,6 +1,6 @@ /* * Copyright (C) 2010, 2013 Mathias Kinzler - * Copyright (C) 2016, Laurent Delaigue and others + * Copyright (C) 2016, 2021 Laurent Delaigue and others * * This program and the accompanying materials are made available under the * terms of the Eclipse Distribution License v. 1.0 which is available at @@ -65,6 +65,7 @@ import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.RefUpdate.Result; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.merge.ContentMergeStrategy; import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevSort; @@ -212,6 +213,8 @@ public class RebaseCommand extends GitCommand { private MergeStrategy strategy = MergeStrategy.RECURSIVE; + private ContentMergeStrategy contentStrategy; + private boolean preserveMerges = false; /** @@ -501,8 +504,11 @@ public class RebaseCommand extends GitCommand { String ourCommitName = getOurCommitName(); try (Git git = new Git(repo)) { CherryPickResult cherryPickResult = git.cherryPick() - .include(commitToPick).setOurCommitName(ourCommitName) - .setReflogPrefix(REFLOG_PREFIX).setStrategy(strategy) + .include(commitToPick) + .setOurCommitName(ourCommitName) + .setReflogPrefix(REFLOG_PREFIX) + .setStrategy(strategy) + .setContentMergeStrategy(contentStrategy) .call(); switch (cherryPickResult.getStatus()) { case FAILED: @@ -556,7 +562,8 @@ public class RebaseCommand extends GitCommand { .include(commitToPick) .setOurCommitName(ourCommitName) .setReflogPrefix(REFLOG_PREFIX) - .setStrategy(strategy); + .setStrategy(strategy) + .setContentMergeStrategy(contentStrategy); if (isMerge) { pickCommand.setMainlineParentNumber(1); // We write a MERGE_HEAD and later commit explicitly @@ -592,6 +599,8 @@ public class RebaseCommand extends GitCommand { MergeCommand merge = git.merge() .setFastForward(MergeCommand.FastForwardMode.NO_FF) .setProgressMonitor(monitor) + .setStrategy(strategy) + .setContentMergeStrategy(contentStrategy) .setCommit(false); for (int i = 1; i < commitToPick.getParentCount(); i++) merge.include(newParents.get(i)); @@ -1137,7 +1146,7 @@ public class RebaseCommand extends GitCommand { } private List calculatePickList(RevCommit headCommit) - throws GitAPIException, NoHeadException, IOException { + throws IOException { List cherryPickList = new ArrayList<>(); try (RevWalk r = new RevWalk(repo)) { r.sort(RevSort.TOPO_KEEP_BRANCH_TOGETHER, true); @@ -1586,6 +1595,21 @@ public class RebaseCommand extends GitCommand { return this; } + /** + * Sets the content merge strategy to use if the + * {@link #setStrategy(MergeStrategy) merge strategy} is "resolve" or + * "recursive". + * + * @param strategy + * the {@link ContentMergeStrategy} to be used + * @return {@code this} + * @since 5.12 + */ + public RebaseCommand setContentMergeStrategy(ContentMergeStrategy strategy) { + this.contentStrategy = strategy; + return this; + } + /** * Whether to preserve merges during rebase * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/StashApplyCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/StashApplyCommand.java index 56b3992fcd..1004d3e50f 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/StashApplyCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/StashApplyCommand.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2012, 2017 GitHub Inc. and others + * Copyright (C) 2012, 2021 GitHub Inc. and others * * This program and the accompanying materials are made available under the * terms of the Eclipse Distribution License v. 1.0 which is available at @@ -38,7 +38,9 @@ import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.RepositoryState; +import org.eclipse.jgit.merge.ContentMergeStrategy; import org.eclipse.jgit.merge.MergeStrategy; +import org.eclipse.jgit.merge.Merger; import org.eclipse.jgit.merge.ResolveMerger; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevTree; @@ -71,6 +73,8 @@ public class StashApplyCommand extends GitCommand { private MergeStrategy strategy = MergeStrategy.RECURSIVE; + private ContentMergeStrategy contentStrategy; + /** * Create command to apply the changes of a stashed commit * @@ -166,16 +170,25 @@ public class StashApplyCommand extends GitCommand { if (restoreUntracked && stashCommit.getParentCount() == 3) untrackedCommit = revWalk.parseCommit(stashCommit.getParent(2)); - ResolveMerger merger = (ResolveMerger) strategy.newMerger(repo); - merger.setCommitNames(new String[] { "stashed HEAD", "HEAD", //$NON-NLS-1$ //$NON-NLS-2$ - "stash" }); //$NON-NLS-1$ - merger.setBase(stashHeadCommit); - merger.setWorkingTreeIterator(new FileTreeIterator(repo)); - boolean mergeSucceeded = merger.merge(headCommit, stashCommit); - List modifiedByMerge = merger.getModifiedFiles(); - if (!modifiedByMerge.isEmpty()) { - repo.fireEvent( - new WorkingTreeModifiedEvent(modifiedByMerge, null)); + Merger merger = strategy.newMerger(repo); + boolean mergeSucceeded; + if (merger instanceof ResolveMerger) { + ResolveMerger resolveMerger = (ResolveMerger) merger; + resolveMerger + .setCommitNames(new String[] { "stashed HEAD", "HEAD", //$NON-NLS-1$ //$NON-NLS-2$ + "stash" }); //$NON-NLS-1$ + resolveMerger.setBase(stashHeadCommit); + resolveMerger + .setWorkingTreeIterator(new FileTreeIterator(repo)); + resolveMerger.setContentMergeStrategy(contentStrategy); + mergeSucceeded = resolveMerger.merge(headCommit, stashCommit); + List modifiedByMerge = resolveMerger.getModifiedFiles(); + if (!modifiedByMerge.isEmpty()) { + repo.fireEvent(new WorkingTreeModifiedEvent(modifiedByMerge, + null)); + } + } else { + mergeSucceeded = merger.merge(headCommit, stashCommit); } if (mergeSucceeded) { DirCache dc = repo.lockDirCache(); @@ -184,11 +197,14 @@ public class StashApplyCommand extends GitCommand { dco.setFailOnConflict(true); dco.checkout(); // Ignoring failed deletes.... if (restoreIndex) { - ResolveMerger ixMerger = (ResolveMerger) strategy - .newMerger(repo, true); - ixMerger.setCommitNames(new String[] { "stashed HEAD", //$NON-NLS-1$ - "HEAD", "stashed index" }); //$NON-NLS-1$//$NON-NLS-2$ - ixMerger.setBase(stashHeadCommit); + Merger ixMerger = strategy.newMerger(repo, true); + if (ixMerger instanceof ResolveMerger) { + ResolveMerger resolveMerger = (ResolveMerger) ixMerger; + resolveMerger.setCommitNames(new String[] { "stashed HEAD", //$NON-NLS-1$ + "HEAD", "stashed index" }); //$NON-NLS-1$//$NON-NLS-2$ + resolveMerger.setBase(stashHeadCommit); + resolveMerger.setContentMergeStrategy(contentStrategy); + } boolean ok = ixMerger.merge(headCommit, stashIndexCommit); if (ok) { resetIndex(revWalk @@ -200,16 +216,20 @@ public class StashApplyCommand extends GitCommand { } if (untrackedCommit != null) { - ResolveMerger untrackedMerger = (ResolveMerger) strategy - .newMerger(repo, true); - untrackedMerger.setCommitNames(new String[] { - "null", "HEAD", "untracked files" }); //$NON-NLS-1$//$NON-NLS-2$//$NON-NLS-3$ - // There is no common base for HEAD & untracked files - // because the commit for untracked files has no parent. If - // we use stashHeadCommit as common base (as in the other - // merges) we potentially report conflicts for files - // which are not even member of untracked files commit - untrackedMerger.setBase(null); + Merger untrackedMerger = strategy.newMerger(repo, true); + if (untrackedMerger instanceof ResolveMerger) { + ResolveMerger resolveMerger = (ResolveMerger) untrackedMerger; + resolveMerger.setCommitNames(new String[] { "null", "HEAD", //$NON-NLS-1$//$NON-NLS-2$ + "untracked files" }); //$NON-NLS-1$ + // There is no common base for HEAD & untracked files + // because the commit for untracked files has no parent. + // If we use stashHeadCommit as common base (as in the + // other merges) we potentially report conflicts for + // files which are not even member of untracked files + // commit. + resolveMerger.setBase(null); + resolveMerger.setContentMergeStrategy(contentStrategy); + } boolean ok = untrackedMerger.merge(headCommit, untrackedCommit); if (ok) { @@ -278,6 +298,23 @@ public class StashApplyCommand extends GitCommand { return this; } + /** + * Sets the content merge strategy to use if the + * {@link #setStrategy(MergeStrategy) merge strategy} is "resolve" or + * "recursive". + * + * @param strategy + * the {@link ContentMergeStrategy} to be used + * @return {@code this} + * @since 5.12 + */ + public StashApplyCommand setContentMergeStrategy( + ContentMergeStrategy strategy) { + checkCallable(); + this.contentStrategy = strategy; + return this; + } + /** * Whether the command should restore untracked files * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ContentMergeStrategy.java b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ContentMergeStrategy.java new file mode 100644 index 0000000000..6d568643d5 --- /dev/null +++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ContentMergeStrategy.java @@ -0,0 +1,27 @@ +/* + * Copyright (C) 2021, Thomas Wolf and others + * + * 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.merge; + +/** + * How to handle content conflicts. + * + * @since 5.12 + */ +public enum ContentMergeStrategy { + + /** Produce a conflict. */ + CONFLICT, + + /** Resolve the conflict hunk using the ours version. */ + OURS, + + /** Resolve the conflict hunk using the theirs version. */ + THEIRS +} \ No newline at end of file diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/merge/MergeAlgorithm.java b/org.eclipse.jgit/src/org/eclipse/jgit/merge/MergeAlgorithm.java index 27141c12c4..80607351ae 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/MergeAlgorithm.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/MergeAlgorithm.java @@ -14,6 +14,7 @@ import java.util.ArrayList; import java.util.Iterator; import java.util.List; +import org.eclipse.jgit.annotations.NonNull; import org.eclipse.jgit.diff.DiffAlgorithm; import org.eclipse.jgit.diff.Edit; import org.eclipse.jgit.diff.EditList; @@ -28,8 +29,12 @@ import org.eclipse.jgit.merge.MergeChunk.ConflictState; * diff algorithm. */ public final class MergeAlgorithm { + private final DiffAlgorithm diffAlg; + @NonNull + private ContentMergeStrategy strategy = ContentMergeStrategy.CONFLICT; + /** * Creates a new MergeAlgorithm which uses * {@link org.eclipse.jgit.diff.HistogramDiff} as diff algorithm @@ -48,6 +53,30 @@ public final class MergeAlgorithm { this.diffAlg = diff; } + /** + * Retrieves the {@link ContentMergeStrategy}. + * + * @return the {@link ContentMergeStrategy} in effect + * @since 5.12 + */ + @NonNull + public ContentMergeStrategy getContentMergeStrategy() { + return strategy; + } + + /** + * Sets the {@link ContentMergeStrategy}. + * + * @param strategy + * {@link ContentMergeStrategy} to set; if {@code null}, set + * {@link ContentMergeStrategy#CONFLICT} + * @since 5.12 + */ + public void setContentMergeStrategy(ContentMergeStrategy strategy) { + this.strategy = strategy == null ? ContentMergeStrategy.CONFLICT + : strategy; + } + // An special edit which acts as a sentinel value by marking the end the // list of edits private static final Edit END_EDIT = new Edit(Integer.MAX_VALUE, @@ -79,29 +108,54 @@ public final class MergeAlgorithm { if (theirs.size() != 0) { EditList theirsEdits = diffAlg.diff(cmp, base, theirs); if (!theirsEdits.isEmpty()) { - // we deleted, they modified -> Let their complete content - // conflict with empty text - result.add(1, 0, 0, ConflictState.FIRST_CONFLICTING_RANGE); - result.add(2, 0, theirs.size(), - ConflictState.NEXT_CONFLICTING_RANGE); - } else + // we deleted, they modified + switch (strategy) { + case OURS: + result.add(1, 0, 0, ConflictState.NO_CONFLICT); + break; + case THEIRS: + result.add(2, 0, theirs.size(), + ConflictState.NO_CONFLICT); + break; + default: + // Let their complete content conflict with empty text + result.add(1, 0, 0, + ConflictState.FIRST_CONFLICTING_RANGE); + result.add(2, 0, theirs.size(), + ConflictState.NEXT_CONFLICTING_RANGE); + break; + } + } else { // we deleted, they didn't modify -> Let our deletion win result.add(1, 0, 0, ConflictState.NO_CONFLICT); - } else + } + } else { // we and they deleted -> return a single chunk of nothing result.add(1, 0, 0, ConflictState.NO_CONFLICT); + } return result; } else if (theirs.size() == 0) { EditList oursEdits = diffAlg.diff(cmp, base, ours); if (!oursEdits.isEmpty()) { - // we modified, they deleted -> Let our complete content - // conflict with empty text - result.add(1, 0, ours.size(), - ConflictState.FIRST_CONFLICTING_RANGE); - result.add(2, 0, 0, ConflictState.NEXT_CONFLICTING_RANGE); - } else + // we modified, they deleted + switch (strategy) { + case OURS: + result.add(1, 0, ours.size(), ConflictState.NO_CONFLICT); + break; + case THEIRS: + result.add(2, 0, 0, ConflictState.NO_CONFLICT); + break; + default: + // Let our complete content conflict with empty text + result.add(1, 0, ours.size(), + ConflictState.FIRST_CONFLICTING_RANGE); + result.add(2, 0, 0, ConflictState.NEXT_CONFLICTING_RANGE); + break; + } + } else { // they deleted, we didn't modify -> Let their deletion win result.add(2, 0, 0, ConflictState.NO_CONFLICT); + } return result; } @@ -249,12 +303,26 @@ public final class MergeAlgorithm { // Add the conflict (Only if there is a conflict left to report) if (minBSize > 0 || BSizeDelta != 0) { - result.add(1, oursBeginB + commonPrefix, oursEndB - - commonSuffix, - ConflictState.FIRST_CONFLICTING_RANGE); - result.add(2, theirsBeginB + commonPrefix, theirsEndB - - commonSuffix, - ConflictState.NEXT_CONFLICTING_RANGE); + switch (strategy) { + case OURS: + result.add(1, oursBeginB + commonPrefix, + oursEndB - commonSuffix, + ConflictState.NO_CONFLICT); + break; + case THEIRS: + result.add(2, theirsBeginB + commonPrefix, + theirsEndB - commonSuffix, + ConflictState.NO_CONFLICT); + break; + default: + result.add(1, oursBeginB + commonPrefix, + oursEndB - commonSuffix, + ConflictState.FIRST_CONFLICTING_RANGE); + result.add(2, theirsBeginB + commonPrefix, + theirsEndB - commonSuffix, + ConflictState.NEXT_CONFLICTING_RANGE); + break; + } } // Add the common lines at end of conflict diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java index b011258981..7767662867 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java @@ -37,6 +37,7 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; +import org.eclipse.jgit.annotations.NonNull; import org.eclipse.jgit.attributes.Attributes; import org.eclipse.jgit.diff.DiffAlgorithm; import org.eclipse.jgit.diff.DiffAlgorithm.SupportedAlgorithm; @@ -267,6 +268,13 @@ public class ResolveMerger extends ThreeWayMerger { */ private int inCoreLimit; + /** + * The {@link ContentMergeStrategy} to use for "resolve" and "recursive" + * merges. + */ + @NonNull + private ContentMergeStrategy contentStrategy = ContentMergeStrategy.CONFLICT; + /** * Keeps {@link CheckoutMetadata} for {@link #checkout()} and * {@link #cleanUp()}. @@ -344,6 +352,29 @@ public class ResolveMerger extends ThreeWayMerger { dircache = DirCache.newInCore(); } + /** + * Retrieves the content merge strategy for content conflicts. + * + * @return the {@link ContentMergeStrategy} in effect + * @since 5.12 + */ + @NonNull + public ContentMergeStrategy getContentMergeStrategy() { + return contentStrategy; + } + + /** + * Sets the content merge strategy for content conflicts. + * + * @param strategy + * {@link ContentMergeStrategy} to use + * @since 5.12 + */ + public void setContentMergeStrategy(ContentMergeStrategy strategy) { + contentStrategy = strategy == null ? ContentMergeStrategy.CONFLICT + : strategy; + } + /** {@inheritDoc} */ @Override protected boolean mergeImpl() throws IOException { @@ -654,7 +685,8 @@ public class ResolveMerger extends ThreeWayMerger { add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, EPOCH, 0); add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, EPOCH, 0); unmergedPaths.add(tw.getPathString()); - mergeResults.put(tw.getPathString(), new MergeResult<>(Collections.emptyList())); + mergeResults.put(tw.getPathString(), + new MergeResult<>(Collections.emptyList())); } return true; } @@ -760,6 +792,19 @@ public class ResolveMerger extends ThreeWayMerger { unmergedPaths.add(tw.getPathString()); return true; } else if (!attributes.canBeContentMerged()) { + // File marked as binary + switch (getContentMergeStrategy()) { + case OURS: + keep(ourDce); + return true; + case THEIRS: + DirCacheEntry theirEntry = add(tw.getRawPath(), theirs, + DirCacheEntry.STAGE_0, EPOCH, 0); + addToCheckout(tw.getPathString(), theirEntry, attributes); + return true; + default: + break; + } add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, EPOCH, 0); add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, EPOCH, 0); add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, EPOCH, 0); @@ -774,8 +819,26 @@ public class ResolveMerger extends ThreeWayMerger { return false; } - MergeResult result = contentMerge(base, ours, theirs, - attributes); + MergeResult result = null; + try { + result = contentMerge(base, ours, theirs, attributes, + getContentMergeStrategy()); + } catch (BinaryBlobException e) { + switch (getContentMergeStrategy()) { + case OURS: + keep(ourDce); + return true; + case THEIRS: + DirCacheEntry theirEntry = add(tw.getRawPath(), theirs, + DirCacheEntry.STAGE_0, EPOCH, 0); + addToCheckout(tw.getPathString(), theirEntry, attributes); + return true; + default: + result = new MergeResult<>(Collections.emptyList()); + result.setContainsConflicts(true); + break; + } + } if (ignoreConflicts) { result.setContainsConflicts(false); } @@ -802,9 +865,16 @@ public class ResolveMerger extends ThreeWayMerger { mergeResults.put(tw.getPathString(), result); unmergedPaths.add(tw.getPathString()); } else { - MergeResult result = contentMerge(base, ours, - theirs, attributes); - + // Content merge strategy does not apply to delete-modify + // conflicts! + MergeResult result; + try { + result = contentMerge(base, ours, theirs, attributes, + ContentMergeStrategy.CONFLICT); + } catch (BinaryBlobException e) { + result = new MergeResult<>(Collections.emptyList()); + result.setContainsConflicts(true); + } if (ignoreConflicts) { // In case a conflict is detected the working tree file // is again filled with new content (containing conflict @@ -866,32 +936,26 @@ public class ResolveMerger extends ThreeWayMerger { * @param ours * @param theirs * @param attributes + * @param strategy * * @return the result of the content merge + * @throws BinaryBlobException + * if any of the blobs looks like a binary blob * @throws IOException */ private MergeResult contentMerge(CanonicalTreeParser base, CanonicalTreeParser ours, CanonicalTreeParser theirs, - Attributes attributes) - throws IOException { - RawText baseText; - RawText ourText; - RawText theirsText; - - try { - baseText = base == null ? RawText.EMPTY_TEXT : getRawText( - base.getEntryObjectId(), attributes); - ourText = ours == null ? RawText.EMPTY_TEXT : getRawText( - ours.getEntryObjectId(), attributes); - theirsText = theirs == null ? RawText.EMPTY_TEXT : getRawText( - theirs.getEntryObjectId(), attributes); - } catch (BinaryBlobException e) { - MergeResult r = new MergeResult<>(Collections.emptyList()); - r.setContainsConflicts(true); - return r; - } - return (mergeAlgorithm.merge(RawTextComparator.DEFAULT, baseText, - ourText, theirsText)); + Attributes attributes, ContentMergeStrategy strategy) + throws BinaryBlobException, IOException { + RawText baseText = base == null ? RawText.EMPTY_TEXT + : getRawText(base.getEntryObjectId(), attributes); + RawText ourText = ours == null ? RawText.EMPTY_TEXT + : getRawText(ours.getEntryObjectId(), attributes); + RawText theirsText = theirs == null ? RawText.EMPTY_TEXT + : getRawText(theirs.getEntryObjectId(), attributes); + mergeAlgorithm.setContentMergeStrategy(strategy); + return mergeAlgorithm.merge(RawTextComparator.DEFAULT, baseText, + ourText, theirsText); } private boolean isIndexDirty() { -- cgit v1.2.3 From a9579ba60cd2fd72179dfd8c2c37d389db5ec402 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Tue, 4 May 2021 23:48:56 +0200 Subject: LockFile: create OutputStream only when needed Don't create the stream eagerly in lock(); that may cause JGit to exceed OS or JVM limits on open file descriptors if many locks need to be created, for instance when creating many refs. Instead create the output stream only when one really needs to write something. Bug: 573328 Change-Id: If9441ed40494d46f594a896d34a5c4f56f91ebf4 Signed-off-by: Thomas Wolf --- .../jgit/internal/storage/file/LockFileTest.java | 153 ++++++++++++++++++++- .../org/eclipse/jgit/internal/JGitText.properties | 3 + .../src/org/eclipse/jgit/internal/JGitText.java | 3 + .../jgit/internal/storage/file/LockFile.java | 142 +++++++++++++------ 4 files changed, 257 insertions(+), 44 deletions(-) (limited to 'org.eclipse.jgit/src/org/eclipse/jgit') diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/LockFileTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/LockFileTest.java index 0f93749d9b..509935dfb9 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/LockFileTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/LockFileTest.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2012, GitHub Inc. and others + * Copyright (C) 2012, 2021 GitHub Inc. and others * * This program and the accompanying materials are made available under the * terms of the Eclipse Distribution License v. 1.0 which is available at @@ -9,10 +9,16 @@ */ package org.eclipse.jgit.internal.storage.file; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import java.io.File; +import java.io.OutputStream; +import java.nio.charset.StandardCharsets; + import org.eclipse.jgit.api.Git; import org.eclipse.jgit.api.errors.JGitInternalException; import org.eclipse.jgit.errors.LockFailedException; @@ -49,4 +55,149 @@ public class LockFileTest extends RepositoryTestCase { } } } + + @Test + public void testLockTwice() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + lock.write("other".getBytes(StandardCharsets.US_ASCII)); + lock.commit(); + assertFalse(lock.isLocked()); + checkFile(f, "other"); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + try (OutputStream out = lock.getOutputStream()) { + out.write("second".getBytes(StandardCharsets.US_ASCII)); + } + lock.commit(); + assertFalse(lock.isLocked()); + checkFile(f, "second"); + } + + @Test + public void testLockTwiceUnlock() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + lock.write("other".getBytes(StandardCharsets.US_ASCII)); + lock.unlock(); + assertFalse(lock.isLocked()); + checkFile(f, "content"); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + try (OutputStream out = lock.getOutputStream()) { + out.write("second".getBytes(StandardCharsets.US_ASCII)); + } + lock.commit(); + assertFalse(lock.isLocked()); + checkFile(f, "second"); + } + + @Test + public void testLockWriteTwiceThrows1() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + lock.write("other".getBytes(StandardCharsets.US_ASCII)); + assertThrows(Exception.class, + () -> lock.write("second".getBytes(StandardCharsets.US_ASCII))); + lock.unlock(); + } + + @Test + public void testLockWriteTwiceThrows2() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + try (OutputStream out = lock.getOutputStream()) { + out.write("other".getBytes(StandardCharsets.US_ASCII)); + } + assertThrows(Exception.class, + () -> lock.write("second".getBytes(StandardCharsets.US_ASCII))); + lock.unlock(); + } + + @Test + public void testLockWriteTwiceThrows3() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + lock.write("other".getBytes(StandardCharsets.US_ASCII)); + assertThrows(Exception.class, () -> { + try (OutputStream out = lock.getOutputStream()) { + out.write("second".getBytes(StandardCharsets.US_ASCII)); + } + }); + lock.unlock(); + } + + @Test + public void testLockWriteTwiceThrows4() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + try (OutputStream out = lock.getOutputStream()) { + out.write("other".getBytes(StandardCharsets.US_ASCII)); + } + assertThrows(Exception.class, () -> { + try (OutputStream out = lock.getOutputStream()) { + out.write("second".getBytes(StandardCharsets.US_ASCII)); + } + }); + lock.unlock(); + } + + @Test + public void testLockUnclosedCommitThrows() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + try (OutputStream out = lock.getOutputStream()) { + out.write("other".getBytes(StandardCharsets.US_ASCII)); + assertThrows(Exception.class, () -> lock.commit()); + } + } + + @Test + public void testLockNested() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + assertThrows(IllegalStateException.class, () -> lock.lock()); + assertTrue(lock.isLocked()); + lock.unlock(); + } + + @Test + public void testLockHeld() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + LockFile lock2 = new LockFile(f); + assertFalse(lock2.lock()); + assertFalse(lock2.isLocked()); + assertTrue(lock.isLocked()); + lock.unlock(); + } + + @Test + public void testLockForAppend() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lockForAppend()); + assertTrue(lock.isLocked()); + lock.write("other".getBytes(StandardCharsets.US_ASCII)); + lock.commit(); + assertFalse(lock.isLocked()); + checkFile(f, "contentother"); + } } 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 feef39744b..2fa8713daa 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -415,11 +415,14 @@ listingPacks=Listing packs localObjectsIncomplete=Local objects incomplete. localRefIsMissingObjects=Local ref {0} is missing object(s). localRepository=local repository +lockAlreadyHeld=Lock on {0} already held lockCountMustBeGreaterOrEqual1=lockCount must be >= 1 lockError=lock error: {0} lockFailedRetry=locking {0} failed after {1} retries lockOnNotClosed=Lock on {0} not closed. lockOnNotHeld=Lock on {0} not held. +lockStreamClosed=Output to lock on {0} already closed +lockStreamMultiple=Output to lock on {0} already opened logInconsistentFiletimeDiff={}: inconsistent duration from file timestamps on {}, {}: {} > {}, but diff = {}. Aborting measurement at resolution {}. logLargerFiletimeDiff={}: inconsistent duration from file timestamps on {}, {}: diff = {} > {} (last good value). Aborting measurement. logSmallerFiletime={}: got smaller file timestamp on {}, {}: {} < {}. Aborting measurement at resolution {}. 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 09fe03e065..ab9fc5c9bb 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -444,10 +444,13 @@ public class JGitText extends TranslationBundle { /***/ public String localRefIsMissingObjects; /***/ public String localRepository; /***/ public String lockCountMustBeGreaterOrEqual1; + /***/ public String lockAlreadyHeld; /***/ public String lockError; /***/ public String lockFailedRetry; /***/ public String lockOnNotClosed; /***/ public String lockOnNotHeld; + /***/ public String lockStreamClosed; + /***/ public String lockStreamMultiple; /***/ public String logInconsistentFiletimeDiff; /***/ public String logLargerFiletimeDiff; /***/ public String logSmallerFiletime; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java index 2e0a6da3a4..ab407a6ae9 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java @@ -1,6 +1,6 @@ /* * Copyright (C) 2007, Robin Rosenberg - * Copyright (C) 2006-2008, Shawn O. Pearce and others + * Copyright (C) 2006-2021, Shawn O. Pearce and others * * This program and the accompanying materials are made available under the * terms of the Eclipse Distribution License v. 1.0 which is available at @@ -96,11 +96,15 @@ public class LockFile { private boolean haveLck; - FileOutputStream os; + private FileOutputStream os; private boolean needSnapshot; - boolean fsync; + private boolean fsync; + + private boolean isAppend; + + private boolean written; private FileSnapshot commitSnapshot; @@ -127,6 +131,10 @@ public class LockFile { * does not hold the lock. */ public boolean lock() throws IOException { + if (haveLck) { + throw new IllegalStateException( + MessageFormat.format(JGitText.get().lockAlreadyHeld, ref)); + } FileUtils.mkdirs(lck.getParentFile(), true); try { token = FS.DETECTED.createNewFileAtomic(lck); @@ -134,18 +142,15 @@ public class LockFile { LOG.error(JGitText.get().failedCreateLockFile, lck, e); throw e; } - if (token.isCreated()) { + boolean obtainedLock = token.isCreated(); + if (obtainedLock) { haveLck = true; - try { - os = new FileOutputStream(lck); - } catch (IOException ioe) { - unlock(); - throw ioe; - } + isAppend = false; + written = false; } else { closeToken(); } - return haveLck; + return obtainedLock; } /** @@ -158,12 +163,24 @@ public class LockFile { * does not hold the lock. */ public boolean lockForAppend() throws IOException { - if (!lock()) + if (!lock()) { return false; + } copyCurrentContent(); + isAppend = true; + written = false; return true; } + // For tests only + boolean isLocked() { + return haveLck; + } + + private FileOutputStream getStream() throws IOException { + return new FileOutputStream(lck, isAppend); + } + /** * Copy the current file content into the temporary file. *

@@ -185,32 +202,31 @@ public class LockFile { */ public void copyCurrentContent() throws IOException { requireLock(); - try { + try (FileOutputStream out = getStream()) { try (FileInputStream fis = new FileInputStream(ref)) { if (fsync) { FileChannel in = fis.getChannel(); long pos = 0; long cnt = in.size(); while (0 < cnt) { - long r = os.getChannel().transferFrom(in, pos, cnt); + long r = out.getChannel().transferFrom(in, pos, cnt); pos += r; cnt -= r; } } else { final byte[] buf = new byte[2048]; int r; - while ((r = fis.read(buf)) >= 0) - os.write(buf, 0, r); + while ((r = fis.read(buf)) >= 0) { + out.write(buf, 0, r); + } } + } catch (FileNotFoundException fnfe) { + if (ref.exists()) { + throw fnfe; + } + // Don't worry about a file that doesn't exist yet, it + // conceptually has no current content to copy. } - } catch (FileNotFoundException fnfe) { - if (ref.exists()) { - unlock(); - throw fnfe; - } - // Don't worry about a file that doesn't exist yet, it - // conceptually has no current content to copy. - // } catch (IOException | RuntimeException | Error ioe) { unlock(); throw ioe; @@ -253,18 +269,22 @@ public class LockFile { */ public void write(byte[] content) throws IOException { requireLock(); - try { + try (FileOutputStream out = getStream()) { + if (written) { + throw new IOException(MessageFormat + .format(JGitText.get().lockStreamClosed, ref)); + } if (fsync) { - FileChannel fc = os.getChannel(); + FileChannel fc = out.getChannel(); ByteBuffer buf = ByteBuffer.wrap(content); - while (0 < buf.remaining()) + while (0 < buf.remaining()) { fc.write(buf); + } fc.force(true); } else { - os.write(content); + out.write(content); } - os.close(); - os = null; + written = true; } catch (IOException | RuntimeException | Error ioe) { unlock(); throw ioe; @@ -283,36 +303,68 @@ public class LockFile { public OutputStream getOutputStream() { requireLock(); - final OutputStream out; - if (fsync) - out = Channels.newOutputStream(os.getChannel()); - else - out = os; + if (written || os != null) { + throw new IllegalStateException(MessageFormat + .format(JGitText.get().lockStreamMultiple, ref)); + } return new OutputStream() { + + private OutputStream out; + + private boolean closed; + + private OutputStream get() throws IOException { + if (written) { + throw new IOException(MessageFormat + .format(JGitText.get().lockStreamMultiple, ref)); + } + if (out == null) { + os = getStream(); + if (fsync) { + out = Channels.newOutputStream(os.getChannel()); + } else { + out = os; + } + } + return out; + } + @Override public void write(byte[] b, int o, int n) throws IOException { - out.write(b, o, n); + get().write(b, o, n); } @Override public void write(byte[] b) throws IOException { - out.write(b); + get().write(b); } @Override public void write(int b) throws IOException { - out.write(b); + get().write(b); } @Override public void close() throws IOException { + if (closed) { + return; + } + closed = true; try { - if (fsync) - os.getChannel().force(true); - out.close(); - os = null; + if (written) { + throw new IOException(MessageFormat + .format(JGitText.get().lockStreamClosed, ref)); + } + if (out != null) { + if (fsync) { + os.getChannel().force(true); + } + out.close(); + os = null; + } + written = true; } catch (IOException | RuntimeException | Error ioe) { unlock(); throw ioe; @@ -322,7 +374,7 @@ public class LockFile { } void requireLock() { - if (os == null) { + if (!haveLck) { unlock(); throw new IllegalStateException(MessageFormat.format(JGitText.get().lockOnNotHeld, ref)); } @@ -411,6 +463,8 @@ public class LockFile { try { FileUtils.rename(lck, ref, StandardCopyOption.ATOMIC_MOVE); haveLck = false; + isAppend = false; + written = false; closeToken(); return true; } catch (IOException e) { @@ -497,6 +551,8 @@ public class LockFile { closeToken(); } } + isAppend = false; + written = false; } /** {@inheritDoc} */ -- cgit v1.2.3 From cc07a471dcd83740d4a8efc4a5323d83eb60d2fb Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Fri, 7 May 2021 10:50:58 +0200 Subject: Add TemporaryBuffer.toString(int limit) Change-Id: I8603fcdfd0244088b3b217f002a78e7a646ea205 Signed-off-by: Matthias Sohn --- .../org/eclipse/jgit/dircache/DirCacheCheckout.java | 6 ++---- .../eclipse/jgit/treewalk/WorkingTreeIterator.java | 3 +-- .../src/org/eclipse/jgit/util/TemporaryBuffer.java | 19 +++++++++++++++++++ 3 files changed, 22 insertions(+), 6 deletions(-) (limited to 'org.eclipse.jgit/src/org/eclipse/jgit') diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java index 671475ed47..c904a782db 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java @@ -1610,11 +1610,9 @@ public class DirCacheCheckout { } if (rc != 0) { throw new IOException(new FilterFailedException(rc, - checkoutMetadata.smudgeFilterCommand, - path, + checkoutMetadata.smudgeFilterCommand, path, result.getStdout().toByteArray(MAX_EXCEPTION_TEXT_SIZE), - RawParseUtils.decode(result.getStderr() - .toByteArray(MAX_EXCEPTION_TEXT_SIZE)))); + result.getStderr().toString(MAX_EXCEPTION_TEXT_SIZE))); } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java index 55b7d6279a..0b7c0a9e4b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java @@ -502,8 +502,7 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator { throw new IOException(new FilterFailedException(rc, filterCommand, getEntryPathString(), result.getStdout().toByteArray(MAX_EXCEPTION_TEXT_SIZE), - RawParseUtils.decode(result.getStderr() - .toByteArray(MAX_EXCEPTION_TEXT_SIZE)))); + result.getStderr().toString(MAX_EXCEPTION_TEXT_SIZE))); } return result.getStdout().openInputStreamWithAutoDestroy(); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/TemporaryBuffer.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/TemporaryBuffer.java index 562eb05dd9..fb893a66f0 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/TemporaryBuffer.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/TemporaryBuffer.java @@ -18,6 +18,7 @@ import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.io.UncheckedIOException; import java.util.ArrayList; import org.eclipse.jgit.internal.JGitText; @@ -212,6 +213,24 @@ public abstract class TemporaryBuffer extends OutputStream { return out; } + /** + * Convert first {@code limit} number of bytes of the buffer content to + * String. + * + * @param limit + * the maximum number of bytes to be converted to String + * @return first {@code limit} number of bytes of the buffer content + * converted to String. + * @since 5.12 + */ + public String toString(int limit) { + try { + return RawParseUtils.decode(toByteArray(limit)); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + /** * Convert this buffer's contents into a contiguous byte array. If this size * of the buffer exceeds the limit only return the first {@code limit} bytes -- cgit v1.2.3 From 00386272264f65c41e36406f7c2e9ea6e901276e Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Tue, 4 May 2021 23:48:56 +0200 Subject: LockFile: create OutputStream only when needed Don't create the stream eagerly in lock(); that may cause JGit to exceed OS or JVM limits on open file descriptors if many locks need to be created, for instance when creating many refs. Instead create the output stream only when one really needs to write something. Bug: 573328 Change-Id: If9441ed40494d46f594a896d34a5c4f56f91ebf4 Signed-off-by: Thomas Wolf --- .../jgit/internal/storage/file/LockFileTest.java | 211 +++++++++++++++++---- .../org/eclipse/jgit/internal/JGitText.properties | 7 + .../src/org/eclipse/jgit/internal/JGitText.java | 50 ++--- .../jgit/internal/storage/file/LockFile.java | 163 +++++++++------- 4 files changed, 285 insertions(+), 146 deletions(-) (limited to 'org.eclipse.jgit/src/org/eclipse/jgit') diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/LockFileTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/LockFileTest.java index 3bfd047783..4e123f4c21 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/LockFileTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/LockFileTest.java @@ -1,51 +1,23 @@ /* - * Copyright (C) 2012, GitHub Inc. - * and other copyright owners as documented in the project's IP log. + * Copyright (C) 2012, 2021 GitHub Inc. and others * - * This program and the accompanying materials are made available - * under the terms of the Eclipse Distribution License v1.0 which - * accompanies this distribution, is reproduced below, and is - * available at http://www.eclipse.org/org/documents/edl-v10.php + * 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. * - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or - * without modification, are permitted provided that the following - * conditions are met: - * - * - Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * - * - Redistributions in binary form must reproduce the above - * copyright notice, this list of conditions and the following - * disclaimer in the documentation and/or other materials provided - * with the distribution. - * - * - Neither the name of the Eclipse Foundation, Inc. nor the - * names of its contributors may be used to endorse or promote - * products derived from this software without specific prior - * written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND - * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, - * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES - * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE - * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR - * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, - * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT - * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; - * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER - * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, - * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) - * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF - * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * SPDX-License-Identifier: BSD-3-Clause */ package org.eclipse.jgit.internal.storage.file; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import java.io.File; +import java.io.OutputStream; +import java.nio.charset.StandardCharsets; + import org.eclipse.jgit.api.Git; import org.eclipse.jgit.api.errors.JGitInternalException; import org.eclipse.jgit.errors.LockFailedException; @@ -82,4 +54,167 @@ public class LockFileTest extends RepositoryTestCase { } } } + + @Test + public void testLockTwice() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + lock.write("other".getBytes(StandardCharsets.US_ASCII)); + lock.commit(); + assertFalse(lock.isLocked()); + checkFile(f, "other"); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + try (OutputStream out = lock.getOutputStream()) { + out.write("second".getBytes(StandardCharsets.US_ASCII)); + } + lock.commit(); + assertFalse(lock.isLocked()); + checkFile(f, "second"); + } + + @Test + public void testLockTwiceUnlock() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + lock.write("other".getBytes(StandardCharsets.US_ASCII)); + lock.unlock(); + assertFalse(lock.isLocked()); + checkFile(f, "content"); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + try (OutputStream out = lock.getOutputStream()) { + out.write("second".getBytes(StandardCharsets.US_ASCII)); + } + lock.commit(); + assertFalse(lock.isLocked()); + checkFile(f, "second"); + } + + @Test + public void testLockWriteTwiceThrows1() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + lock.write("other".getBytes(StandardCharsets.US_ASCII)); + try { + lock.write("second".getBytes(StandardCharsets.US_ASCII)); + fail(); + } catch (Exception e) { + // expected + } + lock.unlock(); + } + + @Test + public void testLockWriteTwiceThrows2() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + try (OutputStream out = lock.getOutputStream()) { + out.write("other".getBytes(StandardCharsets.US_ASCII)); + } + try { + lock.write("second".getBytes(StandardCharsets.US_ASCII)); + fail(); + } catch (Exception e) { + // expected + } + lock.unlock(); + } + + @Test + public void testLockWriteTwiceThrows3() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + lock.write("other".getBytes(StandardCharsets.US_ASCII)); + try (OutputStream out = lock.getOutputStream()) { + out.write("second".getBytes(StandardCharsets.US_ASCII)); + fail(); + } catch (Exception e) { + // expected + } + lock.unlock(); + } + + @Test + public void testLockWriteTwiceThrows4() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + try (OutputStream out = lock.getOutputStream()) { + out.write("other".getBytes(StandardCharsets.US_ASCII)); + } + try (OutputStream out = lock.getOutputStream()) { + out.write("second".getBytes(StandardCharsets.US_ASCII)); + fail(); + } catch (Exception e) { + // expected + } + lock.unlock(); + } + + @Test + public void testLockUnclosedCommitThrows() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + try (OutputStream out = lock.getOutputStream()) { + out.write("other".getBytes(StandardCharsets.US_ASCII)); + lock.commit(); + fail(); + } catch (Exception e) { + // expected + } + } + + @Test + public void testLockNested() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + try { + lock.lock(); + fail(); + } catch (IllegalStateException e) { + // expected + } + assertTrue(lock.isLocked()); + lock.unlock(); + } + + @Test + public void testLockHeld() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lock()); + assertTrue(lock.isLocked()); + LockFile lock2 = new LockFile(f); + assertFalse(lock2.lock()); + assertFalse(lock2.isLocked()); + assertTrue(lock.isLocked()); + lock.unlock(); + } + + @Test + public void testLockForAppend() throws Exception { + File f = writeTrashFile("somefile", "content"); + LockFile lock = new LockFile(f); + assertTrue(lock.lockForAppend()); + assertTrue(lock.isLocked()); + lock.write("other".getBytes(StandardCharsets.US_ASCII)); + lock.commit(); + assertFalse(lock.isLocked()); + checkFile(f, "contentother"); + } } 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 9e483a6c61..1888a87c0a 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -408,11 +408,18 @@ listingPacks=Listing packs localObjectsIncomplete=Local objects incomplete. localRefIsMissingObjects=Local ref {0} is missing object(s). localRepository=local repository +lockAlreadyHeld=Lock on {0} already held lockCountMustBeGreaterOrEqual1=lockCount must be >= 1 lockError=lock error: {0} lockFailedRetry=locking {0} failed after {1} retries lockOnNotClosed=Lock on {0} not closed. lockOnNotHeld=Lock on {0} not held. +lockStreamClosed=Output to lock on {0} already closed +lockStreamMultiple=Output to lock on {0} already opened +logInconsistentFiletimeDiff={}: inconsistent duration from file timestamps on {}, {}: {} > {}, but diff = {}. Aborting measurement at resolution {}. +logLargerFiletimeDiff={}: inconsistent duration from file timestamps on {}, {}: diff = {} > {} (last good value). Aborting measurement. +logSmallerFiletime={}: got smaller file timestamp on {}, {}: {} < {}. Aborting measurement at resolution {}. +logXDGConfigHomeInvalid=Environment variable XDG_CONFIG_HOME contains an invalid path {} maxCountMustBeNonNegative=max count must be >= 0 mergeConflictOnNonNoteEntries=Merge conflict on non-note entries: base = {0}, ours = {1}, theirs = {2} mergeConflictOnNotes=Merge conflict on note {0}. base = {1}, ours = {2}, theirs = {2} 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 535306967b..9d8824467d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -1,45 +1,12 @@ /* * Copyright (C) 2010, 2013 Sasa Zivkov - * Copyright (C) 2012, Research In Motion Limited - * and other copyright owners as documented in the project's IP log. + * Copyright (C) 2012, 2021 Research In Motion Limited and others * - * This program and the accompanying materials are made available - * under the terms of the Eclipse Distribution License v1.0 which - * accompanies this distribution, is reproduced below, and is - * available at http://www.eclipse.org/org/documents/edl-v10.php + * 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. * - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or - * without modification, are permitted provided that the following - * conditions are met: - * - * - Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * - * - Redistributions in binary form must reproduce the above - * copyright notice, this list of conditions and the following - * disclaimer in the documentation and/or other materials provided - * with the distribution. - * - * - Neither the name of the Eclipse Foundation, Inc. nor the - * names of its contributors may be used to endorse or promote - * products derived from this software without specific prior - * written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND - * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, - * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES - * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE - * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR - * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, - * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT - * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; - * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER - * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, - * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) - * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF - * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * SPDX-License-Identifier: BSD-3-Clause */ package org.eclipse.jgit.internal; @@ -470,10 +437,17 @@ public class JGitText extends TranslationBundle { /***/ public String localRefIsMissingObjects; /***/ public String localRepository; /***/ public String lockCountMustBeGreaterOrEqual1; + /***/ public String lockAlreadyHeld; /***/ public String lockError; /***/ public String lockFailedRetry; /***/ public String lockOnNotClosed; /***/ public String lockOnNotHeld; + /***/ public String lockStreamClosed; + /***/ public String lockStreamMultiple; + /***/ public String logInconsistentFiletimeDiff; + /***/ public String logLargerFiletimeDiff; + /***/ public String logSmallerFiletime; + /***/ public String logXDGConfigHomeInvalid; /***/ public String maxCountMustBeNonNegative; /***/ public String mergeConflictOnNonNoteEntries; /***/ public String mergeConflictOnNotes; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java index 6bcd7c0703..f57581a299 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java @@ -1,45 +1,12 @@ /* * Copyright (C) 2007, Robin Rosenberg - * Copyright (C) 2006-2008, Shawn O. Pearce - * and other copyright owners as documented in the project's IP log. + * Copyright (C) 2006-2021, Shawn O. Pearce and others * - * This program and the accompanying materials are made available - * under the terms of the Eclipse Distribution License v1.0 which - * accompanies this distribution, is reproduced below, and is - * available at http://www.eclipse.org/org/documents/edl-v10.php + * 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. * - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or - * without modification, are permitted provided that the following - * conditions are met: - * - * - Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * - * - Redistributions in binary form must reproduce the above - * copyright notice, this list of conditions and the following - * disclaimer in the documentation and/or other materials provided - * with the distribution. - * - * - Neither the name of the Eclipse Foundation, Inc. nor the - * names of its contributors may be used to endorse or promote - * products derived from this software without specific prior - * written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND - * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, - * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES - * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE - * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR - * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, - * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT - * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; - * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER - * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, - * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) - * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF - * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * SPDX-License-Identifier: BSD-3-Clause */ package org.eclipse.jgit.internal.storage.file; @@ -129,11 +96,15 @@ public class LockFile { private boolean haveLck; - FileOutputStream os; + private FileOutputStream os; private boolean needSnapshot; - boolean fsync; + private boolean fsync; + + private boolean isAppend; + + private boolean written; private FileSnapshot commitSnapshot; @@ -160,6 +131,10 @@ public class LockFile { * does not hold the lock. */ public boolean lock() throws IOException { + if (haveLck) { + throw new IllegalStateException( + MessageFormat.format(JGitText.get().lockAlreadyHeld, ref)); + } FileUtils.mkdirs(lck.getParentFile(), true); try { token = FS.DETECTED.createNewFileAtomic(lck); @@ -167,18 +142,15 @@ public class LockFile { LOG.error(JGitText.get().failedCreateLockFile, lck, e); throw e; } - if (token.isCreated()) { + boolean obtainedLock = token.isCreated(); + if (obtainedLock) { haveLck = true; - try { - os = new FileOutputStream(lck); - } catch (IOException ioe) { - unlock(); - throw ioe; - } + isAppend = false; + written = false; } else { closeToken(); } - return haveLck; + return obtainedLock; } /** @@ -191,12 +163,24 @@ public class LockFile { * does not hold the lock. */ public boolean lockForAppend() throws IOException { - if (!lock()) + if (!lock()) { return false; + } copyCurrentContent(); + isAppend = true; + written = false; return true; } + // For tests only + boolean isLocked() { + return haveLck; + } + + private FileOutputStream getStream() throws IOException { + return new FileOutputStream(lck, isAppend); + } + /** * Copy the current file content into the temporary file. *

@@ -218,32 +202,31 @@ public class LockFile { */ public void copyCurrentContent() throws IOException { requireLock(); - try { + try (FileOutputStream out = getStream()) { try (FileInputStream fis = new FileInputStream(ref)) { if (fsync) { FileChannel in = fis.getChannel(); long pos = 0; long cnt = in.size(); while (0 < cnt) { - long r = os.getChannel().transferFrom(in, pos, cnt); + long r = out.getChannel().transferFrom(in, pos, cnt); pos += r; cnt -= r; } } else { final byte[] buf = new byte[2048]; int r; - while ((r = fis.read(buf)) >= 0) - os.write(buf, 0, r); + while ((r = fis.read(buf)) >= 0) { + out.write(buf, 0, r); } } } catch (FileNotFoundException fnfe) { if (ref.exists()) { - unlock(); throw fnfe; } // Don't worry about a file that doesn't exist yet, it // conceptually has no current content to copy. - // + } } catch (IOException | RuntimeException | Error ioe) { unlock(); throw ioe; @@ -286,18 +269,22 @@ public class LockFile { */ public void write(byte[] content) throws IOException { requireLock(); - try { + try (FileOutputStream out = getStream()) { + if (written) { + throw new IOException(MessageFormat + .format(JGitText.get().lockStreamClosed, ref)); + } if (fsync) { - FileChannel fc = os.getChannel(); + FileChannel fc = out.getChannel(); ByteBuffer buf = ByteBuffer.wrap(content); - while (0 < buf.remaining()) + while (0 < buf.remaining()) { fc.write(buf); + } fc.force(true); } else { - os.write(content); + out.write(content); } - os.close(); - os = null; + written = true; } catch (IOException | RuntimeException | Error ioe) { unlock(); throw ioe; @@ -316,36 +303,68 @@ public class LockFile { public OutputStream getOutputStream() { requireLock(); - final OutputStream out; - if (fsync) + if (written || os != null) { + throw new IllegalStateException(MessageFormat + .format(JGitText.get().lockStreamMultiple, ref)); + } + + return new OutputStream() { + + private OutputStream out; + + private boolean closed; + + private OutputStream get() throws IOException { + if (written) { + throw new IOException(MessageFormat + .format(JGitText.get().lockStreamMultiple, ref)); + } + if (out == null) { + os = getStream(); + if (fsync) { out = Channels.newOutputStream(os.getChannel()); - else + } else { out = os; + } + } + return out; + } - return new OutputStream() { @Override public void write(byte[] b, int o, int n) throws IOException { - out.write(b, o, n); + get().write(b, o, n); } @Override public void write(byte[] b) throws IOException { - out.write(b); + get().write(b); } @Override public void write(int b) throws IOException { - out.write(b); + get().write(b); } @Override public void close() throws IOException { + if (closed) { + return; + } + closed = true; try { - if (fsync) + if (written) { + throw new IOException(MessageFormat + .format(JGitText.get().lockStreamClosed, ref)); + } + if (out != null) { + if (fsync) { os.getChannel().force(true); + } out.close(); os = null; + } + written = true; } catch (IOException | RuntimeException | Error ioe) { unlock(); throw ioe; @@ -355,7 +374,7 @@ public class LockFile { } void requireLock() { - if (os == null) { + if (!haveLck) { unlock(); throw new IllegalStateException(MessageFormat.format(JGitText.get().lockOnNotHeld, ref)); } @@ -444,6 +463,8 @@ public class LockFile { try { FileUtils.rename(lck, ref, StandardCopyOption.ATOMIC_MOVE); haveLck = false; + isAppend = false; + written = false; closeToken(); return true; } catch (IOException e) { @@ -530,6 +551,8 @@ public class LockFile { closeToken(); } } + isAppend = false; + written = false; } /** {@inheritDoc} */ -- cgit v1.2.3 From 70e250c3564cd9a17479fecc0d58d1700806c056 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Tue, 11 May 2021 18:36:05 +0200 Subject: Fix formatting which was broken in 00386272 Change-Id: I10a3e2b117e790f64386a8e9e7663db8e59230d9 Signed-off-by: Matthias Sohn --- .../jgit/internal/storage/file/LockFile.java | 27 +++++++++++----------- 1 file changed, 13 insertions(+), 14 deletions(-) (limited to 'org.eclipse.jgit/src/org/eclipse/jgit') diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java index f57581a299..78262e9773 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java @@ -218,14 +218,14 @@ public class LockFile { int r; while ((r = fis.read(buf)) >= 0) { out.write(buf, 0, r); + } } - } - } catch (FileNotFoundException fnfe) { - if (ref.exists()) { - throw fnfe; - } - // Don't worry about a file that doesn't exist yet, it - // conceptually has no current content to copy. + } catch (FileNotFoundException fnfe) { + if (ref.exists()) { + throw fnfe; + } + // Don't worry about a file that doesn't exist yet, it + // conceptually has no current content to copy. } } catch (IOException | RuntimeException | Error ioe) { unlock(); @@ -322,17 +322,16 @@ public class LockFile { if (out == null) { os = getStream(); if (fsync) { - out = Channels.newOutputStream(os.getChannel()); + out = Channels.newOutputStream(os.getChannel()); } else { - out = os; + out = os; } } return out; } @Override - public void write(byte[] b, int o, int n) - throws IOException { + public void write(byte[] b, int o, int n) throws IOException { get().write(b, o, n); } @@ -359,10 +358,10 @@ public class LockFile { } if (out != null) { if (fsync) { - os.getChannel().force(true); + os.getChannel().force(true); } - out.close(); - os = null; + out.close(); + os = null; } written = true; } catch (IOException | RuntimeException | Error ioe) { -- cgit v1.2.3 From 37436cc9334ed7bf49bf1d297ae4f90d5f141068 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Tue, 11 May 2021 18:37:37 +0200 Subject: Remove texts which were added by mistake in 00386272 Change-Id: Iaed25dac0bc9af8f3fda6138a5f9fe553bff5d39 Signed-off-by: Matthias Sohn --- .../resources/org/eclipse/jgit/internal/JGitText.properties | 4 ---- org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java | 4 ---- 2 files changed, 8 deletions(-) (limited to 'org.eclipse.jgit/src/org/eclipse/jgit') 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 1888a87c0a..70a0d8095f 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -416,10 +416,6 @@ lockOnNotClosed=Lock on {0} not closed. lockOnNotHeld=Lock on {0} not held. lockStreamClosed=Output to lock on {0} already closed lockStreamMultiple=Output to lock on {0} already opened -logInconsistentFiletimeDiff={}: inconsistent duration from file timestamps on {}, {}: {} > {}, but diff = {}. Aborting measurement at resolution {}. -logLargerFiletimeDiff={}: inconsistent duration from file timestamps on {}, {}: diff = {} > {} (last good value). Aborting measurement. -logSmallerFiletime={}: got smaller file timestamp on {}, {}: {} < {}. Aborting measurement at resolution {}. -logXDGConfigHomeInvalid=Environment variable XDG_CONFIG_HOME contains an invalid path {} maxCountMustBeNonNegative=max count must be >= 0 mergeConflictOnNonNoteEntries=Merge conflict on non-note entries: base = {0}, ours = {1}, theirs = {2} mergeConflictOnNotes=Merge conflict on note {0}. base = {1}, ours = {2}, theirs = {2} 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 9d8824467d..37a7c7d93c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -444,10 +444,6 @@ public class JGitText extends TranslationBundle { /***/ public String lockOnNotHeld; /***/ public String lockStreamClosed; /***/ public String lockStreamMultiple; - /***/ public String logInconsistentFiletimeDiff; - /***/ public String logLargerFiletimeDiff; - /***/ public String logSmallerFiletime; - /***/ public String logXDGConfigHomeInvalid; /***/ public String maxCountMustBeNonNegative; /***/ public String mergeConflictOnNonNoteEntries; /***/ public String mergeConflictOnNotes; -- cgit v1.2.3