git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Gennady Kupava <gennady.kupava@gmail.com>
Cc: git <git@vger.kernel.org>, Gennady Kupava <gkupava@bloomberg.net>
Subject: Re: [PATCH 1/2] Simplify tracing code by removing trace key normalization concept
Date: Mon, 20 Nov 2017 09:24:38 +0900	[thread overview]
Message-ID: <xmqqr2stokzd.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <CAPu-DQoapb=cgxYEOEVcpZ4nQeh+FuOE6VF=m3NaqRcr2p8Nrg@mail.gmail.com> (Gennady Kupava's message of "Sun, 19 Nov 2017 13:16:04 +0000")

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...".

> 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.



  reply	other threads:[~2017-11-20  0:24 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-11 19:28 [PATCH] Reduce performance penalty for turned off traces gennady.kupava
2017-11-12 14:17 ` Jeff King
2017-11-12 23:24   ` Gennady Kupava
2017-11-17 22:12     ` Jeff King
2017-11-15 19:14   ` Stefan Beller
2017-11-17 22:16     ` Jeff King
2017-11-19  0:42       ` [PATCH 1/2] Simplify tracing code by removing trace key normalization concept gennady.kupava
2017-11-19  0:42         ` [PATCH 2/2] Reduce performance cost of the trace if trace category is disabled gennady.kupava
2017-11-19  8:27           ` Johannes Sixt
2017-11-19 13:18             ` Gennady Kupava
2017-11-19  2:19         ` [PATCH 1/2] Simplify tracing code by removing trace key normalization concept Junio C Hamano
2017-11-19 13:16           ` Gennady Kupava
2017-11-20  0:24             ` Junio C Hamano [this message]
2017-11-20  4:59               ` Junio C Hamano
2017-11-26 20:11                 ` [PATCH 1/2] trace: remove trace key normalization gennady.kupava
2017-11-26 20:11                   ` [PATCH 2/2] trace: improve performance while category is disabled gennady.kupava
2017-11-27  1:21                     ` Junio C Hamano
2017-11-27  1:32                       ` Junio C Hamano
2017-11-27  3:25                         ` Junio C Hamano
2017-11-27 10:12                           ` Gennady Kupava

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqr2stokzd.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=gennady.kupava@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gkupava@bloomberg.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).