[PATCH] Support specific color for a specific remote branches

[PATCH] Support specific color for a specific remote branches

[Aviv Eyal]

Program 'show branch -a' supports different colors for 'local' and 'remote' branches.
When tracking more then one remote, all branches have the same color.
This change lets the user define a color for each remote, providing better visual distinction for different remotes.

Signed-off-by: Aviv Eyal <avivey@gmail.com>
---

Since my C days are now ancient history, I'm worried I might leak some char*s down there. Also, 
I might have missed some apis that would have made this patch smaller.

I'm using a sorted list as a dictionary (Remote name -> Color), which is not too elegant but
probably 'good enough', considering I expect it to be a very small (<3) list.

Aviv.

 Documentation/config.txt |    6 ++
 builtin/branch.c         |  119 +++++++++++++++++++++++++++++++++++++---------
 2 files changed, 102 insertions(+), 23 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0658ffb..efdb61f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -700,6 +700,7 @@ color.branch.<slot>::
 	`current` (the current branch), `local` (a local branch),
 	`remote` (a remote-tracking branch in refs/remotes/), `plain` (other
 	refs).
+	See also `remote.<name>.color`.
 +
 The value for these configuration variables is a list of colors (at most
 two) and attributes (at most one), separated by spaces.  The colors
@@ -1658,6 +1659,11 @@ remote.<name>.mirror::
 	If true, pushing to this remote will automatically behave
 	as if the `\--mirror` option was given on the command line.
 
+remote.<name>.color:
+	If set, the `branch` command will use the colors specified here
+	for the names of all branches tracking this remote. Colors
+	specified here take precedent over `color.branch.remote` config.
+
 remote.<name>.skipDefaultUpdate::
 	If true, this remote will be skipped by default when updating
 	using linkgit:git-fetch[1] or the `update` subcommand of
diff --git a/builtin/branch.c b/builtin/branch.c
index 3142daa..85e6920 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -15,6 +15,7 @@
 #include "branch.h"
 #include "diff.h"
 #include "revision.h"
+#include "string-list.h"
 
 static const char * const builtin_branch_usage[] = {
 	"git branch [options] [-r | -a] [--merged | --no-merged]",
@@ -46,6 +47,9 @@ enum color_branch {
 	BRANCH_COLOR_CURRENT = 4
 };
 
+// this string_list is used as a basic dictionary.
+struct string_list custom_colors = STRING_LIST_INIT_DUP;
+
 static enum merge_filter {
 	NO_FILTER = 0,
 	SHOW_NOT_MERGED,
@@ -68,6 +72,52 @@ static int parse_branch_color_slot(const char *var, int ofs)
 	return -1;
 }
 
+static char *strclone(const char *start, int len)
+{
+	char *result = malloc((len + 1) * sizeof(char));
+	if (!result)
+		return NULL;
+	strncpy(result, start, len);
+	result[len] = '\0';
+	return result;
+}
+
+static int git_branch_config_custom_color_remote(const char *var, const char *value)
+{
+	struct string_list_item *item;
+	char *name, *color;
+	name = strclone(var + 7, strlen(var) - 13); // "remote."=7, "remote..color"=13.
+	if (!name)
+		return 0;
+	color =  malloc(COLOR_MAXLEN * sizeof(char));
+	if (!color) {
+		free(name);
+		return 0;
+	}
+	item = string_list_insert(&custom_colors, name);
+	color_parse(value, var, color);
+	item->util = color;
+	return 0;
+}
+
+static char *git_branch_get_custom_color_remote(const char *name)
+{
+	int name_len;
+	char* repo_name;
+	struct string_list_item *custom;
+	name_len = strchr(name, '/') - name;
+	repo_name = strclone(name, name_len);
+	if (!repo_name)
+		return NULL;
+
+	custom = string_list_lookup(&custom_colors, repo_name);
+	free(repo_name);
+	if (custom)
+		return (char*) custom->util;
+	return NULL;
+}
+
+
 static int git_branch_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "color.branch")) {
@@ -83,14 +133,10 @@ static int git_branch_config(const char *var, const char *value, void *cb)
 		color_parse(value, var, branch_colors[slot]);
 		return 0;
 	}
-	return git_color_default_config(var, value, cb);
-}
+	if (!prefixcmp(var, "remote.") && !suffixcmp(var, ".color"))
+		return git_branch_config_custom_color_remote(var, value);
 
