Re: [PATCH 1/2] Simplify tracing code by removing trace key normalization concept

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 20, 2017, 00:24 UTC
Message-ID: <xmqqr2stokzd.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAPu-DQoapb=cgxYEOEVcpZ4nQeh+FuOE6VF=m3NaqRcr2p8Nrg@mail.gmail.com>

Gennady Kupava <gennady.kupava@gmail.com> writes:

>> The usual style comment on the subject applies here.
>
> Oh sure, 50 characters. 'Remove trace key normalization concept' would
> be better?

I was referring to #summary-section of Documentation/SubmittingPatches

    The first line of the commit message should be a short description (50
    characters is the soft limit, see DISCUSSION in linkgit:git-commit[1]),
    and should skip the full stop.  It is also conventional in most cases to
    prefix the first line with "area: " where the area is a filename or
    identifier for the general area of the code being modified, e.g.

    * doc: clarify distinction between sign-off and pgp-signing
    * githooks.txt: improve the intro section

    If in doubt which identifier to use, run `git log --no-merges` on the
    files you are modifying to see the current conventions.

    [[summary-section]]
    It's customary to start the remainder of the first line after "area: "
    with a lower-case letter. E.g. "doc: clarify...", not "doc:
    Clarify...", or "githooks.txt: improve...", not "githooks.txt:
    Improve...".

Show 13 quoted lines

> So comments explaining that, while implementing trace optimization, I
> saw two options:
> 1. Move normalization function from .c file to .h file
> 2. Remove it
>
> And why I choose removal - not used, would complicate header without
> any benefit, and actually I didn't mention minor benefit of
> compilation speed. This trace.h included and used in lots of places so
> it will take compiler some time to actually eliminate the code.
>
>> Puzzled.
>
> Does it make more sense now?

The thought behind the change flows much better in the above explanation than your four-bullet list (which a reader would often assume are parallel and orthogonal). "Remove this, because it is not used" is the primary thing for this step, and the fact that the later step benefit from that change, while it may be more important to the person who want to see that later change, is incidental to see if this change makes sense as a standalone patch.

And that primary thing, "remove this, because it is not used and only complicates the code" is what we want to start the log message with. The first sentence ("this needs to be moved there") saying what the patch chose not to was the source of the confusion.