On 08/11/2015 02:10 PM, enh wrote: > On Mon, Aug 10, 2015 at 11:58 PM, Rob Landley <[email protected]> wrote: >> On 08/10/2015 10:39 PM, enh wrote: >>> Fix several bugs in date. >> >> Sigh. I honestly did test it, although clearly not thoroughly enough. >> (That's how I set my clock to the year 2157, which networkmangler did >> _not_ like...) > > yeah, that's the advantage of having real unit tests. -d lets us test > the parsing reasonably well though (even if toybox's -d is > incompatible with coreutils').
If you mean "doesn't support multiple different fallback formats via some magic /etc file they only tell you about in the info pages", yes. If you mean something else, tell me more and maybe I can fix it? I admit that "date -D %s -d 1234567890 +%s" isn't something gnu date supports, although it can do date -d @1234567890 %s (Hmmm, mine assumes utc on @unix input but then displays in current timezone. Grrr. Not quite sure design-wise how to fix that. "Unix time is in utc" seemed like the right thing to do, but "time is displayed in current timezone unless you give it a flag" is existing behavior, and it should be consistent...) >> Ow. I see I checked in debug code. (I put my debug code flush with the >> left edge so it's easy to spot and yank again.) Sorry about that. >> >>> We were exiting before actually setting the date, chkmktime was always >>> failing, and we would segfault in glibc's strftime for non-Unix dates. >>> Plus one of the tests was mangled. >> >> Regression test suite. This is another one where I need to run it in a >> VM with root access... > > the date tests run fine without. (well, in the version below where > they don't assume the system timezone is UTC.) the trade-off, of > course, is that they don't actually test setting the date. but they > still catch all the errors we've had so far... First time I ran said test suite it complained because it wasn't root, then set the time to 2158. Admittedly that was because I had the test wrong... :) >>> Also fix existing tests and add some more, and stop using the overly >>> vague "bad" in errors. >> >> I intentionally use "bad" because I'm trying to restrict the error >> message vocabulary so non-native english speakers have a fighting chance >> to follow along without a dictionary. See "Error messages and >> internationalization" in http://landley.net/toybox/design.html >> >> It's likely localized man pages are available, or posix standards, or a >> translation of toybox's help -ah output web page. But annotating a bunch >> of strings with _("thingy") in each command is something I'd prefer to >> avoid because it's intrusive and waaaay too easy to introduce printf >> bugs. (And half the command names are english words anyway so a certain >> amount of english is probably unavoidable, but I would like to minimize >> the requirement for the 2/3 of the planet it's inconvenient for.) > > the problem with this is that the resulting error messages suck > because they're so vague, That is the most common failure mode of this approach, yes. :( > and you can't even tell what the problem was > by looking at the source (because it's one error for every possible > point of failure). i agree that "invalid" isn't great, but i couldn't > come up with a short way to say what it actually means (other than > "non-normal", which is accurate but not helpful even for 99.9% of > native English speakers). Which is why I try to show the thing I couldn't parse, preferably with the location at which I couldn't parse it. User supplied input doesn't need to be translated. :) Not really an option here since the library's doing the parsing for us and not providing this granularity of information. Recreating the output (twice) was the best I could come up with. (Error handling is hard, even when we essentially abort() in response to almost everything.) > (your latest patch actually makes things worse because now you're not > even showing the string you failed to parse! fixed in the patch > below.) Sorry, quick 2am "fix the obvious breakage, look closer in the morning" thing. Wound up spending the morning trying to get cpio -p to work on nommu instead, and starting the skeleton of a tar.c rewrite. Oh, side note: cpio doesn't store xattrs. It also has a 32 bit date (y2038) and 32 bit file lengths. This means initramfs is kinda limited. Do you care strongly about this? There was a thread on linux-kernel a while back about possibly extending the format to fix htis, but it petered out. Since I'm looking at cpio anyway, I'm thinking of doing a 070702 format with some extra data fields, plus an initramfs patch so the kernel can read 'em to populate initramfs... >> I had two messages this time because failure to handle input and failure >> to handle output are two distinct error conditions. (Failure to parse >> date is because it's a bad date. Failure to _set_ the date is usually a >> permissions issue, you'll note the "cannot set date" and "bad format" >> messages are perror_exit() and the libc message probably _is_ >> internationalized.) >> >> Generally when I want to provide more details about failure to parse a >> string I say "bad stringname" and then give the string and the offset in >> it, ala sed's error_exit("bad pattern '%s'@%ld (%c)", errstart, >> line-errstart+1L, *line); >> >> Other times I use "bad $ADJECTIVE date" where you can treat $ADJECTIVE >> as a swear word and still get the general idea. You're distinguishing >> failure to parse ("bad date format", and since format is already a word >> in the help text it's not _entirely_ new) from the date itself being >> invalid ("bad date"). >> >> Rather than asking non english speakers to distinguish "invalid" from >> "bad", I'd rather try to print the input and output dates to show why >> they don't match, ala error_msg("%s != %s", unfiltered, filtered); > > yeah, that's a good idea. done in the patch below. this also turned up > another couple of bugs (also fixed in the patch below). > >> I checked in about 1/3 of your patch (most obvious bugfixes), I need to >> look at the rest in the morning. (I wanted to turn chmktime() into >> xmktime() and stick it in lib/xwrap.c but it didn't error_exit(). >> Changing the semantics of that makes life easier there. But not >> something to fiddle with at 2am... :) > > what you did actually made date worse (with the exception of removing > the exit; at least what's currently checked in should work if you > supply a Unix time with @). if you didn't have such an urge to win the > obfuscated C contest, toybox would be much better off... Sorry, it was 2am I and I was trying _not_ to fiddle, while not leaving the tree in a broken state. Applied this one as is. >> Thanks, >> >> Rob > > the above-promised patch follows... Your email client still wraps long lines, FYI. (Fixed 'em up, but it can lose multiple spaces in the breaks and tests can care..) Thanks, Rob _______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
