{"thread":{"id":"21703","subject":"[PATCH] log --format: document %w","startedAt":"2009-11-22T16:15:31Z","lastAt":"2009-11-23T22:40:03Z","messageCount":5,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"128106","messageId":"4B0963A3.4060804@lsrfire.ath.cx","threadId":"21703","inReplyTo":null,"subject":"[PATCH] log --format: document %w","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-11-22T16:15:31Z","receivedAt":"2009-11-22T16:15:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\nI'm not especially proud of the triple negative in that note.  How to say it\nbetter, yet concise?\n\n Documentation/pretty-formats.txt |    4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 0946202..7ff6a6c 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -135,6 +135,10 @@ The placeholders are:\n - '%m': left, right or boundary mark\n - '%n': newline\n - '%x00': print a byte from a hex code\n+- '%w([<w>[,<i1>[,<i2>]]])': switch line wrapping, like the -w option of\n+  linkgit:git-shortlog[1].  NOTE: Color placeholders (`%C*`) are not\n+  recognized as having no width, so they should not be put into wrapped\n+  sections.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\n-- \n1.6.5.3\n"},{"id":"128114","messageId":"7vzl6eiiyx.fsf@alter.siamese.dyndns.org","threadId":"21703","inReplyTo":"4B0963A3.4060804@lsrfire.ath.cx","subject":"Re: [PATCH] log --format: document %w","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-22T17:10:14Z","receivedAt":"2009-11-22T17:10:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> I'm not especially proud of the triple negative in that note.  How to say it\n> better, yet concise?\n> +- '%w([<w>[,<i1>[,<i2>]]])': switch line wrapping, like the -w option of\n> +  linkgit:git-shortlog[1].  NOTE: Color placeholders (`%C*`) are not\n> +  recognized as having no width, so they should not be put into wrapped\n> +  sections.\n\n\"The code miscounts the width of '%C*' color placeholders\"?\n\nPerhaps somebody in the codepath leading to pick_one_utf8_char() in utf8.c\ncan be made aware of them?\n\nutf8_width() is called from many places (has one caller outside utf8.c as\nwell).  It is given a pointer to a pointer that points at the current\nposition in a string, and is responsible for picking up one logical letter\nadvancing the given pointer to skip over that letter, and returning the\ndisplay width of that one letter.  The function wants the string to be\nencoded in utf-8 and signals by putting NULL in the pointer when it\ndetects the input string is not.\n\nPicking up one logical letter is done by pick_one_utf8_char(), which is a\nnicely written generic \"We are at the character boundary of a potentially\nmulti-byte utf-8 string; pick the first character\" implementation, and we\nwouldn't want to contaminate that with escape sequence logic---we might\nwant to reuse it in other codepaths where we have no reason to expect any\nescape sequences.\n\nSo perhaps we can introduce is_esc_sequence(s, r, w) that\n\n - returns true if we are at the beginning of an esc-sequence;\n - skips the sequence just like utf8_width() does with s and r; and\n - counts the width of the sequence and returns it in *w\n\nThe implementation of the is_esc_sequence() could be to only detect the\ncolor sequence (if the sequence has things like cursor-position control\nthen we are already lost, as calling \"utf8_width()\" on such a string does\nnot make much sense anyway) and report zero-width.\n\nI dunno.\n\ndiff --git a/utf8.c b/utf8.c\nindex 5c18f0c..d45e75f 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -241,7 +241,12 @@ invalid:\n  */\n int utf8_width(const char **start, size_t *remainder_p)\n {\n-\tucs_char_t ch = pick_one_utf8_char(start, remainder_p);\n+\tucs_char_t ch;\n+\tint w;\n+\n+\tif (is_esc_sequence(start, remainder_p, &w))\n+\t\treturn w;\n+\tch = pick_one_utf8_char(start, remainder_p);\n \tif (!*start)\n \t\treturn 0;\n \treturn git_wcwidth(ch);\n"},{"id":"128134","messageId":"4B09CD5A.4070401@lsrfire.ath.cx","threadId":"21703","inReplyTo":"7vzl6eiiyx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] log --format: document %w","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-11-22T23:46:34Z","receivedAt":"2009-11-22T23:46:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Junio C Hamano schrieb:\n> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n> \n>> I'm not especially proud of the triple negative in that note.  How to say it\n>> better, yet concise?\n>> +- '%w([<w>[,<i1>[,<i2>]]])': switch line wrapping, like the -w option of\n>> +  linkgit:git-shortlog[1].  NOTE: Color placeholders (`%C*`) are not\n>> +  recognized as having no width, so they should not be put into wrapped\n>> +  sections.\n> \n> \"The code miscounts the width of '%C*' color placeholders\"?\n> \n> Perhaps somebody in the codepath leading to pick_one_utf8_char() in utf8.c\n> can be made aware of them?\n> \n> utf8_width() is called from many places (has one caller outside utf8.c as\n> well).  It is given a pointer to a pointer that points at the current\n> position in a string, and is responsible for picking up one logical letter\n> advancing the given pointer to skip over that letter, and returning the\n> display width of that one letter.  The function wants the string to be\n> encoded in utf-8 and signals by putting NULL in the pointer when it\n> detects the input string is not.\n> \n> Picking up one logical letter is done by pick_one_utf8_char(), which is a\n> nicely written generic \"We are at the character boundary of a potentially\n> multi-byte utf-8 string; pick the first character\" implementation, and we\n> wouldn't want to contaminate that with escape sequence logic---we might\n> want to reuse it in other codepaths where we have no reason to expect any\n> escape sequences.\n> \n> So perhaps we can introduce is_esc_sequence(s, r, w) that\n> \n>  - returns true if we are at the beginning of an esc-sequence;\n>  - skips the sequence just like utf8_width() does with s and r; and\n>  - counts the width of the sequence and returns it in *w\n> \n> The implementation of the is_esc_sequence() could be to only detect the\n> color sequence (if the sequence has things like cursor-position control\n> then we are already lost, as calling \"utf8_width()\" on such a string does\n> not make much sense anyway) and report zero-width.\n> \n> I dunno.\n> \n> diff --git a/utf8.c b/utf8.c\n> index 5c18f0c..d45e75f 100644\n> --- a/utf8.c\n> +++ b/utf8.c\n> @@ -241,7 +241,12 @@ invalid:\n>   */\n>  int utf8_width(const char **start, size_t *remainder_p)\n>  {\n> -\tucs_char_t ch = pick_one_utf8_char(start, remainder_p);\n> +\tucs_char_t ch;\n> +\tint w;\n> +\n> +\tif (is_esc_sequence(start, remainder_p, &w))\n> +\t\treturn w;\n> +\tch = pick_one_utf8_char(start, remainder_p);\n>  \tif (!*start)\n>  \t\treturn 0;\n>  \treturn git_wcwidth(ch);\n\nI think utf8_width() is too generic for that; we shouldn't teach it terminal\ncontrol details.  Something like this?  It keeps it all local to\nstrbuf_add_wrapped_text(); ignoring display mode escape codes in there can be\njustified with its purpose.\n\n utf8.c |   23 ++++++++++++++++++++++-\n 1 files changed, 22 insertions(+), 1 deletions(-)\n\ndiff --git a/utf8.c b/utf8.c\nindex 5c18f0c..fcc0aeb 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -298,6 +298,21 @@ static void print_spaces(struct strbuf *buf, int count)\n \tstrbuf_write(buf, s, count);\n }\n \n+/* XXX: this handles display mode sequences, only.  Do we need more? */\n+static size_t esc_sequence_len(const char *s)\n+{\n+\tconst char *p = s;\n+\tif (*p++ != '\\033')\n+\t\treturn 0;\n+\tif (*p++ != '[')\n+\t\treturn 0;\n+\twhile (isdigit(*p) || *p == ';')\n+\t\tp++;\n+\tif (*p++ != 'm')\n+\t\treturn 0;\n+\treturn p - s;\n+}\n+\n /*\n  * Wrap the text, if necessary. The variable indent is the indent for the\n  * first line, indent2 is the indent for all other lines.\n@@ -329,7 +344,13 @@ int strbuf_add_wrapped_text(struct strbuf *buf,\n \t}\n \n \tfor (;;) {\n-\t\tchar c = *text;\n+\t\tchar c;\n+\t\tsize_t skip;\n+\n+\t\twhile ((skip = esc_sequence_len(text)))\n+\t\t\ttext += skip;\n+\n+\t\tc = *text;\n \t\tif (!c || isspace(c)) {\n \t\t\tif (w < width || !space) {\n \t\t\t\tconst char *start = bol;\n"},{"id":"128138","messageId":"7vtywmayvk.fsf@alter.siamese.dyndns.org","threadId":"21703","inReplyTo":"4B09CD5A.4070401@lsrfire.ath.cx","subject":"Re: [PATCH] log --format: document %w","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-23T00:06:07Z","receivedAt":"2009-11-23T00:06:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> I think utf8_width() is too generic for that; we shouldn't teach it terminal\n> control details.\n\nI agree that the function whose purpose is to compute display width should\nnot be called utf8_width().\n\nThe outside caller of utf8_width() in diff.c uses it to truncate the\nfunction name hint on the hunk header line at character boundary.  The\ninput shouldn't have color escapes _we_ add (it might contain such\nsequences from the user data, though), so I agree that we shouldn't\ncontaminate this function with color escapes.\n\n> strbuf_add_wrapped_text(); ignoring display mode escape codes in there can be\n> justified with its purpose.\n\nSurely, and thanks.  The patch looks good.\n"},{"id":"128206","messageId":"4B0B0F43.6080509@lsrfire.ath.cx","threadId":"21703","inReplyTo":"7vtywmayvk.fsf@alter.siamese.dyndns.org","subject":"[PATCH] strbuf_add_wrapped_text(): skip over colour codes","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-11-23T22:40:03Z","receivedAt":"2009-11-23T22:40:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Ignore display mode escape sequences (colour codes) for the purpose of\ntext wrapping because they don't have a visible width.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\nRenamed function to document its purpose and limitation, and remove\nthe note that is obsoleted by this patch.\n\n Documentation/pretty-formats.txt |    4 +---\n utf8.c                           |   22 +++++++++++++++++++++-\n 2 files changed, 22 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 7ff6a6c..0683fb3 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -136,9 +136,7 @@ The placeholders are:\n - '%n': newline\n - '%x00': print a byte from a hex code\n - '%w([<w>[,<i1>[,<i2>]]])': switch line wrapping, like the -w option of\n-  linkgit:git-shortlog[1].  NOTE: Color placeholders (`%C*`) are not\n-  recognized as having no width, so they should not be put into wrapped\n-  sections.\n+  linkgit:git-shortlog[1].\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/utf8.c b/utf8.c\nindex 01d1869..7ddff23 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -314,6 +314,20 @@ static void strbuf_add_indented_text(struct strbuf *buf, const char *text,\n \t}\n }\n \n+static size_t display_mode_esc_sequence_len(const char *s)\n+{\n+\tconst char *p = s;\n+\tif (*p++ != '\\033')\n+\t\treturn 0;\n+\tif (*p++ != '[')\n+\t\treturn 0;\n+\twhile (isdigit(*p) || *p == ';')\n+\t\tp++;\n+\tif (*p++ != 'm')\n+\t\treturn 0;\n+\treturn p - s;\n+}\n+\n /*\n  * Wrap the text, if necessary. The variable indent is the indent for the\n  * first line, indent2 is the indent for all other lines.\n@@ -337,7 +351,13 @@ int strbuf_add_wrapped_text(struct strbuf *buf,\n \t}\n \n \tfor (;;) {\n-\t\tchar c = *text;\n+\t\tchar c;\n+\t\tsize_t skip;\n+\n+\t\twhile ((skip = display_mode_esc_sequence_len(text)))\n+\t\t\ttext += skip;\n+\n+\t\tc = *text;\n \t\tif (!c || isspace(c)) {\n \t\t\tif (w < width || !space) {\n \t\t\t\tconst char *start = bol;\n-- \n1.6.5\n"}]}