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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Sep 11, 2015, 18:12 UTC
Message-ID: <CAOLa=ZTZiFzLogP=OXi_cS3Dn4775bU5LqBUWLu8Sf5ocs6T=g@mail.gmail.com>
In-Reply-To: <xmqqwpvwzvaa.fsf@gitster.mtv.corp.google.com>

On Fri, Sep 11, 2015 at 11:00 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 7 quoted lines

>
> 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!

Show 6 quoted lines

> 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 <gitster@pobox.com> wrote:

Show 51 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> 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))

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