[bug-gnu-libiconv] Re: [Mingw-msys] sed on msys error

[Keith Marshall]

[Copying to address@hidden -- this is a libiconv-1.12 bug]

On Friday 27 June 2008 20:14:57 Greg Chicares wrote:
> On 2008-06-27 18:49Z, SUNIL NEGI wrote:
> > While compiling libiconv on msys environment, I found the
> > following line throws an error:
> >
> > address@hidden ~/dlds/libiconv-1.12
> > $ echo "1.12" | sed -n -e "/^[0-9]/{s/^\([0-9]*\).*/\1/p;q}"
> > sed: -e expression #2, char 32: Extra characters after command
>
> Many MSYS 'sed' problems arise from MSYS's path translation:
>   http://article.gmane.org/gmane.comp.gnu.mingw.msys/2183
> but I think this one is different. This slightly-altered
> testcase works:
>
> $echo "1.12" | sed -n -e "s|^\([0-9]*\).*|\1|p;q"
>
> but this one fails with a message such as you quoted:
>
> $echo "1.12" | sed -n -e "{s|^\([0-9]*\).*|\1|p;q}"
>
> but neither uses any '/' character. So I would guess the
>
> problem is that this 'sed':
> 
> > $ sed --version
> > GNU sed version 3.02
>
> is just too old.

Sorry, Greg, but on this occasion, I think you got it wrong; this 
assessment is not only incorrect, but it tends to distract from a 
proper analysis of the problem.  In fact, the original expression, 
appearing in the sed command above, is invalid, and while some sed 
implementations may forgive it, MSYS sed correctly rejects it.

Note the placement of the right hand brace, in the above expression; 
it is immediately preceded by the `q' action verb, but POSIX is very 
explicit here:--
http://www.opengroup.org/onlinepubs/009695399/utilities/sed.html
| [2addr] {function
| function
| ...
| }
|    Execute a list of sed functions only when the pattern space is 
|    selected. The list of sed functions shall be surrounded by braces
|    and separated by <newline>s, and conform to the following rules.
|    The braces can be preceded or followed by <blank>s. The functions
|    can be preceded by <blank>s, but shall not be followed by
|    <blank>s. The <right-brace> shall be preceded by a <newline> and
|    can be preceded or followed by <blank>s.      

(note the explicit requirement for a *newline*, and *nothing* *else*, 
preceding the right brace).

> I get the same error if I put the command 
> in a file and invoke it with 'sed -f'. You can try writing
> the command a little differently; e.g., this seems okay:
>
> echo "1.12" | sed -n -e "/^[0-9]/s/^\([0-9]*\).*/\1/p"
>
> and I don't see why anyone would write 'q' here anyway.

If you examine the full context, where this appears in libiconv-1.12 
windows/winres-options script, it becomes apparent:--

| sed_extract_major='/^[0-9]/{s/^\([0-9]*\).*/\1/p;q}
| a\
| 0
| q
| '
| ...
|   echo "-DPACKAGE_VERSION_MAJOR="`echo "${version}" |
|     sed -n -e "$sed_extract_major"` 

It is not, (as Michael Doubez suggests), to avoid capturing a second 
version identification, (there is no possibility of that, in this 
context); it *is* to prevent falling through to that `a' action, 
which is there to guard against passing in an empty $version; without 
the `a', this would result in no assignment, where one is required; 
with the `a', an assignment of zero is assured, for this case, but 
without the `q', you get the zero in addition to the actual version 
number, in the normal usage case.

IMHO, this is a classical example of clever misuse of an inappropriate 
tool, leading to a fragile implementation; (and in this case it's so 
fragile, that the breakage is built in from the outset).  Ok, we can 
fix that by correcting the syntax of the invalid sed expression, as 
may be seen in the attached libiconv-1.12.patch-1, but better still, 
IMO, to eliminate this complex sed expression altogether, in favour 
of the much simpler, and therefore more robust, implementation 
provided in my recommended alternative libiconv-1.12.patch-2.

Regards,
Keith.

libiconv-1.12.patch-1
Description: Text Data

libiconv-1.12.patch-2
Description: Text Data