Merge branch 'stable-5.11'

* stable-5.11:
  Refactor CommitCommand to improve readability
  CommitCommand: fix formatting
  CommitCommand: remove unncessary comment
  Ensure post-commit hook is called after index lock was released
  sshd: try all configured signature algorithms for a key
  sshd: modernize ssh config file parsing
  sshd: implement ssh config PubkeyAcceptedAlgorithms

Change-Id: Ic3235ffd84c9d7537a1fe5ff4f216578e6e26724
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

4 files changed, 166 insertions, 101 deletions

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..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;
@@ -67,6 +70,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 +83,9 @@ import org.eclipse.jgit.util.ChangeIdUtil;
  *      >Git documentation about Commit</a>
  */
 public class CommitCommand extends GitCommand<RevCommit> {
+	private static final Logger log = LoggerFactory
+			.getLogger(CommitCommand.class);
+
 	private PersonIdent author;
 
 	private PersonIdent committer;
@@ -173,8 +181,7 @@ public class CommitCommand extends GitCommand<RevCommit> {
 
 			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
@@ -212,7 +219,7 @@ public class CommitCommand extends GitCommand<RevCommit> {
 						.setCommitMessage(message).call();
 			}
 
-			// lock the index
+			RevCommit revCommit;
 			DirCache index = repo.lockDirCache();
 			try (ObjectInserter odi = repo.newObjectInserter()) {
 				if (!only.isEmpty())
@@ -226,100 +233,37 @@ public class CommitCommand extends GitCommand<RevCommit> {
 				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);
 
-				RevCommit 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);
-					}
-					Hooks.postCommit(repo,
-							hookOutRedirect.get(PostCommitHook.NAME),
-							hookErrRedirect.get(PostCommitHook.NAME)).call();
-
-					return revCommit;
-				}
-				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();
 			}
+			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) {
@@ -328,6 +272,89 @@ public class CommitCommand extends GitCommand<RevCommit> {
 		}
 	}
 
+	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())
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 3eef49b1ca..09fe03e065 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -558,6 +558,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;
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<String, String> 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<String, String> options;
 
 		private Map<String, List<String>> multiOptions;
 
 		private Map<String, List<String>> 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<String> values = listOptions != null ? listOptions.get(key)
+				List<String> 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<String> getValues(String key) {
-			List<String> values = listOptions != null ? listOptions.get(key)
+			String k = toKey(key);
+			List<String> 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<String> values = multiOptions.get(key);
+				List<String> 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<String> items = multiOptions.get(key);
+				List<String> 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) {
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";