Re: [PATCH] wt-status: take the alignment burden off translators

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> +	static const char *spaces = "                                ";

I do not see anything that limits "len" used to show only show the
prefix bytes in this array not to ask for more than you have spaces
here.  It won't overrun the end of this array, but I suspect your
result will not align when that happens.

> +	static int width = -1;
> +	int len;
>  
>  	one_name = two_name = it->string;
>  	switch (change_type) {
> @@ -309,32 +314,62 @@ static void wt_status_print_change_data(struct wt_status *s,
>  	status_printf(s, color(WT_STATUS_HEADER, s), "\t");
>  	switch (status) {
>  	case DIFF_STATUS_ADDED:
> -		status_printf_more(s, c, _("new file:   %s"), one);
> +		what = _("new file");
>  		break;
>  	case DIFF_STATUS_COPIED:
> -		status_printf_more(s, c, _("copied:     %s -> %s"), one, two);
> +		what = _("copied");
>  		break;
>  	case DIFF_STATUS_DELETED:
> -		status_printf_more(s, c, _("deleted:    %s"), one);
> +		what = _("deleted");
>  		break;
>  	case DIFF_STATUS_MODIFIED:
> -		status_printf_more(s, c, _("modified:   %s"), one);
> +		what = _("modified");
>  		break;
>  	case DIFF_STATUS_RENAMED:
> -		status_printf_more(s, c, _("renamed:    %s -> %s"), one, two);
> +		what = _("renamed");
>  		break;
>  	case DIFF_STATUS_TYPE_CHANGED:
> -		status_printf_more(s, c, _("typechange: %s"), one);
> +		what = _("typechange");
>  		break;
>  	case DIFF_STATUS_UNKNOWN:
> -		status_printf_more(s, c, _("unknown:    %s"), one);
> +		what = _("unknown");
>  		break;
>  	case DIFF_STATUS_UNMERGED:
> -		status_printf_more(s, c, _("unmerged:   %s"), one);
> +		what = _("unmerged");
>  		break;
>  	default:
>  		die(_("bug: unhandled diff status %c"), status);
>  	}
> +	if (width == -1) {
> +		/*
> +		 * Translators: if you do translate this, replace it
> +		 * with a decimal number of how many columns needed
> +		 * to align file names in "git status":
> +		 *
> +		 * |<-columns->|
> +		 *
> +		 *   unmerged: foo.c
> +		 *   copied:   foo.c -> bar.c
> +		 *
> +		 * The default value is 12. Normally you would not
> +		 * need to translate this at all unless the translated
> +		 * strings are longer than 12 columns and therefore
> +		 * break alignment.
> +		 */
> +		width = atoi(_("<wt-status.c:width>"));

Somewhat ugly.  Wouldn't it work better to keep the "what" strings
in a static array and compute "width" just once?  Is the sparseness
of DIFF_STATUS_* enums what makes such an implementation cumbersome?

I dunno.

> +		if (width <= 0 || width > 32)
> +			width = 12;
> +	}
> +	if (width > utf8_strwidth(what) + 1)
> +		len = width - utf8_strwidth(what) - 1;

Just a style, but this is far easier to read if you said:

	if (width > utf8_strwidth(what) + 1)
		len = width - (utf8_strwidth(what) + 1);

because you are essentially doing:

	if (A > B)
        	x = A - B;

Or even

	len = width - (utf8_strwidth(what) + 1);
        if (len < 0)
        	len = 0;

> +	else
> +		len = 0;
> +	if (status == DIFF_STATUS_COPIED || status == DIFF_STATUS_RENAMED)
> +		status_printf_more(s, c, "%s:%.*s%s -> %s",
> +				   what, len, spaces, one, two);
> +	else
> +		status_printf_more(s, c, "%s:%.*s%s",
> +				   what, len, spaces, one);
>  	if (extra.len) {
>  		status_printf_more(s, color(WT_STATUS_HEADER, s), "%s", extra.buf);
>  		strbuf_release(&extra);