Re: [Quilt-dev] Non-portable sed command in find_patch

[Andreas Gruenbacher]

On Thursday 22 September 2005 11:28, Jean Delvare wrote:
> Hi Andreas,
>
> > > There is a non-portable sed command used in the find_patch function:
> > >
> > > sed -e "/^$bre\(\|\.patch\|\.diff\?\)\(\|\.gz\|\.bz2\)\([
> > > "$'\t'"]\|$\)/!d"
> >
> > Not sure what breaks. Does this sed support the construct sed -ne
> > 's/aaa/bbb/p'? If so, then I'd prefer that.
>
> For one thing, the '!d' seems to cause some trouble because the shell
> erroneously attempts to find an event by that name. That might not
> affect non-interactive shells though, just me when testing, and working
> around it is easy anyway.

It only affects interactive shells, yes.

> The real blocker is that this version of sed doesn't support
> \(foo\|bar\) constructs, just like the version of grep found on this
> system doesn't. This means that John's suggested replacement doesn't
> work either. I installed GNU grep to workaround the grep problem, which
> is why my proposed grep replacement works for me.
>
> 1* Go with the grep replacement I suggested, and its slight performance
> drop.

We have multipe instances of sed grouping, which would all need changing:

$ grep -w 'sed.*\\(' */*.in test/*
quilt/fork.in:         | sed -e 's:\(\|\.diff\?\|\.patch\)\(\|\.gz\|
\.bz2\)$::')
quilt/fork.in:        | sed -ne 's:.*\(-[0-9]\+\)$:\1:'p)
scripts/patchfns.in:    echo "$1" | sed -e 's:\([][^$/.*\\]\):\\\1:g'
scripts/patchfns.in:    echo "$1" | sed -e 's:\([][?{(|)}^$/.+*\\]\):\\\1:g'
scripts/patchfns.in:            set -- $(sed -e "/^$bre\(\|\.patch\|\.diff\?
\)\(\|\.gz\|\.bz2\)\([ "$'\t'"]\|$\)/!d" \
test/conflicts.test:    $ sed -e "s/^\\([17]\\)\$/\\1-/" one.orig > one.txt
test/conflicts.test:    $ sed -e "s/^\\([1267]\\)\$/\\1-/" one.orig > one.txt
test/conflicts.test:    $ sed -e "s/^\\([123567]\\)\$/\\1-/" one.orig > 
one.txt

> 2* Go with awk, as my version of awk seems to be happy with (foo|bar)
> constructs, and awk regexps seem to be slightly more standard than
> sed's and grep's.

My experience is that awk is a lot slower than sed.

> 3* Let the code as is, and I install GNU sed.

That's what I would recommend to do. Can we add a configure.ac test for this? 
We really want a version of sed that supports grouping.

> For information, there seems to be only two other uses of this sed
> construct in quilt (in quilt/fork.in and test/conflicts.test).

Plus a few more in patchfns.in.

> Two additional notes:
>
> * Now that we have a compat directory, if would be great if the test
> suite could use it, so that the calls to grep, sed etc... in the test
> suite respect the user's choices. If the user had to tell quilt to use
> a specific version of grep or sed, chances are good that the version
> found in the PATH are somehow broken and the tests are likely to fail
> because of this.

The test suite uses the installed version of quilt, not the one in the 
package. This would need fixing, but I don't see how to do so easily.

> * Could someone comment on the relative merits of the patch_in_series and
> find_patch functions? Looks to me like the latter does a better job.
> Shouldn't the former be deprecated?

find_patch translates patch name provided on the command line to patch 
filenames; everything else in quilt deals with the patch filename. We could 
change the find_patch logic as we like without having to care about the rest 
of Quilt. The patch_in_series function only takes patch filenames, and 
therefoe is much simpler.

-- Andreas.