Revert "bash_it: source reloader.bash without arguments for the default enabling"

[gaelicWizard]

Description

This reverts commit e05fa47.
This reverts commit ee85367.

Motivation and Context

This does not resolve the issue it claims to, as evidenced by the fact that a third commit was required in a different file to fix it. Additionally, this idiom is poor coding practice, as evidenced by the need to disable shellcheck from issuing a warning about it.

The correct working fix for ble.sh was added in plugin/blesh in commit 41cf3cf.

How Has This Been Tested?

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code follows the code style of this project.
• If my change requires a change to the documentation, I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
• I have added tests to cover my changes, and all the new and existing tests pass.

[akinomyoga]

Can I comment on this PR?

This does not resolve the issue it claims to, as evidenced by the fact that a third commit was required in a different file to fix it.

This is false. I have explained it in #2096. If the fixes e05fa47 and ee85367 are applied, actually the fix 41cf3cf is not required. I have added the third fix in the different file just to make it safer against potential breakage in the future (or against the case bash_it.sh is sourced with arguments or sourced in the context with arguments). I think we don't have to care about source bash_it.sh with arguments as it is currently supposed to be called without arguments. Also, the default .bashrc contains source bash_it.sh in the top-level, so I think the cases that source bash_it.sh is called in the context with arguments are rare.

Also, as I have explained in #2096, this is not a specific issue to plugin/blesh. Before applying PR #2096, every source in the plugins, aliases, and completions without arguments started receiving the arguments '' '', which may cause problems when any of the sourced scripts interpret the arguments. For the details, I would appreciate it if you could read #2096. Maybe currently there are no other configurations that interpret the arguments, but it will introduce a certain restriction to the scripts sourced in bash-it, i.e., the script should not interpret the arguments or the script should accept an explicitly specified argument that doesn't change the default behavior. In the case of ble.sh, ble.sh accidentally had the option --attach=prompt which doesn't change the default behavior, but, in principle, there can be a script that does not have an option that doesn't change the default behavior in principle. Anyway, we need to care about more things with this proposed change.

It doesn't seem to me that there are any positive reasons to revert these changes.

SC2014 reported for the line is clearly a false-positive case. SC2140 is not the one that recommends not using the style of the form ${v:+x y z} but is talking about a single argument of the form "A"B"C". On the other hand, ${_bash_it_main_file_type:+"skip" "$_bash_it_main_file_type"} is not meant to be treated as a single argument, but is expected to be expanded to two arguments. Actually, if we would like to just turn off the false-positive error reported by shellcheck, we could write it as ${_bash_it_main_file_type:+skip "$_bash_it_main_file_type"}, for which shellcheck will not produce errors or warnings.

I actually have another suspicion on the original change as I have written in #2096, but maybe I misunderstand something.

Additionally, maybe we should separate the call of reloader.bash for the current enabling mechanism and the calls of reloader.bash for the legacy enabling as we have been doing originally before 23f9b74. I think the reason that they were originally separated is that these codes are considered deprecated. Mixing the up-to-date codes and the deprecated codes doesn't seem to be an improvement.

What do you think of this?

---

Come to think of it again, maybe we can just call set -- in reloader.bash.

[gaelicWizard]

@akinomyoga, thank you for your thorough comment!

My objection is to making a behavior change that affects all components loaded when one specific plugin is having an issue.

The specific behavior in question, positional parameters being inherited by nested source, is in my opinion another oddity of Bash that should not be relied on. If anything we source needs arguments to be expressly provided, then we should expressly provide those arguments in all cases. Thus, we should add the appropriate argument as you did in 41cf3cf. If another plugin needs arguments, then we should expressly provide them in the appropriate place as well.

The syntax that SC2140 warns about is not wrong, just odd. It's "non-idiomatic" we might say. I'm against confusing syntax, even if it is 100% correct and predictable and reliable and documented. Bash-It is used by a lot of people and I think it's valuable for the code to be reasonably readable. I've spent the past six months fixing oversights with quoting and string expansion, so syntax that makes it harder to read how a parameter is expanded seems suspect to me. The shellcheck warning is meant to remind us that the quoting is "weird", not that it's wrong.

---

All that said, I would actually like to remove the "legacy" file loading. bash-it update expressly runs _bash-it-migrate after every update, so it's unlikely that there's anyone with existing "legacy" directories anymore. This could substantially simplify reloader...not to mention many functions in lib/helpers and lib/search, which I think we all would be very happy with.

