Re: [PATCH v6 01/10] ref-filter: introduce 'ref_formatting_state'

On Thu, Jul 30, 2015 at 12:49 AM, Eric Sunshine [off-list ref] wrote:

On Tuesday, July 28, 2015, Karthik Nayak [off-list ref] wrote:

quoted

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.

quoted

Signed-off-by: Karthik Nayak <redacted>
---

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.

quoted

                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 :)

quoted

+       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.

quoted

+}
+
 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