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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Thu, 9 Aug 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > 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.

I disagree that it is a distraction, because the commit messages are the
very location where I am supposed to detail my rationale, motivation, and
considerations that are not obvious from the diff alone.

Of course, I cannot force you to take this commit message, you can
overrule me and edit it. But you would do this against my express wish.

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

It can be an arbitrary range, but it does not matter at this point because
we expect only appending callers. Therefore any range will do, as long as
it covers the range of bytes to be written by the trace functions. And
with 0-0xffffffff, I am fairly certain we got it.

Technically, we could even lock the range 0-1, as all of our callers would
agree on that, and block each other. Other Git implementations might not,
though. So 0-0xffffffff is my best bet and cheap.

> Would it be sufficient to cover just the first one byte instead?

As I said, this would depend on no other software trying to append to the
trace file, including alternative Git implementations.

In other words: it would be "too clever". Clever, but asking for problems.

> Or perhaps give l_len==0 to cover all no matter how large the file
> grows, which sounds like a better range specification.

I must have overlooked that option in the documentation.

*clicketyclick*

Indeed,
http://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html
spells it out pretty clearly:

> A lock shall be set to extend to the largest possible value of the file
> offset for that file by setting l_len to 0. If such a lock also has
> l_start set to 0 and l_whence is set to SEEK_SET, the whole file shall
> be locked.

Sorry about missing this. I will change the implementation to this.

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

Thanks for helping me improve the patch,
Dscho