[PATCH 0/10] Port branch.c to ref-filter.

[PATCH 0/10] Port branch.c to ref-filter.

[Karthik Nayak]

This is part of my GSoC project to unify git tag -l, git branch -l,
git for-each-ref.  This patch series is continued from: Git (next)
https://github.com/git/git/commit/bf5418f49ff0cebc6e5ce04ad1417e1a47c81b61

This series consists of porting branch.c over to using the ref-filter
APIs. This does not involve the usage of show_ref_array_item() as it
has its own changes to be made, and on suggestion of my mentors I have
decided to split the porting of branch.c to this and eventually
implementation of the "--format" option.

The RFC version can be found here :
article.gmane.org/gmane.comp.version-control.git/274737

This is a follow up to the port of tag.c to use ref-filter APIs.
(currently in the 9th iteration)

Changes :
* Change the order of ref_kind[] structure in filter_branch_kind.
* Commit message changes.
* Comments added if required.
* Small code changes.

No interdiff as this was a split version and might as well be treated
as the start of the patch series.

-- 
Regards,
Karthik Nayak

[PATCH 01/10] ref-filter: add option to filter only branches

[Karthik Nayak]

From: Karthik Nayak <karthik.188@gmail.com>

Add an option in 'filter_refs()' to use 'for_each_branch_ref()'
and filter refs. This type checking is done by adding a
'FILTER_REFS_BRANCHES' in 'ref-filter.h'.

Add an option in 'ref_filter_handler()' to filter different
types of branches by calling 'filter_branch_kind()' which
checks for the type of branch needed.

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 | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 ref-filter.h | 10 ++++++++--
 2 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index de84dd4..c573109 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1044,6 +1044,46 @@ static const unsigned char *match_points_at(struct sha1_array *points_at,
 	return NULL;
 }
 
+/*
+ * Checks if a given refname is a branch and returns the kind of
+ * branch it is. If not a branch, 0 is returned.
+ */
+static int filter_branch_kind(struct ref_filter *filter, const char *refname)
+{
+	int kind, i;
+
+	static struct {
+		const char *prefix;
+		int kind;
+	} ref_kind[] = {
+		{ "refs/heads/" , REF_LOCAL_BRANCH },
+		{ "refs/remotes/" , REF_REMOTE_BRANCH },
+	};
+
+	/*  If no kind is specified, no need to filter */
+	if (!filter->branch_kind)
+		return REF_NO_BRANCH_FILTERING;
+
+	for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
+		if (starts_with(refname, ref_kind[i].prefix)) {
+			kind = ref_kind[i].kind;
+			break;
+		}
+	}
+
+	if (ARRAY_SIZE(ref_kind) <= i) {
+		if (!strcmp(refname, "HEAD"))
+			kind = REF_DETACHED_HEAD;
+		else
+			return 0;
+	}
+
+	if ((filter->branch_kind & kind) == 0)
+		return 0;
+
+	return kind;
+}
+
 /* Allocate space for a new ref_array_item and copy the objectname and flag to it */
 static struct ref_array_item *new_ref_array_item(const char *refname,
 						 const unsigned char *objectname,
@@ -1069,6 +1109,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);
@@ -1080,6 +1121,9 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 		return 0;
 	}
 
+	if (!(kind = filter_branch_kind(filter, refname)))
+		return 0;
+
 	if (!filter_pattern_match(filter, refname))
 		return 0;
 
@@ -1108,6 +1152,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 */
 	ref = new_ref_array_item(refname, oid->hash, flag);
 	ref->commit = commit;
+	ref->kind = kind;
 
 	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
 	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
@@ -1198,6 +1243,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 		ret = for_each_ref(ref_filter_handler, &ref_cbdata);
 	else if (type & FILTER_REFS_TAGS)
 		ret = for_each_tag_ref_fullpath(ref_filter_handler, &ref_cbdata);
+	else if (type & FILTER_REFS_BRANCHES)
+		ret = for_each_rawref(ref_filter_handler, &ref_cbdata);
 	else if (type)
 		die("filter_refs: invalid type");
 
diff --git a/ref-filter.h b/ref-filter.h
index 5be3e35..b5a13e8 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -16,6 +16,12 @@
 #define FILTER_REFS_INCLUDE_BROKEN 0x1
 #define FILTER_REFS_ALL 0x2
 #define FILTER_REFS_TAGS 0x4
+#define FILTER_REFS_BRANCHES 0x8
+
+#define REF_DETACHED_HEAD   0x01
+#define REF_LOCAL_BRANCH    0x02
+#define REF_REMOTE_BRANCH   0x04
+#define REF_NO_BRANCH_FILTERING 0x08
 
 #define ALIGN_LEFT 0x01
 #define ALIGN_RIGHT 0x02
