[PATCH v2] commit -c/-C/--amend: reset timestamp and authorship to committer with --reset-author

When we use -c, -C, or --amend, we are trying one of two things: using
the source as a template or modifying a commit with corrections.

When these options are used, the authorship and timestamp recorded in
the newly created commit are always taken from the original commit.
This is inconvenient when we just want to borrow the commit log message
or when our change to the code is so significant that we should take
over the authorship (with the blame for bugs we introduce, of course).

The new --reset-author option is meant to solve this need by
regenerating the timestamp and setting as the new author the committer
or the one specified by --author option.

Signed-off-by: Erick Mattos <redacted>
---

I have remade the testing script to let it easier for people to understand and
to make it do all reasonable tests.

I have made minor message log changes and as you can see by the third paragraph
I am showing a different approach to option --author.  Please read the following
text:

--author text says: "override author for commit".

As I see, something that OVERRIDES supersedes everything else.

IMHO --author shouldn't be blocked by the new option.

Cutting --author away would make impossible for someone to force a new author
with a new timestamp in case he is templating.  As an example he can be using
the --author because he is doing a change in a computer not his own or
something alike.

So I would not wipe "author" out from the new option.

Please don't forget that I am just being a small contributor.  I am just
suggesting things.  You have the final word and if you want we can add your
small test to block it:

	if (force_author && renew_authorship)
		die("Using both --reset-author and --author does not make sense");

 Documentation/git-commit.txt |    7 ++-
 builtin-commit.c             |    9 ++-
 t/t7509-commit.sh            |  123 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 135 insertions(+), 4 deletions(-)
 create mode 100755 t/t7509-commit.sh

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 0578a40..f89db9a 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt

@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git commit' [-a | --interactive] [-s] [-v] [-u<mode>] [--amend] [--dry-run]
-	   [(-c | -C) <commit>] [-F <file> | -m <msg>]
+	   [(-c | -C) <commit>] [-F <file> | -m <msg>] [--reset-author]
 	   [--allow-empty] [--no-verify] [-e] [--author=<author>]
 	   [--cleanup=<mode>] [--] [[-i | -o ]<file>...]
 

@@ -69,6 +69,11 @@ OPTIONS
 	Like '-C', but with '-c' the editor is invoked, so that
 	the user can further edit the commit message.
 
+--reset-author::
+	When used with -C/-c/--amend options, declare that the
+	authorship of the resulting commit now belongs of the committer.
+	This also renews the author timestamp.
+
 -F <file>::
 --file=<file>::
 	Take the commit message from the given file.  Use '-' to

diff --git a/builtin-commit.c b/builtin-commit.c
index beddf01..6b51a1b 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c

@@ -51,7 +51,7 @@ static const char *template_file;
 static char *edit_message, *use_message;
 static char *author_name, *author_email, *author_date;
 static int all, edit_flag, also, interactive, only, amend, signoff;
