Re: [PATCH v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery
Junio C Hamano gits...@pobox.com writes: So I think 'quote' should apply only to the top-level atoms in the nested %(magic)...%(end) world. This is true in most cases, but I think there would also be use-cases where you would want the opposite, like: --format ' %(if:whatever) echo %(refname) %(end) ' I'm not sure what's best, but if both can make sense, perhaps we should just keep the simplest to implement, i.e. the current behavior. -- 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 v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery
Karthik Nayak karthik@gmail.com writes: Speaking of quote_value, The quote doesn't work well with color's for e.g. git for-each-ref --shell --format=%(color:green)%(refname) '''refs/heads/allow-unknown-type''' Seems like an simple fix, probably after GSoC I'll do this :) Anyway, the %(color) is really meant to be displayed on-screen, and the quoting is really meant to feed the value to another program, so I can hardly imagine a use-case where you would want both. But the current behavior seems fine to me: the color escape sequence is quoted, which is good. For example, you can x=$(git for-each-ref --shell --format=nocolor%(color:green)%(refname) | head -n 1) sh -c echo $x it will actually display nocolor without color, then a green refname. I'm not sure the quoting is really necessary, but it doesn't harm and it makes sense since the escape sequence contains a '[' which is a shell metacharacter. -- 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 v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery
On Thu, Aug 20, 2015 at 12:22 AM, Junio C Hamano gits...@pobox.com wrote:
Matthieu Moy matthieu@grenoble-inp.fr writes:
You replaced the quote_style argument with ref_formatting_state, and I
think you should have kept this argument and added ref_formatting_state.
The other option is to add an extra indirection like
struct ref_formatting_state {
int quote_style;
struct ref_formatting_stack *stack;
}
(ref_formatting_stack would be what you currently call
ref_formatting_state). But that's probably overkill.
I think this is the right way to go. As you explained in your later
messages, this is conceptually a global setting that applies to
anybody working in the callchain and not something individual
recursion levels would want to muck with.
Thanks.
I'll work on this :)
--
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 v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery
On Thu, Aug 20, 2015 at 12:59 PM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Karthik Nayak karthik@gmail.com writes: Speaking of quote_value, The quote doesn't work well with color's for e.g. git for-each-ref --shell --format=%(color:green)%(refname) '''refs/heads/allow-unknown-type''' Seems like an simple fix, probably after GSoC I'll do this :) Anyway, the %(color) is really meant to be displayed on-screen, and the quoting is really meant to feed the value to another program, so I can hardly imagine a use-case where you would want both. But the current behavior seems fine to me: the color escape sequence is quoted, which is good. For example, you can x=$(git for-each-ref --shell --format=nocolor%(color:green)%(refname) | head -n 1) sh -c echo $x it will actually display nocolor without color, then a green refname. I'm not sure the quoting is really necessary, but it doesn't harm and it makes sense since the escape sequence contains a '[' which is a shell metacharacter. Thanks for explaining that! I agree with whatever you've said. -- 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 v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery
Matthieu Moy matthieu@grenoble-inp.fr writes:
Junio C Hamano gits...@pobox.com writes:
So I think 'quote' should apply only to the top-level atoms in the
nested %(magic)...%(end) world.
This is true in most cases, but I think there would also be use-cases
where you would want the opposite, like:
--format '
%(if:whatever)
echo %(refname)
%(end)
'
I'm not sure what's best, but if both can make sense, perhaps we should
just keep the simplest to implement, i.e. the current behavior.
I am reasonably sure what's best, as --{shell,tcl,...} with --format
is my invention ;-)
The whole point of these language specific quotes to have
--format output to be an executable script is so that the user can
express control in that scripting language. We must be able process
the examples in the message you are responding to, i.e. allowing
%(atom) and %(magic)...%(end) correctly assigned to a variable of
the target language. If that implementation happens to also grok
your %(if:whatever)...%(then)echo %(refname)%(end) example in a
way you expect, that would be great, but if not, then I do not think
it is worth worrying about it. On the other hand, a solution that
does not solve the primary use case is worthless, even if it is
simple to implement.
I do not think we deeply mind if we forbid use if %(if)...%(end)
when quoting is in use, if the current implementation too broken
beyond salvaging. I however think that %(align:40)%(atom)%(end)
would want to be usable even with quoting, and I suspect that an
implementation that groks %(align) correctly would automatically
grok %(if), too.
So...
--
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 v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery
Matthieu Moy matthieu@grenoble-inp.fr writes: Karthik Nayak karthik@gmail.com writes: Speaking of quote_value, The quote doesn't work well with color's for e.g. git for-each-ref --shell --format=%(color:green)%(refname) '''refs/heads/allow-unknown-type''' Seems like an simple fix, probably after GSoC I'll do this :) Anyway, the %(color) is really meant to be displayed on-screen, and the quoting is really meant to feed the value to another program, so I can hardly imagine a use-case where you would want both. But the current behavior seems fine to me: the color escape sequence is quoted, which is good. For example, you can x=$(git for-each-ref --shell --format=nocolor%(color:green)%(refname) | head -n 1) sh -c echo $x it will actually display nocolor without color, then a green refname. I'm not sure the quoting is really necessary, but it doesn't harm and it makes sense since the escape sequence contains a '[' which is a shell metacharacter. The point of --shell/--tcl/... is so that you can have --format safely write executable scripts in the specified language. Your format string might look like this: --format=short=%(refname:short) long=%(refname) and one entry in the output in --shell mode would expand to short='master' long='refs/heads/master' that can be eval'ed as a script safely without having to worry about expanded atom values having characters that have special meanings in the target language. Your nocolor example works the same way: --format=var=%(color:green)%(refname) --shell would scan 'var=', emit it as literal, see %(color:green) atom, show it quoted, see %(refname), show it quoted, notice that color is not terminated and pretend as if it saw %(color:reset) and show it quoted, which would result in something like: var='ESC[32m''master''ESC[m' Note that the example _knows_ that the quoting rule of the target language, namely, two 'quoted' 'strings' next to each other are simply concatenated. When using a hypothetical target language whose quoting rule is different, e.g. type two single-quotes inside a pair of single-quote to represent a literal single-quote, then you would write something like this to produce a script in that language: --format= var1=%(color:green); var2=%(refname); var=var1+var2; as your format string (and it will not be used with --shell). And the atom-quoting code that knows the language specific rules would quote %(atom) properly. Perhaps the language uses `' for its string quoting, in which case one entry of the output might look like var1=`ESC[32m'; var2=`refs/heads/master'; var=var1+var2; which would be in the valid syntax of that hypothetical language. Maybe you have an atom %(headstar) that expands to an asterisk for the currently checked out branch, in order to mimick 'git branch -l'. Using that, you might use --shell --format to invent a shorter output format that does not show the asterisk but indicates the current branch only with color, like so: --format=' if test -z %(headstar) then echo %(refname:short) else echo %(color:green)%(refname:short)%(color:reset) fi ' and you would want %(headstar)'s expansion to be '*' or ''. If we introduce %(if:empty)%(then)%(else)%(end), the above may become something like this, removing the need for --shell altogether: %(if:empty)%(headstar)%(then )%(refname:short)%(else )%(color:green)%(refname:short)%(color:reset)%(end) With the current implementation, it is likely that this needs to be a single long line; we may want to extend parsing of atoms to allow a LF+whitespace before the close parenthesis to make the string more readable like the above example, but that is an unrelated tangent. But you should still be able to use --shell this way, assigning the whole thing to a variable: --format=' line=%(if:empty)%(headstar)%(then )%(refname:short)%(else )%(color:green)%(refname:short)%(color:reset)%(end) echo $line ' So I think 'quote' should apply only to the top-level atoms in the nested %(magic)...%(end) world. Expand %(if:empty)...%(end) and then apply the quoting rule specific to the target language to make the result safe to use as the RHS of the target language. None of the atoms that appear internally (e.g. %(headstar) that is being tested for emptyness) must NOT be quoted. If you have %(align:40)%(atom) and string%(end), the same logic applies. %(atom) is not a top level item (it is inside %(align)) so you would expand %(atom) and string without quoting, measure its display width, align to 40-cols and then if --shell or any quoting is in effect, applyl that, so that the user can do: --format=' right=%(align:40)%(refname)%(end)
Re: [PATCH v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery
Junio C Hamano gits...@pobox.com writes:
Matthieu Moy matthieu@grenoble-inp.fr writes:
You replaced the quote_style argument with ref_formatting_state, and I
think you should have kept this argument and added ref_formatting_state.
The other option is to add an extra indirection like
struct ref_formatting_state {
int quote_style;
struct ref_formatting_stack *stack;
}
(ref_formatting_stack would be what you currently call
ref_formatting_state). But that's probably overkill.
I think this is the right way to go. As you explained in your later
messages, this is conceptually a global setting that applies to
anybody working in the callchain and not something individual
recursion levels would want to muck with.
The fact that this is conceptually a global setting does not change,
but I think the deeper levels should not care or even _know_ that
language-specific quoting rules exist (see other post).
Sorry for the confusion.
--
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 v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery
Karthik Nayak karthik@gmail.com writes: --- a/ref-filter.c +++ b/ref-filter.c @@ -155,6 +155,8 @@ static void push_new_state(struct ref_formatting_state **stack) strbuf_init(s-output, 0); s-prev = *stack; + if (*stack) + s-quote_style = (*stack)-quote_style; *stack = s; } This seems about right, why do you think it's a stupid fix? If you have a stack of N elemments, why replicate a field N times if all the N instances always have the same value? There's nothing to be pushed or poped with quote_style, so having it in the stack is confusing to the reader (one has to infer the property all instances have the same value by reading the code instead of having just one variable), and error-prone for the author: you already got it wrong once. -- 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 v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery
Karthik Nayak karthik@gmail.com writes:
+static void pop_state(struct ref_formatting_state **stack)
+{
+ struct ref_formatting_state *current = *stack;
+ struct ref_formatting_state *prev = current-prev;
+
+ if (prev)
+ strbuf_addbuf(prev-output, current-output);
I find this if (prev) suspicious: if there's a previous element in the
stack, push to it, but otherwise, you're throwing away the content of
the stack top silently.
Given the rest of the patch, this is correct, since you're using
state-output before pop_state(), but I find it weird to have the same
function to actually pop a state, and to destroy the last element.
Just thinking out loudly, I don't have specific alternative to propose
here.
@@ -1262,23 +1284,24 @@ static void append_literal(const char *cp, const char
*ep, struct ref_formatting
void show_ref_array_item(struct ref_array_item *info, const char *format,
int quote_style)
{
const char *cp, *sp, *ep;
- struct ref_formatting_state state;
+ struct strbuf *final_buf;
+ struct ref_formatting_state *state = NULL;
- strbuf_init(state.output, 0);
- state.quote_style = quote_style;
+ push_new_state(state);
+ state-quote_style = quote_style;
I do not think that the quote_style should belong to the stack. At the
moment, only the bottom of the stack has it set, and as a result you're
getting weird results like:
$ ./git for-each-ref --shell --format '|%(align:80,left)%(author)%(end)|' |
head -n 3
|Junio C Hamano gits...@pobox.com 1435173702 -0700
''|
|Junio C Hamano gits...@pobox.com 1435173701 -0700
''|
|Junio C Hamano gits...@pobox.com 1433277352 -0700
''|
See, the '' are inserted where the %(end) was, but not around atoms as
one would expect.
One stupid fix would be to propagate the quote_style accross the stack,
like this:
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -155,6 +155,8 @@ static void push_new_state(struct ref_formatting_state
**stack)
strbuf_init(s-output, 0);
s-prev = *stack;
+ if (*stack)
+ s-quote_style = (*stack)-quote_style;
*stack = s;
}
After applying this, I do get the '' around the author (= correct
behavior I think), but then one wonders even more why this is part of
the stack.
You replaced the quote_style argument with ref_formatting_state, and I
think you should have kept this argument and added ref_formatting_state.
The other option is to add an extra indirection like
struct ref_formatting_state {
int quote_style;
struct ref_formatting_stack *stack;
}
(ref_formatting_stack would be what you currently call
ref_formatting_state). But that's probably overkill.
Also, after applying my toy patch above, I get useless '' around
%(align) and %(end). I can get rid of them with
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1499,7 +1501,8 @@ void show_ref_array_item(struct ref_array_item *info,
const char *format,
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
atomv);
if (atomv-handler)
atomv-handler(atomv, state);
- append_atom(atomv, state);
+ else
+ append_atom(atomv, state);
}
if (*cp) {
sp = cp + strlen(cp);
Unless I missed something, this second patch is sensible anyway and
should be squashed into [PATCH v12 05/13]: you don't need to call
append_atom() when you have a handler, right?
--
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 v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery
On Wed, Aug 19, 2015 at 8:26 PM, Matthieu Moy
matthieu@grenoble-inp.fr wrote:
Karthik Nayak karthik@gmail.com writes:
+static void pop_state(struct ref_formatting_state **stack)
+{
+ struct ref_formatting_state *current = *stack;
+ struct ref_formatting_state *prev = current-prev;
+
+ if (prev)
+ strbuf_addbuf(prev-output, current-output);
I find this if (prev) suspicious: if there's a previous element in the
stack, push to it, but otherwise, you're throwing away the content of
the stack top silently.
Given the rest of the patch, this is correct, since you're using
state-output before pop_state(), but I find it weird to have the same
function to actually pop a state, and to destroy the last element.
Just thinking out loudly, I don't have specific alternative to propose
here.
Hmm, but destroying the last element is also pop'ing it off the stack in a way.
I can't think of a something else.
@@ -1262,23 +1284,24 @@ static void append_literal(const char *cp, const
char *ep, struct ref_formatting
void show_ref_array_item(struct ref_array_item *info, const char *format,
int quote_style)
{
const char *cp, *sp, *ep;
- struct ref_formatting_state state;
+ struct strbuf *final_buf;
+ struct ref_formatting_state *state = NULL;
- strbuf_init(state.output, 0);
- state.quote_style = quote_style;
+ push_new_state(state);
+ state-quote_style = quote_style;
I do not think that the quote_style should belong to the stack. At the
moment, only the bottom of the stack has it set, and as a result you're
getting weird results like:
$ ./git for-each-ref --shell --format '|%(align:80,left)%(author)%(end)|' |
head -n 3
|Junio C Hamano gits...@pobox.com 1435173702 -0700
''|
|Junio C Hamano gits...@pobox.com 1435173701 -0700
''|
|Junio C Hamano gits...@pobox.com 1433277352 -0700
''|
See, the '' are inserted where the %(end) was, but not around atoms as
one would expect.
One stupid fix would be to propagate the quote_style accross the stack,
like this:
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -155,6 +155,8 @@ static void push_new_state(struct ref_formatting_state
**stack)
strbuf_init(s-output, 0);
s-prev = *stack;
+ if (*stack)
+ s-quote_style = (*stack)-quote_style;
*stack = s;
}
This seems about right, why do you think it's a stupid fix?
After applying this, I do get the '' around the author (= correct
behavior I think), but then one wonders even more why this is part of
the stack.
You replaced the quote_style argument with ref_formatting_state, and I
think you should have kept this argument and added ref_formatting_state.
The other option is to add an extra indirection like
struct ref_formatting_state {
int quote_style;
struct ref_formatting_stack *stack;
}
(ref_formatting_stack would be what you currently call
ref_formatting_state). But that's probably overkill.
Yes, seems like an overkill.
Also, after applying my toy patch above, I get useless '' around
%(align) and %(end). I can get rid of them with
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1499,7 +1501,8 @@ void show_ref_array_item(struct ref_array_item *info,
const char *format,
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
atomv);
if (atomv-handler)
atomv-handler(atomv, state);
- append_atom(atomv, state);
+ else
+ append_atom(atomv, state);
}
if (*cp) {
sp = cp + strlen(cp);
Unless I missed something, this second patch is sensible anyway and
should be squashed into [PATCH v12 05/13]: you don't need to call
append_atom() when you have a handler, right?
Yes, this I'll squash into 05/13.
--
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 v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery
On Wed, Aug 19, 2015 at 9:14 PM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Karthik Nayak karthik@gmail.com writes: --- a/ref-filter.c +++ b/ref-filter.c @@ -155,6 +155,8 @@ static void push_new_state(struct ref_formatting_state **stack) strbuf_init(s-output, 0); s-prev = *stack; + if (*stack) + s-quote_style = (*stack)-quote_style; *stack = s; } This seems about right, why do you think it's a stupid fix? If you have a stack of N elemments, why replicate a field N times if all the N instances always have the same value? There's nothing to be pushed or poped with quote_style, so having it in the stack is confusing to the reader (one has to infer the property all instances have the same value by reading the code instead of having just one variable), and error-prone for the author: you already got it wrong once. Thats also there, I'll guess it makes more sense to remove it from ref_formatting_state. -- 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 v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery
On Wed, Aug 19, 2015 at 9:24 PM, Karthik Nayak karthik@gmail.com wrote: On Wed, Aug 19, 2015 at 9:14 PM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Karthik Nayak karthik@gmail.com writes: --- a/ref-filter.c +++ b/ref-filter.c @@ -155,6 +155,8 @@ static void push_new_state(struct ref_formatting_state **stack) strbuf_init(s-output, 0); s-prev = *stack; + if (*stack) + s-quote_style = (*stack)-quote_style; *stack = s; } This seems about right, why do you think it's a stupid fix? If you have a stack of N elemments, why replicate a field N times if all the N instances always have the same value? There's nothing to be pushed or poped with quote_style, so having it in the stack is confusing to the reader (one has to infer the property all instances have the same value by reading the code instead of having just one variable), and error-prone for the author: you already got it wrong once. Thats also there, I'll guess it makes more sense to remove it from ref_formatting_state. Speaking of quote_value, The quote doesn't work well with color's for e.g. git for-each-ref --shell --format=%(color:green)%(refname) '''refs/heads/allow-unknown-type''' Seems like an simple fix, probably after GSoC I'll do this :) -- 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 v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery
Matthieu Moy matthieu@grenoble-inp.fr writes:
You replaced the quote_style argument with ref_formatting_state, and I
think you should have kept this argument and added ref_formatting_state.
The other option is to add an extra indirection like
struct ref_formatting_state {
int quote_style;
struct ref_formatting_stack *stack;
}
(ref_formatting_stack would be what you currently call
ref_formatting_state). But that's probably overkill.
I think this is the right way to go. As you explained in your later
messages, this is conceptually a global setting that applies to
anybody working in the callchain and not something individual
recursion levels would want to muck with.
Thanks.
--
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
