[arch-projects] [devtools] [PATCH 1/2] makechrootpkg: Fix unconditionally running namcap

[Eli Schwartz]

On 09/01/2017 11:34 PM, Luke Shumaker wrote:
> Are you sure it was there?  Looking at the code (but without actually
> testing it) (and being the one responsible for the bug), I think it
> was actually in 2fd5931a8c67289a8a4acd327b3ce99a5d64c8c7.
> 
> Maybe `git blame` told you the erroneous line was introduced in
> 49088b0--it was actually just re-indented there; it originally
> appeared (un-indented) in 2fd5931.

I think you're right, there. It's all the same basic cause though,
turning everything into multiply localized state variables passed around
functions that are only ever useful if trying to turn makechrootpkg into
a lib.

> So I admit, Arch doesn't really benefit from 49088b0 (except for
> whatever arguments you want to make about code clarity); and that it's
> a change made for the benefit of Parabola.  But hey, Jan merged it :-)

FWIW I happen to feel that the code clarity dropped after your patchset. :p

> Well, as I explained above, Parabola has another program that calls
> prepare_chroot(), and having to set some global variable (especially a
> lower_case one) is a weird calling convention.
> 
> 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.

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.

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".

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

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.

Also main() functions in bash scripts are stupid. As are functions whose
only purpose is to set a bunch of global (!!!) variables immediately
after the function definition.

-- 
Eli Schwartz

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.archlinux.org/pipermail/arch-projects/attachments/20170903/616dd219/attachment.asc>