Re: [PATCH] log --format: document %w
From: René Scharfe <hidden>
Date: 2016-06-15 22:47:45
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH] log --format: document %w · René Scharfe <hidden>
- 2016-06-15 · Re: [PATCH] log --format: document %w · Junio C Hamano <hidden>
Junio C Hamano schrieb:
quoted hunk ↗ jump to hunk
René Scharfe [off-list ref] writes:quoted
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;