v3, 9 partsPort tag.c to use ref-filter APIs

> Add a new atom "align" and support %(align:X) where X is a number.
> This will align the preceeding atom value to the left followed by
> spaces for a total length of X characters. If X is less than the item
> size, the entire atom value is printed.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 7561727..b81a08d 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -53,6 +55,7 @@ static struct {
>         { "flag" },
>         { "HEAD" },
>         { "color" },
> +       { "align" },

>  };
>
>  /*
> @@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)
>                         else
>                                 v->s = " ";
>                         continue;
> +               } else if (starts_with(name, "align:")) {
> +                       const char *valp = NULL;
> +
> +                       skip_prefix(name, "align:", &valp);
> +                       if (!valp[0])
> +                               die(_("No value given with 'align='"));

> +                       v->s = "";
> +                       continue;
>                 } else
>                         continue;
>
> @@ -1254,17 +1268,38 @@ static void emit(const char *cp, const char *ep)
>         }
>  }
>
> +static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)
> +{
> +       if (v->s[0] && ref->align_value) {

> +               if (ref->align_value > len) {
> +                       struct strbuf buf = STRBUF_INIT;
> +                       strbuf_addstr(&buf, v->s);
> +                       if (!v->s[0])
> +                               free((char *)v->s);

> +                       strbuf_addchars(&buf, ' ', ref->align_value - len);
> +                       v->s = strbuf_detach(&buf, NULL);
> +               }
> +               ref->align_value = 0;
> +       }
> +}

> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Add a new atom "align" and support %(align:X) where X is a number.
>> This will align the preceeding atom value to the left followed by
>> spaces for a total length of X characters. If X is less than the item
>> size, the entire atom value is printed.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 7561727..b81a08d 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -53,6 +55,7 @@ static struct {
>>         { "flag" },
>>         { "HEAD" },
>>         { "color" },
>> +       { "align" },
>
> Not a new issue, but some compilers (Solaris?) complain about the
> trailing comma.
>

>>  };
>>
>>  /*
>> @@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)
>>                         else
>>                                 v->s = " ";
>>                         continue;
>> +               } else if (starts_with(name, "align:")) {
>> +                       const char *valp = NULL;
>> +
>> +                       skip_prefix(name, "align:", &valp);
>> +                       if (!valp[0])
>> +                               die(_("No value given with 'align='"));
>
> The parser expects "align:", but the error message talks about
> "align=". Also, current trend is to drop capitalization from the error
> message.
>

>> +                       strtoul_ui(valp, 10, &ref->align_value);
>> +                       if (ref->align_value < 1)
>> +                               die(_("Value should be greater than zero"));
>
> Drop capitalization. Also, the user seeing this message won't
> necessarily know to which value this refers. Better would be to
> provide context ("'align:' value should be..."), and even better would
> be to show the actual value at fault:
>
>     die(_("value should be greater than zero: align:%u\n",
>         ref_align_value);
>
> or something.

>
>> +                       v->s = "";
>> +                       continue;
>>                 } else
>>                         continue;
>>
>> @@ -1254,17 +1268,38 @@ static void emit(const char *cp, const char *ep)
>>         }
>>  }
>>
>> +static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)
>> +{
>> +       if (v->s[0] && ref->align_value) {
>
> Mental note: v->s[0] is not NUL ('\0').
>
> Also, in this code base, this is typically written *v->s rather than v->s[0].
>

>
>> +               if (ref->align_value > len) {
>> +                       struct strbuf buf = STRBUF_INIT;
>> +                       strbuf_addstr(&buf, v->s);
>> +                       if (!v->s[0])
>> +                               free((char *)v->s);
>
> We know from the "mental note" above that v->s[0] is not NUL ('\0'),
> so this 'if' statement can never be true, thus is dead code.

>
>> +                       strbuf_addchars(&buf, ' ', ref->align_value - len);
>> +                       v->s = strbuf_detach(&buf, NULL);
>> +               }
>> +               ref->align_value = 0;
>> +       }
>> +}

> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Add a new atom "align" and support %(align:X) where X is a number.
>> This will align the preceeding atom value to the left followed by
>> spaces for a total length of X characters. If X is less than the item
>> size, the entire atom value is printed.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 7561727..b81a08d 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -53,6 +55,7 @@ static struct {
>>         { "flag" },
>>         { "HEAD" },
>>         { "color" },
>> +       { "align" },
>
> Not a new issue, but some compilers (Solaris?) complain about the
> trailing comma.

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> Add a new atom "align" and support %(align:X) where X is a number.
>>> This will align the preceeding atom value to the left followed by
>>> spaces for a total length of X characters. If X is less than the item
>>> size, the entire atom value is printed.
>>>
>>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>>> ---
>>> diff --git a/ref-filter.c b/ref-filter.c
>>> index 7561727..b81a08d 100644
>>> --- a/ref-filter.c
>>> +++ b/ref-filter.c
>>> @@ -53,6 +55,7 @@ static struct {
>>>         { "flag" },
>>>         { "HEAD" },
>>>         { "color" },
>>> +       { "align" },
>>
>> Not a new issue, but some compilers (Solaris?) complain about the
>> trailing comma.
>
> Hmm, are you sure?  I thought we avoid trailing comma for enum
> definitions, but not a list of values like this.

> @@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)
>  			else
>  				v->s = " ";
>  			continue;
> +		} else if (starts_with(name, "align:")) {
> +			const char *valp = NULL;
> +
> +			skip_prefix(name, "align:", &valp);
> +			if (!valp[0])
> +				die(_("No value given with 'align='"));
> +			strtoul_ui(valp, 10, &ref->align_value);
> +			if (ref->align_value < 1)
> +				die(_("Value should be greater than zero"));
> +			v->s = "";
> +			continue;
>  		} else
>  			continue;
>  
> @@ -1254,17 +1268,38 @@ static void emit(const char *cp, const char *ep)
>  	}
>  }
>  
> +static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)
> +{
> +	if (v->s[0] && ref->align_value) {
> +		unsigned int len = 0;
> +
> +		len = utf8_strwidth(v->s);
> +		if (ref->align_value > len) {
> +			struct strbuf buf = STRBUF_INIT;
> +			strbuf_addstr(&buf, v->s);
> +			if (!v->s[0])
> +				free((char *)v->s);
> +			strbuf_addchars(&buf, ' ', ref->align_value - len);
> +			v->s = strbuf_detach(&buf, NULL);
> +		}
> +		ref->align_value = 0;
> +	}
> +}

>
> What is your plan for this function?  Is it envisioned that this
> will gain more variations of formatting options over time?
> Otherwise it seems misnamed (it is not "assign formatting" but
> merely "pad to the right").
>

>
> Your caller is iterating over the elements in a format string,
> e.g. 'A %(align:20)%(foo) B %(bar) C', and its caller is iterating
> over a list of refs, e.g. 'maint', 'master' branches.  With that
> format string, as long as %(foo) does not expand to something that
> exceeds 20 display places or so, I'd expect literal 'B' for all refs
> to align, but I do not think this code gives me that; what happens
> if '%(foo)' happens to be an empty string for 'maint' but is a
> string, say 'x', for 'master'?

> Add a new atom "align" and support %(align:X) where X is a number.
> This will align the preceeding atom value to the left followed by
> spaces for a total length of X characters. If X is less than the item
> size, the entire atom value is printed.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 7561727..93f59aa 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)
>                         else
>                                 v->s = " ";
>                         continue;
> +               } else if (starts_with(name, "align:")) {
> +                       const char *valp = NULL;
> +
> +                       skip_prefix(name, "align:", &valp);
> +                       if (!valp[0])
> +                               die(_("no value given with 'align:'"));
> +                       strtoul_ui(valp, 10, &ref->align_value);
> +                       if (ref->align_value < 1)
> +                               die(_("value should be greater than zero: align:%u"), ref->align_value);
> +                       v->s = "";

> +                       continue;
>                 } else
>                         continue;
>
> @@ -1254,17 +1268,40 @@ static void emit(const char *cp, const char *ep)
>         }
>  }
>
> +static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)
> +{
> +       if (ref->align_value && !starts_with(used_atom[parsed_atom], "align")) {
> +               unsigned int len = 0;
> +
> +               if (*v->s)
> +                       len = utf8_strwidth(v->s);
> +               if (ref->align_value > len) {
> +                       struct strbuf buf = STRBUF_INIT;
> +                       if (*v->s)
> +                               strbuf_addstr(&buf, v->s);
> +                       if (*v->s && v->s[0] == '\0')
> +                               free((char *)v->s);

> +                       strbuf_addchars(&buf, ' ', ref->align_value - len);
> +                       v->s = strbuf_detach(&buf, NULL);
> +               }
> +               ref->align_value = 0;
> +       }
> +}

> On Mon, Jul 20, 2015 at 1:22 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Add a new atom "align" and support %(align:X) where X is a number.
>> This will align the preceeding atom value to the left followed by
>> spaces for a total length of X characters. If X is less than the item
>> size, the entire atom value is printed.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 7561727..93f59aa 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)
>>                         else
>>                                 v->s = " ";
>>                         continue;
>> +               } else if (starts_with(name, "align:")) {
>> +                       const char *valp = NULL;
>> +
>> +                       skip_prefix(name, "align:", &valp);
>> +                       if (!valp[0])
>> +                               die(_("no value given with 'align:'"));
>> +                       strtoul_ui(valp, 10, &ref->align_value);
>> +                       if (ref->align_value < 1)
>> +                               die(_("value should be greater than zero: align:%u"), ref->align_value);
>> +                       v->s = "";
>
> Mental note: v->s points at literal zero-length string ("").
>
>> +                       continue;
>>                 } else
>>                         continue;
>>
>> @@ -1254,17 +1268,40 @@ static void emit(const char *cp, const char *ep)
>>         }
>>  }
>>
>> +static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)
>> +{
>> +       if (ref->align_value && !starts_with(used_atom[parsed_atom], "align")) {
>> +               unsigned int len = 0;
>> +
>> +               if (*v->s)
>> +                       len = utf8_strwidth(v->s);
>> +               if (ref->align_value > len) {
>> +                       struct strbuf buf = STRBUF_INIT;
>> +                       if (*v->s)
>> +                               strbuf_addstr(&buf, v->s);
>> +                       if (*v->s && v->s[0] == '\0')
>> +                               free((char *)v->s);
>
> Is the "v->s[0] == '\0'" checking for the same literal zero-length
> string assigned above? If so, attempting to free() that string doesn't
> make sense, since it's not heap-allocated. Maybe you meant != '\0'?
>
> Overall, this code is getting rather complex and difficult to follow
> (especially with all the 'v->s' checks thrown in). Junio's proposed
> 'pseudo_atom' and 'ref_formatting_state' would go a long way toward
> simplifying it.
>

> Your caller is iterating over the elements in a format string,
> e.g. 'A %(align:20)%(foo) B %(bar) C', and its caller is iterating
> over a list of refs, e.g. 'maint', 'master' branches.  With that
> format string, as long as %(foo) does not expand to something that
> exceeds 20 display places or so, I'd expect literal 'B' for all refs
> to align, but I do not think this code gives me that; what happens
> if '%(foo)' happens to be an empty string for 'maint' but is a
> string, say 'x', for 'master'?

>  	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>  		struct atom_value *atomv;
> +		int parsed_atom;
>  
>  		ep = strchr(sp, ')');
>  		if (cp < sp)
>  			emit(cp, sp);
> -		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
> +		parsed_atom = parse_ref_filter_atom(sp + 2, ep);
> +		get_ref_atom_value(info, parsed_atom, &atomv);
> +		assign_formating(info, parsed_atom, atomv);
>  		print_value(atomv, quote_style);

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Your caller is iterating over the elements in a format string,
>> e.g. 'A %(align:20)%(foo) B %(bar) C', and its caller is iterating
>> over a list of refs, e.g. 'maint', 'master' branches.  With that
>> format string, as long as %(foo) does not expand to something that
>> exceeds 20 display places or so, I'd expect literal 'B' for all refs
>> to align, but I do not think this code gives me that; what happens
>> if '%(foo)' happens to be an empty string for 'maint' but is a
>> string, say 'x', for 'master'?
>
> Having looked at the caller once again, I have to say that the
> interface to this function is poorly designed.  'info' might have
> been a convenient place to keep the "formatting state" during this
> loop (e.g. "was the previous atom tell us to format this atom in a
> special way and if so how?"), but that state does not belong to the
> 'info' thing we are getting from our caller.  It is something we'd
> want to clear before we come into the for() loop, and mutate and
> utilize while in the loop.  For example, if the caller ever wants
> to show the same ref twice by calling this function with the same
> ref twice, and if the format string ended with %(align:N), you do
> not want that leftover state to right-pad the first atom in the
> second invocation.

>
> Imagine that in the future you might want to affect how things are
> formatted based on how much we have already output for the ref so
> far (e.g. limiting the total line length).  Where would you implement
> such a feature and hook it in this loop?
>
> I'd imagine that a sensible way to organize and structure the
> codeflow to support this "align" and related enhancement we may want
> to have in the future cleanly would be to teach "print_value" about
> the "formatting state" and share it with this loop.  Roughly...
>
>>  void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
>>  {
>>       const char *cp, *sp, *ep;
>>
>
> Insert something like this here:
>
>         struct ref_formatting_state state;
>
>         memset(&state, 0, sizeof(state));
>         state.quote_style = quote_style;
>
>>       for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>>               struct atom_value *atomv;
>> +             int parsed_atom;
>>
>>               ep = strchr(sp, ')');
>>               if (cp < sp)
>>                       emit(cp, sp);
>> -             get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
>> +             parsed_atom = parse_ref_filter_atom(sp + 2, ep);
>> +             get_ref_atom_value(info, parsed_atom, &atomv);
>> +             assign_formating(info, parsed_atom, atomv);
>>               print_value(atomv, quote_style);
>
> and replace all of the above with something like this (a separate
> variable parsed_atom may not be necessary):
>
>                 get_ref_atom_value(&state, info,
>                                 parse_ref_filter_atom(sp + 2, ep), &atomv);
>                 print_value(&state, atomv);
>
> Things like %(align:20) are not really atoms in the sense that they
> are not used as placeholders for attributes that refs being printed
> have, but they are there solely in order to affect the "formating
> state".  Introduce a new field "struct atom_value.pseudo_atom" to
> tell print_value() that fact from get_ref_atom_value(), e.g.
>
>         static void print_value(struct ref_formatting_state *state,
>                                 struct atom_value *v)
>         {
>                 struct strbuf value = STRBUF_INIT;
>                 struct strbuf formatted = STRBUF_INIT;
>
>                 if (v->pseudo_atom)
>                         return;
>                 if (state->pad_to_right) {
>                         strbuf_addf(&value, "%.*s", state->pad_to_right, v->s);
>                         state->pad_to_right = 0;
>                 }
>                 switch (state->quote_style) {
>                 case QUOTE_SHELL:
>                         sq_quote_buf(&formatted, value.buf);
>                         break;
>                         ...
>                 }
>                 fputs(formatted.buf, stdout);
>                 strbuf_release(&value);
>                 strbuf_release(&formatted);
>         }
>
> or something like that.  As this print_value() knows everything that
> happens to a single output line during that loop and is allowed to
> keep track of what it sees in 'state', this would give a natural and
> codeflow to add 'limit the total line length' and things like that
> if desired.

>
> We may want to further clean up to update %(color) thing to clarify
> that it is a pseudo atom.  I suspect %(align:20)%(color:blue) would
> do a wrong thing with the current code, and it would be a reasonable
> thing to allow both of these interchangeably:
>
>   %(align:20)%(color:blue)%(refname:short)%(color:reset)
>   %(color:blue)%(align:20)%(refname:short)%(color:reset)
>
> and implementation of that would become more obvious once you have a
> more explicit "formatting state" that is known to and shared among
> get_value(), the for() loop that walks the format string, and
> print_value().

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +                     strtoul_ui(valp, 10, &ref->align_value);
>> +                     if (ref->align_value < 1)
>> +                             die(_("Value should be greater than zero"));
>
> You're not checking the return value of strtoul_ui, which returns -1
> before assigning align_value if the value can't be parsed. As a result,
> you're testing an undefined value in the 'if' statement in this case.
>
> You should test the return value and issue a distinct error message in
> this case like
>
> if (strtoul_ui(valp, 10, &ref->align_value))
>         die(_("positive integer expected after ':' in align:%u\n",
>             ref_align_value));
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

> In 'tag.c' we can print N lines from the annotation of the tag using
> the '-n<num>' option. Copy code from 'tag.c' to 'ref-filter' and
> modify 'ref-filter' to support printing of N lines from the annotation
> of tags.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 771c48d..82731ac 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1288,7 +1288,48 @@ static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct
>         }
>  }
>
> -void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
> +/* Print 'lines' no of lines of a given oid */

