Re: [RFC/PATCH] Add interpret-trailers builtin

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 4, 2013, 19:02 UTC
Message-ID: <xmqqli14yoqi.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20131103211731.15716.95056.chriscool@tuxfamily.org>

Christian Couder <chriscool@tuxfamily.org> writes:

Show 45 quoted lines

> This RFC patch shows the work in progress to implement a new
> command:
>
> 	git interpret-trailers
>
> 1) Rational:
>
> This command should help with RFC 822 style headers, called
> "trailers", that are found at the end of commit messages.
>
> 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 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] [--infile=file] [<token[=value]>...]
>
> The following features are implemented:
> 	- the result is printed on stdout
> 	- the [<token[=value]>...] arguments are interpreted
> 	- a commit message passed using the "--infile=file" option is interpreted
> 	- the "trailer.<token>.value" options in the config are interpreted
>
> The following features are planned but not yet implemented:
> 	- some documentation
> 	- more tests
> 	- the "trailer.<token>.if_exist" config option
> 	- the "trailer.<token>.if_missing" config option
> 	- the "trailer.<token>.command" config option
>
> 3) Notes:
>
> * "trailer" seems better than "commitTrailer" as the config key because
> it looks like all the config keys are lower case and "committrailer" is not
> very readable.

And closes the door for other things from later acquiring trailers?

> * "trailer.<token>.value" looks better than "trailer.<token>.trailer", so
> I chose the former.

If that is a literal value, then I think ".value" is indeed good.

> * Rather than only one "trailer.<token>.style" config option, it seems
> better to me to have both "trailer.<token>.if_exist" and
> "trailer.<token>.if_missing".

As there is no ".if_exist", ".if_missing", etc. here, I cannot guess what these "seemingly independent and orthogonal, but some combinations are impossible" configuration variables are meant to be used, let alone agreeing to the above claim that they are better than a single ".style". I think you took the ".style" from my thinking-aloud message the other day, but that aloud-thinking lead to ".style" by taking various use cases people wanted to have footers into account, including:

 - just appending, allowing duplication of the field name (e.g. more
   than one "cc:" can name different recipients);

 - appending, allowing duplication of the field <name,val> pair
   (e.g. a patch that flowed from person A to B and possibly
   somewhere else then finally back to A may have "Signed-off-by:
   A", chain of other's Sob, and another "Signed-off-by: A");

 - appending, but without consecutive repeats (e.g. the same
   "Signed-off-by:" example; person A does not pass a patch to
   himself, adding two same <name,val> pair next to each other);

 - adding only if there is no same <name> (e.g. "Change-Id:");

 - adding only if there is no <name,val> pair (e.g. "Closes: #bugId");

 - adding one if there is none, replacing one if there already is.

I do not think it is easier for users to express these (and other) semantics as combinations of "seemingly independent and orthogonal, but some combinations are impossible" configuration variables.

Show 12 quoted lines

> * I might send a patch series instead of just one big patch when there will
> be fewer big changes in the code.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  .gitignore                    |   1 +
>  Makefile                      |   1 +
>  builtin.h                     |   1 +
>  builtin/interpret-trailers.c  | 284 ++++++++++++++++++++++++++++++++++++++++++
>  git.c                         |   1 +
>  strbuf.c                      |   7 ++
>  strbuf.h                      |   1 +

I think you're better off having trailers.c at the top level that is called from builtin/interpret-trailers.c (aside from names), so that we can later hook the former up with builtin/commit.c codepath.