Stop a git commit by a specific author using pre-commit hook

Stop a git commit by a specific author using pre-commit hook

[Adrian Cornish]

Hi All,

My plan is to use git to keep track of changes in /etc but when
committing I want to have the person making the change specify
themselves as author by adding the --author option on the commandline.

So I would like to stop accidental commits as root.

I tried creating this pre-commit hook but it is not working - git var
is still returning root even if I specify the author on commit line.

TIA
Adrian

AUTHOR=`git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/\1/p'`
if [ "$AUTHOR" == "root <root@localhost>" ];
then
   echo "Please commit under your own user name instead of \"$AUTHOR\":"
   echo 'git commit --author="Adrian"'
   echo "or if your name is not already in logs use full ident"
   echo 'git commit --author="Adrian Cornish <a@localhost>"'
   exit 1
fi
exit 0

Re: Stop a git commit by a specific author using pre-commit hook

[Neal Kreitzinger]

On 3/8/2012 7:15 PM, Adrian Cornish wrote:
> My plan is to use git to keep track of changes in /etc but when
> committing I want to have the person making the change specify
> themselves as author by adding the --author option on the commandline.
>
> So I would like to stop accidental commits as root.
>
> I tried creating this pre-commit hook but it is not working - git var
> is still returning root even if I specify the author on commit line.
>
> AUTHOR=`git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/\1/p'`
> if [ "$AUTHOR" == "root<root@localhost>" ];
> then
>     echo "Please commit under your own user name instead of \"$AUTHOR\":"
>     echo 'git commit --author="Adrian"'
>     echo "or if your name is not already in logs use full ident"
>     echo 'git commit --author="Adrian Cornish<a@localhost>"'
>     exit 1
> fi
> exit 0

We use whoami in our pre-commit hook to see who the user is that is 
doing the commit.  I think you could also use GIT_COMMITTER_NAME or 
linux $USER environment variables.  Either way, the --author seems like 
an unnecessary and unreliable way to get the username.  You're relying 
on the user to remember or obey that step.  By doing --author, you also 
seem to be implying that the git commit author value may not be the same 
as the user doing the commit so that would also ruin the original commit 
author data audit trail.  I haven't used these particular git 
environment variables, but I suspect they are dependent upon the user 
having their gitconfig filled out properly, so the linux whoami and 
$USER are probably more reliable.  If people can su to root then $USER 
will not work because it will still be set to their original user name 
(before they did su to root).  Therefore, "whoami" seems like your best 
solution.

v/r,
neal

Re: Stop a git commit by a specific author using pre-commit hook

[Junio C Hamano]

Neal Kreitzinger <nkreitzinger@gmail.com> writes:

> On 3/8/2012 7:15 PM, Adrian Cornish wrote:
>> My plan is to use git to keep track of changes in /etc but when
>> committing I want to have the person making the change specify
>> themselves as author by adding the --author option on the commandline.
>
>> So I would like to stop accidental commits as root.
> ...
> We use whoami in our pre-commit hook to see who the user is that is
> doing the commit.  I think you could also use GIT_COMMITTER_NAME or
> linux $USER environment variables.  Either way, the --author seems
> like an unnecessary and unreliable way to get the username.
> ...  If people can su to root
> then $USER will not work because it will still be set to their
> original user name (before they did su to root).  Therefore, "whoami"
> seems like your best solution.

When people want to raise a red flag against a commit made by root,
they are coming from two different schools.

One is "do not run 'git' or any development tool for that matter
while being root". It is a good discipline to follow in general to
limit what you do with escalated privilege to the minimum.

The other is "record who actually did the work, not 'root' that
people cannot later track down who it actually was".  People from
this school do not mind running development tools as root.

And your advice is a good one for the former, but not very relevant
for the latter.

And I think Adrian is asking for the latter.

Re: Stop a git commit by a specific author using pre-commit hook

[Neal Kreitzinger]

