[v3] rebase: clarify conditionals in todo_list_to_strbuf()

Commit Message

Oswald Buddenhagen Aug. 7, 2023, 5:09 p.m. UTC

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 <[e-mail]>
Signed-off-by: Oswald Buddenhagen <[e-mail]>
---
v2 & v3:
- slightly more verbose commit message

Cc: Taylor Blau <[e-mail]>
Cc: Phillip Wood <[e-mail]>
Cc: Junio C Hamano <[e-mail]>
---
 sequencer.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Comments

Junio C Hamano Aug. 7, 2023, 8:28 p.m. UTC | #1

Oswald Buddenhagen <[e-mail]> 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;
	}

Oswald Buddenhagen Aug. 9, 2023, 4:13 p.m. UTC | #2

On Mon, Aug 07, 2023 at 01:28:50PM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <[e-mail]> 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

Junio C Hamano Aug. 9, 2023, 7:39 p.m. UTC | #3

Oswald Buddenhagen <[e-mail]> 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.

Oswald Buddenhagen Aug. 10, 2023, 12:08 p.m. UTC | #4

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, 4:03 p.m. UTC | #5

Oswald Buddenhagen <[e-mail]> 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.

Oswald Buddenhagen Aug. 11, 2023, 10:33 a.m. UTC | #6

On Thu, Aug 10, 2023 at 09:03:54AM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <[e-mail]> 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 a.m. UTC | #7

> 
> On Thu, Aug 10, 2023 at 09:03:54AM -0700, Junio C Hamano wrote:
> >Oswald Buddenhagen <[e-mail]> 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.

Patch

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