v13, 12 partsport tag.c to use ref-filter APIs

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +static void push_new_stack_element(struct ref_formatting_stack **stack)
>> +{
>
> Micronit.  Perhaps s/_new//;, as you do not call the other function
> pop_old_stack_element().
>
> The remainder of this step looks pretty straight-forward and was a
> pleasant read.
>
> Thanks.

> +static void perform_quote_formatting(struct strbuf *s, const char *str, int quote_style);
> +
> +static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
> +{
> +	struct ref_formatting_stack *current = state->stack;
> +	struct strbuf s = STRBUF_INIT;
> +
> +	if (!current->at_end)
> +		die(_("format: `end` atom used without a supporting atom"));
> +	current->at_end(current);
> +	/*
> +	 * Whenever we have more than one stack element that means we
> +	 * are using a certain modifier atom. In that case we need to
> +	 * perform quote formatting.
> +	 */
> +	if (!state->stack->prev->prev) {

> +		perform_quote_formatting(&s, current->output.buf, state->quote_style);
> +		strbuf_reset(&current->output);
> +		strbuf_addbuf(&current->output, &s);
> +	}
> +	strbuf_release(&s);
> +	pop_stack_element(&state->stack);
> +}
> +

> @@ -1228,29 +1315,33 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
>  	qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);
>  }
>  
> -static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
> +static void perform_quote_formatting(struct strbuf *s, const char *str, int quote_style)
>  {
> -	struct strbuf *s = &state->stack->output;
> -
> -	switch (state->quote_style) {
> +	switch (quote_style) {
>  	case QUOTE_NONE:
> -		strbuf_addstr(s, v->s);
> +		strbuf_addstr(s, str);
>  		break;
>  	case QUOTE_SHELL:
> -		sq_quote_buf(s, v->s);
> +		sq_quote_buf(s, str);
>  		break;
>  	case QUOTE_PERL:
> -		perl_quote_buf(s, v->s);
> +		perl_quote_buf(s, str);
>  		break;
>  	case QUOTE_PYTHON:
> -		python_quote_buf(s, v->s);
> +		python_quote_buf(s, str);
>  		break;
>  	case QUOTE_TCL:
> -		tcl_quote_buf(s, v->s);
> +		tcl_quote_buf(s, str);
>  		break;
>  	}
>  }
>  
> +static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
> +{
> +	struct strbuf *s = &state->stack->output;
> +	perform_quote_formatting(s, v->s, state->quote_style);

> @@ -1307,7 +1398,18 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
>  		if (cp < sp)
>  			append_literal(cp, sp, &state);
>  		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
> -		append_atom(atomv, &state);
> +		/*
> +		 * If the atom is a modifier atom, then call the handler function.
> +		 * Else, if this is the first element on the stack, then we need to
> +		 * format the atom as per the given quote. Else we just add the atom value
> +		 * to the current stack element and handle quote formatting at the end.
> +		 */
> +		if (atomv->handler)
> +			atomv->handler(atomv, &state);
> +		else if (!state.stack->prev)
> +			append_atom(atomv, &state);
> +		else
> +			strbuf_addstr(&state.stack->output, atomv->s);

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +static void end_atom_handler(struct atom_value *atomv, struct
>> ref_formatting_state *state)
>> +{
>> +	struct ref_formatting_stack *current = state->stack;
>> +	struct strbuf s = STRBUF_INIT;
>> +
>> +	if (!current->at_end)
>> +		die(_("format: `end` atom used without a supporting atom"));
>> +	current->at_end(current);
>> +	/*
>> +	 * Whenever we have more than one stack element that means we
>> +	 * are using a certain modifier atom. In that case we need to
>> +	 * perform quote formatting.
>> +	 */
>> +	if (!state->stack->prev->prev) {
>
> The comment and the condition seem to be saying opposite things.
> The code says "If the stack only has one prev that is the very
> initial one, then we do the quoting, i.e. the result of expanding
> the enclosed string in %(start-something)...%(end) is quoted only
> when that appears at the top level", which feels more correct...

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> +static void end_atom_handler(struct atom_value *atomv, struct
>>> ref_formatting_state *state)
>>> +{
>>> +    struct ref_formatting_stack *current = state->stack;
>>> +    struct strbuf s = STRBUF_INIT;
>>> +
>>> +    if (!current->at_end)
>>> +            die(_("format: `end` atom used without a supporting atom"));
>>> +    current->at_end(current);
>>> +    /*
>>> +     * Whenever we have more than one stack element that means we
>>> +     * are using a certain modifier atom. In that case we need to
>>> +     * perform quote formatting.
>>> +     */
>>> +    if (!state->stack->prev->prev) {
>>
>> The comment and the condition seem to be saying opposite things.
>> The code says "If the stack only has one prev that is the very
>> initial one, then we do the quoting, i.e. the result of expanding
>> the enclosed string in %(start-something)...%(end) is quoted only
>> when that appears at the top level", which feels more correct...
>
> As this already allows us to use nested construct, I think we would
> want to have test that uses
>
>     %(align:30,left)%(align:20,right)%(refname:short)%(end)%(end)
>
> or something like that ;-)
>
> Very nice.

> You can see that I expected that "if !state.stack->prev" check to be
> inside append_atom(), and I would imagine future readers would have
> the same expectation when reading this code.  I.e.
>
> 	append_atom(struct atom_value *v, struct ref_f_s *state)
>         {
> 		if (state->stack->prev)
> 			strbuf_addstr(&state->stack->output, v->s);
> 		else
>                 	quote_format(&state->stack->output, v->s, state->quote_style);
> 	}
>
> The end result may be the same,

> Moreover, notice that the function signature of append_atom() is
> exactly the same as atomv->handler's.  I wonder if it would be
> easier to understand if you made append_atom() the handler for a
> non-magic atoms, which would let you do the above without any if/else
> and just a single unconditional

> Junio C Hamano <gitster@pobox.com> writes:
>
>> You can see that I expected that "if !state.stack->prev" check to be
>> inside append_atom(), and I would imagine future readers would have
>> the same expectation when reading this code.  I.e.
>>
>>       append_atom(struct atom_value *v, struct ref_f_s *state)
>>         {
>>               if (state->stack->prev)
>>                       strbuf_addstr(&state->stack->output, v->s);
>>               else
>>                       quote_format(&state->stack->output, v->s, state->quote_style);
>>       }
>>
>> The end result may be the same,
>
> There's another call to append_atom() when inserting the "reset color at
> end of line if needed", so moving this if inside append_atom means we
> would do the check also for the reset color. It would not change the
> behavior (by construction, we insert it only when the stack has only the
> initial element), so it's OK.
>
> I agree that this is a good thing to do.
>
>> Moreover, notice that the function signature of append_atom() is
>> exactly the same as atomv->handler's.  I wonder if it would be
>> easier to understand if you made append_atom() the handler for a
>> non-magic atoms, which would let you do the above without any if/else
>> and just a single unconditional
>
> I can't decide between "ah, very elegant" and "no, too much magic" ;-).
> I lean towards the former.
>

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> I like the idea of using atomv->handler() a lot, mostly cause this
>> would eventually
>> help us clean up populate_atom() which currently seems like a huge dump of code.
>
> I think you already said that last time we had this discussion ;-)
>
> http://thread.gmane.org/gmane.comp.version-control.git/275537/focus=275778
>

> Junio C Hamano <gitster@pobox.com> writes:
>
>> You can see that I expected that "if !state.stack->prev" check to be
>> inside append_atom(), and I would imagine future readers would have
>> the same expectation when reading this code.  I.e.
>>
>> 	append_atom(struct atom_value *v, struct ref_f_s *state)
>>         {
>> 		if (state->stack->prev)
>> 			strbuf_addstr(&state->stack->output, v->s);
>> 		else
>>                 	quote_format(&state->stack->output, v->s, state->quote_style);
>> 	}
>>
>> The end result may be the same,
>
> There's another call to append_atom() when inserting the "reset color at
> end of line if needed",  so moving this if inside append_atom means we
> would do the check also for the reset color. It would not change the
> behavior (by construction, we insert it only when the stack has only the
> initial element), so it's OK.

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +static void perform_quote_formatting(struct strbuf *s, const char *str, int quote_style);
>> +
>> +static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
>> +{
>> +     struct ref_formatting_stack *current = state->stack;
>> +     struct strbuf s = STRBUF_INIT;
>> +
>> +     if (!current->at_end)
>> +             die(_("format: `end` atom used without a supporting atom"));
>> +     current->at_end(current);
>> +     /*
>> +      * Whenever we have more than one stack element that means we
>> +      * are using a certain modifier atom. In that case we need to
>> +      * perform quote formatting.
>> +      */
>> +     if (!state->stack->prev->prev) {
>
> The comment and the condition seem to be saying opposite things.
> The code says "If the stack only has one prev that is the very
> initial one, then we do the quoting, i.e. the result of expanding
> the enclosed string in %(start-something)...%(end) is quoted only
> when that appears at the top level", which feels more correct than
> the comment that says "if we are about to pop after seeing the
> first %(end) in %(start)...%(another)...%(end)...%(end) sequence,
> we quote what is between %(another)...%(end)".
>

>> +             perform_quote_formatting(&s, current->output.buf, state->quote_style);
>> +             strbuf_reset(&current->output);
>> +             strbuf_addbuf(&current->output, &s);
>> +     }
>> +     strbuf_release(&s);
>> +     pop_stack_element(&state->stack);
>> +}
>> +
>
>> @@ -1228,29 +1315,33 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
>>       qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);
>>  }
>>
>> -static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
>> +static void perform_quote_formatting(struct strbuf *s, const char *str, int quote_style)
>>  {
>> -     struct strbuf *s = &state->stack->output;
>> -
>> -     switch (state->quote_style) {
>> +     switch (quote_style) {
>>       case QUOTE_NONE:
>> -             strbuf_addstr(s, v->s);
>> +             strbuf_addstr(s, str);
>>               break;
>>       case QUOTE_SHELL:
>> -             sq_quote_buf(s, v->s);
>> +             sq_quote_buf(s, str);
>>               break;
>>       case QUOTE_PERL:
>> -             perl_quote_buf(s, v->s);
>> +             perl_quote_buf(s, str);
>>               break;
>>       case QUOTE_PYTHON:
>> -             python_quote_buf(s, v->s);
>> +             python_quote_buf(s, str);
>>               break;
>>       case QUOTE_TCL:
>> -             tcl_quote_buf(s, v->s);
>> +             tcl_quote_buf(s, str);
>>               break;
>>       }
>>  }
>>
>> +static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
>> +{
>> +     struct strbuf *s = &state->stack->output;
>> +     perform_quote_formatting(s, v->s, state->quote_style);
>
> Hmmm, do we want to unconditionally do the quote here, or only when
> we are not being captured by upcoming %(end) to be consistent with
> the behaviour of end_atom_handler() above?
>
>> @@ -1307,7 +1398,18 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
>>               if (cp < sp)
>>                       append_literal(cp, sp, &state);
>>               get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
>> -             append_atom(atomv, &state);
>> +             /*
>> +              * If the atom is a modifier atom, then call the handler function.
>> +              * Else, if this is the first element on the stack, then we need to
>> +              * format the atom as per the given quote. Else we just add the atom value
>> +              * to the current stack element and handle quote formatting at the end.
>> +              */
>> +             if (atomv->handler)
>> +                     atomv->handler(atomv, &state);
>> +             else if (!state.stack->prev)
>> +                     append_atom(atomv, &state);
>> +             else
>> +                     strbuf_addstr(&state.stack->output, atomv->s);
>
> Ahh, this explains why you are not doing it above, but I do not
> think if this is a good division of labor.
>
> You can see that I expected that "if !state.stack->prev" check to be
> inside append_atom(), and I would imagine future readers would have
> the same expectation when reading this code.  I.e.
>
>         append_atom(struct atom_value *v, struct ref_f_s *state)
>         {
>                 if (state->stack->prev)
>                         strbuf_addstr(&state->stack->output, v->s);
>                 else
>                         quote_format(&state->stack->output, v->s, state->quote_style);
>         }
>
> The end result may be the same, but I do think "append_atom is to
> always quote, so we do an unquoted appending by hand" is a bad way
> to do this.
>
> Moreover, notice that the function signature of append_atom() is
> exactly the same as atomv->handler's.  I wonder if it would be
> easier to understand if you made append_atom() the handler for a
> non-magic atoms, which would let you do the above without any if/else
> and just a single unconditional
>
>         atomv->handler(atomv, &state);

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Add a function called 'for_each_reftype_fullpath()' to refs.{c,h}
> which iterates through each ref for the given path without trimming
> the path and also accounting for broken refs, if mentioned.

>
> Add 'filter_ref_kind()' in ref-filter.c to check the kind of ref being
> handled and return the kind to 'ref_filter_handler()', where we
> discard refs which we do not need and assign the kind to needed refs.
>
> 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>
> ---
>  ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  ref-filter.h | 12 ++++++++++--
>  refs.c       |  9 +++++++++
>  refs.h       |  1 +
>  4 files changed, 74 insertions(+), 7 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index ffec10a..d5fae1a 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1123,6 +1123,36 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
>  	return ref;
>  }
>  
> +static int filter_ref_kind(struct ref_filter *filter, const char *refname)
> +{
> +	unsigned int i;
> +
> +	static struct {
> +		const char *prefix;
> +		unsigned int kind;
> +	} ref_kind[] = {
> +		{ "refs/heads/" , FILTER_REFS_BRANCHES },
> +		{ "refs/remotes/" , FILTER_REFS_REMOTES },
> +		{ "refs/tags/", FILTER_REFS_TAGS}
> +	};
> +
> +	if (filter->kind == FILTER_REFS_BRANCHES)
> +		return FILTER_REFS_BRANCHES;
> +	else if (filter->kind == FILTER_REFS_REMOTES)
> +		return FILTER_REFS_REMOTES;
> +	else if (filter->kind == FILTER_REFS_TAGS)
> +		return FILTER_REFS_TAGS;
> +	else if (!strcmp(refname, "HEAD"))
> +		return FILTER_REFS_DETACHED_HEAD;
> +
> +	for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
> +		if (starts_with(refname, ref_kind[i].prefix))
> +			return ref_kind[i].kind;
> +	}
> +
> +	return FILTER_REFS_OTHERS;
> +}
> +
>  /*
>   * A call-back given to for_each_ref().  Filter refs and keep them for
>   * later object processing.
> @@ -1133,6 +1163,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>  	struct ref_filter *filter = ref_cbdata->filter;
>  	struct ref_array_item *ref;
>  	struct commit *commit = NULL;
> +	unsigned int kind;
>  
>  	if (flag & REF_BAD_NAME) {
>  		warning("ignoring ref with broken name %s", refname);
> @@ -1144,6 +1175,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>  		return 0;
>  	}
>  
> +	kind = filter_ref_kind(filter, refname);
> +	if (!(kind & filter->kind))
> +		return 0;
> +

> @@ -1175,6 +1210,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>  
>  	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
>  	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
> +	ref->kind = kind;
>  	return 0;
>  }
>  
> @@ -1251,16 +1287,29 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
>  {
>  	struct ref_filter_cbdata ref_cbdata;
>  	int ret = 0;
> +	unsigned int broken = 0;
>  
>  	ref_cbdata.array = array;
>  	ref_cbdata.filter = filter;
>  
>  	/*  Simple per-ref filtering */
> -	if (type & (FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN))
> -		ret = for_each_rawref(ref_filter_handler, &ref_cbdata);
> -	else if (type & FILTER_REFS_ALL)
> -		ret = for_each_ref(ref_filter_handler, &ref_cbdata);
> -	else if (type)
> +	if (type & FILTER_REFS_INCLUDE_BROKEN) {
> +		type &= ~FILTER_REFS_INCLUDE_BROKEN;
> +		broken = 1;
> +	}
> +
> +	filter->kind = type;
> +	if (type == FILTER_REFS_BRANCHES)
> +		ret = for_each_reftype_fullpath(ref_filter_handler, "refs/heads/", broken, &ref_cbdata);
> +	else if (type == FILTER_REFS_REMOTES)
> +		ret = for_each_reftype_fullpath(ref_filter_handler, "refs/remotes/", broken, &ref_cbdata);
> +	else if (type == FILTER_REFS_TAGS)
> +		ret = for_each_reftype_fullpath(ref_filter_handler, "refs/tags/", broken, &ref_cbdata);
> +	else if (type & FILTER_REFS_ALL) {
> +		ret = for_each_reftype_fullpath(ref_filter_handler, "", broken, &ref_cbdata);
> +		if (type & FILTER_REFS_DETACHED_HEAD)
> +			head_ref(ref_filter_handler, &ref_cbdata);
> +	} else
>  		die("filter_refs: invalid type");
>  
>  	/*  Filters that need revision walking */
> diff --git a/ref-filter.h b/ref-filter.h
> index 45026d0..99f081b 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -13,8 +13,14 @@
>  #define QUOTE_PYTHON 4
>  #define QUOTE_TCL 8
>  
> -#define FILTER_REFS_INCLUDE_BROKEN 0x1
> -#define FILTER_REFS_ALL 0x2
> +#define FILTER_REFS_INCLUDE_BROKEN 0x0001
> +#define FILTER_REFS_TAGS           0x0002
> +#define FILTER_REFS_BRANCHES       0x0004
> +#define FILTER_REFS_REMOTES        0x0008
> +#define FILTER_REFS_OTHERS         0x0010
> +#define FILTER_REFS_ALL            (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
> +				    FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
> +#define FILTER_REFS_DETACHED_HEAD  0x0020
>  
>  struct atom_value;
>  
> @@ -27,6 +33,7 @@ struct ref_sorting {
>  struct ref_array_item {
>  	unsigned char objectname[20];
>  	int flag;
> +	unsigned int kind;
>  	const char *symref;
>  	struct commit *commit;
>  	struct atom_value *value;
> @@ -51,6 +58,7 @@ struct ref_filter {
>  	struct commit *merge_commit;
>  
>  	unsigned int with_commit_tag_algo : 1;
> +	unsigned int kind;
>  };
>  
>  struct ref_filter_cbdata {
> diff --git a/refs.c b/refs.c
> index 4e15f60..3266617 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2150,6 +2150,15 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data)
>  			       strlen(git_replace_ref_base), 0, cb_data);
>  }
>  
> +int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data)
> +{
> +	unsigned int flag = 0;
> +
> +	if (broken)
> +		flag = DO_FOR_EACH_INCLUDE_BROKEN;
> +	return do_for_each_ref(&ref_cache, type, fn, 0, flag, cb_data);
> +}
> +
>  int head_ref_namespaced(each_ref_fn fn, void *cb_data)
>  {
>  	struct strbuf buf = STRBUF_INIT;
> diff --git a/refs.h b/refs.h
> index e9a5f32..6e913ee 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -179,6 +179,7 @@ extern int for_each_remote_ref(each_ref_fn fn, void *cb_data);
>  extern int for_each_replace_ref(each_ref_fn fn, void *cb_data);
>  extern int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);
>  extern int for_each_glob_ref_in(each_ref_fn fn, const char *pattern, const char *prefix, void *cb_data);
> +extern int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data);
>  
>  extern int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);
>  extern int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Add a function called 'for_each_reftype_fullpath()' to refs.{c,h}
>> which iterates through each ref for the given path without trimming
>> the path and also accounting for broken refs, if mentioned.
>
> For this part, I think you would want to borrow an extra pair of
> eyes from Michael.
>
>>
>> Add 'filter_ref_kind()' in ref-filter.c to check the kind of ref being
>> handled and return the kind to 'ref_filter_handler()', where we
>> discard refs which we do not need and assign the kind to needed refs.
>>
>> 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>
>> ---
>>  ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>>  ref-filter.h | 12 ++++++++++--
>>  refs.c       |  9 +++++++++
>>  refs.h       |  1 +
>>  4 files changed, 74 insertions(+), 7 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index ffec10a..d5fae1a 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1123,6 +1123,36 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
>>       return ref;
>>  }
>>
>> +static int filter_ref_kind(struct ref_filter *filter, const char *refname)
>> +{
>> +     unsigned int i;
>> +
>> +     static struct {
>> +             const char *prefix;
>> +             unsigned int kind;
>> +     } ref_kind[] = {
>> +             { "refs/heads/" , FILTER_REFS_BRANCHES },
>> +             { "refs/remotes/" , FILTER_REFS_REMOTES },
>> +             { "refs/tags/", FILTER_REFS_TAGS}
>> +     };
>> +
>> +     if (filter->kind == FILTER_REFS_BRANCHES)
>> +             return FILTER_REFS_BRANCHES;
>> +     else if (filter->kind == FILTER_REFS_REMOTES)
>> +             return FILTER_REFS_REMOTES;
>> +     else if (filter->kind == FILTER_REFS_TAGS)
>> +             return FILTER_REFS_TAGS;
>> +     else if (!strcmp(refname, "HEAD"))
>> +             return FILTER_REFS_DETACHED_HEAD;
>> +
>> +     for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
>> +             if (starts_with(refname, ref_kind[i].prefix))
>> +                     return ref_kind[i].kind;
>> +     }
>> +
>> +     return FILTER_REFS_OTHERS;
>> +}
>> +
>>  /*
>>   * A call-back given to for_each_ref().  Filter refs and keep them for
>>   * later object processing.
>> @@ -1133,6 +1163,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>>       struct ref_filter *filter = ref_cbdata->filter;
>>       struct ref_array_item *ref;
>>       struct commit *commit = NULL;
>> +     unsigned int kind;
>>
>>       if (flag & REF_BAD_NAME) {
>>               warning("ignoring ref with broken name %s", refname);
>> @@ -1144,6 +1175,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>>               return 0;
>>       }
>>
>> +     kind = filter_ref_kind(filter, refname);
>> +     if (!(kind & filter->kind))
>> +             return 0;
>> +
>
> When filter->kind is set to some single-bit thing (e.g.
> FILTER_REFS_BRANCHES) by the caller of ref_filter_handler(), then
> this call of filter_ref_kind() will just parrot that without even
> looking at refname.  And then the if statement says "yes, they have
> common bit(s)".  Even when refname is "refs/tags/v1.0" or "HEAD".
>

>
>> @@ -1175,6 +1210,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>>
>>       REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
>>       ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
>> +     ref->kind = kind;
>>       return 0;
>>  }
>>
>> @@ -1251,16 +1287,29 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
>>  {
>>       struct ref_filter_cbdata ref_cbdata;
>>       int ret = 0;
>> +     unsigned int broken = 0;
>>
>>       ref_cbdata.array = array;
>>       ref_cbdata.filter = filter;
>>
>>       /*  Simple per-ref filtering */
>> -     if (type & (FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN))
>> -             ret = for_each_rawref(ref_filter_handler, &ref_cbdata);
>> -     else if (type & FILTER_REFS_ALL)
>> -             ret = for_each_ref(ref_filter_handler, &ref_cbdata);
>> -     else if (type)
>> +     if (type & FILTER_REFS_INCLUDE_BROKEN) {
>> +             type &= ~FILTER_REFS_INCLUDE_BROKEN;
>> +             broken = 1;
>> +     }
>> +
>> +     filter->kind = type;
>> +     if (type == FILTER_REFS_BRANCHES)
>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "refs/heads/", broken, &ref_cbdata);
>> +     else if (type == FILTER_REFS_REMOTES)
>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "refs/remotes/", broken, &ref_cbdata);
>> +     else if (type == FILTER_REFS_TAGS)
>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "refs/tags/", broken, &ref_cbdata);
>> +     else if (type & FILTER_REFS_ALL) {
>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "", broken, &ref_cbdata);
>> +             if (type & FILTER_REFS_DETACHED_HEAD)
>> +                     head_ref(ref_filter_handler, &ref_cbdata);
>> +     } else
>>               die("filter_refs: invalid type");
>>
>>       /*  Filters that need revision walking */
>> diff --git a/ref-filter.h b/ref-filter.h
>> index 45026d0..99f081b 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -13,8 +13,14 @@
>>  #define QUOTE_PYTHON 4
>>  #define QUOTE_TCL 8
>>
>> -#define FILTER_REFS_INCLUDE_BROKEN 0x1
>> -#define FILTER_REFS_ALL 0x2
>> +#define FILTER_REFS_INCLUDE_BROKEN 0x0001
>> +#define FILTER_REFS_TAGS           0x0002
>> +#define FILTER_REFS_BRANCHES       0x0004
>> +#define FILTER_REFS_REMOTES        0x0008
>> +#define FILTER_REFS_OTHERS         0x0010
>> +#define FILTER_REFS_ALL            (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
>> +                                 FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
>> +#define FILTER_REFS_DETACHED_HEAD  0x0020
>>
>>  struct atom_value;
>>
>> @@ -27,6 +33,7 @@ struct ref_sorting {
>>  struct ref_array_item {
>>       unsigned char objectname[20];
>>       int flag;
>> +     unsigned int kind;
>>       const char *symref;
>>       struct commit *commit;
>>       struct atom_value *value;
>> @@ -51,6 +58,7 @@ struct ref_filter {
>>       struct commit *merge_commit;
>>
>>       unsigned int with_commit_tag_algo : 1;
>> +     unsigned int kind;
>>  };
>>
>>  struct ref_filter_cbdata {
>> diff --git a/refs.c b/refs.c
>> index 4e15f60..3266617 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2150,6 +2150,15 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data)
>>                              strlen(git_replace_ref_base), 0, cb_data);
>>  }
>>
>> +int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data)
>> +{
>> +     unsigned int flag = 0;
>> +
>> +     if (broken)
>> +             flag = DO_FOR_EACH_INCLUDE_BROKEN;
>> +     return do_for_each_ref(&ref_cache, type, fn, 0, flag, cb_data);
>> +}
>> +
>>  int head_ref_namespaced(each_ref_fn fn, void *cb_data)
>>  {
>>       struct strbuf buf = STRBUF_INIT;
>> diff --git a/refs.h b/refs.h
>> index e9a5f32..6e913ee 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -179,6 +179,7 @@ extern int for_each_remote_ref(each_ref_fn fn, void *cb_data);
>>  extern int for_each_replace_ref(each_ref_fn fn, void *cb_data);
>>  extern int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);
>>  extern int for_each_glob_ref_in(each_ref_fn fn, const char *pattern, const char *prefix, void *cb_data);
>> +extern int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data);
>>
>>  extern int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);
>>  extern int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);

