{"thread":{"id":"59449","subject":"[PATCH] rebase: clarify conditionals in todo_list_to_strbuf()","startedAt":"2023-03-23T16:47:26Z","lastAt":"2023-08-11T11:46:47Z","messageCount":14,"participants":["Oswald Buddenhagen","Taylor Blau","Phillip Wood","Felipe Contreras","Junio C Hamano","Richard Kerry"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"474001","messageId":"20230323162235.995559-1-oswald.buddenhagen@gmx.de","threadId":"59449","inReplyTo":null,"subject":"[PATCH] rebase: clarify conditionals in todo_list_to_strbuf()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T16:22:35Z","receivedAt":"2023-03-23T16:47:26Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"Make it obvious that the two conditional branches are mutually\nexclusive.\n\nAs a drive-by, remove a pair of unnecessary braces.\n\nSigned-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>\n---\n sequencer.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3be23d7ca2..9169876441 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5868,12 +5868,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis\n \t\t\tif (item->command == TODO_FIXUP) {\n \t\t\t\tif (item->flags & TODO_EDIT_FIXUP_MSG)\n \t\t\t\t\tstrbuf_addstr(buf, \" -c\");\n-\t\t\t\telse if (item->flags & TODO_REPLACE_FIXUP_MSG) {\n+\t\t\t\telse if (item->flags & TODO_REPLACE_FIXUP_MSG)\n \t\t\t\t\tstrbuf_addstr(buf, \" -C\");\n-\t\t\t\t}\n-\t\t\t}\n-\n-\t\t\tif (item->command == TODO_MERGE) {\n+\t\t\t} else if (item->command == TODO_MERGE) {\n \t\t\t\tif (item->flags & TODO_EDIT_MERGE_MSG)\n \t\t\t\t\tstrbuf_addstr(buf, \" -c\");\n \t\t\t\telse\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"474039","messageId":"ZBy3aa+7RhnjJUaG@nand.local","threadId":"59449","inReplyTo":"20230323162235.995559-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH] rebase: clarify conditionals in todo_list_to_strbuf()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-23T20:32:41Z","receivedAt":"2023-03-23T20:32:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 23, 2023 at 05:22:35PM +0100, Oswald Buddenhagen wrote:\n> Make it obvious that the two conditional branches are mutually\n> exclusive.\n>\n> As a drive-by, remove a pair of unnecessary braces.\n>\n> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>\n> ---\n>  sequencer.c | 7 ++-----\n>  1 file changed, 2 insertions(+), 5 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 3be23d7ca2..9169876441 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -5868,12 +5868,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis\n>  \t\t\tif (item->command == TODO_FIXUP) {\n>  \t\t\t\tif (item->flags & TODO_EDIT_FIXUP_MSG)\n>  \t\t\t\t\tstrbuf_addstr(buf, \" -c\");\n> -\t\t\t\telse if (item->flags & TODO_REPLACE_FIXUP_MSG) {\n> +\t\t\t\telse if (item->flags & TODO_REPLACE_FIXUP_MSG)\n>  \t\t\t\t\tstrbuf_addstr(buf, \" -C\");\n> -\t\t\t\t}\n> -\t\t\t}\n> -\n> -\t\t\tif (item->command == TODO_MERGE) {\n> +\t\t\t} else if (item->command == TODO_MERGE) {\n\nI dunno. I think seeing adjacent\n\n    if (item->command == TODO_ABC)\n\nand\n\n    if (item->command == TODO_XYZ)\n\nmakes it clear that these two are mutually exclusive, since TODO_ABC !=\nTODO_XYZ.\n\nSo I don't mind the unnecessary brace cleanup, but I don't think that\nthis adds additional clarity around these two if-statements.\n\nSpecifically: why not combine these two with if-statement that proceeds\nit? That might look something like:\n\n    if (item->command == TODO_EXEC || item->command == TODO_LABEL ||\n        item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {\n      ...\n    } else if (item->command == TODO_FIXUP) {\n      ...\n    } else if (item->command == TODO_MERGE) {\n      ...\n    }\n\nbut at that point, you might consider something like:\n\n    switch (item->command) {\n    case TODO_EXEC:\n    case TODO_LABEL:\n    case TODO_RESET:\n    case TODO_UPDATE_REF:\n      ...\n      break;\n    case TODO_FIXUP:\n      ...\n      break;\n    case TODO_MERGE:\n      ...\n      break;\n    }\n\nwhich is arguably clearer, but I have a hard time justifying as\nworthwhile. TBH, it feels like churn to me, but others may disagree and\nsee it differently.\n\nThanks,\nTaylor\n"},{"id":"474078","messageId":"ZB1miMcYWXWBvGbm@ugly","threadId":"59449","inReplyTo":"ZBy3aa+7RhnjJUaG@nand.local","subject":"Re: [PATCH] rebase: clarify conditionals in todo_list_to_strbuf()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-24T08:59:52Z","receivedAt":"2023-03-24T09:00:17Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Thu, Mar 23, 2023 at 04:32:41PM -0400, Taylor Blau wrote:\n>I dunno. I think seeing adjacent\n>\n>    if (item->command == TODO_ABC)\n>\n>and\n>\n>    if (item->command == TODO_XYZ)\n>\n>makes it clear that these two are mutually exclusive, since TODO_ABC !=\n>TODO_XYZ.\n>\nno, because you have to prove to yourself that the queried value doesn't \nchange in between. and so does the compiler, which may fail to \ntail-merge the embedded strbuf_addstr() calls as a consequence.\n\n>Specifically: why not combine these two with if-statement that proceeds\n>it? That might look something like: [...]\n>\ni don't see what you're referring to, so i guess you got confused about \nthe location of the code in question?\n"},{"id":"474086","messageId":"7a0c66a3-0bf9-5bc5-a44e-c948d0b339f4@dunelm.org.uk","threadId":"59449","inReplyTo":"ZBy3aa+7RhnjJUaG@nand.local","subject":"Re: [PATCH] rebase: clarify conditionals in todo_list_to_strbuf()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-24T14:39:12Z","receivedAt":"2023-03-24T14:39:30Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Taylor & Oswald\n\nOn 23/03/2023 20:32, Taylor Blau wrote:\n> On Thu, Mar 23, 2023 at 05:22:35PM +0100, Oswald Buddenhagen wrote:\n>> Make it obvious that the two conditional branches are mutually\n>> exclusive.\n>>\n>> As a drive-by, remove a pair of unnecessary braces.\n>>\n>> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>\n>> ---\n>>   sequencer.c | 7 ++-----\n>>   1 file changed, 2 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 3be23d7ca2..9169876441 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -5868,12 +5868,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis\n>>   \t\t\tif (item->command == TODO_FIXUP) {\n>>   \t\t\t\tif (item->flags & TODO_EDIT_FIXUP_MSG)\n>>   \t\t\t\t\tstrbuf_addstr(buf, \" -c\");\n>> -\t\t\t\telse if (item->flags & TODO_REPLACE_FIXUP_MSG) {\n>> +\t\t\t\telse if (item->flags & TODO_REPLACE_FIXUP_MSG)\n>>   \t\t\t\t\tstrbuf_addstr(buf, \" -C\");\n>> -\t\t\t\t}\n>> -\t\t\t}\n>> -\n>> -\t\t\tif (item->command == TODO_MERGE) {\n>> +\t\t\t} else if (item->command == TODO_MERGE) {\n> \n> I dunno. I think seeing adjacent\n> \n>      if (item->command == TODO_ABC)\n> \n> and\n> \n>      if (item->command == TODO_XYZ)\n> \n> makes it clear that these two are mutually exclusive, since TODO_ABC !=\n> TODO_XYZ.\n\nI agree, it is easy to see that they are testing different conditions \nand item->command is not mutated in between\n\n> So I don't mind the unnecessary brace cleanup, but I don't think that\n> this adds additional clarity around these two if-statements.\n> \n> Specifically: why not combine these two with if-statement that proceeds\n> it? That might look something like:\n\nI think you're looking at parse_insn_line() here rather than \ntodo_list_to_strbuf() but your analysis of this patch still stands.\n\nBest Wishes\n\nPhillip\n\n> \n>      if (item->command == TODO_EXEC || item->command == TODO_LABEL ||\n>          item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {\n>        ...\n>      } else if (item->command == TODO_FIXUP) {\n>        ...\n>      } else if (item->command == TODO_MERGE) {\n>        ...\n>      }\n> \n> but at that point, you might consider something like:\n> \n>      switch (item->command) {\n>      case TODO_EXEC:\n>      case TODO_LABEL:\n>      case TODO_RESET:\n>      case TODO_UPDATE_REF:\n>        ...\n>        break;\n>      case TODO_FIXUP:\n>        ...\n>        break;\n>      case TODO_MERGE:\n>        ...\n>        break;\n>      }\n> \n> which is arguably clearer, but I have a hard time justifying as\n> worthwhile. TBH, it feels like churn to me, but others may disagree and\n> see it differently.\n> \n> Thanks,\n> Taylor\n"},{"id":"476268","messageId":"20230428125601.1719750-1-oswald.buddenhagen@gmx.de","threadId":"59449","inReplyTo":"20230323162235.995559-1-oswald.buddenhagen@gmx.de","subject":"[PATCH v2] rebase: clarify conditionals in todo_list_to_strbuf()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-04-28T12:56:01Z","receivedAt":"2023-04-28T12:56:14Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"Make it obvious that the two conditional branches are mutually\nexclusive. This makes it easier to comprehend and optimize.\n\nAs a drive-by, remove a pair of unnecessary braces.\n\nSigned-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>\n---\nv2:\n- slightly more verbose commit message\n---\n sequencer.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3be23d7ca2..9169876441 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5868,12 +5868,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis\n \t\t\tif (item->command == TODO_FIXUP) {\n \t\t\t\tif (item->flags & TODO_EDIT_FIXUP_MSG)\n \t\t\t\t\tstrbuf_addstr(buf, \" -c\");\n-\t\t\t\telse if (item->flags & TODO_REPLACE_FIXUP_MSG) {\n+\t\t\t\telse if (item->flags & TODO_REPLACE_FIXUP_MSG)\n \t\t\t\t\tstrbuf_addstr(buf, \" -C\");\n-\t\t\t\t}\n-\t\t\t}\n-\n-\t\t\tif (item->command == TODO_MERGE) {\n+\t\t\t} else if (item->command == TODO_MERGE) {\n \t\t\t\tif (item->flags & TODO_EDIT_MERGE_MSG)\n \t\t\t\t\tstrbuf_addstr(buf, \" -c\");\n \t\t\t\telse\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"476448","messageId":"64515bb97dd1b_1ba2d294f3@chronos.notmuch","threadId":"59449","inReplyTo":"20230428125601.1719750-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH v2] rebase: clarify conditionals in todo_list_to_strbuf()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-02T18:51:37Z","receivedAt":"2023-05-02T18:51:42Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Oswald Buddenhagen wrote:\n> Make it obvious that the two conditional branches are mutually\n> exclusive. This makes it easier to comprehend and optimize.\n> \n> As a drive-by, remove a pair of unnecessary braces.\n> \n> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>\n> ---\n> v2:\n> - slightly more verbose commit message\n> ---\n>  sequencer.c | 7 ++-----\n>  1 file changed, 2 insertions(+), 5 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 3be23d7ca2..9169876441 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -5868,12 +5868,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis\n>  \t\t\tif (item->command == TODO_FIXUP) {\n>  \t\t\t\tif (item->flags & TODO_EDIT_FIXUP_MSG)\n>  \t\t\t\t\tstrbuf_addstr(buf, \" -c\");\n> -\t\t\t\telse if (item->flags & TODO_REPLACE_FIXUP_MSG) {\n> +\t\t\t\telse if (item->flags & TODO_REPLACE_FIXUP_MSG)\n>  \t\t\t\t\tstrbuf_addstr(buf, \" -C\");\n> -\t\t\t\t}\n> -\t\t\t}\n> -\n> -\t\t\tif (item->command == TODO_MERGE) {\n> +\t\t\t} else if (item->command == TODO_MERGE) {\n>  \t\t\t\tif (item->flags & TODO_EDIT_MERGE_MSG)\n>  \t\t\t\t\tstrbuf_addstr(buf, \" -c\");\n>  \t\t\t\telse\n> -- \n\nFWIW makes total sense to me and does make the code easier to\ncomprehend.\n\nReviewed-by: Felipe Contreras <felipe.contreras@gmail.com>\n\n-- \nFelipe Contreras\n"},{"id":"480236","messageId":"20230807170935.2336745-1-oswald.buddenhagen@gmx.de","threadId":"59449","inReplyTo":"20230428125601.1719750-1-oswald.buddenhagen@gmx.de","subject":"[PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-07T17:09:35Z","receivedAt":"2023-08-07T17:09:48Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"Make it obvious that the two conditional branches are mutually\nexclusive. This makes it easier to comprehend and optimize, like a\nswitch statement would do, except that it would be overkill here.\n\nAs a drive-by, remove a pair of unnecessary braces.\n\nReviewed-by: Felipe Contreras <felipe.contreras@gmail.com>\nSigned-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>\n---\nv2 & v3:\n- slightly more verbose commit message\n\nCc: Taylor Blau <me@ttaylorr.com>\nCc: Phillip Wood <phillip.wood123@gmail.com>\nCc: Junio C Hamano <gitster@pobox.com>\n---\n sequencer.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex cc9821ece2..97801d0489 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5880,12 +5880,9 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis\n \t\t\tif (item->command == TODO_FIXUP) {\n \t\t\t\tif (item->flags & TODO_EDIT_FIXUP_MSG)\n \t\t\t\t\tstrbuf_addstr(buf, \" -c\");\n-\t\t\t\telse if (item->flags & TODO_REPLACE_FIXUP_MSG) {\n+\t\t\t\telse if (item->flags & TODO_REPLACE_FIXUP_MSG)\n \t\t\t\t\tstrbuf_addstr(buf, \" -C\");\n-\t\t\t\t}\n-\t\t\t}\n-\n-\t\t\tif (item->command == TODO_MERGE) {\n+\t\t\t} else if (item->command == TODO_MERGE) {\n \t\t\t\tif (item->flags & TODO_EDIT_MERGE_MSG)\n \t\t\t\t\tstrbuf_addstr(buf, \" -c\");\n \t\t\t\telse\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"480256","messageId":"xmqqv8dqd2bh.fsf@gitster.g","threadId":"59449","inReplyTo":"20230807170935.2336745-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-07T20:28:50Z","receivedAt":"2023-08-07T20:28:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:\n\n>  \t\t\tif (item->command == TODO_FIXUP) {\n>  \t\t\t\tif (item->flags & TODO_EDIT_FIXUP_MSG)\n>  \t\t\t\t\tstrbuf_addstr(buf, \" -c\");\n> -\t\t\t\telse if (item->flags & TODO_REPLACE_FIXUP_MSG) {\n> +\t\t\t\telse if (item->flags & TODO_REPLACE_FIXUP_MSG)\n>  \t\t\t\t\tstrbuf_addstr(buf, \" -C\");\n> -\t\t\t\t}\n> -\t\t\t}\n> -\n> -\t\t\tif (item->command == TODO_MERGE) {\n> +\t\t\t} else if (item->command == TODO_MERGE) {\n>  \t\t\t\tif (item->flags & TODO_EDIT_MERGE_MSG)\n>  \t\t\t\t\tstrbuf_addstr(buf, \" -c\");\n>  \t\t\t\telse\n\nThis patch as it stands is a strict Meh at least to me, as we know\nitem->command is not something we will mess with in the loop, so\nturning two if() into if/elseif does not add all that much value in\nreadability.\n\nHaving said that.\n\nThe code makes casual readers curious about other things.\n\n * Are FIXUP and MERGE the only two commands that need to be treated\n   differently here?\n\n * Can item->commit be some other TODO_* command?  What is the\n   reason why they can be no-op?\n\n * When one wants to invent a new kind of TODO_* command, what is\n   the right way to deal with it in this if/else cascade?\n\nAnd that leads me to wonder if this is better rewritten with\n\n\tswitch (item->command) {\n\tcase TODO_FIXUP:\n\t\t...\n\t\tbreak;\n\tcase TODO_MERGE:\n\t\t...\n\t\tbreak;\n\tdefault:\n\t\t/*\n\t\t * all other cases:\n\t\t * we can have a brief explanation on why\n\t\t * they do not need anything done here if we want\n\t\t */\n\t\tbreak;\n\t}\n\n"},{"id":"480362","messageId":"ZNO7IVphPf8KOC3Q@ugly","threadId":"59449","inReplyTo":"xmqqv8dqd2bh.fsf@gitster.g","subject":"Re: [PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-09T16:13:21Z","receivedAt":"2023-08-09T16:13:32Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Mon, Aug 07, 2023 at 01:28:50PM -0700, Junio C Hamano wrote:\n>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:\n>\n>>  \t\t\tif (item->command == TODO_FIXUP) {\n>>  \t\t\t\tif (item->flags & TODO_EDIT_FIXUP_MSG)\n>>  \t\t\t\t\tstrbuf_addstr(buf, \" -c\");\n>> -\t\t\t\telse if (item->flags & TODO_REPLACE_FIXUP_MSG) {\n>> +\t\t\t\telse if (item->flags & TODO_REPLACE_FIXUP_MSG)\n>>  \t\t\t\t\tstrbuf_addstr(buf, \" -C\");\n>> -\t\t\t\t}\n>> -\t\t\t}\n>> -\n>> -\t\t\tif (item->command == TODO_MERGE) {\n>> +\t\t\t} else if (item->command == TODO_MERGE) {\n>>  \t\t\t\tif (item->flags & TODO_EDIT_MERGE_MSG)\n>>  \t\t\t\t\tstrbuf_addstr(buf, \" -c\");\n>>  \t\t\t\telse\n>\n>This patch as it stands is a strict Meh at least to me, as we know\n>item->command is not something we will mess with in the loop,\n>\nthe \"we know\" is actually something the reader needs to establish in \ntheir mind. it's simply unnecessary cognitive load.\n\n>so\n>turning two if() into if/elseif does not add all that much value in\n>readability.\n>\nbut it adds *some* value, and i don't think it's very constructive to \nfight that. in fact, i find the whole thread rather demotivating, and \nit's ironic that felipe's response was the most reasonable one.\n\n>Having said that.\n>\n>The code makes casual readers curious about other things.\n>\n> * Are FIXUP and MERGE the only two commands that need to be treated\n>   differently here?\n>\nyes, and it's obvious why. i don't think that explaining it in prose \nwould make the answer any more accessible.\n\n> * Can item->commit be some other TODO_* command?\n>\nthe fact that it's an else-if implies that much. the definite yes is \nclear from the bigger context.\n\n>What is the reason why they can be no-op?\n>\ni have no clue what you're referring to.\n\n> * When one wants to invent a new kind of TODO_* command, what is\n>   the right way to deal with it in this if/else cascade?\n>\ni think that someone who actually wants to modify the code can be \nexpected to come up with an answer themselves, as this is a much rarer \noccurrence than just reading the code.\n\n>And that leads me to wonder if this is better rewritten with\n>\n>\tswitch (item->command) {\n>\nas the commit message was meant to imply, my answer to that is no.\n\nregards\n"},{"id":"480392","messageId":"xmqqbkfgm2di.fsf@gitster.g","threadId":"59449","inReplyTo":"ZNO7IVphPf8KOC3Q@ugly","subject":"Re: [PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-09T19:39:37Z","receivedAt":"2023-08-09T19:39:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:\n\n>>And that leads me to wonder if this is better rewritten with\n>>\n>>\tswitch (item->command) {\n>>\n> as the commit message was meant to imply, my answer to that is no.\n\nThanks.  Then this patch is still a strict \"Meh\" to me.\n\n"},{"id":"480433","messageId":"ZNTTTAtNE2/DY9vT@ugly","threadId":"59449","inReplyTo":"xmqqbkfgm2di.fsf@gitster.g","subject":"Re: [PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-10T12:08:44Z","receivedAt":"2023-08-10T12:08:49Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Wed, Aug 09, 2023 at 12:39:37PM -0700, Junio C Hamano wrote:\n>Thanks.  Then this patch is still a strict \"Meh\" to me.\n>\ni can't really think of a reason why you reject such a no-brainer other \nthan that you consider it churn. in that case i need to tell you that \nyou have unreasonable standards, which actively contribute to the code \nremaining a mess.\n\nregards\n"},{"id":"480443","messageId":"xmqqleeihok5.fsf@gitster.g","threadId":"59449","inReplyTo":"ZNTTTAtNE2/DY9vT@ugly","subject":"Re: [PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-10T16:03:54Z","receivedAt":"2023-08-10T16:04:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:\n\n> On Wed, Aug 09, 2023 at 12:39:37PM -0700, Junio C Hamano wrote:\n>>Thanks.  Then this patch is still a strict \"Meh\" to me.\n>>\n> i can't really think of a reason why you reject such a no-brainer\n> other than that you consider it churn. in that case i need to tell you\n> that you have unreasonable standards, which actively contribute to the\n> code remaining a mess.\n\nAn ad-hominem remark is a signal that it is good time to disengage.\n\nThere are certain style differences that may be acceptable if it\nwere written from the get-go, but it is not worth the patch churn to\nswitch once it is in the tree.  This one squarely falls into that\ncategory.\n\nBye.\n\n"},{"id":"480529","messageId":"ZNYOco835hbiDZAC@ugly","threadId":"59449","inReplyTo":"xmqqleeihok5.fsf@gitster.g","subject":"Re: [PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-11T10:33:22Z","receivedAt":"2023-08-11T10:33:27Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Thu, Aug 10, 2023 at 09:03:54AM -0700, Junio C Hamano wrote:\n>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:\n>\n>> On Wed, Aug 09, 2023 at 12:39:37PM -0700, Junio C Hamano wrote:\n>>>Thanks.  Then this patch is still a strict \"Meh\" to me.\n>>>\n>> i can't really think of a reason why you reject such a no-brainer\n>> other than that you consider it churn. in that case i need to tell you\n>> that you have unreasonable standards, which actively contribute to the\n>> code remaining a mess.\n>\n>An ad-hominem remark is a signal that it is good time to disengage.\n>\ni'm pointing out what i consider a systematic mistake. there is no way \nof doing that in a way that isn't somewhat personal.\n\nthe thing is that after _such_ an experience, no sane person would ever \ninvest into something that falls under pure code maintenance in this \nproject again. is that really what you want?\n\n>There are certain style differences that may be acceptable if it\n>were written from the get-go,\n>\nit's not just a style difference. it clarifies the code semantically, \nand potentially shrinks the executable a bit.\n\n>but it is not worth the patch churn to switch once it is in the tree.\n>\nwhat is the problem _exactly_?\n\nthe time it takes to discuss such patches? the solution would be not \nbike-shedding them to death.\n\nprocess overhead in applying them? then it's time to amend the process \nand/or tooling to accomodate trivial changes better.\n\nminimizing history size and preserving git blame? then rethink your \npriorities. i'm rather OCD about this myself and would usually reject \nrandom style cleanups, but the actual experience is that a few \"noise\" \ncommits don't really get into the way of doing archeology - searching in \nvariations of `git log -p` and using \"blame parent revision\" in \ninteractive tools are usually required anyway. saving a few seconds in \nthis process really isn't worth keeping the current code messier than \nnecessary.\n\nanything else?\n\nregards\n\n"},{"id":"480530","messageId":"AS8PR02MB730225B5F3D9370326AAB3C99C10A@AS8PR02MB7302.eurprd02.prod.outlook.com","threadId":"59449","inReplyTo":"ZNYOco835hbiDZAC@ugly","subject":"RE: [PATCH v3] rebase: clarify conditionals in todo_list_to_strbuf()","fromName":"Richard Kerry","fromEmail":"richard.kerry@eviden.com","sentAt":"2023-08-11T11:41:36Z","receivedAt":"2023-08-11T11:46:47Z","isPatch":true,"sender":{"key":"richard.kerry@eviden.com","avatar":null},"body":"> \n> On Thu, Aug 10, 2023 at 09:03:54AM -0700, Junio C Hamano wrote:\n> >Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:\n> >\n> >> On Wed, Aug 09, 2023 at 12:39:37PM -0700, Junio C Hamano wrote:\n> >>>Thanks.  Then this patch is still a strict \"Meh\" to me.\n> >>>\n> >> i can't really think of a reason why you reject such a no-brainer\n> >> other than that you consider it churn. in that case i need to tell\n> >> you that you have unreasonable standards, which actively contribute\n> >> to the code remaining a mess.\n> >\n> >An ad-hominem remark is a signal that it is good time to disengage.\n> >\n> i'm pointing out what i consider a systematic mistake. there is no way of\n> doing that in a way that isn't somewhat personal.\n> \n> the thing is that after _such_ an experience, no sane person would ever\n> invest into something that falls under pure code maintenance in this project\n> again. is that really what you want?\n> \n> >There are certain style differences that may be acceptable if it were\n> >written from the get-go,\n> >\n> it's not just a style difference. it clarifies the code semantically, and\n> potentially shrinks the executable a bit.\n> \n> >but it is not worth the patch churn to switch once it is in the tree.\n> >\n> what is the problem _exactly_?\n> \n> the time it takes to discuss such patches? the solution would be not bike-\n> shedding them to death.\n> \n> process overhead in applying them? then it's time to amend the process\n> and/or tooling to accomodate trivial changes better.\n> \n> minimizing history size and preserving git blame? then rethink your priorities.\n> i'm rather OCD about this myself and would usually reject random style\n> cleanups, but the actual experience is that a few \"noise\"\n> commits don't really get into the way of doing archeology - searching in\n> variations of `git log -p` and using \"blame parent revision\" in interactive tools\n> are usually required anyway. saving a few seconds in this process really isn't\n> worth keeping the current code messier than necessary.\n> \n> anything else?\n> \n> regards\n\nI wouldn't get too exercised about this - the last person who did got barred from the list.\nThe Git project's senior management are extremely strongly attached to not breaking Hyrum's Law.\nHowever 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). \n\nIt 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.\nOr maybe start producing a separate release series which can change this area but is distinctly separate from the glacially changing main line of releases.\n\nRegards,\nRichard.\n\n"}]}