[PATCH] fetch: align new ref summary printout in UTF-8 locales

Subsystems: the rest

STALE3737d

3 messages, 1 author, 2016-06-15 · open the first message on its own page

[PATCH] fetch: align new ref summary printout in UTF-8 locales

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:54:37

fetch does printf("%-*s", width, "foo") where "foo" can be an utf-8
string, but width is bytes, not letters. This results in misaligned
ref summary table.

Introduce gettext_length() function that returns the string length in
letters. Make the code use TRANSPORT_SUMMARY(x) where the length is
compensated properly in utf-8 locales.
---
 gettext_length() can be made to support other charsets too. But I'm
 on utf-8, it's not my itch.

 Grepping through '%-*s' does not reveal any other places that obviously
 need adjustment like this (apply and remote might, but pathnames and
 remote names are usually in ascii)

 builtin/fetch.c | 22 ++++++++++------------
 gettext.c       | 14 ++++++++++++--
 gettext.h       |  5 +++++
 transport.c     |  2 +-
 transport.h     |  1 +
 5 files changed, 29 insertions(+), 15 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index bb9a074..d7406d2 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -255,9 +255,8 @@ static int update_local_ref(struct ref *ref,
 	if (!hashcmp(ref->old_sha1, ref->new_sha1)) {
 		if (verbosity > 0)
 			strbuf_addf(display, "= %-*s %-*s -> %s",
-				    TRANSPORT_SUMMARY_WIDTH,
-				    _("[up to date]"), REFCOL_WIDTH,
-				    remote, pretty_ref);
+				    TRANSPORT_SUMMARY(_("[up to date]")),
+				    REFCOL_WIDTH, remote, pretty_ref);
 		return 0;
 	}
 
@@ -271,7 +270,7 @@ static int update_local_ref(struct ref *ref,
 		 */
 		strbuf_addf(display,
 			    _("! %-*s %-*s -> %s  (can't fetch in current branch)"),
-			    TRANSPORT_SUMMARY_WIDTH, _("[rejected]"),
+			    TRANSPORT_SUMMARY(_("[rejected]")),
 			    REFCOL_WIDTH, remote, pretty_ref);
 		return 1;
 	}
@@ -282,7 +281,7 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref("updating tag", ref, 0);
 		strbuf_addf(display, "%c %-*s %-*s -> %s%s",
 			    r ? '!' : '-',
-			    TRANSPORT_SUMMARY_WIDTH, _("[tag update]"),
+			    TRANSPORT_SUMMARY(_("[tag update]")),
 			    REFCOL_WIDTH, remote, pretty_ref,
 			    r ? _("  (unable to update local ref)") : "");
 		return r;
@@ -317,7 +316,7 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref(msg, ref, 0);
 		strbuf_addf(display, "%c %-*s %-*s -> %s%s",
 			    r ? '!' : '*',
-			    TRANSPORT_SUMMARY_WIDTH, what,
+			    TRANSPORT_SUMMARY(what),
 			    REFCOL_WIDTH, remote, pretty_ref,
 			    r ? _("  (unable to update local ref)") : "");
 		return r;
@@ -335,7 +334,7 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref("fast-forward", ref, 1);
 		strbuf_addf(display, "%c %-*s %-*s -> %s%s",
 			    r ? '!' : ' ',
-			    TRANSPORT_SUMMARY_WIDTH, quickref,
+			    TRANSPORT_SUMMARY(quickref),
 			    REFCOL_WIDTH, remote, pretty_ref,
 			    r ? _("  (unable to update local ref)") : "");
 		return r;
