Subject: Re: [PATCH 2/2] server-info.c: remove temporary info files on exit

From: Junio C Hamano <[email-masked]>
Date: Fri, 07 Jun 2024 09:02:14 -0700

Taylor Blau <[email-masked]> writes:

> @@ -121,27 +120,22 @@ static int update_info_file(char *path,
>  	}
>  
>  	uic.cur_fp = NULL;
> -	if (fclose(to_close))
> -		goto out;

We should fflush() of cur_fp before nuking it, at least, no?

In the original code, to_close was a mere copy of uic.cur_fp and we
made sure that anything buffered at the stdio layer are flushed to
the underlying file desciptor (fd that we obtained from
git_mkstemp_mode() in the original code) with this fclose() call.

We no longer do so.  We later call rename_tempfile() to close the
underlying file descriptor and move the temporary file to its final
place, but I do not see what guarantee we have that we do not lose
what we had buffered in the stdio with the updated code.

>  	if (uic_is_stale(&uic)) {
> -		if (adjust_shared_perm(tmp) < 0)
> +		if (adjust_shared_perm(get_tempfile_path(f)) < 0)
>  			goto out;
> -		if (rename(tmp, path) < 0)
> +		if (rename_tempfile(&f, path) < 0)
>  			goto out;
>  	} else {
> -		unlink(tmp);
> +		delete_tempfile(&f);
>  	}
>  	ret = 0;
>  
>  out:
>  	if (ret) {
>  		error_errno("unable to update %s", path);
> -		if (uic.cur_fp)
> -			fclose(uic.cur_fp);
> -		else if (fd >= 0)
> -			close(fd);
> -		unlink(tmp);
> +		if (f)
> +			delete_tempfile(&f);
>  	}
>  	free(tmp);
>  	if (uic.old_fp)