[PATCH 2/4] Rename remote_only to display_mode

Subsystems: the rest

STALE3660d

8 messages, 3 authors, 2016-08-11 · open the first message on its own page

[PATCH 2/4] Rename remote_only to display_mode

From: Andy Parkins <hidden>
Date: 2016-08-11 19:46:57

Signed-off-by: Andy Parkins <redacted>
---
 builtin-branch.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 368b68e..85b7007 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c

@@ -100,12 +100,12 @@ static int ref_cmp(const void *r1, const
 	return strcmp(*(char **)r1, *(char **)r2);
 }
 
-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);

@@ -160,7 +160,7 @@ 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, display_mode = 0;
 	int reflog = 0;
 	int i;

@@ -189,7 +189,7 @@ int cmd_branch(int argc, const char **ar
 			continue;
 		}
 		if (!strcmp(arg, "-r")) {
-			remote_only = 1;
+			display_mode = 1;
 			continue;
 		}
 		if (!strcmp(arg, "-l")) {

@@ -209,7 +209,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(display_mode);
 	else if (i == argc - 1)
 		create_branch(argv[i], head, force_create, reflog);
 	else if (i == argc - 2)

-- 
1.4.3.2

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

From: Andy Parkins <hidden>
Date: 2016-08-11 19:18:25

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 <redacted>
---
 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 2/4] Rename remote_only to display_mode

From: Andy Parkins <hidden>
Date: 2016-08-11 19:28:50

On Friday 2006 November 03 10:51, Andreas Ericsson wrote:

If you *need* to change something, change it. If you *want* to change
something just because it's not written the way you would write it, back
away. If you think some interface you're using needs clearing up
(codewise or with extra comments), send a separate patch for that so the
actual feature/bugfix you're sending in doesn't drown in cosmetic
changes to the interfaces the patch uses/touches.

Thank you for the excellent advice.  What then would you suggest in the case 
in point?  I made as minimal a change as I could make; but that left the code 
a little bit bitty - I had press-ganged a variable into taking on another 
function and was using numeric literals that should really have been given 
meaning with #define?

My question is perhaps different from simply git-etiquette; it's should I 
prefer my patches to be minimal or neat?  If there is a more appropriate way 
of doing something should I do it or should I favour minimalism?

I've actually rewritten it now as per Junio's request, and while I'm happier 
with the code, it was much bigger change, that didn't really lend itself to 
being broken into smaller patches as did my first attempt.

I guess in the end it's a judgement call and the best thing to do is post it 
and see who shoots it down :-)


Andy

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

[PATCH] Colourise git-branch output

From: Andy Parkins <hidden>
Date: 2016-08-11 19:55:09

I wanted to have a visual indication of which branches are local and which are
remote in git-branch -a output; however Junio was concerned that someone might
be using the output in a script.  This patch addresses the problem by colouring
the git-branch output - which in "auto" mode won't be activated.

I've based it off the colouring code for builtin-diff.c; which means there is a
branch.color configuration variable that needs setting to something before the
color will appear.

This patch chooses green for local, red for remote and bold green for current.

As yet, there is no support for changing the colors using the config file; but
it wouldn't be hard to add.

Signed-off-by: Andy Parkins <redacted>
---
 builtin-branch.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 6dd33ee..de7f81e 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c

@@ -5,6 +5,7 @@
  * Based on git-branch.sh by Junio C Hamano.
  */
 
+#include "color.h"
 #include "cache.h"
 #include "refs.h"
 #include "commit.h"

@@ -17,6 +18,38 @@ static const char builtin_branch_usage[]
 static const char *head;
 static unsigned char head_sha1[20];
 
+static int branch_use_color;
+static char branch_colors[][COLOR_MAXLEN] = {
+	"\033[m",	/* reset */
+	"",		/* PLAIN (normal) */
+	"\033[31m",	/* REMOTE (red) */
+	"\033[32m",	/* LOCAL (green) */
+	"\033[1;32m",	/* CURRENT (boldgreen) */
+};
+enum color_branch {
+	COLOR_BRANCH_RESET = 0,
+	COLOR_BRANCH_PLAIN = 1,
+	COLOR_BRANCH_REMOTE = 2,
+	COLOR_BRANCH_LOCAL = 3,
+	COLOR_BRANCH_CURRENT = 4,
+};
+
+int git_branch_config(const char *var, const char *value)
+{
+	if (!strcmp(var, "branch.color")) {
+		branch_use_color = git_config_colorbool(var, value);
+		return 0;
+	}
+	return git_default_config(var, value);
+}
+
+const char *branch_get_color(enum color_branch ix)
+{
+	if (branch_use_color)
+		return branch_colors[ix];
+	return "";
+}
+
 static int in_merge_bases(const unsigned char *sha1,
 			  struct commit *rev1,
 			  struct commit *rev2)

