Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Git detection on Mac OS and Windows #5774

Closed
wants to merge 4 commits into from

Conversation

ahmedyarub
Copy link
Contributor

Checklist

  • I have filed an issue about this change and discussed potential changes with the maintainers.
  • I have received the approval from the maintainers to make this change.
  • This is not a stylistic, refactoring, or cleanup change.

Discussion thread for this change

Issue number: 5773

Description of this change

Adds quoting around the git command's parameters to avoid errors in some terminal shells.

@github-actions github-actions bot added product: CLion CLion plugin product: IntelliJ IntelliJ plugin product: GoLand GoLand plugin awaiting-review Awaiting review from Bazel team on PRs labels Nov 25, 2023
@tpasternak
Copy link
Collaborator

This change makes the quotation mark being passed to git, it doesn't work. Maybe it's a shell issue, can you share what's your default shell?

BTW I'm getting fatal: no such branch: '"'

@ahmedyarub
Copy link
Contributor Author

This change makes the quotation mark being passed to git, it doesn't work. Maybe it's a shell issue, can you share what's your default shell?

BTW I'm getting fatal: no such branch: '"'

On Powershell:
git rev-parse "@\{u\}"

On Windows CMD:
git rev-parse "@{u}"

On MacOS' ZSH and Bash:
both git rev-parse "@{u}" and git rev-parse "@{u}"

@tpasternak
Copy link
Collaborator

yeah, but when you run a command from zsh, the quotation marks are not passed to git, they are used to escape symbols inside

@tpasternak
Copy link
Collaborator

please open jshell and try

Runtime.getRuntime().exec(new String[]{"git", "rev-parse", "\"@{u}\""}).getErrorStream().transferTo(System.out)

@ahmedyarub
Copy link
Contributor Author

both in CMD and Powershell I get:

jshell> Runtime.getRuntime().exec(new String[]{"git", "rev-parse", ""@{u}""}).getErrorStream().transferTo(System.out)
fatal: no upstream configured for branch 'ay/fix_test_ui_mapping'
$1 ==> 66
Which should be OK

On ZSH and Bash on MacOS I get:

fatal: no such branch: '"'

@tpasternak
Copy link
Collaborator

This is what I get on Windows for multiple variants.

jshell> Runtime.getRuntime().exec(new String[]{"git", "rev-parse", "@{u}"}).getInputStream().transferTo(System.out)
0ef2befcdd2ca34819cc2140e07f8f3b2ea5f504
$19 ==> 41

jshell> Runtime.getRuntime().exec(new String[]{"git", "rev-parse", "\"@{u}\""}).getInputStream().transferTo(System.out)
0ef2befcdd2ca34819cc2140e07f8f3b2ea5f504
$20 ==> 41

jshell> Runtime.getRuntime().exec(new String[]{"git", "rev-parse", ""@{u}""}).getInputStream().transferTo(System.out)
|  Error:
|  <identifier> expected
|  Runtime.getRuntime().exec(new String[]{"git", "rev-parse", ""@{u}""}).getInputStream().transferTo(System.out)

I think your problem is unrelated to quotation mark escaping, but rather to the fact that you just don't have an upstream branch set in git and we don't handle it well

@tpasternak
Copy link
Collaborator

Just to clarify - IMO there's no need to add extra exclamation marks, as the current ones work on all three systems. On the other hand we need some nice handling of "git rev-parse @{u}" command if there's no upstream branch set

@ahmedyarub
Copy link
Contributor Author

This is what I get on Windows for multiple variants.

jshell> Runtime.getRuntime().exec(new String[]{"git", "rev-parse", "@{u}"}).getInputStream().transferTo(System.out)
0ef2befcdd2ca34819cc2140e07f8f3b2ea5f504
$19 ==> 41

jshell> Runtime.getRuntime().exec(new String[]{"git", "rev-parse", "\"@{u}\""}).getInputStream().transferTo(System.out)
0ef2befcdd2ca34819cc2140e07f8f3b2ea5f504
$20 ==> 41

jshell> Runtime.getRuntime().exec(new String[]{"git", "rev-parse", ""@{u}""}).getInputStream().transferTo(System.out)
|  Error:
|  <identifier> expected
|  Runtime.getRuntime().exec(new String[]{"git", "rev-parse", ""@{u}""}).getInputStream().transferTo(System.out)

I think your problem is unrelated to quotation mark escaping, but rather to the fact that you just don't have an upstream branch set in git and we don't handle it well

This is what I get for vcpkg on Powershell:

PS E:\cpp\vcpkg> jshell
| Welcome to JShell -- Version 17.0.8.1
| For an introduction type: /help intro

jshell> Runtime.getRuntime().exec(new String[]{"git", "rev-parse", ""@{u}""}).getErrorStream().transferTo(System.out)
$1 ==> 0

jshell> Runtime.getRuntime().exec(new String[]{"git", "rev-parse", "@{u}"}).getErrorStream().transferTo(System.out)
fatal: ambiguous argument '@U': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git [...] -- [...]'
$2 ==> 183

@ahmedyarub
Copy link
Contributor Author

Added fallback for Windows and tested successfully.

@tpasternak
Copy link
Collaborator

So I tried it once again, and it seems to work correctly. This would make sense if we called the command via cmd or powershell. We would have to escape {, } characters as they are used to reference variables with special characters. But if we spawn subprocesses directly via JVM's, the arguments should be tokenized correctly.

My impression is that something is broken in your machine, or JVM. If this is not true then we would have to escape much more things, not only this single git rev-parse argument. Could you please double check on any other machine or VM?

@ahmedyarub
Copy link
Contributor Author

So I tried it once again, and it seems to work correctly. This would make sense if we called the command via cmd or powershell. We would have to escape {, } characters as they are used to reference variables with special characters. But if we spawn subprocesses directly via JVM's, the arguments should be tokenized correctly.

My impression is that something is broken in your machine, or JVM. If this is not true then we would have to escape much more things, not only this single git rev-parse argument. Could you please double check on any other machine or VM?

I'll do that today.

@ahmedyarub
Copy link
Contributor Author

You are right, the problem was that I didn't have an upstream configured.

@ahmedyarub ahmedyarub closed this Dec 19, 2023
@ahmedyarub ahmedyarub deleted the ay/fix_git_detection branch December 19, 2023 20:10
@github-actions github-actions bot removed the awaiting-review Awaiting review from Bazel team on PRs label Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product: CLion CLion plugin product: GoLand GoLand plugin product: IntelliJ IntelliJ plugin
Projects
Development

Successfully merging this pull request may close these issues.

4 participants