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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 10, 2018, 20:05 UTC
Message-ID: <xmqqzhxu6l81.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <a19904682f6984de21fbf77240ff5fb6eed1b744.1533930437.git.gitgitgadget@gmail.com>

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

Show 16 quoted lines

> +
> +#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;
> +
> +	/* (un-)lock the whole file */
> +	flock.l_whence = SEEK_SET;
> +	flock.l_start = 0;
> +	flock.l_len = 0;
> +
> +	return fcntl(fd, F_SETLKW, &flock);
> +}
> +#endif

I think people already told you that this is not needed on systems with properly working O_APPEND [*1*]

	Side note #1: and with network filesystems where O_APPEND
        may not work reliably, fcntl based range locking would not
        work either, so having this would not help.

I saw other Johannes and other Jeff peeking into fixing O_APPEND; I do not know how well that effort goes, but it would be preferrable if we can successfully go that route.

As I said in my review of the first patch in v1 series, I am not fundamentally opposed to a few "lock here to work around lack of O_APPEND" and "unlock here for the same reason" calls to limited codepaths as a workaround, as the damage is limited (that is why I earlier looked at our use of O_APPEND), but that would be the last resort if O_APPEND cannot be made to work reliably on Windows.

But even if we end up doing so, on systems with POSIX O_APPEND working, I think that function should be

    #define lock_or_unlock_for_appending(fd, lock) 0 /* nothing to do */