On Wed, Nov 30, 2016 at 3:38 AM, konsolebox <konsole...@gmail.com> wrote:
> There are some things I noticed in the tmpfiles_process() function:
>
> - `type` currently also checks for functions, alias, and builtins,
> besides executable files. If that's not intended, the `-P` option
> should be added.
I cannot conceive of any way a function or alias would be defined with
these names unless an ebuild author did it intentionally or a user did
it via /etc/portage/bashrc. In either case, I see no need to prevent
that.
> - `[[ ${ROOT} == / ]] || return 0` seems to present a harmless false
> condition, and it doesn't show an error message. I would be helpful
> to have a comment added above it to give details why.
We only want to process tmpfiles for the currently running system.
If ROOT is not /, it indicates we are installing the package for use
on a different system or in a container. In either case, the tmpfiles
would be processed upon boot when that system/container is started.
I find this fairly obvious, but if William wants to document it that's fine.
> I also prefer some things this way:
>
> - Indent the contents of the first `if` block for consistency's sake,
> and less confusion.
I disagree; indenting the entire eclass is silly and does not really
improve readability. Also, this is a very common pattern found in
other eclasses.
> - Patterns in the `case` block doesn't have to be indented. This makes
> the contents of the `case` block aligned with the contents of the
> other blocks (`if`, `while`, etc.), and it makes the use of indents at
> minimum when the block is used recursively.
Sorry, I have no idea what you're trying to say here. Recursive blocks?
This really feels like you're making a personal style suggestion here,
and I personally see nothing wrong with it as-is.
