Re: [PATCH v2 01/11] builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Feb 28, 2014 at 11:03:19AM -0800, Junio C Hamano wrote:

> Michael Haggerty <mhagger@xxxxxxxxxxxx> 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
--
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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]