> diff --git a/ref-filter.h b/ref-filter.h
> index c18781b..7dfdea0 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -87,8 +88,11 @@ int parse_ref_filter_atom(const char *atom, const char *ep);
>  int verify_ref_format(const char *format);
>  /*  Sort the given ref_array as per the ref_sorting provided */
>  void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
> -/*  Print the ref using the given format and quote_style */
> -void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);
> +/*
> + * Print the ref using the given format and quote_style. If lines > 0,
> + * prints the "lines" no of lines of the objected pointed to.
> + */

>  /*  Callback function for parsing the sort option */
>  int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);
>  /*  Default sort option based on refname */
> --
> 2.4.6

> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> In 'tag.c' we can print N lines from the annotation of the tag using
>> the '-n<num>' option. Copy code from 'tag.c' to 'ref-filter' and
>> modify 'ref-filter' to support printing of N lines from the annotation
>> of tags.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 771c48d..82731ac 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1288,7 +1288,48 @@ static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct
>>         }
>>  }
>>
>> -void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
>> +/* Print 'lines' no of lines of a given oid */
>
> This is difficult to read and understand. I presume you meant "no." as
> shorthand for "number" but dropped the period. Even with the period,
> it's still hard to read. Perhaps rewrite it as:
>
>     If 'lines' is greater than 0, print that may lines of...
>
> or something.
>

