Re: [PATCH v13 04/12] ref-filter: implement an `align` atom

[Junio C Hamano <gitster@pobox.com>]

Karthik Nayak <karthik.188@gmail.com> 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(&current->output);
> +		strbuf_addbuf(&current->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 atoms, which would let you do the above without any if/else
and just a single unconditional

	atomv->handler(atomv, &state);