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

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

[Erick Mattos]

When we use one of the options above we are normally trying to do mainly
two things: using the source as a template or recreating a commit with
corrections.

When they are used, the authorship and timestamp recorded in the newly
created commit are always taken from the original commit.  And they
should not when we are using it as a template 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 <erick.mattos@gmail.com>
---
 Documentation/git-commit.txt |    7 +++-
 builtin-commit.c             |    9 +++-
 t/t7509-commit.sh            |   94 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 106 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..7290242
--- /dev/null
+++ b/t/t7509-commit.sh
@@ -0,0 +1,94 @@
+#!/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
+
+test_expect_success '-C without --reset-author uses the author from the old commit' '
+	echo "initial" > "$TEST_FILE" &&
+	git add "$TEST_FILE" &&
+	git commit -m "Initial Commit" --author "Frigate <flying@over.world>" &&
+	test_tick &&
+	echo "Test 1" >> "$TEST_FILE" &&
+	git add "$TEST_FILE" &&
+	git commit -C HEAD &&
+	git cat-file -p HEAD^ | sed -e "/^parent/d" -e "/^tree/d" \
+		-e "/^committer/d" > commit_1 &&
+	git cat-file -p HEAD | sed -e "/^parent/d" -e "/^tree/d" \
+		-e "/^committer/d" > commit_2 &&
+	test_cmp commit_1 commit_2
+'
+
+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 &&
+	git cat-file -p HEAD^ | grep "^author" > commit_1 &&
+	git cat-file -p HEAD | grep "^author" > commit_2 &&
+	test "$(cat commit_1 | sed "s/.*> //")" !=\
+		"$(cat commit_2 | sed "s/.*> //")" &&
+	test "$(cat commit_2 | sed -e "s/author //" -e "s/>.*/>/")" =\
+		"$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>"
+'
+
+test_expect_success '-c without --reset-author uses the author from the old commit' '
+	echo "Initial" > "$TEST_FILE" &&
+	git add "$TEST_FILE" &&
+	git commit -m "Initial Commit" --author "Frigate <flying@over.world>" &&
+	test_tick &&
+	echo "Test 3" >> "$TEST_FILE" &&
+	git add "$TEST_FILE" &&
+	EDITOR=: VISUAL=: git commit -c HEAD
+	git cat-file -p HEAD^ | grep "^author" > commit_1 &&
+	git cat-file -p HEAD | grep "^author" > commit_2 &&
+	test_cmp commit_1 commit_2
+'
+
+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
+	git cat-file -p HEAD^ | grep "^author" > commit_1 &&
+	git cat-file -p HEAD | grep "^author" > commit_2 &&
+	test "$(cat commit_1 | sed "s/.*> //")" !=\
+		"$(cat commit_2 | sed "s/.*> //")" &&
+	test "$(cat commit_2 | sed -e "s/author //" -e "s/>.*/>/")" =\
+		"$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>"
+'
+
+test_expect_success '--amend without --reset-author uses the author from the old commit' '
+	echo "Initial" > "$TEST_FILE" &&
+	git add "$TEST_FILE" &&
+	git commit -m "Initial Commit" --author "Frigate <flying@over.world>" &&
+	echo "Test 5" >> "$TEST_FILE" &&
+	git add "$TEST_FILE" &&
+	git commit -m "--amend test" &&
+	git cat-file -p HEAD | grep "^author" > commit_1 &&
+	test_tick &&
+	git commit -m "Changed" --amend &&
+	git cat-file -p HEAD | grep "^author" > commit_2 &&
+	test_cmp commit_1 commit_2
+'
+
+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 again" --amend --reset-author &&
+	git cat-file -p HEAD | grep "^author" > commit_1 &&
+	test "$(cat commit_1 | sed "s/.*> //")" !=\
+		"$(cat commit_2 | sed "s/.*> //")" &&
+	test "$(cat commit_2 | sed -e "s/author //" -e "s/>.*/>/")" =\
+		"$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>"
+'
+
+test_done
-- 
1.6.5.2.144.g1babf

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

[Nanako Shiraishi]

Quoting Erick Mattos <erick.mattos@gmail.com>

> When we use one of the options above we are normally trying to do mainly
> two things: using the source as a template or recreating a commit with
> corrections.
>
> When they are used, the authorship and timestamp recorded in the newly
> created commit are always taken from the original commit.  And they
> should not when we are using it as a template 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 <erick.mattos@gmail.com>
> ---

