On Wed, Mar 23, 2016 at 9:45 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
>
>>> + if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
>>> + "replay", "log", "run", NULL))
>>
>> If I understood Junio correctly, he meant to line up the second line with
>> the corresponding level. In this case, as "replay" is a parameter of the
>> one_of() function, the indentation would look like this instead:
>>
>> if (one_of(term, "help", "start", "skip", "next", "reset", "visualize",
>> "replay", "log", "run", NULL))
>
> Thanks for clarification. It may also make sense to wrap the first
> line one word earlier.
>
>>> + die("can't use the builtin command '%s' as a term", term);
>>> +
>>> + /* In theory, nothing prevents swapping
>>> + * completely good and bad, but this situation
>>> + * could be confusing and hasn't been tested
>>> + * enough. Forbid it for now.
>>> + */
>>
>> I see quite a few comments that put the closing "*/" on its own line, but
>> do not award the same pleasure to the opening "/*". Personally, I much
>> prefer the opening "/*" to have an entire line to itself, too, but I guess
>> that there is enough precendence in Git's source code to accept both
>> forms.
>
> We do want to see "/*" and "*/" on their own lines, and new code
> should definitely do so.
I also think it is better to promote one format and try and reduce the
other one.
>>> + if (!strcmp(term, "bad") || !strcmp(term, "new"))
>>> + if (strcmp(revision, "bad"))
>>> + die("can't change the meaning of term '%s'", term);
>>> +
>>> + if(!strcmp(term, "good") || !strcmp(term, "old"))
>>> + if (strcmp(revision, "good"))
>>> + die("can't change the meaning of term '%s'", term);
>>
>> I am still convinced that
>>
>> if ((one_of(term, "bad", "new", NULL) && strcmp(orig_term, "bad")) ||
>> (one_of(term, "good", "old", NULL) && strcmp(orig_term, "good")))
>> die("can't change the meaning of term '%s'", term);
>>
>> looks so much nicer.
>
> ... and more importantly, easier to understand what is going on.
I will take care about this next time.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html