Re: [PATCH v13 04/12] ref-filter: implement an `align` atom
On Tue, Aug 25, 2015 at 11:26 PM, Junio C Hamano wrote: > Karthik Nayak writes: > >> I like the idea of using atomv->handler() a lot, mostly cause this >> would eventually >> help us clean up populate_atom() which currently seems like a huge dump of >> code. > > I think you already said that last time we had this discussion ;-) > > http://thread.gmane.org/gmane.comp.version-control.git/275537/focus=275778 > Yes :) I really want to clean that up eventually, every since Duy mentioned about it. -- 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 v13 04/12] ref-filter: implement an `align` atom
Karthik Nayak writes: > I like the idea of using atomv->handler() a lot, mostly cause this > would eventually > help us clean up populate_atom() which currently seems like a huge dump of > code. I think you already said that last time we had this discussion ;-) http://thread.gmane.org/gmane.comp.version-control.git/275537/focus=275778 -- 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 v13 04/12] ref-filter: implement an `align` atom
Matthieu Moy writes:
> Junio C Hamano writes:
>
>> You can see that I expected that "if !state.stack->prev" check to be
>> inside append_atom(), and I would imagine future readers would have
>> the same expectation when reading this code. I.e.
>>
>> append_atom(struct atom_value *v, struct ref_f_s *state)
>> {
>> if (state->stack->prev)
>> strbuf_addstr(&state->stack->output, v->s);
>> else
>> quote_format(&state->stack->output, v->s,
>> state->quote_style);
>> }
>>
>> The end result may be the same,
>
> There's another call to append_atom() when inserting the "reset color at
> end of line if needed", so moving this if inside append_atom means we
> would do the check also for the reset color. It would not change the
> behavior (by construction, we insert it only when the stack has only the
> initial element), so it's OK.
Thanks for checking---I did overlook that other callsite and did not
check if the suggested change was sensible 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 v13 04/12] ref-filter: implement an `align` atom
On Tue, Aug 25, 2015 at 3:45 AM, Junio C Hamano wrote:
> Junio C Hamano writes:
>
>> Karthik Nayak writes:
>>
>>> +static void end_atom_handler(struct atom_value *atomv, struct
>>> ref_formatting_state *state)
>>> +{
>>> +struct ref_formatting_stack *current = state->stack;
>>> +struct strbuf s = STRBUF_INIT;
>>> +
>>> +if (!current->at_end)
>>> +die(_("format: `end` atom used without a supporting atom"));
>>> +current->at_end(current);
>>> +/*
>>> + * Whenever we have more than one stack element that means we
>>> + * are using a certain modifier atom. In that case we need to
>>> + * perform quote formatting.
>>> + */
>>> +if (!state->stack->prev->prev) {
>>
>> The comment and the condition seem to be saying opposite things.
>> The code says "If the stack only has one prev that is the very
>> initial one, then we do the quoting, i.e. the result of expanding
>> the enclosed string in %(start-something)...%(end) is quoted only
>> when that appears at the top level", which feels more correct...
>
> As this already allows us to use nested construct, I think we would
> want to have test that uses
>
> %(align:30,left)%(align:20,right)%(refname:short)%(end)%(end)
>
> or something like that ;-)
>
> Very nice.
Ok, will do that :)
--
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 v13 04/12] ref-filter: implement an `align` atom
On Tue, Aug 25, 2015 at 12:17 PM, Matthieu Moy
wrote:
> Junio C Hamano writes:
>
>> You can see that I expected that "if !state.stack->prev" check to be
>> inside append_atom(), and I would imagine future readers would have
>> the same expectation when reading this code. I.e.
>>
>> append_atom(struct atom_value *v, struct ref_f_s *state)
>> {
>> if (state->stack->prev)
>> strbuf_addstr(&state->stack->output, v->s);
>> else
>> quote_format(&state->stack->output, v->s,
>> state->quote_style);
>> }
>>
>> The end result may be the same,
>
> There's another call to append_atom() when inserting the "reset color at
> end of line if needed", so moving this if inside append_atom means we
> would do the check also for the reset color. It would not change the
> behavior (by construction, we insert it only when the stack has only the
> initial element), so it's OK.
>
> I agree that this is a good thing to do.
>
>> Moreover, notice that the function signature of append_atom() is
>> exactly the same as atomv->handler's. I wonder if it would be
>> easier to understand if you made append_atom() the handler for a
>> non-magic atoms, which would let you do the above without any if/else
>> and just a single unconditional
>
> I can't decide between "ah, very elegant" and "no, too much magic" ;-).
> I lean towards the former.
>
I like the idea of using atomv->handler() a lot, mostly cause this
would eventually
help us clean up populate_atom() which currently seems like a huge dump of code.
--
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 v13 04/12] ref-filter: implement an `align` atom
On Tue, Aug 25, 2015 at 3:43 AM, Junio C Hamano wrote:
> Karthik Nayak writes:
>
>> +static void perform_quote_formatting(struct strbuf *s, const char *str, int
>> quote_style);
>> +
>> +static void end_atom_handler(struct atom_value *atomv, struct
>> ref_formatting_state *state)
>> +{
>> + struct ref_formatting_stack *current = state->stack;
>> + struct strbuf s = STRBUF_INIT;
>> +
>> + if (!current->at_end)
>> + die(_("format: `end` atom used without a supporting atom"));
>> + current->at_end(current);
>> + /*
>> + * Whenever we have more than one stack element that means we
>> + * are using a certain modifier atom. In that case we need to
>> + * perform quote formatting.
>> + */
>> + if (!state->stack->prev->prev) {
>
> The comment and the condition seem to be saying opposite things.
> The code says "If the stack only has one prev that is the very
> initial one, then we do the quoting, i.e. the result of expanding
> the enclosed string in %(start-something)...%(end) is quoted only
> when that appears at the top level", which feels more correct than
> the comment that says "if we are about to pop after seeing the
> first %(end) in %(start)...%(another)...%(end)...%(end) sequence,
> we quote what is between %(another)...%(end)".
>
That sounds misleading indeed will need to change that.
>> + perform_quote_formatting(&s, current->output.buf,
>> state->quote_style);
>> + strbuf_reset(¤t->output);
>> + strbuf_addbuf(¤t->output, &s);
>> + }
>> + strbuf_release(&s);
>> + pop_stack_element(&state->stack);
>> +}
>> +
>
>> @@ -1228,29 +1315,33 @@ 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 append_atom(struct atom_value *v, struct ref_formatting_state
>> *state)
>> +static void perform_quote_formatting(struct strbuf *s, const char *str, int
>> quote_style)
>> {
>> - struct strbuf *s = &state->stack->output;
>> -
>> - switch (state->quote_style) {
>> + switch (quote_style) {
>> case QUOTE_NONE:
>> - strbuf_addstr(s, v->s);
>> + strbuf_addstr(s, str);
>> break;
>> case QUOTE_SHELL:
>> - sq_quote_buf(s, v->s);
>> + sq_quote_buf(s, str);
>> break;
>> case QUOTE_PERL:
>> - perl_quote_buf(s, v->s);
>> + perl_quote_buf(s, str);
>> break;
>> case QUOTE_PYTHON:
>> - python_quote_buf(s, v->s);
>> + python_quote_buf(s, str);
>> break;
>> case QUOTE_TCL:
>> - tcl_quote_buf(s, v->s);
>> + tcl_quote_buf(s, str);
>> break;
>> }
>> }
>>
>> +static void append_atom(struct atom_value *v, struct ref_formatting_state
>> *state)
>> +{
>> + struct strbuf *s = &state->stack->output;
>> + perform_quote_formatting(s, v->s, state->quote_style);
>
> Hmmm, do we want to unconditionally do the quote here, or only when
> we are not being captured by upcoming %(end) to be consistent with
> the behaviour of end_atom_handler() above?
>
>> @@ -1307,7 +1398,18 @@ void show_ref_array_item(struct ref_array_item *info,
>> const char *format, int qu
>> if (cp < sp)
>> append_literal(cp, sp, &state);
>> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
>> &atomv);
>> - append_atom(atomv, &state);
>> + /*
>> + * If the atom is a modifier atom, then call the handler
>> function.
>> + * Else, if this is the first element on the stack, then we
>> need to
>> + * format the atom as per the given quote. Else we just add
>> the atom value
>> + * to the current stack element and handle quote formatting at
>> the end.
>> + */
>> + if (atomv->handler)
>> + atomv->handler(atomv, &state);
>> + else if (!state.stack->prev)
>> + append_atom(atomv, &state);
>> + else
>> + strbuf_addstr(&state.stack->output, atomv->s);
>
> Ahh, this explains why you are not doing it above, but I do not
> think if this is a good division of labor.
>
> You can see that I expected that "if !state.stack->prev" check to be
> inside append_atom(), and I would imagine future readers would have
> the same expectation when reading this code. I.e.
>
> append_atom(struct atom_value *v, struct ref_f_s *state)
> {
> if (state->stack->prev)
> strbuf_addstr(&state->stack->output, v->s);
> else
> quote_format(&state->stack->output, v->s,
> state->quote_style);
> }
>
> The end result may be the same, but I do think "append_atom is to
> always quote, s
Re: [PATCH v13 04/12] ref-filter: implement an `align` atom
Junio C Hamano writes:
> You can see that I expected that "if !state.stack->prev" check to be
> inside append_atom(), and I would imagine future readers would have
> the same expectation when reading this code. I.e.
>
> append_atom(struct atom_value *v, struct ref_f_s *state)
> {
> if (state->stack->prev)
> strbuf_addstr(&state->stack->output, v->s);
> else
> quote_format(&state->stack->output, v->s,
> state->quote_style);
> }
>
> The end result may be the same,
There's another call to append_atom() when inserting the "reset color at
end of line if needed", so moving this if inside append_atom means we
would do the check also for the reset color. It would not change the
behavior (by construction, we insert it only when the stack has only the
initial element), so it's OK.
I agree that this is a good thing to do.
> Moreover, notice that the function signature of append_atom() is
> exactly the same as atomv->handler's. I wonder if it would be
> easier to understand if you made append_atom() the handler for a
> non-magic atoms, which would let you do the above without any if/else
> and just a single unconditional
I can't decide between "ah, very elegant" and "no, too much magic" ;-).
I lean towards the former.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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 v13 04/12] ref-filter: implement an `align` atom
Junio C Hamano writes:
> Karthik Nayak writes:
>
>> +static void end_atom_handler(struct atom_value *atomv, struct
>> ref_formatting_state *state)
>> +{
>> +struct ref_formatting_stack *current = state->stack;
>> +struct strbuf s = STRBUF_INIT;
>> +
>> +if (!current->at_end)
>> +die(_("format: `end` atom used without a supporting atom"));
>> +current->at_end(current);
>> +/*
>> + * Whenever we have more than one stack element that means we
>> + * are using a certain modifier atom. In that case we need to
>> + * perform quote formatting.
>> + */
>> +if (!state->stack->prev->prev) {
>
> The comment and the condition seem to be saying opposite things.
> The code says "If the stack only has one prev that is the very
> initial one, then we do the quoting, i.e. the result of expanding
> the enclosed string in %(start-something)...%(end) is quoted only
> when that appears at the top level", which feels more correct...
As this already allows us to use nested construct, I think we would
want to have test that uses
%(align:30,left)%(align:20,right)%(refname:short)%(end)%(end)
or something like that ;-)
Very nice.
--
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 v13 04/12] ref-filter: implement an `align` atom
Karthik Nayak writes:
> +static void perform_quote_formatting(struct strbuf *s, const char *str, int
> quote_style);
> +
> +static void end_atom_handler(struct atom_value *atomv, struct
> ref_formatting_state *state)
> +{
> + struct ref_formatting_stack *current = state->stack;
> + struct strbuf s = STRBUF_INIT;
> +
> + if (!current->at_end)
> + die(_("format: `end` atom used without a supporting atom"));
> + current->at_end(current);
> + /*
> + * Whenever we have more than one stack element that means we
> + * are using a certain modifier atom. In that case we need to
> + * perform quote formatting.
> + */
> + if (!state->stack->prev->prev) {
The comment and the condition seem to be saying opposite things.
The code says "If the stack only has one prev that is the very
initial one, then we do the quoting, i.e. the result of expanding
the enclosed string in %(start-something)...%(end) is quoted only
when that appears at the top level", which feels more correct than
the comment that says "if we are about to pop after seeing the
first %(end) in %(start)...%(another)...%(end)...%(end) sequence,
we quote what is between %(another)...%(end)".
> + perform_quote_formatting(&s, current->output.buf,
> state->quote_style);
> + strbuf_reset(¤t->output);
> + strbuf_addbuf(¤t->output, &s);
> + }
> + strbuf_release(&s);
> + pop_stack_element(&state->stack);
> +}
> +
> @@ -1228,29 +1315,33 @@ 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 append_atom(struct atom_value *v, struct ref_formatting_state
> *state)
> +static void perform_quote_formatting(struct strbuf *s, const char *str, int
> quote_style)
> {
> - struct strbuf *s = &state->stack->output;
> -
> - switch (state->quote_style) {
> + switch (quote_style) {
> case QUOTE_NONE:
> - strbuf_addstr(s, v->s);
> + strbuf_addstr(s, str);
> break;
> case QUOTE_SHELL:
> - sq_quote_buf(s, v->s);
> + sq_quote_buf(s, str);
> break;
> case QUOTE_PERL:
> - perl_quote_buf(s, v->s);
> + perl_quote_buf(s, str);
> break;
> case QUOTE_PYTHON:
> - python_quote_buf(s, v->s);
> + python_quote_buf(s, str);
> break;
> case QUOTE_TCL:
> - tcl_quote_buf(s, v->s);
> + tcl_quote_buf(s, str);
> break;
> }
> }
>
> +static void append_atom(struct atom_value *v, struct ref_formatting_state
> *state)
> +{
> + struct strbuf *s = &state->stack->output;
> + perform_quote_formatting(s, v->s, state->quote_style);
Hmmm, do we want to unconditionally do the quote here, or only when
we are not being captured by upcoming %(end) to be consistent with
the behaviour of end_atom_handler() above?
> @@ -1307,7 +1398,18 @@ void show_ref_array_item(struct ref_array_item *info,
> const char *format, int qu
> if (cp < sp)
> append_literal(cp, sp, &state);
> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
> &atomv);
> - append_atom(atomv, &state);
> + /*
> + * If the atom is a modifier atom, then call the handler
> function.
> + * Else, if this is the first element on the stack, then we
> need to
> + * format the atom as per the given quote. Else we just add the
> atom value
> + * to the current stack element and handle quote formatting at
> the end.
> + */
> + if (atomv->handler)
> + atomv->handler(atomv, &state);
> + else if (!state.stack->prev)
> + append_atom(atomv, &state);
> + else
> + strbuf_addstr(&state.stack->output, atomv->s);
Ahh, this explains why you are not doing it above, but I do not
think if this is a good division of labor.
You can see that I expected that "if !state.stack->prev" check to be
inside append_atom(), and I would imagine future readers would have
the same expectation when reading this code. I.e.
append_atom(struct atom_value *v, struct ref_f_s *state)
{
if (state->stack->prev)
strbuf_addstr(&state->stack->output, v->s);
else
quote_format(&state->stack->output, v->s,
state->quote_style);
}
The end result may be the same, but I do think "append_atom is to
always quote, so we do an unquoted appending by hand" is a bad way
to do this.
Moreover, notice that the function signature of append_atom() is
exactly the same as atomv->handler's. I wonder if it would be
easier to understand if you made append_atom() the handler for a
non-magic
