{"thread":{"id":"60903","subject":"Bug report: Incorrect GIT_FLUSH behavior in 2.43.1 (regression and breaking)","startedAt":"2024-02-12T15:18:50Z","lastAt":"2024-02-14T10:57:40Z","messageCount":7,"participants":["Xiaoguang WANG","Junio C Hamano","Phillip Wood"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"488445","messageId":"CABn0oJvg3M_kBW-u=j3QhKnO=6QOzk-YFTgonYw_UvFS1NTX4g@mail.gmail.com","threadId":"60903","inReplyTo":null,"subject":"Bug report: Incorrect GIT_FLUSH behavior in 2.43.1 (regression and breaking)","fromName":"Xiaoguang WANG","fromEmail":"wxiaoguang@gmail.com","sentAt":"2024-02-12T15:18:38Z","receivedAt":"2024-02-12T15:18:50Z","isPatch":false,"sender":{"key":"wxiaoguang@gmail.com","avatar":null},"body":"Ref: https://github.com/git/git/commit/556e68032f8248c831e48207e5cb923c9fe0e42c\n\nIf GIT_FLUSH=true, it should mean to \"do the flush\". But that commit\nmade skip_stdout_flush=true when GIT_FLUSH=true.\n\nAnd by the way, only accepting GIT_FLUSH=true is quite breaking, it\ndrops the compatibility of GIT_FLUSH=1: it causes the existing\nprograms which depend on the \"flushing(GIT_FLUSH=1)\" behavior would\nhang forever if they use the new git binary, because the program won't\nsee any flushed output ....\n"},{"id":"488456","messageId":"xmqq8r3p7glr.fsf@gitster.g","threadId":"60903","inReplyTo":"CABn0oJvg3M_kBW-u=j3QhKnO=6QOzk-YFTgonYw_UvFS1NTX4g@mail.gmail.com","subject":"Re: Bug report: Incorrect GIT_FLUSH behavior in 2.43.1 (regression and breaking)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-12T17:11:12Z","receivedAt":"2024-02-12T17:11:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Xiaoguang WANG writes:\n\n> If GIT_FLUSH=true, it should mean to \"do the flush\". But that commit\n> made skip_stdout_flush=true when GIT_FLUSH=true.\n\nThanks for reporting. I am surprised that this flipping of polarity\nslipped through.\n\n> And by the way, only accepting GIT_FLUSH=true is quite breaking, it\n> drops the compatibility of GIT_FLUSH=1\n\nI do not think so. If the polarity is corrected, git_env_bool()\nwould say \"that's affirmative\" when any one of the \"1\", \"true\",\n\"yes\", \"on\", etc. is given. If you have been passing \"1\", you\nshould get the \"always flush\" behaviour.\n\n\nPerhaps something like this would fix it?\n\n\n write-or-die.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git c/write-or-die.c w/write-or-die.c\nindex 3942152865..3ecb9e2af5 100644\n--- c/write-or-die.c\n+++ w/write-or-die.c\n@@ -22,8 +22,11 @@ void maybe_flush_or_die(FILE *f, const char *desc)\n \n \tif (f == stdout) {\n \t\tif (skip_stdout_flush < 0) {\n-\t\t\tskip_stdout_flush = git_env_bool(\"GIT_FLUSH\", -1);\n-\t\t\tif (skip_stdout_flush < 0) {\n+\t\t\tint flush_setting = git_env_bool(\"GIT_FLUSH\", -1);\n+\n+\t\t\tif (0 <= flush_setting)\n+\t\t\t\tskip_stdout_flush = !flush_setting;\n+\t\t\telse {\n \t\t\t\tstruct stat st;\n \t\t\t\tif (fstat(fileno(stdout), &st))\n \t\t\t\t\tskip_stdout_flush = 0;\n"},{"id":"488457","messageId":"CABn0oJupUjbu2k0adFqcjryj6=XtmwQzb0vMGtYan8WccPtYxA@mail.gmail.com","threadId":"60903","inReplyTo":"xmqq8r3p7glr.fsf@gitster.g","subject":"Re: Bug report: Incorrect GIT_FLUSH behavior in 2.43.1 (regression and breaking)","fromName":"Xiaoguang WANG","fromEmail":"wxiaoguang@gmail.com","sentAt":"2024-02-12T17:18:18Z","receivedAt":"2024-02-12T17:18:30Z","isPatch":false,"sender":{"key":"wxiaoguang@gmail.com","avatar":null},"body":"On Tue, Feb 13, 2024 at 1:11 AM Junio C Hamano wrote:\n\n> > And by the way, only accepting GIT_FLUSH=true is quite breaking, it\n> > drops the compatibility of GIT_FLUSH=1\n>\n> I do not think so. If the polarity is corrected, git_env_bool()\n> would say \"that's affirmative\" when any one of the \"1\", \"true\",\n> \"yes\", \"on\", etc. is given. If you have been passing \"1\", you\n> should get the \"always flush\" behaviour.\n\nOh yes, you are right. There is a git_parse_int call in\ngit_parse_maybe_bool, so if the \"flipping\" could be fixed, then\neverything should be fine.\n"},{"id":"488526","messageId":"c243c260-b346-4b53-b8a2-685389ad344e@gmail.com","threadId":"60903","inReplyTo":"xmqq8r3p7glr.fsf@gitster.g","subject":"Re: Bug report: Incorrect GIT_FLUSH behavior in 2.43.1 (regression and breaking)","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-13T11:07:58Z","receivedAt":"2024-02-13T11:08:01Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 12/02/2024 17:11, Junio C Hamano wrote:\n> Xiaoguang WANG writes:\n> \n>> If GIT_FLUSH=true, it should mean to \"do the flush\". But that commit\n>> made skip_stdout_flush=true when GIT_FLUSH=true.\n> \n> Thanks for reporting. I am surprised that this flipping of polarity\n> slipped through.\n> \n>> And by the way, only accepting GIT_FLUSH=true is quite breaking, it\n>> drops the compatibility of GIT_FLUSH=1\n> \n> I do not think so. If the polarity is corrected, git_env_bool()\n> would say \"that's affirmative\" when any one of the \"1\", \"true\",\n> \"yes\", \"on\", etc. is given. If you have been passing \"1\", you\n> should get the \"always flush\" behaviour.\n> \n> \n> Perhaps something like this would fix it?\n> \n> \n> write-or-die.c | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git c/write-or-die.c w/write-or-die.c\n> index 3942152865..3ecb9e2af5 100644\n> --- c/write-or-die.c\n> +++ w/write-or-die.c\n> @@ -22,8 +22,11 @@ void maybe_flush_or_die(FILE *f, const char *desc)\n> \n> \tif (f == stdout) {\n> \t\tif (skip_stdout_flush < 0) {\n> -\t\t\tskip_stdout_flush = git_env_bool(\"GIT_FLUSH\", -1);\n> -\t\t\tif (skip_stdout_flush < 0) {\n> +\t\t\tint flush_setting = git_env_bool(\"GIT_FLUSH\", -1);\n> +\n> +\t\t\tif (0 <= flush_setting)\n> +\t\t\t\tskip_stdout_flush = !flush_setting;\n> +\t\t\telse {\n> \t\t\t\tstruct stat st;\n> \t\t\t\tif (fstat(fileno(stdout), &st))\n> \t\t\t\t\tskip_stdout_flush = 0;\n\nGiven we're in a rc-period a minimal fix like this looks appropriate\n(though it is missing some braces according to our coding\nguidelines). The interaction of \"skip_stdout_flush\" and git_env_bool()\nis unfortunate, It might be clearer if we changed to having\n\"force_stdout_flush\" instead but that would be a more invasive change.\n\nBest Wishes\n\nPhillip\n\ndiff --git a/write-or-die.c b/write-or-die.c\nindex 39421528653..e68265a94a6 100644\n--- a/write-or-die.c\n+++ b/write-or-die.c\n@@ -18,20 +18,20 @@\n */\n void maybe_flush_or_die(FILE *f, const char *desc)\n {\n- static int skip_stdout_flush = -1;\n+ static int force_stdout_flush = -1;\n \n if (f == stdout) {\n- if (skip_stdout_flush < 0) {\n- skip_stdout_flush = git_env_bool(\"GIT_FLUSH\", -1);\n- if (skip_stdout_flush < 0) {\n+ if (force_stdout_flush < 0) {\n+ force_stdout_flush = git_env_bool(\"GIT_FLUSH\", -1);\n+ if (force_stdout_flush < 0) {\n struct stat st;\n if (fstat(fileno(stdout), &st))\n- skip_stdout_flush = 0;\n+ force_stdout_flush = 1;\n else\n- skip_stdout_flush = S_ISREG(st.st_mode);\n+ force_stdout_flush = !S_ISREG(st.st_mode);\n }\n }\n- if (skip_stdout_flush && !ferror(f))\n+ if (!force_stdout_flush && !ferror(f))\n return;\n }\n if (fflush(f)) {\n\n \n\n"},{"id":"488546","messageId":"xmqqle7o6zs8.fsf@gitster.g","threadId":"60903","inReplyTo":"c243c260-b346-4b53-b8a2-685389ad344e@gmail.com","subject":"Re: Bug report: Incorrect GIT_FLUSH behavior in 2.43.1 (regression and breaking)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-13T17:26:47Z","receivedAt":"2024-02-13T17:26:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Given we're in a rc-period a minimal fix like this looks appropriate\n> (though it is missing some braces according to our coding\n> guidelines). The interaction of \"skip_stdout_flush\" and git_env_bool()\n> is unfortunate, It might be clearer if we changed to having\n> \"force_stdout_flush\" instead but that would be a more invasive change.\n\nI admit that I did find the polarity of the existing variable\nannoying, and it does make sense to flip it like you did here.\n\nUnfortunately the minimum fix is already in 'next', so let me turn\nwhat you wrote into an update relative to that. I'll assume your\npatch in the discussion is signed-off already?\n\n------- >8 ------------- >8 ------------- >8 -------\nFrom: Phillip Wood \nSubject: maybe_flush_or_die(): flip the polarity of an internal variable\n\nWe take GIT_FLUSH that tells us if we want to flush (or not) from\nthe outside, but internally use a variable that tells us to skip (or\nnot) the flushing operation, which makes the code flow unnecessarily\nconfusing to read.\n\nWith the understanding of the original motivation behind \"skip\" in\n06f59e9f (Don't fflush(stdout) when it's not helpful, 2007-06-29),\nwe can sympathize with the current naming (we wanted to avoid\nuseless flushing of stdout by default, with an escape hatch to\nalways flush), but it is still not a good excuse.\n\nRetire the \"skip_stdout_flush\" variable and replace it with \"flush_stdout\"\nthat tells if we do or do not want to run fflush().\n\nSigned-off-by: Junio C Hamano \n---\n write-or-die.c | 17 +++++++----------\n 1 file changed, 7 insertions(+), 10 deletions(-)\n\ndiff --git c/write-or-die.c w/write-or-die.c\nindex 3ecb9e2af5..01a9a51fa2 100644\n--- c/write-or-die.c\n+++ w/write-or-die.c\n@@ -18,23 +18,20 @@\n */\n void maybe_flush_or_die(FILE *f, const char *desc)\n {\n-\tstatic int skip_stdout_flush = -1;\n-\n \tif (f == stdout) {\n-\t\tif (skip_stdout_flush < 0) {\n-\t\t\tint flush_setting = git_env_bool(\"GIT_FLUSH\", -1);\n+\t\tstatic int force_flush_stdout = -1;\n \n-\t\t\tif (0 <= flush_setting)\n-\t\t\t\tskip_stdout_flush = !flush_setting;\n-\t\t\telse {\n+\t\tif (force_flush_stdout < 0) {\n+\t\t\tforce_flush_stdout = git_env_bool(\"GIT_FLUSH\", -1);\n+\t\t\tif (force_flush_stdout < 0) {\n \t\t\t\tstruct stat st;\n \t\t\t\tif (fstat(fileno(stdout), &st))\n-\t\t\t\t\tskip_stdout_flush = 0;\n+\t\t\t\t\tforce_flush_stdout = 1;\n \t\t\t\telse\n-\t\t\t\t\tskip_stdout_flush = S_ISREG(st.st_mode);\n+\t\t\t\t\tforce_flush_stdout = !S_ISREG(st.st_mode);\n \t\t\t}\n \t\t}\n-\t\tif (skip_stdout_flush && !ferror(f))\n+\t\tif (!force_flush_stdout && !ferror(f))\n \t\t\treturn;\n \t}\n \tif (fflush(f)) {\n"},{"id":"488568","messageId":"xmqqbk8k5eo0.fsf@gitster.g","threadId":"60903","inReplyTo":"xmqqle7o6zs8.fsf@gitster.g","subject":"Re: Bug report: Incorrect GIT_FLUSH behavior in 2.43.1 (regression and breaking)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-13T19:48:15Z","receivedAt":"2024-02-13T19:48:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Unfortunately the minimum fix is already in 'next', so let me turn\n> what you wrote into an update relative to that. I'll assume your\n> patch in the discussion is signed-off already?\n\nNah, my mistake. The topic still is outside 'next', so I'll replace\nit with the attached while queuing.\n\nThanks.\n\n------- >8 ------------- >8 ------------- >8 -------\nSubject: [PATCH] write-or-die: fix the polarity of GIT_FLUSH environment variable\n\nWhen GIT_FLUSH is set to 1, true, on, yes, then we should disable\nskip_stdout_flush, but the conversion somehow did the opposite.\n\nWith the understanding of the original motivation behind \"skip\" in\n06f59e9f (Don't fflush(stdout) when it's not helpful, 2007-06-29),\nwe can sympathize with the current naming (we wanted to avoid\nuseless flushing of stdout by default, with an escape hatch to\nalways flush), but it is still not a good excuse.\n\nRetire the \"skip_stdout_flush\" variable and replace it with \"flush_stdout\"\nthat tells if we do or do not want to run fflush().\n\nReported-by: Xiaoguang WANG \nHelped-by: Phillip Wood \nSigned-off-by: Junio C Hamano \n---\n write-or-die.c | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/write-or-die.c b/write-or-die.c\nindex 3942152865..01a9a51fa2 100644\n--- a/write-or-die.c\n+++ b/write-or-die.c\n@@ -18,20 +18,20 @@\n */\n void maybe_flush_or_die(FILE *f, const char *desc)\n {\n-\tstatic int skip_stdout_flush = -1;\n-\n \tif (f == stdout) {\n-\t\tif (skip_stdout_flush < 0) {\n-\t\t\tskip_stdout_flush = git_env_bool(\"GIT_FLUSH\", -1);\n-\t\t\tif (skip_stdout_flush < 0) {\n+\t\tstatic int force_flush_stdout = -1;\n+\n+\t\tif (force_flush_stdout < 0) {\n+\t\t\tforce_flush_stdout = git_env_bool(\"GIT_FLUSH\", -1);\n+\t\t\tif (force_flush_stdout < 0) {\n \t\t\t\tstruct stat st;\n \t\t\t\tif (fstat(fileno(stdout), &st))\n-\t\t\t\t\tskip_stdout_flush = 0;\n+\t\t\t\t\tforce_flush_stdout = 1;\n \t\t\t\telse\n-\t\t\t\t\tskip_stdout_flush = S_ISREG(st.st_mode);\n+\t\t\t\t\tforce_flush_stdout = !S_ISREG(st.st_mode);\n \t\t\t}\n \t\t}\n-\t\tif (skip_stdout_flush && !ferror(f))\n+\t\tif (!force_flush_stdout && !ferror(f))\n \t\t\treturn;\n \t}\n \tif (fflush(f)) {\n-- \n2.44.0-rc0-46-g2996f11c1d\n\n"},{"id":"488633","messageId":"bc0bf723-ccde-469d-853e-4a94531ea60b@gmail.com","threadId":"60903","inReplyTo":"xmqqbk8k5eo0.fsf@gitster.g","subject":"Re: Bug report: Incorrect GIT_FLUSH behavior in 2.43.1 (regression and breaking)","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-14T10:57:38Z","receivedAt":"2024-02-14T10:57:40Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 13/02/2024 19:48, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> Unfortunately the minimum fix is already in 'next', so let me turn\n>> what you wrote into an update relative to that. I'll assume your\n>> patch in the discussion is signed-off already?\n> \n> Nah, my mistake. The topic still is outside 'next', so I'll replace\n> it with the attached while queuing.\n >\n> Thanks.\n> \n> ------- >8 ------------- >8 ------------- >8 -------\n> Subject: [PATCH] write-or-die: fix the polarity of GIT_FLUSH environment variable\n> \n> When GIT_FLUSH is set to 1, true, on, yes, then we should disable\n> skip_stdout_flush, but the conversion somehow did the opposite.\n> \n> With the understanding of the original motivation behind \"skip\" in\n> 06f59e9f (Don't fflush(stdout) when it's not helpful, 2007-06-29),\n> we can sympathize with the current naming (we wanted to avoid\n> useless flushing of stdout by default, with an escape hatch to\n> always flush), but it is still not a good excuse.\n> \n> Retire the \"skip_stdout_flush\" variable and replace it with \"flush_stdout\"\n> that tells if we do or do not want to run fflush().\n\nI think the patch looks good and the commit message nicely explains why \nwe want to change the name of the variable.\n\nBest Wishes\n\nPhillip\n\n> Reported-by: Xiaoguang WANG \n> Helped-by: Phillip Wood \n> Signed-off-by: Junio C Hamano \n> ---\n> write-or-die.c | 16 ++++++++--------\n> 1 file changed, 8 insertions(+), 8 deletions(-)\n> \n> diff --git a/write-or-die.c b/write-or-die.c\n> index 3942152865..01a9a51fa2 100644\n> --- a/write-or-die.c\n> +++ b/write-or-die.c\n> @@ -18,20 +18,20 @@\n> */\n> void maybe_flush_or_die(FILE *f, const char *desc)\n> {\n> -\tstatic int skip_stdout_flush = -1;\n> -\n> \tif (f == stdout) {\n> -\t\tif (skip_stdout_flush < 0) {\n> -\t\t\tskip_stdout_flush = git_env_bool(\"GIT_FLUSH\", -1);\n> -\t\t\tif (skip_stdout_flush < 0) {\n> +\t\tstatic int force_flush_stdout = -1;\n> +\n> +\t\tif (force_flush_stdout < 0) {\n> +\t\t\tforce_flush_stdout = git_env_bool(\"GIT_FLUSH\", -1);\n> +\t\t\tif (force_flush_stdout < 0) {\n> \t\t\t\tstruct stat st;\n> \t\t\t\tif (fstat(fileno(stdout), &st))\n> -\t\t\t\t\tskip_stdout_flush = 0;\n> +\t\t\t\t\tforce_flush_stdout = 1;\n> \t\t\t\telse\n> -\t\t\t\t\tskip_stdout_flush = S_ISREG(st.st_mode);\n> +\t\t\t\t\tforce_flush_stdout = !S_ISREG(st.st_mode);\n> \t\t\t}\n> \t\t}\n> -\t\tif (skip_stdout_flush && !ferror(f))\n> +\t\tif (!force_flush_stdout && !ferror(f))\n> \t\t\treturn;\n> \t}\n> \tif (fflush(f)) {\n\n"}]}