Repository: bazelbuild/bazel
Issue: Native patch doesn't preserve executable bit
Labels: type: bug
P2

### Description of the problem / feature request:

Native patch doesn't preserve the executable bit of the patched file. If you're trying to patch an executable file in a repository, the file will become a non-executable file.

### Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

```
.
├── BUILD
├── WORKSPACE
└── rules_apple.patch
```

BUILD is an empty file.

WORKSPACE:

```
load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository")

git_repository(
    name = "build_bazel_rules_apple",
    commit = "3bbee3a593d1f6ddb3391ba3a99ffe3f09a4821b",
    patch_args = ["-p1"],
    patches = ["//:rules_apple.patch"],
    remote = "https://github.com/bazelbuild/rules_apple.git",
    shallow_since = "1582926244 -0800",
)
load(
    "@build_bazel_rules_apple//apple:repositories.bzl",
    "apple_rules_dependencies",
)
apple_rules_dependencies()
```

rules_apple.patch (https://github.com/bazelbuild/rules_apple/pull/724.diff):

```diff
diff --git a/tools/dynamic_framework_slicer/dynamic_framework_slicer.sh b/tools/dynamic_framework_slicer/dynamic_framework_slicer.sh
index c271bd80..357edb87 100755
--- a/tools/dynamic_framework_slicer/dynamic_framework_slicer.sh
+++ b/tools/dynamic_framework_slicer/dynamic_framework_slicer.sh
@@ -79,9 +79,14 @@ else
         fi
     done
 
+    # Create a thin binary if there's only one needed slice, otherwise create a universal binary
     declare -a lipo_args
-    for slice in "${slices_needed[@]}"; do
-        lipo_args+=(-extract $slice)
-    done
+    if [[ "${#slices_needed[@]}" -eq 1 ]]; then
+        lipo_args+=(-thin ${slices_needed[0]})
+    else
+        for slice in "${slices_needed[@]}"; do
+            lipo_args+=(-extract $slice)
+        done
+    fi
     xcrun lipo "$IN" "${lipo_args[@]}" -output "$OUT"
 fi
```

Run:

```
bazel build @build_bazel_rules_apple//tools/dynamic_framework_slicer
```

Observed error:

```
ERROR: /private/var/tmp/_bazel_admin/36cd6eaac350dd8b1091da163994488a/external/build_bazel_rules_apple/tools/dynamic_framework_slicer/BUILD:3:1: failed to create symbolic link 'external/build_bazel_rules_apple/tools/dynamic_framework_slicer/dynamic_framework_slicer': file 'external/build_bazel_rules_apple/tools/dynamic_framework_slicer/dynamic_framework_slicer.sh' is not executable
Target @build_bazel_rules_apple//tools/dynamic_framework_slicer:dynamic_framework_slicer failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 6.818s, Critical Path: 0.00s
INFO: 0 processes.
FAILED: Build did NOT complete successfully
```

The build succeeds if I use the `patch` command line tool as the patch tool.

### What operating system are you running Bazel on?

macOS Catalina 10.15.3 (19D76).

### What's the output of `bazel info release`?

release 2.2.0

###  Have you found anything relevant by searching the web?

Searching with `patch` and `executable` didn't return any relevant issue.

Issue comments:
/cc @meteorcloudy 

Pull request review comments:
We can use bit-operators here to test for the bits in the permission bitmask :)

Executable is `0b001 = 1`
Writable is `0b010 = 2`
Readable is `0b100 = 4`

So we can replace this with:

```java
return permission & 4;
```
Similarly:
```java
return permission & 2;
```
Similarly:
```java
return permission & 1;
```
How is this change related?
If you want to use some more exotic Java data types, you could use `Optional<BitSet>` for the filePermission type (and `BitSet` in general, rather than an `int`, to store the permissions). Then you could use an `enum` as the keys for the `BitSet` (e.g. `EXECUTABLE = 0, WRITABLE = 1, READABLE = 2`).

But using `-1` as the indicator for "no explicit permissions" is also fine, if you prefer it. ;)
Thanks, I'll use 
```java 
return (permission & 4) == 4
```
Done.
Done
So this test case is for testing an error message is thrown when no input and output file name is specified. Before, we only parse file names from `---` and `+++` lines. But now we also parses file names from `diff --git` line, therefore we should also remove them here to keep the test passing.
I tried to use BitSet, but found converting a character (eg. `7`) to a BitSet also requires some steps. The code overall didn't look much cleaner, so I'll keep using `-1` and bit operations ;)

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/PatchUtil.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/PatchUtil.java
index 8c0a2da84a7715..8ab52cdbd56794 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/PatchUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/PatchUtil.java
@@ -156,7 +156,9 @@ private enum LineType {
     GIT_HEADER,
     RENAME_FROM,
     RENAME_TO,
-    GIT_LINE,
+    NEW_MODE,
+    NEW_FILE_MODE,
+    OTHER_GIT_LINE,
     UNKNOWN
   }
 
