Re: [PATCH v6 01/10] ref-filter: introduce 'ref_formatting_state'
On Thu, Jul 30, 2015 at 12:49 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Tuesday, July 28, 2015, Karthik Nayak karthik@gmail.com wrote:
Introduce 'ref_formatting' structure to hold values of pseudo atoms
which help only in formatting. This will eventually be used by atoms
like `color` and the `padright` atom which will be introduced in a
later patch.
Isn't this commit message outdated now that you no longer treat color
specially and since the terminology is changing from pseudo to
modifier? Also, isn't the structure now called
'ref_formatting_state' rather than 'ref_formatting'?
Yes, thanks for pointing it out. will change.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
diff --git a/ref-filter.c b/ref-filter.c
index 7561727..a919a14 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -620,7 +622,7 @@ static void populate_value(struct ref_array_item *ref)
const char *name = used_atom[i];
struct atom_value *v = ref-value[i];
int deref = 0;
- const char *refname;
+ const char *refname = NULL;
What is this change about? It doesn't seem to be related to anything
else in the patch.
In previous versions it was giving a refname not assigned error before usage
error, in the current version, its not needed. will remove.
const char *formatp;
struct branch *branch = NULL;
@@ -1190,30 +1192,47 @@ void ref_array_sort(struct ref_sorting *sorting,
struct ref_array *array)
+static void print_value(struct atom_value *v, struct ref_formatting_state
*state)
+{
+ struct strbuf value = STRBUF_INIT;
+ struct strbuf formatted = STRBUF_INIT;
+
+ /*
+* Some (pesudo) atoms have no immediate side effect, but only
+* affect the next atom. Store the relevant information from
+* these atoms in the 'state' variable for use when displaying
+* the next atom.
+*/
+ apply_formatting_state(state, v, value);
The comment says that this is storing formatting state, however, the
code is actually applying the state. You could move this comment
down to show_ref_array_item() where formatting state actually gets
stored. Or you could fix it to talk about applying the state.
However, now that apply_formatting_state() has a meaningful name, you
could also drop the comment altogether since it doesn't say much
beyond what is said already by the function name.
I guess I'll drop the comment thanks :)
+ switch (state-quote_style) {
case QUOTE_NONE:
- fputs(v-s, stdout);
@@ -1254,9 +1273,26 @@ static void emit(const char *cp, const char *ep)
+static void reset_formatting_state(struct ref_formatting_state *state)
+{
+ int quote_style = state-quote_style;
+ memset(state, 0, sizeof(*state));
+ state-quote_style = quote_style;
I wonder if this sledge-hammer approach of saving one or two values
before clearing the entire 'ref_formatting_state' and then restoring
the saved values will scale well. Would it be better for this to just
individually reset the fields which need resetting and not touch those
that don't?
Also, the fact that quote_style has to be handled specially may be an
indication that it doesn't belong in this structure grouped with the
other modifiers or that you need better classification within the
structure. For instance:
struct ref_formatting_state {
struct global {
int quote_style;
};
struct local {
int pad_right;
};
where 'local' state gets reset by reset_formatting_state(), and
'global' is left alone.
That's just one idea, not necessarily a proposal, but is something to
think about since the current arrangement is kind of yucky.
Did you read Junio's suggestion about not having a reset_formatting_state()
and rather just have each state be responsible of resetting itself.
I think thats seems to be a better approach.
+}
+
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;
+
+ memset(state, 0, sizeof(state));
+ state.quote_style = quote_style;
It's a little bit ugly to use memset() here when you have
reset_formatting_state() available. You could set quote_style first,
and then call reset_formatting_state() rather than memset(). Or,
perhaps, change reset_formatting_state(), as described above, to stop
using the sledge-hammer approach.
I guess even this would be taken care of by implementing Junio's suggestion.
--
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 v6 01/10] ref-filter: introduce 'ref_formatting_state'
On Thu, Jul 30, 2015 at 1:24 AM, Junio C Hamano gits...@pobox.com wrote: Actually, I think it is wrong to have this function in the first place. It is a sign that the caller is doing too little before calling this function. If the act of printing an atom uses the formatting state that says next one needs X, then it is responsible to clear that next one needs X part of the state, as it is the one who consumed that state. E.g. if it used to say next one needs to be padded to the right before entering print_value(), then the function did that padded output, then the next one needs to be padded to the right should be cleared inside print_value(). Hmm, something like what we did in version 5 I guess. And with that arrangement, together with calling emit() with formatting state, %(color:blue) can be handled as a normal part of the formatting state mechanism. The pseudo/modifier atom should update the state to Start printing in blue, and either emit() or print_value(), whichever is called first, would notice that state, does what was requested, and flip that bit down (because we are already printing in blue so the next output function does not have to do the blue thing again). This makes sense, will work on this 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 v6 01/10] ref-filter: introduce 'ref_formatting_state'
Junio C Hamano gits...@pobox.com writes: If the act of printing an atom uses the formatting state that says next one needs X, then it is responsible to clear that next one needs X part of the state, as it is the one who consumed that state. E.g. if it used to say next one needs to be padded to the right before entering print_value(), then the function did that padded output, then the next one needs to be padded to the right should be cleared inside print_value(). Good point. This would also reduce the risk of forgetting to reset, as use and reset would be next to each other in the code. -- 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 v6 01/10] ref-filter: introduce 'ref_formatting_state'
On Tue, Jul 28, 2015 at 12:56 PM, Matthieu Moy
matthieu@grenoble-inp.fr wrote:
Karthik Nayak karthik@gmail.com writes:
-static void print_value(struct atom_value *v, int quote_style)
+static void apply_formatting_state(struct ref_formatting_state *state,
+struct atom_value *v, struct strbuf *value)
{
- struct strbuf sb = STRBUF_INIT;
- switch (quote_style) {
+ /* Eventually we'll formatt based on the ref_formatting_state */
s/formatt/format/
Also, we usually use a single space after /*.
(Neither is very important since it disapears in the next commit)
+ /*
+ * Some (pesudo) atoms have no immediate side effect, but only
s/pesudo/pseudo/. But if you are to rename these atoms to modifier
atoms, you should get rid of this pseudo here.
Ah, I hate making grammatical errors, Even though I check it always gets away.
Anyways waiting for Junio and others to reply on this version. Could do a resend
for this series if needed.
--
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 v6 01/10] ref-filter: introduce 'ref_formatting_state'
Karthik Nayak karthik@gmail.com writes: Ah, I hate making grammatical errors, Even though I check it always gets away. Anyways waiting for Junio and others to reply on this version. Could do a resend for this series if needed. If you took all my remarks into account, I think it's worth a resend as it should make it easier for new reviewers. -- 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 v6 01/10] ref-filter: introduce 'ref_formatting_state'
Eric Sunshine sunsh...@sunshineco.com writes:
@@ -1254,9 +1273,26 @@ static void emit(const char *cp, const char *ep)
+static void reset_formatting_state(struct ref_formatting_state *state)
+{
+ int quote_style = state-quote_style;
+ memset(state, 0, sizeof(*state));
+ state-quote_style = quote_style;
I wonder if this sledge-hammer approach of saving one or two values
before clearing the entire 'ref_formatting_state' and then restoring
the saved values will scale well. Would it be better for this to just
individually reset the fields which need resetting and not touch those
that don't?
Also, the fact that quote_style has to be handled specially may be an
indication that it doesn't belong in this structure grouped with the
other modifiers or that you need better classification within the
structure.
Actually, I think it is wrong to have this function in the first
place. It is a sign that the caller is doing too little before
calling this function.
If the act of printing an atom uses the formatting state that says
next one needs X, then it is responsible to clear that next one
needs X part of the state, as it is the one who consumed that
state. E.g. if it used to say next one needs to be padded to the
right before entering print_value(), then the function did that
padded output, then the next one needs to be padded to the
right should be cleared inside print_value().
--
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 v6 01/10] ref-filter: introduce 'ref_formatting_state'
Eric Sunshine sunsh...@sunshineco.com writes:
@@ -1254,9 +1273,26 @@ static void emit(const char *cp, const char *ep)
+static void reset_formatting_state(struct ref_formatting_state *state)
+{
+ int quote_style = state-quote_style;
+ memset(state, 0, sizeof(*state));
+ state-quote_style = quote_style;
I wonder if this sledge-hammer approach of saving one or two values
before clearing the entire 'ref_formatting_state' and then restoring
the saved values will scale well. Would it be better for this to just
individually reset the fields which need resetting and not touch those
that don't?
Also, the fact that quote_style has to be handled specially may be an
indication that it doesn't belong in this structure grouped with the
other modifiers or that you need better classification within the
structure.
Actually, I think it is wrong to have this function in the first
place. It is a sign that the caller is doing too little before
calling this function.
If the act of printing an atom uses the formatting state that says
next one needs X, then it is responsible to clear that next one
needs X part of the state, as it is the one who consumed that
state. E.g. if it used to say next one needs to be padded to the
right before entering print_value(), then the function did that
padded output, then the next one needs to be padded to the
right should be cleared inside print_value().
And with that arrangement, together with calling emit() with
formatting state, %(color:blue) can be handled as a normal part of
the formatting state mechanism. The pseudo/modifier atom should
update the state to Start printing in blue, and either emit() or
print_value(), whichever is called first, would notice that state,
does what was requested, and flip that bit down (because we are
already printing in blue so the next output function does not have
to do the blue thing again).
--
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 v6 01/10] ref-filter: introduce 'ref_formatting_state'
Eric Sunshine sunsh...@sunshineco.com writes:
@@ -1254,9 +1273,26 @@ static void emit(const char *cp, const char *ep)
+static void reset_formatting_state(struct ref_formatting_state *state)
+{
+ int quote_style = state-quote_style;
+ memset(state, 0, sizeof(*state));
+ state-quote_style = quote_style;
I wonder if this sledge-hammer approach of saving one or two values
before clearing the entire 'ref_formatting_state' and then restoring
the saved values will scale well. Would it be better for this to just
individually reset the fields which need resetting and not touch those
that don't?
I'm the one who suggested these 3 lines. I wrote them this way with the
assumption that there would only be 1 field to keep, and thet the rest
of the series was going to add more fields to reset (currently true I
think), to avoid the risk of forgetting one value to reset.
I'm fine with the other way around too.
--
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 v6 01/10] ref-filter: introduce 'ref_formatting_state'
Karthik Nayak karthik@gmail.com writes:
-static void print_value(struct atom_value *v, int quote_style)
+static void apply_formatting_state(struct ref_formatting_state *state,
+struct atom_value *v, struct strbuf *value)
{
- struct strbuf sb = STRBUF_INIT;
- switch (quote_style) {
+ /* Eventually we'll formatt based on the ref_formatting_state */
s/formatt/format/
Also, we usually use a single space after /*.
(Neither is very important since it disapears in the next commit)
+ /*
+ * Some (pesudo) atoms have no immediate side effect, but only
s/pesudo/pseudo/. But if you are to rename these atoms to modifier
atoms, you should get rid of this pseudo here.
--
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
