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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 8, 2011, 18:08 UTC
Message-ID: <7v8vr3zsh1.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1312818553-25042-1-git-send-email-avivey@gmail.com>

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?

Show 13 quoted lines

> +	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?

Show 7 quoted lines

> +	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.

Show 9 quoted lines

> +	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?