@@ -50,7 +56,7 @@ struct ref_sorting {
 
 struct ref_array_item {
 	unsigned char objectname[20];
-	int flag;
+	int flag, kind;
 	const char *symref;
 	struct commit *commit;
 	struct atom_value *value;
@@ -76,7 +82,7 @@ struct ref_filter {
 
 	unsigned int with_commit_tag_algo : 1,
 		match_as_path : 1;
-	unsigned int lines;
+	unsigned int lines, branch_kind;
 };
 
 struct ref_filter_cbdata {
-- 
2.5.0

Re: [PATCH 01/10] ref-filter: add option to filter only branches

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Add an option in 'filter_refs()' to use 'for_each_branch_ref()'
> and filter refs. This type checking is done by adding a
> 'FILTER_REFS_BRANCHES' in 'ref-filter.h'.
>
> Add an option in 'ref_filter_handler()' to filter different
> types of branches by calling 'filter_branch_kind()' which
> checks for the type of branch needed.
>
> 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 | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  ref-filter.h | 10 ++++++++--
>  2 files changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index de84dd4..c573109 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1044,6 +1044,46 @@ static const unsigned char *match_points_at(struct sha1_array *points_at,
>  	return NULL;
>  }
>  
> +/*
> + * Checks if a given refname is a branch and returns the kind of
> + * branch it is. If not a branch, 0 is returned.
> + */
> +static int filter_branch_kind(struct ref_filter *filter, const char *refname)
> +{
> +	int kind, i;
> +
> +	static struct {
> +		const char *prefix;
> +		int kind;

Make a mental note that this is signed int.

> +	} ref_kind[] = {
> +		{ "refs/heads/" , REF_LOCAL_BRANCH },
> +		{ "refs/remotes/" , REF_REMOTE_BRANCH },
> +	};
> +
> +	/*  If no kind is specified, no need to filter */
> +	if (!filter->branch_kind)
> +		return REF_NO_BRANCH_FILTERING;
> +
> +	for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
> +		if (starts_with(refname, ref_kind[i].prefix)) {
> +			kind = ref_kind[i].kind;
> +			break;
> +		}
> +	}
> +
> +	if (ARRAY_SIZE(ref_kind) <= i) {
> +		if (!strcmp(refname, "HEAD"))
> +			kind = REF_DETACHED_HEAD;
> +		else
> +			return 0;
> +	}
> +
> +	if ((filter->branch_kind & kind) == 0)
> +		return 0;
> +
> +	return kind;
> +}

While this looks fine, I am not sure if this is a good interface for
filtering, though.

If you start from already having everything and want to limit things
down to "refs/heads/", this might make sense but it would be far
more efficient, if you know that you want to limit to "refs/heads/"
upfront, not to collect everything but just limit the collection to
those under "refs/heads/" without wasting cycles in the first place.

> diff --git a/ref-filter.h b/ref-filter.h
> index 5be3e35..b5a13e8 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -16,6 +16,12 @@
>  #define FILTER_REFS_INCLUDE_BROKEN 0x1
>  #define FILTER_REFS_ALL 0x2
>  #define FILTER_REFS_TAGS 0x4
> +#define FILTER_REFS_BRANCHES 0x8

Is this a sensible set of bits?  Does it make sense to have ALL and
TAGS at the same time (and what does that mean?)?

> +#define REF_DETACHED_HEAD   0x01
> +#define REF_LOCAL_BRANCH    0x02
> +#define REF_REMOTE_BRANCH   0x04
> +#define REF_NO_BRANCH_FILTERING 0x08

Where do these values go?  It is a returned by filter-branch-kind
for each ref, i.e. given "refs/heads/bar", it answers "Yeah, that is
a local branch".  Why are these values pretending to be a set of
bits that can be combined together, as if a branch can be both LOCAL
and REMOTE?  This does not make _any_ sense.


>  #define ALIGN_LEFT 0x01
>  #define ALIGN_RIGHT 0x02
> @@ -50,7 +56,7 @@ struct ref_sorting {
>  
>  struct ref_array_item {
>  	unsigned char objectname[20];
> -	int flag;
> +	int flag, kind;

For readability, do not define multiple structure fields on a single
line.

If you are storing a set of bits in an integer, use unsigned.  If it
is an enumeration, int is fine.

>  	const char *symref;
>  	struct commit *commit;
>  	struct atom_value *value;
> @@ -76,7 +82,7 @@ struct ref_filter {
>  
>  	unsigned int with_commit_tag_algo : 1,
>  		match_as_path : 1;
> -	unsigned int lines;
> +	unsigned int lines, branch_kind;

For readability, do not define multiple structure fields on a single
line.

>  };
>  
>  struct ref_filter_cbdata {

Re: [PATCH 01/10] ref-filter: add option to filter only branches

[Karthik Nayak]

On Tue, Aug 11, 2015 at 11:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Add an option in 'filter_refs()' to use 'for_each_branch_ref()'
>> and filter refs. This type checking is done by adding a
>> 'FILTER_REFS_BRANCHES' in 'ref-filter.h'.
>>
>> Add an option in 'ref_filter_handler()' to filter different
>> types of branches by calling 'filter_branch_kind()' which
>> checks for the type of branch needed.
>>
>> 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 | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>>  ref-filter.h | 10 ++++++++--
>>  2 files changed, 55 insertions(+), 2 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index de84dd4..c573109 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1044,6 +1044,46 @@ static const unsigned char *match_points_at(struct sha1_array *points_at,
>>       return NULL;
>>  }
>>
>> +/*
>> + * Checks if a given refname is a branch and returns the kind of
>> + * branch it is. If not a branch, 0 is returned.
>> + */
>> +static int filter_branch_kind(struct ref_filter *filter, const char *refname)
>> +{
>> +     int kind, i;
>> +
>> +     static struct {
>> +             const char *prefix;
>> +             int kind;
>
> Make a mental note that this is signed int.
>
>> +     } ref_kind[] = {
>> +             { "refs/heads/" , REF_LOCAL_BRANCH },
>> +             { "refs/remotes/" , REF_REMOTE_BRANCH },
>> +     };
>> +
>> +     /*  If no kind is specified, no need to filter */
>> +     if (!filter->branch_kind)
>> +             return REF_NO_BRANCH_FILTERING;
>> +
>> +     for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
>> +             if (starts_with(refname, ref_kind[i].prefix)) {
>> +                     kind = ref_kind[i].kind;
>> +                     break;
>> +             }
>> +     }
>> +
>> +     if (ARRAY_SIZE(ref_kind) <= i) {
>> +             if (!strcmp(refname, "HEAD"))
>> +                     kind = REF_DETACHED_HEAD;
>> +             else
>> +                     return 0;
>> +     }
>> +
>> +     if ((filter->branch_kind & kind) == 0)
>> +             return 0;
>> +
>> +     return kind;
>> +}
>
> While this looks fine, I am not sure if this is a good interface for
> filtering, though.
>
> If you start from already having everything and want to limit things
> down to "refs/heads/", this might make sense but it would be far
> more efficient, if you know that you want to limit to "refs/heads/"
> upfront, not to collect everything but just limit the collection to
> those under "refs/heads/" without wasting cycles in the first place.
>

Yes, considering this and what you've said below about how the
bits don't make sense, I re-wrote filter_refs() to filter based on what we want
So this part will be removed entirely. Thanks for pointing me in the
right direction.

>> diff --git a/ref-filter.h b/ref-filter.h
>> index 5be3e35..b5a13e8 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -16,6 +16,12 @@
>>  #define FILTER_REFS_INCLUDE_BROKEN 0x1
>>  #define FILTER_REFS_ALL 0x2
>>  #define FILTER_REFS_TAGS 0x4
>> +#define FILTER_REFS_BRANCHES 0x8
>
> Is this a sensible set of bits?  Does it make sense to have ALL and
> TAGS at the same time (and what does that mean?)?
>
>> +#define REF_DETACHED_HEAD   0x01
>> +#define REF_LOCAL_BRANCH    0x02
>> +#define REF_REMOTE_BRANCH   0x04
>> +#define REF_NO_BRANCH_FILTERING 0x08
>
> Where do these values go?  It is a returned by filter-branch-kind
> for each ref, i.e. given "refs/heads/bar", it answers "Yeah, that is
> a local branch".  Why are these values pretending to be a set of
> bits that can be combined together, as if a branch can be both LOCAL
> and REMOTE?  This does not make _any_ sense.
>

This was taken from branch.c, I thought of using an enum instead but that
would again require most of branch.c, hence it's been carried over
without changing
I'm thinking of changing it, any suggestions?

>
>>  #define ALIGN_LEFT 0x01
>>  #define ALIGN_RIGHT 0x02
>> @@ -50,7 +56,7 @@ struct ref_sorting {
>>
>>  struct ref_array_item {
>>       unsigned char objectname[20];
>> -     int flag;
>> +     int flag, kind;
>
> For readability, do not define multiple structure fields on a single
> line.
>
> If you are storing a set of bits in an integer, use unsigned.  If it
> is an enumeration, int is fine.
>

Thanks will change.

>>       const char *symref;
>>       struct commit *commit;
>>       struct atom_value *value;
>> @@ -76,7 +82,7 @@ struct ref_filter {
>>
>>       unsigned int with_commit_tag_algo : 1,
>>               match_as_path : 1;
>> -     unsigned int lines;
>> +     unsigned int lines, branch_kind;
>
> For readability, do not define multiple structure fields on a single
> line.
>
>>  };
>>
>>  struct ref_filter_cbdata {

Will do.

-- 
Regards,
Karthik Nayak

Re: [PATCH 01/10] ref-filter: add option to filter only branches

[Karthik Nayak]

On Thu, Aug 13, 2015 at 4:21 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>
> This was taken from branch.c, I thought of using an enum instead but that
> would again require most of branch.c, hence it's been carried over
> without changing
> I'm thinking of changing it, any suggestions?
>

What I was thinking was of having:

#define FILTER_REFS_INCLUDE_BROKEN 0x1
#define FILTER_REFS_TAGS 0x2
#define FILTER_REFS_BRANCHES 0x4
#define FILTER_REFS_REMOTES 0x8
#define FILTER_REFS_DETACHED_HEAD 0x16

and using these for showing ref kind also instead of separately
having 'REF_DETACHED_HEAD' and so on.

-- 
Regards,
Karthik Nayak

Re: [PATCH 01/10] ref-filter: add option to filter only branches

[Karthik Nayak]

On Thu, Aug 13, 2015 at 5:05 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Thu, Aug 13, 2015 at 4:21 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> This was taken from branch.c, I thought of using an enum instead but that
>> would again require most of branch.c, hence it's been carried over
>> without changing
>> I'm thinking of changing it, any suggestions?
>>
>
> What I was thinking was of having:
>
> #define FILTER_REFS_INCLUDE_BROKEN 0x1
> #define FILTER_REFS_TAGS 0x2
> #define FILTER_REFS_BRANCHES 0x4
> #define FILTER_REFS_REMOTES 0x8
> #define FILTER_REFS_DETACHED_HEAD 0x16
>
> and using these for showing ref kind also instead of separately
> having 'REF_DETACHED_HEAD' and so on.
>

Something like this:
https://github.com/KarthikNayak/git/commit/0ec5381420dcdfe7c62000b56168e2842d5d0063

-- 
Regards,
Karthik Nayak

Re: [PATCH 01/10] ref-filter: add option to filter only branches

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> On Thu, Aug 13, 2015 at 5:05 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Thu, Aug 13, 2015 at 4:21 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>
>>> This was taken from branch.c, I thought of using an enum instead but that
>>> would again require most of branch.c, hence it's been carried over
>>> without changing
>>> I'm thinking of changing it, any suggestions?
>>>
>>
>> What I was thinking was of having:
>>
>> #define FILTER_REFS_INCLUDE_BROKEN 0x1
>> #define FILTER_REFS_TAGS 0x2
>> #define FILTER_REFS_BRANCHES 0x4
>> #define FILTER_REFS_REMOTES 0x8
>> #define FILTER_REFS_DETACHED_HEAD 0x16
>>
>> and using these for showing ref kind also instead of separately
>> having 'REF_DETACHED_HEAD' and so on.
>>
>
> Something like this:
> https://github.com/KarthikNayak/git/commit/0ec5381420dcdfe7c62000b56168e2842d5d0063

I notice a few things in ref-filter.c in that commit (a web
interface including GitHub one is horrible in showing the things in
wider context across files, so I'll base my discussion by guessing
what the caller of this function and helpers this function calls
do):

 - Your "ALL" silently overrides others.  Is that sensible?  Perhaps
   you would instead want to define FILTER_REFS_OTHER (not needed to
   be exposed to UI) and then define FILTER_REFS_ALL as the ORed
   value of FILTER_REFS_{BRANCHES,...,OTHER}?

 - When the caller asks for "--branches --tags", you run
   ref-filter-handler twice on ref_cbdata.  Does that make sense?
   Shouldn't you iterate over all the available refs just once,
   rejecting ones that aren't in either refs/{heads,tags}/ instead?

Re: [PATCH 01/10] ref-filter: add option to filter only branches

[Karthik Nayak]

On Fri, Aug 14, 2015 at 9:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Thu, Aug 13, 2015 at 5:05 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> On Thu, Aug 13, 2015 at 4:21 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>>
>>>> This was taken from branch.c, I thought of using an enum instead but that
>>>> would again require most of branch.c, hence it's been carried over
>>>> without changing
>>>> I'm thinking of changing it, any suggestions?
>>>>
>>>
>>> What I was thinking was of having:
>>>
>>> #define FILTER_REFS_INCLUDE_BROKEN 0x1
>>> #define FILTER_REFS_TAGS 0x2
>>> #define FILTER_REFS_BRANCHES 0x4
>>> #define FILTER_REFS_REMOTES 0x8
>>> #define FILTER_REFS_DETACHED_HEAD 0x16
>>>
>>> and using these for showing ref kind also instead of separately
>>> having 'REF_DETACHED_HEAD' and so on.
>>>
>>
>> Something like this:
>> https://github.com/KarthikNayak/git/commit/0ec5381420dcdfe7c62000b56168e2842d5d0063
>
> I notice a few things in ref-filter.c in that commit (a web
> interface including GitHub one is horrible in showing the things in
> wider context across files, so I'll base my discussion by guessing
> what the caller of this function and helpers this function calls
> do):
>
>  - Your "ALL" silently overrides others.  Is that sensible?  Perhaps
>    you would instead want to define FILTER_REFS_OTHER (not needed to
>    be exposed to UI) and then define FILTER_REFS_ALL as the ORed
>    value of FILTER_REFS_{BRANCHES,...,OTHER}?
>

Well okay I could something on those lines.

>  - When the caller asks for "--branches --tags", you run
>    ref-filter-handler twice on ref_cbdata.  Does that make sense?
>    Shouldn't you iterate over all the available refs just once,
>    rejecting ones that aren't in either refs/{heads,tags}/ instead?

I was under the idea that since we're dealing with do_for_each_entry()
eventually and in that we set the loose_dir value based on the given 'base'
hence when the caller asks for something like "--branches --tags", it would
be better to just iterate through the refs in the directory of
"--branches --tags"
rather than go through the whole list of refs and drop ones which don't belong
to "--branches --tags". but this was an over the top look at how
do_for_each_entry()
works, I could be totally off the mark.

-- 
Regards,
Karthik Nayak

Re: [PATCH 01/10] ref-filter: add option to filter only branches

[Matthieu Moy]

Le 13 août 2015 13:35:21 GMT+02:00, Karthik Nayak <karthik.188@gmail.com> a écrit :
>On Thu, Aug 13, 2015 at 4:21 PM, Karthik Nayak <karthik.188@gmail.com>
>wrote:
>>
>> This was taken from branch.c, I thought of using an enum instead but
>that
>> would again require most of branch.c, hence it's been carried over
>> without changing
>> I'm thinking of changing it, any suggestions?
>>
>
>What I was thinking was of having:
>
>#define FILTER_REFS_INCLUDE_BROKEN 0x1
>#define FILTER_REFS_TAGS 0x2
>#define FILTER_REFS_BRANCHES 0x4
>#define FILTER_REFS_REMOTES 0x8
>#define FILTER_REFS_DETACHED_HEAD 0x16

You meant 0x10, not 0x16 I guess.

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

Re: [PATCH 01/10] ref-filter: add option to filter only branches

[Karthik Nayak]

On Thu, Aug 13, 2015 at 10:22 PM, Matthieu Moy <matthieu.moy@imag.fr> wrote:
>
>
> Le 13 août 2015 13:35:21 GMT+02:00, Karthik Nayak <karthik.188@gmail.com> a écrit :
>>On Thu, Aug 13, 2015 at 4:21 PM, Karthik Nayak <karthik.188@gmail.com>
>>wrote:
>>>
>>> This was taken from branch.c, I thought of using an enum instead but
>>that
>>> would again require most of branch.c, hence it's been carried over
>>> without changing
>>> I'm thinking of changing it, any suggestions?
>>>
>>
>>What I was thinking was of having:
>>
>>#define FILTER_REFS_INCLUDE_BROKEN 0x1
>>#define FILTER_REFS_TAGS 0x2
>>#define FILTER_REFS_BRANCHES 0x4
>>#define FILTER_REFS_REMOTES 0x8
>>#define FILTER_REFS_DETACHED_HEAD 0x16
>
> You meant 0x10, not 0x16 I guess.
>

Yea! Of course, was randomly typing out an example :)


-- 
Regards,
Karthik Nayak

[PATCH 02/10] branch: refactor width computation

[Karthik Nayak]

From: Karthik Nayak <karthik.188@gmail.com>

Remove unnecessary variables from ref_list and ref_item which were
used for width computation. This is to make ref_item similar to
ref-filter's ref_array_item. This will ensure a smooth port of
branch.c to use ref-filter APIs in further patches.

Previously the maxwidth was computed when inserting the refs into the
ref_list. Now, we obtain the entire ref_list and then compute
maxwidth.

Based-on-patch-by: Jeff King <peff@peff.net>
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>
---
 builtin/branch.c | 61 +++++++++++++++++++++++++++++---------------------------
 1 file changed, 32 insertions(+), 29 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 4fc8beb..b058b74 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -282,14 +282,14 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 struct ref_item {
 	char *name;
 	char *dest;
-	unsigned int kind, width;
+	unsigned int kind;
 	struct commit *commit;
 	int ignore;
 };
 
 struct ref_list {
 	struct rev_info revs;
-	int index, alloc, maxwidth, verbose, abbrev;
+	int index, alloc, verbose, abbrev;
 	struct ref_item *list;
 	struct commit_list *with_commit;
 	int kinds;
@@ -386,15 +386,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
 	newitem->name = xstrdup(refname);
 	newitem->kind = kind;
 	newitem->commit = commit;
-	newitem->width = utf8_strwidth(refname);
 	newitem->dest = resolve_symref(orig_refname, prefix);
 	newitem->ignore = 0;
-	/* adjust for "remotes/" */
-	if (newitem->kind == REF_REMOTE_BRANCH &&
-	    ref_list->kinds != REF_REMOTE_BRANCH)
-		newitem->width += 8;
-	if (newitem->width > ref_list->maxwidth)
-		ref_list->maxwidth = newitem->width;
 
 	return 0;
 }
@@ -505,11 +498,12 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
 }
 
 static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
-			   int abbrev, int current, char *prefix)
+			   int abbrev, int current, const char *remote_prefix)
 {
 	char c;
 	int color;
 	struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
+	const char *prefix = "";
 
 	if (item->ignore)
 		return;
@@ -520,6 +514,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 		break;
 	case REF_REMOTE_BRANCH:
 		color = BRANCH_COLOR_REMOTE;
+		prefix = remote_prefix;
 		break;
 	default:
 		color = BRANCH_COLOR_PLAIN;
@@ -557,16 +552,21 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 	strbuf_release(&out);
 }
 
-static int calc_maxwidth(struct ref_list *refs)
+static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
 {
-	int i, w = 0;
+	int i, max = 0;
 	for (i = 0; i < refs->index; i++) {
+		struct ref_item *it = &refs->list[i];
+		int w = utf8_strwidth(it->name);
+
 		if (refs->list[i].ignore)
 			continue;
-		if (refs->list[i].width > w)
-			w = refs->list[i].width;
+		if (it->kind == REF_REMOTE_BRANCH)
+			w += remote_bonus;
+		if (w > max)
+			max = w;
 	}
-	return w;
+	return max;
 }
 
 static char *get_head_description(void)
@@ -600,21 +600,18 @@ static char *get_head_description(void)
 	return strbuf_detach(&desc, NULL);
 }
 
-static void show_detached(struct ref_list *ref_list)
+static void show_detached(struct ref_list *ref_list, int maxwidth)
 {
 	struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
 
 	if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
 		struct ref_item item;
 		item.name = get_head_description();
-		item.width = utf8_strwidth(item.name);
 		item.kind = REF_LOCAL_BRANCH;
 		item.dest = NULL;
 		item.commit = head_commit;
 		item.ignore = 0;
-		if (item.width > ref_list->maxwidth)
-			ref_list->maxwidth = item.width;
-		print_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
+		print_ref_item(&item, maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
 		free(item.name);
 	}
 }
@@ -624,6 +621,16 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 	int i;
 	struct append_ref_cb cb;
 	struct ref_list ref_list;
+	int maxwidth = 0;
+	const char *remote_prefix = "";
+
+	/*
+	 * If we are listing more than just remote branches,
+	 * then remote branches will have a "remotes/" prefix.
+	 * We need to account for this in the width.
+	 */
+	if (kinds != REF_REMOTE_BRANCH)
+		remote_prefix = "remotes/";
 
 	memset(&ref_list, 0, sizeof(ref_list));
 	ref_list.kinds = kinds;
@@ -667,26 +674,22 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 			clear_commit_marks(item->commit, ALL_REV_FLAGS);
 		}
 		clear_commit_marks(filter, ALL_REV_FLAGS);
-
-		if (verbose)
-			ref_list.maxwidth = calc_maxwidth(&ref_list);
 	}
