Resolve reference for remote git repositories (makes fetchGit work with non-'master' branch) #4638

orbekk · 2021-03-14T17:58:06Z

Resolves the HEAD reference from the remote repository instead of assuming "master".

For remote repositories, the HEAD ref is cached with the same cache policy as the local cached repo.

Fixes #4456.

orbekk · 2021-04-20T12:06:43Z

Update: I've rebased the pull request and added a more explicit test in tests/fetchGit.sh.

nixos-discourse · 2021-07-17T01:30:02Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/562

minijackson · 2022-01-14T08:16:48Z

What is needed to get this PR merged ? I'm quite interested in this functionality since it can cause some confusion for Nix newcomers that create projects through GitHub or GitLab.

orbekk · 2022-01-14T13:05:23Z

@minijackson If someone is willing to review this, I can make the necessary changes (if any) to bring the patches up to date. I haven't heard anything so far.

minijackson

Great stuff! In my opinion, only the first comment about the --symref option is important, the rest are just minor comments.

src/libfetchers/git.cc

minijackson · 2022-01-21T07:52:13Z

src/libfetchers/git.cc

+{
+    // Append something in front of the url to prevent collision with the
+    // cached repo.
+    Path cachePath = getCacheDir() + "/nix/gitv3/" + hashString(htSHA256, "<ref>|" + actualUrl).to_string(Base32, false);


Can this be factorized with the similar line in fetch?

If I'm not mistaken, it is possible to get the remote HEAD by reading the HEAD file in the cached repository. Is it possible to do it here?

Added a helper function for cache path.

HEAD is not currently updated in the cached repo (it is initialized to __nix_dummy_branch). I think we could use it instead of the cache file since it's not used for any other purpose, but it seems more straightforward to just write it to a separate file. Let me know if you still prefer using HEAD.

I personally think that one cache dir is easier to maintain than two, but I do not feel that strongly about it. If you feel this is more straightforward, I won't bother you ^^

On a side note the getCachePath function can also be used on line 418 of git.cc.

Updated the pull request to maintain a single git repo as the cache, and storing the HEAD as a symbolic ref within the cache. This complicated the logic in fetch() slightly, so I also added a commit with some refactoring which I think makes this easier to understand overall.

src/libfetchers/git.cc

minijackson · 2022-01-24T15:05:31Z

Built it with nix build -L 'github:NixOS/nix?ref=pull/4638/head' and tested it for a bit, works wonders.

orbekk · 2022-01-29T22:42:55Z

Built it with nix build -L 'github:NixOS/nix?ref=pull/4638/head' and tested it for a bit, works wonders.

Thank you for the review and additional testing, @minijackson.

orbekk · 2022-01-30T16:53:35Z

Added another commit to update the documentation to account for these changes, and explains how local repositories are handled.

Fixes #5900.

minijackson

LGTM!

All tests pass on my end.

orbekk · 2022-01-31T12:28:22Z

Hi @edolstra. I think this pull request is in a good state and there seems to be some interest in this feature. Could you take a look?

FintanH · 2022-03-01T16:16:03Z

Would be good to see this PR land and #5139 resolved :) I ran into this issue while trying to introduce nix flake into a codebase but it choked on fetching a git repository that defaults to main and has no master ref.

bjornfor · 2022-04-18T21:43:56Z

Needs a rebase.

orbekk · 2022-04-23T00:11:01Z

@bjornfor Thanks, I have rebased and update the pull request. Please take a look.

bjornfor · 2022-04-23T08:21:42Z

@orbekk: Sorry, I was just pointing out that the PR needed a rebase. I don't have commit access nor feel qualified to review.

orbekk · 2022-04-23T10:45:00Z

Hi @thufschmitt, could you help me find a reviewer?

thufschmitt

Hi @thufschmitt, could you help me find a reviewer?

I guess I just did that 😛

Looks good overall, just a couple of small suggestions

src/libfetchers/git.cc

Resolves the HEAD reference from the remote repository instead of assuming "master".

Extract the handling of a local dirty workdir to a helper function.

The previous head caching implementation stored two paths in the local cache; one for the cached git repo and another textfile containing the resolved HEAD ref. This commit instead stores the resolved HEAD by setting the HEAD ref in the local cache appropriately.

Update the documentation about how `ref` is resolved if it is not specified. Add a note about special handling of local workdirs with `git+file`.

These utility functions can be shared between the git and github fetchers.

thufschmitt

Thanks, looks good :)

I’ll leave it for a couple days in case @edolstra wants to have another look since he’s the author of that part of the code, and I’ll merge it if nothing comes before that

bjornfor · 2022-05-02T19:28:03Z

Thanks!

orbekk · 2022-05-10T11:04:54Z

Thanks for the review @thufschmitt

NixOS#6290 introduced a regex pattern to account for tags when resolving sourcehut refs. NixOS#4638 reafactored the code, accidentally treating the pattern as a regular string, causing all non-HEAD ref resolving to break. This fixes the regression and adds more test cases to avoid future breakage.

#6290 introduced a regex pattern to account for tags when resolving sourcehut refs. #4638 reafactored the code, accidentally treating the pattern as a regular string, causing all non-HEAD ref resolving to break. This fixes the regression and adds more test cases to avoid future breakage. (cherry picked from commit 9f6b463)

orbekk force-pushed the read_head branch 5 times, most recently from e131691 to bc4e125 Compare March 26, 2021 14:55

orbekk force-pushed the read_head branch from bc4e125 to 8c4650e Compare April 20, 2021 12:02

