From c7e527b82b3dafed5f0da2ec0e14a47cf8e65def Mon Sep 17 00:00:00 2001
From: Martin Schwaighofer <mschwaig@users.noreply.github.com>
Date: Mon, 13 Dec 2021 21:31:20 +0100
Subject: [PATCH] git fetcher: invoke diff instead of diff-index

diff-index operates on the view that git has of the working tree,
which might be outdated. The higher-level diff command does this
automatically. This change also adds handling for submodules.

fixes #4140

Alternative fixes would be invoking update-index before diff-index or
matching more closely what require_clean_work_tree from git-sh-setup.sh
does, but both those options make it more difficult to reason about
correctness.
---
 src/libfetchers/git.cc | 12 +++++++++++-
 tests/fetchGit.sh      |  7 ++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc
index 7479318e3..d241eb67a 100644
--- a/src/libfetchers/git.cc
+++ b/src/libfetchers/git.cc
@@ -235,7 +235,17 @@ struct GitInputScheme : InputScheme
 
             try {
                 if (hasHead) {
-                    runProgram("git", true, { "-C", actualUrl, "diff-index", "--quiet", "HEAD", "--" });
+                    // Using git diff is preferrable over lower-level operations here,
+                    // because its conceptually simpler and we only need the exit code anyways.
+                    auto gitDiffOpts = Strings({ "-C", actualUrl, "diff", "HEAD", "--quiet"});
+                    if (!submodules) {
+                        // Changes in submodules should only make the tree dirty
+                        // when those submodules will be copied as well.
+                        gitDiffOpts.emplace_back("--ignore-submodules");
+                    }
+                    gitDiffOpts.emplace_back("--");
+                    runProgram("git", true, gitDiffOpts);
+
                     clean = true;
                 }
             } catch (ExecError & e) {
diff --git a/tests/fetchGit.sh b/tests/fetchGit.sh
index dd0d98956..628d96924 100644
--- a/tests/fetchGit.sh
+++ b/tests/fetchGit.sh
@@ -11,7 +11,7 @@ repo=$TEST_ROOT/git
 
 export _NIX_FORCE_HTTP=1
 
-rm -rf $repo ${repo}-tmp $TEST_HOME/.cache/nix $TEST_ROOT/worktree $TEST_ROOT/shallow
+rm -rf $repo ${repo}-tmp $TEST_HOME/.cache/nix $TEST_ROOT/worktree $TEST_ROOT/shallow $TEST_ROOT/minimal
 
 git init $repo
 git -C $repo config user.email "foobar@example.com"
@@ -147,8 +147,13 @@ path3=$(nix eval --impure --raw --expr "(builtins.fetchGit $repo).outPath")
 # (check dirty-tree handling was used)
 [[ $(nix eval --impure --raw --expr "(builtins.fetchGit $repo).rev") = 0000000000000000000000000000000000000000 ]]
 [[ $(nix eval --impure --raw --expr "(builtins.fetchGit $repo).shortRev") = 0000000 ]]
+# Making a dirty tree clean again and fetching it should
+# record correct revision information. See: #4140
+echo world > $repo/hello
+[[ $(nix eval --impure --raw --expr "(builtins.fetchGit $repo).rev") = $rev2 ]]
 
 # Committing shouldn't change store path, or switch to using 'master'
+echo dev > $repo/hello
 git -C $repo commit -m 'Bla5' -a
 path4=$(nix eval --impure --raw --expr "(builtins.fetchGit $repo).outPath")
 [[ $(cat $path4/hello) = dev ]]