+	if (verbose)
+		maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
 
 	qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
 
 	detached = (detached && (kinds & REF_LOCAL_BRANCH));
 	if (detached && match_patterns(pattern, "HEAD"))
-		show_detached(&ref_list);
+		show_detached(&ref_list, maxwidth);
 
 	for (i = 0; i < ref_list.index; i++) {
 		int current = !detached &&
 			(ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
 			!strcmp(ref_list.list[i].name, head);
-		char *prefix = (kinds != REF_REMOTE_BRANCH &&
-				ref_list.list[i].kind == REF_REMOTE_BRANCH)
-				? "remotes/" : "";
-		print_ref_item(&ref_list.list[i], ref_list.maxwidth, verbose,
-			       abbrev, current, prefix);
+		print_ref_item(&ref_list.list[i], maxwidth, verbose,
+			       abbrev, current, remote_prefix);
 	}
 
 	free_ref_list(&ref_list);
-- 
2.5.0

Re: [PATCH 02/10] branch: refactor width computation

[Eric Sunshine]

On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Remove unnecessary variables from ref_list and ref_item which were
> used for width computation. This is to make ref_item similar to
> ref-filter's ref_array_item. This will ensure a smooth port of
> branch.c to use ref-filter APIs in further patches.
>
> Previously the maxwidth was computed when inserting the refs into the
> ref_list. Now, we obtain the entire ref_list and then compute
> maxwidth.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 4fc8beb..b058b74 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -557,16 +552,21 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
> -static int calc_maxwidth(struct ref_list *refs)
> +static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
>  {
> -       int i, w = 0;
> +       int i, max = 0;
>         for (i = 0; i < refs->index; i++) {
> +               struct ref_item *it = &refs->list[i];
> +               int w = utf8_strwidth(it->name);
> +
>                 if (refs->list[i].ignore)
>                         continue;

Nit: Unnecessary work. You compute the width and then immediately
throw it away when 'ignore' is true.

Also, you use 'it' elsewhere rather than 'refs->list[i]' but not this
line, which makes it seem out of place. I would have expected to see:

    if (it->ignore)
        continue;

(Despite the noisier diff, the end result will be more consistent.)

> -               if (refs->list[i].width > w)
> -                       w = refs->list[i].width;
> +               if (it->kind == REF_REMOTE_BRANCH)
> +                       w += remote_bonus;
> +               if (w > max)
> +                       max = w;
>         }
> -       return w;
> +       return max;
>  }


On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Remove unnecessary variables from ref_list and ref_item which were
> used for width computation. This is to make ref_item similar to
> ref-filter's ref_array_item. This will ensure a smooth port of
> branch.c to use ref-filter APIs in further patches.
>
> Previously the maxwidth was computed when inserting the refs into the
> ref_list. Now, we obtain the entire ref_list and then compute
> maxwidth.
>
> Based-on-patch-by: Jeff King <peff@peff.net>
> 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>
> ---
>  builtin/branch.c | 61 +++++++++++++++++++++++++++++---------------------------
>  1 file changed, 32 insertions(+), 29 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 4fc8beb..b058b74 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -282,14 +282,14 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  struct ref_item {
>         char *name;
>         char *dest;
> -       unsigned int kind, width;
> +       unsigned int kind;
>         struct commit *commit;
>         int ignore;
>  };
>
>  struct ref_list {
>         struct rev_info revs;
> -       int index, alloc, maxwidth, verbose, abbrev;
> +       int index, alloc, verbose, abbrev;
>         struct ref_item *list;
>         struct commit_list *with_commit;
>         int kinds;
> @@ -386,15 +386,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
>         newitem->name = xstrdup(refname);
>         newitem->kind = kind;
>         newitem->commit = commit;
> -       newitem->width = utf8_strwidth(refname);
>         newitem->dest = resolve_symref(orig_refname, prefix);
>         newitem->ignore = 0;
> -       /* adjust for "remotes/" */
> -       if (newitem->kind == REF_REMOTE_BRANCH &&
> -           ref_list->kinds != REF_REMOTE_BRANCH)
> -               newitem->width += 8;
> -       if (newitem->width > ref_list->maxwidth)
> -               ref_list->maxwidth = newitem->width;
>
>         return 0;
>  }
> @@ -505,11 +498,12 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
>  }
>
>  static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
> -                          int abbrev, int current, char *prefix)
> +                          int abbrev, int current, const char *remote_prefix)
>  {
>         char c;
>         int color;
>         struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
> +       const char *prefix = "";
>
>         if (item->ignore)
>                 return;
> @@ -520,6 +514,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>                 break;
>         case REF_REMOTE_BRANCH:
>                 color = BRANCH_COLOR_REMOTE;
> +               prefix = remote_prefix;
>                 break;
>         default:
>                 color = BRANCH_COLOR_PLAIN;
> @@ -557,16 +552,21 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>         strbuf_release(&out);
>  }
>
> -static int calc_maxwidth(struct ref_list *refs)
> +static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
>  {
> -       int i, w = 0;
> +       int i, max = 0;
>         for (i = 0; i < refs->index; i++) {
> +               struct ref_item *it = &refs->list[i];
> +               int w = utf8_strwidth(it->name);
> +
>                 if (refs->list[i].ignore)
>                         continue;
> -               if (refs->list[i].width > w)
> -                       w = refs->list[i].width;
> +               if (it->kind == REF_REMOTE_BRANCH)
> +                       w += remote_bonus;
> +               if (w > max)
> +                       max = w;
>         }
> -       return w;
> +       return max;
>  }
>
>  static char *get_head_description(void)
> @@ -600,21 +600,18 @@ static char *get_head_description(void)
>         return strbuf_detach(&desc, NULL);
>  }
>
> -static void show_detached(struct ref_list *ref_list)
> +static void show_detached(struct ref_list *ref_list, int maxwidth)
>  {
>         struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
>
>         if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
>                 struct ref_item item;
>                 item.name = get_head_description();
> -               item.width = utf8_strwidth(item.name);
>                 item.kind = REF_LOCAL_BRANCH;
>                 item.dest = NULL;
>                 item.commit = head_commit;
>                 item.ignore = 0;
> -               if (item.width > ref_list->maxwidth)
> -                       ref_list->maxwidth = item.width;
> -               print_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
> +               print_ref_item(&item, maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
>                 free(item.name);
>         }
>  }
> @@ -624,6 +621,16 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
>         int i;
>         struct append_ref_cb cb;
>         struct ref_list ref_list;
> +       int maxwidth = 0;
> +       const char *remote_prefix = "";
> +
> +       /*
> +        * If we are listing more than just remote branches,
> +        * then remote branches will have a "remotes/" prefix.
> +        * We need to account for this in the width.
> +        */
> +       if (kinds != REF_REMOTE_BRANCH)
> +               remote_prefix = "remotes/";
>
>         memset(&ref_list, 0, sizeof(ref_list));
>         ref_list.kinds = kinds;
> @@ -667,26 +674,22 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
>                         clear_commit_marks(item->commit, ALL_REV_FLAGS);
>                 }
>                 clear_commit_marks(filter, ALL_REV_FLAGS);
> -
> -               if (verbose)
> -                       ref_list.maxwidth = calc_maxwidth(&ref_list);
>         }
> +       if (verbose)
> +               maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
>
>         qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
>
>         detached = (detached && (kinds & REF_LOCAL_BRANCH));
>         if (detached && match_patterns(pattern, "HEAD"))
> -               show_detached(&ref_list);
> +               show_detached(&ref_list, maxwidth);
>
>         for (i = 0; i < ref_list.index; i++) {
>                 int current = !detached &&
>                         (ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
>                         !strcmp(ref_list.list[i].name, head);
> -               char *prefix = (kinds != REF_REMOTE_BRANCH &&
> -                               ref_list.list[i].kind == REF_REMOTE_BRANCH)
> -                               ? "remotes/" : "";
> -               print_ref_item(&ref_list.list[i], ref_list.maxwidth, verbose,
> -                              abbrev, current, prefix);
> +               print_ref_item(&ref_list.list[i], maxwidth, verbose,
> +                              abbrev, current, remote_prefix);
>         }
>
>         free_ref_list(&ref_list);
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/10] branch: refactor width computation

[Karthik Nayak]

On Tue, Aug 11, 2015 at 7:28 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Remove unnecessary variables from ref_list and ref_item which were
>> used for width computation. This is to make ref_item similar to
>> ref-filter's ref_array_item. This will ensure a smooth port of
>> branch.c to use ref-filter APIs in further patches.
>>
>> Previously the maxwidth was computed when inserting the refs into the
>> ref_list. Now, we obtain the entire ref_list and then compute
>> maxwidth.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index 4fc8beb..b058b74 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -557,16 +552,21 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>> -static int calc_maxwidth(struct ref_list *refs)
>> +static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
>>  {
>> -       int i, w = 0;
>> +       int i, max = 0;
>>         for (i = 0; i < refs->index; i++) {
>> +               struct ref_item *it = &refs->list[i];
>> +               int w = utf8_strwidth(it->name);
>> +
>>                 if (refs->list[i].ignore)
>>                         continue;
>
> Nit: Unnecessary work. You compute the width and then immediately
> throw it away when 'ignore' is true.
>
> Also, you use 'it' elsewhere rather than 'refs->list[i]' but not this
> line, which makes it seem out of place. I would have expected to see:
>
>     if (it->ignore)
>         continue;
>
> (Despite the noisier diff, the end result will be more consistent.)
>

Ah right, will put that after the check :D

Yea, that seems out of place. Thanks

-- 
Regards,
Karthik Nayak

[PATCH 03/10] branch: bump get_head_description() to the top

[Karthik Nayak]

This is a preperatory patch for 'roll show_detached HEAD into regular
ref_list'. This patch moves get_head_descrition() to the top so that
it can be used in print_ref_item().

Based-on-patch-by: Jeff King <peff@peff.net>
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>
---
 builtin/branch.c | 62 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index b058b74..65f6d0d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -497,6 +497,37 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
 	strbuf_release(&subject);
 }
 
+static char *get_head_description(void)
+{
+	struct strbuf desc = STRBUF_INIT;
+	struct wt_status_state state;
+	memset(&state, 0, sizeof(state));
+	wt_status_get_state(&state, 1);
+	if (state.rebase_in_progress ||
+	    state.rebase_interactive_in_progress)
+		strbuf_addf(&desc, _("(no branch, rebasing %s)"),
+			    state.branch);
+	else if (state.bisect_in_progress)
+		strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
+			    state.branch);
+	else if (state.detached_from) {
+		/* TRANSLATORS: make sure these match _("HEAD detached at ")
+		   and _("HEAD detached from ") in wt-status.c */
+		if (state.detached_at)
+			strbuf_addf(&desc, _("(HEAD detached at %s)"),
+				state.detached_from);
+		else
+			strbuf_addf(&desc, _("(HEAD detached from %s)"),
+				state.detached_from);
+	}
+	else
+		strbuf_addstr(&desc, _("(no branch)"));
+	free(state.branch);
+	free(state.onto);
+	free(state.detached_from);
+	return strbuf_detach(&desc, NULL);
+}
+
 static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 			   int abbrev, int current, const char *remote_prefix)
 {
@@ -569,37 +600,6 @@ static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
 	return max;
 }
 
