threads / patch / 59449

patchrebase: clarify conditionals in todo_list_to_strbuf()

Subject: [PATCH] rebase: clarify conditionals in todo_list_to_strbuf()

## tl;dr

14 messages between Mar 23, 2023 and Aug 11, 2023. Diffs are folded; open one to read it.

replies: 13people: 6as markdown or json

Oswald Buddenhagen· Mar 23, 2023, 16:22 UTC · lore

Make it obvious that the two conditional branches are mutually exclusive.

As a drive-by, remove a pair of unnecessary braces.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sequencer.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
Show changes to sequencer.c +2 −5
diff --git a/sequencer.c b/sequencer.c
index 3be23d7ca2..9169876441 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5868,12 +5868,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
 			if (item->command == TODO_FIXUP) {
 				if (item->flags & TODO_EDIT_FIXUP_MSG)
 					strbuf_addstr(buf, " -c");
-				else if (item->flags & TODO_REPLACE_FIXUP_MSG) {
+				else if (item->flags & TODO_REPLACE_FIXUP_MSG)
 					strbuf_addstr(buf, " -C");
-				}
-			}
-
-			if (item->command == TODO_MERGE) {
+			} else if (item->command == TODO_MERGE) {
 				if (item->flags & TODO_EDIT_MERGE_MSG)
 					strbuf_addstr(buf, " -c");
 				else

-- 
2.40.0.152.g15d061e6df
Taylor Blau· Mar 23, 2023, 20:32 UTC · re: Oswald Buddenhagen · lore

Re: [PATCH] rebase: clarify conditionals in todo_list_to_strbuf()

On Thu, Mar 23, 2023 at 05:22:35PM +0100, Oswald Buddenhagen wrote:
Show 26 quoted lines
> Make it obvious that the two conditional branches are mutually
> exclusive.
>
> As a drive-by, remove a pair of unnecessary braces.
>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
>  sequencer.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 3be23d7ca2..9169876441 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5868,12 +5868,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
>  			if (item->command == TODO_FIXUP) {
>  				if (item->flags & TODO_EDIT_FIXUP_MSG)
>  					strbuf_addstr(buf, " -c");
> -				else if (item->flags & TODO_REPLACE_FIXUP_MSG) {
> +				else if (item->flags & TODO_REPLACE_FIXUP_MSG)
>  					strbuf_addstr(buf, " -C");
> -				}
> -			}
> -
> -			if (item->command == TODO_MERGE) {
> +			} else if (item->command == TODO_MERGE) {
I dunno. I think seeing adjacent
    if (item->command == TODO_ABC)
and
    if (item->command == TODO_XYZ)

makes it clear that these two are mutually exclusive, since TODO_ABC != TODO_XYZ.

So I don't mind the unnecessary brace cleanup, but I don't think that this adds additional clarity around these two if-statements.

Specifically: why not combine these two with if-statement that proceeds
it? That might look something like:
    if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
        item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {
      ...
    } else if (item->command == TODO_FIXUP) {
      ...
    } else if (item->command == TODO_MERGE) {
      ...
    }
but at that point, you might consider something like:
    switch (item->command) {
    case TODO_EXEC:
    case TODO_LABEL:
    case TODO_RESET:
    case TODO_UPDATE_REF:
      ...
      break;
    case TODO_FIXUP:
      ...
      break;
    case TODO_MERGE:
      ...
      break;
    }

which is arguably clearer, but I have a hard time justifying as worthwhile. TBH, it feels like churn to me, but others may disagree and see it differently.

Thanks, Taylor

Oswald Buddenhagen· Mar 24, 2023, 08:59 UTC · re: Taylor Blau · lore

Re: [PATCH] rebase: clarify conditionals in todo_list_to_strbuf()

On Thu, Mar 23, 2023 at 04:32:41PM -0400, Taylor Blau wrote:
Show 11 quoted lines
>I dunno. I think seeing adjacent
>
>    if (item->command == TODO_ABC)
>
>and
>
>    if (item->command == TODO_XYZ)
>
>makes it clear that these two are mutually exclusive, since TODO_ABC !=
>TODO_XYZ.
>

no, because you have to prove to yourself that the queried value doesn't change in between. and so does the compiler, which may fail to tail-merge the embedded strbuf_addstr() calls as a consequence.

>Specifically: why not combine these two with if-statement that proceeds
>it? That might look something like: [...]
>

i don't see what you're referring to, so i guess you got confused about the location of the code in question?

Phillip Wood· Mar 24, 2023, 14:39 UTC · re: Taylor Blau · lore

Re: [PATCH] rebase: clarify conditionals in todo_list_to_strbuf()

Hi Taylor & Oswald
On 23/03/2023 20:32, Taylor Blau wrote:
Show 38 quoted lines
> On Thu, Mar 23, 2023 at 05:22:35PM +0100, Oswald Buddenhagen wrote:
>> Make it obvious that the two conditional branches are mutually
>> exclusive.
>>
>> As a drive-by, remove a pair of unnecessary braces.
>>
>> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>> ---
>>   sequencer.c | 7 ++-----
>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index 3be23d7ca2..9169876441 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -5868,12 +5868,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
>>   			if (item->command == TODO_FIXUP) {
>>   				if (item->flags & TODO_EDIT_FIXUP_MSG)
>>   					strbuf_addstr(buf, " -c");
>> -				else if (item->flags & TODO_REPLACE_FIXUP_MSG) {
>> +				else if (item->flags & TODO_REPLACE_FIXUP_MSG)
>>   					strbuf_addstr(buf, " -C");
>> -				}
>> -			}
>> -
>> -			if (item->command == TODO_MERGE) {
>> +			} else if (item->command == TODO_MERGE) {
> 
> I dunno. I think seeing adjacent
> 
>      if (item->command == TODO_ABC)
> 
> and
> 
>      if (item->command == TODO_XYZ)
> 
> makes it clear that these two are mutually exclusive, since TODO_ABC !=
> TODO_XYZ.

I agree, it is easy to see that they are testing different conditions and item->command is not mutated in between

Show 5 quoted lines
> So I don't mind the unnecessary brace cleanup, but I don't think that
> this adds additional clarity around these two if-statements.
> 
> Specifically: why not combine these two with if-statement that proceeds
> it? That might look something like:

I think you're looking at parse_insn_line() here rather than todo_list_to_strbuf() but your analysis of this patch still stands.

Best Wishes
Phillip
Show 33 quoted lines
> 
>      if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
>          item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {
>        ...
>      } else if (item->command == TODO_FIXUP) {
>        ...
>      } else if (item->command == TODO_MERGE) {
>        ...
>      }
> 
> but at that point, you might consider something like:
> 
>      switch (item->command) {
>      case TODO_EXEC:
>      case TODO_LABEL:
>      case TODO_RESET:
>      case TODO_UPDATE_REF:
>        ...
>        break;
>      case TODO_FIXUP:
>        ...
>        break;
>      case TODO_MERGE:
>        ...
>        break;
>      }
> 
> which is arguably clearer, but I have a hard time justifying as
> worthwhile. TBH, it feels like churn to me, but others may disagree and
> see it differently.
> 
> Thanks,
> Taylor
Oswald Buddenhagen· Apr 28, 2023, 12:56 UTC · re: Oswald Buddenhagen · lore

[PATCH v2] rebase: clarify conditionals in todo_list_to_strbuf()

Make it obvious that the two conditional branches are mutually exclusive. This makes it easier to comprehend and optimize.

As a drive-by, remove a pair of unnecessary braces.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
v2:
- slightly more verbose commit message
---
 sequencer.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
Show changes to sequencer.c +2 −5
diff --git a/sequencer.c b/sequencer.c
index 3be23d7ca2..9169876441 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5868,12 +5868,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
 			if (item->command == TODO_FIXUP) {
 				if (item->flags & TODO_EDIT_FIXUP_MSG)
 					strbuf_addstr(buf, " -c");
-				else if (item->flags & TODO_REPLACE_FIXUP_MSG) {
+				else if (item->flags & TODO_REPLACE_FIXUP_MSG)
 					strbuf_addstr(buf, " -C");
-				}
-			}
-
-			if (item->command == TODO_MERGE) {
+			} else if (item->command == TODO_MERGE) {
 				if (item->flags & TODO_EDIT_MERGE_MSG)
 					strbuf_addstr(buf, " -c");
 				else

-- 
2.40.0.152.g15d061e6df
Felipe Contreras· May 2, 2023, 18:51 UTC · re: Oswald Buddenhagen · lore

Re: [PATCH v2] rebase: clarify conditionals in todo_list_to_strbuf()

Oswald Buddenhagen wrote:
Show 33 quoted lines
> Make it obvious that the two conditional branches are mutually
> exclusive. This makes it easier to comprehend and optimize.
> 
> As a drive-by, remove a pair of unnecessary braces.
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
> v2:
> - slightly more verbose commit message
> ---
>  sequencer.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 3be23d7ca2..9169876441 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5868,12 +5868,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
>  			if (item->command == TODO_FIXUP) {
>  				if (item->flags & TODO_EDIT_FIXUP_MSG)
>  					strbuf_addstr(buf, " -c");
> -				else if (item->flags & TODO_REPLACE_FIXUP_MSG) {
> +				else if (item->flags & TODO_REPLACE_FIXUP_MSG)
>  					strbuf_addstr(buf, " -C");
> -				}
> -			}
> -
> -			if (item->command == TODO_MERGE) {
> +			} else if (item->command == TODO_MERGE) {
>  				if (item->flags & TODO_EDIT_MERGE_MSG)
>  					strbuf_addstr(buf, " -c");
>  				else
> --

FWIW makes total sense to me and does make the code easier to comprehend.

Reviewed-by: Felipe Contreras <felipe.contreras@gmail.com>
-- 
Felipe Contreras
Oswald Buddenhagen· Aug 7, 2023, 17:09 UTC · re: Oswald Buddenhagen · lore

[PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()

Make it obvious that the two conditional branches are mutually exclusive. This makes it easier to comprehend and optimize, like a switch statement would do, except that it would be overkill here.

As a drive-by, remove a pair of unnecessary braces.
Reviewed-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
v2 & v3:
- slightly more verbose commit message
Cc: Taylor Blau <me@ttaylorr.com>
Cc: Phillip Wood <phillip.wood123@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>
---
 sequencer.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
Show changes to sequencer.c +2 −5
diff --git a/sequencer.c b/sequencer.c
index cc9821ece2..97801d0489 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5880,12 +5880,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
 			if (item->command == TODO_FIXUP) {
 				if (item->flags & TODO_EDIT_FIXUP_MSG)
 					strbuf_addstr(buf, " -c");
-				else if (item->flags & TODO_REPLACE_FIXUP_MSG) {
+				else if (item->flags & TODO_REPLACE_FIXUP_MSG)
 					strbuf_addstr(buf, " -C");
-				}
-			}
-
-			if (item->command == TODO_MERGE) {
+			} else if (item->command == TODO_MERGE) {
 				if (item->flags & TODO_EDIT_MERGE_MSG)
 					strbuf_addstr(buf, " -c");
 				else

-- 
2.40.0.152.g15d061e6df
Junio C Hamano· Aug 7, 2023, 20:28 UTC · re: Oswald Buddenhagen · lore

Re: [PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
Show 14 quoted lines
>  			if (item->command == TODO_FIXUP) {
>  				if (item->flags & TODO_EDIT_FIXUP_MSG)
>  					strbuf_addstr(buf, " -c");
> -				else if (item->flags & TODO_REPLACE_FIXUP_MSG) {
> +				else if (item->flags & TODO_REPLACE_FIXUP_MSG)
>  					strbuf_addstr(buf, " -C");
> -				}
> -			}
> -
> -			if (item->command == TODO_MERGE) {
> +			} else if (item->command == TODO_MERGE) {
>  				if (item->flags & TODO_EDIT_MERGE_MSG)
>  					strbuf_addstr(buf, " -c");
>  				else

This patch as it stands is a strict Meh at least to me, as we know item->command is not something we will mess with in the loop, so turning two if() into if/elseif does not add all that much value in readability.

Having said that.
The code makes casual readers curious about other things.
 * Are FIXUP and MERGE the only two commands that need to be treated
   differently here?
 * Can item->commit be some other TODO_* command?  What is the
   reason why they can be no-op?
 * When one wants to invent a new kind of TODO_* command, what is
   the right way to deal with it in this if/else cascade?
And that leads me to wonder if this is better rewritten with
	switch (item->command) {
	case TODO_FIXUP:
		...
		break;
	case TODO_MERGE:
		...
		break;
	default:
		/*
		 * all other cases:
		 * we can have a brief explanation on why
		 * they do not need anything done here if we want
		 */
		break;
	}
Oswald Buddenhagen· Aug 9, 2023, 16:13 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()

On Mon, Aug 07, 2023 at 01:28:50PM -0700, Junio C Hamano wrote:
Show 20 quoted lines
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>>  			if (item->command == TODO_FIXUP) {
>>  				if (item->flags & TODO_EDIT_FIXUP_MSG)
>>  					strbuf_addstr(buf, " -c");
>> -				else if (item->flags & TODO_REPLACE_FIXUP_MSG) {
>> +				else if (item->flags & TODO_REPLACE_FIXUP_MSG)
>>  					strbuf_addstr(buf, " -C");
>> -				}
>> -			}
>> -
>> -			if (item->command == TODO_MERGE) {
>> +			} else if (item->command == TODO_MERGE) {
>>  				if (item->flags & TODO_EDIT_MERGE_MSG)
>>  					strbuf_addstr(buf, " -c");
>>  				else
>
>This patch as it stands is a strict Meh at least to me, as we know
>item->command is not something we will mess with in the loop,
>

the "we know" is actually something the reader needs to establish in their mind. it's simply unnecessary cognitive load.

>so
>turning two if() into if/elseif does not add all that much value in
>readability.
>

but it adds *some* value, and i don't think it's very constructive to fight that. in fact, i find the whole thread rather demotivating, and it's ironic that felipe's response was the most reasonable one.

Show 7 quoted lines
>Having said that.
>
>The code makes casual readers curious about other things.
>
> * Are FIXUP and MERGE the only two commands that need to be treated
>   differently here?
>

yes, and it's obvious why. i don't think that explaining it in prose would make the answer any more accessible.

> * Can item->commit be some other TODO_* command?
>

the fact that it's an else-if implies that much. the definite yes is clear from the bigger context.

>What is the reason why they can be no-op?
>
i have no clue what you're referring to.
> * When one wants to invent a new kind of TODO_* command, what is
>   the right way to deal with it in this if/else cascade?
>

i think that someone who actually wants to modify the code can be expected to come up with an answer themselves, as this is a much rarer occurrence than just reading the code.

>And that leads me to wonder if this is better rewritten with
>
>	switch (item->command) {
>
as the commit message was meant to imply, my answer to that is no.
regards
Junio C Hamano· Aug 9, 2023, 19:39 UTC · re: Oswald Buddenhagen · lore

Re: [PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
Show 5 quoted lines
>>And that leads me to wonder if this is better rewritten with
>>
>>	switch (item->command) {
>>
> as the commit message was meant to imply, my answer to that is no.
Thanks.  Then this patch is still a strict "Meh" to me.
Oswald Buddenhagen· Aug 10, 2023, 12:08 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()

On Wed, Aug 09, 2023 at 12:39:37PM -0700, Junio C Hamano wrote:
>Thanks.  Then this patch is still a strict "Meh" to me.
>

i can't really think of a reason why you reject such a no-brainer other than that you consider it churn. in that case i need to tell you that you have unreasonable standards, which actively contribute to the code remaining a mess.

regards
Junio C Hamano· Aug 10, 2023, 16:03 UTC · re: Oswald Buddenhagen · lore

Re: [PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
Show 7 quoted lines
> On Wed, Aug 09, 2023 at 12:39:37PM -0700, Junio C Hamano wrote:
>>Thanks.  Then this patch is still a strict "Meh" to me.
>>
> i can't really think of a reason why you reject such a no-brainer
> other than that you consider it churn. in that case i need to tell you
> that you have unreasonable standards, which actively contribute to the
> code remaining a mess.
An ad-hominem remark is a signal that it is good time to disengage.

There are certain style differences that may be acceptable if it were written from the get-go, but it is not worth the patch churn to switch once it is in the tree. This one squarely falls into that category.

Bye.
Oswald Buddenhagen· Aug 11, 2023, 10:33 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()

On Thu, Aug 10, 2023 at 09:03:54AM -0700, Junio C Hamano wrote:
Show 12 quoted lines
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>> On Wed, Aug 09, 2023 at 12:39:37PM -0700, Junio C Hamano wrote:
>>>Thanks.  Then this patch is still a strict "Meh" to me.
>>>
>> i can't really think of a reason why you reject such a no-brainer
>> other than that you consider it churn. in that case i need to tell you
>> that you have unreasonable standards, which actively contribute to the
>> code remaining a mess.
>
>An ad-hominem remark is a signal that it is good time to disengage.
>

i'm pointing out what i consider a systematic mistake. there is no way of doing that in a way that isn't somewhat personal.

the thing is that after _such_ an experience, no sane person would ever invest into something that falls under pure code maintenance in this project again. is that really what you want?

>There are certain style differences that may be acceptable if it
>were written from the get-go,
>

it's not just a style difference. it clarifies the code semantically, and potentially shrinks the executable a bit.

>but it is not worth the patch churn to switch once it is in the tree.
>
what is the problem _exactly_?

the time it takes to discuss such patches? the solution would be not bike-shedding them to death.

process overhead in applying them? then it's time to amend the process and/or tooling to accomodate trivial changes better.

minimizing history size and preserving git blame? then rethink your priorities. i'm rather OCD about this myself and would usually reject random style cleanups, but the actual experience is that a few "noise" commits don't really get into the way of doing archeology - searching in variations of `git log -p` and using "blame parent revision" in interactive tools are usually required anyway. saving a few seconds in this process really isn't worth keeping the current code messier than necessary.

anything else?
regards
Richard Kerry· Aug 11, 2023, 11:41 UTC · re: Oswald Buddenhagen · lore

RE: [PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()

Show 48 quoted lines
> 
> On Thu, Aug 10, 2023 at 09:03:54AM -0700, Junio C Hamano wrote:
> >Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> >
> >> On Wed, Aug 09, 2023 at 12:39:37PM -0700, Junio C Hamano wrote:
> >>>Thanks.  Then this patch is still a strict "Meh" to me.
> >>>
> >> i can't really think of a reason why you reject such a no-brainer
> >> other than that you consider it churn. in that case i need to tell
> >> you that you have unreasonable standards, which actively contribute
> >> to the code remaining a mess.
> >
> >An ad-hominem remark is a signal that it is good time to disengage.
> >
> i'm pointing out what i consider a systematic mistake. there is no way of
> doing that in a way that isn't somewhat personal.
> 
> the thing is that after _such_ an experience, no sane person would ever
> invest into something that falls under pure code maintenance in this project
> again. is that really what you want?
> 
> >There are certain style differences that may be acceptable if it were
> >written from the get-go,
> >
> it's not just a style difference. it clarifies the code semantically, and
> potentially shrinks the executable a bit.
> 
> >but it is not worth the patch churn to switch once it is in the tree.
> >
> what is the problem _exactly_?
> 
> the time it takes to discuss such patches? the solution would be not bike-
> shedding them to death.
> 
> process overhead in applying them? then it's time to amend the process
> and/or tooling to accomodate trivial changes better.
> 
> minimizing history size and preserving git blame? then rethink your priorities.
> i'm rather OCD about this myself and would usually reject random style
> cleanups, but the actual experience is that a few "noise"
> commits don't really get into the way of doing archeology - searching in
> variations of `git log -p` and using "blame parent revision" in interactive tools
> are usually required anyway. saving a few seconds in this process really isn't
> worth keeping the current code messier than necessary.
> 
> anything else?
> 
> regards

I wouldn't get too exercised about this - the last person who did got barred from the list. The Git project's senior management are extremely strongly attached to not breaking Hyrum's Law. However obscure, or wrong, an interface is, someone will be relying on it if there is a sufficiently large user-base. Which there will be for Git, which must have millions of users (given GitHub has claimed a hundred million users).

It is perhaps faintly possible that you could get agreement for a change with the next major version number. Or maybe an announcement that something would be deprecated and maybe the major version after that would change. Or maybe start producing a separate release series which can change this area but is distinctly separate from the glacially changing main line of releases.

Regards, Richard.

← back to recent threads