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
[PATCH v13 04/12] ref-filter: implement an `align` atom
Implement an `align` atom which left-, middle-, or right-aligns the
content between %(align:..) and %(end).
It is followed by `:,`, where the `` is
either left, right or middle and `` is the size of the area
into which the content will be placed. If the content between
%(align:) and %(end) is more than the width then no alignment is
performed. e.g. to align a refname atom to the middle with a total
width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)".
We now have a `handler()` for each atom_value which will be called
when that atom_value is being parsed, and similarly an `at_end`
function for each element of the stack which is to be called when the
`end` atom is encountered. Using this we implement the `align` atom
which aligns the given strbuf by calling `strbuf_utf8_align()` from
utf8.c.
Extract perform_quote_formatting() from append_atom(). Given a string
a quote_value and a strbuf, perform_quote_formatting() formats the
string based on the quote_value and stores it into the strbuf.
Ensure that quote formatting is performed on the whole of
%(align)...%(end) rather than individual atoms. We do this by skipping
individual quote formatting for atoms whenever the stack has more than
one element, and performing formatting for the entire stack element
when the `%(end)` atoms is encountered.
Add documentation and tests for the same.
Mentored-by: Christian Couder
Mentored-by: Matthieu Moy
Signed-off-by: Karthik Nayak
---
Documentation/git-for-each-ref.txt | 9 +++
ref-filter.c | 124 ++---
t/t6302-for-each-ref-filter.sh | 69 +
3 files changed, 192 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt
b/Documentation/git-for-each-ref.txt
index e49d578..943975d 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -127,6 +127,15 @@ color::
Change output color. Followed by `:`, where names
are described in `color.branch.*`.
+align::
+ Left-, middle-, or right-align the content between %(align:..)
+ and %(end). Followed by `:,`, where the
+ `` is either left, right or middle and `` is
+ the total length of the content with alignment. If the
+ contents length is more than the width then no alignment is
+ performed. If used with '--quote' everything in between %(align:..)
+ and %(end) is quoted.
+
In addition to the above, for commit and tag objects, the header
field names (`tree`, `parent`, `object`, `type`, and `tag`) can
be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index d0d8df0..ffec10a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -10,6 +10,7 @@
#include "quote.h"
#include "ref-filter.h"
#include "revision.h"
+#include "utf8.h"
typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
@@ -53,6 +54,13 @@ static struct {
{ "flag" },
{ "HEAD" },
{ "color" },
+ { "align" },
+ { "end" },
+};
+
+struct align {
+ align_type position;
+ unsigned int width;
};
#define REF_FORMATTING_STATE_INIT { 0, NULL }
@@ -60,6 +68,8 @@ static struct {
struct ref_formatting_stack {
struct ref_formatting_stack *prev;
struct strbuf output;
+ void (*at_end)(struct ref_formatting_stack *stack);
+ void *cb_data;
};
struct ref_formatting_state {
@@ -69,6 +79,8 @@ struct ref_formatting_state {
struct atom_value {
const char *s;
+ struct align *align;
+ void (*handler)(struct atom_value *atomv, struct ref_formatting_state
*state);
unsigned long ul; /* used for sorting when not FIELD_STR */
};
@@ -632,6 +644,51 @@ static inline char *copy_advance(char *dst, const char
*src)
return dst;
}
+static void align_handler(struct ref_formatting_stack *stack)
+{
+ struct align *align = (struct align *)stack->cb_data;
+ struct strbuf s = STRBUF_INIT;
+
+ strbuf_utf8_align(&s, align->position, align->width, stack->output.buf);
+ strbuf_swap(&stack->output, &s);
+ strbuf_release(&s);
+ free(align);
+}
+
+static void align_atom_handler(struct atom_value *atomv, struct
ref_formatting_state *state)
+{
+ struct ref_formatting_stack *new;
+
+ push_new_stack_element(&state->stack);
+ new = state->stack;
+ new->at_end = align_handler;
+ new->cb_data = atomv->align;
+}
+
+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
+