-static char *get_head_description(void)
-{
-	struct strbuf desc = STRBUF_INIT;
-	struct wt_status_state state;
-	memset(&state, 0, sizeof(state));
-	wt_status_get_state(&state, 1);
-	if (state.rebase_in_progress ||
-	    state.rebase_interactive_in_progress)
-		strbuf_addf(&desc, _("(no branch, rebasing %s)"),
-			    state.branch);
-	else if (state.bisect_in_progress)
-		strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
-			    state.branch);
-	else if (state.detached_from) {
-		/* TRANSLATORS: make sure these match _("HEAD detached at ")
-		   and _("HEAD detached from ") in wt-status.c */
-		if (state.detached_at)
-			strbuf_addf(&desc, _("(HEAD detached at %s)"),
-				state.detached_from);
-		else
-			strbuf_addf(&desc, _("(HEAD detached from %s)"),
-				state.detached_from);
-	}
-	else
-		strbuf_addstr(&desc, _("(no branch)"));
-	free(state.branch);
-	free(state.onto);
-	free(state.detached_from);
-	return strbuf_detach(&desc, NULL);
-}
-
 static void show_detached(struct ref_list *ref_list, int maxwidth)
 {
 	struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
-- 
2.5.0

Re: [PATCH 03/10] branch: bump get_head_description() to the top

[Eric Sunshine]

On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> This is a preperatory patch for 'roll show_detached HEAD into regular
> ref_list'. This patch moves get_head_descrition() to the top so that

s/descrition/description/

> it can be used in print_ref_item().
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>

Re: [PATCH 03/10] branch: bump get_head_description() to the top

[Karthik Nayak]

On Tue, Aug 11, 2015 at 7:29 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> This is a preperatory patch for 'roll show_detached HEAD into regular
>> ref_list'. This patch moves get_head_descrition() to the top so that
>
> s/descrition/description/
>
>> it can be used in print_ref_item().
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>

Thanks, will change.

-- 
Regards,
Karthik Nayak

[PATCH 04/10] branch: roll show_detached HEAD into regular ref_list

[Karthik Nayak]

Remove show_detached() and make detached HEAD to be rolled into
regular ref_list by adding REF_DETACHED_HEAD as a kind of branch and
supporting the same in append_ref(). This eliminates the need for an
extra function and helps in easier porting of branch.c to use
ref-filter APIs.

Before show_detached() used to check if the HEAD branch satisfies the
'--contains' option, now that is taken care by append_ref().

Based-on-patch-by: Jeff King <peff@peff.net>
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>
---
 builtin/branch.c | 68 +++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 40 insertions(+), 28 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 65f6d0d..81815c9 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -30,6 +30,7 @@ static const char * const builtin_branch_usage[] = {
 
 #define REF_LOCAL_BRANCH    0x01
 #define REF_REMOTE_BRANCH   0x02
+#define REF_DETACHED_HEAD   0x04
 
 static const char *head;
 static unsigned char head_sha1[20];
@@ -352,8 +353,12 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
 			break;
 		}
 	}
-	if (ARRAY_SIZE(ref_kind) <= i)
-		return 0;
+	if (ARRAY_SIZE(ref_kind) <= i) {
+		if (!strcmp(refname, "HEAD"))
+			kind = REF_DETACHED_HEAD;
+		else
+			return 0;
+	}
 
 	/* Don't add types the caller doesn't want */
 	if ((kind & ref_list->kinds) == 0)
@@ -535,6 +540,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 	int color;
 	struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
 	const char *prefix = "";
+	const char *desc = item->name;
 
 	if (item->ignore)
 		return;
@@ -547,6 +553,10 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 		color = BRANCH_COLOR_REMOTE;
 		prefix = remote_prefix;
 		break;
+	case REF_DETACHED_HEAD:
+		color = BRANCH_COLOR_CURRENT;
+		desc = get_head_description();
+		break;
 	default:
 		color = BRANCH_COLOR_PLAIN;
 		break;
@@ -558,7 +568,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 		color = BRANCH_COLOR_CURRENT;
 	}
 
-	strbuf_addf(&name, "%s%s", prefix, item->name);
+	strbuf_addf(&name, "%s%s", prefix, desc);
 	if (verbose) {
 		int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);
 		strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
@@ -581,6 +591,8 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 	}
 	strbuf_release(&name);
 	strbuf_release(&out);
+	if (item->kind == REF_DETACHED_HEAD)
+		free((void *)desc);
 }
 
 static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
@@ -600,25 +612,9 @@ static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
 	return max;
 }
 
-static void show_detached(struct ref_list *ref_list, int maxwidth)
-{
-	struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
-
-	if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
-		struct ref_item item;
-		item.name = get_head_description();
-		item.kind = REF_LOCAL_BRANCH;
-		item.dest = NULL;
-		item.commit = head_commit;
-		item.ignore = 0;
-		print_ref_item(&item, maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
-		free(item.name);
-	}
-}
-
 static int print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)
 {
-	int i;
+	int i, index;
 	struct append_ref_cb cb;
 	struct ref_list ref_list;
 	int maxwidth = 0;
@@ -642,7 +638,14 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 	cb.ref_list = &ref_list;
 	cb.pattern = pattern;
 	cb.ret = 0;
+	/*
+	 * First we obtain all regular branch refs and then if the
+	 * HEAD is detached then we insert that ref to the end of the
+	 * ref_fist so that it can be printed first.
+	 */
 	for_each_rawref(append_ref, &cb);
+	if (detached)
+		head_ref(append_ref, &cb);
 	/*
 	 * The following implementation is currently duplicated in ref-filter. It
 	 * will eventually be removed when we port branch.c to use ref-filter APIs.
@@ -678,15 +681,20 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 	if (verbose)
 		maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
 
-	qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
+	index = ref_list.index;
+
+	/* Print detached HEAD before sorting and printing the rest */
+	if (detached && (ref_list.list[index - 1].kind == REF_DETACHED_HEAD) &&
+	    !strcmp(ref_list.list[index - 1].name, head)) {
+		print_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,
+			       1, remote_prefix);
+		index -= 1;
+	}
 
-	detached = (detached && (kinds & REF_LOCAL_BRANCH));
-	if (detached && match_patterns(pattern, "HEAD"))
-		show_detached(&ref_list, maxwidth);
+	qsort(ref_list.list, index, sizeof(struct ref_item), ref_cmp);
 
-	for (i = 0; i < ref_list.index; i++) {
-		int current = !detached &&
-			(ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
+	for (i = 0; i < index; i++) {
+		int current = !detached && (ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
 			!strcmp(ref_list.list[i].name, head);
 		print_ref_item(&ref_list.list[i], maxwidth, verbose,
 			       abbrev, current, remote_prefix);
@@ -913,7 +921,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("branch name required"));
 		return delete_branches(argc, argv, delete > 1, kinds, quiet);
 	} else if (list) {
-		int ret = print_ref_list(kinds, detached, verbose, abbrev,
+		int ret;
+		/*  git branch --local also shows HEAD when it is detached */
+		if (kinds & REF_LOCAL_BRANCH)
+			kinds |= REF_DETACHED_HEAD;
+		ret = print_ref_list(kinds, detached, verbose, abbrev,
 					 with_commit, argv);
 		print_columns(&output, colopts, NULL);
 		string_list_clear(&output, 0);
-- 
2.5.0

Re: [PATCH 04/10] branch: roll show_detached HEAD into regular ref_list

[Eric Sunshine]

On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Remove show_detached() and make detached HEAD to be rolled into
> regular ref_list by adding REF_DETACHED_HEAD as a kind of branch and
> supporting the same in append_ref(). This eliminates the need for an
> extra function and helps in easier porting of branch.c to use
> ref-filter APIs.
>
> Before show_detached() used to check if the HEAD branch satisfies the
> '--contains' option, now that is taken care by append_ref().
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 65f6d0d..81815c9 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -535,6 +540,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>         int color;
>         struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
>         const char *prefix = "";
> +       const char *desc = item->name;
>
>         if (item->ignore)
>                 return;
> @@ -547,6 +553,10 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>                 color = BRANCH_COLOR_REMOTE;
>                 prefix = remote_prefix;
>                 break;
> +       case REF_DETACHED_HEAD:
> +               color = BRANCH_COLOR_CURRENT;
> +               desc = get_head_description();
> +               break;
>         default:
>                 color = BRANCH_COLOR_PLAIN;
>                 break;
> @@ -558,7 +568,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>                 color = BRANCH_COLOR_CURRENT;
>         }
>
> -       strbuf_addf(&name, "%s%s", prefix, item->name);
> +       strbuf_addf(&name, "%s%s", prefix, desc);
>         if (verbose) {
>                 int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);
>                 strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
> @@ -581,6 +591,8 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>         }
>         strbuf_release(&name);
>         strbuf_release(&out);
> +       if (item->kind == REF_DETACHED_HEAD)
> +               free((void *)desc);

This would be cleaner, and more easily extended to other cases if you
used a 'to_free' variable. For instance, something like this:

    const char *desc = item->name;
    char *to_free = NULL;
    ...
    case REF_DETACHED_HEAD:
        ...
        desc = to_free = get_head_description();
        break;
    ...
    strbuf_addf(&name, "%s%s", prefix, desc);
    ...
    free(to_free);

Note that it's safe to free 'to_free' when it's NULL, so you don't
need to protect the free() with that ugly specialized 'if' which
checks for REF_DETACHED_HEAD. You can also do away with the "cast to
non-const" when freeing.

>  }
> @@ -642,7 +638,14 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
>         cb.ref_list = &ref_list;
>         cb.pattern = pattern;
>         cb.ret = 0;
> +       /*
> +        * First we obtain all regular branch refs and then if the

s/then//

> +        * HEAD is detached then we insert that ref to the end of the
> +        * ref_fist so that it can be printed first.
> +        */
>         for_each_rawref(append_ref, &cb);
> +       if (detached)
> +               head_ref(append_ref, &cb);

Re: [PATCH 04/10] branch: roll show_detached HEAD into regular ref_list

[Karthik Nayak]

On Tue, Aug 11, 2015 at 8:11 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Remove show_detached() and make detached HEAD to be rolled into
>> regular ref_list by adding REF_DETACHED_HEAD as a kind of branch and
>> supporting the same in append_ref(). This eliminates the need for an
>> extra function and helps in easier porting of branch.c to use
>> ref-filter APIs.
>>
>> Before show_detached() used to check if the HEAD branch satisfies the
>> '--contains' option, now that is taken care by append_ref().
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index 65f6d0d..81815c9 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -535,6 +540,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>>         int color;
>>         struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
>>         const char *prefix = "";
>> +       const char *desc = item->name;
>>
>>         if (item->ignore)
>>                 return;
>> @@ -547,6 +553,10 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>>                 color = BRANCH_COLOR_REMOTE;
>>                 prefix = remote_prefix;
>>                 break;
>> +       case REF_DETACHED_HEAD:
>> +               color = BRANCH_COLOR_CURRENT;
>> +               desc = get_head_description();
>> +               break;
>>         default:
>>                 color = BRANCH_COLOR_PLAIN;
>>                 break;
>> @@ -558,7 +568,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>>                 color = BRANCH_COLOR_CURRENT;
>>         }
>>
>> -       strbuf_addf(&name, "%s%s", prefix, item->name);
>> +       strbuf_addf(&name, "%s%s", prefix, desc);
>>         if (verbose) {
>>                 int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);
>>                 strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
>> @@ -581,6 +591,8 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>>         }
>>         strbuf_release(&name);
>>         strbuf_release(&out);
>> +       if (item->kind == REF_DETACHED_HEAD)
>> +               free((void *)desc);
>
> This would be cleaner, and more easily extended to other cases if you
> used a 'to_free' variable. For instance, something like this:
>
>     const char *desc = item->name;
>     char *to_free = NULL;
>     ...
>     case REF_DETACHED_HEAD:
>         ...
>         desc = to_free = get_head_description();
>         break;
>     ...
>     strbuf_addf(&name, "%s%s", prefix, desc);
>     ...
>     free(to_free);
>