On 3/10/2012 3:54 PM, Junio C Hamano wrote:
> Neal Kreitzinger<nkreitzinger@gmail.com>  writes:
>
>> On 3/8/2012 7:15 PM, Adrian Cornish wrote:
>>> My plan is to use git to keep track of changes in /etc but when
>>> committing I want to have the person making the change specify
>>> themselves as author by adding the --author option on the commandline.
>>> So I would like to stop accidental commits as root.
>> ...
>> We use whoami in our pre-commit hook to see who the user is that is
>> doing the commit.  I think you could also use GIT_COMMITTER_NAME or
>> linux $USER environment variables.  Either way, the --author seems
>> like an unnecessary and unreliable way to get the username.
>> ...  If people can su to root
>> then $USER will not work because it will still be set to their
>> original user name (before they did su to root).  Therefore, "whoami"
>> seems like your best solution.
> When people want to raise a red flag against a commit made by root,
> they are coming from two different schools.
>
> One is "do not run 'git' or any development tool for that matter
> while being root". It is a good discipline to follow in general to
> limit what you do with escalated privilege to the minimum.
>
> The other is "record who actually did the work, not 'root' that
> people cannot later track down who it actually was".  People from
> this school do not mind running development tools as root.
>
> And your advice is a good one for the former, but not very relevant
> for the latter.
>
> And I think Adrian is asking for the latter.
>
Now I see.  In that case, at the point pre-commit is run git has not 
overriden GIT_AUTHOR_IDENT with your --author value, yet.  I don't know 
if that is a bug or not.  The prepare-commit-msg hook is the same way.  
However, by the time the commit-msg hook runs git has placed your 
--author override into GIT_AUTHOR_IDENT so if you check it there it will 
work and you can abort the commit.  Of course, at that point the user 
has already typed their commit message and may lose it.  You could 
create a git alias like "git root-commit" that prompts them to enter 
their authorname and then runs git-commit --author with that user 
provided value.  Pre-commit hook could detect root user and error out 
telling them to run git-root-commit instead.

v/r,
neal

Re: Stop a git commit by a specific author using pre-commit hook

[Junio C Hamano]

Neal Kreitzinger <nkreitzinger@gmail.com> writes:

> ... at the point pre-commit is run git has not
> overriden GIT_AUTHOR_IDENT with your --author value, yet.  I don't
> know if that is a bug or not.  The prepare-commit-msg hook is the same
> way.

I'd call it a bug, as the command line --author should have the same
effect as GIT_AUTHOR_NAME and friends observable by the outside
world.

There are two slightly different approaches to solve this, that
share the first two preparatory commits.  I'll send out the
preparatory ones first and then show both of these approaches.

[PATCH 1/(3/4)] test: does pre-commit-hook learn authorship?

[Junio C Hamano]

When "--author" option is used to lie the authorship to "git commit"
command, hooks should learn the author name and email just like when
GIT_AUTHOR_NAME and GIT_AUTHOR_EMAIL environment variables are used
to lie the authorship.  Test this.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * This is the first one of two patches that are shread by two
   approaches. What it tests should be obvious---the current code
   fails with the last piece that use --author.

 t/t7503-pre-commit-hook.sh |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index ee7f0cd..fc6de5b 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
@@ -118,4 +118,22 @@ test_expect_success 'with failing hook requiring GIT_PREFIX' '
 	git checkout -- file
 '
 
+test_expect_failure 'check the author in hook' '
+	cat >"$HOOK" <<-\EOF &&
+	test "$GIT_AUTHOR_NAME" = "New Author" &&
+	test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
+	EOF
+	test_must_fail git commit --allow-empty -m "by a.u.thor" &&
+	(
+		GIT_AUTHOR_NAME="New Author" &&
+		GIT_AUTHOR_EMAIL="newauthor@example.com" &&
+		export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
+		git commit --allow-empty -m "by new.author via env" &&
+		git show -s
+	) &&
+	git commit --author="New Author <newauthor@example.com>" \
+		--allow-empty -m "by new.author via command line" &&
+	git show -s
+'
+
 test_done
-- 
1.7.10.rc0.33.g8866af

[PATCH 2/(3/4)] ident.c: add split_ident_line() to parse formatted ident line

[Junio C Hamano]

The commit formatting logic format_person_part() in pretty.c
implements the logic to split an author/committer ident line into
its parts, intermixed with logic to compute its output using these
piece it computes.

Separate the former out to a helper function split_ident_line() so
that other codepath can use the same logic, and rewrite the function
using the helper function.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * The second shared change between the two approaches.

 cache.h  |   16 +++++++++++++++
 ident.c  |   68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 pretty.c |   64 ++++++++++++++++++----------------------------------------
 3 files changed, 104 insertions(+), 44 deletions(-)

diff --git a/cache.h b/cache.h
index 3a8e125..7c42eca 100644
--- a/cache.h
+++ b/cache.h
@@ -928,6 +928,22 @@ extern const char *fmt_name(const char *name, const char *email);
 extern const char *git_editor(void);
 extern const char *git_pager(int stdout_is_tty);
 
