On Fri, 01 Sep 2017 18:53:13 -0400, Eli Schwartz wrote:

Don't use error-prone logic e.g. foo=true; if $foo ...

Hey, I'm not to blame for this one :-)

This invokes an extra process as opposed to a simple value comparison,

That's not quite true: $ type true true is a shell builtin $ type false false is a shell builtin Bash won't actually make any syscalls when executing these commands in a conditional. Apparently the code paths within Bash are slightly faster for `[[ $v = true ]]` than for `$v`, but that's negligible compared to if it actually had to invoke an extra process as you said. Time for 1 million iterations: "v=true; if $v; then ..." | 7.1 sec "v=true; if [[ $v = true ]]; then ..." | 6.5 sec "v=/bin/true; if $v; then ..." | > 5 minutes (I got bored and killed it)

in the process of completely failing to act as expected when the variable is unset because of unrelated bugs.

And that's the real reason that this change should be applied. :-) -- Happy hacking, ~ Luke Shumaker

On Sun, 03 Sep 2017 03:12:19 -0400, Eli Schwartz wrote:

...

But also, I don't think that it's a bad idea to slowly get rid of global variables in programs (of course in Bash it's not quite a good distinction because it's dynamically scoped instead of lexically scoped, and even if it's a local variable in a function, in anything that you call it might as well be global). And this is a step toward that. Think of all the globals as just local variables to main()--other functions (except for init_variables(), which is really just "main() part 1") shouldn't be touching them.

"it's not quite a good distinction" indeed.

And init_variables() is most emphatically *not* main() part 1, whether it is meant to be or not in practice it doesn't even attempt to be.

From the commit that put the code in init_variables() and main() into functions instead of floating around in the top-level: It [may] make sense to move init_variables() down into the main() function, instead of having it as a separate function up top (if this done, then the `-g` flag passed to `declare` in init_variables() can be dropped). However, in interest of keeping the `diff -w` small, and merges/rebases simpler, this isn't done here. If you cut/paste the code from init_variables() into the top of main(), it works fine. The sole reason that it isn't part of main() is that if Jan didn't apply that patch, then it meant that all of my work on commits after that would be harder to merge/rebase to new versions of devtools.

Global variables aren't automatically a sin. Imperative programming isn't automatically a sin either. When used properly it can be both very nice-looking and descriptive of what is going on.

Oh, definitely. But many of the functions that were already there before I made any changes weren't really functions, they were just labels serving as comments. That is understandable when you look at the history and see them as incrementally improving the legibility of the codebase from relative mess that it was circa early 2013. I was just taking it a step farther; I made it so that if a function was touching global state, then it was explicit.

Figuring out where the state is at any given time can only ever be harder when scoping it (complexity is hard...), but usually that is a negligible cost offset by the reusability of functions that are going to be reused a lot. Which brings us back to "this is only useful as a lib".

Splitting things in to functions (real functions, that stand alone from the calling code) increases readability because when reading that function, you see the signature, and know the inputs, the outputs, and you don't have to know everything that's going on in the entire file, you can read that one function and understand the function. When it starts dealing with global state, well then you start to lose that, as you have to start worrying about the entire codebase again.

Also the existence of this bug testifies that you weren't able to follow what it was doing. :p

Oh, I followed what it was doing. The code in libremakepkg calls prepare_chroot() correctly :P It was just a simple mistake of missing an argument when I called it. As I said, I hadn't generally been looking at main() as closely as I do the other functions (as on Parabola it never gets called), so it slipped through the cracks.

And if it were so advantageous to stick everything inside a main() function in bash scripts, maybe the rest of devtools should be converted as well. Thing is, that doesn't make a whole lot of sense in either makechrootpkg or any other tool if you aren't actually doing much other than sticking the entire contents of a script into a single function and then calling the function.

I kind of feel that makechrootpkg had struck a pretty good balance already.

I mean, I guess an argument could be made that on a strictly "make this more reusable for parabola" level prepare_chroot() should be able to scope run_namcap, which I guess was neatly accomplished (and broken) in the commit you mentioned... but tacking on a bunch of questionable changes and claiming that it was done for code readability purposes is IMHO fallacious.

Who claimed it was done for readability? The commit message said makechrootpkg: Have functions be more function-y. Rather than them simply being named blocks of code with braces around them. That is: have them take things via arguments rather than global variables. I made it known that I was backporting changes made in Parabola, but I never gave any justification for this change more than what is said in the commit message. There's an argument to be made that making them more function-y improves readability (it's explicit what affects that bit of code), though I didn't make that argument. Of course, there was the original motivating argument that them being more function-y makes them easier to call from places other than (what I indented to become) main(). Though I did not discuss that motivation when contributing it upstream.

Also main() functions in bash scripts are stupid.

Eh, it's similar to the idiom of wrapping everything in { ... }, except that it isn't confusing to people who haven't seen that idiom before. For reference, this serves the purpose of making it so that Bash doesn't begin executing anything until the entire script has been parsed. This is important for scripts that aren't real files (maybe piped from curl, a terrible idea, but a thing people do), but regular programs benefit because if the file is edited (via an upgrade) while the program is running, then it probably hasn't had the entire thing read yet, and that can lead to real weirdness.

As are functions whose only purpose is to set a bunch of global (!!!) variables immediately after the function definition.

Eh, I'd have moved it to be at the top of main(), but I was trying to avoid moving code around in the file. -- Happy hacking, ~ Luke Shumaker