Re: [PATCH v7 01/17] ref-filter: implement %(if), %(then), and %(else) atoms

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

Jacob Keller <jacob.keller@gmail.com> writes:

> Ok, so I have only one minor nit, but otherwise this looks quite good
> to me. A few comments explaining my understanding, but only one
> suggested
> change which is really a minor nit and not worth re-rolling just for it.

As you didn't snip parts you didn't comment, I'll use this to add my
own for convenience ;-)

>> +if::
>> +       Used as %(if)...%(then)...(%end) or
>> +       %(if)...%(then)...%(else)...%(end).  If there is an atom with
>> +       value or string literal after the %(if) then everything after
>> +       the %(then) is printed, else if the %(else) atom is used, then
>> +       everything after %(else) is printed. We ignore space when
>> +       evaluating the string before %(then), this is useful when we
>> +       use the %(HEAD) atom which prints either "*" or " " and we
>> +       want to apply the 'if' condition only on the 'HEAD' ref.
>> +
>>  In addition to the above, for commit and tag objects, the header
>>  field names (`tree`, `parent`, `object`, `type`, and `tag`) can
>>  be used to specify the value in the header field.

I see a few instances of (%end) that were meant to be %(end).

Aren't the following two paragraphs ...

>> +When a scripting language specific quoting is in effect (i.e. one of
>> +`--shell`, `--perl`, `--python`, `--tcl` is used), except for opening
>> +atoms, replacement from every %(atom) is quoted when and only when it
>> +appears at the top-level (that is, when it appears outside
>> +%($open)...%(end)).

>> +When a scripting language specific quoting is in effect, everything
>> +between a top-level opening atom and its matching %(end) is evaluated
>> +according to the semantics of the opening atom and its result is
>> +quoted.

... saying the same thing?


>> +               }
>> +       } else if (!if_then_else->condition_satisfied)
>
> Minor nit. I'm not sure what standard we use here at Git, but
> traditionally, I prefer to see { } blocks on all sections even if only
> one of them needs it. (That is, only drop the braces when every
> section is one line.) It also looks weird with a comment since it
> appears as multiple lines to the reader. I think the braces improve
> readability.
>
> I don't know whether that's Git's code base standard or not, however.
> It's not really worth a re-roll unless something else would need to
> change.
>

In principle, we mimick the kernel style of using {} block even on a
single-liner body in if/else if/else cascade when any one of them is
not a single-liner and requires {}.  But we often ignore that when a
truly trivial single liner follows if() even if its else clause is a
big block, e.g.

	if (cond)
		single;
	else {
		big;
		block;
	}

I agree with you that this case should just use {} for the following
paragraph, because it is technically a single-liner, but comes with
a big comment block and is very much easier to read with {} around
it.

>> +               /*
>> +                * No %(else) atom: just drop the %(then) branch if the
>> +                * condition is not satisfied.
>> +                */
>> +               strbuf_reset(&cur->output);

Thanks.