[PATCH] Add support to git-branch to show local and remote branches

[PATCH] Add support to git-branch to show local and remote branches

[Andy Parkins]

Instead of storing a list of refnames in append_ref, a list of structures is
created.  Each of these stores the refname and a symbolic constant representing
its type.

The creation of the list is filtered based on a command line switch; no switch
means "local branches only", "-r" means "remote branches only" (as they always
did); but now "-a" means "local branches or remote branches".

As a side effect, the list is now not global, but allocated in print_ref_list()
where it used.

Also a memory leak is plugged, the memory allocated during the list creation
was never freed.  This is now done in the new function, tidy_ref_list()

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
 builtin-branch.c |   97 +++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 78 insertions(+), 19 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 368b68e..9e0be22 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -11,7 +11,7 @@
 #include "builtin.h"
 
 static const char builtin_branch_usage[] =
-"git-branch (-d | -D) <branchname> | [-l] [-f] <branchname> [<start-point>] | [-r]";
+"git-branch (-d | -D) <branchname> | [-l] [-f] <branchname> [<start-point>] | [-r] | [-a]";
 
 
 static const char *head;
@@ -79,46 +79,100 @@ static void delete_branches(int argc, co
 	}
 }
 
-static int ref_index, ref_alloc;
-static char **ref_list;
+#define REF_UNKNOWN_TYPE    0x00
+#define REF_LOCAL_BRANCH    0x01
+#define REF_REMOTE_BRANCH   0x02
+#define REF_TAG             0x04
+
+struct ref_item {
+	char *name;
+	unsigned int kind;
+};
+
+struct ref_list {
+	int index, alloc;
+	struct ref_item *list;
+	int kinds;
+};
 
 static int append_ref(const char *refname, const unsigned char *sha1, int flags,
 		void *cb_data)
 {
-	if (ref_index >= ref_alloc) {
-		ref_alloc = alloc_nr(ref_alloc);
-		ref_list = xrealloc(ref_list, ref_alloc * sizeof(char *));
+	struct ref_list *ref_list = (struct ref_list*)(cb_data);
+	struct ref_item *newitem;
+	int kind = REF_UNKNOWN_TYPE;
+
+	/* Detect kind */
+	if (!strncmp(refname, "refs/heads/", 11)) {
+		kind = REF_LOCAL_BRANCH;
+		refname += 11;
+	} else if (!strncmp(refname, "refs/remotes/", 13)) {
+		kind = REF_REMOTE_BRANCH;
+		refname += 13;
+	} else if (!strncmp(refname, "refs/tags/", 10)) {
+		kind = REF_TAG;
+		refname += 10;
+	}
+
+	/* Don't add types the caller doesn't want */
+	if ((kind & ref_list->kinds) == 0)
+		return 0;
+
+	/* Resize buffer */
+	if (ref_list->index >= ref_list->alloc) {
+		ref_list->alloc = alloc_nr(ref_list->alloc);
+		ref_list->list = xrealloc(ref_list->list,
+				ref_list->alloc * sizeof(struct ref_item));
 	}
 
-	ref_list[ref_index++] = xstrdup(refname);
+	/* Record the new item */
+	newitem = &(ref_list->list[ref_list->index++]);
+	newitem->name = xstrdup(refname);
+	newitem->kind = kind;
 
 	return 0;
 }
 
+static void tidy_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 );
+}
+
 static int ref_cmp(const void *r1, const void *r2)
 {
+	struct ref_item *c1 = (struct ref_item*)(r1),
+					*c2 = (struct ref_item*)(r2);
+	if( c1->kind != c2->kind )
+		return c1->kind - c2->kind;
 	return strcmp(*(char **)r1, *(char **)r2);
 }
 
