threads / patch / 60189

patchsequencer: remove unreachable exit condition in pick_commits()

Subject: [PATCH] sequencer: remove unreachable exit condition in pick_commits()

## tl;dr

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

replies: 4people: 3as markdown or json

Oswald Buddenhagen· Sep 3, 2023, 15:11 UTC · lore

This was introduced by 56dc3ab04 ("sequencer (rebase -i): implement the 'edit' command", 2017-01-02), and was pointless from the get-go, as the command causes an early return.

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

--- this remains valid after phillip's pending series, through it becomes marginally harder to prove (c.f. "sequencer: factor out part of pick_commits()").

Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
Cc: Phillip Wood <phillip.wood123@gmail.com>
---
 sequencer.c | 4 ----
 1 file changed, 4 deletions(-)

Show changes to sequencer.c +0 −4

diff --git a/sequencer.c b/sequencer.c
index a66dcf8ab2..99e9c520ca 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4832,10 +4832,6 @@ static int pick_commits(struct repository *r,
 		struct strbuf head_ref = STRBUF_INIT, buf = STRBUF_INIT;
 		struct stat st;
 
-		/* Stopped in the middle, as planned? */
-		if (todo_list->current < todo_list->nr)
-			return 0;
-
 		if (read_oneliner(&head_ref, rebase_path_head_name(), 0) &&
 				starts_with(head_ref.buf, "refs/")) {
 			const char *msg;

-- 
2.40.0.152.g15d061e6df

Phillip Wood· Sep 12, 2023, 10:18 UTC · re: Oswald Buddenhagen · lore

Re: [PATCH] sequencer: remove unreachable exit condition in pick_commits()

Hi Oswald

On 03/09/2023 16:11, Oswald Buddenhagen wrote:

> This was introduced by 56dc3ab04 ("sequencer (rebase -i): implement the
> 'edit' command", 2017-01-02), and was pointless from the get-go, as the
> command causes an early return.

While I agree this code is unreachable, just because it was unused when it was added does not mean it is unused now. It would be helpful for the commit message to explain that there are no "break" or "goto" statements in the loop body and therefore this code is only reachable when the loop terminates because todo_list->current == todo_list->nr

Best Wishes

Phillip

Show 28 quoted lines

> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> 
> ---
> this remains valid after phillip's pending series, through it becomes
> marginally harder to prove (c.f. "sequencer: factor out part of
> pick_commits()").
> 
> Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
> Cc: Phillip Wood <phillip.wood123@gmail.com>
> ---
>   sequencer.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index a66dcf8ab2..99e9c520ca 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4832,10 +4832,6 @@ static int pick_commits(struct repository *r,
>   		struct strbuf head_ref = STRBUF_INIT, buf = STRBUF_INIT;
>   		struct stat st;
>   
> -		/* Stopped in the middle, as planned? */
> -		if (todo_list->current < todo_list->nr)
> -			return 0;
> -
>   		if (read_oneliner(&head_ref, rebase_path_head_name(), 0) &&
>   				starts_with(head_ref.buf, "refs/")) {
>   			const char *msg;

Oswald Buddenhagen· Sep 12, 2023, 10:55 UTC · re: Phillip Wood · lore

[PATCH v2] sequencer: remove unreachable exit condition in pick_commits()

This was introduced by 56dc3ab04 ("sequencer (rebase -i): implement the 'edit' command", 2017-01-02), and was pointless from the get-go: all early exits from the loop above are returns, so todo_list->current == todo_list->nr is an invariant after the loop.

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

---
v2:
- improved commit message

Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
Cc: Phillip Wood <phillip.wood123@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>
---
 sequencer.c | 4 ----
 1 file changed, 4 deletions(-)

Show changes to sequencer.c +0 −4

diff --git a/sequencer.c b/sequencer.c
index a66dcf8ab2..99e9c520ca 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4832,10 +4832,6 @@ static int pick_commits(struct repository *r,
 		struct strbuf head_ref = STRBUF_INIT, buf = STRBUF_INIT;
 		struct stat st;
 
-		/* Stopped in the middle, as planned? */
-		if (todo_list->current < todo_list->nr)
-			return 0;
-
 		if (read_oneliner(&head_ref, rebase_path_head_name(), 0) &&
 				starts_with(head_ref.buf, "refs/")) {
 			const char *msg;

-- 
2.42.0.419.g70bf8a5751

Phillip Wood· Sep 12, 2023, 13:27 UTC · re: Oswald Buddenhagen · lore

Re: [PATCH v2] sequencer: remove unreachable exit condition in pick_commits()

On 12/09/2023 11:55, Oswald Buddenhagen wrote:

Show 6 quoted lines

> This was introduced by 56dc3ab04 ("sequencer (rebase -i): implement the
> 'edit' command", 2017-01-02), and was pointless from the get-go: all
> early exits from the loop above are returns, so todo_list->current ==
> todo_list->nr is an invariant after the loop.
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

Thanks for updating the commit message, I think it is clearer now

Best Wishes

Phillip

Show 26 quoted lines

> ---
> v2:
> - improved commit message
> 
> Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
> Cc: Phillip Wood <phillip.wood123@gmail.com>
> Cc: Junio C Hamano <gitster@pobox.com>
> ---
>   sequencer.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index a66dcf8ab2..99e9c520ca 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4832,10 +4832,6 @@ static int pick_commits(struct repository *r,
>   		struct strbuf head_ref = STRBUF_INIT, buf = STRBUF_INIT;
>   		struct stat st;
>   
> -		/* Stopped in the middle, as planned? */
> -		if (todo_list->current < todo_list->nr)
> -			return 0;
> -
>   		if (read_oneliner(&head_ref, rebase_path_head_name(), 0) &&
>   				starts_with(head_ref.buf, "refs/")) {
>   			const char *msg;

Junio C Hamano· Sep 13, 2023, 00:32 UTC · re: Phillip Wood · lore

Re: [PATCH v2] sequencer: remove unreachable exit condition in pick_commits()

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 8 quoted lines

> On 12/09/2023 11:55, Oswald Buddenhagen wrote:
>> This was introduced by 56dc3ab04 ("sequencer (rebase -i): implement the
>> 'edit' command", 2017-01-02), and was pointless from the get-go: all
>> early exits from the loop above are returns, so todo_list->current ==
>> todo_list->nr is an invariant after the loop.
>> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>
> Thanks for updating the commit message, I think it is clearer now

Thanks, both.  Queued.