FS_POSIX: Rework umask detection to make it settable

Avoid always calling `sh -c umask` on startup, instead deferring
the invocation until the first time a working tree file needs to
use the execute bit. This allows servers using bare repos to avoid
a costly fork+exec for a value that is never used.

Store the umask as an int instead of two Boolean. This is slightly
smaller memory (one int vs. two references) and makes it easier for
an application to force setting the umask to a value that overrides
whatever the shell told JGit.

Simplify the code to bail by returning early when canExecute is
false, which is the common case for working tree files.

Change-Id: Ie713647615bc5bdf5d71b731a6748c28ea21c900

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

@@ -46,15 +46,10 @@ package org.eclipse.jgit.util;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
-import static org.junit.Assume.assumeFalse;
-import static org.junit.Assume.assumeNotNull;
 import static org.junit.Assume.assumeTrue;
 
-import java.io.BufferedReader;
 import java.io.File;
 import java.io.IOException;
-import java.io.InputStreamReader;
-import java.nio.charset.Charset;
 import java.nio.file.Files;
 import java.nio.file.attribute.PosixFileAttributeView;
 import java.nio.file.attribute.PosixFilePermission;
@@ -123,29 +118,15 @@ public class FSJava7Test {
 
 	@Test
 	public void testExecutableAttributes() throws Exception {
-		FS fs = FS.DETECTED;
+		FS fs = FS.DETECTED.newInstance();
 		// If this assumption fails the test is halted and ignored.
 		assumeTrue(fs instanceof FS_POSIX);
+		((FS_POSIX) fs).setUmask(0022);
 
 		File f = new File(trash, "bla");
 		assertTrue(f.createNewFile());
 		assertFalse(fs.canExecute(f));
 
-		String umask = readUmask();
-		assumeNotNull(umask);
-
-		char others = umask.charAt(umask.length() - 1);
-
-		boolean badUmask;
-		if (others != '0' && others != '2' && others != '4' && others != '6') {
-			// umask is set in the way that "others" can not "execute" => git
-			// CLI will not set "execute" attribute for "others", so we also
-			// don't care
-			badUmask = true;
-		} else {
-			badUmask = false;
-		}
-
 		Set<PosixFilePermission> permissions = readPermissions(f);
 		assertTrue(!permissions.contains(PosixFilePermission.OTHERS_EXECUTE));
 		assertTrue(!permissions.contains(PosixFilePermission.GROUP_EXECUTE));
@@ -158,27 +139,21 @@ public class FSJava7Test {
 				permissions.contains(PosixFilePermission.OWNER_EXECUTE));
 		assertTrue("'group' execute permission not set",
 				permissions.contains(PosixFilePermission.GROUP_EXECUTE));
-		if (badUmask) {
+		assertTrue("'others' execute permission not set",
-			assertFalse("'others' execute permission set",
+				permissions.contains(PosixFilePermission.OTHERS_EXECUTE));
-					permissions.contains(PosixFilePermission.OTHERS_EXECUTE));
+
-			System.err.println("WARNING: your system's umask: \"" + umask
+		((FS_POSIX) fs).setUmask(0033);
-					+ "\" doesn't allow FSJava7Test to test if setting posix"
+		fs.setExecute(f, false);
-					+ "  permissions for \"others\" works properly");
+		assertFalse(fs.canExecute(f));
-			assumeFalse(badUmask);
+		fs.setExecute(f, true);
-		} else {
-			assertTrue("'others' execute permission not set",
-					permissions.contains(PosixFilePermission.OTHERS_EXECUTE));
-		}
-	}
 
-	private String readUmask() throws Exception {
+		permissions = readPermissions(f);
-		Process p = Runtime.getRuntime().exec(
+		assertTrue("'owner' execute permission not set",
-				new String[] { "sh", "-c", "umask" }, null, null);
+				permissions.contains(PosixFilePermission.OWNER_EXECUTE));
-		final BufferedReader lineRead = new BufferedReader(
+		assertFalse("'group' execute permission set",
-				new InputStreamReader(p.getInputStream(), Charset
+				permissions.contains(PosixFilePermission.GROUP_EXECUTE));
-						.defaultCharset().name()));
+		assertFalse("'others' execute permission set",
-		p.waitFor();
+				permissions.contains(PosixFilePermission.OTHERS_EXECUTE));
-		return lineRead.readLine();
 	}
 
 	private Set<PosixFilePermission> readPermissions(File f) throws IOException {

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

@@ -66,140 +66,70 @@ import org.eclipse.jgit.lib.Repository;
  * @since 3.0
  */
 public class FS_POSIX extends FS {
+	private static final int DEFAULT_UMASK = 0022;
+	private volatile int umask = -1;
 
-	static {
+	/** Default constructor. */
-		String umask = readUmask();
+	protected FS_POSIX() {
-
-		// umask return value consists of 3 or 4 digits, like "002" or "0002"
-		if (umask != null && umask.length() > 0 && umask.matches("\\d{3,4}")) { //$NON-NLS-1$
-			EXECUTE_FOR_OTHERS = isGranted(PosixFilePermission.OTHERS_EXECUTE,
-					umask);
-			EXECUTE_FOR_GROUP = isGranted(PosixFilePermission.GROUP_EXECUTE,
-					umask);
-		} else {
-			EXECUTE_FOR_OTHERS = null;
-			EXECUTE_FOR_GROUP = null;
-		}
 	}
 
 	/**
-	 * @since 4.0
+	 * Constructor
-	 */
+	 *
-	protected static final Boolean EXECUTE_FOR_OTHERS;
+	 * @param src
-
+	 *            FS to copy some settings from
-	/**
-	 * @since 4.0
 	 */
-	protected static final Boolean EXECUTE_FOR_GROUP;
+	protected FS_POSIX(FS src) {
+		super(src);
+		if (src instanceof FS_POSIX) {
+			umask = ((FS_POSIX) src).umask;
+		}
+	}
 
 	@Override
 	public FS newInstance() {
-		return new FS_POSIX();
+		return new FS_POSIX(this);
 	}
 
 	/**
-	 * Derives requested permission from given octal umask value as defined e.g.
+	 * Set the umask, overriding any value observed from the shell.
-	 * in <a href="http://linux.die.net/man/2/umask">http://linux.die.net/man/2/
-	 * umask</a>.
-	 * <p>
-	 * The umask expected here must consist of 3 or 4 digits. Last three digits
-	 * are significant here because they represent file permissions granted to
-	 * the "owner", "group" and "others" (in this order).
-	 * <p>
-	 * Each single digit from the umask represents 3 bits of the mask standing
-	 * for "<b>r</b>ead, <b>w</b>rite, e<b>x</b>ecute" permissions (in this
-	 * order).
-	 * <p>
-	 * The possible umask values table:
-	 *
-	 * <pre>
-	 * Value : Bits:Abbr.: Permission
-	 *     0 : 000 :rwx  : read, write and execute
-	 *     1 : 001 :rw   : read and write
-	 *     2 : 010 :rx   : read and execute
-	 *     3 : 011 :r    : read only
-	 *     4 : 100 :wx   : write and execute
-	 *     5 : 101 :w    : write only
-	 *     6 : 110 :x    : execute only
-	 *     7 : 111 :     : no permissions
-	 * </pre>
-	 * <p>
-	 * Note, that umask value is used to "mask" the requested permissions on
-	 * file creation by combining the requested permission bit with the
-	 * <b>negated</b> value of the umask bit.
-	 * <p>
-	 * Simply speaking, if a bit is <b>not</b> set in the umask, then the
-	 * appropriate right <b>will</b> be granted <b>if</b> requested. If a bit is
-	 * set in the umask value, then the appropriate permission will be not
-	 * granted.
-	 * <p>
-	 * Example:
-	 * <li>umask 023 ("000 010 011" or rwx rx r) combined with the request to
-	 * create an executable file with full set of permissions for everyone (777)
-	 * results in the file with permissions 754 (rwx rx r).
-	 * <li>umask 002 ("000 000 010" or rwx rwx rx) combined with the request to
-	 * create an executable file with full set of permissions for everyone (777)
-	 * results in the file with permissions 775 (rwx rwx rx).
-	 * <li>umask 002 ("000 000 010" or rwx rwx rx) combined with the request to
-	 * create a file without executable rights for everyone (666) results in the
-	 * file with permissions 664 (rw rw r).
 	 *
-	 * @param p
-	 *            non null permission
 	 * @param umask
-	 *            octal umask value represented by at least three digits. The
+	 *            mask to apply when creating files.
-	 *            digits (read from the end to beginning of the umask) represent
-	 *            permissions for "others", "group" and "owner".
-	 *
-	 * @return true if the requested permission is set according to given umask
 	 * @since 4.0
 	 */
-	protected static Boolean isGranted(PosixFilePermission p, String umask) {
+	public void setUmask(int umask) {
-		char val;
+		this.umask = umask;
-		switch (p) {
-		case OTHERS_EXECUTE:
-			// Read last digit, because umask is ordered as: User/Group/Others.
-			val = umask.charAt(umask.length() - 1);
-			return isExecuteGranted(val);
-		case GROUP_EXECUTE:
-			val = umask.charAt(umask.length() - 2);
-			return isExecuteGranted(val);
-		default:
-			throw new UnsupportedOperationException(
-					"isGranted() for " + p + " is not implemented!"); //$NON-NLS-1$ //$NON-NLS-2$
-		}
 	}
 
-	/**
+	private int umask() {
-	 * @param c
+		int u = umask;
-	 *            character representing octal permission value from the table
+		if (u == -1) {
-	 *            in {@link #isGranted(PosixFilePermission, String)}
+			u = readUmask();
-	 * @return true if the "execute" permission is granted according to given
+			umask = u;
-	 *         character
+		}
-	 */
+		return u;
-	private static Boolean isExecuteGranted(char c) {
-		if (c == '0' || c == '2' || c == '4' || c == '6')
-			return Boolean.TRUE;
-		return Boolean.FALSE;
 	}
 
-	/**
+	/** @return mask returned from running {@code umask} command in shell. */
-	 * @return umask returned from running umask command in a shell
+	private static int readUmask() {
-	 * @since 4.0
-	 */
-	protected static String readUmask() {
-		Process p;
 		try {
-			p = Runtime.getRuntime().exec(
+			Process p = Runtime.getRuntime().exec(
-					new String[] { "sh", "-c", "umask" }, null, null); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
+					new String[] { "sh", "-c", "umask" }, //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
+					null, null);
 			try (BufferedReader lineRead = new BufferedReader(
 					new InputStreamReader(p.getInputStream(), Charset
 							.defaultCharset().name()))) {
-				p.waitFor();
+				if (p.waitFor() == 0) {
-				return lineRead.readLine();
+					String s = lineRead.readLine();
+					if (s.matches("0?\\d{3}")) { //$NON-NLS-1$
+						return Integer.parseInt(s, 8);
+					}
+				}
+				return DEFAULT_UMASK;
 			}
 		} catch (Exception e) {
-			return null;
+			return DEFAULT_UMASK;
 		}
 	}
 
@@ -229,23 +159,6 @@ public class FS_POSIX extends FS {
 		return null;
 	}
 
-	/**
-	 * Default constructor
-	 */
-	protected FS_POSIX() {
-		super();
-	}
-
-	/**
-	 * Constructor
-	 *
-	 * @param src
-	 *            FS to copy some settings from
-	 */
-	protected FS_POSIX(FS src) {
-		super(src);
-	}
-
 	@Override
 	public boolean isCaseSensitive() {
 		return !SystemReader.getInstance().isMacOS();
@@ -265,35 +178,40 @@ public class FS_POSIX extends FS {
 	public boolean setExecute(File f, boolean canExecute) {
 		if (!isFile(f))
 			return false;
-		// only if the execute has to be set, and we know the umask
+		if (!canExecute)
-		if (canExecute && EXECUTE_FOR_OTHERS != null) {
+			return f.setExecutable(false);
-			try {
+
-				Path path = f.toPath();
+		try {
-				Set<PosixFilePermission> pset = Files
+			Path path = f.toPath();
-						.getPosixFilePermissions(path);
+			Set<PosixFilePermission> pset = Files.getPosixFilePermissions(path);
-				// user is always allowed to set execute
+
-				pset.add(PosixFilePermission.OWNER_EXECUTE);
+			// owner (user) is always allowed to execute.
-
+			pset.add(PosixFilePermission.OWNER_EXECUTE);
-				if (EXECUTE_FOR_GROUP.booleanValue())
+
-					pset.add(PosixFilePermission.GROUP_EXECUTE);
+			int mask = umask();
-
+			apply(pset, mask, PosixFilePermission.GROUP_EXECUTE, 1 << 3);
-				if (EXECUTE_FOR_OTHERS.booleanValue())
+			apply(pset, mask, PosixFilePermission.OTHERS_EXECUTE, 1);
-					pset.add(PosixFilePermission.OTHERS_EXECUTE);
+			Files.setPosixFilePermissions(path, pset);
-
+			return true;
-				Files.setPosixFilePermissions(path, pset);
+		} catch (IOException e) {
-				return true;
+			// The interface doesn't allow to throw IOException
-			} catch (IOException e) {
+			final boolean debug = Boolean.parseBoolean(SystemReader
-				// The interface doesn't allow to throw IOException
+					.getInstance().getProperty("jgit.fs.debug")); //$NON-NLS-1$
-				final boolean debug = Boolean.parseBoolean(SystemReader
+			if (debug)
-						.getInstance().getProperty("jgit.fs.debug")); //$NON-NLS-1$
+				System.err.println(e);
-				if (debug)
+			return false;
-					System.err.println(e);
+		}
-				return false;
+	}
-			}
+
+	private static void apply(Set<PosixFilePermission> set,
+			int umask, PosixFilePermission perm, int test) {
+		if ((umask & test) == 0) {
+			// If bit is clear in umask, permission is allowed.
+			set.add(perm);
+		} else {
+			// If bit is set in umask, permission is denied.
+			set.remove(perm);
 		}
-		// if umask is not working for some reason: fall back to default (buggy)
-		// implementation which does not consider umask: see bug 424395
-		return f.setExecutable(canExecute);
 	}
 
 	@Override