-static const char *branch_get_color(enum color_branch ix)
-{
-	if (branch_use_color > 0)
-		return branch_colors[ix];
-	return "";
+	return git_color_default_config(var, value, cb);
 }
 
 static int branch_merged(int kind, const char *name,
@@ -354,6 +400,43 @@ static int ref_cmp(const void *r1, const void *r2)
 	return strcmp(c1->name, c2->name);
 }
 
+static const char *branch_get_color(struct ref_item *item, int current)
+{
+	int index;
+	char *color;
+
+	if (branch_use_color <= 0)
+		return "";
+
+	if (!item)
+		index = BRANCH_COLOR_RESET;
+	else if (current)
+		index = BRANCH_COLOR_CURRENT;
+	else switch (item->kind) {
+		case REF_LOCAL_BRANCH:
+			index = BRANCH_COLOR_LOCAL;
+			break;
+		case REF_REMOTE_BRANCH:
+			index = BRANCH_COLOR_REMOTE;
+			color = git_branch_get_custom_color_remote(item->name);
+			if (color)
+				return color;
+			break;
+		default:
+			index = BRANCH_COLOR_PLAIN;
+			break;
+	}
+
+	return branch_colors[index];
+}
+
+static const char *branch_get_color_reset()
+{
+	if (branch_use_color <= 0)
+		return "";
+	return branch_colors[BRANCH_COLOR_RESET];
+}
+
 static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
 		int show_upstream_ref)
 {
@@ -424,18 +507,6 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 	if (!matches_merge_filter(commit))
 		return;
 
-	switch (item->kind) {
-	case REF_LOCAL_BRANCH:
-		color = BRANCH_COLOR_LOCAL;
-		break;
-	case REF_REMOTE_BRANCH:
-		color = BRANCH_COLOR_REMOTE;
-		break;
-	default:
-		color = BRANCH_COLOR_PLAIN;
-		break;
-	}
-
 	c = ' ';
 	if (current) {
 		c = '*';
@@ -444,12 +515,12 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 
 	strbuf_addf(&name, "%s%s", prefix, item->name);
 	if (verbose)
-		strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
+		strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(item, current),
 			    maxwidth, name.buf,
-			    branch_get_color(BRANCH_COLOR_RESET));
+			    branch_get_color_reset());
 	else
-		strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(color),
-			    name.buf, branch_get_color(BRANCH_COLOR_RESET));
+		strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(item, current),
+			    name.buf, branch_get_color_reset());
 
 	if (item->dest)
 		strbuf_addf(&out, " -> %s", item->dest);
@@ -712,5 +783,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	} else
 		usage_with_options(builtin_branch_usage, options);
 
+	string_list_clear(&custom_colors, 1);
+
 	return 0;
 }
-- 
1.7.4.1

Re: [PATCH] Support specific color for a specific remote branches

[Junio C Hamano]

Aviv Eyal <avivey@gmail.com> writes:

> +remote.<name>.color:
> +	If set, the `branch` command will use the colors specified here
> +	for the names of all branches tracking this remote. Colors
> +	specified here take precedent over `color.branch.remote` config.

