Re: [PATCH] commit -c/-C/--amend: acquire authorship and restamp time with --claim

[Junio C Hamano <gitster@pobox.com>]

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

> 2009/11/1 Junio C Hamano <gitster@pobox.com>:
>> Erick Mattos <erick.mattos@gmail.com> writes:
>>
>>>>    % git commit --claim --author='Erick Mattos <eric@mattos>' -C HEAD
>>>>
>>>> Should you detect an error?  Does your code do so?  Do you have a test
>>>> that catches this error?
>>>
>>> It works as intended.  Both together.
>>
>> That does not make any sense.  If you are saying this is yours and it is
>> his at the same time, there can be no sane way to work "as intended", no?.
>
> I am adding a new option not changing the option --author already in
> git.  So it does work together.

Somebody who says "this commit is mine, and its author is this other
person" is not making any sense.  The resulting commit can either have
that person (i.e. the committer) as the author, which is what the "claim"
option means, or it can have the person named with --author as the author,
but both cannot be true at the same time.

When you introduce a new option, sometimes it cannot sanely be used with
an existing option.  In such a case, two options (the new one and the
existing one) are called mutually exclusive.  And you add some code to
catch an user error to use them together.

>>>>> +     git commit -c HEAD <<EOF
>>>>> +     "Changed"
>>>>> +     EOF &&
>>>>
>>>> What editor is reading this "Changed"?
>>>
>>> Nobody cares...  Just a text to change the file.
>>
>> I actually care.  Who uses that Changed string, and where does it end up
>> with?  At the end of the log message?  At the beginning?  What "file"?
>
> I didn't get it.  -c option does not accept -m option and starts an
> editor to change the message.  The text "Changed is just a forced
> message.  I can not use an editor in interactive mode in a script...

How are the existing tests that try "commit -c" do this?  I do not think
there is any here-text redirect into "git commit".

It is sometimes easier to show by example than by giving nudging words
that only show direction, so here is a suggested rewrite on top of your
patch.  I am not very happy with the option name "mine" either, but at
least I think this gets the semantics right.

 Documentation/git-commit.txt |   10 ++--
 Makefile                     |    1 +
 builtin-commit.c             |    9 ++-
 t/t7509-commit.sh            |  144 +++++++++++++++++++++++-------------------
 4 files changed, 92 insertions(+), 72 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 01eeb3e..7832720 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>] [--claim]
+	   [(-c | -C) <commit>] [-F <file> | -m <msg>] [--mine]
 	   [--allow-empty] [--no-verify] [-e] [--author=<author>]
 	   [--cleanup=<mode>] [--] [[-i | -o ]<file>...]
 
@@ -61,18 +61,18 @@ OPTIONS
 -C <commit>::
 --reuse-message=<commit>::
 	Take an existing commit object, and reuse the log message
-	and the authorship information when creating the commit.
+	and the authorship information (including the timestamp)
+	when creating the commit.
 
 -c <commit>::
 --reedit-message=<commit>::
 	Like '-C', but with '-c' the editor is invoked, so that
 	the user can further edit the commit message.
 
---claim::
+--mine::
 	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.  Therefore this option
-	sets the use of only the message from the original commit.
+	This also renews the author timestamp.
 
 -F <file>::
 --file=<file>::
diff --git a/Makefile b/Makefile
index 15ea32d..a9108b3 100644
--- a/Makefile
+++ b/Makefile
@@ -1944,3 +1944,4 @@ coverage-report:
 	grep '^function.*called 0 ' *.c.gcov \
 		| sed -e 's/\([^:]*\)\.gcov: *function \([^ ]*\) called.*/\1: \2/' \
 		| tee coverage-untested-functions
