Merge pull request #5758 from mschwaig/fix-git-workspace-dirty-detection

git fetcher: fix detection of dirty git workspaces

src/libfetchers/git.cc

@@ -222,22 +222,46 @@ struct GitInputScheme : InputScheme
         if (!input.getRef() && !input.getRev() && isLocal) {
             bool clean = false;
 
-            /* Check whether this repo has any commits. There are
+            auto env = getEnv();
-               probably better ways to do this. */
+            // Set LC_ALL to C: because we rely on the error messages from git rev-parse to determine what went wrong
-            auto gitDir = actualUrl + "/.git";
+            // that way unknown errors can lead to a failure instead of continuing through the wrong code path
-            auto commonGitDir = chomp(runProgram(
+            env["LC_ALL"] = "C";
-                "git",
-                true,
-                { "-C", actualUrl, "rev-parse", "--git-common-dir" }
-            ));
-            if (commonGitDir != ".git")
-                    gitDir = commonGitDir;
 
-            bool haveCommits = !readDirectory(gitDir + "/refs/heads").empty();
+            /* Check whether HEAD points to something that looks like a commit,
+               since that is the refrence we want to use later on. */
+            auto result = runProgram(RunOptions {
+                .program = "git",
+                .args = { "-C", actualUrl, "--git-dir=.git", "rev-parse", "--verify", "--no-revs", "HEAD^{commit}" },
+                .environment = env,
+                .mergeStderrToStdout = true
+            });
+            auto exitCode = WEXITSTATUS(result.first);
+            auto errorMessage = result.second;
 
+            if (errorMessage.find("fatal: not a git repository") != std::string::npos) {
+                throw Error("'%s' is not a git repository.", actualUrl);
+            } else if (errorMessage.find("fatal: Needed a single revision") != std::string::npos) {
+                // indicates that the repo does not have any commits
+                // we want to proceed and will consider it dirty later
+            } else if (exitCode != 0) {
+                // any other errors should lead to a failure
+                throw Error("Getting the HEAD of the git tree '%s' failed with exit code %d:\n%s", actualUrl, exitCode, errorMessage);
+            }
+
+            bool hasHead = exitCode == 0;
             try {
-                if (haveCommits) {
+                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) {
@@ -282,7 +306,7 @@ struct GitInputScheme : InputScheme
                 // modified dirty file?
                 input.attrs.insert_or_assign(
                     "lastModified",
-                    haveCommits ? std::stoull(runProgram("git", true, { "-C", actualUrl, "log", "-1", "--format=%ct", "--no-show-signature", "HEAD" })) : 0);
+                    hasHead ? std::stoull(runProgram("git", true, { "-C", actualUrl, "log", "-1", "--format=%ct", "--no-show-signature", "HEAD" })) : 0);
 
                 return {std::move(storePath), input};
             }

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 ]]
@@ -170,6 +175,14 @@ NIX=$(command -v nix)
 path5=$(nix eval --impure --raw --expr "(builtins.fetchGit { url = $repo; ref = \"dev\"; }).outPath")
 [[ $path3 = $path5 ]]
 
+# Fetching from a repo with only a specific revision and no branches should
+# not fall back to copying files and record correct revision information. See: #5302
+mkdir $TEST_ROOT/minimal
+git -C $TEST_ROOT/minimal init
+git -C $TEST_ROOT/minimal fetch $repo $rev2
+git -C $TEST_ROOT/minimal checkout $rev2
+[[ $(nix eval --impure --raw --expr "(builtins.fetchGit { url = $TEST_ROOT/minimal; }).rev") = $rev2 ]]
+
 # Fetching a shallow repo shouldn't work by default, because we can't
 # return a revCount.
 git clone --depth 1 file://$repo $TEST_ROOT/shallow
@@ -193,3 +206,11 @@ rev4_nix=$(nix eval --impure --raw --expr "(builtins.fetchGit { url = \"file://$
 # The name argument should be handled
 path9=$(nix eval --impure --raw --expr "(builtins.fetchGit { url = \"file://$repo\"; ref = \"HEAD\"; name = \"foo\"; }).outPath")
 [[ $path9 =~ -foo$ ]]
+
+# should fail if there is no repo
+rm -rf $repo/.git
+(! nix eval --impure --raw --expr "(builtins.fetchGit \"file://$repo\").outPath")
+
+# should succeed for a repo without commits
+git init $repo
+path10=$(nix eval --impure --raw --expr "(builtins.fetchGit \"file://$repo\").outPath")