+struct ident_split {
+	const char *name_begin;
+	const char *name_end;
+	const char *mail_begin;
+	const char *mail_end;
+	const char *date_begin;
+	const char *date_end;
+	const char *tz_begin;
+	const char *tz_end;
+};
+/*
+ * Signals an success with 0, but time part of the result may be NULL
+ * if the input lacks timestamp and zone
+ */
+extern int split_ident_line(struct ident_split *, const char *, int);
+
 struct checkout {
 	const char *base_dir;
 	int base_dir_len;
diff --git a/ident.c b/ident.c
index f619619..87c697c 100644
--- a/ident.c
+++ b/ident.c
@@ -220,6 +220,74 @@ static int copy(char *buf, size_t size, int offset, const char *src)
 	return offset;
 }
 
+/*
+ * Reverse of fmt_ident(); given an ident line, split the fields
+ * to allow the caller to parse it.
+ * Signal a success by returning 0, but date/tz fields of the result
+ * can still be NULL if the input line only has the name/email part
+ * (e.g. reading from a reflog entry).
+ */
+int split_ident_line(struct ident_split *split, const char *line, int len)
+{
+	const char *cp;
+	size_t span;
+	int status = -1;
+
+	memset(split, 0, sizeof(*split));
+
+	split->name_begin = line;
+	for (cp = line; *cp && cp < line + len; cp++)
+		if (*cp == '<') {
+			split->mail_begin = cp + 1;
+			break;
+		}
+	if (!split->mail_begin)
+		return status;
+
+	for (cp = split->mail_begin - 2; line < cp; cp--)
+		if (!isspace(*cp)) {
+			split->name_end = cp + 1;
+			break;
+		}
+	if (!split->name_end)
+		return status;
+
+	for (cp = split->mail_begin; cp < line + len; cp++)
+		if (*cp == '>') {
+			split->mail_end = cp;
+			break;
+		}
+	if (!split->mail_end)
+		return status;
+
+	for (cp = split->mail_end + 1; cp < line + len && isspace(*cp); cp++)
+		;
+	if (line + len <= cp)
+		goto person_only;
+	split->date_begin = cp;
+	span = strspn(cp, "0123456789");
+	if (!span)
+		goto person_only;
+	split->date_end = split->date_begin + span;
+	for (cp = split->date_end; cp < line + len && isspace(*cp); cp++)
+		;
+	if (line + len <= cp || (*cp != '+' && *cp != '-'))
+		goto person_only;
+	split->tz_begin = cp;
+	span = strspn(cp + 1, "0123456789");
+	if (!span)
+		goto person_only;
+	split->tz_end = split->tz_begin + 1 + span;
+	return 0;
+
+person_only:
+	split->date_begin = NULL;
+	split->date_end = NULL;
+	split->tz_begin = NULL;
+	split->tz_end = NULL;
+	return 0;
+}
+
 static const char *env_hint =
 "\n"
 "*** Please tell me who you are.\n"