>> diff --git a/ref-filter.h b/ref-filter.h
>> index c18781b..7dfdea0 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -87,8 +88,11 @@ int parse_ref_filter_atom(const char *atom, const char *ep);
>>  int verify_ref_format(const char *format);
>>  /*  Sort the given ref_array as per the ref_sorting provided */
>>  void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
>> -/*  Print the ref using the given format and quote_style */
>> -void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);
>> +/*
>> + * Print the ref using the given format and quote_style. If lines > 0,
>> + * prints the "lines" no of lines of the objected pointed to.
>> + */
>
> Same problem. This literal "no" is quite confusing. Perhaps rewrite as above:
>
>     If lines > 0, print that many lines of...

>
>> +void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style, unsigned int lines);
>
> ref-filter.h seems to be setting a precedent for overlong lines.
> Wrapping this manually could improve readability.
>

> This option is included to support sorting by versions in `git tag -l`
> which will eventaully be ported to use ref-filter APIs.
>
> Add documentation and tests for the same.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index e49d578..cc91275 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -144,6 +144,8 @@ blank line.  Finally, the optional GPG signature is `contents:signature`.
>  For sorting purposes, fields with numeric values sort in numeric
>  order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).
>  All other fields are used to sort in their byte-value order.
> +There is also an option to sort by versions, this is done using the
> +field names ('version:refname' or 'v:refname').

