allowing git to passthrough for a git external remote as compared to …#679
allowing git to passthrough for a git external remote as compared to …#679muschellij2 wants to merge 10 commits intor-lib:mainfrom
Conversation
|
Only failure seems to be a windows fail, not due to the code here. |
1 similar comment
|
Only failure seems to be a windows fail, not due to the code here. |
|
Thanks! I wonder if using an option + env var would make more sense in this case, instead of pushing this argument through everywhere, which is somewhat error prone if we also pass |
|
I think that would work better in that case but this seems to pass and
didn’t raise any flags, but it’s hard to know if you’ve gone down every …
rabbit hole. Without the pass through though, some of my installs fail if
I don’t add a PAT in addition to the SSH key.
On Wed, Feb 2, 2022 at 3:37 AM Gábor Csárdi ***@***.***> wrote:
Thanks! I wonder if using an option + env var would make more sense in
this case, instead of pushing this argument through everywhere, which is
somewhat error prone if we also pass ... around.
—
Reply to this email directly, view it on GitHub
<#679 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIGPLRJIIMJJQBCBJFDIX3UZDUL5ANCNFSM5J7X2NWQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
Best,
John
|
Why is that? If we set the default of the |
|
I think the issue is that if you specify |
|
If we use |
|
I think I've teased this out some more. The ScenarioYou have a I think I can boil the issue down to this how x = "git::git@github.com:USER/REPO"
remotes:::parse_one_extra(x)
#> $url
#> [1] "git@github.com:USER/REPO"
#>
#> $subdir
#> NULL
#>
#> $ref
#> NULL
#>
#> $credentials
#> NULL
#>
#> attr(,"class")
#> [1] "git2r_remote" "remote"We see this is remotes:::parse_one_extra(x, git = "external")
#> $url
#> [1] "git@github.com:USER/REPO"
#>
#> $subdir
#> NULL
#>
#> $ref
#> NULL
#>
#> attr(,"class")
#> [1] "xgit_remote" "remote"Created on 2022-02-03 by the reprex package (v2.0.1) This all happens because Line 72 in eb15a1e If you set that option to getOption, I think this behavior would be resolved for me. The only issue with this is that you specify git in install_git and that may not be preserved/the same in remotes:::git_remote (and therefore parse_one_extra) depending on the mismatch of the default option vs. explicit git argument, which I'm not sure is an "issue" vs. just the behavior.
Rabbit HoleHere I just run down where the pass through of Starting with
|
|
To summarize the behavior (with the Minimal change required: Then when you run: All scenarios below are assuming Package uses external and Deps uses externaloptions(remotes.git = "external")
install_git(..., git = "external")Package uses
|
|
Let me know which way you'd like me to go with this. I think the passthrough is fine, but may be harder to maintain, but the above issues exist for the |
|
Yeah, I agree that this is not ideal. OTOH if you are always using xgit or git2r, then you just need to set the option, no? So I am kind of OK with the dependencies not following the argument. Here is another idea, though. At the beginning of the functions that take the |
|
That works as well. Either way, I’m just looking for something that will
work with external git only, even if git2r is installed. And I think I’ve
found all the pass throughs, but would like a solution that’s easier for
you to maintain while keeping functionality. Could be a getOption + on.exit
On Thu, Feb 10, 2022 at 2:28 AM Gábor Csárdi ***@***.***> wrote:
Yeah, I agree that this is not ideal. OTOH if you are always using xgit or
git2r, then you just need to set the option, no? So I am kind of OK with
the dependencies not following the argument.
Here is another idea, though. At the beginning of the functions that take
the git argument, we set the option, and it will be valid until on.exit().
This solves our issues I believe. But it does seem like a bit of an
over-engineered solution.
—
Reply to this email directly, view it on GitHub
<#679 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIGPLWQCF5FT33665T57MDU2NSIVANCNFSM5J7X2NWQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
Best,
John
|
|
Just seeing where this stands as we're currently relying on my branch for production |
|
So should we implement this change or implement a |
|
May I ask will this be merged? Because I'm having the same problem here: But when installing the dependency pkgB, |
|
@fenguoerbian You can probably use |
Thanks for the advice. Finally I asked the admin to install necessary dependencies and re-install |
3 participants
This passes the
gitargument all the way through on theinstall_remotesso that if you have any remote dependencies with a prefix ofgit::and you setgit = "external"on aninstall*(my use case of using SSH keys and no GITHUB_PAT).Here's a reprex that shows the use case. I can turn into an issue if that is better for tracking.
Created on 2021-12-14 by the reprex package (v2.0.1)
Session info