Repository navigation
Authenticate network git commands - #6541
Conversation
3b813e8 to
4da8cd2
Compare
mislav
left a comment
mislav
left a comment
There was a problem hiding this comment.
Implementation looks great but the diff in tests seems excessive. A potential solution proposed in comments
| name: "shorthand", | ||
| args: "GIST", | ||
| want: "git clone https://gist.github.com/GIST.git", | ||
| want: `git -c credential.helper= -c credential.helper=!"some/path/gh" auth git-credential clone https://gist.github.com/GIST.git`, |
There was a problem hiding this comment.
It's potentially worrying that this implementation detail is now added to the expectation in so many tests unrelated to authentication itself. If we ever change a single character of this, all these tests will also have to be updated.
Should these test assertions attempt to normalize the git invocation line with a single shared helper that can be kept to sync with the authentication implementation?
There was a problem hiding this comment.
Yeah I agree it is not great. It is a product of how these tests were written in the past. Two solutions I can think of:
- Modify all these commands to use an interface which is fulfilled by
git.Clientand then in tests mock out the method calls. This would be a good amount of work, but is also probably the most "go" way. - Add a function to the
CommandStubberspecific for registeringgitcommands which would create a single place where the authentication string would be.
What are your thoughts?
There was a problem hiding this comment.
Agreed option (1) would be the most "correct" but perhaps infeasible for this PR. How about option 3: make a test helper that converts a string like git -c credential.helper= -c credential.helper=... <command> <args> to git <command> <args> and use that in equality assertions?
There was a problem hiding this comment.
I don't necessarily like that approach as it weakens the assertions about the git commands being invoked and would no longer assert that gh was set as the credential helper, which is the purpose of this PR.
4da8cd2 to
81fb040
Compare
mislav
left a comment
mislav
left a comment
There was a problem hiding this comment.
I dig the workaround you ended up on; thanks. Only optional suggestions remain
4 participants
The PR changes
gitclient to now useghas the default credential manager for authentication. For now, this is only applying to networkgitcommands such asFetch,Pull,Pull,Clone,AddRemote. The majority of the line changes in this PR are fixing up tests.cc #2944