> From: Karthik Nayak <karthik.188@gmail.com>
> 
> Add a function called 'for_each_reftype_fullpath()' to refs.{c,h}
> which iterates through each ref for the given path without trimming
> the path and also accounting for broken refs, if mentioned.
> 
> Add 'filter_ref_kind()' in ref-filter.c to check the kind of ref being
> handled and return the kind to 'ref_filter_handler()', where we
> discard refs which we do not need and assign the kind to needed refs.
> 
> 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>
> ---
>  ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  ref-filter.h | 12 ++++++++++--
>  refs.c       |  9 +++++++++
>  refs.h       |  1 +
>  4 files changed, 74 insertions(+), 7 deletions(-)
> 
> diff --git a/ref-filter.c b/ref-filter.c
> index ffec10a..d5fae1a 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1123,6 +1123,36 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
>  	return ref;
>  }
>  
> +static int filter_ref_kind(struct ref_filter *filter, const char *refname)
> +{
> +	unsigned int i;
> +
> +	static struct {
> +		const char *prefix;
> +		unsigned int kind;
> +	} ref_kind[] = {
> +		{ "refs/heads/" , FILTER_REFS_BRANCHES },
> +		{ "refs/remotes/" , FILTER_REFS_REMOTES },
> +		{ "refs/tags/", FILTER_REFS_TAGS}
> +	};
> +
> +	if (filter->kind == FILTER_REFS_BRANCHES)
> +		return FILTER_REFS_BRANCHES;
> +	else if (filter->kind == FILTER_REFS_REMOTES)
> +		return FILTER_REFS_REMOTES;
> +	else if (filter->kind == FILTER_REFS_TAGS)
> +		return FILTER_REFS_TAGS;
> +	else if (!strcmp(refname, "HEAD"))
> +		return FILTER_REFS_DETACHED_HEAD;

> +
> +	for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
> +		if (starts_with(refname, ref_kind[i].prefix))
> +			return ref_kind[i].kind;
> +	}
> +
> +	return FILTER_REFS_OTHERS;
> +}
> +
>  /*
>   * A call-back given to for_each_ref().  Filter refs and keep them for
>   * later object processing.
> @@ -1133,6 +1163,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>  	struct ref_filter *filter = ref_cbdata->filter;
>  	struct ref_array_item *ref;
>  	struct commit *commit = NULL;
> +	unsigned int kind;
>  
>  	if (flag & REF_BAD_NAME) {
>  		warning("ignoring ref with broken name %s", refname);
> @@ -1144,6 +1175,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>  		return 0;
>  	}
>  
> +	kind = filter_ref_kind(filter, refname);
> +	if (!(kind & filter->kind))
> +		return 0;
> +
>  	if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
>  		return 0;
>  
> @@ -1175,6 +1210,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>  
>  	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
>  	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
> +	ref->kind = kind;
>  	return 0;
>  }
>  
> @@ -1251,16 +1287,29 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
>  {
>  	struct ref_filter_cbdata ref_cbdata;
>  	int ret = 0;
> +	unsigned int broken = 0;
>  
>  	ref_cbdata.array = array;
>  	ref_cbdata.filter = filter;
>  
>  	/*  Simple per-ref filtering */
> -	if (type & (FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN))
> -		ret = for_each_rawref(ref_filter_handler, &ref_cbdata);
> -	else if (type & FILTER_REFS_ALL)
> -		ret = for_each_ref(ref_filter_handler, &ref_cbdata);
> -	else if (type)
> +	if (type & FILTER_REFS_INCLUDE_BROKEN) {
> +		type &= ~FILTER_REFS_INCLUDE_BROKEN;
> +		broken = 1;
> +	}

