Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libfetchers/git: Allow Git Remote Helpers #10567

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

libfetchers/git: Allow Git Remote Helpers #10567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/libfetchers/git-utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include "environment-variables.hh"
#include "git-utils.hh"
#include "fs-input-accessor.hh"
#include "input-accessor.hh"
Expand Down Expand Up @@ -377,18 +378,23 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this<GitRepoImpl>
auto dir = this->path;
Strings gitArgs;
if (shallow) {
gitArgs = { "-C", dir.string(), "fetch", "--quiet", "--force", "--depth", "1", "--", url, refspec };
gitArgs = { "fetch", "--quiet", "--force", "--depth", "1", "--", url, refspec };
}
else {
gitArgs = { "-C", dir.string(), "fetch", "--quiet", "--force", "--", url, refspec };
gitArgs = { "fetch", "--quiet", "--force", "--", url, refspec };
}


std::map<std::string, std::string> gitEnv = getEnv();
gitEnv.emplace("GIT_DIR", dir.string());

runProgram(RunOptions {
.program = "git",
.searchPath = true,
// FIXME: git stderr messes up our progress indicator, so
// we're using --quiet for now. Should process its stderr.
.args = gitArgs,
.environment = gitEnv,
.input = {},
.isInteractive = true
});
Expand Down
7 changes: 6 additions & 1 deletion src/libfetchers/unix/git.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,11 +167,16 @@ struct GitInputScheme : InputScheme
{
std::optional<Input> inputFromURL(const ParsedURL & url, bool requireTree) const override
{
if (url.scheme != "git" && url.scheme.compare(0, 4, "git+") != 0)
return {};

if (url.scheme != "git" &&
url.scheme != "git+http" &&
url.scheme != "git+https" &&
url.scheme != "git+ssh" &&
url.scheme != "git+file") return {};
url.scheme != "git+file")
warn("URL scheme '%s' is non-standard and requires 'git-remote-%s'.", url.scheme, url.scheme.substr(4, url.scheme.length() - 4));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should come with a way to turn off the warning, for users who consistently use this.
I do think dependencies on remote helpers should be discouraged, because they do make expressions that use rely on them harder to use, so enabling the warning by default would be good.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This applies to basically flake inputs and fetchTree. We could also silence it automatically when a helper is requested by any installable on the CLI.

Copy link
Contributor

@fricklerhandwerk fricklerhandwerk May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This applies to basically flake inputs and fetchTree. We could also silence it automatically when a helper is requested by any installable on the CLI.

This sounds like a hack for something that may as well be two separate code paths, see #10567 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was still trying to figure out requirements, not describing an implementation.

The hack is of your inference. I don't think we need anything truly new architecturally.
The warning could be controlled by a global setting, that besides being a normal setting is set to true by the CLI when it encounters an explicit need for remote helpers in its installables.

Do you really mean a literal extra code path? That sounds a lot heavier than a boolean that controls a warning.



auto url2(url);
if (hasPrefix(url2.scheme, "git+")) url2.scheme = std::string(url2.scheme, 4);
Expand Down
Loading