On Tue, Dec 30, 2014 at 1:45 PM, Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> On Tue, Dec 30, 2014 at 12:36 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@xxxxxxxxxx> wrote:
>>> No functional changes intended.
>>
>> Secondary information can be demoted to the end of the commit message.
>
> I think this would help in case there is a subtle bug introduced with
> such a commit.
> If you bisect it 2 years later and then ask yourself if that subtle
> behavior was
> intentional (can easily happen if the commit message is
> short/unclear). This would then
> tell you it's definitely a bug introduced. I believe to have seen such
> a comment
> somewhere in the history, but I cannot find it again. I'll drop it
> into the notes for now.
I think you misread or misunderstood my comment, which was that
secondary information should follow primary information in the commit
message. I did not suggest moving it below the "---" line.
"No functional changes intended" is meaningful and belongs in the
commit message, but it's not as important as the explanation and
justification of the change.
>>> +static void execute_commands_loop(struct command *commands,
>>> + struct shallow_info *si)
>>
>> Style: Indent the wrapped line to align with the text following the
>> '(' in the first line.
>
> That's true. I have found this problem myself at another patch by
> Michael lately.
> If you apply the patch it is correctly aligned. If you view the patch
> however it is missaligned.
> Because of the leading plus sign the line in which the function
> signature starts is
> indented by one character. The other lines starting with a tab indent
> only to 8 character.
You're right. This one does align properly. Sorry for the noise.
(Of the two pointed out in patch 6/9, the first one also aligns
correctly; but the second does not.)
--
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