fix(git): fixes for Copilot's review of #172

docs/git-stacks.md

@@ -37,12 +37,13 @@ to your shell profile.
 - **The clone** lives on your array, by default under `/mnt/user/appdata/compose.manager/git/<stack>-<id>`,
   never on the flash drive. Each git stack has its own clone of the whole repository.
 - **The stack folder** in the projects folder holds the stack's settings (`git.json`), what was last deployed
-  (`git_state.json`), the stack's `.env` and the plugin's override file. These are never inside the clone, so
-  pulling new commits never touches them.
+  (`git_state.json`), the stack's `.env` and the plugin's override file (and, until the next deploy that works,
+  `git_discarded_changes`, see [below](#config-files-and-recreating-containers)). These are never inside the
+  clone, so pulling new commits never touches them.
 - **A deploy changes no container until everything else has worked.** It fetches the commit, checks it out,
   checks it (see [below](#what-is-checked-before-a-deploy)), pulls and builds images, and only then runs
-  `docker compose up`. If anything before `up` fails, the previous commit is put back and your containers are
-  exactly as they were.
+  `docker compose up`, which neither builds nor pulls again, whatever a service's `pull_policy` says. If
+  anything before `up` fails, the previous commit is put back and your containers are exactly as they were.
 - **Nothing of yours is deleted.** Files the plugin replaces are moved into a backup folder, local changes are
   saved as a patch before they are discarded, and an old clone is moved aside rather than removed. The plugin
   removes only a clone it has just made itself, when setting it up fails (for example, the branch has no
@@ -184,7 +185,8 @@ unchanged, the stack keeps its volumes and networks. Check that it works.
 ### 8. Tidy up
 
 Once you are happy, delete the `pre-git-<date>` folder. Until then it is your way back: to undo the
-conversion, delete `git.json`, `git_state.json`, `indirect` and `indirect_mode` from the stack folder, then
+conversion, delete `git.json`, `git_state.json`, `git_discarded_changes` (if there is one), `indirect` and
+`indirect_mode` from the stack folder, then
 copy the backup folder's files back into it (they include the old `indirect` settings, if the stack had any).
 
 ## Day to day
@@ -229,6 +231,9 @@ With no custom env file set for the stack, a git stack uses, in order:
 Only one of the two is used; they are not merged. Editing a `.env` that came from the repository changes the
 clone, so the next deploy stops with "local changes": make the change in the repository instead.
 
+Variables exported in the shell you run `compose-git deploy` from are not passed on, and `${PWD}` in the compose
+file is the compose file's folder in the clone: a deploy uses only what its checks saw. Put values in the `.env`.
+
 If the `.env` sets `COMPOSE_FILE` with relative paths, they are relative to the folder the `.env` is in. For
 the stack folder's `.env` that is the stack folder, not the clone, so use paths relative to the clone only in
 a `.env` committed next to the compose file.
@@ -286,6 +291,10 @@ a config file the container mounts would otherwise be checked out but never take
 So by default, **when a deploy changes anything in the stack's folder in the repository, every container in
 the stack is recreated**, and the new config takes effect.
 
+A deploy that saves local changes in the stack's folder as a patch and discards them recreates every
+container too, even at the same commit: a container may still hold an edited file. Until a deploy works, the
+stack folder holds a `git_discarded_changes` file that says so.
+
 Only the stack's own folder counts: a change to a shared folder elsewhere in the repository does not
 recreate anything yet. To turn the behaviour off, set `"recreateOnFolderChange": false` in the stack's
 `git.json`.
@@ -308,6 +317,13 @@ is, the deploy stops instead.
 
 - **Before `up`** (a check fails, the pull or build fails): the previous commit is put back and no container
   was changed. Fix the cause, usually in the repository, and deploy again.
+
+  "No container was changed" is about the containers themselves. A running container that mounts a file or
+  folder from the clone (`./config`, say) sees the new commit's files from the moment it is checked out, while
+  the checks, pull and build run, and the old ones again once it is put back. Most apps read their config only
+  when they start, so this does not matter to them. For an app that reloads its config when the file changes
+  (a proxy watching its config folder, say), keep that config at an absolute path outside the clone if a
+  deploy that is put back must not reach it.
 - **During `up`** (a container fails to start, or does not get healthy with `--wait`): a half-finished `up`
   cannot be undone safely, so it is not rolled back. The commit is recorded as failed and shown by
   `compose-git status`. Fix it in the repository and deploy again, or deploy a known good commit with
@@ -336,7 +352,8 @@ without changing anything when:
   folder, is missing;
 - a `container_name` or published port is already taken by another stack or container, or two services of
   the stack publish the same port;
-- Docker could not be asked whether a name or port is free (the daemon is not answering, say).
+- Docker could not be asked whether a name or port is free, or whether an external network or volume exists
+  (the daemon is not answering, say).
 
 The deploy log says which `.env` was used.
 

source/compose.manager/include/DockerCommand.php

@@ -28,6 +28,9 @@ public static function run(array $args, ?string $workingDirectory = null, int $t
             $environment['DOCKER_CONFIG'] = $dockerConfig;
         }
 
+        // As a shell would set it: a compose file can use ${PWD}, and compose.sh's runs see it too.
+        $environment['PWD'] = $workingDirectory ?? '/';
+
         return ProcessRunner::run(
             array_merge([COMPOSE_DOCKER_BIN], $args),
             $workingDirectory ?? '/',

source/compose.manager/include/GitCommand.php

@@ -62,7 +62,13 @@ public static function run(
         array $extraEnvironment = [],
         array $trustedRepositories = []
     ): ProcessResult {
-        $homeDirectory = self::prepareHomeDirectory();
+        $homeDirectory = COMPOSE_GIT_HOME_DIR;
+        $homeProblem = self::homeDirectoryProblem($homeDirectory);
+        if ($homeProblem !== null) {
+            // 128, git's own code for "could not run": some callers read exit code 1 as an
+            // answer (merge-base --is-ancestor says "no" with 1).
+            return new ProcessResult(128, '', $homeProblem, false);
+        }
 
         $command = ['git'];
         foreach (self::FORCED_CONFIG as $setting) {
@@ -142,14 +148,29 @@ private static function trustFile(string $homeDirectory, ?string $workingDirecto
     }
 
     /**
-     * The empty folder git runs with as HOME, so no personal config is read.
+     * Make the folder git runs with as HOME, so no personal config is read, and
+     * say what is wrong with it, if anything.
+     *
+     * It is in /var/tmp, where anyone can create things, and the global config
+     * git reads is kept in it: a folder someone else made first (or a symlink
+     * there) would let its owner give git a config of their own. So only a plain
+     * folder owned by whoever runs this (root on Unraid) is used.
      */
-    private static function prepareHomeDirectory(): string
+    private static function homeDirectoryProblem(string $home): ?string
     {
-        $home = COMPOSE_GIT_HOME_DIR;
-        if (!is_dir($home)) {
-            @mkdir($home, 0700, true);
+        if (!file_exists($home) && @readlink($home) === false && !@mkdir($home, 0700, true) && !is_dir($home)) {
+            return "Could not create $home for git.";
+        }
+        clearstatcache(true, $home);
+        if (@readlink($home) !== false || !is_dir($home)) {
+            return "$home is not a plain folder, so git was not run. Remove it.";
+        }
+        if (@fileowner($home) !== (function_exists('posix_geteuid') ? posix_geteuid() : 0)) {
+            return "$home belongs to another user, so git was not run. Remove it.";
+        }
+        if (!chmod($home, 0700)) {
+            return "Could not make $home private, so git was not run.";
         }
-        return $home;
+        return null;
     }
 }

source/compose.manager/include/GitDeploy.php

@@ -21,6 +21,15 @@
  */
 final class GitDeploy
 {
+    /**
+     * A file in the stack folder that says local changes to files in the stack's
+     * folder in the clone were discarded, and no deploy has succeeded since. The
+     * containers may still hold the discarded files (a single-file bind mount keeps
+     * the old copy), but no commit differs, so only this tells the next "up" to
+     * recreate them.
+     */
+    public const DISCARDED_CHANGES_FILE = 'git_discarded_changes';
+
     /** @var callable(string): void */
     private $say;
 
@@ -113,6 +122,11 @@ public function upArguments(string $previousCommit): array
         $clone = new GitClone($settings);
         $current = $clone->checkedOutCommit();
 
+        if (is_file($this->stackDir . '/' . self::DISCARDED_CHANGES_FILE)) {
+            ($this->say)("Local changes in the stack's folder were discarded, so every container is recreated.");
+            return ['--force-recreate'];
+        }
+
         $compareFrom = [$previousCommit];
         $deployed = GitStackState::load($this->stackDir)->deployedCommit;
         if ($deployed !== null && $deployed !== $previousCommit) {
@@ -156,6 +170,12 @@ public function finish(bool $succeeded): void
             ? new GitStackState($commit, null)
             : new GitStackState($state->deployedCommit, $commit);
         $newState->save($this->stackDir);
+        if ($succeeded) {
+            // The containers were just made from the clone as it is. If this cannot be
+            // removed, every deploy recreates the containers until it is (it is in the
+            // stack folder, and removing it by hand is safe).
+            @unlink($this->stackDir . '/' . self::DISCARDED_CHANGES_FILE);
+        }
     }
 
     /**
@@ -283,9 +303,45 @@ private function dealWithLocalChanges(GitClone $clone, GitStackState $state, str
         if ($patch !== null) {
             ($this->say)("Saved the local changes to $patch");
         }
+        // Recorded before the discard, so a failure here leaves the changes in place.
+        if ($this->anyInStackFolder($clone, $changedFiles)) {
+            $this->recordDiscardedChanges();
+        }
         $clone->discardLocalChanges();
     }
 
+    /**
+     * Whether any of these paths (relative to the repository) is in the stack's folder.
+     *
+     * @param string[] $paths
+     */
+    private function anyInStackFolder(GitClone $clone, array $paths): bool
+    {
+        $stackFolder = $clone->stackFolderInRepo();
+        if ($stackFolder === '') {
+            return $paths !== [];
+        }
+        foreach ($paths as $path) {
+            if (str_starts_with($path, $stackFolder . '/')) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    private function recordDiscardedChanges(): void
+    {
+        $stackDir = rtrim($this->stackDir, '/');
+        if (str_starts_with($stackDir, rtrim(COMPOSE_GIT_MNT_DIR, '/') . '/')) {
+            GitPathGuard::assertSafeToWrite($stackDir);
+        }
+        $file = $stackDir . '/' . self::DISCARDED_CHANGES_FILE;
+        $content = date('c') . "\n";
+        if (file_put_contents($file, $content) !== strlen($content)) {
+            throw new RuntimeException("Could not write $file, so the local changes were not discarded.");
+        }
+    }
+
     /**
      * @param string[] $profiles
      * @throws RuntimeException listing every problem found

source/compose.manager/include/GitDeployCheck.php

@@ -173,8 +173,14 @@ private function missingExternalResources(array $resolved): array
                     continue;
                 }
                 $name = (string) ($definition['name'] ?? $key);
-                if (!DockerCommand::run([$kind, 'inspect', '--', $name], null, 30)->succeeded()) {
+                $inspect = DockerCommand::run([$kind, 'inspect', '--', $name], null, 30);
+                if ($inspect->succeeded()) {
+                    continue;
+                }
+                if (self::failedOnlyOnMissing($inspect, $kind)) {
                     $problems[] = "The external $kind '$name' does not exist. Create it before deploying.";
+                } else {
+                    $problems[] = "Could not check whether the external $kind '$name' exists: " . self::failureText($inspect);
                 }
             }
         }
@@ -254,6 +260,15 @@ private function checkBindSource(string $source, string $cloneReal): ?string
         }
 
         if (GitPathGuard::findMountFor($source) === null) {
+            // A tmpfs the user mounted inside a disk or pool (a RAM transcode folder, say) is
+            // meant to be in RAM. A missing folder in it, empty after every reboot, is left for
+            // Docker to create, as below an existing share.
+            if (GitPathGuard::isOnRamMountInsideADisk($source)) {
+                if (!file_exists($source)) {
+                    $this->foldersDockerWillCreate[] = $source;
+                }
+                return null;
+            }
             return "the bind mount $source is not on a mounted disk, pool or share. "
                 . 'Docker would create it in RAM. Check the path for a typo.';
         }
@@ -520,6 +535,33 @@ private static function failedOnlyOnMissingContainers(ProcessResult $inspect): b
         return true;
     }
 
+    /**
+     * Whether "docker network inspect" or "docker volume inspect" failed only because
+     * nothing has that name, rather than because Docker could not be asked.
+     */
+    private static function failedOnlyOnMissing(ProcessResult $inspect, string $kind): bool
+    {
+        if ($inspect->timedOut) {
+            return false;
+        }
+        $lines = array_filter(array_map('trim', explode("\n", $inspect->stderr)), static fn(string $line): bool => $line !== '');
+        if ($lines === []) {
+            return false;
+        }
+        foreach ($lines as $line) {
+            // Docker today: "Error response from daemon: network x not found" and
+            // "Error response from daemon: get x: no such volume". Older Docker:
+            // "Error: No such network: x" and "Error: No such volume: x".
+            $missing = $kind === 'network'
+                ? preg_match('/network .+ not found$/i', $line) === 1 || stripos($line, 'no such network') !== false
+                : stripos($line, 'no such volume') !== false;
+            if (!$missing) {
+                return false;
+            }
+        }
+        return true;
+    }
+
     private static function failureText(ProcessResult $result): string
     {
         return trim($result->stderr) !== '' ? trim($result->stderr) : $result->errorSummary();