> +	if (type == FILTER_REFS_BRANCHES)
> +		ret = for_each_reftype_fullpath(ref_filter_handler, "refs/heads/", broken, &ref_cbdata);
> +	else if (type == FILTER_REFS_REMOTES)
> +		ret = for_each_reftype_fullpath(ref_filter_handler, "refs/remotes/", broken, &ref_cbdata);
> +	else if (type == FILTER_REFS_TAGS)
> +		ret = for_each_reftype_fullpath(ref_filter_handler, "refs/tags/", broken, &ref_cbdata);
> +	else if (type & FILTER_REFS_ALL) {
> +		ret = for_each_reftype_fullpath(ref_filter_handler, "", broken, &ref_cbdata);
> +		if (type & FILTER_REFS_DETACHED_HEAD)
> +			head_ref(ref_filter_handler, &ref_cbdata);

> +	} else
>  		die("filter_refs: invalid type");
>  
>  	/*  Filters that need revision walking */
> diff --git a/ref-filter.h b/ref-filter.h
> index 45026d0..99f081b 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -13,8 +13,14 @@
>  #define QUOTE_PYTHON 4
>  #define QUOTE_TCL 8
>  
> -#define FILTER_REFS_INCLUDE_BROKEN 0x1
> -#define FILTER_REFS_ALL 0x2
> +#define FILTER_REFS_INCLUDE_BROKEN 0x0001
> +#define FILTER_REFS_TAGS           0x0002
> +#define FILTER_REFS_BRANCHES       0x0004
> +#define FILTER_REFS_REMOTES        0x0008
> +#define FILTER_REFS_OTHERS         0x0010
> +#define FILTER_REFS_ALL            (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
> +				    FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
> +#define FILTER_REFS_DETACHED_HEAD  0x0020

