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

scripts: follow up of #3115 (feedback) #3124

Merged
merged 3 commits into from
Feb 27, 2023
Merged

Conversation

georglauterbach
Copy link
Member

Description

Follow-up of #3115 (branch name of this PR wrong, lol). See
#3115 (comment) for example.

Type of change

  • Improvement (non-breaking change that does improve existing functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md or the documentation under docs/)
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@georglauterbach georglauterbach added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Feb 26, 2023
@georglauterbach georglauterbach added this to the v12.0.0 milestone Feb 26, 2023
@georglauterbach georglauterbach self-assigned this Feb 26, 2023
@georglauterbach georglauterbach changed the title apply PR feedback scripts: follow up of #3115 (feedback) Feb 26, 2023
@@ -1,5 +1,7 @@
#!/bin/bash

shopt -s globstar
Copy link
Member

Choose a reason for hiding this comment

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

Can this introduce unwanted side-effects with our existing scripts that are called afterwards?

Another option would be, to just use it in the _setup function.

Edit: After a quick grep, we don't seem to use '**/' anywhere else.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was wondering about this too. But I left it up to the tests. Seems there is one place we need to adjust. I will do this later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind, was a sporadic BATS issue.

This comment was marked as resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided against this for two reasons:

  1. The effects of shopt are not scoped to a function, i.e.
$ cat test.sh
#!/bin/bash

shopt -u globstar
shopt | grep "globstar"

function a() {
  shopt -s globstar
  shopt | grep "globstar"
}

a
shopt | grep "globstar"

$ bash test.sh
globstar     off
globstar     on
globstar     on

Hence, setting this inside the whole setup, i.e. changing the behavior at a certain point, is undesirable. We could turn it off again, but actually setting it as fine I think; we may be able to use it in the future and it does not hurt at all.
2. I want Bash adjustments like shopt or set to be used at the very beginning of scripts so everyone immediately notices them. I plan on introducing set -u and shopt -s inherit_errexit too, and these should go together at the beginning start-mailserver.sh.

Copy link
Member

Choose a reason for hiding this comment

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

I am aware of that. I didn't express it clearly enough. I meant something like this:

function foo() {
shopt -s globstar
for i in /foo/**/*; do
bar
done
shopt -u globstar
}

Setting globstar in the function and revert it after usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually think that this feature is useful in general, and having it available everywhere is a nice thing :)

@@ -1,5 +1,7 @@
#!/bin/bash

shopt -s globstar

This comment was marked as resolved.

@georglauterbach georglauterbach merged commit 9ead9a5 into master Feb 27, 2023
@georglauterbach georglauterbach deleted the scripts/follow-up-3112 branch February 27, 2023 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/scripts kind/improvement Improve an existing feature, configuration file or the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants