Allow alternative remotes to be handled by LFS #5066

srohmen · 2022-07-04T13:35:50Z

This fixes issue #3835. LFS could not derive the right remote endpoint under certain circumstances. For example, in a detached state, a simple heuristic was used to derive the remote (i.e. branch.*.remote, remote.lfsdefault, any other single remote, origin). This fails if there are multiple remotes in play. This patch derives the corresponding remote from the commit hash / treeish.

Thanks to @stanhu for providing the first version which this PR is based on.

srohmen · 2022-07-11T14:58:52Z

what can I do about the ruby gem issue within the CI job? it does not look like my patch is responsible for this

bk2204 · 2022-07-11T15:09:21Z

It's not. I'm working on finishing the patch which should fix that and I'll re-run the job once it's merged.

bk2204

Hey,

Thanks for the patch, and welcome to Git LFS!

Overall, I think this could be a valuable feature. I think we'll need to make these options since overall they change behaviour in an incompatible way. We'll also need some integration tests here (those would be in the t/ directory). Finally, CI has been fixed, but you'll need to rebase on the main branch to pick up the fixes.

If you need any help on getting this into shape, let me know, and I'll try to help you out. My apologies for taking so long to get to this.

commands/command_filter_process.go

lfs/gitfilter_smudge.go

srohmen · 2022-07-16T12:16:14Z

Thanks @bk2204 for your feedback! We (a co-worker @Leo1690 and me) have treated most of your comments. I rebased my branch, squashed the new changes in the old commits and pushed it to my feature branch. However, we are still working on the test environment. We have also shell scripts which reproduce the issue, but to port that to the environment within t/ of git-lfs it takes a bit more effort. I hope we can at least review and discuss the updated patch in the meantime.

Leo1690 · 2022-07-17T21:35:05Z

Thanks @bk2204 for your feedback!
I have a couple of issues creating the tests.

