Don't log error if system git config does not exist

- enhance FS.readPipe to throw an exception if the external command
fails to enable the caller to handle the command failure
- reduce log level to warning if system git config does not exist
- improve log message 

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

org.eclipse.jgit.test/tst/org/eclipse/jgit/util/FSTest.java

@@ -50,11 +50,13 @@ import static org.junit.Assume.assumeTrue;
 
 import java.io.File;
 import java.io.IOException;
+import java.nio.charset.Charset;
 import java.nio.file.Files;
 import java.nio.file.attribute.PosixFileAttributeView;
 import java.nio.file.attribute.PosixFilePermission;
 import java.util.Set;
 
+import org.eclipse.jgit.errors.CommandFailedException;
 import org.eclipse.jgit.junit.RepositoryTestCase;
 import org.junit.After;
 import org.junit.Assume;
@@ -164,4 +166,15 @@ public class FSTest {
 				.readAttributes().permissions();
 	}
 
+	@Test(expected = CommandFailedException.class)
+	public void testReadPipePosixCommandFailure()
+			throws CommandFailedException {
+		FS fs = FS.DETECTED.newInstance();
+		assumeTrue(fs instanceof FS_POSIX);
+
+		String r = FS.readPipe(fs.userHome(),
+				new String[] { "bash", "--login", "-c", "foobar" },
+				Charset.defaultCharset().name());
+		System.out.println(r);
+	}
 }

org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties

@@ -264,7 +264,7 @@ exceptionCaughtDuringExecutionOfResetCommand=Exception caught during execution o
 exceptionCaughtDuringExecutionOfRevertCommand=Exception caught during execution of revert command. {0}
 exceptionCaughtDuringExecutionOfRmCommand=Exception caught during execution of rm command
 exceptionCaughtDuringExecutionOfTagCommand=Exception caught during execution of tag command
-exceptionCaughtDuringExcecutionOfCommand=Exception caught during execution of command {0} in {1}
+exceptionCaughtDuringExcecutionOfCommand=Exception caught during execution of command ''{0}'' in ''{1}'', return code ''{2}'', error message ''{3}''
 exceptionHookExecutionInterrupted=Execution of "{0}" hook interrupted.
 exceptionOccurredDuringAddingOfOptionToALogCommand=Exception occurred during adding of {0} as option to a Log command
 exceptionOccurredDuringReadingOfGIT_DIR=Exception occurred during reading of $GIT_DIR/{0}. {1}

org.eclipse.jgit/src/org/eclipse/jgit/errors/CommandFailedException.java

@@ -0,0 +1,87 @@
+/*
+ * Copyright (C) 2016, Matthias Sohn <matthias.sohn@sap.com>
+ * and other copyright owners as documented in the project's IP log.
+ *
+ * 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
+ *
+ * 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.
+ */
+package org.eclipse.jgit.errors;
+
+/**
+ * Thrown when an external command failed
+ *
+ * @since 4.5
+ */
+public class CommandFailedException extends Exception {
+
+	private static final long serialVersionUID = 1L;
+
+	private int returnCode;
+
+	/**
+	 * @param returnCode
+	 *            return code returned by the command
+	 * @param message
+	 *            error message
+	 */
+	public CommandFailedException(int returnCode, String message) {
+		super(message);
+		this.returnCode = returnCode;
+	}
+
+	/**
+	 * @param returnCode
+	 *            return code returned by the command
+	 * @param message
+	 *            error message
+	 * @param cause
+	 *            exception causing this exception
+	 */
+	public CommandFailedException(int returnCode, String message,
+			Throwable cause) {
+		super(message, cause);
+		this.returnCode = returnCode;
+	}
+
+	/**
+	 * @return return code returned by the command
+	 */
+	public int getReturnCode() {
+		return returnCode;
+	}
+}

org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java

@@ -64,9 +64,11 @@ import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
 
 import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.api.errors.JGitInternalException;