[akinomyoga]

Hi, thank you for your reply!

My objection is to making a behavior change that affects all components loaded when one specific plugin is having an issue.

The situation is actually the opposite. Originally all sources without arguments did not receive arguments. Then 23f9b74 (more specifically 0d346b2) has introduced a behavior change that affects all components, where all sources without arguments started to inherit the arguments '' '' (ten days before). In PR #2096 (e05fa47), the behavior change was reverted to the original behavior (five days before).

The specific behavior in question, positional parameters being inherited by nested source, is in my opinion another oddity of Bash that should not be relied on.

Yes. I agree that we should not rely on this behavior so would like to somehow turn it off. The original change 0d346b2 has unexpectedly turned it on in some sense, so I wanted to revert it in #2096. This PR #2097 actually tries to again effectively turn the inheriting behavior on.

If anything we source needs arguments to be expressly provided, then we should expressly provide those arguments in all cases. Thus, we should add the appropriate argument as you did in 41cf3cf. If another plugin needs arguments, then we should expressly provide them in the appropriate place as well.

The situation is actually the opposite as well. The above argument is talking about the source needing arguments, but PR #2096 is a fix for the sources that needs to be called without arguments. ble.sh is originally intended to be sourced without arguments, but unexpected arguments have started to be specified, so I have added a redundant "no-op" argument in blesh.plugin.bash as a workaround. The problem is that not all the scripts that interpret arguments would support any "no-op" arguments.

The syntax that SC2140 warns about is not wrong, just odd. It's "non-idiomatic" we might say. I'm against confusing syntax, even if it is 100% correct and predictable and reliable and documented. Bash-It is used by a lot of people and I think it's valuable for the code to be reasonably readable. I've spent the past six months fixing oversights with quoting and string expansion, so syntax that makes it harder to read how a parameter is expanded seems suspect to me. The shellcheck warning is meant to remind us that the quoting is "weird", not that it's wrong.

Fair enough.

---

All that said, I would actually like to remove the "legacy" file loading. bash-it update expressly runs _bash-it-migrate after every update, so it's unlikely that there's anyone with existing "legacy" directories anymore. This could substantially simplify reloader...not to mention many functions in lib/helpers and lib/search, which I think we all would be very happy with.

Yeah, I think now we can safely remove these legacy loading after five years.

[NoahGorny]

I also support getting rid of the legacy loading, after 5 long years..

[seefood]

seems ok and passes the tests, so why th hell not...

[akinomyoga]

@seefood Did you merge this understanding the context? IMHO, the content of this PR is the one that has been rejected, yet another completely different direction (of removing the legacy loading) was suggested.

[seefood]

@akinomyoga I admit I didn't dive deep enough into this. Looking at the PR's fidd it seemed like there was only a shellcheck comment dropped and the other line is so similar I thought it was a fixed typo. just to make sure I echoed both values now to see if they produce different outputs and they are both empty.

Should I worry and revert this?

[akinomyoga]

just to make sure I echoed both values now to see if they produce different outputs and they are both empty.

With _bash_it_main_file_type="",

source "${BASH_IT}/scripts/reloader.bash" ${_bash_it_main_file_type:+"skip" "$_bash_it_main_file_type"}

results in

source "${BASH_IT}/scripts/reloader.bash"

but

source "${BASH_IT}/scripts/reloader.bash" "${_bash_it_main_file_type:+skip}" "$_bash_it_main_file_type"

results in

source "${BASH_IT}/scripts/reloader.bash" "" ""

Those empty arguments "" "" are inherited by the plugin files and will cause a problem with the plugins that parse arguments.

Originally, these extra empty arguments were not specified. They were introduced in #1902 unintentionally and broke a plugin, and #2096 recovered the original behavior. However, @gaelicWizard somehow considered it the other way around as if #2096 introduced the behavioral change.

Should I worry and revert this?

The originally expected solution would have been to remove the legacy loading from reloader.bash so that it doesn't need to receive those special arguments in the first place. If it would be applied soon, this doesn't need to be reverted as it will anyway removed soon. If it would take time, it would be safer to revert this as a tentative solution until the legacy loading is cleaned up.

[seefood]

"soon" may be years to never, since it seems Noah is busy and the rest are incomunicado, which is why Noah gave me rights to clean up the PRs.

I guess I'll revert it to be on the safe side.