{"thread":{"id":"59457","subject":"[PATCH] sequencer: rectify empty hint in call of require_clean_work_tree()","startedAt":"2023-03-23T16:47:52Z","lastAt":"2023-08-24T16:00:04Z","messageCount":15,"participants":["Oswald Buddenhagen","Junio C Hamano","Felipe Contreras"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"474012","messageId":"20230323162234.995514-1-oswald.buddenhagen@gmx.de","threadId":"59457","inReplyTo":null,"subject":"[PATCH] sequencer: rectify empty hint in call of require_clean_work_tree()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T16:22:34Z","receivedAt":"2023-03-23T16:47:52Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"The canonical way to represent \"no error hint\" is making it null, which\nshortcuts the error() call altogether.\n\nSigned-off-by: Oswald Buddenhagen \n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3be23d7ca2..7c275c9a65 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -6168,7 +6168,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n \t\tgoto cleanup;\n \n-\tif (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\n+\tif (require_clean_work_tree(r, \"rebase\", NULL, 1, 1))\n \t\tgoto cleanup;\n \n \ttodo_list_write_total_nr(&new_todo);\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"476175","messageId":"ZEorMhPZRL/w4yKM@ugly","threadId":"59457","inReplyTo":"20230323162234.995514-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH] sequencer: rectify empty hint in call of require_clean_work_tree()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-04-27T07:58:42Z","receivedAt":"2023-04-27T07:58:50Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"ping!\n\nOn Thu, Mar 23, 2023 at 05:22:34PM +0100, Oswald Buddenhagen wrote:\n>The canonical way to represent \"no error hint\" is making it null, which\n>shortcuts the error() call altogether.\n>\n"},{"id":"476234","messageId":"xmqqmt2thvuu.fsf@gitster.g","threadId":"59457","inReplyTo":"ZEorMhPZRL/w4yKM@ugly","subject":"Re: [PATCH] sequencer: rectify empty hint in call of require_clean_work_tree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-27T21:13:29Z","receivedAt":"2023-04-27T21:13:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> ping!\n>\n> On Thu, Mar 23, 2023 at 05:22:34PM +0100, Oswald Buddenhagen wrote:\n>>The canonical way to represent \"no error hint\" is making it null, which\n>>shortcuts the error() call altogether.\n\nI won't repeat what Peff already said on another ping! we saw\nrecently on the list.\n\nThe call to require_clean_work_tree() with \"\" hint existed ever\nsince this part of the \"rebase\" machinery was rewritten in C by\nb97e1873 (rebase -i: rewrite complete_action() in C, 2018-08-28).\nI added the author of that change to the Cc: list.\n\nThe original implementation of require_clean_work_tree looked like\nthis:\n\nrequire_clean_work_tree () {\n\tgit rev-parse --verify HEAD >/dev/null || exit 1\n\tgit update-index -q --ignore-submodules --refresh\n\terr=0\n\n\tif ! git diff-files --quiet --ignore-submodules\n\tthen\n\t\t...\n\t\terr=1\n\tfi\n\n\tif ! git diff-index --cached --quiet --ignore-submodules HEAD --\n\tthen\n\t\t...\n\t\terr=1\n\tfi\n\n\tif test $err = 1\n\tthen\n\t\ttest -n \"$2\" && echo \"$2\" >&2\n\t\texit 1\n\tfi\n}\n\nI.e. the second argument, \"hint\", is shown only when it was a\nnon-empty string. It did not add \"error:\" prefix before the\nmessage.\n\nIn contrast, this is what wt-status.c has:\n\nint require_clean_work_tree(struct repository *r,\n\t\t\t const char *action,\n\t\t\t const char *hint,\n\t\t\t int ignore_submodules,\n\t\t\t int gently)\n{\n\tstruct lock_file lock_file = LOCK_INIT;\n\tint err = 0, fd;\n\n\t...\n\tif (err) {\n\t\tif (hint)\n\t\t\terror(\"%s\", hint);\n\t\tif (!gently)\n\t\t\texit(128);\n\t}\n\n\treturn err;\n}\n\nArguably, using error() as a replacement for 'echo \"$2\" >&2' was a\nsloppy conversion made back in ea63b393 (pull: make code more\nsimilar to the shell script again, 2016-10-07), but I suspect that\nin-tree callers that do have something to say, and the end-users who\nare used to see the messages these callers produce, expect to see\nthe \"error:\" prefix these days, so it needs further study if we\nwanted to \"fix\" the misuse of error() there. In any case, the\nobservation that motivated your patch is not error() vs fputs().\n\nFor squelching a useless \"hint\" that is empty (other than that\nmistaken \"error:\" prefix), however, I think you can and should do\nbetter than replacing \"\" with NULL on the callers' side. As we can\nsee from the comparison between the original, scripted version and\nthe verison in C that is in wt-status.c of require_clean_work_tree,\nchecking for NULL-ness of hint is another misconversion made when\nit was rewritten in C.\n\nI think the right fix would be more like the attached patch, which\nwill fix any other callsites that pass \"\" at the same time. Of\ncourse, you can fix the callers on top, but that is secondary.\n\n wt-status.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git i/wt-status.c w/wt-status.c\nindex 97b9c1c035..2b6dc4d6ac 100644\n--- i/wt-status.c\n+++ w/wt-status.c\n@@ -2650,7 +2650,7 @@ int require_clean_work_tree(struct repository *r,\n \t}\n \n \tif (err) {\n-\t\tif (hint)\n+\t\tif (hint && *hint)\n \t\t\terror(\"%s\", hint);\n \t\tif (!gently)\n \t\t\texit(128);\n\n"},{"id":"476245","messageId":"ZEr4VKMPSYZZxgIM@ugly","threadId":"59457","inReplyTo":"xmqqmt2thvuu.fsf@gitster.g","subject":"Re: [PATCH] sequencer: rectify empty hint in call of require_clean_work_tree()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-04-27T22:33:56Z","receivedAt":"2023-04-27T22:34:28Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Thu, Apr 27, 2023 at 02:13:29PM -0700, Junio C Hamano wrote:\n>I think the right fix would be more like the attached patch, which\n>will fix any other callsites that pass \"\" at the same time. Of\n>course, you can fix the callers on top, but that is secondary.\n>\nthere is only that one incorrect (in-tree) call.\ni don't think that making the behavior more compliant with the shell \nimplementation is particularly elegant or even useful.\nif i wanted to be super-pedantic about it, i'd assert that non-null \nstrings are non-empty. but that would only help if all error paths \nactually have test coverage.\n\n>--- i/wt-status.c\n>+++ w/wt-status.c\n>@@ -2650,7 +2650,7 @@ int require_clean_work_tree(struct repository *r,\n> \n>-\t\tif (hint)\n>+\t\tif (hint && *hint)\n> \t\t\terror(\"%s\", hint);\n\n-- ossi\n\n"},{"id":"476450","messageId":"64515cff45e03_1ba2d29458@chronos.notmuch","threadId":"59457","inReplyTo":"ZEr4VKMPSYZZxgIM@ugly","subject":"Re: [PATCH] sequencer: rectify empty hint in call of require_clean_work_tree()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-02T18:57:03Z","receivedAt":"2023-05-02T18:57:09Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Oswald Buddenhagen wrote:\n> if i wanted to be super-pedantic about it, i'd assert that non-null \n> strings are non-empty.\n\nI would disagree. \"\" is empty but not null, not just in C but in many\nother languages, including shell.\n\n-- \nFelipe Contreras\n"},{"id":"476492","messageId":"ZFIKIXF+nOoC+U1D@ugly","threadId":"59457","inReplyTo":"64515cff45e03_1ba2d29458@chronos.notmuch","subject":"Re: [PATCH] sequencer: rectify empty hint in call of require_clean_work_tree()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-05-03T07:15:45Z","receivedAt":"2023-05-03T07:15:57Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Tue, May 02, 2023 at 12:57:03PM -0600, Felipe Contreras wrote:\n>Oswald Buddenhagen wrote:\n>> if i wanted to be super-pedantic about it, i'd assert that non-null \n>> strings are non-empty.\n>\n>I would disagree. \"\" is empty but not null, not just in C but in many\n>other languages, including shell.\n>\nyes. that's kinda the point the assert would make.\n\n-- ossi\n\n"},{"id":"480235","messageId":"20230807170935.2336730-1-oswald.buddenhagen@gmx.de","threadId":"59457","inReplyTo":"20230323162234.995514-1-oswald.buddenhagen@gmx.de","subject":"[PATCH v2] sequencer: rectify empty hint in call of require_clean_work_tree()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-07T17:09:35Z","receivedAt":"2023-08-07T17:09:47Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"The canonical way to represent \"no error hint\" is making it null, which\nshortcuts the error() call altogether. This fixes the output by removing\nthe line which said just \"error:\".\n\nAlternatively, one could make the function treat empty strings like null\nstrings, which would make it resemble its original script implementation\nmore closely, but that doesn't seem like a worthwhile goal. If anything,\nI'd go in the opposite direction and assert() that the argument is not\nan empty string.\n\nSigned-off-by: Oswald Buddenhagen \n---\nv2:\n- expanded commit message\n\nCc: Junio C Hamano \n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex cc9821ece2..d15a7409d8 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -6182,7 +6182,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n \t\tgoto cleanup;\n \n-\tif (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\n+\tif (require_clean_work_tree(r, \"rebase\", NULL, 1, 1))\n \t\tgoto cleanup;\n \n \ttodo_list_write_total_nr(&new_todo);\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"480254","messageId":"xmqqa5v2ehba.fsf@gitster.g","threadId":"59457","inReplyTo":"20230807170935.2336730-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH v2] sequencer: rectify empty hint in call of require_clean_work_tree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-07T20:19:37Z","receivedAt":"2023-08-07T20:19:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> The canonical way to represent \"no error hint\" is making it null, which\n> shortcuts the error() call altogether. This fixes the output by removing\n> the line which said just \"error:\".\n>\n> Alternatively, one could make the function treat empty strings like null\n> strings, which would make it resemble its original script implementation\n> more closely, but that doesn't seem like a worthwhile goal. If anything,\n\nThe analysis I gave you in the previous round was primarily to\nexplain how this bug came to be, i.e. a misconversion of the\nscripted original, and that would be a lot more pertinent\ninformation than \"is this closer to or further from the scripted\noriginal?\" IOW, \"equating NULL and an empty string is how the\noriginal implemented the logic correctly, but the misconversion made\nthe current source to fail to do so\" is the take-home message. \n\nYou could correct the current version by making it follow the same\n\"NULL and an empty string are the same\" convention to implement the\nlogic correctly, or you can build on the misconverted callee that\ntreats only NULL specially hence pass NULL instead of \"\". Either\none is a valid implementation, but the reason to choose the former\nwould not be to \"make it resemble the original\".\n\nIOW, \"doesn't seem like a worthwhile goal\" is beating a strawman.\nNobody advocates \"if (!hint || !*hint)\" because \"the code must look\nexactly like the original\". It is merely that it is one known way\nto achieve correctness, as it was how the original achieved its\ncorrectness.\n\n> I'd go in the opposite direction and assert() that the argument is not\n> an empty string.\n\nIf we were writing this program from scratch, \"NULL means something\ndifferent from any sttring, and an empty string does not have\nanything special compared to any other strings\" would probably be a\ngood semantics to give to this helper function. I'd be OK with the\nchange. Also, adding \"if (!*hint) BUG(...)\" would be a good future\ndirection, I would think.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index cc9821ece2..d15a7409d8 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -6182,7 +6182,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n> \t\tgoto cleanup;\n> \n> -\tif (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\n> +\tif (require_clean_work_tree(r, \"rebase\", NULL, 1, 1))\n> \t\tgoto cleanup;\n> \n> \ttodo_list_write_total_nr(&new_todo);\n"},{"id":"480345","messageId":"xmqqcyzx6ma1.fsf@gitster.g","threadId":"59457","inReplyTo":"20230807170935.2336730-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH v2] sequencer: rectify empty hint in call of require_clean_work_tree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-09T01:24:06Z","receivedAt":"2023-08-09T01:24:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> The canonical way to represent \"no error hint\" is making it null, which\n> shortcuts the error() call altogether. This fixes the output by removing\n> the line which said just \"error:\".\n\nSorry, but I forgot to point out a rather obvious thing in my\nreview. We would want to see a reproduction recipe described here\nin the proposed log message at least. Even better is an addition to\nan existing test to ensure that there is no such empty \"error:\"\nline, which will make sure that we will notice when anybody by\nmistake (this includes a mismerge of other topics) breaks this fix.\n\nThanks.\n\n> sequencer.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index cc9821ece2..d15a7409d8 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -6182,7 +6182,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n> \tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n> \t\tgoto cleanup;\n> \n> -\tif (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\n> +\tif (require_clean_work_tree(r, \"rebase\", NULL, 1, 1))\n> \t\tgoto cleanup;\n> \n> \ttodo_list_write_total_nr(&new_todo);\n"},{"id":"480375","messageId":"20230809171531.2564829-1-oswald.buddenhagen@gmx.de","threadId":"59457","inReplyTo":"xmqqa5v2ehba.fsf@gitster.g","subject":"[PATCH v3] sequencer: rectify empty hint in call of require_clean_work_tree()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-09T17:15:31Z","receivedAt":"2023-08-09T17:15:47Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"The canonical way to represent \"no error hint\" is making it NULL, which\nshortcuts the error() call altogether. This fixes the output by removing\nthe line which said just \"error:\", which would appear when starting a\nrebase whose initial checkout worked fine despite a dirty worktree. This\nwas introduced by 97e1873 (rebase -i: rewrite complete_action() in C,\n2018-08-28), which did a somewhat inaccurate conversion from shell.\n\nTo avoid that such bugs re-appear, test for the condition in\nrequire_clean_work_tree().\n\nSigned-off-by: Oswald Buddenhagen \n---\nv3:\n- added BUG()\n- rewrote commit message again\nv2:\n- expanded commit message\n\nCc: Junio C Hamano \n---\n sequencer.c | 2 +-\n wt-status.c | 6 +++++-\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex cc9821ece2..d15a7409d8 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -6182,7 +6182,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n \t\tgoto cleanup;\n \n-\tif (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\n+\tif (require_clean_work_tree(r, \"rebase\", NULL, 1, 1))\n \t\tgoto cleanup;\n \n \ttodo_list_write_total_nr(&new_todo);\ndiff --git a/wt-status.c b/wt-status.c\nindex 8a1a4fb1f0..c8c1780566 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -2654,8 +2654,12 @@ int require_clean_work_tree(struct repository *r,\n \t}\n \n \tif (err) {\n-\t\tif (hint)\n+\t\tif (hint) {\n+\t\t\tif (!*hint)\n+\t\t\t\tBUG(\"empty hint passed to require_clean_work_tree();\"\n+\t\t\t\t \" use NULL instead\");\n \t\t\terror(\"%s\", hint);\n+\t\t}\n \t\tif (!gently)\n \t\t\texit(128);\n \t}\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"480408","messageId":"xmqq7cq3lwli.fsf@gitster.g","threadId":"59457","inReplyTo":"20230809171531.2564829-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH v3] sequencer: rectify empty hint in call of require_clean_work_tree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-09T21:44:25Z","receivedAt":"2023-08-09T21:44:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> The canonical way to represent \"no error hint\" is making it NULL, which\n> shortcuts the error() call altogether. This fixes the output by removing\n> the line which said just \"error:\", which would appear when starting a\n> rebase whose initial checkout worked fine despite a dirty worktree. This\n\nThanks for adding this info. \n\n\"git rebase\" does not seem to start (i.e. does not perform \"initial\ncheckout\") from a dirty working tree, with\n\n\terror: cannot rebase: You have unstaged changes.\n\terror: Please commit or stash them.\n\nat least from my quick attempt. I am not sure if this is actually\ntriggerble?\n\nIn any case, I've replaced v2 that I had with this version, as the\ndescription is much better. The change to the code does look\ncorrect but now it does not seem to trigger, it is unclear to me if\nthe fix is merely theoretical (a command-by-command transcript would\nhave helped here).\n\nThanks.\n\n"},{"id":"480432","messageId":"ZNTI6xpXIRaoFlxx@ugly","threadId":"59457","inReplyTo":"xmqq7cq3lwli.fsf@gitster.g","subject":"Re: [PATCH v3] sequencer: rectify empty hint in call of require_clean_work_tree()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-10T11:24:27Z","receivedAt":"2023-08-10T11:24:33Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Wed, Aug 09, 2023 at 02:44:25PM -0700, Junio C Hamano wrote:\n>\"git rebase\" does not seem to start (i.e. does not perform \"initial\n>checkout\") from a dirty working tree, with\n>\n>\terror: cannot rebase: You have unstaged changes.\n>\terror: Please commit or stash them.\n>\n>at least from my quick attempt. I am not sure if this is actually\n>triggerble?\n>\nhmm, unless i'm missing something, it is apparently not ... currently.\n\nit becomes actually relevant only after my \"rebase: improve resumption \nfrom incorrect initial todo list\" patch, which still needs reworking.\n\nso there are several possibilities how to proceed with this:\n- you merge it as a merely theoretical fix (adjust the commit message \n yourself if saving the round-trip is more convenient for you)\n- we postpone the change until it becomes not theoretical\n- we make sure that the code really is not triggerable and delete it. \n this would obviously introduce some churn, as i'll need to \n re-introduce it. but then, i remember that something was wrong with it \n anyway (i.e., it didn't actually trigger in some cases i expected it \n to, but i haven't figured out yet why), so the new code would be \n slightly different anyway.\n\nregards\n"},{"id":"480444","messageId":"xmqqedkahoio.fsf@gitster.g","threadId":"59457","inReplyTo":"ZNTI6xpXIRaoFlxx@ugly","subject":"Re: [PATCH v3] sequencer: rectify empty hint in call of require_clean_work_tree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-10T16:04:47Z","receivedAt":"2023-08-10T16:10:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> it becomes actually relevant only after my \"rebase: improve resumption\n> from incorrect initial todo list\" patch, which still needs reworking.\n\nWe are already past -rc1 by the time you see this response, so this\nfix is not of immediate urgency. It has been with us for more than\nseveral cycles, it does not seem to be readily triggerable, and its\neffect is merely a single extra empty error message when other things\nare mentioned there.\n\nEven if it may not be triggerable, consistent use of the API is\nsomething that is worth aiming for. \n\nIf I were working on a separate change (i.e. your \"resume from bad\ninitial todo\") that will make this triggerable, I would explain this\npatch as a pure clean-up patch to use the API function consistently,\nand tell readers that there is currently no way to trigger it\n(assuming that it is the case---I only poked a bit yesterday and\nwould not claim that I did a thorough investigation), and that it\nwill start mattering with a later step in the series. And make it\nan early part of the series that will contain the \"resume from bad\ninitial todo\" patch.\n\nThanks.\n\n"},{"id":"480995","messageId":"20230824150046.802008-1-oswald.buddenhagen@gmx.de","threadId":"59457","inReplyTo":"ZNTI6xpXIRaoFlxx@ugly","subject":"[PATCH v4] sequencer: rectify empty hint in call of require_clean_work_tree()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-24T15:00:46Z","receivedAt":"2023-08-24T15:01:31Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"The canonical way to represent \"no error hint\" is making it NULL, which\nshortcuts the error() call altogether. This fixes the output by removing\nthe line which said just \"error:\", which would appear when the worktree\nis dirtied while editing the initial rebase todo file. This was\nintroduced by 97e1873 (rebase -i: rewrite complete_action() in C,\n2018-08-28), which did a somewhat inaccurate conversion from shell.\n\nTo avoid that such bugs re-appear, test for the condition in\nrequire_clean_work_tree().\n\nSigned-off-by: Oswald Buddenhagen \n\n---\nyeah, so i _did_ miss something.\n\nv4:\n- fixed reproduction instructions\nv3:\n- added BUG()\n- rewrote commit message again\nv2:\n- expanded commit message\n\nCc: Junio C Hamano \n---\n sequencer.c | 2 +-\n wt-status.c | 6 +++++-\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex cc9821ece2..d15a7409d8 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -6182,7 +6182,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \tif (checkout_onto(r, opts, onto_name, &oid, orig_head))\n \t\tgoto cleanup;\n \n-\tif (require_clean_work_tree(r, \"rebase\", \"\", 1, 1))\n+\tif (require_clean_work_tree(r, \"rebase\", NULL, 1, 1))\n \t\tgoto cleanup;\n \n \ttodo_list_write_total_nr(&new_todo);\ndiff --git a/wt-status.c b/wt-status.c\nindex 8a1a4fb1f0..c8c1780566 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -2654,8 +2654,12 @@ int require_clean_work_tree(struct repository *r,\n \t}\n \n \tif (err) {\n-\t\tif (hint)\n+\t\tif (hint) {\n+\t\t\tif (!*hint)\n+\t\t\t\tBUG(\"empty hint passed to require_clean_work_tree();\"\n+\t\t\t\t \" use NULL instead\");\n \t\t\terror(\"%s\", hint);\n+\t\t}\n \t\tif (!gently)\n \t\t\texit(128);\n \t}\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"480999","messageId":"xmqqr0nsbfe9.fsf@gitster.g","threadId":"59457","inReplyTo":"20230824150046.802008-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH v4] sequencer: rectify empty hint in call of require_clean_work_tree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-24T15:59:10Z","receivedAt":"2023-08-24T16:00:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> The canonical way to represent \"no error hint\" is making it NULL, which\n> shortcuts the error() call altogether. This fixes the output by removing\n> the line which said just \"error:\", which would appear when the worktree\n> is dirtied while editing the initial rebase todo file.\n> ...\n> v4:\n> - fixed reproduction instructions\n\nNice. Will queue and mark it for 'next'. Thanks.\n"}]}