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