Re: [PATCH v2] misc: Allow selective disabling of debug facility names

[Glenn Washburn]

On Tue, 7 Dec 2021 22:01:14 +0100
Michael Schierl <schierlm@gmx.de> wrote:

> Hello Glenn,
> 
> 
> Am 07.12.2021 um 10:17 schrieb Glenn Washburn:
> 
> >> maybe move the conditional before the assignment of ret?
> >
> > I'm understanding you to be suggesting to move the assignment of ret to
> > after the if statement that follows it.
> 
> Correct.
> 
> > The only point I see is saving
> > an assignment in the case that debug=all. Is there more to it?
> 
> No. It just bugged me to do a definitely never used assignment directly
> before returning. Feel free to ignore it.

I'll think about this. Originally, I didn't think it looked as nice, but
on second look it seems fine.

> > Yep, this is an issue. It was intentional when I had the code only
> > running in the case "all" was present, in which case skipping the
> > condition if its the at the start of $debug is fine because the
> > condition is already enabled. After changing to run also without "all"
> > being present, this becomes an issue.
> 
> If "all" still was present I would probably have complained about
> initializing it to +1 instead of +3 as the first three characters cannot
> be a match if they are "all". :)

Yes, but I didn't want to assume that "all" is the first item in the
list. I decided that just the presence of an "all" item any where in
the list of conditionals enables all conditionals at the beginning of
processing. So debug="-btrfs,all" is the same as debug="all,-btrfs" and
means all of the debug conditionals are turned on except btrfs.

I decided that it doesn't make as much sense to have the position of
"all" matter, and effectively be a macro for all conditionals, because
at that point why append ",all,...", just set debug to "all,..."
erasing the previous contents.

I'm open to suggestions on what the preferred way to handle this might
be. We could say that "all" must appear at the beginning, but that
seems unnecessary and overly restrictive. What's less clear to me is
what situation I might want to put have "all" not at the beginning of
the string. I'm thinking there might be one, which is why I'm leaving
that as an option, but maybe I'm wrong.

> 
> > I was using grub_iswordseparator in the original patch, but decided
> > against it this round because the documentation states that separators
> > are whitespace or comma.
> 
> Fair point. I don't think there are any common cases where this
> backward-incompatible change really matters.

Yeah, and its undocumented that other seperators would work.

Glenn