-static void print_ref_list(int remote_only)
+static void print_ref_list( int kinds )
 {
 	int i;
 	char c;
+	struct ref_list ref_list;
 
-	if (remote_only)
-		for_each_remote_ref(append_ref, NULL);
-	else
-		for_each_branch_ref(append_ref, NULL);
+	memset( &ref_list, 0, sizeof( ref_list ) );
+	ref_list.kinds = kinds;
+	for_each_ref(append_ref, &ref_list);
 
-	qsort(ref_list, ref_index, sizeof(char *), ref_cmp);
+	qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
 
-	for (i = 0; i < ref_index; i++) {
+	for (i = 0; i < ref_list.index; i++) {
 		c = ' ';
-		if (!strcmp(ref_list[i], head))
+		if (ref_list.list[i].kind == REF_LOCAL_BRANCH &&
+				!strcmp(ref_list.list[i].name, head))
 			c = '*';
 
-		printf("%c %s\n", c, ref_list[i]);
+		printf("%c %s\n", c, ref_list.list[i].name );
 	}
+
+	tidy_ref_list( &ref_list );
 }
 
 static void create_branch(const char *name, const char *start,
@@ -160,8 +214,9 @@ static void create_branch(const char *na
 
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
-	int delete = 0, force_delete = 0, force_create = 0, remote_only = 0;
+	int delete = 0, force_delete = 0, force_create = 0;
 	int reflog = 0;
+	int kinds = REF_LOCAL_BRANCH;
 	int i;
 
 	git_config(git_default_config);
@@ -189,7 +244,11 @@ int cmd_branch(int argc, const char **ar
 			continue;
 		}
 		if (!strcmp(arg, "-r")) {
-			remote_only = 1;
+			kinds = REF_REMOTE_BRANCH;
+			continue;
+		}
+		if (!strcmp(arg, "-a")) {
+			kinds = REF_REMOTE_BRANCH | REF_LOCAL_BRANCH;
 			continue;
 		}
 		if (!strcmp(arg, "-l")) {
@@ -209,7 +268,7 @@ int cmd_branch(int argc, const char **ar
 	if (delete)
 		delete_branches(argc - i, argv + i, force_delete);
 	else if (i == argc)
-		print_ref_list(remote_only);
+		print_ref_list(kinds);
 	else if (i == argc - 1)
 		create_branch(argv[i], head, force_create, reflog);
 	else if (i == argc - 2)
-- 
1.4.3.5

Re: [PATCH] Add support to git-branch to show local and remote branches

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> Instead of storing a list of refnames in append_ref, a list of
> structures is created.  Each of these stores the refname and a
> symbolic constant representing its type.

Thanks.  I'll drop the one in 'pu' and will replace with this
patch but with style fixes and keeping the documentation updates
from there.

Re: [PATCH] Add support to git-branch to show local and remote branches

[Andy Parkins]

On Tuesday 2006 November 21 21:14, Junio C Hamano wrote:

> Thanks.  I'll drop the one in 'pu' and will replace with this
> patch but with style fixes and keeping the documentation updates
> from there.

Sorry; I must remember to write documentation.  I'll be sure to look at the 
style changes and try to adhere to that style in future.

Andy
-- 
Dr Andy Parkins, M Eng (hons), MIEE

Re: [PATCH 2/4] Rename remote_only to display_mode

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> -static void print_ref_list(int remote_only)
> +static void print_ref_list(int display_mode)
>  {
>  	int i;
>  	char c;
>  
> -	if (remote_only)
> +	if (display_mode)
>  		for_each_remote_ref(append_ref, NULL);
>  	else
>  		for_each_branch_ref(append_ref, NULL);

If you make this a "mode", it probably is better to make 1 and 0
into symbolic constants.  This patch taken alone is regression
in readability.

By the way, it might make sense to make it bitfields; that would
allow you to show either one kind or both.

Something like this untested patch, that is...


diff --git a/builtin-branch.c b/builtin-branch.c
index 368b68e..182648c 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -79,45 +79,73 @@ static void delete_branches(int argc, co
 	}
 }
 
+#define REF_LOCAL_BRANCH	01
+#define REF_REMOTE_BRANCH	02
+
 static int ref_index, ref_alloc;
-static char **ref_list;
+static struct ref_list {
+	int kind;
+	char name[FLEX_ARRAY];
+} **ref_list;
 
-static int append_ref(const char *refname, const unsigned char *sha1, int flags,
-		void *cb_data)
+static int append_ref(const char *refname, const unsigned char *sha1,
+		      int flags, void *cb_data)
 {
+	int kinds = *((int*)cb_data);
+	int this_kind, strip;
+	struct ref_list *elem;
+
+	if (!strncmp(refname, "refs/heads/", 11)) {
+		this_kind = REF_LOCAL_BRANCH;
+		strip = 11;
+	}
+	else if (!strncmp(refname, "refs/remotes/", 13)) {
+		this_kind = REF_REMOTE_BRANCH;
+		strip = 13;
+	}
+	else
+		this_kind = 0;
+
+	if ((this_kind & kinds) == 0)
+		return 0;
+
 	if (ref_index >= ref_alloc) {
 		ref_alloc = alloc_nr(ref_alloc);
 		ref_list = xrealloc(ref_list, ref_alloc * sizeof(char *));
 	}
-
-	ref_list[ref_index++] = xstrdup(refname);
+	
+	elem = xcalloc(1, sizeof(*elem) + strlen(refname) - strip);
+	strcpy(elem->name, refname + strip); 
+	elem->kind = this_kind;
+	ref_list[ref_index++] = elem;
 
 	return 0;
 }
 
-static int ref_cmp(const void *r1, const void *r2)
+static int ref_cmp(const void *r1_, const void *r2_)
 {
-	return strcmp(*(char **)r1, *(char **)r2);
+	const struct ref_list *r1 = *((const struct ref_list **)r1_);
+	const struct ref_list *r2 = *((const struct ref_list **)r2_);
+
+	if (r1->kind != r2->kind)
+		return r1->kind - r2->kind;
+	return strcmp(r1->name, r2->name);
 }
 
-static void print_ref_list(int remote_only)
+static void print_ref_list(int kinds)
 {
 	int i;
 	char c;
 
-	if (remote_only)
-		for_each_remote_ref(append_ref, NULL);
-	else
-		for_each_branch_ref(append_ref, NULL);
-
+	for_each_ref(append_ref, &kinds);
 	qsort(ref_list, ref_index, sizeof(char *), ref_cmp);
 
 	for (i = 0; i < ref_index; i++) {
 		c = ' ';
-		if (!strcmp(ref_list[i], head))
+		if (ref_list[i]->kind == REF_LOCAL_BRANCH &&
+		    !strcmp(ref_list[i]->name, head))
 			c = '*';
-
-		printf("%c %s\n", c, ref_list[i]);
+		printf("%c %s\n", c, ref_list[i]->name);
 	}
 }
 
@@ -160,8 +188,9 @@ static void create_branch(const char *na
 
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
-	int delete = 0, force_delete = 0, force_create = 0, remote_only = 0;
+	int delete = 0, force_delete = 0, force_create = 0;
 	int reflog = 0;
+	int kinds = REF_LOCAL_BRANCH;
 	int i;
 
 	git_config(git_default_config);
@@ -189,7 +218,11 @@ int cmd_branch(int argc, const char **ar
 			continue;
 		}
 		if (!strcmp(arg, "-r")) {
-			remote_only = 1;
+			kinds = REF_REMOTE_BRANCH;
+			continue;
+		}
+		if (!strcmp(arg, "-a")) {
+			kinds = REF_REMOTE_BRANCH | REF_LOCAL_BRANCH;
 			continue;
 		}
 		if (!strcmp(arg, "-l")) {
@@ -209,7 +242,7 @@ int cmd_branch(int argc, const char **ar
 	if (delete)
 		delete_branches(argc - i, argv + i, force_delete);
 	else if (i == argc)
-		print_ref_list(remote_only);
+		print_ref_list(kinds);
 	else if (i == argc - 1)
 		create_branch(argv[i], head, force_create, reflog);
 	else if (i == argc - 2)

[PATCH] Add support to git-branch to show local and remote branches

[Andy Parkins]

Instead of storing a list of refnames in append_ref, a list of structures is
created.  Each of these stores the refname and a symbolic constant representing
its type.

The creation of the list is filtered based on a command line switch; no switch
means "local branches only", "-r" means "remote branches only" (as they always
did); but now "-a" means "local branches or remote branches".

As a side effect, the list is now not global, but allocated in print_ref_list()
where it used.

Also a memory leak is plugged, the memory allocated during the list creation
was never freed.  This is now done in the new function, tidy_ref_list()

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
 builtin-branch.c |   95 +++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 77 insertions(+), 18 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 368b68e..6dd33ee 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -79,46 +79,100 @@ static void delete_branches(int argc, co
 	}
 }
 
-static int ref_index, ref_alloc;
-static char **ref_list;
+#define REF_UNKNOWN_TYPE    0x00
+#define REF_LOCAL_BRANCH    0x01
+#define REF_REMOTE_BRANCH   0x02
+#define REF_TAG             0x04
+
+struct ref_item {
+	char *name;
+	unsigned int type;
+};
+
+struct ref_list {
+	int index, alloc;
+	struct ref_item *list;
+	int type_wanted;
+};
 
 static int append_ref(const char *refname, const unsigned char *sha1, int flags,
 		void *cb_data)
 {
-	if (ref_index >= ref_alloc) {
-		ref_alloc = alloc_nr(ref_alloc);
-		ref_list = xrealloc(ref_list, ref_alloc * sizeof(char *));
+	struct ref_list *ref_list = (struct ref_list*)(cb_data);
+	struct ref_item *newitem;
+	int type = REF_UNKNOWN_TYPE;
+
+	/* Detect type */
+	if (!strncmp(refname, "refs/heads/", 11)) {
+		type = REF_LOCAL_BRANCH;
+		refname += 11;
+	} else if (!strncmp(refname, "refs/remotes/", 13)) {
+		type = REF_REMOTE_BRANCH;
+		refname += 13;
+	} else if (!strncmp(refname, "refs/tags/", 10)) {
+		type = REF_TAG;
+		refname += 10;
+	}
+
+	/* Don't add type the caller doesn't want */
+	if ((type & ref_list->type_wanted) == 0) {
+		return 0;
+	}
+
+	/* Resize buffer */
+	if (ref_list->index >= ref_list->alloc) {
+		ref_list->alloc = alloc_nr(ref_list->alloc);
+		ref_list->list = xrealloc(ref_list->list,
+				ref_list->alloc * sizeof(struct ref_item));
 	}
 
-	ref_list[ref_index++] = xstrdup(refname);
+	/* Record the new item */
+	newitem = &(ref_list->list[ref_list->index++]);
+	newitem->name = xstrdup(refname);
+	newitem->type = type;
 
 	return 0;
 }
 
+static int tidy_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 );
+}
+
 static int ref_cmp(const void *r1, const void *r2)
 {
+	struct ref_item *c1 = (struct ref_item*)(r1),
+					*c2 = (struct ref_item*)(r2);
+	if( c1->type != c2->type )
+		return c1->type - c2->type;
 	return strcmp(*(char **)r1, *(char **)r2);
 }
 
-static void print_ref_list(int remote_only)
+static void print_ref_list( int type_wanted )
 {
 	int i;
 	char c;
+	struct ref_list ref_list;
 
-	if (remote_only)
-		for_each_remote_ref(append_ref, NULL);
-	else
-		for_each_branch_ref(append_ref, NULL);
+	memset( &ref_list, 0, sizeof( ref_list ) );
+	ref_list.type_wanted = type_wanted;
+	for_each_ref(append_ref, &ref_list);
 
-	qsort(ref_list, ref_index, sizeof(char *), ref_cmp);
+	qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
 
-	for (i = 0; i < ref_index; i++) {
+	for (i = 0; i < ref_list.index; i++) {
 		c = ' ';
-		if (!strcmp(ref_list[i], head))
+		if (!strcmp(ref_list.list[i].name, head))
 			c = '*';
 
-		printf("%c %s\n", c, ref_list[i]);
+		printf("%c %s\n", c, ref_list.list[i].name);
 	}
+
+	tidy_ref_list( &ref_list );
 }
 
 static void create_branch(const char *name, const char *start,
@@ -160,9 +214,10 @@ static void create_branch(const char *na
 
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
-	int delete = 0, force_delete = 0, force_create = 0, remote_only = 0;
+	int delete = 0, force_delete = 0, force_create = 0;
 	int reflog = 0;
 	int i;
+	int type_wanted = REF_LOCAL_BRANCH;
 
 	git_config(git_default_config);
 
@@ -189,7 +244,11 @@ int cmd_branch(int argc, const char **ar
 			continue;
 		}
 		if (!strcmp(arg, "-r")) {
-			remote_only = 1;
+			type_wanted = REF_REMOTE_BRANCH;
+			continue;
+		}
+		if (!strcmp(arg, "-a")) {
+			type_wanted = REF_LOCAL_BRANCH | REF_REMOTE_BRANCH;
 			continue;
 		}
 		if (!strcmp(arg, "-l")) {
@@ -209,7 +268,7 @@ int cmd_branch(int argc, const char **ar
 	if (delete)
 		delete_branches(argc - i, argv + i, force_delete);
 	else if (i == argc)
-		print_ref_list(remote_only);
+		print_ref_list(type_wanted);
 	else if (i == argc - 1)
 		create_branch(argv[i], head, force_create, reflog);
 	else if (i == argc - 2)
-- 
1.4.3.2

Re: [PATCH] Add support to git-branch to show local and remote branches

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> Instead of storing a list of refnames in append_ref, a list of
> structures is created.  Each of these stores the refname and a
> symbolic constant representing its type.
>
> The creation of the list is filtered based on a command line
> switch; no switch means "local branches only", "-r" means
> "remote branches only" (as they always did); but now "-a"
> means "local branches or remote branches".
>
> As a side effect, the list is now not global, but allocated in
> print_ref_list() where it used.
>
> Also a memory leak is plugged, the memory allocated during the
> list creation was never freed.  This is now done in the new
> function, tidy_ref_list()

I would not call that a "leak" given that print_ref_list() is
the last thing to be called before the command exits.  I'd
rather not to spend cycles calling free().

> +static int tidy_ref_list( struct ref_list *ref_list )

Style.  No spaces before or after parameter list.

I see you already parse "refs/tags" prefix. "git branch" would
not print tags, but that part might be useful when we want to
redo git-tag in C.

Other than that, I think it is equivalent to what I have in "pu"
right now.

Re: [PATCH] Add support to git-branch to show local and remote branches

[Andy Parkins]

On Friday 2006 November 03 12:08, Junio C Hamano wrote:

> > +static int tidy_ref_list( struct ref_list *ref_list )
>
> Style.  No spaces before or after parameter list.

Bah!  It's so hard getting my fingers to remember what style goes in which 
project :-)

> I see you already parse "refs/tags" prefix. "git branch" would
> not print tags, but that part might be useful when we want to
> redo git-tag in C.

I'm going to have a look at that soon; I've got enough of a hang of things 
with this git-branch work that I think I could cope with writing git-tag in 
C.


Andy
-- 
Dr Andy Parkins, M Eng (hons), MIEE

Re: [PATCH] Add support to git-branch to show local and remote branches

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> On Friday 2006 November 03 12:08, Junio C Hamano wrote:
>
>> > +static int tidy_ref_list( struct ref_list *ref_list )
>>
>> Style.  No spaces before or after parameter list.
>
> Bah!  It's so hard getting my fingers to remember what style goes in which 
> project :-)
>
>> I see you already parse "refs/tags" prefix. "git branch" would
>> not print tags, but that part might be useful when we want to
>> redo git-tag in C.
>
> I'm going to have a look at that soon;...

Just to make sure you do not misunderstand.  I do not see much
need for rewriting git-tag in C right now, and often it is more
convenient to leave higher layer commands in shell scripts to
allow people to try out new things and flush out UI issues.  I
was merely pointing out that part of the code would be usable if
we were to do it.