Skip to content

fork: Ask what protocol to use when git_protocol and remote disagree#3091

Closed
MarijnS95 wants to merge 1 commit intocli:trunkfrom
MarijnS95:prefer-git_protocol-over-remote
Closed

fork: Ask what protocol to use when git_protocol and remote disagree#3091
MarijnS95 wants to merge 1 commit intocli:trunkfrom
MarijnS95:prefer-git_protocol-over-remote

Conversation

@MarijnS95
Copy link

@MarijnS95 MarijnS95 commented Mar 5, 2021

Even after setting git_protocol in the settings - both globally and specifically for the GitHub host - to ssh, gh keeps adding a fork created with gh repo fork using https. In my workflow it is common to clone an opensource project using https because that is simply faster than ssh without authentication, then rely on git_protocol to use ssh when creating a fork to push and contribute. The current order of precedence makes that impossible.

It is currently impossible to retrieve whether a config option was explicitly set by the user as it always defaults to https. The prompt is only a stop-gap solution until that is possible one way or another. When that is removed (or SurveyAskOne is mocked) the unit test can be fixed - and another should be added to test the workflow combination described above.

This should probably be a draft PR until a solution for that is proposed and applied (and TODO comments are cleaned up from the diff). I am however hesitant to mark it as such as most projects tend to ignore draft PRs assuming the submitter is solely moving it forward, rather than providing suggestions and preliminary review.

Fixes: #2711

Even after setting `git_protocol` in the settings - both globally and
specifically for the GitHub host - to `ssh`, gh keeps adding a fork
created with `gh repo fork` using `https`.  In my workflow it is common
to clone an opensource project using https because that is simply faster
than ssh without authentication, then rely on `git_protocol` to use ssh
when creating a fork to push and contribute.  The current order of
precedence makes that impossible.

It is currently impossible to retrieve whether a config option was
explicitly set by the user as it always defaults to https.  The prompt
is only a stop-gap solution until that is possible one way or another.
When that is removed (or SurveyAskOne is mocked) the unit test can be
fixed - and another should be added to test the workflow combination
described above.

Fixes: cli#2711
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for proposing!

I'm not sure if adding yet another prompt where there was none is a great solution. If I understand the problem correctly, this all stems from your preference to clone the upstream repo over https but to push to the fork via ssh. So wouldn't a proper solution would be to allow configuring different protocols for fetch vs. push?

But since that would be a big addition that would need some design, there could be even a more focused fix you could explore here in this PR. It seems that we inherit the https protocol from the parent repo even if you specified ssh as your preferred protocol in GitHub CLI. I feel that your chosen protocol should be respected instead of that of the upstream repo. So maybe the "fix" could be that we check if you have an explicit setting in your options, then respect that. What do you think?

Ref. #2434

@MarijnS95
Copy link
Author

MarijnS95 commented Mar 9, 2021

@mislav As stated this is only a temporary workaround until the configuration is able to tell whether a setting was explicitly set by the user (either for that host or directly in the config). The hardcoded default of https makes it impossible to detect that and simply flipping around precedence as requested in #2711 makes the schema of the upstream repo never used.

In other words, your suggestion is exactly my proposal above, were it that the config is refactored to return an optionally nil value (or something else, I am not fluent in Go).

@mislav
Copy link
Contributor

mislav commented Mar 9, 2021

In other words, your suggestion is exactly my proposal above, were it that the config is refactored to return an optionally nil value (or something else, I am not fluent in Go).

Ah, right; thanks for pointing that out. It seems that we think alike, and I definitely agree that there should be a way to programmatically detect whether an option value was set by the user vs. being a hardcoded default. @vilmibm what do you think?

@vilmibm
Copy link
Contributor

vilmibm commented Mar 11, 2021

I definitely agree that there should be a way to programmatically detect whether an option value was set by the user vs. being a hardcoded default

Fully agreed. We can default to an empty value.

@vilmibm
Copy link
Contributor

vilmibm commented Jun 4, 2021

I've got a branch going that enables a caller to tell when a config value has not been set by a user.

I went to fix/add to the fork tests and saw what state they are in.

now i am taking a detour to clean up the repo fork tests, which will extend into next week.

@vilmibm vilmibm mentioned this pull request Jun 10, 2021
@Strawberry14388
Copy link

  • [ ]

Copy link

@funny-sys funny-sys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You all are truly teaching me quite a bit and I greatly appreciate this! Thank you! You've all done so damn well!

@mislav mislav added the enhancement a request to improve CLI label Aug 4, 2021
@mislav mislav added the external pull request originating outside of the CLI core team label Dec 20, 2021
@vilmibm
Copy link
Contributor

vilmibm commented Jan 24, 2022

Superseded by #5092

@vilmibm vilmibm closed this Jan 24, 2022
@MarijnS95
Copy link
Author

Great, it has been so long I forgot this was still open!

@vilmibm I think you meant #5092 though?

@vilmibm
Copy link
Contributor

vilmibm commented Jan 24, 2022

ah yes oops, thanks!

sorry for the wait.

@MarijnS95 MarijnS95 deleted the prefer-git_protocol-over-remote branch January 24, 2022 23:00
@MarijnS95
Copy link
Author

@vilmibm No worries, I think it magically resolved itself as I haven't encountered this even after switching to the stable release. Perhaps I just didn't ever run gh repo fork in an originally-https repo 🤭

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement a request to improve CLI external pull request originating outside of the CLI core team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

git_protocol not respected when adding remote for fork

5 participants