Make author and committer available to pre-commit hook

Make author and committer available to pre-commit hook

[Gisle Aas]

I would like to implement a pre-commit hook that validates the
--author set for the commit.  Our use case is a shared repository of
configuration info where different persons all commit as root; but we
want to make sure they override the --author to something sensible.

What would be the preferred mechanism to pass on this information?  It
could for instance be arguments to the hook script or via environment
variables.

I created the following patch to explore what it would take to pass on
this information as command line arguments.  It seems to do the trick
for me.  Any drawback with this approach?

Regards,
Gisle


diff --git a/builtin/commit.c b/builtin/commit.c
index c101f00..acb8dc2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -557,8 +557,20 @@ static int prepare_to_commit(const char
*index_file, const char *prefix,
        const char *hook_arg2 = NULL;
        int ident_shown = 0;

-       if (!no_verify && run_hook(index_file, "pre-commit", NULL))
+       determine_author_info();
+       if (!no_verify) {
+           int rc;
+           hook_arg1 = xstrdup(fmt_name(author_name, author_email));
+           hook_arg2 = xstrdup(fmt_name(getenv("GIT_COMMITTER_NAME"),
+                                        getenv("GIT_COMMITTER_EMAIL")));
+           rc = run_hook(index_file, "pre-commit", hook_arg1, hook_arg2, NULL);
+           free((char*)hook_arg1);
+           free((char*)hook_arg2);
+           if (rc)
                return 0;
+           hook_arg1 = NULL;
+           hook_arg2 = NULL;
+       }

        if (message.len) {
                strbuf_addbuf(&sb, &message);
@@ -632,8 +644,6 @@ static int prepare_to_commit(const char
*index_file, const char *prefix,

        strbuf_release(&sb);

-       determine_author_info();
-
        /* This checks if committer ident is explicitly given */
        git_committer_info(0);
        if (use_editor && include_status) {

Re: Make author and committer available to pre-commit hook

[Jeff King]

On Mon, Jul 05, 2010 at 01:30:46PM +0200, Gisle Aas wrote:

> I would like to implement a pre-commit hook that validates the
> --author set for the commit.  Our use case is a shared repository of
> configuration info where different persons all commit as root; but we
> want to make sure they override the --author to something sensible.
> 
> What would be the preferred mechanism to pass on this information?  It
> could for instance be arguments to the hook script or via environment
> variables.

It would make sense to me for it to be passed in through the environment
using GIT_{AUTHOR,COMMITTER}_{NAME,EMAIL}. You could even pass
GIT_AUTHOR_DATE to detect if somebody is using "git commit -c" to get
the date from a previous commit.

-Peff

Re: Make author and committer available to pre-commit hook

[Gisle Aas]

[-- Attachment #1: Type: text/plain, Size: 1062 bytes --]

On Mon, Jul 5, 2010 at 13:46, Jeff King <peff@peff.net> wrote:
> On Mon, Jul 05, 2010 at 01:30:46PM +0200, Gisle Aas wrote:
>
>> I would like to implement a pre-commit hook that validates the
>> --author set for the commit.  Our use case is a shared repository of
>> configuration info where different persons all commit as root; but we
>> want to make sure they override the --author to something sensible.
>>
>> What would be the preferred mechanism to pass on this information?  It
>> could for instance be arguments to the hook script or via environment
>> variables.
>
> It would make sense to me for it to be passed in through the environment
> using GIT_{AUTHOR,COMMITTER}_{NAME,EMAIL}. You could even pass
> GIT_AUTHOR_DATE to detect if somebody is using "git commit -c" to get
> the date from a previous commit.

I agree that would be a more natural interface.  Attached is a patch
that sets these environment variables before the hooks are invoked.
The patch also updates the documentation and adds some tests.

Regards,
Gisle

[-- Attachment #2: 0001-Set-ident-environment-variables-for-the-commit-hooks.patch --]
[-- Type: application/octet-stream, Size: 6049 bytes --]

From c352cfdda7dbd3f9185d4c0b1b4472c69cd64399 Mon Sep 17 00:00:00 2001
From: Gisle Aas <gisle@aas.no>
Date: Mon, 5 Jul 2010 17:45:30 +0200
Subject: [PATCH] Set ident environment variables for the commit hooks to observe

The GIT_{AUTHOR,COMMITTER}_{NAME,EMAIL} environment variables are set to
reflect the values about to be committed.
---
 Documentation/githooks.txt |    4 ++-
 builtin/commit.c           |    9 ++++---
 cache.h                    |    2 +
 ident.c                    |   46 ++++++++++++++++++++++++++++++-------------
 t/t7503-pre-commit-hook.sh |   26 ++++++++++++++++++++++++
 5 files changed, 68 insertions(+), 19 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 7183aa9..a330a67 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -84,7 +84,9 @@ such a line is found.
 
 All the 'git commit' hooks are invoked with the environment
 variable `GIT_EDITOR=:` if the command will not bring up an editor
-to modify the commit message.
+to modify the commit message.  The 'GIT_AUTHOR_NAME', 'GIT_AUTHOR_EMAIL',
+'GIT_COMMITTER_NAME', 'GIT_COMMITTER_EMAIL' environment variables
+will also be set.
 
 prepare-commit-msg
 ~~~~~~~~~~~~~~~~~~
diff --git a/builtin/commit.c b/builtin/commit.c
index c101f00..06e9342 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -557,6 +557,11 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	const char *hook_arg2 = NULL;
 	int ident_shown = 0;
 
+	determine_author_info();
+	git_committer_info(0); /* This checks if committer ident is explicitly given */
+	git_setenv_ident(author_name, author_email,
+			 getenv("GIT_COMMITTER_NAME"), getenv("GIT_COMMITTER_EMAIL"));
+
 	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
 		return 0;
 
@@ -632,10 +637,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 
 	strbuf_release(&sb);
 
-	determine_author_info();
-
-	/* This checks if committer ident is explicitly given */
-	git_committer_info(0);
 	if (use_editor && include_status) {
 		char *author_ident;
 		const char *committer_ident;
diff --git a/cache.h b/cache.h
index c9fa3df..2e70ff7 100644
--- a/cache.h
+++ b/cache.h
@@ -822,6 +822,8 @@ enum date_mode parse_date_format(const char *format);
 #define IDENT_NO_DATE	       4
 extern const char *git_author_info(int);
 extern const char *git_committer_info(int);
+extern void git_setenv_ident(const char *author_name, const char *author_email,
+			     const char *committer_name, const char *committer_email);
 extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);
 extern const char *fmt_name(const char *name, const char *email);
 extern const char *git_editor(void);
diff --git a/ident.c b/ident.c
index 9e24388..190e4a0 100644
--- a/ident.c
+++ b/ident.c
@@ -182,39 +182,45 @@ static const char *env_hint =
 "Omit --global to set the identity only in this repository.\n"
 "\n";
 
-const char *fmt_ident(const char *name, const char *email,
-		      const char *date_str, int flag)
+static void get_ident(const char **name, const char **email, int flag)
 {
-	static char buffer[1000];
-	char date[50];
-	int i;
 	int error_on_no_name = (flag & IDENT_ERROR_ON_NO_NAME);
 	int warn_on_no_name = (flag & IDENT_WARN_ON_NO_NAME);
-	int name_addr_only = (flag & IDENT_NO_DATE);
 
 	setup_ident();
-	if (!name)
-		name = git_default_name;
-	if (!email)
-		email = git_default_email;
+	if (!*name)
+		*name = git_default_name;
+	if (!*email)
+		*email = git_default_email;
 
-	if (!*name) {
+	if (!**name) {
 		struct passwd *pw;
 
 		if ((warn_on_no_name || error_on_no_name) &&
-		    name == git_default_name && env_hint) {
+		    *name == git_default_name && env_hint) {
 			fputs(env_hint, stderr);
 			env_hint = NULL; /* warn only once */
 		}
 		if (error_on_no_name)
-			die("empty ident %s <%s> not allowed", name, email);
+			die("empty ident %s <%s> not allowed", *name, *email);
 		pw = getpwuid(getuid());
 		if (!pw)
 			die("You don't exist. Go away!");
 		strlcpy(git_default_name, pw->pw_name,
 			sizeof(git_default_name));
-		name = git_default_name;
+		*name = git_default_name;
 	}
+}
+
+const char *fmt_ident(const char *name, const char *email,
+		      const char *date_str, int flag)
+{
+	static char buffer[1000];
+	char date[50];
+	int i;
+	int name_addr_only = (flag & IDENT_NO_DATE);
+
+	get_ident(&name, &email, flag);
 
 	strcpy(date, git_default_date);
 	if (!name_addr_only && date_str)
@@ -240,6 +246,18 @@ const char *fmt_name(const char *name, const char *email)
 	return fmt_ident(name, email, NULL, IDENT_ERROR_ON_NO_NAME | IDENT_NO_DATE);
 }
 
+void git_setenv_ident(const char *author_name, const char *author_email,
+		      const char *committer_name, const char *committer_email)
+{
+	get_ident(&author_name, &author_email, IDENT_ERROR_ON_NO_NAME);
+	setenv("GIT_AUTHOR_NAME", author_name, 1);
+	setenv("GIT_AUTHOR_EMAIL", author_email, 1);
+
+	get_ident(&committer_name, &committer_email, IDENT_ERROR_ON_NO_NAME);
+	setenv("GIT_COMMITTER_NAME", committer_name, 1);
+	setenv("GIT_COMMITTER_EMAIL", committer_email, 1);
+}
+
 const char *git_author_info(int flag)
 {
 	return fmt_ident(getenv("GIT_AUTHOR_NAME"),
diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index 8528f64..21ac57d 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
@@ -85,4 +85,30 @@ test_expect_success POSIXPERM '--no-verify with non-executable hook' '
 
 '
 
+# a hook that test author
+cat > "$HOOK" <<EOT
+#!/bin/sh
+case "\$GIT_AUTHOR_NAME" in Superman) exit 0;; esac
+echo "Only patches from Superman is accepted now"
+exit 1
+EOT
+cat "$HOOK"
+chmod +x "$HOOK"
+
+test_expect_success 'with failing hook' '
+
+	echo "another" >> file &&
+	git add file &&
+	test_must_fail git commit -m "another"
+
+'
+
+test_expect_success 'with non-failing hook' '
+
+	echo "another" >> file &&
+	git add file &&
+	git commit -m "another" --author "Superman <root@example.com>"
+
+'
+
 test_done
-- 
1.6.6.rc1.31.g1a56b

Re: Make author and committer available to pre-commit hook

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 5, 2010 at 18:04, Gisle Aas <gisle@aas.no> wrote:
> I agree that would be a more natural interface.  Attached is a patch
> that sets these environment variables before the hooks are invoked.
> The patch also updates the documentation and adds some tests.

Hi, just a note that patches submitted for inclusion in Git should
have [PATCH] in the subject, be submitted inline and you should CC
Junio. See Documentation/SubmittingPatches for the instructions.

Re: Make author and committer available to pre-commit hook

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Hi, just a note that patches submitted for inclusion in Git should
> have [PATCH] in the subject, be submitted inline and you should CC
> Junio.

Thanks.  Please add "... after a list concensus that it is a good change
is reached" at the end.

I wonder if we should also make author/committer dates available via the
same mechanism.

Re: Make author and committer available to pre-commit hook

[Jeff King]

On Mon, Jul 05, 2010 at 08:04:39PM -0700, Junio C Hamano wrote:

> I wonder if we should also make author/committer dates available via the
> same mechanism.

I don't see why not. It is a trivial amount of code, and I can see at
least author date being useful.

-Peff

Re: Make author and committer available to pre-commit hook

[Gisle Aas]

On Tue, Jul 6, 2010 at 09:29, Jeff King <peff@peff.net> wrote:
> On Mon, Jul 05, 2010 at 08:04:39PM -0700, Junio C Hamano wrote:
>
>> I wonder if we should also make author/committer dates available via the
>> same mechanism.
>
> I don't see why not. It is a trivial amount of code, and I can see at
> least author date being useful.

One question that arise is what format the date variables should have.
 Is the raw format (that is "1278400946 +0200") sensible?

I've created a patch for this locally, but it has some side effects in
messing up the dates stored by 'git commit -C...' which I have not
figured out yet; and now I need to get some other stuff done.  It
might take some days until a revised patch is ready.  Feel free to
contribute your "trivial amount of code" before I get around to it :-)

Regards,
Gisle

Re: Make author and committer available to pre-commit hook

[Junio C Hamano]

Gisle Aas <gisle@aas.no> writes:

> One question that arise is what format the date variables should have.
> Is the raw format (that is "1278400946 +0200") sensible?

I'd say so.  We are doing this for scripts (hooks) and there is not much
point doing machine-readable-to-human conversion they could do if they so
choose.

> I've created a patch for this locally, but it has some side effects in
> messing up the dates stored by 'git commit -C...' which I have not
> figured out yet; and now I need to get some other stuff done.

I imagine that you would be sending the date information just the same way
you send the author information, so if your addition messes up the dates
for "commit -C", wouldn't your original patch mess up the authorship
information the same way?

In any case, this new feature won't be in 'master' nor 'next' while we are
in pre-release freeze, so no need to rush.

Thanks.