[PATCH] update autogen-waf.sh

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] update autogen-waf.sh

Samba - samba-technical mailing list

OK, here is the first one, this is basically just running the script
through 'shellcheck' and fixing any errors found, it should not change
how the script works.

Rowland

autogen-waf.sh-Remove-all-errors-reported-by-shellch.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] update autogen-waf.sh

Samba - samba-technical mailing list
Hi Rowland,

On Thu, 10 Aug 2017 14:21:34 +0100, Rowland Penny via
samba-technical <[hidden email]> wrote:

> OK, here is the first one, this is basically just running the script
> through 'shellcheck' and fixing any errors found, it should not change
> how the script works.

It all looks good.  Can I suggest a minor improvement in a few places?

Instead of doing:

  "$p"/Makefile

it is probably better to do:

  "$p/Makefile"

That way the quotes are around the whole string and future edits are
less likely to be fragile.

However, you need to be careful with examples like this:

  "$p"/include/config*.h*

If you double-quote the wildcards then they don't get expanded.  There
are a few answers to this, including:

  "$p/include/config"*.h*

Deciding where to end the quotes is quite subjective.  :-)

Personally, I tend to "over-punctuate" my shell scripts.  I would write
the first example as:

  "${p}/Makefile"

While the braces aren't necessary in this case, I believe that they
make the variable expansion more obvious... especially in cases where
there are multiple expansions in a string.  The only place I don't use
the braces is when the variable expansion is the whole string, as in
"$p".

peace & happiness,
martin

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] update autogen-waf.sh

Samba - samba-technical mailing list
On Fri, 11 Aug 2017 18:53:16 +1000
Martin Schwenke <[hidden email]> wrote:

> Hi Rowland,
>
> On Thu, 10 Aug 2017 14:21:34 +0100, Rowland Penny via
> samba-technical <[hidden email]> wrote:
>
> > OK, here is the first one, this is basically just running the script
> > through 'shellcheck' and fixing any errors found, it should not
> > change how the script works.
>
> It all looks good.  Can I suggest a minor improvement in a few places?
>
> Instead of doing:
>
>   "$p"/Makefile
>
> it is probably better to do:
>
>   "$p/Makefile"
>
> That way the quotes are around the whole string and future edits are
> less likely to be fragile.
>
> However, you need to be careful with examples like this:
>
>   "$p"/include/config*.h*
>
> If you double-quote the wildcards then they don't get expanded.  There
> are a few answers to this, including:
>
>   "$p/include/config"*.h*
>
> Deciding where to end the quotes is quite subjective.  :-)
>
> Personally, I tend to "over-punctuate" my shell scripts.  I would
> write the first example as:
>
>   "${p}/Makefile"

I understand all this Martin and as I said earlier, where do you stop ?
>
> While the braces aren't necessary in this case, I believe that they
> make the variable expansion more obvious... especially in cases where
> there are multiple expansions in a string.  The only place I don't use
> the braces is when the variable expansion is the whole string, as in
> "$p".

I normally use them in every case, just in case ;-) but in this case,
I am just following 'shellcheck' and fixing obvious errors.

I think I will start again, follow 'shellcheck' but add what I would
normally do and also fix any errors. I will also take on board what you
say about the ctdb tests and ignore them.

Thanks

Rowland



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] update autogen-waf.sh

Samba - samba-technical mailing list
On Fri, 11 Aug 2017 11:21:00 +0100, Rowland Penny via samba-technical
<[hidden email]> wrote:

> On Fri, 11 Aug 2017 18:53:16 +1000
> Martin Schwenke <[hidden email]> wrote:
>
> > Hi Rowland,
> >
> > On Thu, 10 Aug 2017 14:21:34 +0100, Rowland Penny via
> > samba-technical <[hidden email]> wrote:
> >  
> > > OK, here is the first one, this is basically just running the script
> > > through 'shellcheck' and fixing any errors found, it should not
> > > change how the script works.  
> >
> > It all looks good.  Can I suggest a minor improvement in a few places?
> >
> > Instead of doing:
> >
> >   "$p"/Makefile
> >
> > it is probably better to do:
> >
> >   "$p/Makefile"
> >
> > That way the quotes are around the whole string and future edits are
> > less likely to be fragile.
> >
> > However, you need to be careful with examples like this:
> >
> >   "$p"/include/config*.h*
> >
> > If you double-quote the wildcards then they don't get expanded.  There
> > are a few answers to this, including:
> >
> >   "$p/include/config"*.h*
> >
> > Deciding where to end the quotes is quite subjective.  :-)
> >
> > Personally, I tend to "over-punctuate" my shell scripts.  I would
> > write the first example as:
> >
> >   "${p}/Makefile"  
>
> I understand all this Martin and as I said earlier, where do you stop ?

Cool.  I'm just over-communicating given the usual time turnaround
for long distance email conversations...  :-)

> > While the braces aren't necessary in this case, I believe that they
> > make the variable expansion more obvious... especially in cases where
> > there are multiple expansions in a string.  The only place I don't use
> > the braces is when the variable expansion is the whole string, as in
> > "$p".  
>
> I normally use them in every case, just in case ;-) but in this case,
> I am just following 'shellcheck' and fixing obvious errors.

Yeah, shellcheck often provides hints that the code could be better.

> I think I will start again, follow 'shellcheck' but add what I would
> normally do and also fix any errors. I will also take on board what you
> say about the ctdb tests and ignore them.

I think that's a good move.

That said, if you really do want to attack the CTDB tests then I think
the order could be:

1. Fix shellcheck hits in unit tests
2. Fix shellcheck hits in the (bash) integration tests
3. Convert bash to POSIX sh

However, it is a major undertaking...

TV time.  :-)

peace & happiness,
martin

Loading...