Re: [PATCH] log --format: document %w
- From
René Scharfe <rene.scharfe@lsrfire.ath.cx>
- Date
- Nov 22, 2009, 23:46 UTC
- Message-ID
- <4B09CD5A.4070401@lsrfire.ath.cx>
- In-Reply-To
- <7vzl6eiiyx.fsf@alter.siamese.dyndns.org>
Junio C Hamano schrieb:
Show 60 quoted lines
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>
>> I'm not especially proud of the triple negative in that note. How to say it
>> better, yet concise?
>> +- '%w([<w>[,<i1>[,<i2>]]])': switch line wrapping, like the -w option of
>> + linkgit:git-shortlog[1]. NOTE: Color placeholders (`%C*`) are not
>> + recognized as having no width, so they should not be put into wrapped
>> + sections.
>
> "The code miscounts the width of '%C*' color placeholders"?
>
> Perhaps somebody in the codepath leading to pick_one_utf8_char() in utf8.c
> can be made aware of them?
>
> utf8_width() is called from many places (has one caller outside utf8.c as
> well). It is given a pointer to a pointer that points at the current
> position in a string, and is responsible for picking up one logical letter
> advancing the given pointer to skip over that letter, and returning the
> display width of that one letter. The function wants the string to be
> encoded in utf-8 and signals by putting NULL in the pointer when it
> detects the input string is not.
>
> Picking up one logical letter is done by pick_one_utf8_char(), which is a
> nicely written generic "We are at the character boundary of a potentially
> multi-byte utf-8 string; pick the first character" implementation, and we
> wouldn't want to contaminate that with escape sequence logic---we might
> want to reuse it in other codepaths where we have no reason to expect any
> escape sequences.
>
> So perhaps we can introduce is_esc_sequence(s, r, w) that
>
> - returns true if we are at the beginning of an esc-sequence;
> - skips the sequence just like utf8_width() does with s and r; and
> - counts the width of the sequence and returns it in *w
>
> The implementation of the is_esc_sequence() could be to only detect the
> color sequence (if the sequence has things like cursor-position control
> then we are already lost, as calling "utf8_width()" on such a string does
> not make much sense anyway) and report zero-width.
>
> I dunno.
>
> diff --git a/utf8.c b/utf8.c
> index 5c18f0c..d45e75f 100644
> --- a/utf8.c
> +++ b/utf8.c
> @@ -241,7 +241,12 @@ invalid:
> */
> int utf8_width(const char **start, size_t *remainder_p)
> {
> - ucs_char_t ch = pick_one_utf8_char(start, remainder_p);
> + ucs_char_t ch;
> + int w;
> +
> + if (is_esc_sequence(start, remainder_p, &w))
> + return w;
> + ch = pick_one_utf8_char(start, remainder_p);
> if (!*start)
> return 0;
> return git_wcwidth(ch);I think utf8_width() is too generic for that; we shouldn't teach it terminal control details. Something like this? It keeps it all local to strbuf_add_wrapped_text(); ignoring display mode escape codes in there can be justified with its purpose.
utf8.c | 23 ++++++++++++++++++++++- 1 files changed, 22 insertions(+), 1 deletions(-)
diff --git a/utf8.c b/utf8.c index 5c18f0c..fcc0aeb 100644 --- a/utf8.c +++ b/utf8.c @@ -298,6 +298,21 @@ static void print_spaces(struct strbuf *buf, int count) strbuf_write(buf, s, count); } +/* XXX: this handles display mode sequences, only. Do we need more? */ +static size_t esc_sequence_len(const char *s) +{ + const char *p = s; + if (*p++ != '\033') + return 0; + if (*p++ != '[') + return 0; + while (isdigit(*p) || *p == ';') + p++; + if (*p++ != 'm') + return 0; + return p - s; +} + /* * Wrap the text, if necessary. The variable indent is the indent for the * first line, indent2 is the indent for all other lines. @@ -329,7 +344,13 @@ int strbuf_add_wrapped_text(struct strbuf *buf, } for (;;) { - char c = *text; + char c; + size_t skip; + + while ((skip = esc_sequence_len(text))) + text += skip; + + c = *text; if (!c || isspace(c)) { if (w < width || !space) { const char *start = bol;