diff --git a/lib/snap_diff/vcs.rb b/lib/snap_diff/vcs.rb index a5b34081..50c8e7d6 100644 --- a/lib/snap_diff/vcs.rb +++ b/lib/snap_diff/vcs.rb @@ -5,6 +5,13 @@ module SnapDiff module Vcs + # `-C ` 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 diff --git a/test/unit/vcs_test.rb b/test/unit/vcs_test.rb index 83ab98b3..609fb3cf 100644 --- a/test/unit/vcs_test.rb +++ b/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 show HEAD:` 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= git -C 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 --