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

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