-static int quiet, verbose, no_verify, allow_empty, dry_run;
+static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static char *untracked_files_arg;
 /*
  * The default commit message cleanup mode will remove the lines

@@ -91,8 +91,9 @@ static struct option builtin_commit_options[] = {
 	OPT_FILENAME('F', "file", &logfile, "read log from file"),
 	OPT_STRING(0, "author", &force_author, "AUTHOR", "override author for commit"),
 	OPT_CALLBACK('m', "message", &message, "MESSAGE", "specify commit message", opt_parse_m),
-	OPT_STRING('c', "reedit-message", &edit_message, "COMMIT", "reuse and edit message from specified commit "),
+	OPT_STRING('c', "reedit-message", &edit_message, "COMMIT", "reuse and edit message from specified commit"),
 	OPT_STRING('C', "reuse-message", &use_message, "COMMIT", "reuse message from specified commit"),
+	OPT_BOOLEAN(0, "reset-author", &renew_authorship, "reset timestamp and authorship to committer"),
 	OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
 	OPT_FILENAME('t', "template", &template_file, "use specified template file"),
 	OPT_BOOLEAN('e', "edit", &edit_flag, "force edit of commit"),

@@ -381,7 +382,7 @@ static void determine_author_info(void)
 	email = getenv("GIT_AUTHOR_EMAIL");
 	date = getenv("GIT_AUTHOR_DATE");
 
-	if (use_message) {
+	if (use_message && !renew_authorship) {
 		const char *a, *lb, *rb, *eol;
 
 		a = strstr(use_message_buffer, "\nauthor ");

@@ -780,6 +781,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		use_message = edit_message;
 	if (amend && !use_message)
 		use_message = "HEAD";
+	if (!use_message && renew_authorship)
+		die("Option --reset-author is used only with -C/-c/--amend.");
 	if (use_message) {
 		unsigned char sha1[20];
 		static char utf8[] = "UTF-8";

diff --git a/t/t7509-commit.sh b/t/t7509-commit.sh
new file mode 100755
index 0000000..1c27de7
--- /dev/null
+++ b/t/t7509-commit.sh

@@ -0,0 +1,123 @@
+#!/bin/sh
+#
+# Copyright (c) 2009 Erick Mattos
+#
+
+test_description='git commit
+
+Tests for --reset-author option on a commit.'
+
+. ./test-lib.sh
+
+TEST_FILE=foo
+
+author_id () {
+	git cat-file -p "$1" | \
+	grep "^author" | \
+	sed -e "s/author //" -e "s/>.*/>/"
+}
+
+author_timestamp () {
+	git cat-file -p "$1" | \
+	grep "^author" | \
+	sed "s/.*> //"
+}
+
+message_body () {
+	git cat-file commit "$1" | \
+	sed -e '1,/^$/d'
+}
+
+initiate_test () {
+	test_tick
+	echo "initial" >> "$TEST_FILE"
+	git add "$TEST_FILE"
+	git commit -m "Initial Commit" --author "Frigate <flying@over.world>"
+	test_tick
+}
+
+make_files () {
+	author_id "$1" > "aid$2"
+	author_timestamp "$1" > "atime$2"
+	message_body "$1" > "message$2"
+}
+
+get_committer_id () {
+	echo "$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>" > aid1
+}
+
+test_expect_success '-C without --reset-author uses the author from the old commit' '
+	initiate_test &&
+	echo "Test 1" >> "$TEST_FILE" &&
+	git add "$TEST_FILE" &&
+	git commit -C HEAD &&
+	make_files HEAD^ 1 &&
+	make_files HEAD 2 &&
+	test_cmp aid1 aid2 &&
+	test_cmp atime1 atime2 &&
+	test_cmp message1 message2
+'
+
+test_expect_success '-C with --reset-author makes me the author' '
+	test_tick &&
+	echo "Test 2" >> "$TEST_FILE" &&
+	git add "$TEST_FILE" &&
+	git commit -C HEAD^ --reset-author &&
+	make_files HEAD^ 1 &&
+	make_files HEAD 2 &&
+	get_committer_id &&
+	test_cmp aid1 aid2 &&
+	test_must_fail cmp atime1 atime2 &&
+	test_cmp message1 message2
+'
+
+test_expect_success '-c without --reset-author uses the author from the old commit' '
+	initiate_test &&
+	echo "Test 3" >> "$TEST_FILE" &&
+	git add "$TEST_FILE" &&
+	EDITOR=: VISUAL=: git commit -c HEAD &&
+	make_files HEAD^ 1 &&
+	make_files HEAD 2 &&
+	test_cmp aid1 aid2 &&
+	test_cmp atime1 atime2 &&
+	test_cmp message1 message2
+'
+
+test_expect_success '-c with --reset-author makes me the author' '
+	test_tick &&
+	echo "Test 4" >> "$TEST_FILE" &&
+	git add "$TEST_FILE" &&
+	EDITOR=: VISUAL=: git commit -c HEAD^ --reset-author &&
+	make_files HEAD^ 1 &&
+	make_files HEAD 2 &&
+	get_committer_id &&
+	test_cmp aid1 aid2 &&
+	test_must_fail cmp atime1 atime2 &&
+	test_cmp message1 message2
+'
+
+test_expect_success '--amend without --reset-author uses the author from the old commit' '
+	initiate_test &&
+	make_files HEAD 2 &&
+	echo "Test 5" >> "$TEST_FILE" &&
+	git add "$TEST_FILE" &&
+	git commit -m "--amend test" --amend &&
+	make_files HEAD 1 &&
+	test_cmp aid1 aid2 &&
+	test_cmp atime1 atime2 &&
+	test_must_fail cmp message1 message2
+'
+
+test_expect_success '--amend with --reset-author makes me the author' '
+	test_tick &&
+	echo "Test 6" >> "$TEST_FILE" &&
+	git add "$TEST_FILE" &&
+	git commit -m "Changed" --amend --reset-author &&
+	make_files HEAD 2 &&
+	get_committer_id &&
+	test_cmp aid1 aid2 &&
+	test_must_fail cmp atime1 atime2 &&
+	test_must_fail cmp message1 message2
+'
+
+test_done