+
diff --git a/builtin-commit.c b/builtin-commit.c
index 1aeafa6..aa42989 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, claim;
+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
@@ -93,7 +93,7 @@ static struct option builtin_commit_options[] = {
 	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', "reuse-message", &use_message, "COMMIT", "reuse message from specified commit"),
-	OPT_BOOLEAN(0, "claim", &claim, "acquire authorship and restamp time of resulting commit"),
+	OPT_BOOLEAN(0, "mine", &renew_authorship, "the commit is authored by me now (used with -C-c/--amend)"),
 	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"),
@@ -382,7 +382,7 @@ static void determine_author_info(void)
 	email = getenv("GIT_AUTHOR_EMAIL");
 	date = getenv("GIT_AUTHOR_DATE");
 
-	if (use_message && !claim) {
+	if (use_message && !renew_authorship) {
 		const char *a, *lb, *rb, *eol;
 
 		a = strstr(use_message_buffer, "\nauthor ");
@@ -748,6 +748,9 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (force_author && !strchr(force_author, '>'))
 		force_author = find_author_by_nickname(force_author);
 
+	if (force_author && renew_authorship)
+		die("Using both --mine and --author does not make sense");
+
 	if (logfile || message.len || use_message)
 		use_editor = 0;
 	if (edit_flag)
diff --git a/t/t7509-commit.sh b/t/t7509-commit.sh
index 6d9eb26..ec13cea 100755
--- a/t/t7509-commit.sh
+++ b/t/t7509-commit.sh
@@ -3,85 +3,101 @@
 # Copyright (c) 2009 Erick Mattos
 #
 
-test_description='git commit
-
-Tests for --claim option on a commit.'
+test_description='git commit --mine'
 
 . ./test-lib.sh
 
-TEST_FILE="$PWD"/foo
+author_header () {
+	git cat-file commit "$1" |
+	sed -n -e '/^$/q' -e '/^author /p'
+}
+
+message_body () {
+	git cat-file commit "$1" |
+	sed -e '1,/^$/d'
+}
 
-test_expect_success '-C option should be working' '
-	echo "Initial" > "$TEST_FILE" &&
-	git add "$TEST_FILE" &&
+test_expect_success '-C option copies authorship and message' '
+	echo "Initial" >foo &&
+	git add foo &&
+	test_tick &&
 	git commit -m "Initial Commit" --author Frigate\ \<flying@over.world\> &&
-	sleep 1 &&
-	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 &&
-	cmp commit_1 commit_2
+	git tag Initial &&
+	echo "Test 1" >>foo &&
+	test_tick &&
+	git commit -a -C Initial &&
+	author_header Initial >expect &&
+	author_header HEAD >actual &&
+	test_cmp expect actual
 '
 
-test_expect_success '-C option with --claim is working properly' '
-	sleep 1 &&
-	echo "Test 2" >> "$TEST_FILE" &&
-	git add "$TEST_FILE" &&
-	git commit -C HEAD^ --claim &&
-	git cat-file -p HEAD^ | grep '^author' > commit_1 &&
-	git cat-file -p HEAD | grep '^author' > commit_2 &&
-	test_must_fail cmp commit_1 commit_2
+test_expect_success '-C option copies only the message with --mine' '
+	echo "Test 2" >>foo &&
+	test_tick &&
+	git commit -a -C Initial --mine &&
+	echo "author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE" >expect &&
+	author_header HEAD >actual
+	test_cmp expect actual &&
+
+	message_body Initial >expect &&
+	message_body HEAD >actual &&
+	test_cmp expect actual
 '
 
-test_expect_success '-c option should be working' '
-	echo "Initial" > "$TEST_FILE" &&
-	git add "$TEST_FILE" &&
-	git commit -m "Initial Commit" --author Frigate\ \<flying@over.world\> &&
-	sleep 1 &&
-	echo "Test 3" >> "$TEST_FILE" &&
-	git add "$TEST_FILE" &&
-	git commit -c HEAD <<EOF
-	"Changed"
-	EOF &&
-	git cat-file -p HEAD^ | grep '^author' > commit_1 &&
-	git cat-file -p HEAD | grep '^author' > commit_2 &&
-	cmp commit_1 commit_2
+test_expect_success '-c option copies authorship and message' '
+	echo "Test 3" >>foo &&
+	test_tick &&
+	EDITOR=: VISUAL=: git commit -a -c Initial &&
+	author_header Initial >expect &&
+	author_header HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '-c option copies only the message with --mine' '
+	echo "Test 4" >>foo &&
+	test_tick &&
+	EDITOR=: VISUAL=: git commit -a -c Initial --mine &&
+	echo "author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE" >expect &&
+	author_header HEAD >actual &&
+	test_cmp expect actual &&
+
+	message_body Initial >expect &&
+	message_body HEAD >actual &&
+	test_cmp expect actual
 '
 
-test_expect_success '-c option with --claim is working properly' '
-	sleep 1 &&
-	echo "Test 4" >> "$TEST_FILE" &&
-	git add "$TEST_FILE" &&
-	git commit -c HEAD^ --claim <<EOF
-	"Changed again"
-	EOF &&
-	git cat-file -p HEAD^ | grep '^author' > commit_1 &&
-	git cat-file -p HEAD | grep '^author' > commit_2 &&
-	test_must_fail cmp commit_1 commit_2
+test_expect_success '--amend option copies authorship' '
+	git checkout Initial &&
+	echo "Test 5" >>foo &&
+	test_tick &&
+	git commit -a --amend -m "amend test" &&
+	author_header Initial >expect &&
+	author_header HEAD >actual &&
+
+	echo "amend test" >expect &&
+	message_body HEAD >actual &&
+	test_cmp expect actual
 '
 
-test_expect_success '--amend option should be working' '
-	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 &&
-	sleep 1 &&
-	git commit -m "Changed" --amend &&
-	git cat-file -p HEAD | grep '^author' > commit_2 &&
-	cmp commit_1 commit_2
+test_expect_success '--mine makes the commit ours even with --amend option' '
+	git checkout Initial &&
+	echo "Test 6" >>foo &&
+	test_tick &&
+	git commit -a --mine -m "Changed again" --amend &&
+	echo "author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE" >expect &&
+	author_header HEAD >actual &&
+	test_cmp expect actual &&
+
+	echo "Changed again" >expect &&
+	message_body HEAD >actual &&
+	test_cmp expect actual
 '
 
-test_expect_success '--amend option with --claim is working properly' '
-	sleep 1 &&
-	echo "Test 6" >> "$TEST_FILE" &&
-	git add "$TEST_FILE" &&
-	git commit -m "Changed again" --amend --claim &&
-	git cat-file -p HEAD | grep '^author' > commit_1 &&
-	test_must_fail cmp commit_1 commit_2
+test_expect_success '--mine and --author are mutually exclusive' '
+	git checkout Initial &&
+	echo "Test 7" >>foo &&
+	test_tick &&
+	test_must_fail git commit -a --mine --author="Xyzzy <frotz@nitfol.xz>"
 '
 
 test_done