Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Fri, Aug 7, 2015 at 11:00 PM, Eric Sunshine wrote:
> On Fri, Aug 7, 2015 at 7:37 AM, Karthik Nayak wrote:
>> On Fri, Aug 7, 2015 at 10:13 AM, Eric Sunshine
>> wrote:
>>> On Thu, Aug 6, 2015 at 11:53 PM, Karthik Nayak
>>> wrote:
On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine
wrote:
> It feels strange to assign a local variable reference to state.output,
> and there's no obvious reason why you should need to do so. I would
> have instead expected ref_format_state to be declared as:
>
> struct ref_formatting_state {
>int quote_style;
>struct strbuf output;
> };
>
> and initialized as so:
>
> memset(&state, 0, sizeof(state));
> state.quote_style = quote_style;
> strbuf_init(&state.output, 0);
>
> (In fact, the memset() isn't even necessary here since you're
> initializing all fields explicitly, though perhaps you want the
> memset() because a future patch adds more fields which are not
> initialized explicitly?)
Yea the memset is needed for bit fields evnetually added in the future.
>>>
>>> Perhaps move the memset() to the first patch which actually requires
>>> it, where it won't be (effectively) dead code, as it becomes here once
>>> you make the above change.
>>
>> But why would I need it there, we need to only memset() the
>> ref_formatting_state
>> which is introduced here. Also here it helps in setting the strbuf
>> within ref_formatting_state to {0, 0, 0}.
>
> If you declare ref_formatting_state as shown above, and then
> initialize it like so:
>
> state.quote_style = quote_style;
> strbuf_init(&state.output, 0);
>
> then (as of this patch) the structure is fully initialized because
> you've initialized each member individually. Adding a memset() above
> those two lines would be do-nothing -- it would be wasted code -- and
> would likely confuse someone reading the code, specifically because
> the code is do-nothing and has no value (in this patch). Making each
> patch understandable is one of your goals when organizing the patch
> series; if a patch confuses a reader (say, by doing unnecessary work),
> then it isn't satisfying that goal.
>
> As for the strbuf member, it's initialized explicitly via
> strbuf_init(), so there's no value in having memset() first initialize
> it to {0, 0, 0}. Again, that's wasted code.
Yes, what you're saying is true, if we replace memset() with
state.quote_style = quote_style;
strbuf_init(&state.output, 0);
That does replace the need for memset and make the patch
more self-explanatory.
>
> In a later patch, when you add another ref_formatting_state member or
> two, then you will need to initialize those members too. That
> initialization may be in the form of explicit assignment to each
> member, or it may be the memset() sledgehammer approach, but the
> initialization for those members should be added in the patch which
> introduces them.
>
> It's true that the end result is the same. By the end of the series,
> you'll have memset() above the 'state.quote' and 'state.output'
> initialization lines to ensure that your various boolean fields and
> whatnot are initialized to zero, but each patch should be
> self-contained and make sense on its own, doing just the work that it
> needs to do, and not doing unrelated work. For this patch, the
> memset() is unrelated work. For the later patch in which you add more
> fields to ref_formatting_state(), the memset() is work necessary to
> satisfy that patch's objective, thus belongs there.
I understand what you're saying, it makes sense to have individual patches
only bring in changes related to the patch. This makes a lot of sense.
I will change it up. Thanks :)
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Fri, Aug 7, 2015 at 7:37 AM, Karthik Nayak wrote:
> On Fri, Aug 7, 2015 at 10:13 AM, Eric Sunshine
> wrote:
>> On Thu, Aug 6, 2015 at 11:53 PM, Karthik Nayak wrote:
>>> On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine
>>> wrote:
It feels strange to assign a local variable reference to state.output,
and there's no obvious reason why you should need to do so. I would
have instead expected ref_format_state to be declared as:
struct ref_formatting_state {
int quote_style;
struct strbuf output;
};
and initialized as so:
memset(&state, 0, sizeof(state));
state.quote_style = quote_style;
strbuf_init(&state.output, 0);
(In fact, the memset() isn't even necessary here since you're
initializing all fields explicitly, though perhaps you want the
memset() because a future patch adds more fields which are not
initialized explicitly?)
>>>
>>> Yea the memset is needed for bit fields evnetually added in the future.
>>
>> Perhaps move the memset() to the first patch which actually requires
>> it, where it won't be (effectively) dead code, as it becomes here once
>> you make the above change.
>
> But why would I need it there, we need to only memset() the
> ref_formatting_state
> which is introduced here. Also here it helps in setting the strbuf
> within ref_formatting_state to {0, 0, 0}.
If you declare ref_formatting_state as shown above, and then
initialize it like so:
state.quote_style = quote_style;
strbuf_init(&state.output, 0);
then (as of this patch) the structure is fully initialized because
you've initialized each member individually. Adding a memset() above
those two lines would be do-nothing -- it would be wasted code -- and
would likely confuse someone reading the code, specifically because
the code is do-nothing and has no value (in this patch). Making each
patch understandable is one of your goals when organizing the patch
series; if a patch confuses a reader (say, by doing unnecessary work),
then it isn't satisfying that goal.
As for the strbuf member, it's initialized explicitly via
strbuf_init(), so there's no value in having memset() first initialize
it to {0, 0, 0}. Again, that's wasted code.
In a later patch, when you add another ref_formatting_state member or
two, then you will need to initialize those members too. That
initialization may be in the form of explicit assignment to each
member, or it may be the memset() sledgehammer approach, but the
initialization for those members should be added in the patch which
introduces them.
It's true that the end result is the same. By the end of the series,
you'll have memset() above the 'state.quote' and 'state.output'
initialization lines to ensure that your various boolean fields and
whatnot are initialized to zero, but each patch should be
self-contained and make sense on its own, doing just the work that it
needs to do, and not doing unrelated work. For this patch, the
memset() is unrelated work. For the later patch in which you add more
fields to ref_formatting_state(), the memset() is work necessary to
satisfy that patch's objective, thus belongs there.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Fri, Aug 7, 2015 at 10:13 AM, Eric Sunshine wrote:
> On Thu, Aug 6, 2015 at 11:53 PM, Karthik Nayak wrote:
>> On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine
>> wrote:
>>> On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak wrote:
+static void apply_formatting_state(struct ref_formatting_state *state,
struct strbuf *final)
+{
+ /* More formatting options to be evetually added */
+ strbuf_addbuf(final, state->output);
+ strbuf_release(state->output);
>>>
>>> I guess the idea here is that you intend state->output to be re-used
>>> and it is convenient to "clear" it here rather than making that the
>>> responsibility of each caller. For re-use, it is more typical to use
>>> strbuf_reset() than strbuf_release() (though Junio may disagree[1]).
>>
>> it seems like a smarter way to around this without much overhead But it
>> was more of to release it as its no longer required unless another modifier
>> atom
>> is encountered. Is it worth keeping hoping for another modifier atom
>> eventually,
>> and release it at the end like you suggested below?
>
> If I understand your question correctly, it sounds like you're asking
> about a memory micro-optimization. From an architectural standpoint,
> it's cleaner for the entity which allocates a resource to also release
> it. In this case, show_ref_array_item() allocates the strbuf, thus it
> should be the one to release it.
>
> And, although we shouldn't be worrying about micro-optimizations at
> this point, if it were to be an issue, resetting the strbuf via
> strbuf_reset(), which doesn't involve slow memory
> deallocation/reallocation, is likely to be a winner over repeated
> strbuf_release().
>
Exactly what I was asking, thanks for explaining :D
+ memset(&state, 0, sizeof(state));
+ state.quote_style = quote_style;
+ state.output = &value;
>>>
>>> It feels strange to assign a local variable reference to state.output,
>>> and there's no obvious reason why you should need to do so. I would
>>> have instead expected ref_format_state to be declared as:
>>>
>>> struct ref_formatting_state {
>>>int quote_style;
>>>struct strbuf output;
>>> };
>>>
>>> and initialized as so:
>>>
>>> memset(&state, 0, sizeof(state));
>>> state.quote_style = quote_style;
>>> strbuf_init(&state.output, 0);
>>
>> This looks neater, thanks. It'll go along with the previous patch.
>>
>>> (In fact, the memset() isn't even necessary here since you're
>>> initializing all fields explicitly, though perhaps you want the
>>> memset() because a future patch adds more fields which are not
>>> initialized explicitly?)
>>
>> Yea the memset is needed for bit fields evnetually added in the future.
>
> Perhaps move the memset() to the first patch which actually requires
> it, where it won't be (effectively) dead code, as it becomes here once
> you make the above change.
>
But why would I need it there, we need to only memset() the ref_formatting_state
which is introduced here. Also here it helps in setting the strbuf
within ref_formatting_state
to {0, 0, 0}.
for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
- struct atom_value *atomv;
+ struct atom_value *atomv = NULL;
>>>
>>> What is this change about?
>>
>> To remove the warning about atomv being unassigned before usage.
>
> Hmm, where were you seeing that warning? The first use of 'atomv'
> following its declaration is in the get_ref_atom_value() below, and
> (as far as the compiler knows) that should be setting its value.
>
I'll check this out.
ep = strchr(sp, ')');
- if (cp < sp)
- emit(cp, sp, &output);
+ if (cp < sp) {
+ emit(cp, sp, &state);
+ apply_formatting_state(&state, &final_buf);
+ }
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2,
ep), &atomv);
- print_value(atomv, quote_style, &output);
+ process_formatting_state(atomv, &state);
+ print_value(atomv, &state);
+ apply_formatting_state(&state, &final_buf);
}
if (*cp) {
sp = cp + strlen(cp);
- emit(cp, sp, &output);
+ emit(cp, sp, &state);
+ apply_formatting_state(&state, &final_buf);
>>>
>>> I'm getting the feeling that these functions
>>> (process_formatting_state, print_value, emit, apply_formatting_state)
>>> are becoming misnamed (again) with the latest structural changes (but
>>> perhaps I haven't read far enough into the series yet?).
>>>
>>> process_formatting_state() is rather generic.
>>
>> perhaps set_formatting_state()?
>
> I don't know. I don't have a proper high-level overview of the
> functionality yet to say if that is a good name o
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Thu, Aug 6, 2015 at 11:53 PM, Karthik Nayak wrote:
> On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine wrote:
>> On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak wrote:
>>> +static void apply_formatting_state(struct ref_formatting_state *state,
>>> struct strbuf *final)
>>> +{
>>> + /* More formatting options to be evetually added */
>>> + strbuf_addbuf(final, state->output);
>>> + strbuf_release(state->output);
>>
>> I guess the idea here is that you intend state->output to be re-used
>> and it is convenient to "clear" it here rather than making that the
>> responsibility of each caller. For re-use, it is more typical to use
>> strbuf_reset() than strbuf_release() (though Junio may disagree[1]).
>
> it seems like a smarter way to around this without much overhead But it
> was more of to release it as its no longer required unless another modifier
> atom
> is encountered. Is it worth keeping hoping for another modifier atom
> eventually,
> and release it at the end like you suggested below?
If I understand your question correctly, it sounds like you're asking
about a memory micro-optimization. From an architectural standpoint,
it's cleaner for the entity which allocates a resource to also release
it. In this case, show_ref_array_item() allocates the strbuf, thus it
should be the one to release it.
And, although we shouldn't be worrying about micro-optimizations at
this point, if it were to be an issue, resetting the strbuf via
strbuf_reset(), which doesn't involve slow memory
deallocation/reallocation, is likely to be a winner over repeated
strbuf_release().
>>> + memset(&state, 0, sizeof(state));
>>> + state.quote_style = quote_style;
>>> + state.output = &value;
>>
>> It feels strange to assign a local variable reference to state.output,
>> and there's no obvious reason why you should need to do so. I would
>> have instead expected ref_format_state to be declared as:
>>
>> struct ref_formatting_state {
>>int quote_style;
>>struct strbuf output;
>> };
>>
>> and initialized as so:
>>
>> memset(&state, 0, sizeof(state));
>> state.quote_style = quote_style;
>> strbuf_init(&state.output, 0);
>
> This looks neater, thanks. It'll go along with the previous patch.
>
>> (In fact, the memset() isn't even necessary here since you're
>> initializing all fields explicitly, though perhaps you want the
>> memset() because a future patch adds more fields which are not
>> initialized explicitly?)
>
> Yea the memset is needed for bit fields evnetually added in the future.
Perhaps move the memset() to the first patch which actually requires
it, where it won't be (effectively) dead code, as it becomes here once
you make the above change.
>>> for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>>> - struct atom_value *atomv;
>>> + struct atom_value *atomv = NULL;
>>
>> What is this change about?
>
> To remove the warning about atomv being unassigned before usage.
Hmm, where were you seeing that warning? The first use of 'atomv'
following its declaration is in the get_ref_atom_value() below, and
(as far as the compiler knows) that should be setting its value.
>>> ep = strchr(sp, ')');
>>> - if (cp < sp)
>>> - emit(cp, sp, &output);
>>> + if (cp < sp) {
>>> + emit(cp, sp, &state);
>>> + apply_formatting_state(&state, &final_buf);
>>> + }
>>> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
>>> &atomv);
>>> - print_value(atomv, quote_style, &output);
>>> + process_formatting_state(atomv, &state);
>>> + print_value(atomv, &state);
>>> + apply_formatting_state(&state, &final_buf);
>>> }
>>> if (*cp) {
>>> sp = cp + strlen(cp);
>>> - emit(cp, sp, &output);
>>> + emit(cp, sp, &state);
>>> + apply_formatting_state(&state, &final_buf);
>>
>> I'm getting the feeling that these functions
>> (process_formatting_state, print_value, emit, apply_formatting_state)
>> are becoming misnamed (again) with the latest structural changes (but
>> perhaps I haven't read far enough into the series yet?).
>>
>> process_formatting_state() is rather generic.
>
> perhaps set_formatting_state()?
I don't know. I don't have a proper high-level overview of the
functionality yet to say if that is a good name or not (which is one
reason I didn't suggest an alternative).
>> print_value() and emit() both imply outputting something, but neither
>> does so anymore.
>
> I think I'll append a "to_state" to each of them.
Meh. print_value() might be better named format_value(). emit() might
become append_literal() or append_non_atom() or something.
>> apply_formatting_state() seems to be more about finalizing the
>> already-formatted output.
>
> perform_state_for
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine wrote:
> On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak wrote:
>> Introduce a ref_formatting_state which will eventually hold the values
>> of modifier atoms. Implement this within ref-filter.
>>
>> Signed-off-by: Karthik Nayak
>> ---
>> +static void apply_formatting_state(struct ref_formatting_state *state,
>> struct strbuf *final)
>> +{
>> + /* More formatting options to be evetually added */
>> + strbuf_addbuf(final, state->output);
>> + strbuf_release(state->output);
>
> I guess the idea here is that you intend state->output to be re-used
> and it is convenient to "clear" it here rather than making that the
> responsibility of each caller. For re-use, it is more typical to use
> strbuf_reset() than strbuf_release() (though Junio may disagree[1]).
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/273094
>
it seems like a smarter way to around this without much overhead But it
was more of to release it as its no longer required unless another modifier atom
is encountered. Is it worth keeping hoping for another modifier atom eventually,
and release it at the end like you suggested below?
>> +}
>> +
>> void show_ref_array_item(struct ref_array_item *info, const char *format,
>> int quote_style)
>> {
>> const char *cp, *sp, *ep;
>> - struct strbuf output = STRBUF_INIT;
>> + struct strbuf value = STRBUF_INIT;
>> + struct strbuf final_buf = STRBUF_INIT;
>> + struct ref_formatting_state state;
>> int i;
>>
>> + memset(&state, 0, sizeof(state));
>> + state.quote_style = quote_style;
>> + state.output = &value;
>
> It feels strange to assign a local variable reference to state.output,
> and there's no obvious reason why you should need to do so. I would
> have instead expected ref_format_state to be declared as:
>
> struct ref_formatting_state {
>int quote_style;
>struct strbuf output;
> };
>
> and initialized as so:
>
> memset(&state, 0, sizeof(state));
> state.quote_style = quote_style;
> strbuf_init(&state.output, 0);
>
This looks neater, thanks. It'll go along with the previous patch.
> (In fact, the memset() isn't even necessary here since you're
> initializing all fields explicitly, though perhaps you want the
> memset() because a future patch adds more fields which are not
> initialized explicitly?)
>
Yea the memset is needed for bit fields evnetually added in the future.
> This still allows re-use via strbuf_reset() mentioned above.
>
> And, of course, you'd want to strbuf_release() it at the end of this
> function where you're already releasing final_buf.
>
Addressed this above.
>> for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>> - struct atom_value *atomv;
>> + struct atom_value *atomv = NULL;
>
> What is this change about?
>
To remove the warning about atomv being unassigned before usage.
>> ep = strchr(sp, ')');
>> - if (cp < sp)
>> - emit(cp, sp, &output);
>> + if (cp < sp) {
>> + emit(cp, sp, &state);
>> + apply_formatting_state(&state, &final_buf);
>> + }
>> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
>> &atomv);
>> - print_value(atomv, quote_style, &output);
>> + process_formatting_state(atomv, &state);
>> + print_value(atomv, &state);
>> + apply_formatting_state(&state, &final_buf);
>> }
>> if (*cp) {
>> sp = cp + strlen(cp);
>> - emit(cp, sp, &output);
>> + emit(cp, sp, &state);
>> + apply_formatting_state(&state, &final_buf);
>
> I'm getting the feeling that these functions
> (process_formatting_state, print_value, emit, apply_formatting_state)
> are becoming misnamed (again) with the latest structural changes (but
> perhaps I haven't read far enough into the series yet?).
>
> process_formatting_state() is rather generic.
>
perhaps set_formatting_state()?
> print_value() and emit() both imply outputting something, but neither
> does so anymore.
>
I think I'll append a "to_state" to each of them.
> apply_formatting_state() seems to be more about finalizing the
> already-formatted output.
perform_state_formatting()? perhaps.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak wrote:
> Introduce a ref_formatting_state which will eventually hold the values
> of modifier atoms. Implement this within ref-filter.
>
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 91482c9..2c074a1 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1238,35 +1238,58 @@ static void emit(const char *cp, const char *ep,
> struct strbuf *output)
> +static void apply_formatting_state(struct ref_formatting_state *state,
> struct strbuf *final)
> +{
> + /* More formatting options to be evetually added */
I forgot to mention this in my earlier review of this patch:
s/evetually/eventually/
> + strbuf_addbuf(final, state->output);
> + strbuf_release(state->output);
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak wrote:
> Introduce a ref_formatting_state which will eventually hold the values
> of modifier atoms. Implement this within ref-filter.
>
> Signed-off-by: Karthik Nayak
> ---
> +static void apply_formatting_state(struct ref_formatting_state *state,
> struct strbuf *final)
> +{
> + /* More formatting options to be evetually added */
> + strbuf_addbuf(final, state->output);
> + strbuf_release(state->output);
I guess the idea here is that you intend state->output to be re-used
and it is convenient to "clear" it here rather than making that the
responsibility of each caller. For re-use, it is more typical to use
strbuf_reset() than strbuf_release() (though Junio may disagree[1]).
[1]: http://article.gmane.org/gmane.comp.version-control.git/273094
> +}
> +
> void show_ref_array_item(struct ref_array_item *info, const char *format,
> int quote_style)
> {
> const char *cp, *sp, *ep;
> - struct strbuf output = STRBUF_INIT;
> + struct strbuf value = STRBUF_INIT;
> + struct strbuf final_buf = STRBUF_INIT;
> + struct ref_formatting_state state;
> int i;
>
> + memset(&state, 0, sizeof(state));
> + state.quote_style = quote_style;
> + state.output = &value;
It feels strange to assign a local variable reference to state.output,
and there's no obvious reason why you should need to do so. I would
have instead expected ref_format_state to be declared as:
struct ref_formatting_state {
int quote_style;
struct strbuf output;
};
and initialized as so:
memset(&state, 0, sizeof(state));
state.quote_style = quote_style;
strbuf_init(&state.output, 0);
(In fact, the memset() isn't even necessary here since you're
initializing all fields explicitly, though perhaps you want the
memset() because a future patch adds more fields which are not
initialized explicitly?)
This still allows re-use via strbuf_reset() mentioned above.
And, of course, you'd want to strbuf_release() it at the end of this
function where you're already releasing final_buf.
> for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
> - struct atom_value *atomv;
> + struct atom_value *atomv = NULL;
What is this change about?
> ep = strchr(sp, ')');
> - if (cp < sp)
> - emit(cp, sp, &output);
> + if (cp < sp) {
> + emit(cp, sp, &state);
> + apply_formatting_state(&state, &final_buf);
> + }
> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
> &atomv);
> - print_value(atomv, quote_style, &output);
> + process_formatting_state(atomv, &state);
> + print_value(atomv, &state);
> + apply_formatting_state(&state, &final_buf);
> }
> if (*cp) {
> sp = cp + strlen(cp);
> - emit(cp, sp, &output);
> + emit(cp, sp, &state);
> + apply_formatting_state(&state, &final_buf);
I'm getting the feeling that these functions
(process_formatting_state, print_value, emit, apply_formatting_state)
are becoming misnamed (again) with the latest structural changes (but
perhaps I haven't read far enough into the series yet?).
process_formatting_state() is rather generic.
print_value() and emit() both imply outputting something, but neither
does so anymore.
apply_formatting_state() seems to be more about finalizing the
already-formatted output.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 02/11] ref-filter: introduce ref_formatting_state
Introduce a ref_formatting_state which will eventually hold the values
of modifier atoms. Implement this within ref-filter.
Mentored-by: Christian Couder
Mentored-by: Matthieu Moy
Signed-off-by: Karthik Nayak
---
ref-filter.c | 64 +---
ref-filter.h | 5 +
2 files changed, 49 insertions(+), 20 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 91482c9..2c074a1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1190,23 +1190,23 @@ void ref_array_sort(struct ref_sorting *sorting, struct
ref_array *array)
qsort(array->items, array->nr, sizeof(struct ref_array_item *),
compare_refs);
}
-static void print_value(struct atom_value *v, int quote_style, struct strbuf
*output)
+static void print_value(struct atom_value *v, struct ref_formatting_state
*state)
{
- switch (quote_style) {
+ switch (state->quote_style) {
case QUOTE_NONE:
- strbuf_addstr(output, v->s);
+ strbuf_addstr(state->output, v->s);
break;
case QUOTE_SHELL:
- sq_quote_buf(output, v->s);
+ sq_quote_buf(state->output, v->s);
break;
case QUOTE_PERL:
- perl_quote_buf(output, v->s);
+ perl_quote_buf(state->output, v->s);
break;
case QUOTE_PYTHON:
- python_quote_buf(output, v->s);
+ python_quote_buf(state->output, v->s);
break;
case QUOTE_TCL:
- tcl_quote_buf(output, v->s);
+ tcl_quote_buf(state->output, v->s);
break;
}
}
@@ -1229,7 +1229,7 @@ static int hex2(const char *cp)
return -1;
}
-static void emit(const char *cp, const char *ep, struct strbuf *output)
+static void emit(const char *cp, const char *ep, struct ref_formatting_state
*state)
{
while (*cp && (!ep || cp < ep)) {
if (*cp == '%') {
@@ -1238,35 +1238,58 @@ static void emit(const char *cp, const char *ep, struct
strbuf *output)
else {
int ch = hex2(cp + 1);
if (0 <= ch) {
- strbuf_addch(output, ch);
+ strbuf_addch(state->output, ch);
cp += 3;
continue;
}
}
}
- strbuf_addch(output, *cp);
+ strbuf_addch(state->output, *cp);
cp++;
}
}
+static void process_formatting_state(struct atom_value *atomv, struct
ref_formatting_state *state)
+{
+ /* Based on the atomv values, the formatting state is set */
+}
+
+static void apply_formatting_state(struct ref_formatting_state *state, struct
strbuf *final)
+{
+ /* More formatting options to be evetually added */
+ strbuf_addbuf(final, state->output);
+ strbuf_release(state->output);
+}
+
void show_ref_array_item(struct ref_array_item *info, const char *format, int
quote_style)
{
const char *cp, *sp, *ep;
- struct strbuf output = STRBUF_INIT;
+ struct strbuf value = STRBUF_INIT;
+ struct strbuf final_buf = STRBUF_INIT;
+ struct ref_formatting_state state;
int i;
+ memset(&state, 0, sizeof(state));
+ state.quote_style = quote_style;
+ state.output = &value;
+
for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
- struct atom_value *atomv;
+ struct atom_value *atomv = NULL;
ep = strchr(sp, ')');
- if (cp < sp)
- emit(cp, sp, &output);
+ if (cp < sp) {
+ emit(cp, sp, &state);
+ apply_formatting_state(&state, &final_buf);
+ }
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
&atomv);
- print_value(atomv, quote_style, &output);
+ process_formatting_state(atomv, &state);
+ print_value(atomv, &state);
+ apply_formatting_state(&state, &final_buf);
}
if (*cp) {
sp = cp + strlen(cp);
- emit(cp, sp, &output);
+ emit(cp, sp, &state);
+ apply_formatting_state(&state, &final_buf);
}
if (need_color_reset_at_eol) {
struct atom_value resetv;
@@ -1275,12 +1298,13 @@ void show_ref_array_item(struct ref_array_item *info,
const char *format, int qu
if (color_parse("reset", color) < 0)
die("BUG: couldn't parse 'reset' as a color");
resetv.s = color;
- print_value(&resetv, quote_style, &output);
+ print_value(&resetv, &state);
+ apply_formatting_state(&state, &final_buf
