Re: [RFC] git: make signoff header configurable

From: Junio C Hamano <gitster@pobox.com>
Date: May 7, 2013, 04:53 UTC
Message-ID: <7vehdjo0fo.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1367876032-6833-1-git-send-email-arend@broadcom.com>

"Arend van Spriel" <arend@broadcom.com> writes:

Show 5 quoted lines

> I had an itch to scratch. Like the -s command line parameter to
> get the signed-off message added, but not all projects use the
> same signature format. So let me know what you think about this
> idea. Never contributed to git before so decided to make it an
> RFC first as this solution may be a bit hack-ish.

It is customary to declare things like "char *get_signoff_header()" that are meant to be available pretty much everywhere in cache.h (look for /* Environment bits from configuration mechanism */) and not in an unrelated header like sequencer.h (which builtin/commit.c or config.c have no business including). Also, default-config may be a bit too generic place to read this information; it is used by many read-only commands like "log", "diff", etc. that have no reason to know this custom trailer setting.

Other than these, I do not see anything glaringly "hack-ish" in the implementation itself. Of course, a non-RFC patch would come with documentation and test script updates.

By the way, what you are adding is a trailer, not a header, as it comes at the very end ;-).

To projects that adopt the S-o-b convention from the kernel, the act of signing off has a very specific legal meaning (I know it is not an electronic signature, but the intention counts in court); I am not sure if it is even a good idea to make "-s" mean something different depending on the configuration in the first place, though.

Wouldn't a commit template a better alternative for appending a random stuff in the log message, I wonder.

Show 15 quoted lines

>  builtin/commit.c |    5 +++--
>  config.c         |   12 ++++++++++++
>  sequencer.c      |   15 +++++++++++++--
>  sequencer.h      |    3 ++-
>  4 files changed, 30 insertions(+), 5 deletions(-)
> diff --git a/sequencer.h b/sequencer.h
> index 1fc22dc..5a91105 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -48,8 +48,9 @@ struct replay_opts {
>  
>  int sequencer_pick_revisions(struct replay_opts *opts);
>  
> -extern const char sign_off_header[];
> +extern char* sign_off_header;

The asterisk sticks to the identifier, not type, i.e.

	extern char *signoff_label;

But I suspect you do not need to make this extern (for the same reason we can keep def_signoff_header[] a static), as long as the public facing API "get-signoff-header" is extern.

> +char *get_signoff_header(void);
>  void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
>  
>  #endif