sed behaving strangely when -n and the delete command are combined

Denys Vlasenko vda.linux at googlemail.com
Mon Jul 13 23:04:17 UTC 2009


On Saturday 11 July 2009 03:41, Rob Landley wrote:
> > But this breaks another case:
> >
> > # echo -ne "first\nsecond\nthird\nfourth\n" | sed -n '1d;1,3p'
> > second
> > third
> > # echo -ne "first\nsecond\nthird\nfourth\n" | ./busybox sed -n '1d;1,3p'
> > (nothing)
> >
> > See? You waste your time writing up agressive emails
> > but you make mistakes too.
> 
> I'm not attempting to be aggressive.

I thought that since your mail does not contain a patch,
it effectively says that I wrote bad code. Without providing
a better alternative. I tried this approach too.
Does not make me new friends :(

However.

> I'd just like it to _work_, because I tend to break things.  And I like them 
> to be simple, because _when_ I break things I have to wade through the code to 
> figure out how to fix it, and wading through the sed code was never fun.

Me too. I just am not a genius and do not have the ability to
write ideal code.

> In this case, I've been suggesting that your entire patch could be replaced 
> with something like the following small change (against 1.14.1) to address 
> this bug:

I tested your patch and it indeed works. The idea of disabling
range maches *at the beginning* didn't occur to me.
> 
> --- busybox-1.14.1/editors/sed.c	2009-05-13 09:32:07.000000000 -0500
> +++ busybox.new/editors/sed.c	2009-07-10 19:05:54.000000000 -0500
> @@ -893,7 +893,7 @@
>  			|| (!sed_cmd->beg_line && !sed_cmd->end_line
>  				&& !sed_cmd->beg_match && !sed_cmd->end_match)
>  			/* Or did we match the start of a numerical range? */
> -			|| (sed_cmd->beg_line > 0 && (sed_cmd->beg_line == linenum))
> +			|| (sed_cmd->beg_line > 0 && (sed_cmd->beg_line <= linenum))
>  			/* Or does this line match our begin address regex? */
>  			|| (beg_match(sed_cmd, pattern_space))
>  			/* Or did we match last line of input? */
> @@ -906,6 +906,7 @@
>  		/* Is this line the end of the current match? */
>  
>  		if (matched) {
> +			if (sed_cmd->beg_line > 0) sed_cmd->beg_line = -1;
>  			sed_cmd->in_match = !(
>  				/* has the ending line come, or is this a single address command? */
>  				(sed_cmd->end_line ?
> 
> That patch (to 1.14.1) handles both of the cases you listed just fine.  Why 
> you're posting enormous patches to address something this simple is what I'm 
> trying to _understand_.

Because I'm stupid. B]


> I'd run the test suite against it to make sure it didn't cause any other 
> regressions, but for some reason "make test" appears to be a NOP in an 
> allnoconfig+sed 1.14.1:

I do not use "make test". I also do not run testsuite on "small"
configs. If you can find time to improve that, it would be great.

> Going into the testsuite diectory and running "./runtest sed" by hand says 
> that sed wasn't built.  (Even though I can run it by hand out of the busybox 
> binary I built, one directory up.)

Looks like a bug.

> On a related note, the busybox multiplexer also appears to be broken, at least 
> for allnoconfig+sed.  When I just tried it (make distclean, make allnoconfig, 
> make menuconfig, switch on editors->sed, make) it went:
> 
> $echo -e "first\nsecond\nthird\nfourth" | ./busybox sed -n '1d;1,3p'
> sed: unmatched 'e'
> bash: echo: write error: Broken pipe

That's actually a feature. If you have *only one* applet,
multiplexer is disabled. :) It does not matter how you rename
the binary, it will work as that applet.

> I had to create a "sed" symlink and use that to test this functionality.  
> (Switching long options back on didn't fix it.  I encountered that in 1.14.1 
> but just confirmed that this behavior is the same for me in current -git.  
> (Perhaps that's what's confusing the test suite?)

Maybe.

> By the way, the first time I tried to build my above patch to test it (again 
> working against the 1.14.1 release version), when I did a "make -j" (again on 
> the allnoconfig plus editors->sed version), it did this about halfway through:
> 
> libbb/appletlib.c:53:27: error: applet_tables.h: No such file or directory
> libbb/appletlib.c:149:5: warning: "NUM_APPLETS" is not defined
> libbb/appletlib.c:159:5: warning: "NUM_APPLETS" is not defined
> libbb/appletlib.c: In function ‘find_applet_by_name’:
> libbb/appletlib.c:170: error: ‘NUM_APPLETS’ undeclared (first use in this 
> function)
> libbb/appletlib.c:170: error: (Each undeclared identifier is reported only once
> libbb/appletlib.c:170: error: for each function it appears in.)
> libbb/appletlib.c: In function ‘busybox_main’:
> libbb/appletlib.c:644: error: ‘MAX_APPLET_NAME_LEN’ undeclared (first use in 
> this function)
> make[1]: *** [libbb/appletlib.o] Error 1
> make[1]: *** Waiting for unfinished jobs....
> 
> That might have been fixed in the current version, I haven't really tried to 
> reproduce it since.  It seems to be an asynchronous weirdness with make -j, 
> and I stopped feeding -j to the busybox build.
> 
> (When I say "I break things", I mean that trying to test the fix for one bug 
> and encountering three other unrelated bugs along the way is a normal 
> afternoon for me.  It always has been.  I don't know why.  It's kind of 
> annoying, really.)

Yeah, build systems tend to keep working correctly only in usage cases
which are frequently executed by developers. I use "make -j" very rarely.


I am committing your patch. Thanks!

sed: simpler fix for recent GNU compat stuff (by Rob Landley)

function                                             old     new   delta
process_files                                       2120    2102     -18

Signed-off-by: Rob Landley <rob at landley.net>
Signed-off-by: Denys Vlasenko <vda.linux at googlemail.com>

--
vda


More information about the busybox mailing list