From: Junio C Hamano <gitster@pobox•com>
To: Christian Couder <chriscool@tuxfamily•org>
Cc: git@vger•kernel.org, Johan Herland <johan@herland•net>,
Josh Triplett <josh@joshtriplett•org>,
Thomas Rast <tr@thomasrast•ch>,
Michael Haggerty <mhagger@alum•mit.edu>,
Dan Carpenter <dan.carpenter@oracle•com>,
Greg Kroah-Hartman <greg@kroah•com>, Jeff King <peff@peff•net>,
Eric Sunshine <sunshine@sunshineco•com>,
Ramsay Jones <ramsay@ramsay1•demon.co.uk>
Subject: Re: [PATCH v8 00/12] Add interpret-trailers builtin
Date: Wed, 26 Mar 2014 16:05:55 -0700 [thread overview]
Message-ID: <xmqq4n2kh86k.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: 20140326215858.11352.89243.chriscool@tuxfamily.org
Christian Couder <chriscool@tuxfamily•org> writes:
> Until now git commit has only supported the well known
> "Signed-off-by: " trailer, that is used by many projects like
> the Linux kernel and Git.
>
> It is better to implement features for these trailers first in a
> new command rather than in builtin/commit.c, because this way the
> prepare-commit-msg and commit-msg hooks can reuse this command.
The "first" is somewhat questionable.
It is better to keep builtin/commit.c uncontaminated by any more
hard-wired logic, like what we have for the signed-off-by line. Any
new things can and should be doable in hooks, and this filter would
help writing these hooks.
And that is why the design goal of the filter is to make it at least
as powerful as the built-in logic we have for signed-off-by lines;
that would allow us to later eject the hard-wired logic for
signed-off-by line from the main codepath, if/when we wanted to.
Alternatively, we could build a library-ish API around this filter
code and replace the hard-wired logic for signed-off-by line with a
call into that API, if/when we wanted to, but that requires (in
addition to the "at least as powerful as the built-in logic") that
the implementation of this stand-alone filter can be cleanly made
into a reusable library, so that is a bit higher bar to cross than
"everything can be doable with hooks" alternative.
> 3) Changes since version 7, thanks to Junio:
>
> * improved handling of empty trailer token
> * clearer way to create 'expected' files in tests
> * other small test cleanups
> * improved commit message
> * new way to parse config keys
> * strcasecmp() is not used anymore in some config related functions
It is unclear which of the 12 patches are unchanged since the last
round. Are reviewers expected to re-read all of them?
> Some values from the config file are lowercased instead.
> To enable that a new patch (3/12) is introduced to rationalize
> lowercase related functions. I am not very happy with these
> changes.
I can see why you are not very happy. Perhaps it may make you
happier if you did not move lowercase() at all, did the
xstrdup_tolower() in a cleaner and more efficient way, and only used
the latter in the code?
next prev parent reply other threads:[~2014-03-26 23:06 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-26 22:15 [PATCH v8 00/12] Add interpret-trailers builtin Christian Couder
2014-03-26 22:15 ` [PATCH v8 01/12] Add data structures and basic functions for commit trailers Christian Couder
2014-03-26 23:06 ` Junio C Hamano
2014-03-27 7:55 ` Christian Couder
2014-03-26 22:15 ` [PATCH v8 02/12] trailer: process trailers from stdin and arguments Christian Couder
2014-03-26 22:15 ` [PATCH v8 03/12] Move lower case functions into wrapper.c Christian Couder
2014-03-26 23:07 ` Junio C Hamano
2014-03-27 7:47 ` Christian Couder
2014-03-27 16:42 ` Junio C Hamano
2014-03-27 22:16 ` Junio C Hamano
2014-03-27 22:34 ` Jeff King
2014-03-27 22:47 ` Junio C Hamano
2014-03-27 22:56 ` Jeff King
2014-03-28 17:12 ` Junio C Hamano
2014-03-28 18:50 ` Jeff King
2014-03-28 7:02 ` Christian Couder
2014-03-26 22:15 ` [PATCH v8 04/12] trailer: read and process config information Christian Couder
2014-03-26 22:15 ` [PATCH v8 05/12] trailer: process command line trailer arguments Christian Couder
2014-03-26 22:15 ` [PATCH v8 06/12] trailer: parse trailers from stdin Christian Couder
2014-03-26 22:15 ` [PATCH v8 07/12] trailer: put all the processing together and print Christian Couder
2014-03-26 22:15 ` [PATCH v8 08/12] trailer: add interpret-trailers command Christian Couder
2014-03-26 22:15 ` [PATCH v8 09/12] trailer: add tests for "git interpret-trailers" Christian Couder
2014-03-26 22:15 ` [PATCH v8 10/12] trailer: execute command from 'trailer.<name>.command' Christian Couder
2014-03-26 22:15 ` [PATCH v8 11/12] trailer: add tests for commands in config file Christian Couder
2014-03-26 22:15 ` [PATCH v8 12/12] Documentation: add documentation for 'git interpret-trailers' Christian Couder
2014-03-26 23:05 ` Junio C Hamano [this message]
2014-03-27 7:53 ` [PATCH v8 00/12] Add interpret-trailers builtin Christian Couder
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=xmqq4n2kh86k.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=chriscool@tuxfamily$(echo .)org \
--cc=dan.carpenter@oracle$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=greg@kroah$(echo .)com \
--cc=johan@herland$(echo .)net \
--cc=josh@joshtriplett$(echo .)org \
--cc=mhagger@alum$(echo .)mit.edu \
--cc=peff@peff$(echo .)net \
--cc=ramsay@ramsay1$(echo .)demon.co.uk \
--cc=sunshine@sunshineco$(echo .)com \
--cc=tr@thomasrast$(echo .)ch \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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