>  
>  struct atom_value;
>  
> @@ -27,6 +33,7 @@ struct ref_sorting {
>  struct ref_array_item {
>  	unsigned char objectname[20];
>  	int flag;
> +	unsigned int kind;
>  	const char *symref;
>  	struct commit *commit;
>  	struct atom_value *value;
> @@ -51,6 +58,7 @@ struct ref_filter {
>  	struct commit *merge_commit;
>  
>  	unsigned int with_commit_tag_algo : 1;
> +	unsigned int kind;
>  };
>  
>  struct ref_filter_cbdata {
> diff --git a/refs.c b/refs.c
> index 4e15f60..3266617 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2150,6 +2150,15 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data)
>  			       strlen(git_replace_ref_base), 0, cb_data);
>  }
>  
> +int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data)
> +{
> +	unsigned int flag = 0;
> +
> +	if (broken)
> +		flag = DO_FOR_EACH_INCLUDE_BROKEN;
> +	return do_for_each_ref(&ref_cache, type, fn, 0, flag, cb_data);
> +}
> +
>  int head_ref_namespaced(each_ref_fn fn, void *cb_data)
>  {
>  	struct strbuf buf = STRBUF_INIT;
> diff --git a/refs.h b/refs.h
> index e9a5f32..6e913ee 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -179,6 +179,7 @@ extern int for_each_remote_ref(each_ref_fn fn, void *cb_data);
>  extern int for_each_replace_ref(each_ref_fn fn, void *cb_data);
>  extern int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);
>  extern int for_each_glob_ref_in(each_ref_fn fn, const char *pattern, const char *prefix, void *cb_data);
> +extern int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data);

> Comments inline.
>
> On 08/22/2015 05:39 AM, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Add a function called 'for_each_reftype_fullpath()' to refs.{c,h}
>> which iterates through each ref for the given path without trimming
>> the path and also accounting for broken refs, if mentioned.
>>
>> Add 'filter_ref_kind()' in ref-filter.c to check the kind of ref being
>> handled and return the kind to 'ref_filter_handler()', where we
>> discard refs which we do not need and assign the kind to needed refs.
>>
>> 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>
>> ---
>>  ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>>  ref-filter.h | 12 ++++++++++--
>>  refs.c       |  9 +++++++++
>>  refs.h       |  1 +
>>  4 files changed, 74 insertions(+), 7 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index ffec10a..d5fae1a 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1123,6 +1123,36 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
>>       return ref;
>>  }
>>
>> +static int filter_ref_kind(struct ref_filter *filter, const char *refname)
>> +{
>> +     unsigned int i;
>> +
>> +     static struct {
>> +             const char *prefix;
>> +             unsigned int kind;
>> +     } ref_kind[] = {
>> +             { "refs/heads/" , FILTER_REFS_BRANCHES },
>> +             { "refs/remotes/" , FILTER_REFS_REMOTES },
>> +             { "refs/tags/", FILTER_REFS_TAGS}
>> +     };
>> +
>> +     if (filter->kind == FILTER_REFS_BRANCHES)
>> +             return FILTER_REFS_BRANCHES;
>> +     else if (filter->kind == FILTER_REFS_REMOTES)
>> +             return FILTER_REFS_REMOTES;
>> +     else if (filter->kind == FILTER_REFS_TAGS)
>> +             return FILTER_REFS_TAGS;
>> +     else if (!strcmp(refname, "HEAD"))
>> +             return FILTER_REFS_DETACHED_HEAD;
>
> I think this would be clearer written like so:
>
>     if (filter->kind == FILTER_REFS_BRANCHES ||
>         filter->kind == FILTER_REFS_REMOTES ||
>         filter->kind == FILTER_REFS_TAGS)
>             return filter->kind;
>     if (!strcmp(refname, "HEAD"))
>             return FILTER_REFS_DETACHED_HEAD;
>

> Or, even better, if you can set filter->kind to zero if it is not one of
> these constants, then you could simplify this to
>
>     if (filter->kind)
>             return filter->kind;
>     if (!strcmp(refname, "HEAD"))
>             return FILTER_REFS_DETACHED_HEAD;
>

>> +
>> +     for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
>> +             if (starts_with(refname, ref_kind[i].prefix))
>> +                     return ref_kind[i].kind;
>> +     }
>> +
>> +     return FILTER_REFS_OTHERS;
>> +}
>> +
>>  /*
>>   * A call-back given to for_each_ref().  Filter refs and keep them for
>>   * later object processing.
>> @@ -1133,6 +1163,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>>       struct ref_filter *filter = ref_cbdata->filter;
>>       struct ref_array_item *ref;
>>       struct commit *commit = NULL;
>> +     unsigned int kind;
>>
>>       if (flag & REF_BAD_NAME) {
>>               warning("ignoring ref with broken name %s", refname);
>> @@ -1144,6 +1175,10 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>>               return 0;
>>       }
>>
>> +     kind = filter_ref_kind(filter, refname);
>> +     if (!(kind & filter->kind))
>> +             return 0;
>> +
>>       if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
>>               return 0;
>>
>> @@ -1175,6 +1210,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>>
>>       REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
>>       ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
>> +     ref->kind = kind;
>>       return 0;
>>  }
>>
>> @@ -1251,16 +1287,29 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
>>  {
>>       struct ref_filter_cbdata ref_cbdata;
>>       int ret = 0;
>> +     unsigned int broken = 0;
>>
>>       ref_cbdata.array = array;
>>       ref_cbdata.filter = filter;
>>
>>       /*  Simple per-ref filtering */
>> -     if (type & (FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN))
>> -             ret = for_each_rawref(ref_filter_handler, &ref_cbdata);
>> -     else if (type & FILTER_REFS_ALL)
>> -             ret = for_each_ref(ref_filter_handler, &ref_cbdata);
>> -     else if (type)
>> +     if (type & FILTER_REFS_INCLUDE_BROKEN) {
>> +             type &= ~FILTER_REFS_INCLUDE_BROKEN;
>> +             broken = 1;
>> +     }
>
> I wouldn't mask out the FILTER_REFS_INCLUDE_BROKEN bit here. Instead I
> would write
>
>> +
>> +     filter->kind = type;
>
> as
>
>         filter->kind = type & FILTER_REFS_KIND_MASK;
>
> where FILTER_REFS_KIND_MASK is the OR of all of the kind bits. The
> advantage is that this approach would remain correct if more bits are
> added to type. Then in the following if statements, test filter->kind
> instead of type.
>

>> +     if (type == FILTER_REFS_BRANCHES)
>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "refs/heads/", broken, &ref_cbdata);
>> +     else if (type == FILTER_REFS_REMOTES)
>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "refs/remotes/", broken, &ref_cbdata);
>> +     else if (type == FILTER_REFS_TAGS)
>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "refs/tags/", broken, &ref_cbdata);
>> +     else if (type & FILTER_REFS_ALL) {
>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "", broken, &ref_cbdata);
>> +             if (type & FILTER_REFS_DETACHED_HEAD)
>> +                     head_ref(ref_filter_handler, &ref_cbdata);
>
> The usual promise of the for_each_ref functions is that they stop
> iterating if the function ever returns a nonzero value. So the above
> should be
>
>                 if (! ret && (type & FILTER_REFS_DETACHED_HEAD))
>                         ret = head_ref(ref_filter_handler, &ref_cbdata);
>
> Also, these functions usually iterate in lexicographic order, so I think
> you should process HEAD before the others.