By default all remotes uses the same lfs server. To reproduce the issue I need each remote to be linked to a different lfs server and I cannot find out how to do it
I can use lfs directly with the file system but I get this error batch {request: missing protocol: "file://} when I use sparse-checkout and cherry-pick. I tried to set up the standalone transfer mode but It did not work.

Leo1690 · 2022-07-18T09:12:27Z

Additionally, I can try to mount the remotes as docker containers and and use git with ssh to transfer lfs data, but I do not know if you would accept a merge request that adds docker containers to your testing environment.
I think that if we use docker containers as remotes or repos in the test suite it would help to isolate the git configuration and Env variables between tests. I believe it is worth considering.

srohmen · 2022-07-18T11:55:29Z

I pushed a commit authored by @Leo1690 which implements the tests. I hope it is fine that this MR contains now two different authors in the line of commits. It seems that using a more recent git version did do the trick to support file:// URL based remotes.

bk2204 · 2022-07-19T20:02:57Z

Thanks @bk2204 for your feedback! I have a couple of issues creating the tests.
1. By default all remotes uses the same lfs server. To reproduce the issue I need each remote to be linked to a different lfs server and I cannot find out how to do it

I believe, provided you use different URLs, the test LFS server should expose different repositories. It should be sufficient to expose multiple repositories on the same server instead of different servers, I would think.

2. I can use lfs directly with the file system but I get this error batch {request: missing protocol: "file://} when I use sparse-checkout and cherry-pick. I tried to set up the standalone transfer mode but It did not work.

The standalone transfer agent should be built-in automatically for file URLs. However, it is known that it requires an absolute URL and it doesn't work for Windows SMB paths. Sparse checkout almost certainly doesn't work and is tracked in #3803.

Can you provide an example of a shell script that doesn't work for you (without sparse checkout)?

Leo1690 · 2022-07-20T10:12:46Z

Hi @bk2204.

If all remotes uses the same lfs server, the tests succeed even without our patch. The problem shows up only if different remotes point to different lfs servers. The issue can be reproduced with 2 gitlab repos.
I solved the problem with the file system remotes in the /t folder, and our tests succeed with our patch and fails without it (as expected). So I was finally able to implement all necessary tests including the sparse-checkout test.
I saw that the CI failed. If I am not wrong, it looks like the problem is when I attempt to access a folder outside of $TRASHDIR. Anyway, I changed the tests to work only inside a subdirectory in $TRASHDIR/home
make: Leaving directory /root/src/git-lfs/rpm/BUILD/git-lfs-3.2.0/t
error: Bad exit status from /var/tmp/rpm-tmp.cqNFbK (%check)

bk2204

This is looking pretty good. I left some more comments, but I think we should be good to go soon.

bk2204 · 2022-07-20T12:17:38Z

t/t-multiple-remotes.sh

+  HOME=$MRHOME
+  git config --global user.name "Git LFS Tests"
+  git config --global user.email "[email protected]"
+}


I don't think we should be setting HOME here, since the test library does that, and we also don't need to set the Git config values, since those should already be set.

Also, please here and everywhere else, double-quote all variables. On Windows, it's extremely common for a user's home directory (and thus all paths to their repositories) to have spaces, and we need to handle that gracefully.

You are right. We added a comment to explain our motivation for that.
The global configuration sets a common http endpoint (ip:port) for lfs
We deactivate the git global configuration related with the http protocol
by setting a new .gitconfig file
Additionally, We are not setting a new $HOME. Instead We are removing .gitconfig file and adding the global settings we need. Which it seems ok because at the begging of the next test, the original .gitconfig file is restored by the function begin_test()

I'm sorry, I still think there's something I'm missing. What specific setting is causing the problem? In t/t-standalone-file.sh, we use file:/// URLs just fine, so it can't be lfs.url.

Can we keep the regular .gitconfig and simply unset the offending option in each test with git config --global --unset? I'd rather we do that because it means that if we add an important new setting into the testsuite in the future, the entire testsuite picks it up.

I am sorry. I seems that it was not necessary to chage the global configuration after all. Thank you for pointing that out. The new patch uses the global seetings provided by setup()

t/t-multiple-remotes.sh

bk2204 · 2022-07-20T12:23:11Z

t/t-multiple-remotes.sh

+  rm -rf $smain
+  rm -rf $sfork
+  rm -rf $cmain
+  rm -rf $cfork


Instead of deleting and recreating repos like this, let's use a parameter which is specific to each call and set that differently in each individual test. Then we can avoid different tests affecting each other and let the cleanup code at the end of the test clean up all of the test data for us.

You can see an example in t/t-pull.sh how we do this with setup_remote_repo.

I agree. That looks cleaner. We changed the new function prepare_forks() to create remote and local repos with different paths
prepare_forks test_case

bk2204 · 2022-07-20T12:24:49Z

t/t-multiple-remotes.sh

+  git push -u fr master
+  #Add a .bin in main repo
+  touch a.bin
+  echo 1234 >> a.bin


We typically prefer printf over echo in our codebase. It's more consistent and flexible, and in some Windows environments, echo may insert CRLF, which makes our hashes change.

We fixed it.
printf "1234" > a.bin

bk2204 · 2022-07-20T12:25:04Z

t/t-multiple-remotes.sh

+  git push mr master
+  prepare_consumer $cfork
+}
+begin_test "pull from different remote"


Let's put a space here and in between each set of tests.

We fixed it.
prepare_consumer "$cfork"
}