This looks neater!

> Note that it's safe to free 'to_free' when it's NULL, so you don't
> need to protect the free() with that ugly specialized 'if' which
> checks for REF_DETACHED_HEAD. You can also do away with the "cast to
> non-const" when freeing.
>

Yea makes sense will change to what you suggested.

>>  }
>> @@ -642,7 +638,14 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
>>         cb.ref_list = &ref_list;
>>         cb.pattern = pattern;
>>         cb.ret = 0;
>> +       /*
>> +        * First we obtain all regular branch refs and then if the
>
> s/then//
>

Thanks

-- 
Regards,
Karthik Nayak

[PATCH 05/10] branch: move 'current' check down to the presentation layer

[Karthik Nayak]

We check if given ref is the current branch in print_ref_list(). Move
this check to print_ref_item() where it is checked right before
printing.

Based-on-patch-by: Jeff King <peff@peff.net>
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>
---
 builtin/branch.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 81815c9..c5f2944 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -534,9 +534,10 @@ static char *get_head_description(void)
 }
 
 static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
-			   int abbrev, int current, const char *remote_prefix)
+			   int abbrev, int detached, const char *remote_prefix)
 {
 	char c;
+	int current = 0;
 	int color;
 	struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
 	const char *prefix = "";
@@ -547,15 +548,18 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 
 	switch (item->kind) {
 	case REF_LOCAL_BRANCH:
-		color = BRANCH_COLOR_LOCAL;
+		if (!detached && !strcmp(item->name, head))
+			current = 1;
+		else
+			color = BRANCH_COLOR_LOCAL;
 		break;
 	case REF_REMOTE_BRANCH:
 		color = BRANCH_COLOR_REMOTE;
 		prefix = remote_prefix;
 		break;
 	case REF_DETACHED_HEAD:
-		color = BRANCH_COLOR_CURRENT;
 		desc = get_head_description();
+		current = 1;
 		break;
 	default:
 		color = BRANCH_COLOR_PLAIN;
@@ -684,21 +688,17 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 	index = ref_list.index;
 
 	/* Print detached HEAD before sorting and printing the rest */
-	if (detached && (ref_list.list[index - 1].kind == REF_DETACHED_HEAD) &&
-	    !strcmp(ref_list.list[index - 1].name, head)) {
+	if (detached) {
 		print_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,
-			       1, remote_prefix);
+			       detached, remote_prefix);
 		index -= 1;
 	}
 
 	qsort(ref_list.list, index, sizeof(struct ref_item), ref_cmp);
 
-	for (i = 0; i < index; i++) {
-		int current = !detached && (ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
-			!strcmp(ref_list.list[i].name, head);
+	for (i = 0; i < index; i++)
 		print_ref_item(&ref_list.list[i], maxwidth, verbose,
-			       abbrev, current, remote_prefix);
-	}
+			       abbrev, detached, remote_prefix);
 
 	free_ref_list(&ref_list);
 
-- 
2.5.0

[PATCH 06/10] branch: drop non-commit error reporting

[Karthik Nayak]

Remove the error reporting variable to make the code easier to port
over to using ref-filter APIs.

Based-on-patch-by: Jeff King <peff@peff.net>
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>
---
 builtin/branch.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index c5f2944..1ac7fbc 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -313,7 +313,6 @@ static char *resolve_symref(const char *src, const char *prefix)
 struct append_ref_cb {
 	struct ref_list *ref_list;
 	const char **pattern;
-	int ret;
 };
 
 static int match_patterns(const char **pattern, const char *refname)
@@ -370,10 +369,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
 	commit = NULL;
 	if (ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER) {
 		commit = lookup_commit_reference_gently(oid->hash, 1);
-		if (!commit) {
-			cb->ret = error(_("branch '%s' does not point at a commit"), refname);
+		if (!commit)
 			return 0;
-		}
 
 		/* Filter with with_commit if specified */
 		if (!is_descendant_of(commit, ref_list->with_commit))
@@ -616,7 +613,7 @@ static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
 	return max;
 }
 
-static int print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)
+static void print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)
 {
 	int i, index;
 	struct append_ref_cb cb;
@@ -641,7 +638,6 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 		init_revisions(&ref_list.revs, NULL);
 	cb.ref_list = &ref_list;
 	cb.pattern = pattern;
-	cb.ret = 0;
 	/*
 	 * First we obtain all regular branch refs and then if the
 	 * HEAD is detached then we insert that ref to the end of the
@@ -701,11 +697,6 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 			       abbrev, detached, remote_prefix);
 
 	free_ref_list(&ref_list);
-
-	if (cb.ret)
-		error(_("some refs could not be read"));
-
-	return cb.ret;
 }
 
 static void rename_branch(const char *oldname, const char *newname, int force)
@@ -921,15 +912,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("branch name required"));
 		return delete_branches(argc, argv, delete > 1, kinds, quiet);
 	} else if (list) {
-		int ret;
 		/*  git branch --local also shows HEAD when it is detached */
 		if (kinds & REF_LOCAL_BRANCH)
 			kinds |= REF_DETACHED_HEAD;
-		ret = print_ref_list(kinds, detached, verbose, abbrev,
+		print_ref_list(kinds, detached, verbose, abbrev,
 					 with_commit, argv);
 		print_columns(&output, colopts, NULL);
 		string_list_clear(&output, 0);
-		return ret;
+		return 0;
 	}
 	else if (edit_description) {
 		const char *branch_name;
-- 
2.5.0

[PATCH 07/10] branch.c: use 'ref-filter' data structures

[Karthik Nayak]

Make 'branch.c' use 'ref-filter' data structures and make changes to
support the new data structures. This is a part of the process of
porting 'branch.c' to use 'ref-filter' APIs.

This is a temporary step before porting 'branch.c' to use 'ref-filter'
completely. As this is a temporary step, most of the code introduced
here will be removed when 'branch.c' is ported over to use
'ref-filter' APIs

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>
---
 builtin/branch.c | 295 ++++++++++++++++++++++---------------------------------
 ref-filter.h     |   7 +-
 2 files changed, 122 insertions(+), 180 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 1ac7fbc..edc3d7d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -19,6 +19,7 @@
 #include "column.h"
 #include "utf8.h"
 #include "wt-status.h"
+#include "ref-filter.h"
 
 static const char * const builtin_branch_usage[] = {
 	N_("git branch [<options>] [-r | -a] [--merged | --no-merged]"),
@@ -28,10 +29,6 @@ static const char * const builtin_branch_usage[] = {
 	NULL
 };
 
-#define REF_LOCAL_BRANCH    0x01
-#define REF_REMOTE_BRANCH   0x02
-#define REF_DETACHED_HEAD   0x04
-
 static const char *head;
 static unsigned char head_sha1[20];
 
@@ -53,13 +50,6 @@ enum color_branch {
 	BRANCH_COLOR_UPSTREAM = 5
 };
 
-static enum merge_filter {
-	NO_FILTER = 0,
-	SHOW_NOT_MERGED,
-	SHOW_MERGED
-} merge_filter;
-static unsigned char merge_filter_ref[20];
-
 static struct string_list output = STRING_LIST_INIT_DUP;
 static unsigned int colopts;
 
@@ -280,22 +270,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 	return(ret);
 }
 
-struct ref_item {
-	char *name;
-	char *dest;
-	unsigned int kind;
-	struct commit *commit;
-	int ignore;
-};
-
-struct ref_list {
-	struct rev_info revs;
-	int index, alloc, verbose, abbrev;
-	struct ref_item *list;
-	struct commit_list *with_commit;
-	int kinds;
-};
-
 static char *resolve_symref(const char *src, const char *prefix)
 {
 	unsigned char sha1[20];
@@ -310,11 +284,6 @@ static char *resolve_symref(const char *src, const char *prefix)
 	return xstrdup(dst);
 }
 
-struct append_ref_cb {
-	struct ref_list *ref_list;
-	const char **pattern;
-};
-
 static int match_patterns(const char **pattern, const char *refname)
 {
 	if (!*pattern)
@@ -327,11 +296,29 @@ static int match_patterns(const char **pattern, const char *refname)
 	return 0;
 }
 
+/*
+ * Allocate memory for a new ref_array_item and insert that into the
+ * given ref_array. Doesn't take the objectname unlike
+ * new_ref_array_item(). This is a temporary function which will be
+ * removed when we port branch.c to use ref-filter APIs.
+ */
+static struct ref_array_item *ref_array_append(struct ref_array *array, const char *refname)
+{
+	size_t len = strlen(refname);
+	struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
+	memcpy(ref->refname, refname, len);
+	ref->refname[len] = '\0';
+	REALLOC_ARRAY(array->items, array->nr + 1);
+	array->items[array->nr++] = ref;
+	return ref;
+}
+
 static int append_ref(const char *refname, const struct object_id *oid, int flags, void *cb_data)
 {
-	struct append_ref_cb *cb = (struct append_ref_cb *)(cb_data);
-	struct ref_list *ref_list = cb->ref_list;
-	struct ref_item *newitem;
+	struct ref_filter_cbdata *cb = (struct ref_filter_cbdata *)(cb_data);
+	struct ref_filter *filter = cb->filter;
+	struct ref_array *array = cb->array;
+	struct ref_array_item *item;
 	struct commit *commit;
 	int kind, i;
 	const char *prefix, *orig_refname = refname;
@@ -360,59 +347,46 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
 	}
 
 	/* Don't add types the caller doesn't want */
-	if ((kind & ref_list->kinds) == 0)
+	if ((kind & filter->branch_kind) == 0)
 		return 0;
 
-	if (!match_patterns(cb->pattern, refname))
+	if (!match_patterns(filter->name_patterns, refname))
 		return 0;
 
 	commit = NULL;
-	if (ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER) {
+	if (filter->verbose || filter->with_commit || filter->merge != REF_FILTER_MERGED_NONE) {
 		commit = lookup_commit_reference_gently(oid->hash, 1);
 		if (!commit)
 			return 0;
 
 		/* Filter with with_commit if specified */
-		if (!is_descendant_of(commit, ref_list->with_commit))
+		if (!is_descendant_of(commit, filter->with_commit))
 			return 0;
 
-		if (merge_filter != NO_FILTER)
-			add_pending_object(&ref_list->revs,
+		if (filter->merge != REF_FILTER_MERGED_NONE)
+			add_pending_object(array->revs,
 					   (struct object *)commit, refname);
 	}
 
-	ALLOC_GROW(ref_list->list, ref_list->index + 1, ref_list->alloc);
+	item = ref_array_append(array, refname);
 
 	/* Record the new item */
-	newitem = &(ref_list->list[ref_list->index++]);
-	newitem->name = xstrdup(refname);
-	newitem->kind = kind;
-	newitem->commit = commit;
-	newitem->dest = resolve_symref(orig_refname, prefix);
-	newitem->ignore = 0;
+	item->kind = kind;
+	item->commit = commit;
+	item->symref = resolve_symref(orig_refname, prefix);
+	item->ignore = 0;
 
 	return 0;
 }
 
-static void free_ref_list(struct ref_list *ref_list)
-{
-	int i;
-
-	for (i = 0; i < ref_list->index; i++) {
-		free(ref_list->list[i].name);
-		free(ref_list->list[i].dest);
-	}
-	free(ref_list->list);
-}
-
 static int ref_cmp(const void *r1, const void *r2)
 {
-	struct ref_item *c1 = (struct ref_item *)(r1);
-	struct ref_item *c2 = (struct ref_item *)(r2);
+	struct ref_array_item *c1 = *((struct ref_array_item **)r1);
+	struct ref_array_item *c2 = *((struct ref_array_item **)r2);
 
 	if (c1->kind != c2->kind)
 		return c1->kind - c2->kind;
-	return strcmp(c1->name, c2->name);
+	return strcmp(c1->refname, c2->refname);
 }
 
 static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
@@ -477,8 +451,8 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
 	free(ref);
 }
 
-static void add_verbose_info(struct strbuf *out, struct ref_item *item,
-			     int verbose, int abbrev)
+static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
+			     struct ref_filter *filter)
 {
 	struct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;
 	const char *sub = _(" **** invalid ref ****");
@@ -490,10 +464,10 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
 	}
 
 	if (item->kind == REF_LOCAL_BRANCH)
-		fill_tracking_info(&stat, item->name, verbose > 1);
+		fill_tracking_info(&stat, item->refname, filter->verbose > 1);
 
 	strbuf_addf(out, " %s %s%s",
-		find_unique_abbrev(item->commit->object.sha1, abbrev),
+		find_unique_abbrev(item->commit->object.sha1, filter->abbrev),
 		stat.buf, sub);
 	strbuf_release(&stat);
 	strbuf_release(&subject);
@@ -530,22 +504,22 @@ static char *get_head_description(void)
 	return strbuf_detach(&desc, NULL);
 }
 
