Re: [PATCH v4 2/3] rebase: use test_commit helper in setup

[John Cai <johncai86@gmail.com>]

Hi Phillip,

On 18 Mar 2022, at 7:14, Phillip Wood wrote:

> Hi John
>
> On 17/03/2022 19:53, John Cai via GitGitGadget wrote:
>> From: John Cai <johncai86@gmail.com>
>>
>> To prepare for the next commit that will test rebase with oids instead
>> of branch names, update the rebase setup test to add a couple of tags we
>> can use. This uses the test_commit helper so we can replace some lines
>> that add a commit manually.
>>
>> Setting logAllRefUpdates is not necessary because it's on by default for
>> repositories with a working tree.
>>
>> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
>> Signed-off-by: John Cai <johncai86@gmail.com>
>> ---
>>   t/t3400-rebase.sh | 18 ++++--------------
>>   1 file changed, 4 insertions(+), 14 deletions(-)
>>
>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>> index 5c4073f06d6..2fb3fabe60e 100755
>> --- a/t/t3400-rebase.sh
>> +++ b/t/t3400-rebase.sh
>> @@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address
>>   export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
>>    test_expect_success 'prepare repository with topic branches' '
>> -	git config core.logAllRefUpdates true &&
>> -	echo First >A &&
>> -	git update-index --add A &&
>> -	git commit -m "Add A." &&
>> +	test_commit "Add A." A First First &&
>>   	git checkout -b force-3way &&
>>   	echo Dummy >Y &&
>>   	git update-index --add Y &&
>> @@ -32,17 +29,13 @@ test_expect_success 'prepare repository with topic branches' '
>>   	git mv A D/A &&
>>   	git commit -m "Move A." &&
>>   	git checkout -b my-topic-branch main &&
>> -	echo Second >B &&
>> -	git update-index --add B &&
>> -	git commit -m "Add B." &&
>> +	test_commit "Add B." B Second Second &&
>>   	git checkout -f main &&
>>   	echo Third >>A &&
>>   	git update-index A &&
>>   	git commit -m "Modify A." &&
>>   	git checkout -b side my-topic-branch &&
>
> This version has added some more conversions that are not mentioned it the commit message. If you want to covert the whole setup to use test_commit then that's great but I think you need to do the whole thing and say in the commit message that you're modernizing everything. As it stands a reader is left wondering why some of the setup that is not used in the next commit  has been converted but other bits such as the "Modify A." above are left as is. I think the two possibilities that make sense are (a) convert only what we need for the next commit, or (b) modernize the test and convert everything.

I see your point. Then for the sake of a smaller series for the patch, I'll opt
to only update the parts we need. Then maybe we can have a separate series to
modernize this suite.

I'll re-roll this series with the clarifications to the commit message that
Junio made.

>
> Best Wishes
>
> Phillip

Thanks!
John

>
>> -	echo Side >>C &&
>> -	git add C &&
>> -	git commit -m "Add C" &&
>> +	test_commit --no-tag "Add C" C Side &&
>>   	git checkout -f my-topic-branch &&
>>   	git tag topic
>>   '
>> @@ -119,10 +112,7 @@ test_expect_success 'rebase off of the previous branch using "-"' '
>>   test_expect_success 'rebase a single mode change' '
>>   	git checkout main &&
>>   	git branch -D topic &&
>> -	echo 1 >X &&
>> -	git add X &&
>> -	test_tick &&
>> -	git commit -m prepare &&
>> +	test_commit prepare X 1 &&
>>   	git checkout -b modechange HEAD^ &&
>>   	echo 1 >X &&
>>   	git add X &&