Account for url.<url>.insteadOf in all operations #1117

artagnon · 2016-03-31T02:25:58Z

We tackle the problem in two places: first, when a raw clone url is given, we
must load possible replacement for substrings from the config; second, when we
are fetching a certain remote's url from the config, we must try to replace the
relevant part of the url.

The implementation iterates over the map insteadOfs, and attempts to
strings.Replace() the url with all the key-value pairs.

technoweenie · 2016-03-31T14:36:51Z

Hi @artagnon, thanks for the patch 🤘 This is just an initial triage of the PR, so I haven't reviewed the code yet, but it now has a review label so one of us can come back to it. You do have a test that you should be able to replicate by running test/test-config.sh. The stderr output is echoing all the commands, until a command returns a non-zero exit code, failing the test. It's failing on this grep 'Endpoint=http://actual-url (auth=none)' env.log line near the bottom:

-- stderr --
    +set -e
    +reponame=default-config
    +mkdir default-config
    +cd default-config
    +git init
    +git remote add origin http://127.0.0.1:40053/default-config
    +tee env.log
    +git lfs env
    +grep 'Endpoint=http://127.0.0.1:40053/default-config.git/info/lfs (auth=none)' env.log
    +git config --file=.gitconfig lfs.url http://gitconfig-file-ignored
    +git config --file=.lfsconfig lfs.url http://lfsconfig-file
    +tee env.log
    +git lfs env
    +grep 'Endpoint=http://lfsconfig-file (auth=none)' env.log
    +git config lfs.url http://local-lfsconfig
    +tee env.log
    +git lfs env
    +grep 'Endpoint=http://local-lfsconfig (auth=none)' env.log
    +git config url.http://actual-url.insteadOf alias:
    +git lfs env
    +tee env.log
    +grep 'Endpoint=http://actual-url (auth=none)' env.log
    +end_test
    +test_status=1
    +set +x -e

https://travis-ci.org/github/git-lfs/jobs/119704573

artagnon · 2016-04-01T01:14:34Z

Oops, fixed the minor detail. Thanks.

diff --git a/test/test-config.sh b/test/test-config.sh
index a535651..bf4a63e 100755
--- a/test/test-config.sh
+++ b/test/test-config.sh
@@ -22,9 +22,10 @@ begin_test "default config"
   git lfs env | tee env.log
   grep "Endpoint=http://local-lfsconfig (auth=none)" env.log

-  git config url."http://actual-url".insteadOf alias:
+  git config url."http://actual-url/".insteadOf alias:
+  git config lfs.url alias:rest
   git lfs env | tee env.log
-  grep "Endpoint=http://actual-url (auth=none)" env.log
+  grep "Endpoint=http://actual-url/rest (auth=none)" env.log
 )
 end_test

technoweenie · 2016-04-11T18:08:45Z

lfs/config.go

+	uniqKeys := make(map[string]string)
+	for _, line := range lines {
+		// Why is this not "="?
+		pieces := strings.SplitN(line, " ", 2)


// Why is this not "="?

This is the first time the pieces var is used in this scope, so it must be declared. Same goes for values like key or value below. I suppose we could declare them above the for block, but I don't think it's worth the hassle.

No, I meant why the string we're splitting on is not "=": inconsistency in the way git config reports; see line 489.

Doh, sorry. That's silly of me. No clue why git config --list differs from --get-regexp, either. Looks like the only way we could parse the output of both commands the same is by using -z.

We tackle the problem in two places: first, when a raw clone url is given, we must load possible replacement for substrings from the config; second, when we are fetching a certain remote's url from the config, we must try to replace the relevant part of the url. The implementation iterates over the map insteadOfs, and attempts to strings.Replace() the url with all the key-value pairs.

technoweenie · 2016-04-12T01:44:11Z

Hey @artagnon, thanks for reworking this based on my feedback so quickly. I'm beginning the QA phase for Git LFS 1.2 now, but this will probably just barely make the cut :)

sinbad · 2016-04-12T13:46:16Z

If we're going to support insteadOf, should we also support pushInsteadOf? I added upload/download context on most operations to support pushurl (#949) so it should be doable.

sinbad · 2016-04-12T13:53:49Z

lfs/config.go

+	c.readGitInsteadOfConfig(listOutput, uniqInsteadOfs)
+	c.insteadOfs = uniqInsteadOfs
+
+	c.readGitConfigFromFiles(configFiles, 0, uniqRemotes)


There's a silent dependency here on readGitInsteadOfConfig being called before readGitConfigFromFiles, and having assigned c.insteadOfs before it, even though that member var isn't used anywhere else.

I think the call to readGitInsteadOfConfig should be nested inside the readGitConfigFromFiles to make this explicit, and the output uniqInsteadOfs not stored in the Config at all, just used to affect the behaviour of the rest of readGitConfigFromFiles, as it currently does. This would eliminate the hidden 'magic' ordering and unused state.

technoweenie · 2016-04-12T15:48:38Z

You think it's worth shipping this, or waiting until pushInsteadOf is supported? I think it's useful as-is, and pushInsteadOf could be added in 1.2.1 or 1.3 (whatever we do next).

I'm inclined to let https://github.com/github/git-lfs/pull/1117/files#r59379179 slide until the next version too.

@sinbad: if you feel strongly about it, we can hold off though.

sinbad · 2016-04-12T16:01:24Z

TBH this is the first time I've ever heard of these features, I'd never heard from anyone who used them before :) When I looked them up they just seemed to go together. If it's important to ship now I don't have major objections.