+import org.eclipse.jgit.errors.CommandFailedException;
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.Repository;
@@ -453,9 +455,12 @@ public abstract class FS {
 	 *            to be used to parse the command's output
 	 * @return the one-line output of the command or {@code null} if there is
 	 *         none
+	 * @throws CommandFailedException
+	 *             thrown when the command failed (return code was non-zero)
 	 */
 	@Nullable
-	protected static String readPipe(File dir, String[] command, String encoding) {
+	protected static String readPipe(File dir, String[] command,
+			String encoding) throws CommandFailedException {
 		return readPipe(dir, command, encoding, null);
 	}
 
@@ -473,10 +478,14 @@ public abstract class FS {
 	 *            current process
 	 * @return the one-line output of the command or {@code null} if there is
 	 *         none
+	 * @throws CommandFailedException
+	 *             thrown when the command failed (return code was non-zero)
 	 * @since 4.0
 	 */
 	@Nullable
-	protected static String readPipe(File dir, String[] command, String encoding, Map<String, String> env) {
+	protected static String readPipe(File dir, String[] command,
+			String encoding, Map<String, String> env)
+			throws CommandFailedException {
 		final boolean debug = LOG.isDebugEnabled();
 		try {
 			if (debug) {
@@ -512,11 +521,14 @@ public abstract class FS {
 					gobbler.join();
 					if (rc == 0 && !gobbler.fail.get()) {
 						return r;
+					} else {
+						if (debug) {
+							LOG.debug("readpipe rc=" + rc); //$NON-NLS-1$
+						}
+						throw new CommandFailedException(rc,
+								gobbler.errorMessage.get(),
+								gobbler.exception.get());
 					}
-					if (debug) {
-						LOG.debug("readpipe rc=" + rc); //$NON-NLS-1$
-					}
-					break;
 				} catch (InterruptedException ie) {
 					// Stop bothering me, I have a zombie to reap.
 				}
@@ -535,6 +547,8 @@ public abstract class FS {
 		private final String desc;
 		private final String dir;
 		final AtomicBoolean fail = new AtomicBoolean();
+		final AtomicReference<String> errorMessage = new AtomicReference<>();
+		final AtomicReference<Throwable> exception = new AtomicReference<>();
 
 		GobblerThread(Process p, String[] command, File dir) {
 			this.p = p;
@@ -542,6 +556,7 @@ public abstract class FS {
 			this.dir = Objects.toString(dir);
 		}
 
+		@Override
 		public void run() {
 			StringBuilder err = new StringBuilder();
 			try (InputStream is = p.getErrorStream()) {
@@ -551,22 +566,26 @@ public abstract class FS {
 				}
 			} catch (IOException e) {
 				if (p.exitValue() != 0) {
-					logError(e);
+					setError(e, e.getMessage());
 					fail.set(true);
 				} else {
-					// ignore. git terminated faster and stream was just closed
+					// ignore. command terminated faster and stream was just closed
 				}
 			} finally {
 				if (err.length() > 0) {
-					LOG.error(err.toString());
+					setError(null, err.toString());
+					if (p.exitValue() != 0) {
+						fail.set(true);
+					}
 				}
 			}
 		}
 
-		private void logError(Throwable t) {
-			String msg = MessageFormat.format(
-					JGitText.get().exceptionCaughtDuringExcecutionOfCommand, desc, dir);
-			LOG.error(msg, t);
+		private void setError(IOException e, String message) {
+			exception.set(e);
+			errorMessage.set(MessageFormat.format(
+					JGitText.get().exceptionCaughtDuringExcecutionOfCommand,
+					desc, dir, Integer.valueOf(p.exitValue()), message));
 		}
 	}
 
@@ -589,10 +608,17 @@ public abstract class FS {
 		}
 
 		// Bug 480782: Check if the discovered git executable is JGit CLI
-		String v = readPipe(gitExe.getParentFile(),
+		String v;
+		try {
+			v = readPipe(gitExe.getParentFile(),
 				new String[] { "git", "--version" }, //$NON-NLS-1$ //$NON-NLS-2$
 				Charset.defaultCharset().name());
-		if (v != null && v.startsWith("jgit")) { //$NON-NLS-1$
+		} catch (CommandFailedException e) {
+			LOG.warn(e.getMessage());
+			return null;
+		}
+		if (StringUtils.isEmptyOrNull(v)
+				|| (v != null && v.startsWith("jgit"))) { //$NON-NLS-1$
 			return null;
 		}
 
@@ -601,9 +627,15 @@ public abstract class FS {
 		Map<String, String> env = new HashMap<>();
 		env.put("GIT_EDITOR", "echo"); //$NON-NLS-1$ //$NON-NLS-2$
 
-		String w = readPipe(gitExe.getParentFile(),
+		String w;
+		try {
+			w = readPipe(gitExe.getParentFile(),
 				new String[] { "git", "config", "--system", "--edit" }, //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$
 				Charset.defaultCharset().name(), env);
+		} catch (CommandFailedException e) {
+			LOG.warn(e.getMessage());
+			return null;
+		}
 		if (StringUtils.isEmptyOrNull(w)) {
 			return null;
 		}

org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java

@@ -57,8 +57,11 @@ import java.util.List;
 import java.util.Set;
 
 import org.eclipse.jgit.api.errors.JGitInternalException;
+import org.eclipse.jgit.errors.CommandFailedException;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.Repository;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Base FS for POSIX based systems
@@ -66,6 +69,8 @@ import org.eclipse.jgit.lib.Repository;
  * @since 3.0
  */
 public class FS_POSIX extends FS {
+	private final static Logger LOG = LoggerFactory.getLogger(FS_POSIX.class);
+
 	private static final int DEFAULT_UMASK = 0022;
 	private volatile int umask = -1;
 
@@ -144,11 +149,18 @@ public class FS_POSIX extends FS {
 					// On MacOSX, PATH is shorter when Eclipse is launched from the
 					// Finder than from a terminal. Therefore try to launch bash as a
 					// login shell and search using that.
-					String w = readPipe(userHome(),
+					String w;
+					try {
+						w = readPipe(userHome(),
 							new String[]{"bash", "--login", "-c", "which git"}, // //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$
 							Charset.defaultCharset().name());
-					if (!StringUtils.isEmptyOrNull(w))
+					} catch (CommandFailedException e) {
+						LOG.warn(e.getMessage());
+						return null;
+					}
+					if (!StringUtils.isEmptyOrNull(w)) {
 						gitExe = new File(w);
+					}
 				}
 			}
 		}

org.eclipse.jgit/src/org/eclipse/jgit/util/FS_Win32.java

@@ -51,6 +51,10 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 
+import org.eclipse.jgit.errors.CommandFailedException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 
 /**
  * FS implementation for Windows
@@ -58,6 +62,7 @@ import java.util.List;
  * @since 3.0
  */
 public class FS_Win32 extends FS {
+	private final static Logger LOG = LoggerFactory.getLogger(FS_Win32.class);
 
 	private volatile Boolean supportSymlinks;
 
@@ -113,12 +118,19 @@ public class FS_Win32 extends FS {
 			if (searchPath(path, "bash.exe") != null) { //$NON-NLS-1$
 				// This isn't likely to work, but its worth trying:
 				// If bash is in $PATH, git should also be in $PATH.
-				String w = readPipe(userHome(),
+				String w;
+				try {
+					w = readPipe(userHome(),
 						new String[]{"bash", "--login", "-c", "which git"}, // //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$
 						Charset.defaultCharset().name());
-				if (!StringUtils.isEmptyOrNull(w))
+				} catch (CommandFailedException e) {
+					LOG.warn(e.getMessage());
+					return null;
+				}
+				if (!StringUtils.isEmptyOrNull(w)) {
 					// The path may be in cygwin/msys notation so resolve it right away
 					gitExe = resolve(null, w);
+				}
 			}
 		}
 

org.eclipse.jgit/src/org/eclipse/jgit/util/FS_Win32_Cygwin.java

@@ -54,8 +54,11 @@ import java.util.Arrays;
 import java.util.List;
 
 import org.eclipse.jgit.api.errors.JGitInternalException;
+import org.eclipse.jgit.errors.CommandFailedException;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.Repository;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * FS implementation for Cygwin on Windows
@@ -63,6 +66,9 @@ import org.eclipse.jgit.lib.Repository;
  * @since 3.0
  */
 public class FS_Win32_Cygwin extends FS_Win32 {
+	private final static Logger LOG = LoggerFactory
+			.getLogger(FS_Win32_Cygwin.class);
+
 	private static String cygpath;
 
 	/**
@@ -107,11 +113,18 @@ public class FS_Win32_Cygwin extends FS_Win32 {
 	public File resolve(final File dir, final String pn) {
 		String useCygPath = System.getProperty("jgit.usecygpath"); //$NON-NLS-1$
 		if (useCygPath != null && useCygPath.equals("true")) { //$NON-NLS-1$
-			String w = readPipe(dir, //
+			String w;
+			try {
+				w = readPipe(dir, //
 					new String[] { cygpath, "--windows", "--absolute", pn }, // //$NON-NLS-1$ //$NON-NLS-2$
 					"UTF-8"); //$NON-NLS-1$
-			if (w != null)
+			} catch (CommandFailedException e) {
+				LOG.warn(e.getMessage());
+				return null;
+			}
+			if (!StringUtils.isEmptyOrNull(w)) {
 				return new File(w);
+			}
 		}
 		return super.resolve(dir, pn);
 	}