>
> But there's another problem here. It seems like
> FILTER_REFS_DETACHED_HEAD is only processed if (type & FILTER_REFS_ALL)
> is nonzero. But shouldn't it be allowed to process *only* HEAD?
>
> So, finally, I think this code should look like
>
>         else if (!filter->kind)
>                 die("filter_refs: invalid type");
>         else {
>                 if (filter->kind & FILTER_REFS_DETACHED_HEAD)
>                         ret = head_ref(ref_filter_handler, &ref_cbdata);
>                 if (! ret && (filter->kind & FILTER_REFS_ALL))
>                         ret =
> for_each_reftype_fullpath(ref_filter_handler, "", broken, &ref_cbdata);
>         }
>

>> +     } else
>>               die("filter_refs: invalid type");
>>
>>       /*  Filters that need revision walking */
>> diff --git a/ref-filter.h b/ref-filter.h
>> index 45026d0..99f081b 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -13,8 +13,14 @@
>>  #define QUOTE_PYTHON 4
>>  #define QUOTE_TCL 8
>>
>> -#define FILTER_REFS_INCLUDE_BROKEN 0x1
>> -#define FILTER_REFS_ALL 0x2
>> +#define FILTER_REFS_INCLUDE_BROKEN 0x0001
>> +#define FILTER_REFS_TAGS           0x0002
>> +#define FILTER_REFS_BRANCHES       0x0004
>> +#define FILTER_REFS_REMOTES        0x0008
>> +#define FILTER_REFS_OTHERS         0x0010
>> +#define FILTER_REFS_ALL            (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
>> +                                 FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
>> +#define FILTER_REFS_DETACHED_HEAD  0x0020
>
> Do you expect it to be useful to OR these bits together, or will (in
> practice) all callers want to iterate over one or the other type of
> reference, or all references? I ask because allowing an arbitrary
> bitmask complicates the code a bit (otherwise filter_ref_kind() wouldn't
> be needed).
>
> If there is code that wants to iterate over, say, branches *and* tags,
> then maybe it is an acceptable imposition for it to make two function calls,
>
>     for_each_reftype_fullpath(fn, "refs/heads/", broken, cb_data) ||
>     for_each_reftype_fullpath(fn, "refs/tags/", broken, cb_data);
>
> which might even be a bit more efficient because it only has to iterate
> over those two namespaces rather than all references.
>
> But the more flexible bitmask code is not a lot of extra overhead, so if
> you think it will have a use case then it is fine to keep this interface.
>

>>
>>  struct atom_value;
>>
>> @@ -27,6 +33,7 @@ struct ref_sorting {
>>  struct ref_array_item {
>>       unsigned char objectname[20];
>>       int flag;
>> +     unsigned int kind;
>>       const char *symref;
>>       struct commit *commit;
>>       struct atom_value *value;
>> @@ -51,6 +58,7 @@ struct ref_filter {
>>       struct commit *merge_commit;
>>
>>       unsigned int with_commit_tag_algo : 1;
>> +     unsigned int kind;
>>  };
>>
>>  struct ref_filter_cbdata {
>> diff --git a/refs.c b/refs.c
>> index 4e15f60..3266617 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2150,6 +2150,15 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data)
>>                              strlen(git_replace_ref_base), 0, cb_data);
>>  }
>>
>> +int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data)
>> +{
>> +     unsigned int flag = 0;
>> +
>> +     if (broken)
>> +             flag = DO_FOR_EACH_INCLUDE_BROKEN;
>> +     return do_for_each_ref(&ref_cache, type, fn, 0, flag, cb_data);
>> +}
>> +
>>  int head_ref_namespaced(each_ref_fn fn, void *cb_data)
>>  {
>>       struct strbuf buf = STRBUF_INIT;
>> diff --git a/refs.h b/refs.h
>> index e9a5f32..6e913ee 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -179,6 +179,7 @@ extern int for_each_remote_ref(each_ref_fn fn, void *cb_data);
>>  extern int for_each_replace_ref(each_ref_fn fn, void *cb_data);
>>  extern int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);
>>  extern int for_each_glob_ref_in(each_ref_fn fn, const char *pattern, const char *prefix, void *cb_data);
>> +extern int for_each_reftype_fullpath(each_ref_fn fn, char *type, unsigned int broken, void *cb_data);
>
> This function is most like for_each_ref_in(prefix, fn, cb_data).
> Therefore, I suggest that you rename the "type" parameter" to "prefix",
> maybe reorder its arguments, and maybe rename it (to
> for_each_fullref_in()?) for consistency, and maybe put its declaration
> next to that function's. (I see that the argument orders among these
> functions are already pretty inconsistent, but it seems best to be
> consistent with the function that is most similar to it.)
>
> I think the "type"/"prefix" argument can be "const char *".
>

>>
>>  extern int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);
>>  extern int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);
>>
>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
>

> On Wed, Aug 26, 2015 at 9:40 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 08/22/2015 05:39 AM, Karthik Nayak wrote:
>>> [...]
>>> +     if (type == FILTER_REFS_BRANCHES)
>>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "refs/heads/", broken, &ref_cbdata);
>>> +     else if (type == FILTER_REFS_REMOTES)
>>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "refs/remotes/", broken, &ref_cbdata);
>>> +     else if (type == FILTER_REFS_TAGS)
>>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "refs/tags/", broken, &ref_cbdata);
>>> +     else if (type & FILTER_REFS_ALL) {
>>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "", broken, &ref_cbdata);
>>> +             if (type & FILTER_REFS_DETACHED_HEAD)
>>> +                     head_ref(ref_filter_handler, &ref_cbdata);
>>
>> The usual promise of the for_each_ref functions is that they stop
>> iterating if the function ever returns a nonzero value. So the above
>> should be
>>
>>                 if (! ret && (type & FILTER_REFS_DETACHED_HEAD))
>>                         ret = head_ref(ref_filter_handler, &ref_cbdata);
>>
>> Also, these functions usually iterate in lexicographic order, so I think
>> you should process HEAD before the others.
> 
> This is done on purpose, cause we need to print the HEAD ref separately
> so we print the last ref_array_item in the ref_array, free that memory and
> sort and print the rest, hence HEAD ref is attached to the last.

>> But there's another problem here. It seems like
>> FILTER_REFS_DETACHED_HEAD is only processed if (type & FILTER_REFS_ALL)
>> is nonzero. But shouldn't it be allowed to process *only* HEAD?
>>
>> So, finally, I think this code should look like
>>
>>         else if (!filter->kind)
>>                 die("filter_refs: invalid type");
>>         else {
>>                 if (filter->kind & FILTER_REFS_DETACHED_HEAD)
>>                         ret = head_ref(ref_filter_handler, &ref_cbdata);
>>                 if (! ret && (filter->kind & FILTER_REFS_ALL))
>>                         ret =
>> for_each_reftype_fullpath(ref_filter_handler, "", broken, &ref_cbdata);
>>         }
>>
> 
> So finally something like this perhaps
> 
>     if (!filter->kind)
>         die("filter_refs: invalid type");
>     else {
>         if (filter->kind == FILTER_REFS_BRANCHES)
>             ret = for_each_reftype_fullpath(ref_filter_handler,
> "refs/heads/", broken, &ref_cbdata);
>         else if (filter->kind == FILTER_REFS_REMOTES)
>             ret = for_each_reftype_fullpath(ref_filter_handler,
> "refs/remotes/", broken, &ref_cbdata);
>         else if (filter->kind == FILTER_REFS_TAGS)
>             ret = for_each_reftype_fullpath(ref_filter_handler,
> "refs/tags/", broken, &ref_cbdata);
>         else if (filter->kind & FILTER_REFS_ALL)
>             ret = for_each_reftype_fullpath(ref_filter_handler, "",
> broken, &ref_cbdata);
>         if (filter->kind & FILTER_REFS_DETACHED_HEAD)
>             head_ref(ref_filter_handler, &ref_cbdata);
>     }

> On 08/27/2015 02:42 PM, Karthik Nayak wrote:
>> On Wed, Aug 26, 2015 at 9:40 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> On 08/22/2015 05:39 AM, Karthik Nayak wrote:
>>>> [...]
>>>> +     if (type == FILTER_REFS_BRANCHES)
>>>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "refs/heads/", broken, &ref_cbdata);
>>>> +     else if (type == FILTER_REFS_REMOTES)
>>>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "refs/remotes/", broken, &ref_cbdata);
>>>> +     else if (type == FILTER_REFS_TAGS)
>>>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "refs/tags/", broken, &ref_cbdata);
>>>> +     else if (type & FILTER_REFS_ALL) {
>>>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "", broken, &ref_cbdata);
>>>> +             if (type & FILTER_REFS_DETACHED_HEAD)
>>>> +                     head_ref(ref_filter_handler, &ref_cbdata);
>>>
>>> The usual promise of the for_each_ref functions is that they stop
>>> iterating if the function ever returns a nonzero value. So the above
>>> should be
>>>
>>>                 if (! ret && (type & FILTER_REFS_DETACHED_HEAD))
>>>                         ret = head_ref(ref_filter_handler, &ref_cbdata);
>>>
>>> Also, these functions usually iterate in lexicographic order, so I think
>>> you should process HEAD before the others.
>>
>> This is done on purpose, cause we need to print the HEAD ref separately
>> so we print the last ref_array_item in the ref_array, free that memory and
>> sort and print the rest, hence HEAD ref is attached to the last.
>
> Without having looked at the other patches, this makes me wonder whether
> it makes sense to store HEAD in the ref_array at all or whether it
> should be handled separately.
>

