Jacob Keller <[email protected]> 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.
