Re: [PATCH] Colorize commit decorations

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 17, 2010, 20:22 UTC
Message-ID: <7vtyp11k9x.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1276791344-25550-1-git-send-email-ayiehere@gmail.com>

Nazri Ramliy <ayiehere@gmail.com> writes:

Show 11 quoted lines

> Use different color for each type of refs (local, remote, tags, HEAD,
> and stash). This makes the decorations and their type stand out more
> and easier to distinguish in 'git log --decorate'.
>
> Currently all the different types of decorations are shown in the same
> color as the commit id, which is not that easy to spot.
>
> The color applied for each type of refs are customizable via
> color.log.decorate.<slot> config entry, as documented in
> Documentation/config.txt.
> ---

Sign-off?

Show 12 quoted lines

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 95cf73c..afa4f5a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -725,6 +725,11 @@ color.interactive.<slot>::
>  	commands.  The values of these variables may be specified as
>  	in color.branch.<slot>.
>  
> +color.log.decorate.<slot>::
> +	Use customized color for 'git log --decorate' output.
> +	`<slot>` is one of `local`, `remote`, `tag`, `stash` or `head`
> +	for local refs, remote refs, tags, stash and HEAD, respectively.

Configuration variable names are either 2-level or 3-level. There are existing examples for <slot> (e.g. color.branch.<slot>).

Show 19 quoted lines

> diff --git a/decorate.c b/decorate.c
> index 2f8a63e..cbef2b4 100644
> --- a/decorate.c
> +++ b/decorate.c
> @@ -5,6 +5,48 @@
>  #include "cache.h"
>  #include "object.h"
>  #include "decorate.h"
> +#include "color.h"
> ...
> +int parse_decorate_color_config(const char *var, const int ofs, const char *value) {
> +	int slot = parse_decorate_color_slot(var + ofs);
> +	if (slot < 0)
> +		return 0;
> +	if (!value)
> +		return config_error_nonbool(var);
> +	color_parse(value, var, decoration_colors[slot]);
> +	return 0;
> +}

I don't think any of the above belongs to "decorate.c", which is the generic mechanism to annotate commits with arbitrary data. The Porcelain feature "log --decorate" is just one user that happens to use refname as that "arbitrary data", and that is what you are coloring. The code for that is in log-tree.c, I think.

> diff --git a/decorate.h b/decorate.h
> index e732804..d593d32 100644

So is any change to this file.

Show 21 quoted lines

> diff --git a/log-tree.c b/log-tree.c
> index d3ae969..95ebf1a 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -10,29 +10,41 @@
>  
>  struct decoration name_decoration = { "object names" };
>  
> -static void add_name_decoration(const char *prefix, const char *name, struct object *obj)
> +static void add_name_decoration(enum decoration_type type, const char *name, struct object *obj)
>  {
> -	int plen = strlen(prefix);
>  	int nlen = strlen(name);
> -	struct name_decoration *res = xmalloc(sizeof(struct name_decoration) + plen + nlen);
> -	memcpy(res->name, prefix, plen);
> -	memcpy(res->name + plen, name, nlen + 1);
> +	struct name_decoration *res = xmalloc(sizeof(struct name_decoration) + nlen);
> +	memcpy(res->name, name, nlen + 1);
> +	res->type = type;
>  	res->next = add_decoration(&name_decoration, obj, res);
>  }

When color is not in use (e.g. output is not going to the terminal), you would lose "tag: " prefix, wouldn't you?

Even when color _is_ in use, people may want to see familiar "tag: " prefix in the output---I personally do not think that is necessary, though.

Show 17 quoted lines

>  static int add_ref_decoration(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
>  {
>  	struct object *obj = parse_object(sha1);
> +	enum decoration_type type = DECORATION_NONE;
>  	if (!obj)
>  		return 0;
> +
> +	if (!prefixcmp(refname, "refs/heads"))
> +		type = DECORATION_REF_LOCAL;
> +	else if (!prefixcmp(refname, "refs/remotes"))
> +		type = DECORATION_REF_REMOTE;
> +	else if (!prefixcmp(refname, "refs/tags"))
> +		type = DECORATION_TAG;
> +	else if (!prefixcmp(refname, "refs/stash"))
> +		type = DECORATION_STASH;
> +	else if (!prefixcmp(refname, "HEAD"))
> +		type = DECORATION_HEAD;

I suspect that users would expect DECORATION_HEAD to be used to highlight the object at the tip of the current branch, but I also suspect this code would not do so.