Re: [PATCH 1/4] Introduce a function to lock/unlock file descriptors when appending

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 9, 2018, 19:01 UTC
Message-ID: <xmqqtvo3bc0g.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <e449ed75fe3705692175017f98438815aeccf0fb.1533836122.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 16 quoted lines

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> This function will be used to make write accesses in trace_write() a bit
> safer.
> ...
> To set a precedent for a better approach, let's introduce a proper
> abstraction: a function that says in its name precisely what Git
> wants it to do (as opposed to *how* it does it on Linux):
> lock_or_unlock_fd_for_appending().
>
> The next commit will provide a Windows-specific implementation of this
> function/functionality.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> squash! Introduce a function to lock/unlock file descriptors when appending

If we can keep the custom, narrow and easy-to-port API (like this patch introduces) focused and resist feature creep over time, then it would be worth spending effort to come up with such a custom helper that is easy to port. So I agree with the approach in general but I tend to think "set a precedent for a better approach" is a way-too-early and wishful verdict. We do not know if we can really keep that custom API easy-to-port-and-maintain yet.

In short, even though I agree with the approach, most of the verbiage above is unnecessary and mere distraction.

Show 36 quoted lines

> ---
>  git-compat-util.h |  2 ++
>  wrapper.c         | 14 ++++++++++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 9a64998b2..13b83bade 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -1202,6 +1202,8 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
>  #define getc_unlocked(fh) getc(fh)
>  #endif
>  
> +extern int lock_or_unlock_fd_for_appending(int fd, int lock_it);
> +
>  /*
>   * Our code often opens a path to an optional file, to work on its
>   * contents when we can successfully open it.  We can ignore a failure
> diff --git a/wrapper.c b/wrapper.c
> index e4fa9d84c..6c2116272 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -690,3 +690,17 @@ int xgethostname(char *buf, size_t len)
>  		buf[len - 1] = 0;
>  	return ret;
>  }
> +
> +#ifndef GIT_WINDOWS_NATIVE
> +int lock_or_unlock_fd_for_appending(int fd, int lock_it)
> +{
> +	struct flock flock;
> +
> +	flock.l_type = lock_it ? F_WRLCK : F_UNLCK;
> +	flock.l_whence = SEEK_SET;
> +	flock.l_start = 0;
> +	flock.l_len = 0xffffffff; /* arbitrary number of bytes */

If this can be an arbitrary range, do we need to cover this many (or only this few, depending on your point of view) bytes?

Would it be sufficient to cover just the first one byte instead? Or perhaps give l_len==0 to cover all no matter how large the file grows, which sounds like a better range specification.

> +	return fcntl(fd, F_SETLKW, &flock);
> +}
> +#endif