Re: [PATCH v17 00/14] port tag.c to use ref-filter APIs

Eric Sunshine [off-list ref] writes:

In addition to a link to the previous round and an interdiff, it would
be helpful to reviewers for you to annotate each patch (in the
commentary are below the "---" line after your sign-off) with a
description of the changes in that patch since the previous round in
order to focus the reviewer's attention (where it needs to be) on the
latest changes.

I may have got confused by seeing the same v17 (if they were marked
as v18 or v17bis, it would have been easier to make sure I didn't
miss anything), but here is the difference between what I had last
night and what I queued.  The removal of !body[1] and flipping the
order of to_free/format are not seen because I already had a local
fix-up SQUASH??? commits queued in the yesterday's batch.


$ git diff --stat -p kn/for-each-tag@{4.hours} kn/for-each-tag
 ref-filter.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 226e94d..fd839ac 100644
--- a/ref-filter.c
+++ b/ref-filter.c

@@ -47,9 +47,6 @@ static struct {
 	{ "subject" },
 	{ "body" },
 	{ "contents" },
-	{ "contents:subject" },
-	{ "contents:body" },
-	{ "contents:signature" },
 	{ "upstream" },
 	{ "push" },
 	{ "symref" },

@@ -58,7 +55,6 @@ static struct {
 	{ "color" },
 	{ "align" },
 	{ "end" },
-	{ "contents:lines" },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }

@@ -899,6 +895,7 @@ static void populate_value(struct ref_array_item *ref)
 			align->position = ALIGN_LEFT;
 
 			while (*s) {
+				/*  Strip trailing comma */
 				if (s[1])
 					strbuf_setlen(s[0], s[0]->len - 1);
 				if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))

On Fri, Sep 11, 2015 at 11:00 PM, Eric Sunshine [off-list ref] wrote:

Hmm, but what actually changed in the re-sent patches? Without a link
to the discussion leading up to the re-send of changed-only patches,
and without an interdiff, the re-send is opaque and less accessible to
the reviewer; which is at odds with Matthieu's suggestion which was
intended to make review easier and more streamlined.

Should have put an interdiff, my bad!

In addition to a link to the previous round and an interdiff, it would
be helpful to reviewers for you to annotate each patch (in the
commentary are below the "---" line after your sign-off) with a
description of the changes in that patch since the previous round in
order to focus the reviewer's attention (where it needs to be) on the
latest changes.

WIll follow this next time :)

On Fri, Sep 11, 2015 at 11:35 PM, Junio C Hamano [off-list ref] wrote:

The complete interdiff:

diff --git a/builtin/tag.c b/builtin/tag.c
index 081fe84..f2f6e2d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c

@@ -43,9 +43,9 @@ static int list_tags(struct ref_filter *filter,

struct ref_sorting *sorting, con

     if (!format) {
         if (filter->lines) {
-            format = xstrfmt("%s %%(contents:lines=%d)",
-                     "%(align:15)%%(refname:short)%%(end)", filter->lines);
-            to_free = format;
+            to_free = xstrfmt("%s %%(contents:lines=%d)",
+                      "%(align:15)%(refname:short)%(end)", filter->lines);
+            format = to_free;
         } else
             format = "%(refname:short)";
     }

diff --git a/ref-filter.c b/ref-filter.c
index 59716db..fd839ac 100644
--- a/ref-filter.c
+++ b/ref-filter.c

@@ -47,9 +47,6 @@ static struct {
     { "subject" },
     { "body" },
     { "contents" },
-    { "contents:subject" },
-    { "contents:body" },
-    { "contents:signature" },
     { "upstream" },
     { "push" },
     { "symref" },

@@ -58,7 +55,6 @@ static struct {
     { "color" },
     { "align" },
     { "end" },
-    { "contents:lines" },
 };

 #define REF_FORMATTING_STATE_INIT  { 0, NULL }

@@ -268,7 +264,7 @@ static int match_atom_name(const char *name, const

char *atom_name, const char *

     if (!skip_prefix(name, atom_name, &body))
         return 0; /* doesn't even begin with "atom_name" */
-    if (!body[0] || !body[1]) {
+    if (!body[0]) {
         *val = NULL; /* %(atom_name) and no customization */
         return 1;
     }

@@ -899,6 +895,7 @@ static void populate_value(struct ref_array_item *ref)
             align->position = ALIGN_LEFT;

             while (*s) {
+                /*  Strip trailing comma */
                 if (s[1])
                     strbuf_setlen(s[0], s[0]->len - 1);
                 if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))

-- 
Regards,
Karthik Nayak