begin_test "reset to different remote"
(

bk2204 · 2022-07-20T12:32:00Z

git/git.go

+// We could use git for-each-ref with %(upstream:remotename) if there were a tracking branch,
+// but this is not guaranteed to exist either.
+func remoteForRef(refname string) string {
+	tracerx.Printf("Git Working Ref %", refname)


I don't think this works with just a single percent sign. Also, typically, we write the trace messages downcase, often with a prefix (so, here, “git: working ref: %s”), which would be great to fix here and elsewhere. We typically write mostly fragments or phrases which are punctuated with colons as necessary to avoid ambiguity.

We fixed it.
tracerx.Printf("git: working ref: %s", refname)
tracerx.Printf("git: ref remote: cannot determine remote for ref %s since remote %s contains a slash", refname, name)
tracerx.Printf("git: working remote %s", remote)
tracerx.Printf("git: remote treeish: no valid remote refs parsed for %q", treeish)
tracerx.Printf("git: smudge: default remote failed. searching alternate remotes")

bk2204

I left a few more comments. We probably want to also add some documentation in AsciiDoc in the git-lfs-config manual page so folks will know how this works.

bk2204 · 2022-07-21T20:15:46Z

t/t-multiple-remotes.sh

+  HOME=$MRHOME
+  git config --global user.name "Git LFS Tests"
+  git config --global user.email "[email protected]"
+}


I'm sorry, I still think there's something I'm missing. What specific setting is causing the problem? In t/t-standalone-file.sh, we use file:/// URLs just fine, so it can't be lfs.url.

Can we keep the regular .gitconfig and simply unset the offending option in each test with git config --global --unset? I'd rather we do that because it means that if we add an important new setting into the testsuite in the future, the entire testsuite picks it up.

bk2204 · 2022-07-21T20:24:24Z

t/t-multiple-remotes.sh

+  sfork="$HOME"/"$reponame"-"$testcase"-fork-remote.git
+  cmain="$HOME"/"$reponame"-"$testcase"-main-repo
+  cfork="$HOME"/"$reponame"-"$testcase"-fork-repo
+  prepare_remote "$smain"


Can we change this to setup_remote_repo "$smain" and then use "$REMOTEDIR/$smain.git"? We shouldn't need the git lfs install since that's already been done once by our setup code, and if we don't delete the .gitconfig file as I suggested, then we'll have the configuration persisted.

Basically, my request here is to use as much of the same setup code and other helper functions as possible. The style of this test is currently very different from our other tests, and if we can make it as similar as possible, then it will be easier to maintain. You can see what t/t-verify.sh looks like, which I think is a good example of a typical test.

You are right, I adapted the tests to use the function setup_remote_repo()

srohmen · 2022-07-22T14:23:36Z

I added a commit which extends the documentation. I hope this is to your liking what I have phrased @bk2204

bk2204

This looks good to me. Thanks so much for the patch and working with me on this.

I'm going to announce this as an experimental feature in v3.3.0 so that we can see if there are any bugs and shake them out before we commit to supporting it long term.

I've kicked off CI, and I'll check back on Monday to verify it's passed and then merge.

srohmen · 2022-07-24T23:22:25Z

Oh, I just fixed the phrasing of a comment of a function. I didn't knew that the approval would reset completely. So this is just a minor cosmetic last minute change.

bk2204 · 2022-07-25T13:25:36Z

Hmmm, it looks like CI is broken. Could you look into that?

srohmen · 2022-07-25T16:58:03Z

@bk2204 2 CI jobs failed because the feature can only be supported for git version 2.27 or newer. We added an exclusion rule for the tests. The other job, which uses the git development version of git, is not explainable. We never touched any of the other tests and the new feature is disabled in that setup. In particular, the job which uses the default git version on Ubuntu runs without any issue. My guess is right now that git upstream behavior changed such that the tests are failing now. Can we run the same CI job without our patch to confirm this theory?

bk2204 · 2022-07-25T17:38:52Z

I'm testing a PR in #5082 to see if it makes any difference.

Before this change, LFS was only respecting remotes which were directly tracked by a branch, or, in a detached state, using a heuristic. Sometimes this failed as reported in issue git-lfs#3835. This patch allows to use the tree-ish to retrieve a corresonding remote. Thanks to Stan Hu, who provided inspiration to fix this bug.

The tree-ish during cherry-pick points to the resulting commit. This commit has never been pushed to any remote and thus it can not resolve to a remote. This patch introduces a fallback mechanism to check all remotes for the existence of the LFS object.

… and lfs.remote.searchall options

srohmen · 2022-07-26T14:42:28Z

@bk2204 I just rebased the branch on your latest fix. I just hope this fixes the CI issues we had before.

srohmen requested a review from a team as a code owner July 4, 2022 13:35

srohmen force-pushed the fix-3835 branch from 1649f35 to eb007dd Compare July 8, 2022 18:28

bk2204 reviewed Jul 12, 2022

View reviewed changes

commands/command_filter_process.go Show resolved Hide resolved

lfs/gitfilter_smudge.go Show resolved Hide resolved

lfs/gitfilter_smudge.go Show resolved Hide resolved

srohmen force-pushed the fix-3835 branch 2 times, most recently from e1037c7 to cd2a789 Compare July 16, 2022 12:08

srohmen force-pushed the fix-3835 branch from 956b9e7 to 09250aa Compare July 20, 2022 10:05

bk2204 reviewed Jul 20, 2022

View reviewed changes

srohmen force-pushed the fix-3835 branch from 09250aa to eb4c737 Compare July 20, 2022 17:01

bk2204 reviewed Jul 21, 2022

View reviewed changes

srohmen force-pushed the fix-3835 branch 4 times, most recently from 34c4ee1 to c85a7c0 Compare July 22, 2022 11:48

bk2204 approved these changes Jul 22, 2022

View reviewed changes

srohmen force-pushed the fix-3835 branch from c85a7c0 to 2a92297 Compare July 24, 2022 23:00

srohmen force-pushed the fix-3835 branch from 2a92297 to 00d4ed0 Compare July 25, 2022 16:53

srohmen and others added 3 commits July 26, 2022 16:38

Add tests for extension which allow to handle multiple remotes by LFS

abb5c99

docs: extend config documentation to explain new fs.remote.autodetect…

55f2c0a

… and lfs.remote.searchall options

srohmen force-pushed the fix-3835 branch from 00d4ed0 to 55f2c0a Compare July 26, 2022 14:40

bk2204 changed the title ~~Fix 3835: Allowing alternative remotes to be handled by LFS~~ Allow alternative remotes to be handled by LFS Jul 27, 2022

bk2204 merged commit a927e5d into git-lfs:main Jul 27, 2022

tmortagne mentioned this pull request Sep 13, 2023

Smudge error: Error downloading object for "git checkout" from alternative remote #3835

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow alternative remotes to be handled by LFS #5066

Allow alternative remotes to be handled by LFS #5066

srohmen commented Jul 4, 2022

srohmen commented Jul 11, 2022

bk2204 commented Jul 11, 2022

bk2204 left a comment

srohmen commented Jul 16, 2022 •

edited

Loading

Leo1690 commented Jul 17, 2022 •

edited

Loading

Leo1690 commented Jul 18, 2022

srohmen commented Jul 18, 2022 •

edited

Loading

bk2204 commented Jul 19, 2022

Leo1690 commented Jul 20, 2022

bk2204 left a comment

bk2204 Jul 20, 2022

Leo1690 Jul 20, 2022 •

edited

Loading

bk2204 Jul 21, 2022

Leo1690 Jul 22, 2022

bk2204 Jul 20, 2022

Leo1690 Jul 20, 2022 •

edited

Loading

bk2204 Jul 20, 2022

Leo1690 Jul 20, 2022 •

edited

Loading

bk2204 Jul 20, 2022

Leo1690 Jul 20, 2022 •

edited

Loading

bk2204 Jul 20, 2022

Leo1690 Jul 20, 2022 •

edited

Loading

bk2204 left a comment

bk2204 Jul 21, 2022

bk2204 Jul 21, 2022

Leo1690 Jul 22, 2022 •

edited

Loading

srohmen commented Jul 22, 2022

bk2204 left a comment

srohmen commented Jul 24, 2022 •

edited

Loading

bk2204 commented Jul 25, 2022

srohmen commented Jul 25, 2022 •

edited

Loading

bk2204 commented Jul 25, 2022

srohmen commented Jul 26, 2022

Allow alternative remotes to be handled by LFS #5066

Allow alternative remotes to be handled by LFS #5066

Conversation

srohmen commented Jul 4, 2022

srohmen commented Jul 11, 2022

bk2204 commented Jul 11, 2022

bk2204 left a comment

Choose a reason for hiding this comment

srohmen commented Jul 16, 2022 • edited Loading

Leo1690 commented Jul 17, 2022 • edited Loading

Leo1690 commented Jul 18, 2022

srohmen commented Jul 18, 2022 • edited Loading

bk2204 commented Jul 19, 2022

Leo1690 commented Jul 20, 2022

bk2204 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leo1690 Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leo1690 Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leo1690 Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leo1690 Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leo1690 Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

bk2204 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leo1690 Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

srohmen commented Jul 22, 2022

bk2204 left a comment

Choose a reason for hiding this comment

srohmen commented Jul 24, 2022 • edited Loading

bk2204 commented Jul 25, 2022

srohmen commented Jul 25, 2022 • edited Loading

bk2204 commented Jul 25, 2022

srohmen commented Jul 26, 2022

srohmen commented Jul 16, 2022 •

edited

Loading

Leo1690 commented Jul 17, 2022 •

edited

Loading

srohmen commented Jul 18, 2022 •

edited

Loading

Leo1690 Jul 20, 2022 •

edited

Loading

Leo1690 Jul 20, 2022 •

edited

Loading

Leo1690 Jul 20, 2022 •

edited

Loading

Leo1690 Jul 20, 2022 •

edited

Loading

Leo1690 Jul 20, 2022 •

edited

Loading

Leo1690 Jul 22, 2022 •

edited

Loading

srohmen commented Jul 24, 2022 •

edited

Loading

srohmen commented Jul 25, 2022 •

edited

Loading