-- 
1.6.5.2.144.g27f8d.dirty

Erick Mattos [off-list ref] writes:

Cutting --author away would make impossible for someone to force a new author
with a new timestamp in case he is templating.  As an example he can be using
the --author because he is doing a change in a computer not his own or
something alike.

Sorry, but I cannot help feeling a bit frustrated and mildly irritated.

I had an impression that we have already established that setting the
author with --author="Somebody Else <s@b.e>" and committing with the
current time does not make much sense from the workflow point of view long
time ago in this thread.

The mail transport might have mangled the name, and when using --amend (or
read-tree followed by commit -c), it is handy to fix the mangled name by
using --author, but in such a case you would actively want to keep the
timestamp obtained from the e-mail via either --amend or -c.

But allowing this combination, even though it might not make much sense,
is just giving extra length to the rope, so it may not be such a big deal.

I didn't feel motivated enough to read the whole thing while other patches
are in my inbox, so I instead ran diff between the previous one (without
my suggestion today) and this round.

I see that you fixed a lot of grammar in the log message of my earlier
suggestion, all of which looked very good.  Also you added a check in the
program to make sure that --renew is given only when -C/-c/--amend is
given, which is also good.  Neither of our set of tests checks this
condition, though.  IOW, we would need to add something like this at the
end of my version (adjust to --reset-author for your version):

    test_expect_success '--mine should be rejected without -c/-C/--amend' '
            git checkout Initial &&
            echo "Test 7" >>foo &&
            test_tick &&
            test_must_fail git commit -a --mine -m done
    '

I am not sure why you insist to use your version of test script and keep
changing it, though.  It looks a lot worse even only after reviewing its
early part.

 - author_id runs an extra grep that is unnecessary.  The separation of
   _id and _timestamp are unnecessary if you checked against an expected
   author ident and timestamp as a single string, i.e.

   FRIGATE='Frigate [off-list ref]' ;# do this only once at the beginning
   ...
   git commit -C HEAD --reset-author --author="$FRIGATE" &&
   echo "author $FRIGATE $GIT_AUTHOR_TIME" >expect &&
   author_header HEAD >actual &&
   test_cmp expect actual

   This becomes irrelevant if we don't support mixing --renew and
   --author, of course.

 - message_body() now has a backslash whose sole purpose is to be an
   eyesore.

 - initiate_test() does not string the commands together with &&

I might change my mind after I take a break, review others' patches, and
spend some time on my own hacking on other topics before revisiting this
patch, but at this point I find that reviewing newer rounds of this series
has rather quickly diminishing value, and more time is being spent on
teaching shell scripting to you rather than on polishing the end result.

Sorry, but I cannot help feeling a bit frustrated and mildly irritated.
Time to take a break and attend other topics for a change.

2009/11/4 Junio C Hamano [off-list ref]:

Erick Mattos [off-list ref] writes:

quoted

Cutting --author away would make impossible for someone to force a new author
with a new timestamp in case he is templating.  As an example he can be using
the --author because he is doing a change in a computer not his own or
something alike.

Sorry, but I cannot help feeling a bit frustrated and mildly irritated.

I had an impression that we have already established that setting the
author with --author="Somebody Else <s@b.e>" and committing with the
current time does not make much sense from the workflow point of view long
time ago in this thread.

The mail transport might have mangled the name, and when using --amend (or
read-tree followed by commit -c), it is handy to fix the mangled name by
using --author, but in such a case you would actively want to keep the
timestamp obtained from the e-mail via either --amend or -c.

But allowing this combination, even though it might not make much sense,
is just giving extra length to the rope, so it may not be such a big deal.

I don't see a reason to be hassled by a suggestion, made because I
didn't really was confident you got it right from me from the previous
email flood.  No big deal.

I didn't feel motivated enough to read the whole thing while other patches
are in my inbox, so I instead ran diff between the previous one (without
my suggestion today) and this round.