orbekk changed the title ~~Resolve reference for remote git repositories~~ Resolve reference for remote git repositories (makes fetchGit work with non-'master' branch) Apr 20, 2021

abathur mentioned this pull request Jul 22, 2021

short form of fetchGit assumes master branch exists #4456

Closed

minijackson reviewed Jan 21, 2022

View reviewed changes

orbekk force-pushed the read_head branch 2 times, most recently from 6926bea to 1467741 Compare January 23, 2022 19:23

orbekk force-pushed the read_head branch 2 times, most recently from 99fabc9 to 375c48d Compare January 29, 2022 22:39

orbekk force-pushed the read_head branch from 24fd677 to e69f25b Compare January 30, 2022 16:50

orbekk force-pushed the read_head branch from e69f25b to c41ab3e Compare January 30, 2022 16:56

minijackson approved these changes Jan 31, 2022

View reviewed changes

FintanH mentioned this pull request Mar 2, 2022

error: no matching package named ... found nix-community/crate2nix#179

Open

veehaitch mentioned this pull request Mar 5, 2022

build(nix): add enarx package enarx/enarx#1455

Merged

orbekk force-pushed the read_head branch 2 times, most recently from 375c48d to 1979834 Compare April 21, 2022 01:13

orbekk force-pushed the read_head branch from 1979834 to 665e1c2 Compare April 22, 2022 23:28

thufschmitt requested changes Apr 25, 2022

View reviewed changes

src/libfetchers/git.cc Outdated Show resolved Hide resolved

src/libfetchers/git.cc Show resolved Hide resolved

Kjetil Orbekk added 6 commits April 29, 2022 18:42

Resolve reference for remote repository

401e60f

Resolves the HEAD reference from the remote repository instead of assuming "master".

Test fetchGit with non-'master' remote repo

05a3fba

Refactor fetching of dirty workdir

de54e1c

Extract the handling of a local dirty workdir to a helper function.

Update nix flake documentation of ref handling

c21afd6

Update the documentation about how `ref` is resolved if it is not specified. Add a note about special handling of local workdirs with `git+file`.

Extract git reference parsing to a shared library

9bf296c

These utility functions can be shared between the git and github fetchers.

orbekk force-pushed the read_head branch from 233eaf5 to 9bf296c Compare April 29, 2022 22:49

orbekk requested a review from thufschmitt April 29, 2022 23:34

thufschmitt approved these changes May 2, 2022

View reviewed changes

edolstra merged commit a26be9f into NixOS:master May 2, 2022

This was referenced Jun 10, 2022

Building a flake by another uid fails with unclear error message #6636

Closed

libfetchers/git: add missing --git-dir flags #6643

Merged

Misterio77 mentioned this pull request Jun 11, 2022

fix sourcehut brach/tag resolving regression #6653

Merged

treeowl mentioned this pull request Nov 23, 2022

Experiment using logict-sequence runtimeverification/haskell-backend#3376

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve reference for remote git repositories (makes fetchGit work with non-'master' branch) #4638

Resolve reference for remote git repositories (makes fetchGit work with non-'master' branch) #4638

orbekk commented Mar 14, 2021

orbekk commented Apr 20, 2021

nixos-discourse commented Jul 17, 2021

minijackson commented Jan 14, 2022

orbekk commented Jan 14, 2022

minijackson left a comment

minijackson Jan 21, 2022

minijackson Jan 21, 2022

orbekk Jan 23, 2022

minijackson Jan 24, 2022

minijackson Jan 24, 2022

orbekk Jan 29, 2022

minijackson commented Jan 24, 2022

orbekk commented Jan 29, 2022

orbekk commented Jan 30, 2022

minijackson left a comment •

edited

orbekk commented Jan 31, 2022

FintanH commented Mar 1, 2022

bjornfor commented Apr 18, 2022

orbekk commented Apr 23, 2022

bjornfor commented Apr 23, 2022

orbekk commented Apr 23, 2022

thufschmitt left a comment

thufschmitt left a comment

bjornfor commented May 2, 2022

orbekk commented May 10, 2022

Resolve reference for remote git repositories (makes fetchGit work with non-'master' branch) #4638

Resolve reference for remote git repositories (makes fetchGit work with non-'master' branch) #4638

Conversation

orbekk commented Mar 14, 2021

orbekk commented Apr 20, 2021

nixos-discourse commented Jul 17, 2021

minijackson commented Jan 14, 2022

orbekk commented Jan 14, 2022

minijackson left a comment

Choose a reason for hiding this comment

minijackson Jan 21, 2022

Choose a reason for hiding this comment

minijackson Jan 21, 2022

Choose a reason for hiding this comment

orbekk Jan 23, 2022

Choose a reason for hiding this comment

minijackson Jan 24, 2022

Choose a reason for hiding this comment

minijackson Jan 24, 2022

Choose a reason for hiding this comment

orbekk Jan 29, 2022

Choose a reason for hiding this comment

minijackson commented Jan 24, 2022

orbekk commented Jan 29, 2022

orbekk commented Jan 30, 2022

minijackson left a comment • edited

Choose a reason for hiding this comment

orbekk commented Jan 31, 2022

FintanH commented Mar 1, 2022

bjornfor commented Apr 18, 2022

orbekk commented Apr 23, 2022

bjornfor commented Apr 23, 2022

orbekk commented Apr 23, 2022

thufschmitt left a comment

Choose a reason for hiding this comment

thufschmitt left a comment

Choose a reason for hiding this comment

bjornfor commented May 2, 2022

orbekk commented May 10, 2022

minijackson left a comment •

edited