On Fri, Nov 11, 2016 at 4:43 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Jacob Keller <jacob.keller@xxxxxxxxx> 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).
>
Will change that.
> 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?
>
Yes. I'm not sure of what the context even was, but I shall remove the
first paragraph,
the second one seems to notify the same thing in simpler terms.
>
>>> + }
>>> + } 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.
>
Ah! I see, sure I'll change it :)
--
Regards,
Karthik Nayak