Re: [PATCH v8 00/12] Add interpret-trailers builtin

2014-03-27 Thread Christian Couder
From: Junio C Hamano gits...@pobox.com

 Christian Couder chrisc...@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.

Ok, I will try to improve this part of the Rationale section.

Thanks,
Christian.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v8 00/12] Add interpret-trailers builtin

2014-03-26 Thread Junio C Hamano
Christian Couder chrisc...@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?

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html