>  In any case, a field name that refers to a field inapplicable to
>  the object referred by the ref does not cause an error.  It
> diff --git a/ref-filter.c b/ref-filter.c
> index 82731ac..85c561e 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1169,18 +1170,22 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
>
>         get_ref_atom_value(a, s->atom, &va);
>         get_ref_atom_value(b, s->atom, &vb);
> -       switch (cmp_type) {
> -       case FIELD_STR:
> -               cmp = strcmp(va->s, vb->s);
> -               break;
> -       default:
> -               if (va->ul < vb->ul)
> -                       cmp = -1;
> -               else if (va->ul == vb->ul)
> -                       cmp = 0;
> -               else
> -                       cmp = 1;
> -               break;
> +       if (s->version)
> +               cmp = versioncmp(va->s, vb->s);
> +       else {
> +               switch (cmp_type) {
> +               case FIELD_STR:
> +                       cmp = strcmp(va->s, vb->s);
> +                       break;
> +               default:
> +                       if (va->ul < vb->ul)
> +                               cmp = -1;
> +                       else if (va->ul == vb->ul)
> +                               cmp = 0;
> +                       else
> +                               cmp = 1;
> +                       break;
> +               }

>         }
>         return (s->reverse) ? -cmp : cmp;
>  }
> diff --git a/ref-filter.h b/ref-filter.h
> index 7dfdea0..6f1646b 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -25,7 +25,7 @@ struct atom_value {
>  struct ref_sorting {
>         struct ref_sorting *next;
>         int atom; /* index into used_atom array (internal) */
> -       unsigned reverse : 1;
> +       unsigned reverse : 1, version : 1;

>  };
>
>  struct ref_array_item {
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 505a360..c31fd2f 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -81,4 +81,30 @@ test_expect_success 'filtering with --contains' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'version sort' '
> +       git tag -l --sort=version:refname | grep "foo" >actual &&
> +       cat >expect <<-\EOF &&
> +       foo1.3
> +       foo1.6
> +       foo1.10
> +       EOF
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'reverse version sort' '
> +       git tag -l --sort=-version:refname | grep "foo" >actual &&

> +       cat >expect <<-\EOF &&
> +       foo1.10
> +       foo1.6
> +       foo1.3
> +       EOF
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.4.6

>
> Assuming I'm a reader without prior knowledge, the first question
> which pops into my mind is "what's the difference between
> 'version:refname' and 'v:refname'?" Is one just shorthand for the
> other, or is there some subtle difference in behavior, or what? The
> documentation should explain this better.
>

>>  In any case, a field name that refers to a field inapplicable to
>>  the object referred by the ref does not cause an error.  It
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 82731ac..85c561e 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1169,18 +1170,22 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
>>
>>         get_ref_atom_value(a, s->atom, &va);
>>         get_ref_atom_value(b, s->atom, &vb);
>> -       switch (cmp_type) {
>> -       case FIELD_STR:
>> -               cmp = strcmp(va->s, vb->s);
>> -               break;
>> -       default:
>> -               if (va->ul < vb->ul)
>> -                       cmp = -1;
>> -               else if (va->ul == vb->ul)
>> -                       cmp = 0;
>> -               else
>> -                       cmp = 1;
>> -               break;
>> +       if (s->version)
>> +               cmp = versioncmp(va->s, vb->s);
>> +       else {
>> +               switch (cmp_type) {
>> +               case FIELD_STR:
>> +                       cmp = strcmp(va->s, vb->s);
>> +                       break;
>> +               default:
>> +                       if (va->ul < vb->ul)
>> +                               cmp = -1;
>> +                       else if (va->ul == vb->ul)
>> +                               cmp = 0;
>> +                       else
>> +                               cmp = 1;
>> +                       break;
>> +               }
>
> The logic might be slightly easier to follow, and give a much less
> noisy diff if you rewrite it like this instead:
>
>     if (s->version)
>         cmp = versioncmp(va->s, vb->s);
>     else if (cmp_type == FIELD_STR)
>         cmp = strcmp(va->s, vb->s);
>     else {
>         if (va->ul < vb->ul)
>             cmp = -1;
>         else if (va->ul == vb->ul)
>             cmp = 0;
>         else
>             cmp = 1;
>     }
>
> Or, if you don't mind a noisy diff, you can outdent the other branches, as well:
>
>     if (s->version)
>        cmp = versioncmp(va->s, vb->s);
>     else if (cmp_type == FIELD_STR)
>        cmp = strcmp(va->s, vb->s);
>     else if (va->ul < vb->ul)
>        cmp = -1;
>     else if (va->ul == vb->ul)
>        cmp = 0;
>     else
>        cmp = 1;
>
> (I rather prefer the latter, despite the noisy diff.)
>

>>         }
>>         return (s->reverse) ? -cmp : cmp;
>>  }
>> diff --git a/ref-filter.h b/ref-filter.h
>> index 7dfdea0..6f1646b 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -25,7 +25,7 @@ struct atom_value {
>>  struct ref_sorting {
>>         struct ref_sorting *next;
>>         int atom; /* index into used_atom array (internal) */
>> -       unsigned reverse : 1;
>> +       unsigned reverse : 1, version : 1;
>
> This is difficult to read. Style elsewhere (if I'm not mistaken) is to
> place the declaration on a line by itself.
>

>>  };
>>
>>  struct ref_array_item {
>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> index 505a360..c31fd2f 100755
>> --- a/t/t6302-for-each-ref-filter.sh
>> +++ b/t/t6302-for-each-ref-filter.sh
>> @@ -81,4 +81,30 @@ test_expect_success 'filtering with --contains' '
>>         test_cmp expect actual
>>  '
>>
>> +test_expect_success 'version sort' '
>> +       git tag -l --sort=version:refname | grep "foo" >actual &&
>> +       cat >expect <<-\EOF &&
>> +       foo1.3
>> +       foo1.6
>> +       foo1.10
>> +       EOF
>> +       test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'reverse version sort' '
>> +       git tag -l --sort=-version:refname | grep "foo" >actual &&
>
> Maybe use 'v:refname' in one of these tests in order to exercise that
> alias as well?
>

> Since 'ref-filter' only has an option to match path names add an
> option for plain fnmatch pattern-matching.
>
> This is to support the pattern matching options which are used in `git
> tag -l` and `git branch -l` where we can match patterns like `git tag
> -l foo*` which would match all tags which has a "foo*" pattern.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 85c561e..7ff3ded 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -966,6 +980,15 @@ static int match_name_as_path(const char **pattern, const char *refname)
>         return 0;
>  }
>
> +static int filter_pattern_match(struct ref_filter *filter, const char *refname)
> +{
> +       if (!*filter->name_patterns)
> +               return 1;
> +       if (filter->match_as_path)
> +               return match_name_as_path(filter->name_patterns, refname);
> +       return match_pattern(filter->name_patterns, refname);
> +}
> +
>  /*
>   * Given a ref (sha1, refname), check if the ref belongs to the array
>   * of sha1s. If the given ref is a tag, check if the given tag points
> @@ -1034,7 +1057,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>                 return 0;
>         }
>
> -       if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
> +       if (!filter_pattern_match(filter, refname))
>                 return 0;

> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Since 'ref-filter' only has an option to match path names add an
>> option for plain fnmatch pattern-matching.
>>
>> This is to support the pattern matching options which are used in `git
>> tag -l` and `git branch -l` where we can match patterns like `git tag
>> -l foo*` which would match all tags which has a "foo*" pattern.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 85c561e..7ff3ded 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -966,6 +980,15 @@ static int match_name_as_path(const char **pattern, const char *refname)
>>         return 0;
>>  }
>>
>> +static int filter_pattern_match(struct ref_filter *filter, const char *refname)
>> +{
>> +       if (!*filter->name_patterns)
>> +               return 1;
>> +       if (filter->match_as_path)
>> +               return match_name_as_path(filter->name_patterns, refname);
>> +       return match_pattern(filter->name_patterns, refname);
>> +}
>> +
>>  /*
>>   * Given a ref (sha1, refname), check if the ref belongs to the array
>>   * of sha1s. If the given ref is a tag, check if the given tag points
>> @@ -1034,7 +1057,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>>                 return 0;
>>         }
>>
>> -       if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
>> +       if (!filter_pattern_match(filter, refname))
>>                 return 0;
>
> I find it much more difficult to grok the new logic due to
> '*filter->name_patterns' having moved into the called function and its
> negation inside the function returning 1 which is then negated (again)
> upon return here. This sort of twisty logic places a higher cognitive
> load on the reader. Retaining the original logic makes the code far
> simpler to understand:
>
>     if (*filter->name_patterns &&
>         !filter_pattern_match(filter, refname))
>         return 0;
>
> although it's a bit less nicely encapsulated, so I dunno...

> On Mon, Jul 20, 2015 at 8:24 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> +static int filter_pattern_match(struct ref_filter *filter, const char *refname)
>>> +{
>>> +       if (!*filter->name_patterns)
>>> +               return 1;
>>> +       if (filter->match_as_path)
>>> +               return match_name_as_path(filter->name_patterns, refname);
>>> +       return match_pattern(filter->name_patterns, refname);
>>> +}
>>> @@ -1034,7 +1057,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>>> -       if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
>>> +       if (!filter_pattern_match(filter, refname))
>>>                 return 0;
>>
>> I find it much more difficult to grok the new logic due to
>> '*filter->name_patterns' having moved into the called function and its
>> negation inside the function returning 1 which is then negated (again)
>> upon return here. This sort of twisty logic places a higher cognitive
>> load on the reader. Retaining the original logic makes the code far
>> simpler to understand:
>>
>>     if (*filter->name_patterns &&
>>         !filter_pattern_match(filter, refname))
>>         return 0;
>>
>> although it's a bit less nicely encapsulated, so I dunno...
>
> I think a comment before filter_pattern_match() and perhaps also one
> inside it might help. For example something like:
>
> /* Return 1 if the refname matches one of the patterns, otherwise 0. */
> static int filter_pattern_match(struct ref_filter *filter, const char *refname)
> {
>        if (!*filter->name_patterns)
>                return 1; /* No pattern always matches */
>        if (filter->match_as_path)
>                return match_name_as_path(filter->name_patterns, refname);
>        return match_pattern(filter->name_patterns, refname);
> }

> On Mon, Jul 20, 2015 at 4:01 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> On Mon, Jul 20, 2015 at 8:24 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>> +static int filter_pattern_match(struct ref_filter *filter, const char *refname)
>>>> +{
>>>> +       if (!*filter->name_patterns)
>>>> +               return 1;
>>>> +       if (filter->match_as_path)
>>>> +               return match_name_as_path(filter->name_patterns, refname);
>>>> +       return match_pattern(filter->name_patterns, refname);
>>>> +}
>>>> @@ -1034,7 +1057,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>>>> -       if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
>>>> +       if (!filter_pattern_match(filter, refname))
>>>>                 return 0;
>>>
>>> I find it much more difficult to grok the new logic due to
>>> '*filter->name_patterns' having moved into the called function and its
>>> negation inside the function returning 1 which is then negated (again)
>>> upon return here. This sort of twisty logic places a higher cognitive
>>> load on the reader. Retaining the original logic makes the code far
>>> simpler to understand:
>>>
>>>     if (*filter->name_patterns &&
>>>         !filter_pattern_match(filter, refname))
>>>         return 0;
>>>
>>> although it's a bit less nicely encapsulated, so I dunno...
>>
>> I think a comment before filter_pattern_match() and perhaps also one
>> inside it might help. For example something like:
>>
>> /* Return 1 if the refname matches one of the patterns, otherwise 0. */
>> static int filter_pattern_match(struct ref_filter *filter, const char *refname)
>> {
>>        if (!*filter->name_patterns)
>>                return 1; /* No pattern always matches */
>>        if (filter->match_as_path)
>>                return match_name_as_path(filter->name_patterns, refname);
>>        return match_pattern(filter->name_patterns, refname);
>> }
>
> Yes, the comments do improve the situation and may be a reasonable compromise...

> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -943,9 +943,23 @@ static int commit_contains(struct ref_filter *filter, struct commit *commit)
>  
>  /*
>   * Return 1 if the refname matches one of the patterns, otherwise 0.
> + * A pattern can be a literal prefix (e.g. a refname "refs/heads/master"
> + * matches a pattern "refs/heads/m") or a wildcard (e.g. the same ref
> + * matches "refs/heads/m*", too).
> + */
> +static int match_pattern(const char **patterns, const char *refname)
> +{
> +	for (; *patterns; patterns++)
> +		if (!wildmatch(*patterns, refname, 0, NULL))
> +			return 1;
> +	return 0;
> +}
> +
> +/*
> + * Return 1 if the refname matches one of the patterns, otherwise 0.
>   * A pattern can be path prefix (e.g. a refname "refs/heads/master"
>   * matches a pattern "refs/heads/") or a wildcard (e.g. the same ref

> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -13,7 +13,8 @@ SYNOPSIS
>  	<tagname> [<commit> | <object>]
>  'git tag' -d <tagname>...
>  'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
> -	[--column[=<options>] | --no-column] [--sort=<key>] [<pattern>...]
> +	[--column[=<options>] | --no-column] [--sort=<key>] [--format=<format>]
> +	[<pattern>...]
>  'git tag' -v <tagname>...
>  
>  DESCRIPTION
> @@ -155,6 +156,19 @@ This option is only applicable when listing tags without annotation lines.
>  	The object that the new tag will refer to, usually a commit.
>  	Defaults to HEAD.
>  
> +<format>::
> +	A string that interpolates `%(fieldname)` from the
> +	object pointed at by a ref being shown.  If `fieldname`
> +	is prefixed with an asterisk (`*`) and the ref points
> +	at a tag object, the value for the field in the object
> +	tag refers is used.  When unspecified, defaults to
> +	`%(objectname) SPC %(objecttype) TAB %(refname)`.

> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1507,4 +1507,20 @@ EOF"
>  	test_cmp expect actual
>  '
>  
> +test_expect_success '--format cannot be used with -n' '
> +	test_must_fail git tag -l -n4 --format="%(refname)"
> +'
> +
> +test_expect_success '--format should list tags as per format given' '
> +	cat >expect <<-\EOF &&
> +	foo1.10
> +	foo1.3
> +	foo1.6
> +	foo1.6-rc1
> +	foo1.6-rc2
> +	EOF
> +	git tag -l --format="%(refname)" "foo*" >actual &&
> +	test_cmp expect actual
> +'

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Using 'ref-filter' APIs implement the '--merged' and '--no-merged'
> options into 'tag.c'. The '--merged' option lets the user to only
> list tags merged into the named commit. The '--no-merged' option
> lets the user to only list tags not merged into the named commit.
> If no object is provided it assumes HEAD as the object.
>
> Add documentation and tests for the same.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  Documentation/git-tag.txt | 10 +++++++++-
>  builtin/tag.c             |  6 +++++-
>  t/t7004-tag.sh            | 27 +++++++++++++++++++++++++++
>  3 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 16e396c..74ed157 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -14,7 +14,7 @@ SYNOPSIS
>  'git tag' -d <tagname>...
>  'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
>         [--column[=<options>] | --no-column] [--sort=<key>] [--format=<format>]
> -       [<pattern>...]
> +       [(--merged | --no-merged) [<commit>]] [<pattern>...]

>  'git tag' -v <tagname>...
>
>  DESCRIPTION
> @@ -169,6 +169,14 @@ This option is only applicable when listing tags without annotation lines.
>         `%09` to `\t` (TAB) and `%0a` to `\n` (LF).
>         The fields are same as those in `git for-each-ref`.
>
> +--merged [<commit>]::
> +       Only list tags whose tips are reachable from the
> +       specified commit (HEAD if not specified).
> +
> +--no-merged [<commit>]::
> +       Only list tags whose tips are not reachable from the
> +       specified commit (HEAD if not specified).

>
>  CONFIGURATION
>  -------------
> diff --git a/builtin/tag.c b/builtin/tag.c
> index cae113b..0fa1d31 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -23,7 +23,7 @@ static const char * const git_tag_usage[] = {
>         N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] <tagname> [<head>]"),
>         N_("git tag -d <tagname>..."),
>         N_("git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>]"
> -               "\n\t\t[<pattern>...]"),
> +               "\n\t\t[--merged [<commit>]] [--no-merged [<commit>]] [<pattern>...]"),

> On Sun, Jul 19, 2015 at 12:00 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Using 'ref-filter' APIs implement the '--merged' and '--no-merged'
>> options into 'tag.c'. The '--merged' option lets the user to only
>> list tags merged into the named commit. The '--no-merged' option
>> lets the user to only list tags not merged into the named commit.
>> If no object is provided it assumes HEAD as the object.
>>
>> Add documentation and tests for the same.
>>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  Documentation/git-tag.txt | 10 +++++++++-
>>  builtin/tag.c             |  6 +++++-
>>  t/t7004-tag.sh            | 27 +++++++++++++++++++++++++++
>>  3 files changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
>> index 16e396c..74ed157 100644
>> --- a/Documentation/git-tag.txt
>> +++ b/Documentation/git-tag.txt
>> @@ -14,7 +14,7 @@ SYNOPSIS
>>  'git tag' -d <tagname>...
>>  'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
>>         [--column[=<options>] | --no-column] [--sort=<key>] [--format=<format>]
>> -       [<pattern>...]
>> +       [(--merged | --no-merged) [<commit>]] [<pattern>...]
>
> Maybe [--[no-]merged [<commit>]] instead of [(--merged | --no-merged)
> [<commit>]].
>

>>  'git tag' -v <tagname>...
>>
>>  DESCRIPTION
>> @@ -169,6 +169,14 @@ This option is only applicable when listing tags without annotation lines.
>>         `%09` to `\t` (TAB) and `%0a` to `\n` (LF).
>>         The fields are same as those in `git for-each-ref`.
>>
>> +--merged [<commit>]::
>> +       Only list tags whose tips are reachable from the
>> +       specified commit (HEAD if not specified).
>> +
>> +--no-merged [<commit>]::
>> +       Only list tags whose tips are not reachable from the
>> +       specified commit (HEAD if not specified).
>
> Here also you could write something like:
>
> +--[no-]merged [<commit>]::
> +       Only list tags whose tips are reachable, or not reachable
> +       if --no-merged is used, from the specified commit
> +       (HEAD if not specified).
>
>>
>>  CONFIGURATION
>>  -------------
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index cae113b..0fa1d31 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -23,7 +23,7 @@ static const char * const git_tag_usage[] = {
>>         N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] <tagname> [<head>]"),
>>         N_("git tag -d <tagname>..."),
>>         N_("git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>]"
>> -               "\n\t\t[<pattern>...]"),
>> +               "\n\t\t[--merged [<commit>]] [--no-merged [<commit>]] [<pattern>...]"),
>
> [--[no-]merged [<commit>]] here too.
>
> Thanks,
> Christian.

> +--merged [<commit>]::
> +	Only list tags whose tips are reachable from the
> +	specified commit (HEAD if not specified).
> +
> +--no-merged [<commit>]::
> +	Only list tags whose tips are not reachable from the
> +	specified commit (HEAD if not specified).

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +--merged [<commit>]::
>> +     Only list tags whose tips are reachable from the
>> +     specified commit (HEAD if not specified).
>> +
>> +--no-merged [<commit>]::
>> +     Only list tags whose tips are not reachable from the
>> +     specified commit (HEAD if not specified).
>
> You may want to spell it `HEAD` (with backticks).
>