On Mon, 2010-04-26 at 08:46 +0200, Denys Vlasenko wrote:
> On Monday 26 April 2010 06:59, Christopher Barry wrote:
> > > > DESKTOP option adds extra functionality to grep?? Wow. Is that really on
> > > > purpose? I'm not building BB for a desktop scenario, so I definitely
> > > > switched that off - in fact I never even looked in there, as I just
> > > > assumed, as it's name certainly implies, that it was a bunch of extra,
> > > > unneeded, desktopy fluff. 
> > > 
> > > What is needed and what is not needed will vary depending on user.
> > > 
> > > > Curious, why are additional grep parameters hidden in a completely
> > > > unrelated option group? Seems like 'Finding Utilities' was already
> > > > available (and indeed has the other grep options), and would be the
> > > > logical location for that kind of thing. Is having grep isolate on word
> > > > boundaries, a core functionality of grep I would argue, related to a
> > > > DESKTOP in some way that I'm simply not grokking?
> > > 
> > > Easy: just send a patch which make it possible to select this option
> > > with dedicated CONFIG_FEATURE_GREP_FOO.
> > > 
> > Patch attached.
> 
> I meant: create new option, say, CONFIG_FEATURE_GREP_W,
> and make -w support conditional on that option,
> not on CONFIG_DESKTOP.
> 
> You made -w support unconditional.
> 
> I think this approach is ok too in this case,
> since -w code is actually small.
> 
> You broke indentation - replacement lines are indented
> with spaces.
> 
> You forgot to fix help text.
> 
> The patch does not apply with patch -p1, I had to tweak patch header.
> 
> Please next time be more careful.
> 
> Applied to git, thanks.

Sorry about all that. I saw that the change had no effect on BB size, so
I skipped the feature route.

I'll read the directions about contributing, before I go off and do that
wrong again. :)

Thanks,
-C

_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox

Reply via email to