Re: [PATCH v3 10/13] pretty: add %C(auto) for auto-coloring

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Nguyễn Thái Ngọc Duy  <pclouds@xxxxxxxxx> writes:

> This is not simply convenient over %C(auto,xxx). Some placeholders
> (actually only one, %d) do multi coloring and we can't emit a multiple
> colors with %C(auto,xxx).
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
> ---
>  Documentation/pretty-formats.txt |  3 ++-
>  pretty.c                         | 17 +++++++++++++++--
>  2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 6bde67e..bad627a 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -156,7 +156,8 @@ The placeholders are:
>    adding `auto,` at the beginning will emit color only when colors are
>    enabled for log output (by `color.diff`, `color.ui`, or `--color`, and
>    respecting the `auto` settings of the former if we are going to a
> -  terminal)
> +  terminal). `auto` alone (i.e. `%C(auto)`) will turn on auto coloring
> +  on the next placeholders until the color is switched again.
>  - '%m': left, right or boundary mark
>  - '%n': newline
>  - '%%': a raw '%'

Good addition.

In the previous round, it used to be "%C(auto)" was remembered in
this field to color the _next_ one, and the momorized value was
toggled off.  As the field no longer toggles automatically once it
is used, perhaps it should simply be called "auto_color" without
"next", though?

I can locally tweak that before queuing if you think it is better.

> diff --git a/pretty.c b/pretty.c
> index e0413e3..f385176 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -778,6 +778,7 @@ struct format_commit_context {
>  	char *message;
>  	char *commit_encoding;
>  	size_t width, indent1, indent2;
> +	int auto_color_next;
>  
>  	/* These offsets are relative to the start of the commit message. */
>  	struct chunk author;
> @@ -1005,7 +1006,15 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  	/* these are independent of the commit */
>  	switch (placeholder[0]) {
>  	case 'C':
> -		return parse_color(sb, placeholder, c);
> +		if (!prefixcmp(placeholder + 1, "(auto)")) {
> +			c->auto_color_next = 1;
> +			return 7;
> +		} else {
> +			int ret = parse_color(sb, placeholder, c);
> +			if (ret)
> +				c->auto_color_next = 0;
> +			return ret;
> +		}

This is to handle a corrupt input, e.g. "%C(auto)%Cbleu%H" where
(perhaps deprecated) "%Cblue" is misspelled, and parse_color()
returns 0 without consuming any byte.

Does it make sense not to turn auto off in such a case?  Otherwise
the above would become

	if (!prefixcmp(placeholder + 1, "(auto)")) {
        	c->auto_color_next = 1;
                return 7; /* consumed 7 bytes, "C(auto)" */
	}
        c->auto_color_next = 0;
        return parse_color(sb, placeholder, c);

which may be simpler.  When we see %C, previous %C(auto) is
cancelled.

>  	case 'n':		/* newline */
>  		strbuf_addch(sb, '\n');
>  		return 1;
> @@ -1051,13 +1060,17 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  
>  	switch (placeholder[0]) {
>  	case 'H':		/* commit hash */
> +		strbuf_addstr(sb, diff_get_color(c->auto_color_next, DIFF_COMMIT));
>  		strbuf_addstr(sb, sha1_to_hex(commit->object.sha1));
> +		strbuf_addstr(sb, diff_get_color(c->auto_color_next, DIFF_RESET));
>  		return 1;

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]