threads / patch / 60224

patcht3404-rebase-interactive.sh: fix name of a rewording test

Subject: [PATCH] t3404-rebase-interactive.sh: fix name of a rewording test

## tl;dr

7 messages between Sep 11, 2023 and Sep 13, 2023. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Oswald Buddenhagen· Sep 11, 2023, 12:21 UTC · lore

The given test name made no sense to me at all; it seems to be a concatenation of two unrelated things. This was introduced by commit 0c164ae7a ("rebase -i: add another reword test", 20-08-20).

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

--- actually, i don't understand what the test even does. shouldn't it, to match the description, actually dirty the tree and verify that the operation fails?

Cc: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3404-rebase-interactive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t3404-rebase-interactive.sh +1 −1
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 96a56aafbe..31ee5bc1f6 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -758,7 +758,7 @@ test_expect_success 'reword' '
 	git show HEAD~2 | grep "C changed"
 '
 
-test_expect_success 'no uncommited changes when rewording the todo list is reloaded' '
+test_expect_success 'no uncommitted changes when rewording' '
 	git checkout E &&
 	test_when_finished "git checkout @{-1}" &&
 	(
-- 
2.42.0.419.g70bf8a5751
Junio C Hamano· Sep 11, 2023, 22:36 UTC · re: Oswald Buddenhagen · lore

Re: [PATCH] t3404-rebase-interactive.sh: fix name of a rewording test

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
Show 12 quoted lines
> The given test name made no sense to me at all; it seems to be a
> concatenation of two unrelated things. This was introduced by
> commit 0c164ae7a ("rebase -i: add another reword test", 20-08-20).
>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>
> ---
> actually, i don't understand what the test even does. shouldn't it, to
> match the description, actually dirty the tree and verify that the
> operation fails?
>
> Cc: Phillip Wood <phillip.wood@dunelm.org.uk>

As a typofix the updated text looks more correct, but the above is a very good point, so I'll let Phillip, whose 0c164ae7 (rebase -i: add another reword test, 2021-08-20) introduced this test, react first.

Thanks for spotting and fixing.
Show 17 quoted lines
> ---
>  t/t3404-rebase-interactive.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 96a56aafbe..31ee5bc1f6 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -758,7 +758,7 @@ test_expect_success 'reword' '
>  	git show HEAD~2 | grep "C changed"
>  '
>  
> -test_expect_success 'no uncommited changes when rewording the todo list is reloaded' '
> +test_expect_success 'no uncommitted changes when rewording' '
>  	git checkout E &&
>  	test_when_finished "git checkout @{-1}" &&
>  	(
Phillip Wood· Sep 12, 2023, 10:15 UTC · re: Junio C Hamano · lore

Re: [PATCH] t3404-rebase-interactive.sh: fix name of a rewording test

On 11/09/2023 23:36, Junio C Hamano wrote:
Show 17 quoted lines
> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> 
>> The given test name made no sense to me at all; it seems to be a
>> concatenation of two unrelated things. This was introduced by
>> commit 0c164ae7a ("rebase -i: add another reword test", 20-08-20).
>>
>> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>>
>> ---
>> actually, i don't understand what the test even does. shouldn't it, to
>> match the description, actually dirty the tree and verify that the
>> operation fails?
>>
>> Cc: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> As a typofix the updated text looks more correct, but the above is a > very good point, so I'll let Phillip, whose 0c164ae7 (rebase -i: add
> another reword test, 2021-08-20) introduced this test, react first.

Thanks. There is a typo, but that typo is a missing "and", the title should be

no uncommitted changes when rewording and the todo list is reloaded
The purpose of the test is to ensure that
  (i) There are no uncommited changes when the editor runs. i.e.  we
      commit without running the editor and then reword by amending
      that commit. This ensures that we have the same user experience
      whether or not the commit was fast-forwarded [1].
(ii) That the todo list is re-read after the commit has been
      reworded. This is to allow the user to update the todo list while
      the rebase is paused for editing the commit message.

All of the action happens in the functions called from this test. set_reword_editor() sets up an editor that runs "git diff --exit-code HEAD" to ensure there are no uncommitted changes when it is run and also updates the todo list. Then check_reworded_commits() checks that the commits were reworded and that the todo list was updated.

Best Wishes
Phillip
[1] https://lore.kernel.org/git/20190812175046.GM20404@szeder.dev/
Show 19 quoted lines
> Thanks for spotting and fixing.
> 
>> ---
>>   t/t3404-rebase-interactive.sh | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index 96a56aafbe..31ee5bc1f6 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -758,7 +758,7 @@ test_expect_success 'reword' '
>>   	git show HEAD~2 | grep "C changed"
>>   '
>>   
>> -test_expect_success 'no uncommited changes when rewording the todo list is reloaded' '
>> +test_expect_success 'no uncommitted changes when rewording' '
>>   	git checkout E &&
>>   	test_when_finished "git checkout @{-1}" &&
>>   	(
Oswald Buddenhagen· Sep 12, 2023, 10:42 UTC · re: Phillip Wood · lore

[PATCH v2] t3404-rebase-interactive.sh: fix typos in title of a rewording test

This test was introduced by commit 0c164ae7a ("rebase -i: add another reword test", 2021-08-20). I didn't quite get what it was meant to do, so here's an explanation from Phillip:

The purpose of the test is to ensure that
  (i) There are no uncommited changes when the editor runs. I.e., we
      commit without running the editor and then reword by amending
      that commit. This ensures that we have the same user experience
      whether or not the commit was fast-forwarded [1].
 (ii) That the todo list is re-read after the commit has been reworded.
      This is to allow the user to update the todo list while the rebase
      is paused for editing the commit message.
[1] https://lore.kernel.org/git/20190812175046.GM20404@szeder.dev/
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
v2:
- actually, it's kinda a new patch now
Cc: Phillip Wood <phillip.wood@dunelm.org.uk>
Cc: Junio C Hamano <gitster@pobox.com>
---
 t/t3404-rebase-interactive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t3404-rebase-interactive.sh +1 −1
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 96a56aafbe..bf94eb28b9 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -758,7 +758,7 @@ test_expect_success 'reword' '
 	git show HEAD~2 | grep "C changed"
 '
 
-test_expect_success 'no uncommited changes when rewording the todo list is reloaded' '
+test_expect_success 'no uncommitted changes when rewording and the todo list is reloaded' '
 	git checkout E &&
 	test_when_finished "git checkout @{-1}" &&
 	(
-- 
2.42.0.419.g70bf8a5751
Oswald Buddenhagen· Sep 12, 2023, 11:14 UTC · re: Oswald Buddenhagen · lore

Re: [PATCH v2] t3404-rebase-interactive.sh: fix typos in title of a rewording test

On Tue, Sep 12, 2023 at 12:42:36PM +0200, Oswald Buddenhagen wrote:
Show 5 quoted lines
>so here's an explanation from Phillip:
>
>The purpose of the test is to ensure that
>
>  (i) There are no uncommited changes when the editor runs. I.e., we
			   ^
you may want to add the missing 't' here when applying.🙃
regards
Phillip Wood· Sep 12, 2023, 13:26 UTC · re: Oswald Buddenhagen · lore

Re: [PATCH v2] t3404-rebase-interactive.sh: fix typos in title of a rewording test

On 12/09/2023 11:42, Oswald Buddenhagen wrote:
Show 18 quoted lines
> This test was introduced by commit 0c164ae7a ("rebase -i: add another
> reword test", 2021-08-20). I didn't quite get what it was meant to do,
> so here's an explanation from Phillip:
> 
> The purpose of the test is to ensure that
> 
>    (i) There are no uncommited changes when the editor runs. I.e., we
>        commit without running the editor and then reword by amending
>        that commit. This ensures that we have the same user experience
>        whether or not the commit was fast-forwarded [1].
> 
>   (ii) That the todo list is re-read after the commit has been reworded.
>        This is to allow the user to update the todo list while the rebase
>        is paused for editing the commit message.
> 
> [1] https://lore.kernel.org/git/20190812175046.GM20404@szeder.dev/
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
This looks good to me, Thanks
Phillip
Show 23 quoted lines
> ---
> v2:
> - actually, it's kinda a new patch now
> 
> Cc: Phillip Wood <phillip.wood@dunelm.org.uk>
> Cc: Junio C Hamano <gitster@pobox.com>
> ---
>   t/t3404-rebase-interactive.sh | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 96a56aafbe..bf94eb28b9 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -758,7 +758,7 @@ test_expect_success 'reword' '
>   	git show HEAD~2 | grep "C changed"
>   '
>   
> -test_expect_success 'no uncommited changes when rewording the todo list is reloaded' '
> +test_expect_success 'no uncommitted changes when rewording and the todo list is reloaded' '
>   	git checkout E &&
>   	test_when_finished "git checkout @{-1}" &&
>   	(
Junio C Hamano· Sep 13, 2023, 00:25 UTC · re: Phillip Wood · lore

Re: [PATCH v2] t3404-rebase-interactive.sh: fix typos in title of a rewording test

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 20 quoted lines
> On 12/09/2023 11:42, Oswald Buddenhagen wrote:
>> This test was introduced by commit 0c164ae7a ("rebase -i: add another
>> reword test", 2021-08-20). I didn't quite get what it was meant to do,
>> so here's an explanation from Phillip:
>> The purpose of the test is to ensure that
>>    (i) There are no uncommited changes when the editor runs. I.e.,
>> we
>>        commit without running the editor and then reword by amending
>>        that commit. This ensures that we have the same user experience
>>        whether or not the commit was fast-forwarded [1].
>>   (ii) That the todo list is re-read after the commit has been
>> reworded.
>>        This is to allow the user to update the todo list while the rebase
>>        is paused for editing the commit message.
>> [1] https://lore.kernel.org/git/20190812175046.GM20404@szeder.dev/
>> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>
> This looks good to me, Thanks
>
> Phillip
Thanks, both.  Will queue.

← back to recent threads