Branko Čibej wrote on Tue, Jan 27, 2015 at 11:41:27 +0100:
> On 27.01.2015 11:14, Daniel Shahaf wrote:
> > + /* Are there exactly two double quotes, at the first and last positions?
> > */
> > + if (str[0] == '"' && (second_double_quote = strchr(str+1, '"'))
> > + && second_double_quote[1] == '\0')
>
> Ouch. I don't like assignments in conditions; this bit can be rewritten
> to be more readable. It's not as if we have to write the fastest
> possible code here.
>
I wasn't optimizing for speed; I thought this was the most
straightforward way to implement the condition described in the
associated comment. But this will need to change anyway when \ parsing
is added.
> > + /* Are there any backslashes? */
> > + if (!strchr(str, '\\'))
> > + /* Return whatever is between the quotes. */
> > + return apr_pstrndup(result_pool, str+1, second_double_quote -
> > (str+1));
>
> It should be fairly easy to strip the escapes from the string.
>
Yes, see in my reply to Stefan.
> > @@ -527,6 +599,10 @@ linux_release_name(apr_pool_t *pool)
> > Covers, for example, Debian, Ubuntu and SuSE. */
> > const char *release_name = lsb_release(pool);
> >
> > + /* Try the systemd way (covers Arch). */
> > + if (!release_name)
> > + release_name = systemd_release(pool);
> > +
> > /* Try RHEL/Fedora/CentOS */
> > if (!release_name)
> > release_name = redhat_release(pool);
>
> I'd try the redhat_release before systemd_release; for example, googling
> lists reported bugs on CentOS, where /etc/os-release says "RHEL" but
> /etc/redhat-release says "CentOS", so the latter is more likely to be
> correct.
>
I would call this a vendor bug. Debian has a similar issue, where
/etc/os-release isn't as specific as lsb_release:
% head -n1 /etc/os-release
PRETTY_NAME="Debian GNU/Linux 7 (wheezy)"
% lsb_release -d
Description: Debian GNU/Linux 7.8 (wheezy)
Having said that, I don't have a strong preference about the order.
Whatever people decide is fine by me.
Thanks,
Daniel
> -- Brane
