search

kubernetes-sigs / krew

📦 Find and install kubectl plugins
https://krew.sigs.k8s.io
Apache License 2.0
6.42k stars 369 forks source link

Prevent windows msys shell from interpreting @{upstream} arg #739

Closed mattn closed 2 years ago

mattn commented 3 years ago

Fixes #737

linux-foundation-easycla[bot] commented 3 years ago

CLA Signed

The committers are authorized under a signed CLA.

mattn commented 3 years ago

/assign @corneliusweig

ahmetb commented 3 years ago

Thanks for the patch. I'd love to understand this first.

To me this is very awkward because you'd think something like this would be necessary only if we were executing the git process in a /bin/bash -c "..." or cmd.exe /c "...".

but we're doing this execution through the Go os/exec package which uses CreateProcess API in Win32, that's why I am not fully getting why such escaping is necessary.

Does this syscall somehow have an arguments interpretation/evaluation step that we're not aware of? It would be good to learn why's this happening first.

mattn commented 3 years ago

msys2 shell handle @{} in peculiar argument parser.

image

mattn commented 3 years ago

image

mattn commented 3 years ago

FYI, On Windows, arguments are parsed in the process it-self. (not a shell part like UNIX)

ahmetb commented 3 years ago

@mattn I still don't understand the problem and I do need to understand the problem fully so that we do not break other hundreds or thousands of Windows users who have been using Krew on Windows just fine.

So it seems in your gcc example that the args are sent to the process correctly. (I don't know why you're showing msys echo.exe here).

What matters here is whether git is handling the @{upstream} arg correctly and based on other Windows users I have asked, the answer for them seems to be yes, that argument works just fine. I want to understand why is your git behaving differently.

mattn commented 3 years ago

If the program is linked with msys2 or cygwin runtime, that escaping is required. AFAIK, git on Windows shound be linked those runtimes.

image

mattn commented 3 years ago

One another candidate for fixing is quote. image

mattn commented 3 years ago

This is a part converting that @{}.

https://github.com/cygwin/cygwin/blob/44a79a6eca3d322020dda0919023d78dda129d4d/winsup/cygwin/dcrt0.cc#L338-L344

mattn commented 3 years ago

How about to use remotes/origin/HEAD instead of @{upstream} ?

mattn commented 3 years ago

I found a spell to fix this issue MSYS=noglob.

ahmetb commented 3 years ago

How about to use remotes/origin/HEAD instead of @{upstream} ?

IIRC this was previously discussed and we wanted to do it this way for a reason, but I might be wrong. See the blame history, maybe that helps.

I found a spell to fix this issue MSYS=noglob.

That's great, although again, I am not sure if we should set this or whether it should be set by the user in their .bashrc or whatever they use. I am not sure what % of Krew users are using MSYS/Cygwin, but this little env-override (especially since it's only for windows, and only does something on cygwin/msys) might be okay...

mattn commented 3 years ago

I think this is less wrong for users for msys2 git.

ahmetb commented 3 years ago

/retitle Prevent windows msys shell from interpreting @{upstream} arg

ahmetb commented 3 years ago

for future reference, I'll drop the issue you opened 5 years ago here ;) https://github.com/msys2/MSYS2-packages/issues/522#issue-144797722

/cc @corneliusweig any concerns?

k8s-ci-robot commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: corneliusweig, mattn

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/krew/blob/master/OWNERS)~~ [corneliusweig] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
mattn commented 3 years ago

Sorry, I was confused and commented wrong mistaken with another PR.

k8s-triage-robot commented 2 years ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot commented 2 years ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot commented 2 years ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot commented 2 years ago

@k8s-triage-robot: Closed this PR.

In response to [this](https://github.com/kubernetes-sigs/krew/pull/739#issuecomment-1114026367): >The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. > >This bot triages issues and PRs according to the following rules: >- After 90d of inactivity, `lifecycle/stale` is applied >- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied >- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed > >You can: >- Reopen this issue or PR with `/reopen` >- Mark this issue or PR as fresh with `/remove-lifecycle rotten` >- Offer to help out with [Issue Triage][1] > >Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). > >/close > >[1]: https://www.kubernetes.dev/docs/guide/issue-triage/ Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.