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.
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
Add allRefs = true
argument to fetchGit
to explicitly perform a full checkout
#3814
Conversation
Git clone fetches all branches by default, that's why I think fetchGit should do the same, by simply omitting the ref argument. |
Well, we're not cloning a git repo though, only the specified ref (or |
Also, I think it's fine to fetch master only by default: unless you need it, you shouldn't have to fetch a full repo. |
I know, but instead of falling back to master we could fall back to "all". IMHO that's more natural, because |
@bjornfor I agree with @edolstra's reasoning in #2431 (comment). This should just be an escape-hatch if you really need it. |
f49c707
to
9cdd68e
Compare
Rebased onto latest master. |
@edolstra would you be fine with the overall approach taken here to work around the usability-issues of fetchGit? |
Rebased onto master now. |
cc @edolstra would you mind taking a look at this? :) |
@edolstra @domenkozar rebased onto latest master. Would you mind taking a look? :) |
@edolstra is there anything missing in this PR? :) |
For backwards compatibility, instead of adding an BTW, when using this, what |
Unfortunately I don't remember the exact details why I decided to create a new attribute, but I think that this was to avoid doing even more magic with However I just realized that we'd have to use a string anyways to get this working as flake input (unless we alter the logic in
None as I just realized that using this as flake-input doesn't even evaluate (only tested with The reason is that flake-attrs can't seem to be a nix/src/libexpr/flake/flake.cc Lines 118 to 122 in 5999978
|
ping @edolstra how shall we proceed here? :) |
ping @edolstra |
We can just handle booleans there. The only reason they're not implemented is because there was no need at the time. |
...which means that the current approach is fine? |
cc @edolstra anything else tbd here now? :) |
@edolstra is the current change fine now? |
cc @edolstra anything tbd here? :) |
cc @edolstra can we merge this? |
|
||
auto result = runProgram(checkCommitOpts); | ||
if (WEXITSTATUS(result.first) == 128 | ||
&& result.second.find("bad file") != std::string::npos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking for strings like "bad file" in stderr seems pretty fragile... Do we actually need this test? Presumably a wrong rev
will fail in the subsequent call to git archive
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid no: if git cat-file
errors, the exit code is always 128, the reason for the error is on stderr.
…revision can't be checked out A common pitfall when using e.g. `builtins.fetchGit` is the `fatal: not a tree object`-error when trying to fetch a revision of a git-repository that isn't on the `master` branch and no `ref` is specified. In order to make clear what's the problem, I added a simple check whether the revision in question exists and if it doesn't a more meaningful error-message is displayed: ``` nix-repl> builtins.fetchGit { url = "https://github.com/owner/myrepo"; rev = "<commit not on master>"; } moderror: --- Error -------------------------------------------------------------------- nix Cannot find Git revision 'bf1cc5c648e6aed7360448a3745bb2fe4fbbf0e9' in ref 'master' of repository 'https://gitlab.com/Ma27/nvim.nix'! Please make sure that the rev exists on the ref you've specified or add allRefs = true; to fetchGit. ``` Closes NixOS#2431
Sometimes it's necessary to fetch a git repository at a revision and it's unknown which ref contains the revision in question. An example would be a Cargo.lock which only provides the URL and the revision when using a git repository as build input. However it's considered a bad practice to perform a full checkout of a repository since this may take a lot of time and can eat up a lot of disk space. This patch makes a full checkout explicit by adding an `allRefs` argument to `builtins.fetchGit` which fetches all refs if explicitly set to true. Closes NixOS#2409
@edolstra rebased, fixed the docs and also added a test :) |
Thanks, merged! |
Originally I thought so too. But in practice this could leave missing commits in master branch unnoticed, until a feature branch gets deleted on remote. |
Sometimes it's needed to fetch the revision of a git repository without knowing the specific
ref
. In those cases it's needed to perform a full checkout which is now possible by declaringbuiltins.fetchGit { allRefs = true; /* url, rev, etc */ }
.Refs #2431 #2409
For further details, please read the descriptions of the two commits.
cc @edolstra