If you are sending an update to a previous patch (I am comparing
this patch with the "show by example" patch Junio sent on 11/02 
http://article.gmane.org/gmane.comp.version-control.git/131893), 
it is a common courtesy to summarize what you changed relative 
to the base version after the three dashes line, so that people 
will know which part can be skipped while reviewing your patch.

I was originally puzzled by Junio's "mine" because I thought 
"what does taking over the authorship have to do with digging 
diamonds out of earth?". "reset-author" makes it very clear,
and I think it is a much better name for the option.

Your first paragraph mentions "one of the options", but subject 
mentions four (-c, -C, --amend and --reset-author) and you want 
only the first three of them.

The second paragraph of gmane/131893 says "borrow the commit log 
message" instead of repeating "a template". It has the effect of 
clarifying that only the message part is borrowed but not the 
authorship information.

I think gmane/131893 has much better description than your first 
two paragraphs for the these reasons.

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

I have to say that the test script is much worse than what 
gmane/131893 had.

The old test made sure that -C copied the message, with or 
without the --mine option. But this test only checks the 
author line (and it doesn't even make sure that "^author" 
matches only in the header). The messages are unchecked, 
and it will let a bug when someone breaks --reset-author 
logic in the future in such a way that it corrupts the 
message by mistake go unnoticed.

Also the old test was much more readable because it used 
shell functions to avoid repeating cat-file and pipe to sed 
script. It also tagged the initial commit, which had a nice 
effect that a failure in any intermediate test will not change 
which earlier commit is reused (eg. yours say "-C HEAD^" but 
old test said "-C Initial").

It looks silly to create an "Initial Commit" in the middle 
of history, too (^_^).

I think it is much better to replace "--mine" in gmane/131893 
with "--reset-author" and make no other change to the test.

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

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

[Erick Mattos]

2009/11/3 Nanako Shiraishi <nanako3@lavabit.com>:
> If you are sending an update to a previous patch (I am comparing
> this patch with the "show by example" patch Junio sent on 11/02
> http://article.gmane.org/gmane.comp.version-control.git/131893),
> it is a common courtesy to summarize what you changed relative
> to the base version after the three dashes line, so that people
> will know which part can be skipped while reviewing your patch.

I got your point.  I will try to improve that.

I have been talking to Junio during the weekend and with a lot of
emails sent to each other.  It happens that when he sent gmane/131893
(I had to find out what that code meant because I was using
[marc.info]! :-) ) I had already sent another patch with the
suggestions he made in a previous email.
So his message was late.  While I was waiting for his acknowledgment I
started thinking he could be lost on those e-mails so I sent the one
you are replying to make it the last on the queue.

> I have to say that the test script is much worse than what
> gmane/131893 had.
>
> The old test made sure that -C copied the message, with or
> without the --mine option. But this test only checks the
> author line (and it doesn't even make sure that "^author"
> matches only in the header). The messages are unchecked,
> and it will let a bug when someone breaks --reset-author
> logic in the future in such a way that it corrupts the
> message by mistake go unnoticed.

I think you misunderstood something here:
* On his patch (which he sent before seeing mine), while testing -C,
on first check, he is checking author_header only.
* While testing -C on mine I compare both messages without "parent",
"tree" and "committer".  Whole!

After check one I did concentrate only on author data.  But on mine I
separate the tests between timestamp and author (name and email),
making sure the author was set to the actual committer and that the
timestamp was behaving as expected.

I am not saying mine is better.  What I am saying is that I expected
him to notice and change/improve THIS patch.  Not the other.

The new option only touches on getting new author or copying the
original so that is why I made the first check in whole and the others
only by author.  If people think that this operation is so uncertain,
then everything should be compared: parent, author and message on all
tests.

It is not a problem for me adding more code to the test even if I
consider it unnecessary.  I am doing this only to give a pay-back for
all the good service this free software gave to me so I am very
patient to all demands.  I will be letting this effort go only at the
real end.  No matter how long it takes.

> Also the old test was much more readable because it used
> shell functions to avoid repeating cat-file and pipe to sed
> script. It also tagged the initial commit, which had a nice
> effect that a failure in any intermediate test will not change
> which earlier commit is reused (eg. yours say "-C HEAD^" but
> old test said "-C Initial").

I am so used to scripts that I really haven't thought it difficult to
read but I can do some cosmetic too if it is important.  As I said
early, I was waiting for Junio's jugdement over my later patch.

> It looks silly to create an "Initial Commit" in the middle
> of history, too (^_^).

This is something more laborious but which I thought was important to
let murphy's law act on a real case.  We never do an amend on an
initial commit so I only did the tests on a later one.

> I think it is much better to replace "--mine" in gmane/131893
> with "--reset-author" and make no other change to the test.

Let's see Junio's opinion...  We have changed names a lot since start.  ;-)

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

