Re: [PATCH v7 02/14] csum-file: refactor finalize_hashfile() method

From: Jakub Narebski <jnareb@gmail.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com, sbeller@google.com,
	szeder.dev@gmail.com, ramsay@ramsayjones.plus.com,
	git@jeffhostetler.com, peff@peff.net,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH v7 02/14] csum-file: refactor finalize_hashfile() method
Date: Sun, 08 Apr 2018 00:59:01 +0200	[thread overview]
Message-ID: <86lgdyei8a.fsf@gmail.com> (raw)
In-Reply-To: <20180402203427.170177-3-dstolee@microsoft.com> (Derrick Stolee's message of "Mon, 2 Apr 2018 16:34:15 -0400")

Derrick Stolee <stolee@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> If we want to use a hashfile on the temporary file for a lockfile, then
> we need finalize_hashfile() to fully write the trailing hash but also keep
> the file descriptor open.
>
> Do this by adding a new CSUM_HASH_IN_STREAM flag along with a functional
> change that checks this flag before writing the checksum to the stream.
> This differs from previous behavior since it would be written if either
> CSUM_CLOSE or CSUM_FSYNC is provided.

I'm sorry, but I don't understand from this description what this flag
does and what it is meant to do.

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---

[...]
> diff --git a/csum-file.h b/csum-file.h
> index 9ba87f0a6c..c5a2e335e7 100644
> --- a/csum-file.h
> +++ b/csum-file.h
> @@ -27,8 +27,9 @@ extern void hashfile_checkpoint(struct hashfile *, struct hashfile_checkpoint *)
>  extern int hashfile_truncate(struct hashfile *, struct hashfile_checkpoint *);
>  
>  /* finalize_hashfile flags */
> -#define CSUM_CLOSE	1
> -#define CSUM_FSYNC	2
> +#define CSUM_CLOSE		1
> +#define CSUM_FSYNC		2
> +#define CSUM_HASH_IN_STREAM	4

Especially that it is not commented / described here, and the name is
unsufficiently descriptive for me.

[...]
> diff --git a/csum-file.c b/csum-file.c
> index e6c95a6915..53ce37f7ca 100644
> --- a/csum-file.c
> +++ b/csum-file.c
> @@ -61,11 +61,11 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result, unsigned int fl
>  	the_hash_algo->final_fn(f->buffer, &f->ctx);
>  	if (result)
>  		hashcpy(result, f->buffer);
> -	if (flags & (CSUM_CLOSE | CSUM_FSYNC)) {
> -		/* write checksum and close fd */
> +	if (flags & CSUM_HASH_IN_STREAM)
>  		flush(f, f->buffer, the_hash_algo->rawsz);

Wouldn't CSUM_FLUSH be a better name for this flag?

> -		if (flags & CSUM_FSYNC)
> -			fsync_or_die(f->fd, f->name);
> +	if (flags & CSUM_FSYNC)
> +		fsync_or_die(f->fd, f->name);
> +	if (flags & CSUM_CLOSE) {
>  		if (close(f->fd))
>  			die_errno("%s: sha1 file error on close", f->name);
>  		fd = 0;

--
Jakub Narębski