diff --git a/pretty.c b/pretty.c
index 8688b8f..f2dee30 100644
--- a/pretty.c
+++ b/pretty.c
@@ -531,41 +531,24 @@ static size_t format_person_part(struct strbuf *sb, char part,
 {
 	/* currently all placeholders have same length */
 	const int placeholder_len = 2;
-	int start, end, tz = 0;
+	int tz;
 	unsigned long date = 0;
-	char *ep;
-	const char *name_start, *name_end, *mail_start, *mail_end, *msg_end = msg+len;
 	char person_name[1024];
 	char person_mail[1024];
+	struct ident_split s;
+	const char *name_start, *name_end, *mail_start, *mail_end;
 
-	/* advance 'end' to point to email start delimiter */
-	for (end = 0; end < len && msg[end] != '<'; end++)
-		; /* do nothing */
-
-	/*
-	 * When end points at the '<' that we found, it should have
-	 * matching '>' later, which means 'end' must be strictly
-	 * below len - 1.
-	 */
-	if (end >= len - 2)
+	if (split_ident_line(&s, msg, len) < 0)
 		goto skip;
 
-	/* Seek for both name and email part */
-	name_start = msg;
-	name_end = msg+end;
-	while (name_end > name_start && isspace(*(name_end-1)))
-		name_end--;
-	mail_start = msg+end+1;
-	mail_end = mail_start;
-	while (mail_end < msg_end && *mail_end != '>')
-		mail_end++;
-	if (mail_end == msg_end)
-		goto skip;
-	end = mail_end-msg;
+	name_start = s.name_begin;
+	name_end = s.name_end;
+	mail_start = s.mail_begin;
+	mail_end = s.mail_end;
 
 	if (part == 'N' || part == 'E') { /* mailmap lookup */
-		strlcpy(person_name, name_start, name_end-name_start+1);
-		strlcpy(person_mail, mail_start, mail_end-mail_start+1);
+		strlcpy(person_name, name_start, name_end - name_start + 1);
+		strlcpy(person_mail, mail_start, mail_end - mail_start + 1);
 		mailmap_name(person_mail, sizeof(person_mail), person_name, sizeof(person_name));
 		name_start = person_name;
 		name_end = name_start + strlen(person_name);
@@ -581,28 +564,20 @@ static size_t format_person_part(struct strbuf *sb, char part,
 		return placeholder_len;
 	}
 
-	/* advance 'start' to point to date start delimiter */
-	for (start = end + 1; start < len && isspace(msg[start]); start++)
-		; /* do nothing */
-	if (start >= len)
-		goto skip;
-	date = strtoul(msg + start, &ep, 10);
-	if (msg + start == ep)
+	if (!s.date_begin)
 		goto skip;
 
+	date = strtoul(s.date_begin, NULL, 10);
+
 	if (part == 't') {	/* date, UNIX timestamp */
-		strbuf_add(sb, msg + start, ep - (msg + start));
+		strbuf_add(sb, s.date_begin, s.date_end - s.date_begin);
 		return placeholder_len;
 	}
 
 	/* parse tz */
-	for (start = ep - msg + 1; start < len && isspace(msg[start]); start++)
-		; /* do nothing */
-	if (start + 1 < len) {
-		tz = strtoul(msg + start + 1, NULL, 10);
-		if (msg[start] == '-')
-			tz = -tz;
-	}
+	tz = strtoul(s.tz_begin + 1, NULL, 10);
+	if (*s.tz_begin == '-')
+		tz = -tz;
 
 	switch (part) {
 	case 'd':	/* date */
@@ -621,8 +596,9 @@ static size_t format_person_part(struct strbuf *sb, char part,
 
 skip:
 	/*
-	 * bogus commit, 'sb' cannot be updated, but we still need to
-	 * compute a valid return value.
+	 * reading from either a bogus commit, or a reflog entry with
+	 * %gn, %ge, etc.; 'sb' cannot be updated, but we still need
+	 * to compute a valid return value.
 	 */
 	if (part == 'n' || part == 'e' || part == 't' || part == 'd'
 	    || part == 'D' || part == 'r' || part == 'i')
-- 
1.7.10.rc0.33.g8866af

[PATCH 3/3] commit: pass author/committer info to hooks

[Junio C Hamano]

When lying the author name via GIT_AUTHOR_NAME environment variable
to "git commit", the hooks run by the command saw it and could act
on the name that will be recorded in the final commit. When the user
uses the "--author" option from the command line, the command should
give the same information to the hook, and back when "git command"
was a scripted Porcelain, it did set the environment variable and
hooks can learn the author name from it.

However, when the command was reimplemented in C, the rewritten code
was not very faithful to the original, and hooks stopped getting the
authorship information given with "--author".  Fix this by exporting
the necessary environment variables.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is the last patch of the simpler of the two approaches, that
   builds on top of the two common preparatory patches.  It uses setenv()
   to directly affect the execution environment of "git commit" process,
   which is closer to the original scripted Porcelain implementation.

 builtin/commit.c           |   22 +++++++++++++++++++---
 t/t7503-pre-commit-hook.sh |    2 +-
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index eae5a29..57a60f9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -533,9 +533,20 @@ static int is_a_merge(const struct commit *current_head)
 
 static const char sign_off_header[] = "Signed-off-by: ";
 
+static void export_one(const char *var, const char *s, const char *e, int hack)
+{
+	struct strbuf buf = STRBUF_INIT;
+	if (hack)
+		strbuf_addch(&buf, hack);
+	strbuf_addf(&buf, "%.*s", (int)(e - s), s);
+	setenv(var, buf.buf, 1);
+	strbuf_release(&buf);
+}
+
 static void determine_author_info(struct strbuf *author_ident)
 {
 	char *name, *email, *date;
+	struct ident_split author;
 
 	name = getenv("GIT_AUTHOR_NAME");
 	email = getenv("GIT_AUTHOR_EMAIL");
@@ -585,6 +596,11 @@ static void determine_author_info(struct strbuf *author_ident)
 		date = force_date;
 	strbuf_addstr(author_ident, fmt_ident(name, email, date,
 					      IDENT_ERROR_ON_NO_NAME));
+	if (!split_ident_line(&author, author_ident->buf, author_ident->len)) {
+		export_one("GIT_AUTHOR_NAME", author.name_begin, author.name_end, 0);
+		export_one("GIT_AUTHOR_EMAIL", author.mail_begin, author.mail_end, 0);
+		export_one("GIT_AUTHOR_DATE", author.date_begin, author.tz_end, '@');
+	}
 }
 
 static int ends_rfc2822_footer(struct strbuf *sb)
@@ -652,6 +668,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	int ident_shown = 0;
 	int clean_message_contents = (cleanup_mode != CLEANUP_NONE);
 
+	/* This checks and barfs if author is badly specified */
+	determine_author_info(author_ident);
+
 	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
 		return 0;
 
@@ -771,9 +790,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 
 	strbuf_release(&sb);
 
-	/* This checks and barfs if author is badly specified */
-	determine_author_info(author_ident);
-
 	/* This checks if committer ident is explicitly given */
 	strbuf_addstr(&committer_ident, git_committer_info(0));
 	if (use_editor && include_status) {
diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index fc6de5b..9301a0c 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
@@ -118,7 +118,7 @@ test_expect_success 'with failing hook requiring GIT_PREFIX' '
 	git checkout -- file
 '
 
-test_expect_failure 'check the author in hook' '
+test_expect_success 'check the author in hook' '
 	cat >"$HOOK" <<-\EOF &&
 	test "$GIT_AUTHOR_NAME" = "New Author" &&
 	test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
-- 
1.7.10.rc0.33.g8866af

[PATCH 3/4] run_hook(): enhance the interface to pass arbitrary environment

[Junio C Hamano]

Originally "run_hook()" that used to live in builtin/commit.c had a
perfectly appropriate API and name for a private function to spawn a
hook from "git commit" command. The only necessary tweak in the
context was to optionally specify which file to use as the index
file.

But then we stupidly moved this private API to run-command.c without
making the interface suitable for general consumption, and there is
no way to tweak environment variables other than GIT_INDEX_FILE when
running a hook.

Correct this mistake by adding run_hook_e() that takes an array of
environment variables.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is the third patch of the more complex of the two approaches.
   Instead of the simpler "setenv()" one, the final one tries to
   limit the environment change only to the hook scripts, without
   affecting the "git commit" process itself. It probably should not
   make much difference in the short run, but later when we want to
   distinguish where the authorship came from, it may be easier to
   tell with this approach as it does not contaminate the environment
   of "git commit" itself with what we obtained from "--author" option.

   Also this is independently a useful refactoring.

 run-command.c |   49 ++++++++++++++++++++++++++++++++++++-------------
 run-command.h |    1 +
 2 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/run-command.c b/run-command.c
index 1db8abf..a51d8e5 100644
--- a/run-command.c
+++ b/run-command.c
@@ -672,36 +672,59 @@ int finish_async(struct async *async)
 #endif
 }
 
-int run_hook(const char *index_file, const char *name, ...)
+static int run_hook_le(const char *const *env,
+		       const char *name,
+		       va_list args)
 {
 	struct child_process hook;
 	struct argv_array argv = ARGV_ARRAY_INIT;
-	const char *p, *env[2];
-	char index[PATH_MAX];
-	va_list args;
+	const char *p;
 	int ret;
 
 	if (access(git_path("hooks/%s", name), X_OK) < 0)
 		return 0;
 
-	va_start(args, name);
 	argv_array_push(&argv, git_path("hooks/%s", name));
 	while ((p = va_arg(args, const char *)))
 		argv_array_push(&argv, p);
-	va_end(args);
 
 	memset(&hook, 0, sizeof(hook));
 	hook.argv = argv.argv;
 	hook.no_stdin = 1;
 	hook.stdout_to_stderr = 1;
-	if (index_file) {
-		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-		env[0] = index;
-		env[1] = NULL;
-		hook.env = env;
-	}
-
+	hook.env = env;
 	ret = run_command(&hook);
 	argv_array_clear(&argv);
 	return ret;
 }
+
+int run_hook(const char *index_file, const char *name, ...)
+{
+	const char *const *env = NULL, *env_buf[2];
+	struct strbuf index_buf = STRBUF_INIT;
+	int status;
+	va_list args;
+
+	va_start(args, name);
+	if (index_file) {
+		strbuf_addf(&index_buf, "GIT_INDEX_FILE=%s", index_file);
+		env_buf[0] = index_buf.buf;
+		env_buf[1] = NULL;
+		env = env_buf;
+	}
+	status = run_hook_le(env, name, args);
+	va_end(args);
+	strbuf_release(&index_buf);
+	return status;
+}
+
+int run_hook_e(const char *const *env, const char *name, ...)
+{
+	int status;
+	va_list args;
+
+	va_start(args, name);
+	status = run_hook_le(env, name, args);
+	va_end(args);
+	return status;
+}
diff --git a/run-command.h b/run-command.h
index 44f7d2b..87207b9 100644
--- a/run-command.h
+++ b/run-command.h
@@ -47,6 +47,7 @@ int finish_command(struct child_process *);
 int run_command(struct child_process *);
 
 extern int run_hook(const char *index_file, const char *name, ...);
+extern int run_hook_e(const char *const *, const char *name, ...);
 
 #define RUN_COMMAND_NO_STDIN 1
 #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */
-- 
1.7.10.rc0.33.g8866af

[PATCH 4/4] commit: pass author/committer info to hooks

[Junio C Hamano]

When lying the author name via GIT_AUTHOR_NAME environment variable
to "git commit", the hooks run by the command saw it and could act
on the name that will be recorded in the final commit. When the user
uses the "--author" option from the command line, the command should
give the same information to the hook, and back when "git command"
was a scripted Porcelain, it did set the environment variable and
hooks can learn the author name from it.

However, when the command was reimplemented in C, the rewritten code
was not very faithful to the original, and hooks stopped getting the
authorship information given with "--author".  Fix this by exporting
the necessary environment variables.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * And this is the final patch in the more complex of the two
   approaches.

 builtin/commit.c           |  132 ++++++++++++++++++++++++++++++--------------
 t/t7503-pre-commit-hook.sh |    2 +-
 2 files changed, 93 insertions(+), 41 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index eae5a29..4a47e39 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -27,6 +27,7 @@
 #include "quote.h"
 #include "submodule.h"
 #include "gpg-interface.h"
+#include "argv-array.h"
 
 static const char * const builtin_commit_usage[] = {
 	"git commit [options] [--] <filepattern>...",
@@ -637,13 +638,63 @@ static char *cut_ident_timestamp_part(char *string)
 	return ket;
 }
 
-static int prepare_to_commit(const char *index_file, const char *prefix,
+struct commit_context {
+	const char *index_file;
+	struct strbuf *author_ident;
+	struct strbuf *committer_ident;
+	struct argv_array env;
+};
+
+static void setup_commit_context(struct commit_context *ctx,
+				 const char *index_file,
+				 struct strbuf *author_ident,
+				 struct strbuf *committer_ident)
+{
+	struct ident_split s;
+	struct argv_array *env = &ctx->env;
+
+	ctx->index_file = index_file;
+	ctx->author_ident = author_ident;
+	ctx->committer_ident = committer_ident;
+	argv_array_init(env);
+	if (index_file)
+		argv_array_pushf(env, "GIT_INDEX_FILE=%s", index_file);
+
+	if (!split_ident_line(&s, author_ident->buf, author_ident->len)) {
+		argv_array_pushf(env, "GIT_AUTHOR_NAME=%.*s",
+				 (int)(s.name_end - s.name_begin),
+				 s.name_begin);
+		argv_array_pushf(env, "GIT_AUTHOR_EMAIL=%.*s",
+				 (int)(s.mail_end - s.mail_begin),
+				 s.mail_begin);
+		argv_array_pushf(env, "GIT_AUTHOR_DATE=@%.*s %.*s",
+				 (int)(s.date_end - s.date_begin),
+				 s.date_begin,
+				 (int)(s.tz_end - s.tz_begin),
+				 s.tz_begin);
+	}
+
+	if (!split_ident_line(&s, committer_ident->buf, committer_ident->len)) {
+		argv_array_pushf(env, "GIT_COMMITTER_NAME=%.*s",
+				 (int)(s.name_end - s.name_begin),
+				 s.name_begin);
+		argv_array_pushf(env, "GIT_COMMITTER_EMAIL=%.*s",
+				 (int)(s.mail_end - s.mail_begin),
+				 s.mail_begin);
+		argv_array_pushf(env, "GIT_COMMITTER_DATE=@%.*s %.*s",
+				 (int)(s.date_end - s.date_begin),
+				 s.date_begin,
+				 (int)(s.tz_end - s.tz_begin),
+				 s.tz_begin);
+	}
+}
+
+static int prepare_to_commit(struct commit_context *ctx,
+			     const char *prefix,
 			     struct commit *current_head,
-			     struct wt_status *s,
-			     struct strbuf *author_ident)
+			     struct wt_status *s)
 {
 	struct stat statbuf;
-	struct strbuf committer_ident = STRBUF_INIT;
 	int commitable, saved_color_setting;
 	struct strbuf sb = STRBUF_INIT;
 	char *buffer;
@@ -652,7 +703,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	int ident_shown = 0;
 	int clean_message_contents = (cleanup_mode != CLEANUP_NONE);
 
-	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
+	if (!no_verify && run_hook_e(ctx->env.argv, "pre-commit", NULL))
 		return 0;
 
 	if (squash_message) {
@@ -771,11 +822,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 
 	strbuf_release(&sb);
 
-	/* This checks and barfs if author is badly specified */
-	determine_author_info(author_ident);
-
-	/* This checks if committer ident is explicitly given */
-	strbuf_addstr(&committer_ident, git_committer_info(0));
 	if (use_editor && include_status) {
 		char *ai_tmp, *ci_tmp;
 		if (whence != FROM_COMMIT)
@@ -809,28 +855,28 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			status_printf_ln(s, GIT_COLOR_NORMAL,
 					"%s", only_include_assumed);
 
-		ai_tmp = cut_ident_timestamp_part(author_ident->buf);
-		ci_tmp = cut_ident_timestamp_part(committer_ident.buf);
-		if (strcmp(author_ident->buf, committer_ident.buf))
+		ai_tmp = cut_ident_timestamp_part(ctx->author_ident->buf);
+		ci_tmp = cut_ident_timestamp_part(ctx->committer_ident->buf);
+		if (strcmp(ctx->author_ident->buf, ctx->committer_ident->buf))
 			status_printf_ln(s, GIT_COLOR_NORMAL,
-				_("%s"
-				"Author:    %s"),
-				ident_shown++ ? "" : "\n",
-				author_ident->buf);
+					 _("%s"
+					   "Author:    %s"),
+					 ident_shown++ ? "" : "\n",
+					 ctx->author_ident->buf);
 
 		if (!user_ident_sufficiently_given())
 			status_printf_ln(s, GIT_COLOR_NORMAL,
-				_("%s"
-				"Committer: %s"),
-				ident_shown++ ? "" : "\n",
-				committer_ident.buf);
+					 _("%s"
+					   "Committer: %s"),
+					 ident_shown++ ? "" : "\n",
+					 ctx->committer_ident->buf);
 
 		if (ident_shown)
 			status_printf_ln(s, GIT_COLOR_NORMAL, "");
 
 		saved_color_setting = s->use_color;
 		s->use_color = 0;
-		commitable = run_status(s->fp, index_file, prefix, 1, s);
+		commitable = run_status(s->fp, ctx->index_file, prefix, 1, s);
 		s->use_color = saved_color_setting;
 
 		*ai_tmp = ' ';
@@ -850,8 +896,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		else
 			commitable = index_differs_from(parent, 0);
 	}
-	strbuf_release(&committer_ident);
-
 	fclose(s->fp);
 
 	/*
@@ -861,7 +905,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 */
 	if (!commitable && whence != FROM_MERGE && !allow_empty &&
 	    !(amend && is_a_merge(current_head))) {
-		run_status(stdout, index_file, prefix, 0, s);
+		run_status(stdout, ctx->index_file, prefix, 0, s);
 		if (amend)
 			fputs(_(empty_amend_advice), stderr);
 		else if (whence == FROM_CHERRY_PICK)
@@ -875,22 +919,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 * the editor and after we invoke run_status above.
 	 */
 	discard_cache();
-	read_cache_from(index_file);
+	read_cache_from(ctx->index_file);
 	if (update_main_cache_tree(0)) {
 		error(_("Error building trees"));
 		return 0;
 	}
 
-	if (run_hook(index_file, "prepare-commit-msg",
-		     git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
+	if (run_hook_e(ctx->env.argv, "prepare-commit-msg",
+		       git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
 		return 0;
 
 	if (use_editor) {
-		char index[PATH_MAX];
-		const char *env[2] = { NULL };
-		env[0] =  index;
-		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-		if (launch_editor(git_path(commit_editmsg), NULL, env)) {
+		if (launch_editor(git_path(commit_editmsg), NULL, ctx->env.argv)) {
 			fprintf(stderr,
 			_("Please supply the message using either -m or -F option.\n"));
 			exit(1);
@@ -898,7 +938,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	}
 
 	if (!no_verify &&
-	    run_hook(index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
+	    run_hook_e(ctx->env.argv, "commit-msg", git_path(commit_editmsg), NULL)) {
 		return 0;
 	}
 
@@ -1389,6 +1429,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf author_ident = STRBUF_INIT;
+	struct strbuf committer_ident = STRBUF_INIT;
 	const char *index_file, *reflog_msg;
 	char *nl, *p;
 	unsigned char sha1[20];
@@ -1399,6 +1440,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	struct wt_status s;
 	struct commit *current_head = NULL;
 	struct commit_extra_header *extra = NULL;
+	struct commit_context commit_ctx;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_commit_usage, builtin_commit_options);
@@ -1420,10 +1462,16 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		return dry_run_commit(argc, argv, prefix, current_head, &s);
 	index_file = prepare_index(argc, argv, prefix, current_head, 0);
 
-	/* Set up everything for writing the commit object.  This includes
-	   running hooks, writing the trees, and interacting with the user.  */
-	if (!prepare_to_commit(index_file, prefix,
-			       current_head, &s, &author_ident)) {
+	/*
+	 * Set up everything for writing the commit object.  This includes
+	 * running hooks, writing the trees, and interacting with the user.
+	 */
+	determine_author_info(&author_ident);
+	strbuf_addstr(&committer_ident, git_committer_info(0));
+	setup_commit_context(&commit_ctx,
+			     index_file, &author_ident, &committer_ident);
+
+	if (!prepare_to_commit(&commit_ctx, prefix, current_head, &s)) {
 		rollback_index_files();
 		return 1;
 	}
@@ -1513,7 +1561,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		rollback_index_files();
 		die(_("failed to write commit object"));
 	}
-	strbuf_release(&author_ident);
 	free_commit_extra_headers(extra);
 
 	ref_lock = lock_any_ref_for_update("HEAD",
@@ -1552,7 +1599,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		     "not exceeded, and then \"git reset HEAD\" to recover."));
 
 	rerere(0);
-	run_hook(get_index_file(), "post-commit", NULL);
+
+	argv_array_clear(&commit_ctx.env);
+	setup_commit_context(&commit_ctx,
+			     get_index_file(),
+			     &author_ident, &committer_ident);
+	run_hook_e(commit_ctx.env.argv, "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
 		struct notes_rewrite_cfg *cfg;
 		cfg = init_copy_notes_for_rewrite("amend");
diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index fc6de5b..9301a0c 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
@@ -118,7 +118,7 @@ test_expect_success 'with failing hook requiring GIT_PREFIX' '
 	git checkout -- file
 '
 
-test_expect_failure 'check the author in hook' '
+test_expect_success 'check the author in hook' '
 	cat >"$HOOK" <<-\EOF &&
 	test "$GIT_AUTHOR_NAME" = "New Author" &&
 	test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
-- 
1.7.10.rc0.33.g8866af

Re: [PATCH 1/(3/4)] test: does pre-commit-hook learn authorship?

[Johannes Sixt]

Am 11.03.2012 12:08, schrieb Junio C Hamano:
> +test_expect_failure 'check the author in hook' '
> +	cat >"$HOOK" <<-\EOF &&
> +	test "$GIT_AUTHOR_NAME" = "New Author" &&
> +	test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
> +	EOF

Please insert #!/bin/sh as the first line of the script. We'll need it
on Windows.

-- Hannes

Re: [PATCH 1/(3/4)] test: does pre-commit-hook learn authorship?

[Junio C Hamano]

Thanks; I should have just used write_script() for this.

Re: [PATCH 3/3] commit: pass author/committer info to hooks

[Jeff King]

On Sun, Mar 11, 2012 at 04:11:38AM -0700, Junio C Hamano wrote:

> However, when the command was reimplemented in C, the rewritten code
> was not very faithful to the original, and hooks stopped getting the
> authorship information given with "--author".  Fix this by exporting
> the necessary environment variables.
> [...]
>  * This is the last patch of the simpler of the two approaches, that
>    builds on top of the two common preparatory patches.  It uses setenv()
>    to directly affect the execution environment of "git commit" process,
>    which is closer to the original scripted Porcelain implementation.

FWIW, I like this approach better. It's simpler, and I think it is
perfectly sane to define "--author" as "act as if the contents were in
GIT_AUTHOR_*"[1]. We do the same thing already with "git --git-dir",
"git --work-tree", and other variables, and it keeps the code simple by
making the environment variable the definitive source.

-Peff

[1] Your approach is slightly more complex, in that it handles "-c"
    (which is a good thing). But I think the principle is the same.