[Toybox] [groups] : ! More than one in group is error

[Rob Landley]

On 12/27/2012 10:32:51 PM, Ashwini Sharma wrote:
> Hi Rob,
> 
>   With your fix, it doesn't segfault now.
> But does it matter, to give the proper option name in error message.  
> If
> yes, then the
> fix fails in that.
> e.g. when running *./toybox touch -d 12 -r f2 f1* it throws the error
> message as
> *touch: No 'r' with 't'*.
> 
> I feel the error message is not explanatory here. it should give the  
> proper
> option names in here.
> my patch had this fix.

I hadn't noticed, because you didn't attach a patch. You gave a text  
description a change, which makes it harder for me to see what changed.

Your description quoted some source, but it was badly whitespace  
damaged and had asterisks around things I thought were out of place  
pointer dereferencing (I see now it was an attempt to highlight the  
changed bits, but I didn't get that at the time). The email separated  
the two versions so much I couldn't keep them on the screen at the same  
time in my mail reader window, which made comparing what changed  
difficult. The first time I read it, the first thing I noticed was you  
added a null pointer check to mask an error that should never happen  
(if we fall off the end of the list, we set up the data structure  
wrong), at which point I stopped reading, especially since your problem  
description did not include a test case I could reproduce.

Long story short: Patches are good. Even if I don't wind up using them  
as-is, they communicate code changes clearly.

Now you've given me a test I can reproduce, which is also good. I agree  
that's the wrong error message, so let me take another look at what  
your fix did...

Ok, first thing you did was switch back off the bits that this option  
enabled, including any [+abc] grouping. And by doing that you might  
indeed create a case where the condition causing the conflict no longer  
exists, so the loop finds no conflict and falls off the end. But in  
that case it still won't report it correctly.

Hmmm... I'm starting to think that [+abc] groupings are a bad design,  
both because A) it's just as easy to test for FLAG_a|FLAG_b|FLAG_c in  
the command (which doesn't result in bigger code because the | gets  
resolved at compile time according to c99), and B) it makes figuring  
out _which_ option had a problem (after the fact) hard, because we  
didn't record which options we saw, just which bits got enabled as a  
result.

I just committed another attempt at a fix. Does this look reasonable?  
(Since a command can't forbid itself, this shouldn't be able to fall  
off the end of the list.)

Sorry it took so long. Thanks for the bug report.

Rob
 1356772843.0