From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Michael Haggerty <mhagger@alum.mit.edu>,
Duy Nguyen <pclouds@gmail.com>,
"Dmitry S. Dolzhenko" <dmitrys.dolzhenko@yandex.ru>,
Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH v2 01/11] builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW()
Date: Sat, 1 Mar 2014 02:07:58 -0500 [thread overview]
Message-ID: <20140301070758.GE20397@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqqfvn3ukjs.fsf@gitster.dls.corp.google.com>
On Fri, Feb 28, 2014 at 11:03:19AM -0800, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
> > So my vote is that the patches are OK the way Dmitry wrote them (mind, I
> > have only read through 05/11 so far).
>
> Seconded ;-)
>
> By the way, I do not like these long subjects. "change" is a
> redundant word when one sends a patch---as all patches are about
> changing something.
>
> Subject: builtin/pack-objects.c: use ALLOC_GROW() in check_pbase_path()
>
> would be a lot more appropriate for "git shortlog" consumption.
I would actually go one step further and drop or shorten the filename in
the subject. It is very long, it is already easy to see which file was
changed from the diffstat, and it doesn't give any useful context for
other parts of the subject.
I really like the "foo:" convention for starting a subject line, because
it immediately makes clear what area you are working in without having
to waste space on English conjunctions or prepositions. But it does not
have to be a filename. It can be a subsystem, a command, a function, an
area of the project, or anything that gives context to the rest of the
line.
So I would suggest one of:
Subject: use ALLOC_GROW() in check_pbase_path()
Talking about the filename is redundant; there's only one
check_pbase_path.
Subject: check_pbase_path: use ALLOW_GROW
Even shorter.
Subject: builtin/pack-objects.c: use ALLOC_GROW
This one implies to me that the point of the commit is to convert
the whole file to use ALLOC_GROW where appropriate, not just that
function (even if that function may be the only spot changed).
I'd probably not use:
Subject: pack-objects: use ALLOC_GROW
as the scope is not about the command, but about the C file.
I realize that I just bikeshedded on subject lines for half a page, and
part of me wants to go kill myself in shame. But I feel like I see the
technique misapplied often enough that maybe some guidance is merited.
Feel free to ignore. :)
-Peff
next prev parent reply other threads:[~2014-03-01 7:08 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-02-27 20:45 [PATCH] Use ALLOC_GROW() instead of inline code Dmitry S. Dolzhenko
2014-02-27 22:11 ` Michael Haggerty
2014-02-27 22:44 ` Junio C Hamano
2014-02-28 9:29 ` [PATCH v2 00/11] " Dmitry S. Dolzhenko
2014-02-28 9:40 ` [PATCH v2 01/11] builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW() Dmitry S. Dolzhenko
2014-02-28 12:32 ` Duy Nguyen
2014-02-28 12:40 ` Duy Nguyen
2014-02-28 14:20 ` Michael Haggerty
2014-02-28 14:36 ` Duy Nguyen
2014-02-28 19:03 ` Junio C Hamano
2014-03-01 7:07 ` Jeff King [this message]
2014-03-03 18:23 ` Junio C Hamano
2014-02-28 9:41 ` [PATCH v2 02/11] bundle.c: change add_to_ref_list() " Dmitry S. Dolzhenko
2014-02-28 9:41 ` [PATCH v2 03/11] cache-tree.c: change find_subtree() " Dmitry S. Dolzhenko
2014-02-28 9:42 ` [PATCH v2 04/11] commit.c: change register_commit_graft() " Dmitry S. Dolzhenko
2014-02-28 9:43 ` [PATCH v2 05/11] diff.c: use ALLOC_GROW() instead of inline code Dmitry S. Dolzhenko
2014-02-28 9:44 ` [PATCH v2 06/11] diffcore-rename.c: " Dmitry S. Dolzhenko
2014-02-28 9:45 ` [PATCH v2 07/11] patch-ids.c: change add_commit() to use ALLOC_GROW() Dmitry S. Dolzhenko
2014-02-28 9:45 ` [PATCH v2 08/11] replace_object.c: change register_replace_object() " Dmitry S. Dolzhenko
2014-02-28 9:46 ` [PATCH v2 09/11] reflog-walk.c: use ALLOC_GROW() instead of inline code Dmitry S. Dolzhenko
2014-02-28 12:39 ` Duy Nguyen
2014-02-28 19:06 ` Junio C Hamano
2014-02-28 9:46 ` [PATCH v2 10/11] dir.c: change create_simplify() to use ALLOC_GROW() Dmitry S. Dolzhenko
2014-02-28 9:47 ` [PATCH v2 11/11] attr.c: change handle_attr_line() " Dmitry S. Dolzhenko
2014-02-28 14:38 ` [PATCH v2 00/11] Use ALLOC_GROW() instead of inline code Michael Haggerty
2014-03-01 6:57 ` Dmitry S. Dolzhenko
2014-03-03 18:20 ` Junio C Hamano
2014-03-03 7:18 ` [PATCH v3 " Dmitry S. Dolzhenko
2014-03-03 7:19 ` [PATCH v3 01/11] builtin/pack-objects.c: use ALLOC_GROW() in check_pbase_path() Dmitry S. Dolzhenko
2014-03-03 7:20 ` [PATCH v3 02/11] bundle.c: use ALLOC_GROW() in add_to_ref_list() Dmitry S. Dolzhenko
2014-03-03 7:20 ` [PATCH v3 03/11] cache-tree.c: use ALLOC_GROW() in find_subtree() Dmitry S. Dolzhenko
2014-03-03 7:21 ` [PATCH v3 04/11] commit.c: use ALLOC_GROW() in register_commit_graft() Dmitry S. Dolzhenko
2014-03-03 7:22 ` [PATCH v3 05/11] diff.c: use ALLOC_GROW() Dmitry S. Dolzhenko
2014-03-03 7:22 ` [PATCH v3 06/11] diffcore-rename.c: " Dmitry S. Dolzhenko
2014-03-03 7:23 ` [PATCH v3 07/11] patch-ids.c: use ALLOC_GROW() in add_commit() Dmitry S. Dolzhenko
2014-03-03 7:23 ` [PATCH v3 08/11] replace_object.c: use ALLOC_GROW() in register_replace_object() Dmitry S. Dolzhenko
2014-03-03 7:24 ` [PATCH v3 09/11] reflog-walk.c: use ALLOC_GROW() Dmitry S. Dolzhenko
2014-03-03 7:25 ` [PATCH v3 10/11] dir.c: use ALLOC_GROW() in create_simplify() Dmitry S. Dolzhenko
2014-03-03 7:25 ` [PATCH v3 11/11] attr.c: use ALLOC_GROW() in handle_attr_line() Dmitry S. Dolzhenko
2014-03-03 8:23 ` [PATCH v3 00/11] Use ALLOC_GROW() instead of inline code Eric Sunshine
2014-03-03 19:07 ` Junio C Hamano
2014-03-03 14:20 ` [PATCH] " He Sun
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140301070758.GE20397@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=dmitrys.dolzhenko@yandex.ru \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=mhagger@alum.mit.edu \
--cc=pclouds@gmail.com \
/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.
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 NNTP newsgroup(s).