>>> But there's another problem here. It seems like
>>> FILTER_REFS_DETACHED_HEAD is only processed if (type & FILTER_REFS_ALL)
>>> is nonzero. But shouldn't it be allowed to process *only* HEAD?
>>>
>>> So, finally, I think this code should look like
>>>
>>>         else if (!filter->kind)
>>>                 die("filter_refs: invalid type");
>>>         else {
>>>                 if (filter->kind & FILTER_REFS_DETACHED_HEAD)
>>>                         ret = head_ref(ref_filter_handler, &ref_cbdata);
>>>                 if (! ret && (filter->kind & FILTER_REFS_ALL))
>>>                         ret =
>>> for_each_reftype_fullpath(ref_filter_handler, "", broken, &ref_cbdata);
>>>         }
>>>
>>
>> So finally something like this perhaps
>>
>>     if (!filter->kind)
>>         die("filter_refs: invalid type");
>>     else {
>>         if (filter->kind == FILTER_REFS_BRANCHES)
>>             ret = for_each_reftype_fullpath(ref_filter_handler,
>> "refs/heads/", broken, &ref_cbdata);
>>         else if (filter->kind == FILTER_REFS_REMOTES)
>>             ret = for_each_reftype_fullpath(ref_filter_handler,
>> "refs/remotes/", broken, &ref_cbdata);
>>         else if (filter->kind == FILTER_REFS_TAGS)
>>             ret = for_each_reftype_fullpath(ref_filter_handler,
>> "refs/tags/", broken, &ref_cbdata);
>>         else if (filter->kind & FILTER_REFS_ALL)
>>             ret = for_each_reftype_fullpath(ref_filter_handler, "",
>> broken, &ref_cbdata);
>>         if (filter->kind & FILTER_REFS_DETACHED_HEAD)
>>             head_ref(ref_filter_handler, &ref_cbdata);
>>     }
>
> Yes, but the last test should be
>
>         if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
>
> unless you have a reason not to follow the usual convention that a
> nonzero return value from fn means that the iteration should be aborted.
>

> --- 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] [--create-reflog] [--sort=<key>] [<pattern>...]
> +	[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
> +	[--format=<format>] [<pattern>...]
>  'git tag' -v <tagname>...
>  
>  DESCRIPTION
> @@ -158,6 +159,11 @@ 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>::

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> --- 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] [--create-reflog] [--sort=<key>] [<pattern>...]
>> +     [--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
>> +     [--format=<format>] [<pattern>...]
>>  'git tag' -v <tagname>...
>>
>>  DESCRIPTION
>> @@ -158,6 +159,11 @@ 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>::
>
> Shouldn't this be --format <format>, not just <format>? We usually use
> the named argument in the SYNOPSIS for positional arguments, but not for
> arguments following an option.
>

> On Mon, Aug 24, 2015 at 1:26 AM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> --- 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] [--create-reflog] [--sort=<key>] [<pattern>...]
>>> +     [--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
>>> +     [--format=<format>] [<pattern>...]
>>>  'git tag' -v <tagname>...
>>>
>>>  DESCRIPTION
>>> @@ -158,6 +159,11 @@ 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>::
>>
>> Shouldn't this be --format <format>, not just <format>? We usually use
>> the named argument in the SYNOPSIS for positional arguments, but not for
>> arguments following an option.
>
> This is how it was in for-each-ref Documentation, hence to keep it similar I
> just put <format>.

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Mon, Aug 24, 2015 at 1:26 AM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> --- 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] [--create-reflog] [--sort=<key>] [<pattern>...]
>>>> +     [--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
>>>> +     [--format=<format>] [<pattern>...]
>>>>  'git tag' -v <tagname>...
>>>>
>>>>  DESCRIPTION
>>>> @@ -158,6 +159,11 @@ 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>::
>>>
>>> Shouldn't this be --format <format>, not just <format>? We usually use
>>> the named argument in the SYNOPSIS for positional arguments, but not for
>>> arguments following an option.
>>
>> This is how it was in for-each-ref Documentation, hence to keep it similar I
>> just put <format>.
>
> "It's wrong in another place" sounds like an argument to fix the other
> place, not to get it wrong here too ;-).
>

> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 1997657..06d468e 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -133,7 +133,8 @@ align::
>  	`<position>` is either left, right or middle and `<width>` is
>  	the total length of the content with alignment. If the
>  	contents length is more than the width then no alignment is
> -	performed.
> +	performed. If used with '--quote' everything in between %(align:..)
> +	and %(end) is quoted.

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index 1997657..06d468e 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -133,7 +133,8 @@ align::
>>       `<position>` is either left, right or middle and `<width>` is
>>       the total length of the content with alignment. If the
>>       contents length is more than the width then no alignment is
>> -     performed.
>> +     performed. If used with '--quote' everything in between %(align:..)
>> +     and %(end) is quoted.
>
> There's no --quote, there are --shell, --python, ... (well, actually, I
> would have prefered to have a single --quote=language option, but that's
> not how it is now).

>
> I had already commented on a preliminary version of this series
> off-list. I think all my previous comments have been taken into account.
>
> Thanks,
>

> On Mon, Aug 24, 2015 at 1:30 AM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>>> index 1997657..06d468e 100644
>>> --- a/Documentation/git-for-each-ref.txt
>>> +++ b/Documentation/git-for-each-ref.txt
>>> @@ -133,7 +133,8 @@ align::
>>>       `<position>` is either left, right or middle and `<width>` is
>>>       the total length of the content with alignment. If the
>>>       contents length is more than the width then no alignment is
>>> -     performed.
>>> +     performed. If used with '--quote' everything in between %(align:..)
>>> +     and %(end) is quoted.
>>
>> There's no --quote, there are --shell, --python, ... (well, actually, I
>> would have prefered to have a single --quote=language option, but that's
>> not how it is now).
>
> That'd be easy to implement, but I didn't because `git tag -l` is
> human readable and
> I didn't see the necessity for having something like `--<quote_type>` here.

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Mon, Aug 24, 2015 at 1:30 AM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>>>> index 1997657..06d468e 100644
>>>> --- a/Documentation/git-for-each-ref.txt
>>>> +++ b/Documentation/git-for-each-ref.txt
>>>> @@ -133,7 +133,8 @@ align::
>>>>       `<position>` is either left, right or middle and `<width>` is
>>>>       the total length of the content with alignment. If the
>>>>       contents length is more than the width then no alignment is
>>>> -     performed.
>>>> +     performed. If used with '--quote' everything in between %(align:..)
>>>> +     and %(end) is quoted.
>>>
>>> There's no --quote, there are --shell, --python, ... (well, actually, I
>>> would have prefered to have a single --quote=language option, but that's
>>> not how it is now).
>>
>> That'd be easy to implement, but I didn't because `git tag -l` is
>> human readable and
>> I didn't see the necessity for having something like `--<quote_type>` here.
>
> Agreed: tag does not have --shell, --python & so and does not need it.
>
> But that's not my point: you write "if used with '--quote'", and the
> option name is not --quote. It should be "if used with `--shell`,
> `--python`, ... then everything ...".
>

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index 1997657..06d468e 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -133,7 +133,8 @@ align::
>>  	`<position>` is either left, right or middle and `<width>` is
>>  	the total length of the content with alignment. If the
>>  	contents length is more than the width then no alignment is
>> -	performed.
>> +	performed. If used with '--quote' everything in between %(align:..)
>> +	and %(end) is quoted.
>
> There's no --quote, there are --shell, --python, ... (well, actually, I
> would have prefered to have a single --quote=language option, but that's
> not how it is now).
>
> I had already commented on a preliminary version of this series
> off-list. I think all my previous comments have been taken into account.

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>>> index 1997657..06d468e 100644
>>> --- a/Documentation/git-for-each-ref.txt
>>> +++ b/Documentation/git-for-each-ref.txt
>>> @@ -133,7 +133,8 @@ align::
>>>  	`<position>` is either left, right or middle and `<width>` is
>>>  	the total length of the content with alignment. If the
>>>  	contents length is more than the width then no alignment is
>>> -	performed.
>>> +	performed. If used with '--quote' everything in between %(align:..)
>>> +	and %(end) is quoted.
>>
>> There's no --quote, there are --shell, --python, ... (well, actually, I
>> would have prefered to have a single --quote=language option, but that's
>> not how it is now).
>>
>> I had already commented on a preliminary version of this series
>> off-list. I think all my previous comments have been taken into account.
>
> Thanks, both.  I think this is pretty close to being satisfactory
> ;-)  There may probably be a handful of minor nits like the above
> that need to be addressed, but I do not think I saw anything
> glaringly wrong that makes the series unsalvageable.  It was a very
> pleasant read.
>
> It's almost there, and I am very happy to see how this and other
> series evolved so far ;-)

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>>
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>>>> index 1997657..06d468e 100644
>>>> --- a/Documentation/git-for-each-ref.txt
>>>> +++ b/Documentation/git-for-each-ref.txt
>>>> @@ -133,7 +133,8 @@ align::
>>>>     `<position>` is either left, right or middle and `<width>` is
>>>>     the total length of the content with alignment. If the
>>>>     contents length is more than the width then no alignment is
>>>> -   performed.
>>>> +   performed. If used with '--quote' everything in between %(align:..)
>>>> +   and %(end) is quoted.
>>>
>>> There's no --quote, there are --shell, --python, ... (well, actually, I
>>> would have prefered to have a single --quote=language option, but that's
>>> not how it is now).
>>>
>>> I had already commented on a preliminary version of this series
>>> off-list. I think all my previous comments have been taken into account.
>>
>> Thanks, both.  I think this is pretty close to being satisfactory
>> ;-)  There may probably be a handful of minor nits like the above
>> that need to be addressed, but I do not think I saw anything
>> glaringly wrong that makes the series unsalvageable.  It was a very
>> pleasant read.
>>
>> It's almost there, and I am very happy to see how this and other
>> series evolved so far ;-)
>
> Having said all that, it seems that there is some trivial typo or
> thinko in the formatting code to break t7004.
>
> Here is what I see...
>
> ok 98 - verifying rfc1991 signature
>
> expecting success:
>         echo "rfc1991" >gpghome/gpg.conf &&
>         echo "rfc1991-signed-tag RFC1991 signed tag" >expect &&
>         git tag -l -n1 rfc1991-signed-tag >actual &&
>         test_cmp expect actual &&
>         git tag -l -n2 rfc1991-signed-tag >actual &&
>         test_cmp expect actual &&
>         git tag -l -n999 rfc1991-signed-tag >actual &&
>         test_cmp expect actual
>
> --- expect      2015-08-24 22:54:44.607272653 +0000
> +++ actual      2015-08-24 22:54:44.611272643 +0000
> @@ -1 +1 @@
> -rfc1991-signed-tag RFC1991 signed tag
> +rfc1991-signed-tagRFC1991 signed tag
> not ok 99 - list tag with rfc1991 signature
>