@@ -157,6 +190,7 @@ static void print_ref_list( int type_wan
 	int i;
 	char c;
 	struct ref_list ref_list;
+	int color;
 
 	memset( &ref_list, 0, sizeof( ref_list ) );
 	ref_list.type_wanted = type_wanted;

@@ -165,11 +199,28 @@ static void print_ref_list( int type_wan
 	qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
 
 	for (i = 0; i < ref_list.index; i++) {
+		switch( ref_list.list[i].type ) {
+			case REF_LOCAL_BRANCH:
+				color = COLOR_BRANCH_LOCAL;
+				break;
+			case REF_REMOTE_BRANCH:
+				color = COLOR_BRANCH_REMOTE;
+				break;
+			default:
+				color = COLOR_BRANCH_PLAIN;
+				break;
+		}
+
 		c = ' ';
-		if (!strcmp(ref_list.list[i].name, head))
+		if (!strcmp(ref_list.list[i].name, head)) {
 			c = '*';
+			color = COLOR_BRANCH_CURRENT;
+		}
 
-		printf("%c %s\n", c, ref_list.list[i].name);
+		printf("%c %s%s%s\n", c,
+				branch_get_color(color),
+				ref_list.list[i].name,
+				branch_get_color(COLOR_BRANCH_RESET));
 	}
 
 	tidy_ref_list( &ref_list );

@@ -219,7 +270,7 @@ int cmd_branch(int argc, const char **ar
 	int i;
 	int type_wanted = REF_LOCAL_BRANCH;
 
-	git_config(git_default_config);
+	git_config(git_branch_config);
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];

-- 
1.4.3.2

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

From: Andreas Ericsson <hidden>
Date: 2016-08-11 20:08:32

Andy Parkins wrote:

On Friday 2006 November 03 10:51, Andreas Ericsson wrote:

quoted

If you *need* to change something, change it. If you *want* to change
something just because it's not written the way you would write it, back
away. If you think some interface you're using needs clearing up
(codewise or with extra comments), send a separate patch for that so the
actual feature/bugfix you're sending in doesn't drown in cosmetic
changes to the interfaces the patch uses/touches.

Thank you for the excellent advice.  What then would you suggest in the case 
in point?  I made as minimal a change as I could make; but that left the code 
a little bit bitty - I had press-ganged a variable into taking on another 
function and was using numeric literals that should really have been given 
meaning with #define?

My question is perhaps different from simply git-etiquette; it's should I 
prefer my patches to be minimal or neat?  If there is a more appropriate way 
of doing something should I do it or should I favour minimalism?

Neat, imo. Re-using old variables might be appropriate if the name of 
the variable still makes sense, but rename it if there's a better name 
for it.

I've actually rewritten it now as per Junio's request, and while I'm happier 
with the code, it was much bigger change, that didn't really lend itself to 
being broken into smaller patches as did my first attempt.

I guess in the end it's a judgement call and the best thing to do is post it 
and see who shoots it down :-)

Probably the most sensible approach. Even though the list is pretty 
trigger-happy, the guns are more of the playful water-squirt type than 
the high-powered big-calibre kind.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se

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

From: Andy Parkins <hidden>
Date: 2016-08-11 20:15:52

On Friday 2006 November 03 02:40, Junio C Hamano wrote:

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.

In my own code I would have done exactly that; however I've been trying to 
keep my patches as minimal as possible.

Digressing a little: what is the polite form of patches for git?  My strategy 
with this set was to make each patch as small as possible to reach my end 
point.  If those patches were okayed on the list, I could then do a "make 
more beautiful" patch, which is really nothing to do with the original 
changes to functionality but would make the code prettier.  Really I'm asking 
what level of intrusiveness of patch is not considered rude?  In making my 
patches, should I ride rough-shod over current implementation and just do it 
how I'd do it or should I try to fit in (as I did in this case)?

Something like this untested patch, that is...

I'm very much in favour; I shall make changes of this form soon.


Andy

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

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

From: Andreas Ericsson <hidden>
Date: 2016-08-11 20:22:42

Andy Parkins wrote:

Digressing a little: what is the polite form of patches for git?  My strategy 
with this set was to make each patch as small as possible to reach my end 
point.  If those patches were okayed on the list, I could then do a "make 
more beautiful" patch, which is really nothing to do with the original 
changes to functionality but would make the code prettier.

I believe the order of preferrence goes: tested, concise, short.

Linus has a nasty habit of ending his mails with "totally untested 
ofcourse", which is not a good strategy to adopt if you want your 
patches included.

 Really I'm asking 
what level of intrusiveness of patch is not considered rude?  In making my 
patches, should I ride rough-shod over current implementation and just do it 
how I'd do it or should I try to fit in (as I did in this case)?

If you *need* to change something, change it. If you *want* to change 
something just because it's not written the way you would write it, back 
away. If you think some interface you're using needs clearing up 
(codewise or with extra comments), send a separate patch for that so the 
actual feature/bugfix you're sending in doesn't drown in cosmetic 
changes to the interfaces the patch uses/touches.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se

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

From: Junio C Hamano <hidden>
Date: 2016-08-11 20:35:48

Andy Parkins [off-list ref] 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)

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help