@@ -203,9 +205,15 @@ private static LineType getLineType(String line, boolean isReadingChunk, boolean
         if (line.startsWith("rename to ")) {
           return LineType.RENAME_TO;
         }
+        if (line.startsWith("new mode ")) {
+          return LineType.NEW_MODE;
+        }
+        if (line.startsWith("new file mode ")) {
+          return LineType.NEW_FILE_MODE;
+        }
         for (String prefix : GIT_LINE_PREFIXES) {
           if (line.startsWith(prefix)) {
-            return LineType.GIT_LINE;
+            return LineType.OTHER_GIT_LINE;
           }
         }
       }
@@ -229,8 +237,35 @@ private static void writeFile(Path file, List<String> content) throws IOExceptio
     FileSystemUtils.writeLinesAs(file, StandardCharsets.UTF_8, content);
   }
 
+  private static boolean getReadPermission(int permission) {
+    // Parse read permission from posix file permission notation
+    return (permission & 4) == 4;
+  }
+
+  private static boolean getWritePermission(int permission) {
+    // Parse write permission from posix file permission notation
+    return (permission & 2) == 2;
+  }
+
+  private static boolean getExecutablePermission(int permission) {
+    // Parse executable permission from posix file permission notation
+    return (permission & 1) == 1;
+  }
+
+  private static int getFilePermissionValue(Path file) throws IOException {
+    return (file.isReadable() ? 4 : 0) +
+        (file.isWritable() ? 2 : 0) +
+        (file.isExecutable() ? 1 : 0);
+  }
+
+  private static void setFilePermission(Path file, int permission) throws IOException {
+      file.setReadable(getReadPermission(permission));
+      file.setWritable(getWritePermission(permission));
+      file.setExecutable((getExecutablePermission(permission)));
+  }
+
   private static void applyPatchToFile(
-      Patch<String> patch, Path oldFile, Path newFile, boolean isRenaming)
+      Patch<String> patch, Path oldFile, Path newFile, boolean isRenaming, int filePermission)
       throws IOException, PatchFailedException {
     // The file we should read oldContent from.
     Path inputFile = null;
@@ -245,6 +280,10 @@ private static void applyPatchToFile(
       oldContent = new ArrayList<>();
     } else {
       oldContent = readFile(inputFile);
+      // Preserve old file permission if no explicit permission is set.
+      if (filePermission == -1) {
+        filePermission = getFilePermissionValue(inputFile);
+      }
     }
 
     List<String> newContent = OffsetPatch.applyTo(patch, oldContent);
@@ -268,6 +307,9 @@ private static void applyPatchToFile(
 
     if (outputFile != null && !isDeleteFile) {
       writeFile(outputFile, newContent);
+      if (filePermission != -1) {
+        setFilePermission(outputFile, filePermission);
+      }
     }
   }
 
@@ -448,6 +490,7 @@ public static void apply(Path patchFile, int strip, Path outputDirectory)
     Path newFile = null;
     int oldLineCount = 0;
     int newLineCount = 0;
+    int filePermission = -1;
     Result result;
 
     for (int i = 0; i < patchFileLines.size(); i++) {
@@ -464,6 +507,18 @@ public static void apply(Path patchFile, int strip, Path outputDirectory)
           newFileStr = extractPath(line, strip, i + 1);
           newFile = getFilePath(newFileStr, outputDirectory, i + 1);
           break;
+        case NEW_MODE:
+        case NEW_FILE_MODE:
+          // The line should look like: "new mode 100755" or "new file mode 100755"
+          // 7 is the file permission for owner, which is at index 12 or 17
+          int index = type == LineType.NEW_MODE ? 12 : 17;
+          char c = line.charAt(index);
+          if (c < '0' || c > '7') {
+            throw new PatchFailedException(
+                "Wrong file mode format at line "+ (i + 1) + ": " + line);
+          }
+          filePermission = Character.getNumericValue(c);
+          break;
         case CHUNK_HEAD:
           int pos = line.indexOf("@@", 2);
           String headerStr = line.substring(0, pos + 2);
@@ -543,7 +598,7 @@ public static void apply(Path patchFile, int strip, Path outputDirectory)
             newFile = getFilePath(newFileStr, outputDirectory, i + 1);
           }
           break;
-        case GIT_LINE:
+        case OTHER_GIT_LINE:
           break;
         case GIT_HEADER:
         case UNKNOWN:
@@ -553,7 +608,7 @@ public static void apply(Path patchFile, int strip, Path outputDirectory)
           // Renaming is a git only format
           boolean isRenaming = isGitDiff && hasRenameFrom && hasRenameTo;
 
-          if (!patchContent.isEmpty() || isRenaming) {
+          if (!patchContent.isEmpty() || isRenaming || filePermission != -1) {
             // We collected something useful, let's do some sanity checks before applying the patch.
             int patchStartLocation = i + 1 - patchContent.size();
 
@@ -569,7 +624,7 @@ public static void apply(Path patchFile, int strip, Path outputDirectory)
             checkFilesStatusForPatching(
                 patch, oldFile, newFile, oldFileStr, newFileStr, patchStartLocation);
 
-            applyPatchToFile(patch, oldFile, newFile, isRenaming);
+            applyPatchToFile(patch, oldFile, newFile, isRenaming, filePermission);
           }
 
           patchContent.clear();
@@ -578,11 +633,27 @@ public static void apply(Path patchFile, int strip, Path outputDirectory)
           newFileStr = null;
           oldFile = null;
           newFile = null;
+          filePermission = -1;
           oldLineCount = 0;
           newLineCount = 0;
           isReadingChunk = false;
           // If the new patch starts with "diff --git " then it's a git diff.
           isGitDiff = type == LineType.GIT_HEADER;
+          if (isGitDiff) {
+            // In case there is no line starting with +++ and --- (file permission change),
+            // try to parse the file names from the line starting with "diff --git"
+            String[] args = line.split(" ");
+            if (args.length >= 4) {
+              oldFileStr = stripPath(args[2], strip);
+              if (!oldFileStr.isEmpty()) {
+                oldFile = getFilePath(oldFileStr, outputDirectory, i + 1);
+              }
+              newFileStr = stripPath(args[3], strip);
+              if (!newFileStr.isEmpty()) {
+                newFile = getFilePath(newFileStr, outputDirectory, i + 1);
+              }
+            }
+          }
           hasRenameFrom = false;
           hasRenameTo = false;
           break;


Test patch:
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/PatchUtilTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/PatchUtilTest.java
index 5fbcace554a2b1..64fb6d0eea7a63 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/PatchUtilTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/PatchUtilTest.java
@@ -50,7 +50,7 @@ public void testAddFile() throws IOException, PatchFailedException {
         scratch.file(
             "/root/patchfile",
             "diff --git a/newfile b/newfile",
-            "new file mode 100644",
+            "new file mode 100544",
             "index 0000000..f742c88",
             "--- /dev/null",
             "+++ b/newfile",
@@ -63,6 +63,10 @@ public void testAddFile() throws IOException, PatchFailedException {
     Path newFile = root.getRelative("newfile");
     ImmutableList<String> newFileContent = ImmutableList.of("I'm a new file", "hello, world");
     assertThat(PatchUtil.readFile(newFile)).containsExactlyElementsIn(newFileContent);
+    // Make sure file permission is set as specified.
+    assertThat(newFile.isReadable()).isTrue();
+    assertThat(newFile.isWritable()).isFalse();
+    assertThat(newFile.isExecutable()).isTrue();
   }
 
   @Test
@@ -154,6 +158,9 @@ public void testApplyToOldFile() throws IOException, PatchFailedException {
   public void testApplyToNewFile() throws IOException, PatchFailedException {
     // If only newfile exists, we should patch the new file.
     Path newFile = scratch.file("/root/newfile", "line one");
+    newFile.setReadable(true);
+    newFile.setWritable(true);
+    newFile.setExecutable(true);
     Path patchFile =
         scratch.file(
             "/root/patchfile",
@@ -165,6 +172,29 @@ public void testApplyToNewFile() throws IOException, PatchFailedException {
     PatchUtil.apply(patchFile, 0, root);
     ImmutableList<String> newContent = ImmutableList.of("line one", "line two");
     assertThat(PatchUtil.readFile(newFile)).containsExactlyElementsIn(newContent);
+    // Make sure file permission is preserved.
+    assertThat(newFile.isReadable()).isTrue();
+    assertThat(newFile.isWritable()).isTrue();
+    assertThat(newFile.isExecutable()).isTrue();
+  }
+
+  @Test
+  public void testChangeFilePermission() throws IOException, PatchFailedException {
+    Path myFile = scratch.file("/root/test.sh", "line one");
+    myFile.setReadable(true);
+    myFile.setWritable(true);
+    myFile.setExecutable(false);
+    Path patchFile =
+        scratch.file(
+            "/root/patchfile",
+            "diff --git a/test.sh b/test.sh",
+            "old mode 100644",
+            "new mode 100755");
+    PatchUtil.apply(patchFile, 1, root);
+    assertThat(PatchUtil.readFile(myFile)).containsExactlyElementsIn(ImmutableList.of("line one"));
+    assertThat(myFile.isReadable()).isTrue();
+    assertThat(myFile.isWritable()).isTrue();
+    assertThat(myFile.isExecutable()).isTrue();
   }
 
   @Test
@@ -378,7 +408,7 @@ public void testMissingBothOldAndNewFile() throws IOException {
     Path patchFile =
         scratch.file(
             "/root/patchfile",
-            "diff --git a/foo.cc b/foo.cc",
+            "diff --git a/ b/",
             "index f3008f9..ec4aaa0 100644",
             "@@ -2,4 +2,5 @@",
             " ",