Thank you very much for all your comments.  I really appreciate about
being noticed by you people.  It is nice for a newcomer!

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

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> I think it is much better to replace "--mine" in gmane/131893 
> with "--reset-author" and make no other change to the test.

Heh, I would not claim my tests were perfect, even though I agree with
most of the suggestions and comments, including the parts I did not quote
that are not about the test script.

My first test did not check the message contents, but all the other ones
(except the last one that we expect the command to fail) check output from
both author_header and message_body, so that not just "--mine affects the
authorship" but also "--mine does not mangle the message contents" are
checked.

One thing you did not mention was that I made sure that the command failed
when given both --mine and --author options; I think Erick's last round
fails to test this condition.

    Side note.  When writing tests for their shiny new toy, people often
    forget to test "the other side".

    It is just human nature.  It is more fun to demonstrate what your new
    feature does, than making sure that the new feature does not kick in
    when it is not supposed to, nor it does not change what it is not
    supposed to change.

    Negative tests are not particularly hard to write.  The harder part is
    to force the habit of writing them on yourself.  Right after designing
    and implementing a new feature, the list of things it is supposed to
    do and when it is supposed to kick in are pretty clear in your mind,
    and that is what makes writing positive tests psychologically a lot
    easier.

    On the other hand, it takes concious effort to list what it is _not_
    supposed to do or when it is _not_ supposed to kick in.  That is why
    people often fail to write negative tests.

I think in addition to the obvious "s/--mine/--reset-author/g"
replacements, we would need this patch on top of mine.

 t/t7509-commit.sh |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/t/t7509-commit.sh b/t/t7509-commit.sh
index ec13cea..1dad228 100755
--- a/t/t7509-commit.sh
+++ b/t/t7509-commit.sh
@@ -28,6 +28,10 @@ test_expect_success '-C option copies authorship and message' '
 	git commit -a -C Initial &&
 	author_header Initial >expect &&
 	author_header HEAD >actual &&
+	test_cmp expect actual &&
+
+	message_body Initial >expect &&
+	message_body HEAD >actual &&
 	test_cmp expect actual
 '
 

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

[Junio C Hamano]

Erick Mattos <erick.mattos@gmail.com> writes:

> ... I had already sent another patch with the
> suggestions he made in a previous email.

That happens in real life with people working in different timezones.

> The new option only touches on getting new author or copying the
> original so that is why I made the first check in whole and the others
> only by author.  If people think that this operation is so uncertain,
> then everything should be compared: parent, author and message on all
> tests.

You probably have misunderstood why we write tests; it is not about making
sure _your_ implementation is Ok.  If that were the case, using knowledge
of implementation details to short-circuit the tests would perfectly be
acceptable.

We write tests so that long after you get bored and stop visiting the git
project mailing-list, if somebody _else_ changes the program and its
behaviour gets changed in a way _you_ did not expect, such a mistake can
be caught, even if you are not monitoring the mailing list to actively
catch such a bad change to go into the system.  So we prefer to test both
sides of the coin without saying "this option only affects this codepath
(currently) so it never can break this part, it is not worth checking this
and that (right now)" when it is not too much trouble.  It is a win in the
long run.

In any case, I like --reset-author better than --mine.  I didn't think of
diamond-mine, though ;-)

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

[Erick Mattos]

2009/11/3 Junio C Hamano <gitster@pobox.com>:
> Erick Mattos <erick.mattos@gmail.com> writes:
>
>> ... I had already sent another patch with the
>> suggestions he made in a previous email.
>
> That happens in real life with people working in different timezones.

6 hours between you and me!

>> The new option only touches on getting new author or copying the
>> original so that is why I made the first check in whole and the others
>> only by author.  If people think that this operation is so uncertain,
>> then everything should be compared: parent, author and message on all
>> tests.
>
> You probably have misunderstood why we write tests; it is not about making
> sure _your_ implementation is Ok.  If that were the case, using knowledge
> of implementation details to short-circuit the tests would perfectly be
> acceptable.
>
> We write tests so that long after you get bored and stop visiting the git
> project mailing-list, if somebody _else_ changes the program and its
> behaviour gets changed in a way _you_ did not expect, such a mistake can
> be caught, even if you are not monitoring the mailing list to actively
> catch such a bad change to go into the system.  So we prefer to test both
> sides of the coin without saying "this option only affects this codepath
> (currently) so it never can break this part, it is not worth checking this
> and that (right now)" when it is not too much trouble.  It is a win in the
> long run.

I really did not get the reason before the other guy argued...  :-S

> In any case, I like --reset-author better than --mine.  I didn't think of
> diamond-mine, though ;-)
>

So that's it!  Diamond...  me neither!  :-D

I am going to send you another patch in a few minutes.  I hope this
time will be almost there.

Regards