Re: [PATCH v2 3/3] for-each-ref: introduce %(color:...) for color

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 13, 2013, 20:01 UTC
Message-ID: <xmqqbo1odqb4.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1384335406-16332-4-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> +		} else if (!prefixcmp(name, "color")) {
> +			;

How is "%(color:short)" parsed with this code?

This part says, "Yeah, I see something starting with color", and then later strchr(name, ':') will point formatp to "short".

Luckily, "%(colorgarbage:short)" does not even come this far because parse_atom() would not have allowed the codeflow to, but comparing with "color:" here may be a lot more defensive and safe, I think.

And find the color-value here, stuffing v->s inside this "else if", continue, without letting the formatp part work on refname this piece of code does not even set. Just like how we handle "flag" without falling thru to the formatp code.

Show 17 quoted lines

> +		} else if (!strcmp(name, "flag")) {
>  			char buf[256], *cp = buf;
>  			if (ref->flag & REF_ISSYMREF)
>  				cp = copy_advance(cp, ",symref");
> @@ -729,6 +732,12 @@ static void populate_value(struct refinfo *ref)
>  				else
>  					v->s = "<>";
>  				continue;
> +			} else if (!prefixcmp(name, "color")) {
> +				char color[COLOR_MAXLEN] = "";
> +
> +				color_parse(formatp, "--format", color);
> +				v->s = xstrdup(color);
> +				continue;
>  			} else
>  				die("unknown %.*s format %s",
>  				    (int)(formatp - name), name, formatp);