Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 3, 2015, 17:01 UTC
Message-ID: <xmqqpp0v3mzo.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <vpq8u7kb89o.fsf@grenoble-inp.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Show 14 quoted lines

> My take on it:
>
> Implement %(if), %(then) and %(else) atoms. Used as
> %(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the
> format string between %(if) and %(then) expands to an empty string, or
> to only whitespaces, then the string following %(then) is printed.
> Otherwise, the string following %(else), if any, is printed.
>
>> +When a scripting language specific quoting is in effect,
>
> This may not be immediately clear to the reader. I'd add explicitly:
>
> When a scripting language specific quoting is in effect (i.e. one of
> `--shell`, `--perl`, `--python`, `--tcl` is used), ...

I found all the suggestions very good, except that the distinction between "expands to" and "is printed" bothers me a bit, as they want to mean exactly the same thing (imagine this whole thing were inside another %(if)...%(then)).

Show 6 quoted lines

>>  EXAMPLES
>>  --------
>
> This is just the context of the patch, but I read it as a hint that we
> could add some examples with complex --format usage to illustrate the
> theory above.

Yes ;-)

>> +static int is_empty(const char * s){
>
> char * s -> char *s

Also "{" must sit alone on the next line.

Show 16 quoted lines

>> +static void then_atom_handler(struct atom_value *atomv, struct
>> ref_formatting_state *state)
>> +{
>> +	struct ref_formatting_stack *cur = state->stack;
>> +	struct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;
>> +
>> +	if (!if_then_else)
>> +		die(_("format: %%(then) atom used without an %%(if) atom"));
>
> You've just casted at_end_data to if_then_else. if_then_else being not
> NULL does not mean that it is properly typed. It can be the at_end_data
> of another opening atom. What happens if you use
> %(align)foo%(then)bar%(end)?
>
> One way to be safer would be to check that cur->at_end does point to
> if_then_else_handler.

Good eyes.  Thanks.

Show 9 quoted lines

> So, while parsing the %(else)...%(end), the stack contains both the
> %(then)...%(else) part, and the %(else)...%(end).
>
> I'm wondering if we can simplify this. We already know if the condition
> is satisfied, and if it's not, we can just drop the %(then) part right
> now, and write to the top of the stack normally (the %(end) atom will
> only have to pop the string normally). But if the condition is not
> satisfied, we need to preserve the %(then) part and need to do something
> about the %(else).

Good suggestion.

Thanks.