From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 08 Oct 2015 18:35:45 GMT
Subject: Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
Message-ID: <xmqq4mi1rywu.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAOLa=ZQOO9BjoTj1B-b=kUviL=617F7y46BeX1sOXpeHcatFVQ@mail.gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

> No i mean I could follow up with the way we use it in align, but I don't see
> how I can make a function out of this.

At least you should be able to pre-parse the %(<atom>:<modifier>)
construct, instead of doing strcmp() every time populate_value() is
called, no?  Then your parser would not be doing

>>>> +                             if (!strcmp(valp, ",nobracket"))
>>>> +                                     nobracket = 1;

with 'valp' that scans over 'name' (which is an element of
used_atom[], i.e. a name from valid_atom[] followed by ":<modifier>"
for each customization).  Instead your used_atom[] would be an array
of a data structure that is richer than just a string.

The <modifier> would be split at comma boundary and made into an
array of two field structure, as the most general form of it is

    <modifier> ::= <attrval> | <attrval> ',' <modifier>
    <attrval> ::= <attr> '=' <value> | <attr> | <value>

if we recall the discussion we had while designing %(align), i.e.

 * The most general form is "%(align:position=left,width=32)"

 * The value domain of attributes may be distinct, in which case the
   value itself could imply what attr it is talking about,
   e.g. "%(align:32,left)" is sufficiently clear as '32' cannot be
   position and 'left' cannot be width.

And clearly there can be an attr whose presense alone can imply its
value (iow, a boolean), even though your %(align) does not yet have
such a modifier.  It is easy to imagine that you would later want to
add %(align:position=left,width=32,truncate) that truncates the value
when its display width exceeds '32'.  The 'nobracket' above would be
an another example of a boolean (whose name is negative, which is
not a very good design in general, though).

Then used_atom[] could become something like

    struct {
    	const char *str; /* e.g. "align:position=left,32" */
	struct {
        	const char *part0; /* everything before '=' */
                const char *part1; /* optional */
	} *modifier;
        int modifier_nr;
    } *used_atom;

and "align:position=left,32" would be parsed into

	.str = "align:position=left,32";
        .modifier = { { "position", "left" }, { "32", NULL } };
        .modifier_nr = 2;

when the format string is read, which is done only once.

The looping over all the refs done in populate_value() could just
use the pre-parsed representation.

Hmm?