threads / patch / 36084

v7, 11 partsAdd interpret-trailers builtin

Subject: [PATCH v7 00/11] Add interpret-trailers builtin

## tl;dr

4 messages between Mar 6, 2014 and Mar 10, 2014. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Christian Couder· Mar 6, 2014, 22:06 UTC · lore
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] [(<token>[(=|:)<value>])...]
The following features are implemented:
        - the result is printed on stdout
        - the [<token>[=<value>]>] arguments are interpreted
        - a commit message read from stdin is interpreted
        - the "trailer.<token>.key" options in the config are interpreted
        - the "trailer.<token>.where" options are interpreted
        - the "trailer.<token>.ifExist" options are interpreted
        - the "trailer.<token>.ifMissing" options are interpreted
        - the "trailer.<token>.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 6, thanks to Junio and Ramsey:
* many style fixes
* clearer and nicer setup tests
* trailer.h is included at the beginning of trailer.c
Christian Couder (11):
  Add data structures and basic functions for commit trailers
  trailer: process trailers from stdin and arguments
  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.<name>.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 ++
 git.c                                    |   1 +
 t/t7513-interpret-trailers.sh            | 261 ++++++++++++
 trailer.c                                | 667 +++++++++++++++++++++++++++++++
 trailer.h                                |   6 +
 9 files changed, 1095 insertions(+)
 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.8.5.2.204.gcfe299d.dirty
Junio C Hamano· Mar 7, 2014, 21:49 UTC · re: Christian Couder · lore

Re: [PATCH v7 00/11] Add interpret-trailers builtin

Christian Couder <chriscool@tuxfamily.org> writes:
> * many style fixes
This round is readable ;-)  Thanks.
> * clearer and nicer setup tests

Those long lines that use "printf" with many embedded \n were harder to read and also looked harder to maintain if we ever wanted to change them. Splicing a string with \n in the middle of a long single line is far harder than adding an independent line, I would think. For example:

	... &&
	printf "Fixes: \nAcked-by= \nReviewed-by: \nSigned-off-by: \n" >>expected &&
	...

is easier to read and maintain if written like so (with using HT properly---our MUAs may damage it and turn the indentation into spaces):

	... &&
	sed -e "s/ Z$/ /" >>expect <<-\EOF &&
        Fixes: Z
        Acked-by= Z
        Reviewed-by: Z
        Signed-off-by: Z
	EOF
	...
Øystein Walle· Mar 8, 2014, 11:54 UTC · re: Junio C Hamano · lore

Re: [PATCH v7 00/11] Add interpret-trailers builtin

Junio C Hamano <gitster <at> pobox.com> writes:
Show 16 quoted lines
> 
> 	...
> 
> is easier to read and maintain if written like so (with using HT
> properly---our MUAs may damage it and turn the indentation into
> spaces):
> 
> 	... &&
> 	sed -e "s/ Z$/ /" >>expect <<-\EOF &&
>         Fixes: Z
>         Acked-by= Z
>         Reviewed-by: Z
>         Signed-off-by: Z
> 	EOF
> 	...
> 
How about:
   printf '%s: \n' Fixes Acked-by Reviewed-by Signed-off-by >> expect

This solution scores high marks in both readability and maintainability in my mind.

Junio C Hamano· Mar 10, 2014, 15:27 UTC · re: Øystein Walle · lore

Re: [PATCH v7 00/11] Add interpret-trailers builtin

Øystein Walle <oystwa@gmail.com> writes:
Show 22 quoted lines
> Junio C Hamano <gitster <at> pobox.com> writes:
>
>> 
>> 	...
>> 
>> is easier to read and maintain if written like so (with using HT
>> properly---our MUAs may damage it and turn the indentation into
>> spaces):
>> 
>> 	... &&
>> 	sed -e "s/ Z$/ /" >>expect <<-\EOF &&
>>         Fixes: Z
>>         Acked-by= Z
>>         Reviewed-by: Z
>>         Signed-off-by: Z
>> 	EOF
>> 	...
>> 
>
> How about:
>
>    printf '%s: \n' Fixes Acked-by Reviewed-by Signed-off-by >> expect
Not really.
> This solution scores high marks in both readability and maintainability
> in my mind.

I actually considered that approach while I was writing the message you responded to, but discarded it because it forces us to commit to the view that we forsee no need to test an output that does not end with a trailing whitespace. And I do not think that is a limitation we want to place on this test.

← back to recent threads