> +static char *strclone(const char *start, int len)
> +{

Hmm, don't we have xmemdupz() for this?

> +	char *result = malloc((len + 1) * sizeof(char));
> +	if (!result)
> +		return NULL;
> +	strncpy(result, start, len);
> +	result[len] = '\0';
> +	return result;
> +}
> +
> +static int git_branch_config_custom_color_remote(const char *var, const char *value)
> +{
> +	struct string_list_item *item;
> +	char *name, *color;
> +	name = strclone(var + 7, strlen(var) - 13); // "remote."=7, "remote..color"=13.

No // comments please.

> +	if (!name)
> +		return 0;
> +	color =  malloc(COLOR_MAXLEN * sizeof(char));

At least use xmalloc(); also isn't sizeof(char) by definition 1?

> +	if (!color) {
> +		free(name);
> +		return 0;
> +	}
> +	item = string_list_insert(&custom_colors, name);
> +	color_parse(value, var, color);
> +	item->util = color;

It may be just a style thing, but I'd rather see this caller call
color_parse() first, and after letting that function validate the end-user
input, call string_list_insert().

Also what happens when there are two entries that talk about the same
remote branch in the configuration? Such a configuration is not an error;
your ~/.gitconfig may say one thing for remote.origin.master and your
repository specific .git/config may override it for a particular
repository.

> +	return 0;
> +}
> +
> +static char *git_branch_get_custom_color_remote(const char *name)
> +{
> +	int name_len;
> +	char* repo_name;
> +	struct string_list_item *custom;
> +	name_len = strchr(name, '/') - name;

Who said a remote name is terminated with (and cannot contain) a slash?

Shouldn't this code be consulting the configuration file to learn the
remote mapping, e.g.

    [remote "frotz"]
        fetch = +refs/heads/*:refs/remotes/nitfol/*

so that remote branches from "frotz" remote, that happen to be stored
under refs/remotes/nitfol/ hierarchy, are painted in the correct color?

Re: [PATCH] Support specific color for a specific remote branches

[Jeff King]

On Mon, Aug 08, 2011 at 11:08:26AM -0700, Junio C Hamano wrote:

> > +	return 0;
> > +}
> > +
> > +static char *git_branch_get_custom_color_remote(const char *name)
> > +{
> > +	int name_len;
> > +	char* repo_name;
> > +	struct string_list_item *custom;
> > +	name_len = strchr(name, '/') - name;
> 
> Who said a remote name is terminated with (and cannot contain) a slash?
> 
> Shouldn't this code be consulting the configuration file to learn the
> remote mapping, e.g.
> 
>     [remote "frotz"]
>         fetch = +refs/heads/*:refs/remotes/nitfol/*
> 
> so that remote branches from "frotz" remote, that happen to be stored
> under refs/remotes/nitfol/ hierarchy, are painted in the correct color?

This seems related to the recent thread about showing branches only for
a specific remote:

  http://article.gmane.org/gmane.comp.version-control.git/178668

Maybe the two should share code.

Right now, "git branch -r" means "show everything under refs/remotes
instead of refs/heads". This would be easy to implement if it instead
meant "show all refs created by the RHS of a fetch refspec in a
configured remote". The two are equivalent in the default config, but
the latter may make more sense in a complex case.

-Peff

Re: [PATCH] Support specific color for a specific remote branches

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 08, 2011 at 11:08:26AM -0700, Junio C Hamano wrote:
>
> This seems related to the recent thread about showing branches only for
> a specific remote:
>
>   http://article.gmane.org/gmane.comp.version-control.git/178668

Yeah, that one I do recall.

> Right now, "git branch -r" means "show everything under refs/remotes
> instead of refs/heads". This would be easy to implement if it instead
> meant "show all refs created by the RHS of a fetch refspec in a
> configured remote". The two are equivalent in the default config, but
> the latter may make more sense in a complex case.

I actually am a bit ambivalent about this. I do not necessarily consider
the contrived "remote.frotz.fetch = refs/heads/*:refs/remotes/nitfol/*"
example something that we _must_ solve. It is unlikely people would do
that, and if we give them an unexpected result, they deserve it ;-).

But in real-life, it is entirely plausible that people with multiple
integration branches are taking advantage of the simplicity of the old
layout, i.e.

    [remote "origin"]
    	fetch = refs/heads/master:refs/heads/origin
        fetch = refs/heads/next:refs/heads/next
        fetch = refs/heads/maint:refs/heads/maint
        fetch = +refs/heads/pu:refs/heads/pu

I have many repositories of this style, and it is very convenient to be
able to say:

    $ git checkout master && git pull --ff-only
    $ for b in master next maint pu
      do
          git checkout $b && make install || break
      done

I do not think I want to ever switch them to new layout, and I suspect
that many others do feel the same.

Now, for these repositories, is "next" a local branch or a remote one? I
have a feeling that it might be easier to understand if we label anything
that you can update with "checkout && commit" a local one for the purpose
of "branch -r" listing; IOW, the current "git branch -r" classification
would match this use pattern better, even though refs/heads/next _is_ an
RHS of a rule to follow others.

In that sense, I would be entirely happy if the configuration variable
used in this series were branch.<namepattern>.color and let you specify

	[branch "refs/heads"] color = yellow
        [branch "refs/remotes/origin"] color = purple
        [branch "refs/remotes/nitfol"] color = cyan

It becomes complicated (and for no good reason, in my opinion; see the
"next" example above) if you try to tie this with remote.<name> hierarchy,
as it obviously becomes illogical not to use the "RHS of a fetch refspec"
logic when we are talking about remote.<name>.

Re: [PATCH] Support specific color for a specific remote branches

[Jeff King]

On Mon, Aug 08, 2011 at 02:31:39PM -0700, Junio C Hamano wrote:

> But in real-life, it is entirely plausible that people with multiple
> integration branches are taking advantage of the simplicity of the old
> layout, i.e.
> 
>     [remote "origin"]
>     	fetch = refs/heads/master:refs/heads/origin
>         fetch = refs/heads/next:refs/heads/next
>         fetch = refs/heads/maint:refs/heads/maint
>         fetch = +refs/heads/pu:refs/heads/pu
>
> [...]
>
> Now, for these repositories, is "next" a local branch or a remote one? I
> have a feeling that it might be easier to understand if we label anything
> that you can update with "checkout && commit" a local one for the purpose
> of "branch -r" listing; IOW, the current "git branch -r" classification
> would match this use pattern better, even though refs/heads/next _is_ an
> RHS of a rule to follow others.

Agreed, that really muddies the idea of what is a "remote branch" and
what is not. I think there is a sense among users that "local branches"
and "remote branches" are mutually exclusive sets, even though by some
definitions they may not be (i.e., if you define the former by where in
the refs/ hierarchy they exist, and the latter by being the RHS of a
remote fetch refspec).

> In that sense, I would be entirely happy if the configuration variable
> used in this series were branch.<namepattern>.color and let you specify
> 
> 	[branch "refs/heads"] color = yellow
>         [branch "refs/remotes/origin"] color = purple
>         [branch "refs/remotes/nitfol"] color = cyan

There are two issues I see with that:

  1. Until now, "branch" sections like this have always been about local
     branches.  What are the rules for defining something like
     "branch.refs/remotes/origin/foo.merge"? Knowing how the code works,
     it is easy to say it is a noop, as we will never look at it. But I
     wonder how it looks from the perspective if a recent git user.

  2. Until now, "branch" sections specify full refs, not subsets or
     wildcard patterns. What does "branch.refs/heads.merge" mean?
     A noop? A wildcard with slightly lesser precedence than
     "branch.refs/heads/foo.merge"?

If we are going to go this route, I think it is really about introducing
properties not on branches, but on subsets of the ref namespace. So
might say:

  [ref "refs/heads/*"] color = yellow

which says "whenever you are dealing with this part of the refs
namespace, my preferred color is yellow". And "git branch" happens to
respect that (and we could just as easily have a "%(refcolor)" marker in
git-for-each-ref).

I know the difference is subtle, but I just think it removes entirely
the question about what is a branch and what is not. Furthermore, it
naturally extends to other commands (e.g., you could color subsets of
the tag namespace differently), to more complex layouts (e.g., if we
end up moving fetched tags into the refs/remotes namespace eventually),
and to other properties besides color (though I haven't though up any
applications).

> It becomes complicated (and for no good reason, in my opinion; see the
> "next" example above) if you try to tie this with remote.<name> hierarchy,
> as it obviously becomes illogical not to use the "RHS of a fetch refspec"
> logic when we are talking about remote.<name>.

We've been discussing the coloring issue here. But the other thread I
pointed out was about asking "which fetched branches do we have locally
for this remote?".  Which is a very reasonable thing to ask, and which
don't do a good job of answering right now. And I think it has to
do the "RHS of a fetch refspec thing".

Right now you can do "git remote show". But:

  1. It's very heavyweight. It shows you way more than you want in most
     cases, and it touches the network by default (there is a "-n"
     option, but touching the network by default makes it pretty
     un-git).

  2. It's not as easily discoverable as "git branch -r". It's not
     unreasonable for users to mentally go through the sequence:

       a. "git branch" shows me branches

       b. oh, it has an "-r" option for remotes

       c. how do I limit to one remote?

I'm not sure what the right solution is. Going from 2b to 2c is a very
natural thing for a user to want to do. But it means jumping from one
definition of "remote" (i.e., everything under "refs/remotes") to
another (the config defined by remote "foo"). In the default config,
that is a natural jump, as they are semantically connected. But they
don't have to be.

-Peff

Re: [PATCH] Support specific color for a specific remote branches

[Aviv Eyal]

On 8 August 2011 21:08, Junio C Hamano <gitster@pobox.com> wrote:
>
> Who said a remote name is terminated with (and cannot contain) a slash?
...
>
> Shouldn't this code be consulting the configuration file to learn the
...
> so that remote branches from "frotz" remote, that happen to be stored
> under refs/remotes/nitfol/ hierarchy, are painted in the correct color?
>

Sorry, that's too dip & complex for me... Consider this patch 'withdrawn' then.

-- Aviv