>> Here is what I see...
>>
>> ok 98 - verifying rfc1991 signature
>>
>> expecting success:
>>         echo "rfc1991" >gpghome/gpg.conf &&
>>         echo "rfc1991-signed-tag RFC1991 signed tag" >expect &&
>>         git tag -l -n1 rfc1991-signed-tag >actual &&
>>         test_cmp expect actual &&
>>         git tag -l -n2 rfc1991-signed-tag >actual &&
>>         test_cmp expect actual &&
>>         git tag -l -n999 rfc1991-signed-tag >actual &&
>>         test_cmp expect actual
>>
>> --- expect      2015-08-24 22:54:44.607272653 +0000
>> +++ actual      2015-08-24 22:54:44.611272643 +0000
>> @@ -1 +1 @@
>> -rfc1991-signed-tag RFC1991 signed tag
>> +rfc1991-signed-tagRFC1991 signed tag
>> not ok 99 - list tag with rfc1991 signature
>
> Thats weird, I just ran all tests, and nothing failed.

> I didn't check how wide the original is supposed to be, but perhaps
> changing builtin/tag.c this way
>
> 		if (filter->lines)
> -			format = "%(align:16,left)%(refname:short)%(end)";
> +			format = "%(align:15,left)%(refname:short)%(end) ";
> 		else
> 			format = "%(refname:short)";
> 	}
>
> may be one way to fix it.  Check the original "tag -l" output for
> tags whose names are 14, 15, 16 and 17 display-columns wide and try
> to match it.

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>> Here is what I see...
>>>
>>> ok 98 - verifying rfc1991 signature
>>>
>>> expecting success:
>>>         echo "rfc1991" >gpghome/gpg.conf &&
>>>         echo "rfc1991-signed-tag RFC1991 signed tag" >expect &&
>>>         git tag -l -n1 rfc1991-signed-tag >actual &&
>>>         test_cmp expect actual &&
>>>         git tag -l -n2 rfc1991-signed-tag >actual &&
>>>         test_cmp expect actual &&
>>>         git tag -l -n999 rfc1991-signed-tag >actual &&
>>>         test_cmp expect actual
>>>
>>> --- expect      2015-08-24 22:54:44.607272653 +0000
>>> +++ actual      2015-08-24 22:54:44.611272643 +0000
>>> @@ -1 +1 @@
>>> -rfc1991-signed-tag RFC1991 signed tag
>>> +rfc1991-signed-tagRFC1991 signed tag
>>> not ok 99 - list tag with rfc1991 signature
>>
>> Thats weird, I just ran all tests, and nothing failed.
>
> You may be missing GPG or RFC1991 prerequisites and not running this
> particular test, which could be why you missed it.
>

> Your builtin/tag.c calls show_ref_array_item() with
>
>         "%(align:16,left)%(refname:short)%(end)"
>
> as the format, and "rfc1991-signed-tag" is longer than 16.
>
> And immediately after showing that, there is this hack at the end of
> show_ref_array_item() function, which I _think_ should not be there
> in ref-filter.c:show_ref_array_item() in the first place.
>
>         if (lines > 0) {
>                 struct object_id oid;
>                 hashcpy(oid.hash, info->objectname);
>                 show_tag_lines(&oid, lines);
>         }
>
> This belongs to the caller who knows that it is dealing with a tag.
>

> I didn't check how wide the original is supposed to be, but perhaps
> changing builtin/tag.c this way
>
>                 if (filter->lines)
> -                       format = "%(align:16,left)%(refname:short)%(end)";
> +                       format = "%(align:15,left)%(refname:short)%(end) ";
>                 else
>                         format = "%(refname:short)";
>         }
>
> may be one way to fix it.  Check the original "tag -l" output for
> tags whose names are 14, 15, 16 and 17 display-columns wide and try
> to match it.
>

> And then move the tag-specific code at the end of
> show_ref_array_item() to here:
>
>         verify_ref_format(format);
>         filter_refs(&array, filter, FILTER_REFS_TAGS);
>         ref_array_sort(sorting, &array);
>
> -       for (i = 0; i < array.nr; i++)
> +       for (i = 0; i < array.nr; i++) {
>                 show_ref_array_item(array.items[i], format, QUOTE_NONE, filter->lines);
> +               if (lines) {
> +                       struct object_id oid;
> +                       hashcpy(oid.hash, info->objectname);
> +                       show_tag_lines(&oid, lines);
> +               }
> +       }
>
> perhaps.

> Heh, I did it myself.  %(align:15) with trailing whitespace is what
> you want.
>
> An alternative way to spell it would be
>
>     "%(align:16,left)%(refname:short) %(end)"
>
> I don't know which one is more readable, though.

> On Wed, Aug 26, 2015 at 12:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>> I didn't check how wide the original is supposed to be, but perhaps
>> changing builtin/tag.c this way
>>
>>                 if (filter->lines)
>> -                       format = "%(align:16,left)%(refname:short)%(end)";
>> +                       format = "%(align:15,left)%(refname:short)%(end) ";
>>                 else
>>                         format = "%(refname:short)";
>>         }
>>
>> may be one way to fix it.  Check the original "tag -l" output for
>> tags whose names are 14, 15, 16 and 17 display-columns wide and try
>> to match it.
>
> That should be the fix, since it's a space problem.
> ....
> The problem with doing this is, the lines need to be displayed
> immediately after  the refname,
> followed by a newline, what you're suggesting breaks that flow.

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Wed, Aug 26, 2015 at 12:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>> I didn't check how wide the original is supposed to be, but perhaps
>>> changing builtin/tag.c this way
>>>
>>>                 if (filter->lines)
>>> -                       format = "%(align:16,left)%(refname:short)%(end)";
>>> +                       format = "%(align:15,left)%(refname:short)%(end) ";
>>>                 else
>>>                         format = "%(refname:short)";
>>>         }
>>>
>>> may be one way to fix it.  Check the original "tag -l" output for
>>> tags whose names are 14, 15, 16 and 17 display-columns wide and try
>>> to match it.
>>
>> That should be the fix, since it's a space problem.
>> ....
>> The problem with doing this is, the lines need to be displayed
>> immediately after  the refname,
>> followed by a newline, what you're suggesting breaks that flow.
>
> That is only because show_ref_array_item() does too much.  The
> function is given a placeholder string and a set of data to fill the
> placeholder with for an item, and the reason why the caller
> repeatedly calls it, once per item it has, is because it wants to
> produce a one-line-per-item output.  An alternative valid way to
> structure the API is to have it format into a string and leave the
> printing to the caller.  You can give a new format_ref_array_item()
> that does not print but fills a strbuf to this caller, make
> show_ref_array_item() a thin wrapper that calls it and prints it
> with the final LF for other callers.
>
> Another alternate approach, if you want to give "tag -l" annotation
> available to for-each-ref, would be to do this:
>
>        if (filter->lines)
>                format = xstrfmt("%%(align:15,left)%%(refname:short)%%(end) "
>                                 "%%(contents:lines=%s)", filter->lines);
>        else
>                format = "%(refname:short)";
>
> and teach a new %(contents:lines=1) atom.  That way, you can lose
> the ugly special case call to show_tag_lines() that can only be at
> the end of the output.  I somehow think this is a better approach.
>

> On Wed, Aug 26, 2015 at 8:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> ...  You can give a new format_ref_array_item()
>> that does not print but fills a strbuf to this caller, make
>> show_ref_array_item() a thin wrapper that calls it and prints it
>> with the final LF for other callers.
>>
> You're saying remove show_ref_array_item() (even the wrapper you mentioned
> above) and just have something like format_ref_array_item() which
> would output to a strbuf. and let the caller worry about the printing?

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Wed, Aug 26, 2015 at 8:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> ...  You can give a new format_ref_array_item()
>>> that does not print but fills a strbuf to this caller, make
>>> show_ref_array_item() a thin wrapper that calls it and prints it
>>> with the final LF for other callers.
>>>
>> You're saying remove show_ref_array_item() (even the wrapper you mentioned
>> above) and just have something like format_ref_array_item() which
>> would output to a strbuf. and let the caller worry about the printing?
>
> Among the current callers, the one in builtin/tag.c that wants to
> trigger show_tag_lines() hack embedded in show_ref_array_item()
> function can stop calling show_ref_array_item() and instead can do
>
>         for (i = 0; i < array.nr; i++) {
>                 struct strbuf out = STRBUF_INIT;
>                 format_ref_array_item(&out, ...);
>                 if (filter->lines) {
>                         ... append tag lines to out ...
>                 }
>                 printf("%s\n", out.buf);
>                 strbuf_reset(&out);
>         }
>
> The current and future callers of show_ref_array_item() that do not
> want to trigger the show_tag_liens() hack embedded in there may
> still want it to print the formatted string including the trailing
> LF, so you can keep show_ref_array_item() as a thin wrapper around
> format_ref_array_item() for them to call, e.g.
>
>         show_ref_array_item(...) {
>                 struct strbuf out = STRBUF_INIT;
>                 format_ref_array_item(&out, ...);
>                 printf("%s\n", out.buf);
>                 strbuf_release(&out);
>         }
>
> But if it has only one caller each, you may not even want to have
> show_ref_array_item(), if you are going to do the "output to strbuf"
> variant.
>
>
>

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>>> index 1997657..06d468e 100644
>>> --- a/Documentation/git-for-each-ref.txt
>>> +++ b/Documentation/git-for-each-ref.txt
>>> @@ -133,7 +133,8 @@ align::
>>>      `<position>` is either left, right or middle and `<width>` is
>>>      the total length of the content with alignment. If the
>>>      contents length is more than the width then no alignment is
>>> -    performed.
>>> +    performed. If used with '--quote' everything in between %(align:..)
>>> +    and %(end) is quoted.
>>
>> There's no --quote, there are --shell, --python, ... (well, actually, I
>> would have prefered to have a single --quote=language option, but that's
>> not how it is now).
>>
>> I had already commented on a preliminary version of this series
>> off-list. I think all my previous comments have been taken into account.
>
> Thanks, both.  I think this is pretty close to being satisfactory
> ;-)  There may probably be a handful of minor nits like the above
> that need to be addressed, but I do not think I saw anything
> glaringly wrong that makes the series unsalvageable.  It was a very
> pleasant read.
>
> It's almost there, and I am very happy to see how this and other
> series evolved so far ;-)

