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

interp/api: set GID #723

Merged
merged 1 commit into from
Sep 8, 2021
Merged

interp/api: set GID #723

merged 1 commit into from
Sep 8, 2021

Conversation

sylv-io
Copy link
Contributor

@sylv-io sylv-io commented Sep 6, 2021

Set GID environment variable analog to UID.

Rel: go-task/task#561

Signed-off-by: Marcello Sylvester Bauer sylv@sylv.io

@sylv-io
Copy link
Contributor Author

sylv-io commented Sep 6, 2021

Related Issue: #514

@sylv-io
Copy link
Contributor Author

sylv-io commented Sep 6, 2021

cc: @jjzcru

@sylv-io
Copy link
Contributor Author

sylv-io commented Sep 6, 2021

cc @AcLr

Copy link
Owner

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

Thanks! Can you add a very simple test to check that GID isn't empty? Near where we have the UID tests. Something like [[ -z $GID ]] && echo "GID not set" expecting the output to be empty.

@sylv-io
Copy link
Contributor Author

sylv-io commented Sep 8, 2021

Thanks! Can you add a very simple test to check that GID isn't empty? Near where we have the UID tests. Something like [[ -z $GID ]] && echo "GID not set" expecting the output to be empty.

I had to add #IGNORE since bash does not set GID by default. Is this PR still a desirable adaptation or should it be 1 to 1 bash complaint? I personally think it's great addtion.

@mvdan
Copy link
Owner

mvdan commented Sep 8, 2021

Oh, right, Bash sets UID but not GID. I was under the assumption it set both.

On the other hand, Zsh does set it: https://askubuntu.com/questions/1124674/why-is-my-gid-environment-variable-empty

And I think it's probably harmless if we provide extra built-in information. The interpreter tends to default to bash behavior, but it's also not just for interpreting bash - ideally it should also do an OK job at interpreting POSIX shell, mksh, etc. mksh has KSHGID too, for example.

So, sounds good to me :)

@mvdan
Copy link
Owner

mvdan commented Sep 8, 2021

Ah, your last-minute push confused me while merging. Are you done with the changes?

@mvdan
Copy link
Owner

mvdan commented Sep 8, 2021

Also, please squash your commits, as otherwise I'd squash-merge.

@sylv-io
Copy link
Contributor Author

sylv-io commented Sep 8, 2021

Ah, your last-minute push confused me while merging. Are you done with the changes?

sorry, i did not see you answer in time. i will squash and only keep the readonly checks.

Set GID environment variable analog to UID.

Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io>
@mvdan
Copy link
Owner

mvdan commented Sep 8, 2021

I can also squash of course, but best if you write your own unified commit message :)

@sylv-io
Copy link
Contributor Author

sylv-io commented Sep 8, 2021

I can also squash of course, but best if you write your own unified commit message :)

already done :). I removed the stat test, since it isn't a shell buildin. 👍

@mvdan mvdan merged commit d48a421 into mvdan:master Sep 8, 2021
@sylv-io sylv-io deleted the feature/setgid branch September 8, 2021 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants