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