> Interdiff:
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 1997657..06d468e 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -133,7 +133,8 @@ align::
>  	`<position>` is either left, right or middle and `<width>` is
>  	the total length of the content with alignment. If the
>  	contents length is more than the width then no alignment is
> -	performed.
> +	performed. If used with '--quote' everything in between %(align:..)
> +	and %(end) is quoted.

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Interdiff:
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index 1997657..06d468e 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -133,7 +133,8 @@ align::
>>       `<position>` is either left, right or middle and `<width>` is
>>       the total length of the content with alignment. If the
>>       contents length is more than the width then no alignment is
>> -     performed.
>> +     performed. If used with '--quote' everything in between %(align:..)
>> +     and %(end) is quoted.
>
> That sounds like a buggy behaviour that we may want to fix later,
> though.  Perhaps document it as a known bug, e.g. "Currently it does
> not work well when used with language-specific quoting like --shell,
> etc." (while punting on fixing the implementation for now)?
>

> On Mon, Aug 24, 2015 at 10:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>> ...
>>> +     performed. If used with '--quote' everything in between %(align:..)
>>> +     and %(end) is quoted.
> ...
> I might have misunderstood you, But based on the discussion held here
> (thread.gmane.org/gmane.comp.version-control.git/276140)
> I thought we wanted everything inside the %(align) .... %(end) atoms
> to be quoted.

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Mon, Aug 24, 2015 at 10:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>> ...
>>>> +     performed. If used with '--quote' everything in between %(align:..)
>>>> +     and %(end) is quoted.
>> ...
>> I might have misunderstood you, But based on the discussion held here
>> (thread.gmane.org/gmane.comp.version-control.git/276140)
>> I thought we wanted everything inside the %(align) .... %(end) atoms
>> to be quoted.
>
> Perhaps I misunderstood your intention in the doc.
> 	
> I took "everything in between %(align:...) and %(end) is quoted" to
> mean that
>
> 	%(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)
>
> can never satisfy %(if:empty), because %(align)%(end) would expand
> to a string that has two single-quotes, that is not an empty string.
>
> If that is not what would happen in the "branch --list" enhancement,
> then the proposed behaviour is good, but the above documentation would
> need to be updated when it happens, I think.  It at least is misleading.

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> On Mon, Aug 24, 2015 at 10:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>> ...
>>>>> +     performed. If used with '--quote' everything in between %(align:..)
>>>>> +     and %(end) is quoted.
>>> ...
>>> I might have misunderstood you, But based on the discussion held here
>>> (thread.gmane.org/gmane.comp.version-control.git/276140)
>>> I thought we wanted everything inside the %(align) .... %(end) atoms
>>> to be quoted.
>>
>> Perhaps I misunderstood your intention in the doc.
>>
>> I took "everything in between %(align:...) and %(end) is quoted" to
>> mean that
>>
>>       %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)
>>
>> can never satisfy %(if:empty), because %(align)%(end) would expand
>> to a string that has two single-quotes, that is not an empty string.
>>
>> If that is not what would happen in the "branch --list" enhancement,
>> then the proposed behaviour is good, but the above documentation would
>> need to be updated when it happens, I think.  It at least is misleading.
>
> OK, now I checked the code, and I _think_ the recursive logic is
> doing the right thing (modulo minor nits on comment-vs-code
> discrepancy and code structure I sent separately).
>

> On Tue, Aug 25, 2015 at 4:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> On Mon, Aug 24, 2015 at 10:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>>> ...
>>>>>> +     performed. If used with '--quote' everything in between %(align:..)
>>>>>> +     and %(end) is quoted.
>>>> ...
>>>> I might have misunderstood you, But based on the discussion held here
>>>> (thread.gmane.org/gmane.comp.version-control.git/276140)
>>>> I thought we wanted everything inside the %(align) .... %(end) atoms
>>>> to be quoted.
>>>
>>> Perhaps I misunderstood your intention in the doc.
>>>
>>> I took "everything in between %(align:...) and %(end) is quoted" to
>>> mean that
>>>
>>>       %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)
>>>
>>> can never satisfy %(if:empty), because %(align)%(end) would expand
>>> to a string that has two single-quotes, that is not an empty string.
>>>
>>> If that is not what would happen in the "branch --list" enhancement,
>>> then the proposed behaviour is good, but the above documentation would
>>> need to be updated when it happens, I think.  It at least is misleading.
>>
>> OK, now I checked the code, and I _think_ the recursive logic is
>> doing the right thing (modulo minor nits on comment-vs-code
>> discrepancy and code structure I sent separately).
>>
>
> For the current code %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)
> would print non-empty, I guess the documentation holds in that case.
> Not sure if we require it to print non-empty.

>> For the current code %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)
>> would print non-empty, I guess the documentation holds in that case.
>> Not sure if we require it to print non-empty.
>
> You don't want the %(if) condition to depend on whether
> --shell/--python/... is used. Since %(if:empty)%(align)%(end)%(then)
> holds when you don't use --shell, you also want it to hold when you
> quote. IOW, you should check for emptyness before (or actually without)
> doing the quoting. I guess this is what you're doing, and if so, I think
> it's "The Right Thing".

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>>> For the current code %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)
>>> would print non-empty, I guess the documentation holds in that case.
>>> Not sure if we require it to print non-empty.
>>
>> You don't want the %(if) condition to depend on whether
>> --shell/--python/... is used. Since %(if:empty)%(align)%(end)%(then)
>> holds when you don't use --shell, you also want it to hold when you
>> quote. IOW, you should check for emptyness before (or actually without)
>> doing the quoting. I guess this is what you're doing, and if so, I think
>> it's "The Right Thing".
>
> I agree that %(align)%(end) should expand to empty and %(if:empty)...%(then)
> should look at that empty string without quoting.  So 
>
>     %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)
>
> should give "Empty"; otherwise the code is buggy, I think.

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>>
>>>> For the current code %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)
>>>> would print non-empty, I guess the documentation holds in that case.
>>>> Not sure if we require it to print non-empty.
>>>
>>> You don't want the %(if) condition to depend on whether
>>> --shell/--python/... is used. Since %(if:empty)%(align)%(end)%(then)
>>> holds when you don't use --shell, you also want it to hold when you
>>> quote. IOW, you should check for emptyness before (or actually without)
>>> doing the quoting. I guess this is what you're doing, and if so, I think
>>> it's "The Right Thing".
>>
>> I agree that %(align)%(end) should expand to empty and %(if:empty)...%(then)
>> should look at that empty string without quoting.  So 
>>
>>     %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)
>>
>> should give "Empty"; otherwise the code is buggy, I think.
>
> (I shouldn't be typing while eating...)
>
> It should give "Empty", but the --shell/--python/... may make the
> whole "Empty", as the result of %(if:...)...%(end), be quoted.  So
> you may see "'Empty'" in the output.

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>>>
>>>>> For the current code %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)
>>>>> would print non-empty, I guess the documentation holds in that case.
>>>>> Not sure if we require it to print non-empty.
>>>>
>>>> You don't want the %(if) condition to depend on whether
>>>> --shell/--python/... is used. Since %(if:empty)%(align)%(end)%(then)
>>>> holds when you don't use --shell, you also want it to hold when you
>>>> quote. IOW, you should check for emptyness before (or actually without)
>>>> doing the quoting. I guess this is what you're doing, and if so, I think
>>>> it's "The Right Thing".
>>>
>>> I agree that %(align)%(end) should expand to empty and %(if:empty)...%(then)
>>> should look at that empty string without quoting.  So
>>>
>>>     %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)
>>>
>>> should give "Empty"; otherwise the code is buggy, I think.
>>
>> (I shouldn't be typing while eating...)
>>
>> It should give "Empty", but the --shell/--python/... may make the
>> whole "Empty", as the result of %(if:...)...%(end), be quoted.  So
>> you may see "'Empty'" in the output.
>
> Agreed (with both points).
>

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Mon, Aug 24, 2015 at 10:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>> ...
>>>> +     performed. If used with '--quote' everything in between %(align:..)
>>>> +     and %(end) is quoted.
>> ...
>> I might have misunderstood you, But based on the discussion held here
>> (thread.gmane.org/gmane.comp.version-control.git/276140)
>> I thought we wanted everything inside the %(align) .... %(end) atoms
>> to be quoted.
>
> Perhaps I misunderstood your intention in the doc.
>
> I took "everything in between %(align:...) and %(end) is quoted" to
> mean that
>
>         %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end)
>
> can never satisfy %(if:empty), because %(align)%(end) would expand
> to a string that has two single-quotes, that is not an empty string.
>
> If that is not what would happen in the "branch --list" enhancement,
> then the proposed behaviour is good, but the above documentation would
> need to be updated when it happens, I think.  It at least is misleading.
>
> Thanks.