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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 7, 2024, 16:02 UTC
Message-ID: <xmqqfrtog2rd.fsf@gitster.g>
In-Reply-To: <2d5a0536af1a6d45835622e2c020266079fa0873.1717712358.git.me@ttaylorr.com>

Taylor Blau <me@ttaylorr.com> writes:

Show 6 quoted lines

> @@ -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.

Show 26 quoted lines

>  	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)