@@ -351,13 +350,13 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref("forced-update", ref, 1);
 		strbuf_addf(display, "%c %-*s %-*s -> %s  (%s)",
 			    r ? '!' : '+',
-			    TRANSPORT_SUMMARY_WIDTH, quickref,
+			    TRANSPORT_SUMMARY(quickref),
 			    REFCOL_WIDTH, remote, pretty_ref,
 			    r ? _("unable to update local ref") : _("forced update"));
 		return r;
 	} else {
 		strbuf_addf(display, "! %-*s %-*s -> %s  %s",
-			    TRANSPORT_SUMMARY_WIDTH, _("[rejected]"),
+			    TRANSPORT_SUMMARY(_("[rejected]")),
 			    REFCOL_WIDTH, remote, pretty_ref,
 			    _("(non-fast-forward)"));
 		return 1;
@@ -479,8 +478,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				free(ref);
 			} else
 				strbuf_addf(&note, "* %-*s %-*s -> FETCH_HEAD",
-					    TRANSPORT_SUMMARY_WIDTH,
-					    *kind ? kind : "branch",
+					    TRANSPORT_SUMMARY(*kind ? kind : "branch"),
 					    REFCOL_WIDTH,
 					    *what ? what : "HEAD");
 			if (note.len) {
@@ -554,7 +552,7 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)
 			result |= delete_ref(ref->name, NULL, 0);
 		if (verbosity >= 0) {
 			fprintf(stderr, " x %-*s %-*s -> %s\n",
-				TRANSPORT_SUMMARY_WIDTH, _("[deleted]"),
+				TRANSPORT_SUMMARY(_("[deleted]")),
 				REFCOL_WIDTH, _("(none)"), prettify_refname(ref->name));
 			warn_dangling_symref(stderr, dangling_msg, ref->name);
 		}
diff --git a/gettext.c b/gettext.c
index f75bca7..e9f0f0d 100644
--- a/gettext.c
+++ b/gettext.c
@@ -4,6 +4,8 @@
 
 #include "git-compat-util.h"
 #include "gettext.h"
+#include "strbuf.h"
+#include "utf8.h"
 
 #ifndef NO_GETTEXT
 #	include <locale.h>
@@ -27,10 +29,9 @@ int use_gettext_poison(void)
 #endif
 
 #ifndef NO_GETTEXT
