fix: an inherited GIT_DIR silently redirected every baseline lookup

lib/snap_diff/vcs.rb

@@ -5,6 +5,13 @@
 
 module SnapDiff
   module Vcs
+    # `-C <dir>` sets the working directory, but GIT_DIR/GIT_WORK_TREE OVERRIDE
+    # it -- so a suite launched from a git hook (which exports both) reads the
+    # WRONG repository. Every baseline lookup then fails, and because
+    # `fail_if_new` is false locally, screenshots are recorded as new and the
+    # tests PASS. Scrubbing them makes `-C` mean what this code already assumes.
+    GIT_ENV = {"GIT_DIR" => nil, "GIT_WORK_TREE" => nil, "GIT_INDEX_FILE" => nil}.freeze
+
     @git_roots = {}
     @git_roots_lock = Mutex.new
 
@@ -17,13 +24,13 @@ def self.checkout_vcs(root, screenshot_path, checkout_path)
 
       if SnapDiff.config.use_lfs
         tmp_path = "#{checkout_path}.tmp"
-        success = system("git", "-C", root_path, "show", "HEAD:#{vcs_file_path}", out: tmp_path, err: File::NULL)
+        success = system(GIT_ENV, "git", "-C", root_path, "show", "HEAD:#{vcs_file_path}", out: tmp_path, err: File::NULL)
         if success
-          system("git", "-C", root_path, "lfs", "smudge", in: tmp_path, out: checkout_path.to_s, err: File::NULL)
+          system(GIT_ENV, "git", "-C", root_path, "lfs", "smudge", in: tmp_path, out: checkout_path.to_s, err: File::NULL)
         end
         File.delete(tmp_path) if File.exist?(tmp_path)
       else
-        success = system("git", "-C", root_path, "show", "HEAD:#{vcs_file_path}", out: checkout_path.to_s, err: File::NULL)
+        success = system(GIT_ENV, "git", "-C", root_path, "show", "HEAD:#{vcs_file_path}", out: checkout_path.to_s, err: File::NULL)
       end
 
       unless success
@@ -53,7 +60,7 @@ def self.git_root_for(root_path)
       @git_roots_lock.synchronize do
         next @git_roots[root_path] if @git_roots.key?(root_path)
 
-        git_root, _, status = Open3.capture3("git", "-C", root_path, "rev-parse", "--show-toplevel")
+        git_root, _, status = Open3.capture3(GIT_ENV, "git", "-C", root_path, "rev-parse", "--show-toplevel")
         @git_roots[root_path] = status.success? && git_root.chomp
       end
     end

test/unit/vcs_test.rb

@@ -55,6 +55,35 @@ def status.success? = true
     assert_equal 1, calls
   end
 
+  # A git hook (pre-push, pre-commit) exports GIT_DIR, and GIT_DIR OVERRIDES
+  # `-C`. So `git -C <root> show HEAD:<path>` silently reads the wrong
+  # repository, the baseline lookup fails, and -- because fail_if_new defaults
+  # to false locally -- the screenshot is recorded as new and the test PASSES.
+  # Verified by hand: `GIT_DIR=<other> git -C <repo> show HEAD:f.txt` =>
+  # "fatal: path 'f.txt' exists on disk, but not in 'HEAD'".
+  test "#checkout_vcs ignores an inherited GIT_DIR and honours its own root" do
+    screenshot_path = file_fixture("images/a.png")
+    base_screenshot_path = Pathname.new(@base_screenshot.path)
+    elsewhere = @tmp_dir / "unrelated_repo_#{Time.now.nsec}"
+    FileUtils.mkdir_p(elsewhere)
+    Open3.capture3("git", "-C", elsewhere.to_s, "init", "--quiet")
+
+    with_env("GIT_DIR" => (elsewhere / ".git").to_s) do
+      SnapDiff::Vcs.checkout_vcs(PROJECT_ROOT, screenshot_path, base_screenshot_path)
+    end
+
+    assert base_screenshot_path.exist?, "an inherited GIT_DIR must not redirect the baseline lookup"
+    assert_equal screenshot_path.size, base_screenshot_path.size
+  end
+
+  def with_env(vars)
+    previous = vars.keys.to_h { |k| [k, ENV[k]] }
+    vars.each { |k, v| ENV[k] = v }
+    yield
+  ensure
+    previous.each { |k, v| ENV[k] = v }
+  end
+
   # Concurrent callers must share the cached answer, not each spawn their own
   # git. MRI releases the GVL for the duration of `Open3.capture3`, so without
   # synchronization every thread misses `key?` before any thread writes --