I really can't imagine all the work you have.  Probably very hard.

As we were doing before, you were saying what was wrong to you and I
was fixing it to your
demands.  So I imagine that you are doing the diffs over my versions.

I haven't got a change in that way of working.

I see that you fixed a lot of grammar in the log message of my earlier
suggestion, all of which looked very good.  Also you added a check in the
program to make sure that --renew is given only when -C/-c/--amend is
given, which is also good.  Neither of our set of tests checks this
condition, though.  IOW, we would need to add something like this at the
end of my version (adjust to --reset-author for your version):

   test_expect_success '--mine should be rejected without -c/-C/--amend' '
           git checkout Initial &&
           echo "Test 7" >>foo &&
           test_tick &&
           test_must_fail git commit -a --mine -m done
   '

I am not sure why you insist to use your version of test script and keep
changing it, though.  It looks a lot worse even only after reviewing its
early part.

As I told you before I thought you were wanting me to do it.  I didn't
get a change about me working under your supervision as the coder...

I know anyone in this list is able to code those or any other change.
It is just about who is available to work at anything in particular.

 - author_id runs an extra grep that is unnecessary.  The separation of
  _id and _timestamp are unnecessary if you checked against an expected
  author ident and timestamp as a single string, i.e.

author_id or author_timestamp could be changed independently and a
single string would find it corrected in any case.  The new option
ought to change both as expected in the algorithm.

  FRIGATE='Frigate [off-list ref]' ;# do this only once at the beginning
  ...
  git commit -C HEAD --reset-author --author="$FRIGATE" &&
  echo "author $FRIGATE $GIT_AUTHOR_TIME" >expect &&
  author_header HEAD >actual &&
  test_cmp expect actual

If you make my script fail in any of the checks then you are going to
have "trash..." folder holding the full message log history and the
file foo with each step recorded on it with the "initials" separating
the -C, -c and --amend.  This way you can also check the differences
in the author log timestamp among cited options because the "initials"
make a barrier in between.  I made it purposely to become easier to
audit.

  This becomes irrelevant if we don't support mixing --renew and
  --author, of course.

It won't be supported.

 - message_body() now has a backslash whose sole purpose is to be an
  eyesore.

No backslash then.

 - initiate_test() does not string the commands together with &&

It is not something difficult to add.

I might change my mind after I take a break, review others' patches, and
spend some time on my own hacking on other topics before revisiting this
patch, but at this point I find that reviewing newer rounds of this series
has rather quickly diminishing value, and more time is being spent on
teaching shell scripting to you rather than on polishing the end result.

Now you are being impolite and then I am not saying anything back.

Sorry, but I cannot help feeling a bit frustrated and mildly irritated.
Time to take a break and attend other topics for a change.

Again:  I don't see a reason to be irritated by a suggestion...

Sorry fellow.  I just tried to help.  Let me know if you want my work anytime.

And sorry for any communication problem we had, taking in account that
I am not a native english speaker.

Best regards.

Quoting Junio C Hamano [off-list ref]

I had an impression that we have already established that setting the
author with --author="Somebody Else <s@b.e>" and committing with the
current time does not make much sense from the workflow point of view long
time ago in this thread.
<snip>
But allowing this combination, even though it might not make much sense,
is just giving extra length to the rope, so it may not be such a big deal.

It may be wise to forbid a combination of options if it 
encourages mistakes or a wrong workflow, but I don't think 
using --author and --reset-author with 'git commit --amend' 
is such a case.

Imagine somebody other than you (eg. me) were the maintainer, 
and a message by Szeder was sent with a good commit log message.

 http://article.gmane.org/gmane.comp.version-control.git/132029

Then you sent a replacement patch that solves the same problem 
in a more elegant way, but without anything that is usable as the 
commit log message.

 http://article.gmane.org/gmane.comp.version-control.git/132041

If I were the maintainer, I would find it very convenient if I can 
work like this:

 % git am -s 132029   --- first I apply Szeder's version

Then I see your message. Replace the code change but use Szeder's
log message.

 % git reset --hard HEAD^
 % git am 132041   --- your version with no usable log message
 % git commit --amend -s -c @{2} --author='Junio C Hamano <...>'

Sorry, but I cannot help feeling a bit frustrated and mildly irritated.

Don't try to be perfect and feel stressed out, and please take 
a good rest.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/