Re: [PATCH v8 00/12] Add interpret-trailers builtin
From: Junio C Hamano > Christian Couder 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
Christian Couder 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
[PATCH v8 00/12] Add interpret-trailers builtin
This patch series implements a new command: git interpret-trailers and an infrastructure to process trailers that can be reused, for example in "commit.c". 1) Rationale: This command should help with RFC 822 style headers, called "trailers", that are found at the end of commit messages. (Note that these headers do not follow and are not intended to follow many rules that are in RFC 822. For example they do not follow the line breaking rules, the encoding rules and probably many other rules.) For a long time, these trailers have become a de facto standard way to add helpful information into commit messages. 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. 2) Current state: Currently the usage string of this command is: git interpret-trailers [--trim-empty] [([(=|:)])...] The following features are implemented: - the result is printed on stdout - the [[=]>] arguments are interpreted - a commit message read from stdin is interpreted - the "trailer..key" options in the config are interpreted - the "trailer..where" options are interpreted - the "trailer..ifExist" options are interpreted - the "trailer..ifMissing" options are interpreted - the "trailer..command" config works - $ARG can be used in commands - there are some tests - there is some documentation The following features are planned but not yet implemented: - add more tests related to commands - add examples in documentation - integration with "git commit" Possible improvements: - support GIT_COMMIT_PROTO env variable in commands 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 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. Christian Couder (12): Add data structures and basic functions for commit trailers trailer: process trailers from stdin and arguments Move lower case functions into wrapper.c trailer: read and process config information trailer: process command line trailer arguments trailer: parse trailers from stdin trailer: put all the processing together and print trailer: add interpret-trailers command trailer: add tests for "git interpret-trailers" trailer: execute command from 'trailer..command' trailer: add tests for commands in config file Documentation: add documentation for 'git interpret-trailers' .gitignore | 1 + Documentation/git-interpret-trailers.txt | 123 ++ Makefile | 2 + builtin.h| 1 + builtin/interpret-trailers.c | 33 ++ config.c | 6 - daemon.c | 8 - git-compat-util.h| 4 + git.c| 1 + t/t7513-interpret-trailers.sh| 407 ++ trailer.c| 691 +++ trailer.h| 6 + wrapper.c| 14 + 13 files changed, 1283 insertions(+), 14 deletions(-) create mode 100644 Documentation/git-interpret-trailers.txt create mode 100644 builtin/interpret-trailers.c create mode 100755 t/t7513-interpret-trailers.sh create mode 100644 trailer.c create mode 100644 trailer.h -- 1.9.0.164.g3aa33cd.dirty -- 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