+static const char *charset;
 static void init_gettext_charset(const char *domain)
 {
-	const char *charset;
-
 	/*
 	   This trick arranges for messages to be emitted in the user's
 	   requested encoding, but avoids setting LC_CTYPE from the
@@ -128,4 +129,13 @@ void git_setup_gettext(void)
 	init_gettext_charset("git");
 	textdomain("git");
 }
+
+int gettext_length(const char *s)
+{
+	static int is_utf8 = -1;
+	if (is_utf8 == -1)
+		is_utf8 = !strcmp(charset, "UTF-8");
+
+	return is_utf8 ? utf8_strwidth(s) : strlen(s);
+}
 #endif
diff --git a/gettext.h b/gettext.h
index 57ba8bb..7ea6db4 100644
--- a/gettext.h
+++ b/gettext.h
@@ -30,10 +30,15 @@
 
 #ifndef NO_GETTEXT
 extern void git_setup_gettext(void);
+extern int gettext_length(const char *s);
 #else
 static inline void git_setup_gettext(void)
 {
 }
+static inline int gettext_length(const char *s)
+{
+	return strlen(s);
+}
 #endif
 
 #ifdef GETTEXT_POISON
diff --git a/transport.c b/transport.c
index 1811b50..863aaa6 100644
--- a/transport.c
+++ b/transport.c
@@ -629,7 +629,7 @@ static void print_ref_status(char flag, const char *summary, struct ref *to, str
 		else
 			fprintf(stdout, "%s\n", summary);
 	} else {
-		fprintf(stderr, " %c %-*s ", flag, TRANSPORT_SUMMARY_WIDTH, summary);
+		fprintf(stderr, " %c %-*s ", flag, TRANSPORT_SUMMARY(summary));
 		if (from)
 			fprintf(stderr, "%s -> %s", prettify_refname(from->name), prettify_refname(to->name));
 		else
diff --git a/transport.h b/transport.h
index b866c12..1c95a8c 100644
--- a/transport.h
+++ b/transport.h
@@ -106,6 +106,7 @@ struct transport {
 #define TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND 256
 
 #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
+#define TRANSPORT_SUMMARY(x) (TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_length(x)), (x)
 
 /* Returns a transport suitable for the url */
 struct transport *transport_get(struct remote *, const char *);
-- 
1.7.12.rc2.18.g61b472e

[PATCH v2] fetch: align new ref summary printout in UTF-8 locales

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:54:37

fetch does printf("%-*s", width, "foo") where "foo" can be a utf-8
string, but width is in bytes, not columns. For ASCII it's fine as one
byte takes one column. For utf-8, this may result in misaligned ref
summary table.

Introduce gettext_width() function that returns the string length in
columns (currently only supports utf-8 locales). Make the code use
TRANSPORT_SUMMARY(x) where the length is compensated properly in
non-English locales.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
  - rename gettext_length() to gettext_width()
  - use "width" instead of letters
  - leave other "%-*s" places unchanged if they always take ascii
    strings (i.e. no _() calls)
  - note to self, may need to i18n-ize print_ref_status() in
    transport.c, looks like it's used by git-push only

 builtin/fetch.c | 15 +++++++--------
 gettext.c       | 15 +++++++++++++--
 gettext.h       |  5 +++++
 transport.h     |  1 +
 4 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index bb9a074..85e291f 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -255,9 +255,8 @@ static int update_local_ref(struct ref *ref,
 	if (!hashcmp(ref->old_sha1, ref->new_sha1)) {
 		if (verbosity > 0)
 			strbuf_addf(display, "= %-*s %-*s -> %s",
-				    TRANSPORT_SUMMARY_WIDTH,
-				    _("[up to date]"), REFCOL_WIDTH,
-				    remote, pretty_ref);
+				    TRANSPORT_SUMMARY(_("[up to date]")),
+				    REFCOL_WIDTH, remote, pretty_ref);
 		return 0;
 	}
 
@@ -271,7 +270,7 @@ static int update_local_ref(struct ref *ref,
 		 */
 		strbuf_addf(display,
 			    _("! %-*s %-*s -> %s  (can't fetch in current branch)"),
-			    TRANSPORT_SUMMARY_WIDTH, _("[rejected]"),
+			    TRANSPORT_SUMMARY(_("[rejected]")),
 			    REFCOL_WIDTH, remote, pretty_ref);
 		return 1;
 	}
@@ -282,7 +281,7 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref("updating tag", ref, 0);
 		strbuf_addf(display, "%c %-*s %-*s -> %s%s",
 			    r ? '!' : '-',
-			    TRANSPORT_SUMMARY_WIDTH, _("[tag update]"),
+			    TRANSPORT_SUMMARY(_("[tag update]")),
 			    REFCOL_WIDTH, remote, pretty_ref,
 			    r ? _("  (unable to update local ref)") : "");
 		return r;
@@ -317,7 +316,7 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref(msg, ref, 0);
 		strbuf_addf(display, "%c %-*s %-*s -> %s%s",
 			    r ? '!' : '*',
-			    TRANSPORT_SUMMARY_WIDTH, what,
+			    TRANSPORT_SUMMARY(what),
 			    REFCOL_WIDTH, remote, pretty_ref,
 			    r ? _("  (unable to update local ref)") : "");
 		return r;
@@ -357,7 +356,7 @@ static int update_local_ref(struct ref *ref,
 		return r;
 	} else {
 		strbuf_addf(display, "! %-*s %-*s -> %s  %s",
-			    TRANSPORT_SUMMARY_WIDTH, _("[rejected]"),
+			    TRANSPORT_SUMMARY(_("[rejected]")),
 			    REFCOL_WIDTH, remote, pretty_ref,
 			    _("(non-fast-forward)"));
 		return 1;
@@ -554,7 +553,7 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)
 			result |= delete_ref(ref->name, NULL, 0);
 		if (verbosity >= 0) {
 			fprintf(stderr, " x %-*s %-*s -> %s\n",
-				TRANSPORT_SUMMARY_WIDTH, _("[deleted]"),
+				TRANSPORT_SUMMARY(_("[deleted]")),
 				REFCOL_WIDTH, _("(none)"), prettify_refname(ref->name));
 			warn_dangling_symref(stderr, dangling_msg, ref->name);
 		}
diff --git a/gettext.c b/gettext.c
index f75bca7..71e9545 100644
--- a/gettext.c
+++ b/gettext.c
@@ -4,6 +4,8 @@
 
 #include "git-compat-util.h"
 #include "gettext.h"
+#include "strbuf.h"
+#include "utf8.h"
 
 #ifndef NO_GETTEXT
 #	include <locale.h>
@@ -27,10 +29,9 @@ int use_gettext_poison(void)
 #endif
 
 #ifndef NO_GETTEXT
+static const char *charset;
 static void init_gettext_charset(const char *domain)
 {
-	const char *charset;
-
 	/*
 	   This trick arranges for messages to be emitted in the user's
 	   requested encoding, but avoids setting LC_CTYPE from the
@@ -128,4 +129,14 @@ void git_setup_gettext(void)
 	init_gettext_charset("git");
 	textdomain("git");
 }
+
+/* return the number of columns of string 's' in current locale */
+int gettext_width(const char *s)
+{
+	static int is_utf8 = -1;
+	if (is_utf8 == -1)
+		is_utf8 = !strcmp(charset, "UTF-8");
+
+	return is_utf8 ? utf8_strwidth(s) : strlen(s);
+}
 #endif
diff --git a/gettext.h b/gettext.h
index 57ba8bb..e44d8bc 100644
--- a/gettext.h
+++ b/gettext.h
@@ -30,10 +30,15 @@
 
 #ifndef NO_GETTEXT
 extern void git_setup_gettext(void);
+extern int gettext_width(const char *s);
 #else
 static inline void git_setup_gettext(void)
 {
 }
+static inline int gettext_width(const char *s)
+{
+	return strlen(s);
+}
 #endif
 
 #ifdef GETTEXT_POISON
diff --git a/transport.h b/transport.h
index b866c12..a5d375e 100644
--- a/transport.h
+++ b/transport.h
@@ -106,6 +106,7 @@ struct transport {
 #define TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND 256
 
 #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
+#define TRANSPORT_SUMMARY(x) (TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
 
 /* Returns a transport suitable for the url */
 struct transport *transport_get(struct remote *, const char *);
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH v2] fetch: align new ref summary printout in UTF-8 locales

From: Nguyen Thai Ngoc Duy <hidden>
Date: 2016-06-15 22:54:43

Ping.. what happens to this patch? Do you want to support other
encodings as well via iconv()? I can't test that though.

On Tue, Sep 4, 2012 at 5:39 PM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:
quoted hunk
fetch does printf("%-*s", width, "foo") where "foo" can be a utf-8
string, but width is in bytes, not columns. For ASCII it's fine as one
byte takes one column. For utf-8, this may result in misaligned ref
summary table.

Introduce gettext_width() function that returns the string length in
columns (currently only supports utf-8 locales). Make the code use
TRANSPORT_SUMMARY(x) where the length is compensated properly in
non-English locales.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
  - rename gettext_length() to gettext_width()
  - use "width" instead of letters
  - leave other "%-*s" places unchanged if they always take ascii
    strings (i.e. no _() calls)
  - note to self, may need to i18n-ize print_ref_status() in
    transport.c, looks like it's used by git-push only

 builtin/fetch.c | 15 +++++++--------
 gettext.c       | 15 +++++++++++++--
 gettext.h       |  5 +++++
 transport.h     |  1 +
 4 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index bb9a074..85e291f 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -255,9 +255,8 @@ static int update_local_ref(struct ref *ref,
        if (!hashcmp(ref->old_sha1, ref->new_sha1)) {
                if (verbosity > 0)
                        strbuf_addf(display, "= %-*s %-*s -> %s",
-                                   TRANSPORT_SUMMARY_WIDTH,
-                                   _("[up to date]"), REFCOL_WIDTH,
-                                   remote, pretty_ref);
+                                   TRANSPORT_SUMMARY(_("[up to date]")),
+                                   REFCOL_WIDTH, remote, pretty_ref);
                return 0;
        }
@@ -271,7 +270,7 @@ static int update_local_ref(struct ref *ref,
                 */
                strbuf_addf(display,
                            _("! %-*s %-*s -> %s  (can't fetch in current branch)"),
-                           TRANSPORT_SUMMARY_WIDTH, _("[rejected]"),
+                           TRANSPORT_SUMMARY(_("[rejected]")),
                            REFCOL_WIDTH, remote, pretty_ref);
                return 1;
        }
@@ -282,7 +281,7 @@ static int update_local_ref(struct ref *ref,
                r = s_update_ref("updating tag", ref, 0);
                strbuf_addf(display, "%c %-*s %-*s -> %s%s",
                            r ? '!' : '-',
-                           TRANSPORT_SUMMARY_WIDTH, _("[tag update]"),
+                           TRANSPORT_SUMMARY(_("[tag update]")),
                            REFCOL_WIDTH, remote, pretty_ref,
                            r ? _("  (unable to update local ref)") : "");
                return r;
@@ -317,7 +316,7 @@ static int update_local_ref(struct ref *ref,
                r = s_update_ref(msg, ref, 0);
                strbuf_addf(display, "%c %-*s %-*s -> %s%s",
                            r ? '!' : '*',
-                           TRANSPORT_SUMMARY_WIDTH, what,
+                           TRANSPORT_SUMMARY(what),
                            REFCOL_WIDTH, remote, pretty_ref,
                            r ? _("  (unable to update local ref)") : "");
                return r;
@@ -357,7 +356,7 @@ static int update_local_ref(struct ref *ref,
                return r;
        } else {
                strbuf_addf(display, "! %-*s %-*s -> %s  %s",
-                           TRANSPORT_SUMMARY_WIDTH, _("[rejected]"),
+                           TRANSPORT_SUMMARY(_("[rejected]")),
                            REFCOL_WIDTH, remote, pretty_ref,
                            _("(non-fast-forward)"));
                return 1;
@@ -554,7 +553,7 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)
                        result |= delete_ref(ref->name, NULL, 0);
                if (verbosity >= 0) {
                        fprintf(stderr, " x %-*s %-*s -> %s\n",
-                               TRANSPORT_SUMMARY_WIDTH, _("[deleted]"),
+                               TRANSPORT_SUMMARY(_("[deleted]")),
                                REFCOL_WIDTH, _("(none)"), prettify_refname(ref->name));
                        warn_dangling_symref(stderr, dangling_msg, ref->name);
                }
diff --git a/gettext.c b/gettext.c
index f75bca7..71e9545 100644
--- a/gettext.c
+++ b/gettext.c
@@ -4,6 +4,8 @@

 #include "git-compat-util.h"
 #include "gettext.h"
+#include "strbuf.h"
+#include "utf8.h"

 #ifndef NO_GETTEXT
 #      include <locale.h>
@@ -27,10 +29,9 @@ int use_gettext_poison(void)
 #endif

 #ifndef NO_GETTEXT
+static const char *charset;
 static void init_gettext_charset(const char *domain)
 {
-       const char *charset;
-
        /*
           This trick arranges for messages to be emitted in the user's
           requested encoding, but avoids setting LC_CTYPE from the
@@ -128,4 +129,14 @@ void git_setup_gettext(void)
        init_gettext_charset("git");
        textdomain("git");
 }
+
+/* return the number of columns of string 's' in current locale */
+int gettext_width(const char *s)
+{
+       static int is_utf8 = -1;
+       if (is_utf8 == -1)
+               is_utf8 = !strcmp(charset, "UTF-8");
+
+       return is_utf8 ? utf8_strwidth(s) : strlen(s);
+}
 #endif
diff --git a/gettext.h b/gettext.h
index 57ba8bb..e44d8bc 100644
--- a/gettext.h
+++ b/gettext.h
@@ -30,10 +30,15 @@

 #ifndef NO_GETTEXT
 extern void git_setup_gettext(void);
+extern int gettext_width(const char *s);
 #else
 static inline void git_setup_gettext(void)
 {
 }
+static inline int gettext_width(const char *s)
+{
+       return strlen(s);
+}
 #endif

 #ifdef GETTEXT_POISON
diff --git a/transport.h b/transport.h
index b866c12..a5d375e 100644
--- a/transport.h
+++ b/transport.h
@@ -106,6 +106,7 @@ struct transport {
 #define TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND 256

 #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
+#define TRANSPORT_SUMMARY(x) (TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)

 /* Returns a transport suitable for the url */
 struct transport *transport_get(struct remote *, const char *);
--
1.7.12.rc2.18.g61b472e


-- 
Duy
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help