From bcf17d8b58c32a8e3bd8708d32fcc42377f1338f Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Thu, 28 Nov 2024 02:20:35 -0500
Subject: [PATCH 1/4] Check for executable bits that disagree with shebangs

It is rare that text files should be stored in a repository with a
leading `#!` (shebang, a.k.a. hashbang) but `-x` permissions, or
with no leading `#!` but `+x` permissions. But it is easy for this
to happen by accident.

This introduces a script to check for that situation -- currently
only in files whose names end in `.sh`, which are the files in
which this minor problem has tended to arise in this repository.

This adds a `justfile` recipe for it, as well as a CI job that runs
the script. (The CI job does not run it via `just`, since not doing
so allows it to save time by not installing anything.)

Currently, this:

- Looks only at what is committed and staged, ignoring unstaged
  files and unstaged mode changes. (This is intended to allow it to
  be cross platform, because on Windows, Git repositories support
  the same modes as anywhere else, but the filesystem doesn't
  support Unix-style executable permissions.)

- Is implemented as a shell script. Unlike `copy-packetline.sh`,
  there would be no major disadvantage to having this be Rust code
  instead, since it is never used to correct a problem that keeps
  Rust code from building.

- Is called from a separate CI job than any others. But it could
  probably be called from one of the existing jobs instead.

There are some files already in the repository that fail the new
check, which should be given `+x` permissions. In this commit, they
are kept as-is, and new files that should be detected as *wrongly*
having `+x` permissions are added. This is to verify that the
script is fully working as expected, including when run on CI. Once
that is confirmed, the new test files can be removed, the scripts
missing `+x` fixed, and the CI job made to run only on Ubuntu.

(See the commented discussion in #1589 for further information.)
---
 .github/workflows/ci.yml              | 18 ++++++++++++
 etc/check-mode.sh                     | 41 +++++++++++++++++++++++++++
 etc/delete-after-testing/empty.sh     |  0
 etc/delete-after-testing/not empty.sh |  2 ++
 justfile                              |  4 +++
 5 files changed, 65 insertions(+)
 create mode 100755 etc/check-mode.sh
 create mode 100755 etc/delete-after-testing/empty.sh
 create mode 100755 etc/delete-after-testing/not empty.sh

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index afc1cea684a..40368503d13 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -359,6 +359,24 @@ jobs:
       - name: gix-pack with all features (including wasm)
         run: cd gix-pack && cargo build --all-features --target "$TARGET"
 
+  check-modes:
+    # FIXME: Only run this on ubuntu-latest (don't use a matrix).
+    strategy:
+      fail-fast: false
+      matrix:
+        os: [ ubuntu-latest, macos-latest, windows-latest ]
+
+    runs-on: ${{ matrix.os }}
+
+    defaults:
+      run:
+        shell: bash
+
+    steps:
+      - uses: actions/checkout@v4
+      - name: Find scripts with mode/shebang mismatch
+        run: etc/check-mode.sh
+
   check-packetline:
     strategy:
       fail-fast: false
diff --git a/etc/check-mode.sh b/etc/check-mode.sh
new file mode 100755
index 00000000000..7e4bc4c56a8
--- /dev/null
+++ b/etc/check-mode.sh
@@ -0,0 +1,41 @@
+#!/usr/bin/env bash
+
+set -eu -o pipefail
+shopt -s lastpipe
+
+# Go to the worktree's root. (Even if the dir name ends in a newline.)
+root_padded="$(git rev-parse --show-toplevel && echo -n .)"
+root="${root_padded%$'\n.'}"
+cd -- "$root"
+
+symbolic_shebang="$(printf '#!' | od -An -ta)"
+status=0
+
+function check () {
+  local mode="$1" oid="$2" path="$3" symbolic_magic
+
+  # Extract the first two bytes (or less if shorter) and put in symbolic form.
+  symbolic_magic="$(git cat-file blob "$oid" | od -N2 -An -ta)"
+
+  # Check for inconsistency between the mode and whether `#!` is present.
+  if [ "$mode" = 100644 ] && [ "$symbolic_magic" = "$symbolic_shebang" ]; then
+    printf 'mode -x but has shebang: %q\n' "$path"
+  elif [ "$mode" = 100755 ] && [ "$symbolic_magic" != "$symbolic_shebang" ]; then
+    printf 'mode +x but no shebang: %q\n' "$path"
+  else
+    return 0
+  fi
+
+  status=1
+}
+
+# For now, check just regular files named with a `.sh` suffix.
+git ls-files -sz -- '*.sh' | while read -rd '' mode oid _stage_number path; do
+  case "$mode" in
+  100644 | 100755)
+    check "$mode" "$oid" "$path"
+    ;;
+  esac
+done
+
+exit "$status"
diff --git a/etc/delete-after-testing/empty.sh b/etc/delete-after-testing/empty.sh
new file mode 100755
index 00000000000..e69de29bb2d
diff --git a/etc/delete-after-testing/not empty.sh b/etc/delete-after-testing/not empty.sh
new file mode 100755
index 00000000000..c85b9ee924a
--- /dev/null
+++ b/etc/delete-after-testing/not empty.sh	
@@ -0,0 +1,2 @@
+This doesn't start with a #!.
+#! on another line doesn't count.
diff --git a/justfile b/justfile
index 6323a0670da..42b8cb0c449 100755
--- a/justfile
+++ b/justfile
@@ -255,6 +255,10 @@ fmt:
 find-yanked:
     cargo install --debug --locked --no-default-features --features max-pure --path .
 
