Re: [PATCH v2 23/23] rebase -i: enable options --signoff, --reset-author for pick, reword

[Fabian Ruch <bafain@gmail.com>]

Hi Thomas,

Thomas Rast writes:
> Fabian Ruch <bafain@gmail.com> writes:
>> @@ -634,21 +644,24 @@ do_replay () {
>>  		comment_for_reflog pick
>>  
>>  		mark_action_done
>> -		do_pick $sha1 || die_with_patch $sha1 "Could not apply $sha1... $rest"
>> +		eval do_pick $opts $sha1 \
>> +			|| die_with_patch $sha1 "Could not apply $sha1... $rest"
> 
> You had me a little puzzled at the switch to 'eval' here.  That is
> necessary to match the quoting added in 20/23, not for any change in
> this commit.  This commit is simply the first one to trigger this.

This patch switches to 'eval' here because it is the first time
'opts' occurs. However, I agree that it might be confusing that
'opts' wasn't already added to the 'do_pick' lines by 20/23. By
"trigger" you mean that this commit is the first to actually fill
'opts' with contents? I will move these changes to 20/23 then.

> Also, are you sure $sha1 does not require quoting through an eval?

At least if we can assume that it is really a SHA-1 object name. As
such it does not contain characters interpreted by the shell, like
backslashes, quotes or whitespace.

> Please add tests to this patch.

The ones I had in mind are attached below the scissors line. The
current reroll fails the authorship checks and the 'git rebase
--continue' test cases. As the necessary changes would obfuscate this
sub-thread, they will be included in the next reroll.

   Fabian

-- >8 --
diff --git a/t/t3427-rebase-line-options.sh b/t/t3427-rebase-line-options.sh
index 5881162..a5a9e66 100755
--- a/t/t3427-rebase-line-options.sh
+++ b/t/t3427-rebase-line-options.sh
@@ -6,10 +6,32 @@ test_description='git rebase -i with line options'
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
+commit_message () {
+	git cat-file commit "$1" | sed '1,/^$/d'
+}
+
+commit_authorship () {
+	git cat-file commit "$1" | sed -n '/^$/q;/^author /p'
+}
+
+authorship () {
+	echo "author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE"
+}
+
+test_diff_file () {
+	if cmp "$1" "$2" >/dev/null
+	then
+		echo "'$1' and '$2' are the same"
+		return 1
+	fi
+}
+
 test_expect_success 'Set up repository' '
 	test_commit Initial &&
 	test_commit Commit1 &&
-	test_commit Commit2
+	test_commit Commit2 &&
+	git checkout -b branch Commit1 &&
+	test_commit Commit2_ Commit2.t
 '
 
 test_expect_success 'Unknown option' '
@@ -23,4 +45,137 @@ test_expect_success 'Unknown option' '
 	git rebase --continue
 '
 
+test_msg_author () {
+	set_fake_editor &&
+	FAKE_LINES="1 $1 2" git rebase -i HEAD~2 &&
+	commit_message HEAD >actual.msg &&
+	commit_authorship HEAD >actual.author &&
+	test_cmp expected.msg actual.msg &&
+	test_cmp expected.author actual.author
+}
+
+test_msg_author_misspelled () {
+	set_cat_todo_editor &&
+	test_must_fail git rebase -i HEAD^ >todo &&
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="1 $1-misspelled 2" git rebase -i HEAD~2 &&
+	set_fixed_todo_editor "$(pwd)"/todo &&
+	FAKE_LINES="$1 1" git rebase --edit-todo &&
+	git rebase --continue &&
+	commit_message HEAD >actual.msg &&
+	commit_authorship HEAD >actual.author &&
+	test_cmp expected.msg actual.msg &&
+	test_cmp expected.author actual.author
+}
+
+test_msg_author_conflicted () {
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="$1 1" git rebase -i master &&
+	git checkout --theirs Commit2.t &&
+	git add Commit2.t &&
+	git rebase --continue &&
+	commit_message HEAD >actual.msg &&
+	commit_authorship HEAD >actual.author &&
+	test_cmp expected.msg actual.msg &&
+	test_cmp expected.author actual.author
+}
+
+test_expect_success 'Misspelled pick --signoff' '
+	git checkout -b misspelled-pick--signoff master &&
+	cat >expected.msg <<-EOF &&
+	$(commit_message HEAD)
+
+	Signed-off-by: C O Mitter <committer@example.com>
+	EOF
+	commit_authorship HEAD >expected.author &&
+	test_msg_author_misspelled pick_--signoff
+'
+
+test_expect_success 'Conflicted pick --signoff' '
+	git checkout -b conflicted-pick--signoff branch &&
+	cat >expected.msg <<-EOF &&
+	$(commit_message HEAD)
+
+	Signed-off-by: C O Mitter <committer@example.com>
+	EOF
+	commit_authorship HEAD >expected.author &&
+	test_msg_author_conflicted pick_--signoff
+'
+
+test_expect_success 'pick --signoff' '
+	git checkout -b pick--signoff master &&
+	cat >expected.msg <<-EOF &&
+	$(commit_message HEAD)
+
+	Signed-off-by: C O Mitter <committer@example.com>
+	EOF
+	commit_authorship HEAD >expected.author &&
+	test_msg_author pick_--signoff
+'
+
+test_expect_success 'reword --signoff' '
+	git checkout -b reword--signoff master &&
+	cat >expected.msg <<-EOF &&
+	$(commit_message HEAD)
+
+	Signed-off-by: C O Mitter <committer@example.com>
+	EOF
+	commit_authorship HEAD >expected.author &&
+	test_msg_author reword_--signoff
+'
+
+test_expect_success 'Misspelled pick --reset-author' '
+	git checkout -b misspelled-pick--reset-author master &&
+	commit_message HEAD >expected.msg &&
+	test_tick &&
+	authorship >expected.author &&
+	commit_authorship HEAD >original.author &&
+	test_diff_file expected.author original.author &&
+	test_msg_author_misspelled pick_--reset-author
+'
+
+test_expect_success 'Conflicted pick --reset-author' '
+	git checkout -b conflicted-pick--reset-author branch &&
+	commit_message HEAD >expected.msg &&
+	test_tick &&
+	authorship >expected.author &&
+	commit_authorship HEAD >original.author &&
+	test_diff_file expected.author original.author &&
+	test_msg_author_conflicted pick_--reset-author
+'
+
+test_expect_success 'pick --reset-author' '
+	git checkout -b pick--reset-author master &&
+	commit_message HEAD >expected.msg &&
+	test_tick &&
+	authorship >expected.author &&
+	commit_authorship HEAD >original.author &&
+	test_diff_file expected.author original.author &&
+	test_msg_author pick_--reset-author
+'
+
+test_expect_success 'pick --reset-author --signoff' '
+	git checkout -b pick--reset-author--signoff master &&
+	cat >expected.msg <<-EOF &&
+	$(commit_message HEAD)
+
+	Signed-off-by: C O Mitter <committer@example.com>
+	EOF
+	test_tick &&
+	authorship >expected.author &&
+	commit_authorship HEAD >original.author &&
+	test_diff_file expected.author original.author &&
+	test_msg_author pick_--reset-author_--signoff
+'
+
+test_expect_success 'reword --reset-author' '
+	git checkout -b reword--reset-author master &&
+	commit_message HEAD >expected.msg &&
+	test_tick &&
+	authorship >expected.author &&
+	commit_authorship HEAD >original.author &&
+	test_diff_file expected.author original.author &&
+	test_msg_author reword_--reset-author
+'
+
 test_done