From 8e8416387c935f2dec5bd24ceac3e3553cae0d59 Mon Sep 17 00:00:00 2001 From: Farid Zakaria Date: Fri, 11 Jul 2025 09:34:06 -0700 Subject: [PATCH 1/3] Add error message when git returns non-0 for fetch Users have complained that fetchGit is flaky however the culprit is likely that `git fetch` was unable itself to download the repository for whatever reason (i.e. poor network etc..) Nothing was checking the status of `git fetch` and the error message that would eventually surface to the users were that the commit was not found. Add explicit error checking for status code from `git fetch` and return a message earlier on to indicate that the failure was from that point. fixes #10431 --- src/libfetchers/git-utils.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index 9fe271fe8..563c2180d 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -545,7 +545,7 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this append(gitArgs, {"--depth", "1"}); append(gitArgs, {std::string("--"), url, refspec}); - runProgram(RunOptions { + auto [status, output] = runProgram(RunOptions { .program = "git", .lookupPath = true, // FIXME: git stderr messes up our progress indicator, so @@ -554,6 +554,10 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this .input = {}, .isInteractive = true }); + + if (status > 0) { + throw Error("Failed to fetch git repository %s: %s", url, output); + } } void verifyCommit( From fb6f494d35656556465f05db9f4ee8dbdb1d3bf5 Mon Sep 17 00:00:00 2001 From: Farid Zakaria Date: Fri, 11 Jul 2025 09:39:34 -0700 Subject: [PATCH 2/3] merge stderr to stdout so we can emit it --- src/libfetchers/git-utils.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index 563c2180d..d76f6879d 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -552,11 +552,12 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this // we're using --quiet for now. Should process its stderr. .args = gitArgs, .input = {}, + .mergeStderrToStdout = true, .isInteractive = true }); - + if (status > 0) { - throw Error("Failed to fetch git repository %s: %s", url, output); + throw Error("Failed to fetch git repository %s : %s", url, output); } } From a4f548fed1b6fe7c7f1882c3214175d4147ddfe4 Mon Sep 17 00:00:00 2001 From: Farid Zakaria Date: Fri, 11 Jul 2025 13:28:04 -0700 Subject: [PATCH 3/3] Fix FetchGit test --- src/libfetchers/git.cc | 14 +++++++++++-- tests/functional/fetchGit.sh | 39 +++++++++++++++++++----------------- 2 files changed, 33 insertions(+), 20 deletions(-) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index cf255c001..88fe2e83d 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -444,7 +444,11 @@ struct GitInputScheme : InputScheme // repo, treat as a remote URI to force a clone. static bool forceHttp = getEnv("_NIX_FORCE_HTTP") == "1"; // for testing auto url = parseURL(getStrAttr(input.attrs, "url")); - bool isBareRepository = url.scheme == "file" && !pathExists(url.path + "/.git"); + + // Why are we checking for bare repository? + // well if it's a bare repository we want to force a git fetch rather than copying the folder + bool isBareRepository = url.scheme == "file" && pathExists(url.path) && + !pathExists(url.path + "/.git"); // // FIXME: here we turn a possibly relative path into an absolute path. // This allows relative git flake inputs to be resolved against the @@ -462,6 +466,12 @@ struct GitInputScheme : InputScheme "See https://github.com/NixOS/nix/issues/12281 for details.", url); } + + // If we don't check here for the path existence, then we can give libgit2 any directory + // and it will initialize them as git directories. + if (!pathExists(url.path)) { + throw Error("The path '%s' does not exist.", url.path); + } repoInfo.location = std::filesystem::absolute(url.path); } else { if (url.scheme == "file") @@ -599,7 +609,7 @@ struct GitInputScheme : InputScheme ? cacheDir / ref : cacheDir / "refs/heads" / ref; - bool doFetch; + bool doFetch = false; time_t now = time(0); /* If a rev was specified, we need to fetch if it's not in the diff --git a/tests/functional/fetchGit.sh b/tests/functional/fetchGit.sh index a41aa35c0..dc5d8f818 100755 --- a/tests/functional/fetchGit.sh +++ b/tests/functional/fetchGit.sh @@ -53,6 +53,27 @@ rm -rf $TEST_HOME/.cache/nix path=$(nix eval --impure --raw --expr "(builtins.fetchGit file://$repo).outPath") [[ $(cat $path/hello) = world ]] +# Fetch again. This should be cached. +# NOTE: This has to be done before the test case below which tries to pack-refs +# the reason being that the lookup on the cache uses the ref-file `/refs/heads/master` +# which does not exist after packing. +mv $repo ${repo}-tmp +path2=$(nix eval --impure --raw --expr "(builtins.fetchGit file://$repo).outPath") +[[ $path = $path2 ]] + +[[ $(nix eval --impure --expr "(builtins.fetchGit file://$repo).revCount") = 2 ]] +[[ $(nix eval --impure --raw --expr "(builtins.fetchGit file://$repo).rev") = $rev2 ]] +[[ $(nix eval --impure --raw --expr "(builtins.fetchGit file://$repo).shortRev") = ${rev2:0:7} ]] + +# Fetching with a explicit hash should succeed. +path2=$(nix eval --refresh --raw --expr "(builtins.fetchGit { url = file://$repo; rev = \"$rev2\"; }).outPath") +[[ $path = $path2 ]] + +path2=$(nix eval --refresh --raw --expr "(builtins.fetchGit { url = file://$repo; rev = \"$rev1\"; }).outPath") +[[ $(cat $path2/hello) = utrecht ]] + +mv ${repo}-tmp $repo + # Fetch when the cache has packed-refs # Regression test of #8822 git -C $TEST_HOME/.cache/nix/gitv3/*/ pack-refs --all @@ -83,24 +104,6 @@ path2=$(nix eval --raw --expr "(builtins.fetchGit { url = file://$repo; rev = \" # But without a hash, it fails. expectStderr 1 nix eval --expr 'builtins.fetchGit "file:///foo"' | grepQuiet "'fetchGit' doesn't fetch unlocked input" -# Fetch again. This should be cached. -mv $repo ${repo}-tmp -path2=$(nix eval --impure --raw --expr "(builtins.fetchGit file://$repo).outPath") -[[ $path = $path2 ]] - -[[ $(nix eval --impure --expr "(builtins.fetchGit file://$repo).revCount") = 2 ]] -[[ $(nix eval --impure --raw --expr "(builtins.fetchGit file://$repo).rev") = $rev2 ]] -[[ $(nix eval --impure --raw --expr "(builtins.fetchGit file://$repo).shortRev") = ${rev2:0:7} ]] - -# Fetching with a explicit hash should succeed. -path2=$(nix eval --refresh --raw --expr "(builtins.fetchGit { url = file://$repo; rev = \"$rev2\"; }).outPath") -[[ $path = $path2 ]] - -path2=$(nix eval --refresh --raw --expr "(builtins.fetchGit { url = file://$repo; rev = \"$rev1\"; }).outPath") -[[ $(cat $path2/hello) = utrecht ]] - -mv ${repo}-tmp $repo - # Using a clean working tree should produce the same result. path2=$(nix eval --impure --raw --expr "(builtins.fetchGit $repo).outPath") [[ $path = $path2 ]]