technoweenie · 2016-04-14T12:45:34Z

Hey @artagnon, I found a bug during my QA where it looks like this is doing the insteadof replacements twice for default remote urls in remote.<name>.url. It works great for lfs.url and remote.<name>.lfsurl though.

Unfortunately, I'm going to hold this back from Git LFS v1.2. I'd like the following to be fixed before I merge this:

Fix the double-replacement issue I mentioned above ^
Support url.<url>.pushinsteadof as suggested here.
Fix this readGitInsteadofConfig() dependency issue described here.

Thanks for your work on this. Let me know if I can help you out in anyway on here or gitter.

artagnon · 2016-04-15T01:46:57Z

Thanks for catching those bugs @technoweenie. I'll be on vacation for 10 days, and I'll continue development when I'm back.

technoweenie · 2016-05-10T21:13:47Z

@artagnon Hey, friendly nudge. Do you have time to pick this back up? If not, that's totally cool. I or someone else should be able to push this over the finish line.

artagnon · 2016-05-11T11:58:33Z

@technoweenie Hey, I don't think I have the time to finish this up in the
near future. In any case, I'll send a PR if I find this incomplete when I
find some time. Don't wait up for me though.

Thanks.

On Tue, May 10, 2016 at 5:14 PM risk danger olson notifications@github.com
wrote:

@artagnon https://github.com/artagnon Hey, friendly nudge. Do you have
time to pick this back up? If not, that's totally cool. I or someone else
should be able to push this over the finish line.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1117 (comment)

technoweenie · 2016-08-12T21:46:34Z

@artagnon Thanks again for submitting this PR. Unfortunately, I dropped the ball here. Lately, we've been on a refactoring kick in the config package (see #1425), meaning this PR doesn't merge cleanly anymore. I've rewritten the PR: #1443.

I also changed the implementation, since strings.Replace() doesn't work with some of the edge cases from git's documentation. The specific items are in bold:

Any URL that starts with this value will be rewritten to start, instead, with . In cases where some site serves a large number of repositories, and serves them with multiple access methods, and some users need to use different access methods, this feature allows people to specify any of the equivalent URLs and have Git automatically rewrite the URL to the best alternative for the particular user, even for a never-before-seen repository on the site. When more than one insteadOf strings match a given URL, the longest match is used.

Just FYI :)

Thanks again for your original patch. Hope to see you around again!

technoweenie added the review label Mar 31, 2016

technoweenie reviewed Apr 11, 2016
View reviewed changes

technoweenie mentioned this pull request Apr 12, 2016

Git LFS v1.2 #844

Closed

17 tasks

sinbad reviewed Apr 12, 2016
View reviewed changes

This was referenced May 4, 2016

Changelog after v1.2.0 #1180

Closed

Doesn't work if remote URL contains shorthands #693

Closed

technoweenie mentioned this pull request Aug 12, 2016

add support for url.*.insteadof in git config #1443

Merged

technoweenie closed this Aug 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account for url.<url>.insteadOf in all operations #1117

Account for url.<url>.insteadOf in all operations #1117

artagnon commented Mar 31, 2016

technoweenie commented Mar 31, 2016

artagnon commented Apr 1, 2016

technoweenie Apr 11, 2016

artagnon Apr 11, 2016

technoweenie Apr 12, 2016

technoweenie commented Apr 12, 2016

sinbad commented Apr 12, 2016

sinbad Apr 12, 2016

technoweenie commented Apr 12, 2016

sinbad commented Apr 12, 2016

technoweenie commented Apr 14, 2016

artagnon commented Apr 15, 2016

technoweenie commented May 10, 2016

artagnon commented May 11, 2016

technoweenie commented Aug 12, 2016

Account for url.<url>.insteadOf in all operations #1117

Account for url.<url>.insteadOf in all operations #1117

Conversation

artagnon commented Mar 31, 2016

technoweenie commented Mar 31, 2016

artagnon commented Apr 1, 2016

technoweenie Apr 11, 2016

Choose a reason for hiding this comment

artagnon Apr 11, 2016

Choose a reason for hiding this comment

technoweenie Apr 12, 2016

Choose a reason for hiding this comment

technoweenie commented Apr 12, 2016

sinbad commented Apr 12, 2016

sinbad Apr 12, 2016

Choose a reason for hiding this comment

technoweenie commented Apr 12, 2016

sinbad commented Apr 12, 2016

technoweenie commented Apr 14, 2016

artagnon commented Apr 15, 2016

technoweenie commented May 10, 2016

artagnon commented May 11, 2016

technoweenie commented Aug 12, 2016