-static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
-			   int abbrev, int detached, const char *remote_prefix)
+static void print_ref_item(struct ref_array_item *item, int maxwidth,
+			   struct ref_filter *filter, const char *remote_prefix)
 {
 	char c;
 	int current = 0;
 	int color;
 	struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
 	const char *prefix = "";
-	const char *desc = item->name;
+	const char *desc = item->refname;
 
 	if (item->ignore)
 		return;
 
 	switch (item->kind) {
 	case REF_LOCAL_BRANCH:
-		if (!detached && !strcmp(item->name, head))
+		if (!filter->detached && !strcmp(item->refname, head))
 			current = 1;
 		else
 			color = BRANCH_COLOR_LOCAL;
@@ -570,7 +544,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 	}
 
 	strbuf_addf(&name, "%s%s", prefix, desc);
-	if (verbose) {
+	if (filter->verbose) {
 		int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);
 		strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
 			    maxwidth + utf8_compensation, name.buf,
@@ -579,13 +553,13 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 		strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(color),
 			    name.buf, branch_get_color(BRANCH_COLOR_RESET));
 
-	if (item->dest)
-		strbuf_addf(&out, " -> %s", item->dest);
-	else if (verbose)
+	if (item->symref)
+		strbuf_addf(&out, " -> %s", item->symref);
+	else if (filter->verbose)
 		/* " f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h" */
-		add_verbose_info(&out, item, verbose, abbrev);
+		add_verbose_info(&out, item, filter);
 	if (column_active(colopts)) {
-		assert(!verbose && "--column and --verbose are incompatible");
+		assert(!filter->verbose && "--column and --verbose are incompatible");
 		string_list_append(&output, out.buf);
 	} else {
 		printf("%s\n", out.buf);
@@ -596,14 +570,14 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 		free((void *)desc);
 }
 
-static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
+static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
 {
 	int i, max = 0;
-	for (i = 0; i < refs->index; i++) {
-		struct ref_item *it = &refs->list[i];
-		int w = utf8_strwidth(it->name);
+	for (i = 0; i < refs->nr; i++) {
+		struct ref_array_item *it = refs->items[i];
+		int w = utf8_strwidth(it->refname);
 
-		if (refs->list[i].ignore)
+		if (it->ignore)
 			continue;
 		if (it->kind == REF_REMOTE_BRANCH)
 			w += remote_bonus;
@@ -613,90 +587,81 @@ static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
 	return max;
 }
 
-static void print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)
+static void print_ref_list(struct ref_filter *filter)
 {
 	int i, index;
-	struct append_ref_cb cb;
-	struct ref_list ref_list;
+	struct ref_array array;
+	struct ref_filter_cbdata data;
 	int maxwidth = 0;
 	const char *remote_prefix = "";
+	struct rev_info revs;
 
 	/*
 	 * If we are listing more than just remote branches,
 	 * then remote branches will have a "remotes/" prefix.
 	 * We need to account for this in the width.
 	 */
-	if (kinds != REF_REMOTE_BRANCH)
+	if (filter->branch_kind != REF_REMOTE_BRANCH)
 		remote_prefix = "remotes/";
 
-	memset(&ref_list, 0, sizeof(ref_list));
-	ref_list.kinds = kinds;
-	ref_list.verbose = verbose;
-	ref_list.abbrev = abbrev;
-	ref_list.with_commit = with_commit;
-	if (merge_filter != NO_FILTER)
-		init_revisions(&ref_list.revs, NULL);
-	cb.ref_list = &ref_list;
-	cb.pattern = pattern;
+	memset(&array, 0, sizeof(array));
+	if (filter->merge != REF_FILTER_MERGED_NONE)
+		init_revisions(&revs, NULL);
+
+	data.array = &array;
+	data.filter = filter;
+	array.revs = &revs;
+
 	/*
 	 * First we obtain all regular branch refs and then if the
 	 * HEAD is detached then we insert that ref to the end of the
 	 * ref_fist so that it can be printed first.
 	 */
-	for_each_rawref(append_ref, &cb);
-	if (detached)
-		head_ref(append_ref, &cb);
+	for_each_rawref(append_ref, &data);
+	if (filter->detached)
+		head_ref(append_ref, &data);
 	/*
 	 * The following implementation is currently duplicated in ref-filter. It
 	 * will eventually be removed when we port branch.c to use ref-filter APIs.
 	 */
-	if (merge_filter != NO_FILTER) {
-		struct commit *filter;
-		filter = lookup_commit_reference_gently(merge_filter_ref, 0);
-		if (!filter)
-			die(_("object '%s' does not point to a commit"),
-			    sha1_to_hex(merge_filter_ref));
-
-		filter->object.flags |= UNINTERESTING;
-		add_pending_object(&ref_list.revs,
-				   (struct object *) filter, "");
-		ref_list.revs.limited = 1;
-
-		if (prepare_revision_walk(&ref_list.revs))
+	if (filter->merge != REF_FILTER_MERGED_NONE) {
+		filter->merge_commit->object.flags |= UNINTERESTING;
+		add_pending_object(&revs, &filter->merge_commit->object, "");
+		revs.limited = 1;
+
+		if (prepare_revision_walk(&revs))
 			die(_("revision walk setup failed"));
 
-		for (i = 0; i < ref_list.index; i++) {
-			struct ref_item *item = &ref_list.list[i];
+		for (i = 0; i < array.nr; i++) {
+			struct ref_array_item *item = array.items[i];
 			struct commit *commit = item->commit;
 			int is_merged = !!(commit->object.flags & UNINTERESTING);
-			item->ignore = is_merged != (merge_filter == SHOW_MERGED);
+			item->ignore = is_merged != (filter->merge == REF_FILTER_MERGED_INCLUDE);
 		}
 
-		for (i = 0; i < ref_list.index; i++) {
-			struct ref_item *item = &ref_list.list[i];
+		for (i = 0; i < array.nr; i++) {
+			struct ref_array_item *item = array.items[i];
 			clear_commit_marks(item->commit, ALL_REV_FLAGS);
 		}
-		clear_commit_marks(filter, ALL_REV_FLAGS);
+		clear_commit_marks(filter->merge_commit, ALL_REV_FLAGS);
 	}
-	if (verbose)
-		maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
+	if (filter->verbose)
+		maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
 
-	index = ref_list.index;
+	index = array.nr;
 
 	/* Print detached HEAD before sorting and printing the rest */
-	if (detached) {
-		print_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,
-			       detached, remote_prefix);
+	if (filter->detached) {
+		print_ref_item(array.items[index - 1], maxwidth, filter, remote_prefix);
 		index -= 1;
 	}
 
-	qsort(ref_list.list, index, sizeof(struct ref_item), ref_cmp);
+	qsort(array.items, index, sizeof(struct ref_array_item *), ref_cmp);
 
 	for (i = 0; i < index; i++)
-		print_ref_item(&ref_list.list[i], maxwidth, verbose,
-			       abbrev, detached, remote_prefix);
+		print_ref_item(array.items[i], maxwidth, filter, remote_prefix);
 
-	free_ref_list(&ref_list);
+	ref_array_clear(&array);
 }
 
 static void rename_branch(const char *oldname, const char *newname, int force)
@@ -752,24 +717,6 @@ static void rename_branch(const char *oldname, const char *newname, int force)
 	strbuf_release(&newsection);
 }
 
-/*
- * This function is duplicated in ref-filter. It will eventually be removed
- * when we port branch.c to use ref-filter APIs.
- */
-static int opt_parse_merge_filter(const struct option *opt, const char *arg, int unset)
-{
-	merge_filter = ((opt->long_name[0] == 'n')
-			? SHOW_NOT_MERGED
-			: SHOW_MERGED);
-	if (unset)
-		merge_filter = SHOW_NOT_MERGED; /* b/c for --no-merged */
-	if (!arg)
-		arg = "HEAD";
-	if (get_sha1(arg, merge_filter_ref))
-		die(_("malformed object name %s"), arg);
-	return 0;
-}
-
 static const char edit_description[] = "BRANCH_DESCRIPTION";
 
 static int edit_branch_description(const char *branch_name)
@@ -809,17 +756,15 @@ static int edit_branch_description(const char *branch_name)
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
 	int delete = 0, rename = 0, force = 0, list = 0;
-	int verbose = 0, abbrev = -1, detached = 0;
 	int reflog = 0, edit_description = 0;
 	int quiet = 0, unset_upstream = 0;
 	const char *new_upstream = NULL;
 	enum branch_track track;
-	int kinds = REF_LOCAL_BRANCH;
-	struct commit_list *with_commit = NULL;
+	struct ref_filter filter;
 
 	struct option options[] = {
 		OPT_GROUP(N_("Generic options")),
-		OPT__VERBOSE(&verbose,
+		OPT__VERBOSE(&filter.verbose,
 			N_("show hash and subject, give twice for upstream branch")),
 		OPT__QUIET(&quiet, N_("suppress informational messages")),
 		OPT_SET_INT('t', "track",  &track, N_("set up tracking mode (see git-pull(1))"),
@@ -829,14 +774,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_STRING('u', "set-upstream-to", &new_upstream, "upstream", "change the upstream info"),
 		OPT_BOOL(0, "unset-upstream", &unset_upstream, "Unset the upstream info"),
 		OPT__COLOR(&branch_use_color, N_("use colored output")),
-		OPT_SET_INT('r', "remotes",     &kinds, N_("act on remote-tracking branches"),
+		OPT_SET_INT('r', "remotes",     &filter.branch_kind, N_("act on remote-tracking branches"),
 			REF_REMOTE_BRANCH),
-		OPT_CONTAINS(&with_commit, N_("print only branches that contain the commit")),
-		OPT_WITH(&with_commit, N_("print only branches that contain the commit")),
-		OPT__ABBREV(&abbrev),
+		OPT_CONTAINS(&filter.with_commit, N_("print only branches that contain the commit")),
+		OPT_WITH(&filter.with_commit, N_("print only branches that contain the commit")),
+		OPT__ABBREV(&filter.abbrev),
 
 		OPT_GROUP(N_("Specific git-branch actions:")),
-		OPT_SET_INT('a', "all", &kinds, N_("list both remote-tracking and local branches"),
+		OPT_SET_INT('a', "all", &filter.branch_kind, N_("list both remote-tracking and local branches"),
 			REF_REMOTE_BRANCH | REF_LOCAL_BRANCH),
 		OPT_BIT('d', "delete", &delete, N_("delete fully merged branch"), 1),
 		OPT_BIT('D', NULL, &delete, N_("delete branch (even if not merged)"), 2),
@@ -847,22 +792,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "edit-description", &edit_description,
 			 N_("edit the description for the branch")),
 		OPT__FORCE(&force, N_("force creation, move/rename, deletion")),
-		{
-			OPTION_CALLBACK, 0, "no-merged", &merge_filter_ref,
-			N_("commit"), N_("print only not merged branches"),
-			PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG,
-			opt_parse_merge_filter, (intptr_t) "HEAD",
-		},
-		{
-			OPTION_CALLBACK, 0, "merged", &merge_filter_ref,
-			N_("commit"), N_("print only merged branches"),
-			PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG,
-			opt_parse_merge_filter, (intptr_t) "HEAD",
-		},
+		OPT_MERGED(&filter, N_("print only branches that are merged")),
+		OPT_NO_MERGED(&filter, N_("print only branches that are not merged")),
 		OPT_COLUMN(0, "column", &colopts, N_("list branches in columns")),
 		OPT_END(),
 	};
 
+	memset(&filter, 0, sizeof(filter));
+	filter.branch_kind = REF_LOCAL_BRANCH;
+	filter.abbrev = -1;
+
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_branch_usage, options);
 
@@ -874,11 +813,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (!head)
 		die(_("Failed to resolve HEAD as a valid ref."));
 	if (!strcmp(head, "HEAD"))
-		detached = 1;
+		filter.detached = 1;
 	else if (!skip_prefix(head, "refs/heads/", &head))
 		die(_("HEAD not found below refs/heads!"));
-	hashcpy(merge_filter_ref, head_sha1);
-
 
 	argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
 			     0);
@@ -886,17 +823,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)
 		list = 1;
 
-	if (with_commit || merge_filter != NO_FILTER)
+	if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE)
 		list = 1;
 
 	if (!!delete + !!rename + !!new_upstream +
 	    list + unset_upstream > 1)
 		usage_with_options(builtin_branch_usage, options);
 
-	if (abbrev == -1)
-		abbrev = DEFAULT_ABBREV;
+	if (filter.abbrev == -1)
+		filter.abbrev = DEFAULT_ABBREV;
 	finalize_colopts(&colopts, -1);
-	if (verbose) {
+	if (filter.verbose) {
 		if (explicitly_enable_column(colopts))
 			die(_("--column and --verbose are incompatible"));
 		colopts = 0;
@@ -910,13 +847,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (delete) {
 		if (!argc)
 			die(_("branch name required"));
-		return delete_branches(argc, argv, delete > 1, kinds, quiet);
+		return delete_branches(argc, argv, delete > 1, filter.branch_kind, quiet);
 	} else if (list) {
 		/*  git branch --local also shows HEAD when it is detached */
-		if (kinds & REF_LOCAL_BRANCH)
-			kinds |= REF_DETACHED_HEAD;
-		print_ref_list(kinds, detached, verbose, abbrev,
-					 with_commit, argv);
+		if (filter.branch_kind & REF_LOCAL_BRANCH)
+			filter.branch_kind |= REF_DETACHED_HEAD;
+		filter.name_patterns = argv;
+		print_ref_list(&filter);
 		print_columns(&output, colopts, NULL);
 		string_list_clear(&output, 0);
 		return 0;
@@ -926,7 +863,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		struct strbuf branch_ref = STRBUF_INIT;
 
 		if (!argc) {
-			if (detached)
+			if (filter.detached)
 				die(_("Cannot give description to detached HEAD"));
 			branch_name = head;
 		} else if (argc == 1)
@@ -1014,7 +951,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (!branch)
 			die(_("no such branch '%s'"), argv[0]);
 
-		if (kinds != REF_LOCAL_BRANCH)
+		if (filter.branch_kind != REF_LOCAL_BRANCH)
 			die(_("-a and -r options to 'git branch' do not make sense with a branch name"));
 
 		if (track == BRANCH_TRACK_OVERRIDE)
diff --git a/ref-filter.h b/ref-filter.h
index b5a13e8..1b1e6de 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -5,6 +5,7 @@
 #include "refs.h"
 #include "commit.h"
 #include "parse-options.h"
+#include "revision.h"
 
 /* Quoting styles */
 #define QUOTE_NONE 0
@@ -57,6 +58,7 @@ struct ref_sorting {
 struct ref_array_item {
 	unsigned char objectname[20];
 	int flag, kind;
+	int ignore : 1; /* To be removed in the next patch */
 	const char *symref;
 	struct commit *commit;
 	struct atom_value *value;
@@ -66,6 +68,7 @@ struct ref_array_item {
 struct ref_array {
 	int nr, alloc;
 	struct ref_array_item **items;
+	struct rev_info *revs;
 };
 
 struct ref_filter {
@@ -81,8 +84,10 @@ struct ref_filter {
 	struct commit *merge_commit;
 
 	unsigned int with_commit_tag_algo : 1,
-		match_as_path : 1;
+		match_as_path : 1,
+		detached : 1;
 	unsigned int lines, branch_kind;
+	int abbrev, verbose;
 };
 
 struct ref_filter_cbdata {
-- 
2.5.0

[PATCH 08/10] branch.c: use 'ref-filter' APIs

[Karthik Nayak]

Make 'branch.c' use 'ref-filter' APIs for iterating through refs
sorting. This removes most of the code used in 'branch.c' replacing it
with calls to the 'ref-filter' library.

Make 'branch.c' use the 'filter_refs()' function provided by 'ref-filter'
to filter out tags based on the options set.

We provide a sorting option provided for 'branch.c' by using the sorting
options provided by 'ref-filter'.

Also remove the 'ignore' variable from ref_array_item as it was
previously used for the '--merged' option and now that is handled by
ref-filter.

The test t1430 'git branch shows badly named ref' fails as this checks
the ability of branch.c to list broken refs, which is now displayed as
a warning while using ref-filter APIs.

Modify documentation and add 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-branch.txt |   9 +-
 builtin/branch.c             | 219 ++++++++-----------------------------------
 ref-filter.c                 |   8 +-
 ref-filter.h                 |   1 -
 t/t1430-bad-ref-name.sh      |  10 +-
 t/t3203-branch-output.sh     |  11 +++
 6 files changed, 68 insertions(+), 190 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index a67138a..897cd81 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git branch' [--color[=<when>] | --no-color] [-r | -a]
 	[--list] [-v [--abbrev=<length> | --no-abbrev]]
 	[--column[=<options>] | --no-column]
-	[(--merged | --no-merged | --contains) [<commit>]] [<pattern>...]
+	[(--merged | --no-merged | --contains) [<commit>]] [--sort=<key>] [<pattern>...]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]
 'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]
 'git branch' --unset-upstream [<branchname>]
@@ -229,6 +229,13 @@ start-point is either a local or remote-tracking branch.
 	The new name for an existing branch. The same restrictions as for
 	<branchname> apply.
 
+--sort=<key>::
+	Sort based on the key given. Prefix `-` to sort in descending
+	order of the value. You may use the --sort=<key> option
+	multiple times, in which case the last key becomes the primary
+	key. The keys supported are the same as those in `git
+	for-each-ref`. Sort order defaults to sorting based on branch
+	type.
 
 Examples
 --------
diff --git a/builtin/branch.c b/builtin/branch.c
index edc3d7d..34ccf0c 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -270,125 +270,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 	return(ret);
 }
 
-static char *resolve_symref(const char *src, const char *prefix)
-{
-	unsigned char sha1[20];
-	int flag;
-	const char *dst;
-
-	dst = resolve_ref_unsafe(src, 0, sha1, &flag);
-	if (!(dst && (flag & REF_ISSYMREF)))
-		return NULL;
-	if (prefix)
-		skip_prefix(dst, prefix, &dst);
-	return xstrdup(dst);
-}
-
-static int match_patterns(const char **pattern, const char *refname)
-{
-	if (!*pattern)
-		return 1; /* no pattern always matches */
-	while (*pattern) {
-		if (!wildmatch(*pattern, refname, 0, NULL))
-			return 1;
-		pattern++;
-	}
-	return 0;
-}
-
-/*
- * Allocate memory for a new ref_array_item and insert that into the
- * given ref_array. Doesn't take the objectname unlike
- * new_ref_array_item(). This is a temporary function which will be
- * removed when we port branch.c to use ref-filter APIs.
- */
-static struct ref_array_item *ref_array_append(struct ref_array *array, const char *refname)
-{
-	size_t len = strlen(refname);
-	struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
-	memcpy(ref->refname, refname, len);
-	ref->refname[len] = '\0';
-	REALLOC_ARRAY(array->items, array->nr + 1);
-	array->items[array->nr++] = ref;
-	return ref;
-}
-
-static int append_ref(const char *refname, const struct object_id *oid, int flags, void *cb_data)
-{
-	struct ref_filter_cbdata *cb = (struct ref_filter_cbdata *)(cb_data);
-	struct ref_filter *filter = cb->filter;
-	struct ref_array *array = cb->array;
-	struct ref_array_item *item;
-	struct commit *commit;
-	int kind, i;
-	const char *prefix, *orig_refname = refname;
-
-	static struct {
-		int kind;
-		const char *prefix;
-	} ref_kind[] = {
-		{ REF_LOCAL_BRANCH, "refs/heads/" },
-		{ REF_REMOTE_BRANCH, "refs/remotes/" },
-	};
-
-	/* Detect kind */
-	for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
-		prefix = ref_kind[i].prefix;
-		if (skip_prefix(refname, prefix, &refname)) {
-			kind = ref_kind[i].kind;
-			break;
-		}
-	}
-	if (ARRAY_SIZE(ref_kind) <= i) {
-		if (!strcmp(refname, "HEAD"))
-			kind = REF_DETACHED_HEAD;
-		else
-			return 0;
-	}
-
-	/* Don't add types the caller doesn't want */
-	if ((kind & filter->branch_kind) == 0)
-		return 0;
-
-	if (!match_patterns(filter->name_patterns, refname))
-		return 0;
-
-	commit = NULL;
-	if (filter->verbose || filter->with_commit || filter->merge != REF_FILTER_MERGED_NONE) {
-		commit = lookup_commit_reference_gently(oid->hash, 1);
-		if (!commit)
-			return 0;
-
-		/* Filter with with_commit if specified */
-		if (!is_descendant_of(commit, filter->with_commit))
-			return 0;
-
-		if (filter->merge != REF_FILTER_MERGED_NONE)
-			add_pending_object(array->revs,
-					   (struct object *)commit, refname);
-	}
-
-	item = ref_array_append(array, refname);
-
-	/* Record the new item */
-	item->kind = kind;
-	item->commit = commit;
-	item->symref = resolve_symref(orig_refname, prefix);
-	item->ignore = 0;
-
-	return 0;
-}
-
-static int ref_cmp(const void *r1, const void *r2)
-{
-	struct ref_array_item *c1 = *((struct ref_array_item **)r1);
-	struct ref_array_item *c2 = *((struct ref_array_item **)r2);
-
-	if (c1->kind != c2->kind)
-		return c1->kind - c2->kind;
-	return strcmp(c1->refname, c2->refname);
-}
-
 static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
 		int show_upstream_ref)
 {
@@ -452,7 +333,7 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
 }
 
 static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
-			     struct ref_filter *filter)
+			     struct ref_filter *filter, const char *refname)
 {
 	struct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;
 	const char *sub = _(" **** invalid ref ****");
@@ -464,7 +345,7 @@ static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
 	}
 
 	if (item->kind == REF_LOCAL_BRANCH)
-		fill_tracking_info(&stat, item->refname, filter->verbose > 1);
+		fill_tracking_info(&stat, refname, filter->verbose > 1);
 
 	strbuf_addf(out, " %s %s%s",
 		find_unique_abbrev(item->commit->object.sha1, filter->abbrev),
@@ -504,8 +385,8 @@ static char *get_head_description(void)
 	return strbuf_detach(&desc, NULL);
 }
 
-static void print_ref_item(struct ref_array_item *item, int maxwidth,
-			   struct ref_filter *filter, const char *remote_prefix)
+static void format_and_print_ref_item(struct ref_array_item *item, int maxwidth,
+				      struct ref_filter *filter, const char *remote_prefix)
 {
 	char c;
 	int current = 0;
@@ -514,17 +395,16 @@ static void print_ref_item(struct ref_array_item *item, int maxwidth,
 	const char *prefix = "";
 	const char *desc = item->refname;
 
-	if (item->ignore)
-		return;
-
 	switch (item->kind) {
 	case REF_LOCAL_BRANCH:
-		if (!filter->detached && !strcmp(item->refname, head))
+		skip_prefix(desc, "refs/heads/", &desc);
+		if (!filter->detached && !strcmp(desc, head))
 			current = 1;
 		else
 			color = BRANCH_COLOR_LOCAL;
 		break;
 	case REF_REMOTE_BRANCH:
+		skip_prefix(desc, "refs/remotes/", &desc);
 		color = BRANCH_COLOR_REMOTE;
 		prefix = remote_prefix;
 		break;
@@ -553,11 +433,13 @@ static void print_ref_item(struct ref_array_item *item, int maxwidth,
 		strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(color),
 			    name.buf, branch_get_color(BRANCH_COLOR_RESET));
 
-	if (item->symref)
-		strbuf_addf(&out, " -> %s", item->symref);
+	if (item->symref) {
+		skip_prefix(item->symref, "refs/remotes/", &desc);
+		strbuf_addf(&out, " -> %s", desc);
+	}
 	else if (filter->verbose)
 		/* " f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h" */
-		add_verbose_info(&out, item, filter);
+		add_verbose_info(&out, item, filter, desc);
 	if (column_active(colopts)) {
 		assert(!filter->verbose && "--column and --verbose are incompatible");
 		string_list_append(&output, out.buf);
@@ -575,10 +457,13 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
 	int i, max = 0;
 	for (i = 0; i < refs->nr; i++) {
 		struct ref_array_item *it = refs->items[i];
-		int w = utf8_strwidth(it->refname);
+		const char *desc = it->refname;
+		int w;
+
+		skip_prefix(it->refname, "refs/heads/", &desc);
+		skip_prefix(it->refname, "refs/remotes/", &desc);
+		w = utf8_strwidth(desc);
 
-		if (it->ignore)
-			continue;
 		if (it->kind == REF_REMOTE_BRANCH)
 			w += remote_bonus;
 		if (w > max)
@@ -587,14 +472,14 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
 	return max;
 }
 
-static void print_ref_list(struct ref_filter *filter)
+static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting)
 {
 	int i, index;
 	struct ref_array array;
-	struct ref_filter_cbdata data;
 	int maxwidth = 0;
 	const char *remote_prefix = "";
-	struct rev_info revs;
+	struct ref_sorting def_sorting;
+	const char *sort_type = "type";
 
 	/*
 	 * If we are listing more than just remote branches,
@@ -605,46 +490,10 @@ static void print_ref_list(struct ref_filter *filter)
 		remote_prefix = "remotes/";
 
 	memset(&array, 0, sizeof(array));
-	if (filter->merge != REF_FILTER_MERGED_NONE)
-		init_revisions(&revs, NULL);
-
-	data.array = &array;
-	data.filter = filter;
-	array.revs = &revs;
 
-	/*
-	 * First we obtain all regular branch refs and then if the
-	 * HEAD is detached then we insert that ref to the end of the
-	 * ref_fist so that it can be printed first.
-	 */
-	for_each_rawref(append_ref, &data);
-	if (filter->detached)
-		head_ref(append_ref, &data);
-	/*
-	 * The following implementation is currently duplicated in ref-filter. It
-	 * will eventually be removed when we port branch.c to use ref-filter APIs.
-	 */
-	if (filter->merge != REF_FILTER_MERGED_NONE) {
-		filter->merge_commit->object.flags |= UNINTERESTING;
-		add_pending_object(&revs, &filter->merge_commit->object, "");
-		revs.limited = 1;
-
-		if (prepare_revision_walk(&revs))
-			die(_("revision walk setup failed"));
-
-		for (i = 0; i < array.nr; i++) {
-			struct ref_array_item *item = array.items[i];
-			struct commit *commit = item->commit;
-			int is_merged = !!(commit->object.flags & UNINTERESTING);
-			item->ignore = is_merged != (filter->merge == REF_FILTER_MERGED_INCLUDE);
-		}
+	verify_ref_format("%(refname)%(symref)");
+	filter_refs(&array, filter, FILTER_REFS_BRANCHES);
 
-		for (i = 0; i < array.nr; i++) {
-			struct ref_array_item *item = array.items[i];
-			clear_commit_marks(item->commit, ALL_REV_FLAGS);
-		}
-		clear_commit_marks(filter->merge_commit, ALL_REV_FLAGS);
-	}
 	if (filter->verbose)
 		maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
 
@@ -652,15 +501,22 @@ static void print_ref_list(struct ref_filter *filter)
 
 	/* Print detached HEAD before sorting and printing the rest */
 	if (filter->detached) {
-		print_ref_item(array.items[index - 1], maxwidth, filter, remote_prefix);
-		index -= 1;
+		format_and_print_ref_item(array.items[index - 1], maxwidth, filter, remote_prefix);
+		array.nr--;
 	}
 
-	qsort(array.items, index, sizeof(struct ref_array_item *), ref_cmp);
+	if (!sorting) {
+		def_sorting.next = NULL;
+		def_sorting.atom = parse_ref_filter_atom(sort_type,
+							 sort_type + strlen(sort_type));
+		sorting = &def_sorting;
+	}
+	ref_array_sort(sorting, &array);
 
-	for (i = 0; i < index; i++)
-		print_ref_item(array.items[i], maxwidth, filter, remote_prefix);
+	for (i = 0; i < array.nr; i++)
+		format_and_print_ref_item(array.items[i], maxwidth, filter, remote_prefix);
 
+	array.nr = index;
 	ref_array_clear(&array);
 }
 
@@ -761,6 +617,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	const char *new_upstream = NULL;
 	enum branch_track track;
 	struct ref_filter filter;
+	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 
 	struct option options[] = {
 		OPT_GROUP(N_("Generic options")),
@@ -795,6 +652,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_MERGED(&filter, N_("print only branches that are merged")),
 		OPT_NO_MERGED(&filter, N_("print only branches that are not merged")),
 		OPT_COLUMN(0, "column", &colopts, N_("list branches in columns")),
+		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		OPT_END(),
 	};
 
@@ -853,7 +712,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (filter.branch_kind & REF_LOCAL_BRANCH)
 			filter.branch_kind |= REF_DETACHED_HEAD;
 		filter.name_patterns = argv;
-		print_ref_list(&filter);
+		print_ref_list(&filter, sorting);
 		print_columns(&output, colopts, NULL);
 		string_list_clear(&output, 0);
 		return 0;
diff --git a/ref-filter.c b/ref-filter.c
index c573109..d83d4d2 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1135,7 +1135,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * obtain the commit using the 'oid' available and discard all
 	 * non-commits early. The actual filtering is done later.
 	 */
-	if (filter->merge_commit || filter->with_commit) {
+	if (filter->merge_commit || filter->with_commit || filter->verbose) {
 		commit = lookup_commit_reference_gently(oid->hash, 1);
 		if (!commit)
 			return 0;
@@ -1243,9 +1243,11 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 		ret = for_each_ref(ref_filter_handler, &ref_cbdata);
 	else if (type & FILTER_REFS_TAGS)
 		ret = for_each_tag_ref_fullpath(ref_filter_handler, &ref_cbdata);
-	else if (type & FILTER_REFS_BRANCHES)
+	else if (type & FILTER_REFS_BRANCHES) {
 		ret = for_each_rawref(ref_filter_handler, &ref_cbdata);
-	else if (type)
+		if (filter->detached)
+			head_ref(ref_filter_handler, &ref_cbdata);
+	} else if (type)
 		die("filter_refs: invalid type");
 
 	/*  Filters that need revision walking */
diff --git a/ref-filter.h b/ref-filter.h
index 1b1e6de..4379741 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -58,7 +58,6 @@ struct ref_sorting {
 struct ref_array_item {
 	unsigned char objectname[20];
 	int flag, kind;
-	int ignore : 1; /* To be removed in the next patch */
 	const char *symref;
 	struct commit *commit;
 	struct atom_value *value;
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index 16d0b8b..dcf2931 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -38,11 +38,11 @@ test_expect_success 'fast-import: fail on invalid branch name "bad[branch]name"'
 	test_must_fail git fast-import <input
 '
 
-test_expect_success 'git branch shows badly named ref' '
-	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
-	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
-	git branch >output &&
-	grep -e "broken\.\.\.ref" output
+test_expect_failure 'git branch shows badly named ref' '
+       cp .git/refs/heads/master .git/refs/heads/broken...ref &&
+       test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+       git branch >output &&
+       grep -e "broken\.\.\.ref" output
 '
 
 test_expect_success 'branch -d can delete badly named ref' '
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index f51d0f3..38c68bd 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -143,4 +143,15 @@ EOF
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'git branch `--sort` option' '
+	cat >expect <<EOF &&
+* (HEAD detached from fromtag)
+  branch-two
+  branch-one
+  master
+EOF
+	git branch --sort=objectsize >actual &&
+	test_i18ncmp expect actual
+'
+
 test_done
-- 
2.5.0

[PATCH 09/10] branch: add '--points-at' option

[Karthik Nayak]

Add the '--points-at' option provided by 'ref-filter'. The option lets
the user to list only branches which points at the given 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-branch.txt | 6 +++++-
 builtin/branch.c             | 7 ++++++-
 t/t3203-branch-output.sh     | 9 +++++++++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 897cd81..211cfc3 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -11,7 +11,8 @@ SYNOPSIS
 'git branch' [--color[=<when>] | --no-color] [-r | -a]
 	[--list] [-v [--abbrev=<length> | --no-abbrev]]
 	[--column[=<options>] | --no-column]
-	[(--merged | --no-merged | --contains) [<commit>]] [--sort=<key>] [<pattern>...]
+	[(--merged | --no-merged | --contains) [<commit>]] [--sort=<key>]
+	[--points-at <object>] [<pattern>...]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]
 'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]
 'git branch' --unset-upstream [<branchname>]
@@ -237,6 +238,9 @@ start-point is either a local or remote-tracking branch.
 	for-each-ref`. Sort order defaults to sorting based on branch
 	type.
 
+--points-at <object>::
+	Only list branches of the given object.
+
 Examples
 --------
 
diff --git a/builtin/branch.c b/builtin/branch.c
index 34ccf0c..5dad1da 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -26,6 +26,7 @@ static const char * const builtin_branch_usage[] = {
 	N_("git branch [<options>] [-l] [-f] <branch-name> [<start-point>]"),
 	N_("git branch [<options>] [-r] (-d | -D) <branch-name>..."),
 	N_("git branch [<options>] (-m | -M) [<old-branch>] <new-branch>"),
+	N_("git branch [<options>] [-r | -a] [--points-at]"),
 	NULL
 };
 
@@ -654,6 +655,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_COLUMN(0, "column", &colopts, N_("list branches in columns")),
 		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
 			     N_("field name to sort on"), &parse_opt_ref_sorting),
+		{
+			OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
+			N_("print only branches of the object"), 0, parse_opt_object_name
+		},
 		OPT_END(),
 	};
 
@@ -682,7 +687,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)
 		list = 1;
 
-	if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE)
+	if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)
 		list = 1;
 
 	if (!!delete + !!rename + !!new_upstream +
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 38c68bd..1deb7cb 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -154,4 +154,13 @@ EOF
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'git branch --points-at option' '
+	cat >expect <<EOF &&
+  master
+  branch-one
+EOF
+	git branch --points-at=branch-one >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.5.0

Re: [PATCH 0/10] Port branch.c to ref-filter.

[Karthik Nayak]

There are nine patches in the series. Have put "0/10" by mistake.

-- 
Regards,
Karthik Nayak

Re: [PATCH 0/10] Port branch.c to ref-filter.

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> There are nine patches in the series. Have put "0/10" by mistake.

FYI, format-patch has --cover-letter option.

Re: [PATCH 0/10] Port branch.c to ref-filter.

[Karthik Nayak]

On Thu, Aug 6, 2015 at 3:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> There are nine patches in the series. Have put "0/10" by mistake.
>
> FYI, format-patch has --cover-letter option.

Thanks! I need to check out a lot of options :)



-- 
Regards,
Karthik Nayak