Re: Bug report: Incorrect GIT_FLUSH behavior in 2.43.1 (regression and breaking)

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Feb 13, 2024, 11:07 UTC
Message-ID: <c243c260-b346-4b53-b8a2-685389ad344e@gmail.com>
In-Reply-To: <xmqq8r3p7glr.fsf@gitster.g>

On 12/02/2024 17:11, Junio C Hamano wrote:

Show 41 quoted lines

> Xiaoguang WANG <wxiaoguang@gmail.com> writes:
> 
>> If GIT_FLUSH=true, it should mean to "do the flush". But that commit
>> made skip_stdout_flush=true when GIT_FLUSH=true.
> 
> Thanks for reporting.  I am surprised that this flipping of polarity
> slipped through.
> 
>> And by the way, only accepting GIT_FLUSH=true is quite breaking, it
>> drops the compatibility of GIT_FLUSH=1
> 
> I do not think so. If the polarity is corrected, git_env_bool()
> would say "that's affirmative" when any one of the "1", "true",
> "yes", "on", etc. is given.  If you have been passing "1", you
> should get the "always flush" behaviour.
> 
> 
> Perhaps something like this would fix it?
> 
> 
>   write-or-die.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git c/write-or-die.c w/write-or-die.c
> index 3942152865..3ecb9e2af5 100644
> --- c/write-or-die.c
> +++ w/write-or-die.c
> @@ -22,8 +22,11 @@ void maybe_flush_or_die(FILE *f, const char *desc)
>   
>   	if (f == stdout) {
>   		if (skip_stdout_flush < 0) {
> -			skip_stdout_flush = git_env_bool("GIT_FLUSH", -1);
> -			if (skip_stdout_flush < 0) {
> +			int flush_setting = git_env_bool("GIT_FLUSH", -1);
> +
> +			if (0 <= flush_setting)
> +				skip_stdout_flush = !flush_setting;
> +			else {
>   				struct stat st;
>   				if (fstat(fileno(stdout), &st))
>   					skip_stdout_flush = 0;

Given we're in a rc-period a minimal fix like this looks appropriate (though it is missing some braces according to our coding guidelines). The interaction of "skip_stdout_flush" and git_env_bool() is unfortunate, It might be clearer if we changed to having "force_stdout_flush" instead but that would be a more invasive change.

Best Wishes

Phillip

diff --git a/write-or-die.c b/write-or-die.c
index 39421528653..e68265a94a6 100644
--- a/write-or-die.c
+++ b/write-or-die.c
@@ -18,20 +18,20 @@
   */
  void maybe_flush_or_die(FILE *f, const char *desc)
  {
-        static int skip_stdout_flush = -1;
+        static int force_stdout_flush = -1;
  
          if (f == stdout) {
-                if (skip_stdout_flush < 0) {
-                        skip_stdout_flush = git_env_bool("GIT_FLUSH", -1);
-                        if (skip_stdout_flush < 0) {
+                if (force_stdout_flush < 0) {
+                        force_stdout_flush = git_env_bool("GIT_FLUSH", -1);
+                        if (force_stdout_flush < 0) {
                                  struct stat st;
                                  if (fstat(fileno(stdout), &st))
-                                        skip_stdout_flush = 0;
+                                        force_stdout_flush = 1;
                                  else
-                                        skip_stdout_flush = S_ISREG(st.st_mode);
+                                        force_stdout_flush = !S_ISREG(st.st_mode);
                          }
                  }
-                if (skip_stdout_flush && !ferror(f))
+                if (!force_stdout_flush && !ferror(f))
                          return;
          }
          if (fflush(f)) {