+# Find shell scripts whose +x/-x bits and magic bytes (e.g. `#!`) disagree
+check-mode:
+    ./etc/check-mode.sh
+
 # Delete gix-packetline-blocking/src and regenerate from gix-packetline/src
 copy-packetline:
     ./etc/copy-packetline.sh

From 7ba80dada3e8b9ebb5eeb056cd04e9431fb2f661 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Thu, 28 Nov 2024 02:43:41 -0500
Subject: [PATCH 2/4] Make `check-mode.sh` usable on macOS

The version of `bash` on macOS is old and doed not have `lastpipe`.
This uses process substitution instead.

This also fixes a couple of bugs in the CI job:

- Change the name of the CI job to be the same as the script
  basename and `justfile` recipe. (I'm not sure if `check-mode` or
  `check-modes` is better, but it should be the same for all.)

- Add the job as a dependency of `tests-pass`. (Otherwise it will
  fail `check-blocking` if not explicitly listed as not being
  required for PR auto-merge. Since the problems it finds are
  usually easy to fix correctly, it should probably not be exempt.)
---
 .github/workflows/ci.yml | 3 ++-
 etc/check-mode.sh        | 7 +++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 40368503d13..7c4261d842c 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -359,7 +359,7 @@ jobs:
       - name: gix-pack with all features (including wasm)
         run: cd gix-pack && cargo build --all-features --target "$TARGET"
 
-  check-modes:
+  check-mode:
     # FIXME: Only run this on ubuntu-latest (don't use a matrix).
     strategy:
       fail-fast: false
@@ -459,6 +459,7 @@ jobs:
       - test-32bit-cross
       - lint
       - cargo-deny
+      - check-mode
       - check-packetline
       - check-blocking
 
diff --git a/etc/check-mode.sh b/etc/check-mode.sh
index 7e4bc4c56a8..4bc15b79ba5 100755
--- a/etc/check-mode.sh
+++ b/etc/check-mode.sh
@@ -1,7 +1,6 @@
 #!/usr/bin/env bash
 
 set -eu -o pipefail
-shopt -s lastpipe
 
 # Go to the worktree's root. (Even if the dir name ends in a newline.)
 root_padded="$(git rev-parse --show-toplevel && echo -n .)"
@@ -29,13 +28,13 @@ function check () {
   status=1
 }
 
-# For now, check just regular files named with a `.sh` suffix.
-git ls-files -sz -- '*.sh' | while read -rd '' mode oid _stage_number path; do
+# Check regular files named with a `.sh` suffix.
+while read -rd '' mode oid _stage_number path; do
   case "$mode" in
   100644 | 100755)
     check "$mode" "$oid" "$path"
     ;;
   esac
-done
+done < <(git ls-files -sz -- '*.sh')
 
 exit "$status"

From ed757ea0f4f80968d80c5d9d75ba49f031ee77fc Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Thu, 28 Nov 2024 02:51:44 -0500
Subject: [PATCH 3/4] Add missing executable bits on fixture scripts

Five fixture scripts were found by `etc/check-mode.sh` to begin
with `#!` but not be marked executable. This adds `+x` to them.

