Re: [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams

Karthik Nayak [off-list ref] writes:

On Fri, Oct 9, 2015 at 12:10 AM, Matthieu Moy
[off-list ref] wrote:

quoted

Karthik Nayak [off-list ref] writes:

quoted

--- a/ref-filter.c
+++ b/ref-filter.c

@@ -1118,8 +1118,10 @@ static void populate_value(struct ref_array_item *ref)
                              char buf[40];

                              if (stat_tracking_info(branch, &num_ours,
-                                                    &num_theirs, NULL))
+                                                    &num_theirs, NULL)) {
+                                     v->s = "[gone]";

My remark about translation still holds. The string was previously
translated in "branch" and you are removing this translation (well, not
here, but when 09/10 starts using this code).

I should have mentioned in my cover letter, I didn't really understand
what has to be done about this, couldn't find much reference to go
about this. What do you suggest?

From the user point of view :

git for-each-ref --format '%(upstream:track)' => Should always be the
same, because this may be parsed by scripts (plumbing). Should not
depend on $LANG, and shouldn't change from a version of Git to another.

git branch --format '%(upstream:track)' => Should show what is most
pleasant to the user (porcelain): translated according to $LANG and
friends, and may be improved in the future.

I already pointed out a fix where a string was translated in a plumbing
command. Another example is setup_unpack_trees_porcelain() in
unpack-trees.c which solves exactly the same problem.

I'll followup with a small series on top of yours to show the way. I did
not try to polish it since I guess you have local changes on the same
part of the code. Feel free to squash patches together or to squash them
with yours. The commit messages are not meant to be final either.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

The char buf[40] is safe (at least while the strings are not
translated), but I'd rather avoid magic numbers like this 40 in the
code, and use a construct that does not have this size limitation.
Especially if it makes the code shorter.

Signed-off-by: Matthieu Moy <redacted>
---
 ref-filter.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 6044eb0..7932c21 100644
--- a/ref-filter.c
+++ b/ref-filter.c

@@ -1116,7 +1116,6 @@ static void populate_value(struct ref_array_item *ref)
 				 strcmp(formatp, "trackshort") &&
 				 (starts_with(name, "upstream") ||
 				  starts_with(name, "push"))) {
-				char buf[40];
 				unsigned int nobracket = 0;
 
 				if (!strcmp(valp, ",nobracket"))

@@ -1135,24 +1134,21 @@ static void populate_value(struct ref_array_item *ref)
 					v->s = "";
 				else if (!num_ours) {
 					if (nobracket)
-						sprintf(buf, "behind %d", num_theirs);
+						v->s = xstrfmt("behind %d", num_theirs);
 					else
-						sprintf(buf, "[behind %d]", num_theirs);
-					v->s = xstrdup(buf);
+						v->s = xstrfmt("[behind %d]", num_theirs);
 				} else if (!num_theirs) {
 					if (nobracket)
-						sprintf(buf, "ahead %d", num_ours);
+						v->s = xstrfmt("ahead %d", num_ours);
 					else
-						sprintf(buf, "[ahead %d]", num_ours);
-					v->s = xstrdup(buf);
+						v->s = xstrfmt("[ahead %d]", num_ours);
 				} else {
 					if (nobracket)
-						sprintf(buf, "ahead %d, behind %d",
-							num_ours, num_theirs);
+						v->s = xstrfmt("ahead %d, behind %d",
+							       num_ours, num_theirs);
 					else
-						sprintf(buf, "[ahead %d, behind %d]",
-						num_ours, num_theirs);
-					v->s = xstrdup(buf);
+						v->s = xstrfmt("[ahead %d, behind %d]",
+							       num_ours, num_theirs);
 				}
 				continue;
 			} else if (!strcmp(formatp, "trackshort") &&

-- 
2.6.0.rc2.24.gb06d8e9.dirty

Signed-off-by: Matthieu Moy <redacted>
---
 builtin/branch.c | 2 ++
 builtin/tag.c    | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/builtin/branch.c b/builtin/branch.c
index 9d6c062..041c649 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c

@@ -531,6 +531,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
+	setup_ref_filter_porcelain_msg();
+
 	memset(&filter, 0, sizeof(filter));
 	filter.kind = FILTER_REFS_BRANCHES;
 	filter.abbrev = -1;

diff --git a/builtin/tag.c b/builtin/tag.c
index 9e17dca..b6d262f 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c

@@ -372,6 +372,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	setup_ref_filter_porcelain_msg();
+
 	git_config(git_tag_config, sorting_tail);
 
 	memset(&opt, 0, sizeof(opt));

-- 
2.6.0.rc2.24.gb06d8e9.dirty

This patch shows the way, but is obviously incomplete as it works only
for "nobracket" version. Actually, I think the code should first build
the unbracketed output string and then do something like

if (!nobracket) {
	const char *to_free = v->s;
	v->s = xstrfmt("[%s]", v->s);
	free(to_free);
}

so we don't have to worry about brackets anywhere else in the code.

Signed-off-by: Matthieu Moy <redacted>
---
 ref-filter.c | 28 ++++++++++++++++++++++++----
 ref-filter.h |  3 +++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 7932c21..c2ee8c9 100644
--- a/ref-filter.c
+++ b/ref-filter.c

@@ -15,6 +15,26 @@
 #include "version.h"
 #include "wt-status.h"
 
+static struct ref_msg {
+	const char *gone;
+	const char *ahead;
+	const char *behind;
+	const char *ahead_behind;
+} msgs = {
+	"gone",
+	"ahead %d",
+	"behind %d",
+	"ahead %d, behind %d"
+};
+
+void setup_ref_filter_porcelain_msg(void)
+{
+	msgs.gone = _("gone");
+	msgs.ahead = _("ahead %d");
+	msgs.behind = _("behind %d");
+	msgs.ahead_behind = _("ahead %d, behind %d");
+}
+
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
 static struct {

@@ -1124,7 +1144,7 @@ static void populate_value(struct ref_array_item *ref)
 				if (stat_tracking_info(branch, &num_ours,
 						       &num_theirs, NULL)) {
 					if (nobracket)
-						v->s = "gone";
+						v->s = msgs.gone;
 					else
 						v->s = "[gone]";
 					continue;

@@ -1134,17 +1154,17 @@ static void populate_value(struct ref_array_item *ref)
 					v->s = "";
 				else if (!num_ours) {
 					if (nobracket)
-						v->s = xstrfmt("behind %d", num_theirs);
+						v->s = xstrfmt(msgs.behind, num_theirs);
 					else
 						v->s = xstrfmt("[behind %d]", num_theirs);
 				} else if (!num_theirs) {
 					if (nobracket)
-						v->s = xstrfmt("ahead %d", num_ours);
+						v->s = xstrfmt(msgs.ahead, num_ours);
 					else
 						v->s = xstrfmt("[ahead %d]", num_ours);
 				} else {
 					if (nobracket)
-						v->s = xstrfmt("ahead %d, behind %d",
+						v->s = xstrfmt(msgs.ahead_behind,
 							       num_ours, num_theirs);
 					else
 						v->s = xstrfmt("[ahead %d, behind %d]",

diff --git a/ref-filter.h b/ref-filter.h
index 0014b92..2cce02c 100644
--- a/ref-filter.h
+++ b/ref-filter.h

@@ -112,4 +112,7 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
 /*  Get the current HEAD's description */
 char *get_head_description(void);
 
+/*  Set up translated strings in the output. */
+void setup_ref_filter_porcelain_msg(void);
+
 #endif /*  REF_FILTER_H  */

-- 
2.6.0.rc2.24.gb06d8e9.dirty

On Sun, Oct 11, 2015 at 9:42 PM, Matthieu Moy
[off-list ref] wrote:

Karthik Nayak [off-list ref] writes:

quoted

On Fri, Oct 9, 2015 at 12:10 AM, Matthieu Moy
[off-list ref] wrote:

quoted

Karthik Nayak [off-list ref] writes:

quoted

--- a/ref-filter.c
+++ b/ref-filter.c

@@ -1118,8 +1118,10 @@ static void populate_value(struct ref_array_item *ref)
                              char buf[40];

                              if (stat_tracking_info(branch, &num_ours,
-                                                    &num_theirs, NULL))
+                                                    &num_theirs, NULL)) {
+                                     v->s = "[gone]";

My remark about translation still holds. The string was previously
translated in "branch" and you are removing this translation (well, not
here, but when 09/10 starts using this code).

I should have mentioned in my cover letter, I didn't really understand
what has to be done about this, couldn't find much reference to go
about this. What do you suggest?

From the user point of view :

git for-each-ref --format '%(upstream:track)' => Should always be the
same, because this may be parsed by scripts (plumbing). Should not
depend on $LANG, and shouldn't change from a version of Git to another.

A little blurry on how this works, as in how translation takes place,
probably need to look at some code.

git branch --format '%(upstream:track)' => Should show what is most
pleasant to the user (porcelain): translated according to $LANG and
friends, and may be improved in the future.

I already pointed out a fix where a string was translated in a plumbing
command. Another example is setup_unpack_trees_porcelain() in
unpack-trees.c which solves exactly the same problem.

You did. I was just too clueless.

I'll followup with a small series on top of yours to show the way. I did
not try to polish it since I guess you have local changes on the same
part of the code. Feel free to squash patches together or to squash them
with yours. The commit messages are not meant to be final either.

Thanks a lot for this. This should help me out. I'll probably squash them along.
Thanks :)

-- 
Regards,
Karthik Nayak

On Sun, Oct 11, 2015 at 9:46 PM, Matthieu Moy [off-list ref] wrote:

This patch shows the way, but is obviously incomplete as it works only
for "nobracket" version. Actually, I think the code should first build
the unbracketed output string and then do something like

if (!nobracket) {
        const char *to_free = v->s;
        v->s = xstrfmt("[%s]", v->s);
        free(to_free);
}

Makes sense.

I'm squashing this and the other two patches into my series. Thanks :)

-- 
Regards,
Karthik Nayak