[PATCH] rebase: clarify conditionals in todo_list_to_strbuf()

[PATCH] rebase: clarify conditionals in todo_list_to_strbuf()

[Oswald Buddenhagen]

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) {
 				if (item->flags & TODO_EDIT_MERGE_MSG)
 					strbuf_addstr(buf, " -c");
 				else
-- 
2.40.0.152.g15d061e6df

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

[Taylor Blau]

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.

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

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

[Oswald Buddenhagen]

On Thu, Mar 23, 2023 at 04:32:41PM -0400, Taylor Blau wrote:
>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?

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

[Phillip Wood]

Hi Taylor & Oswald

On 23/03/2023 20:32, Taylor Blau wrote:
> 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

> 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

> 
>      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

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

[Oswald Buddenhagen]

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
-- 
2.40.0.152.g15d061e6df

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

[Felipe Contreras]

Oswald Buddenhagen wrote:
> 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

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

[Oswald Buddenhagen]

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(-)

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

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

[Junio C Hamano]

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, 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;
	}

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

[Oswald Buddenhagen]

On Mon, Aug 07, 2023 at 01:28:50PM -0700, Junio C Hamano wrote:
>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.

>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

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

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

>>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.

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

[Oswald Buddenhagen]

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

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

[Junio C Hamano]

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.

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.

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

[Oswald Buddenhagen]

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

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

[Richard Kerry]

> 
> 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.