(I inspected them to make sure that this, and not removing the
shebangs, was the correct fix.)

This commit also removes the two temporary test files that had been
added to ensure `etc/check-mode.sh` is capable of identifying the
opposite situation. The script is now expected to pass.
---
 etc/delete-after-testing/empty.sh                       | 0
 etc/delete-after-testing/not empty.sh                   | 2 --
 gix-diff/tests/fixtures/make_diff_for_rewrites_repo.sh  | 0
 gix-merge/tests/fixtures/make_blob_repo.sh              | 0
 gix-merge/tests/fixtures/text-baseline.sh               | 0
 gix-merge/tests/fixtures/tree-baseline.sh               | 0
 gix-revision/tests/fixtures/merge_base_octopus_repos.sh | 0
 7 files changed, 2 deletions(-)
 delete mode 100755 etc/delete-after-testing/empty.sh
 delete mode 100755 etc/delete-after-testing/not empty.sh
 mode change 100644 => 100755 gix-diff/tests/fixtures/make_diff_for_rewrites_repo.sh
 mode change 100644 => 100755 gix-merge/tests/fixtures/make_blob_repo.sh
 mode change 100644 => 100755 gix-merge/tests/fixtures/text-baseline.sh
 mode change 100644 => 100755 gix-merge/tests/fixtures/tree-baseline.sh
 mode change 100644 => 100755 gix-revision/tests/fixtures/merge_base_octopus_repos.sh

diff --git a/etc/delete-after-testing/empty.sh b/etc/delete-after-testing/empty.sh
deleted file mode 100755
index e69de29bb2d..00000000000
diff --git a/etc/delete-after-testing/not empty.sh b/etc/delete-after-testing/not empty.sh
deleted file mode 100755
index c85b9ee924a..00000000000
--- a/etc/delete-after-testing/not empty.sh	
+++ /dev/null
@@ -1,2 +0,0 @@
-This doesn't start with a #!.
-#! on another line doesn't count.
diff --git a/gix-diff/tests/fixtures/make_diff_for_rewrites_repo.sh b/gix-diff/tests/fixtures/make_diff_for_rewrites_repo.sh
old mode 100644
new mode 100755
diff --git a/gix-merge/tests/fixtures/make_blob_repo.sh b/gix-merge/tests/fixtures/make_blob_repo.sh
old mode 100644
new mode 100755
diff --git a/gix-merge/tests/fixtures/text-baseline.sh b/gix-merge/tests/fixtures/text-baseline.sh
old mode 100644
new mode 100755
diff --git a/gix-merge/tests/fixtures/tree-baseline.sh b/gix-merge/tests/fixtures/tree-baseline.sh
old mode 100644
new mode 100755
diff --git a/gix-revision/tests/fixtures/merge_base_octopus_repos.sh b/gix-revision/tests/fixtures/merge_base_octopus_repos.sh
old mode 100644
new mode 100755

From 82cbc0af401b75fcc5c2d91bfe7dcd9cb9cd8570 Mon Sep 17 00:00:00 2001
From: Eliah Kagan <degeneracypressure@gmail.com>
Date: Thu, 28 Nov 2024 02:54:47 -0500
Subject: [PATCH 4/4] Simplify the `check-mode` CI job

By running it only on Ubuntu (not macOS and Windows), and removing
parts that were only needed for running it on Windows.

Unlike `etc/copy-packetline.sh`, the `etc/check-mode.sh` script
probably does not need to be made easy to retest on other platforms
after changes, becaus it does not delete (nor otherwise modify) the
repository, and becuase it is considerably shorter, simpler, and
less reliant on subtleties of standard tools that vary across
platforms.
---
 .github/workflows/ci.yml | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 7c4261d842c..42246e7e52f 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -360,17 +360,7 @@ jobs:
         run: cd gix-pack && cargo build --all-features --target "$TARGET"
 
   check-mode:
-    # FIXME: Only run this on ubuntu-latest (don't use a matrix).
-    strategy:
-      fail-fast: false
-      matrix:
-        os: [ ubuntu-latest, macos-latest, windows-latest ]
-
-    runs-on: ${{ matrix.os }}
-
-    defaults:
-      run:
-        shell: bash
+    runs-on: ubuntu-latest
 
     steps:
       - uses: actions/checkout@v4