[PATCH] flexi-sized user/group/size fields for ls

Denys Vlasenko vda.linux at googlemail.com
Mon Jan 18 00:21:23 UTC 2010


On Saturday 16 January 2010 08:24, Ian Wienand wrote:
> Thanks for the review; here's another go
> 
> Denys Vlasenko wrote:
> > On Friday 15 January 2010 21:16, Ian Wienand wrote:
> > The code growth is somewhat bigt, the code is not Unicode friendly,
> > and patch introduces broken indentation.
> 
> For unicode I used bb_mbstrlen()

Using bb_mbstrlen is ok for now, but recently it was brought
to my attention that Unicode has double-wide chars (Japanese
and such)... >>>8O
Apparently we'd need to migrate to wc[s]width() functions
to support those. Gosh...

In the patch, printf("%*s") with unicode does not work properly.
It pads the value based on its strlen, not unicode width.

There is code to play around it, for example, in dumpleases.c:

#if ENABLE_FEATURE_ASSUME_UNICODE
                printf(" %-16s%s%*s", inet_ntoa(addr), lease.hostname,
                        20 - (int)bb_mbstrlen(lease.hostname), "");
#else
                printf(" %-16s%-20s", inet_ntoa(addr), lease.hostname);
#endif

but I think we need to create and use a better support infrastructure.
Something like:

	/* equivalent to printf("%-20.20s", str) */
	char unicode_buffer[20 * MB_CUR_MAX];
	printf("%s", unicode_exact(20, str, unicode_buffer);
	/* no need to free */

	/* equivalent to printf("%-20s", str) */
	char *malloced = unicode_minimum(20, str);
	printf("%s", malloced);
	free(malloced); /* ugh */

	/* equivalent to printf("%-20s", str), better one */
	printf("%s%*s", str, unicode_pad_to_width(str, 20), "");
 
	/* equivalent to printf("%20s", str) */
	printf("%*s%s", unicode_pad_to_width(str, 20), "", str);


> Indentation; should be fixed

It is not fixed. Two space indents are still there.

> Fixed compliation when opts turned off and used the right groups function too.
> 
> For code growth, I reworked it a little.  For the case of the tiny
> system with ENABLE_FEATURE_AUTOWIDTH and ENABLE_FEATURE_LS_USERNAME
> disabled growth is 10 bytes

It should be 0 bytes. You needlessly use %*s format in this case.


> With both enabled, IMHO I'd prefer to use a bit more space and know
> that the output will look right

     w = bb_mbstrlen(make_human_readable_str(dn[i]->dstat.st_size, 1, 0));

This one (1) is never in unicode, and
(2) it is known that it is always 7 chars wide or less,
can just use 7-byte field.

--
vda


More information about the busybox mailing list