Re: [PATCH v6 02/39] api-lockfile: revise and expand the documentation

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 30, 2014, 17:39 UTC
Message-ID: <xmqqsij9rose.fsf@gitster.dls.corp.google.com>
In-Reply-To: <542A8B90.50507@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 13 quoted lines

> I was being repetitive because I didn't want the docs to depend on the
> user remembering what the "bss" section is (which, technically, is also
> not part of the C standard). I think a better way would be to just not
> mention "bss section" at all and reword the rest. Maybe something like
>
>   The caller:
>
>   * Allocates a variable `struct lock_file lock`, initialized to zeros.
>     Because the `lock_file` structure is used in an `atexit(3)` handler,
>     its storage has to remain valid throughout the life of the program;
>     e.g., it should be a static variable or space allocated on the heap.
>
> Better?

Somewhat.  I like that you droped "BSS", though.

	Allocates a 'struct lock_file' either as a static variable
	or on the heap, initialized to zeros.  Once you use the
	structure to call hold_lock_file_* family of functions, it
	belongs to the lockfile subsystem and its storage must
	remain valid throughout the life of the program (i.e. you
	cannot use an on-stack variable to hold this structure).

Show 15 quoted lines

>> It feels a bit conflicting between "must not be freed or ALTERED"
>> and "it may be REUSED".  Drop "or altered" to reduce confusion?  And
>> this repeats the third sentence I suggested to remove from the first
>> paragraph (above 'see below' refers here).
>
> The purpose of "or altered" is to make sure that users don't imagine
> that they should memset() the structure to zeros or something before
> reusing it (especially since the "caller" instructions earlier say that
> the object should be "initialized to zeros").
>
> Would it help if I change
>
>     s/altered/altered by the caller/
>
> ?

The fundamental rule is that callers outside the lockfile system must not touch individual members of "struct lock_file" that is "live". They must not free, they must not alter, they must not do anything other than calling the lockfile API functions.

While it is natural that the readers would understand such a rule must be followed for a lockfile that is in either the initialized, locked, closed-but-not-committed state, I agree that it is not just possible but likely that people misunderstand and think that once a lockfile is committed or rolled back it no longer has to follow that rule. We would want to make sure readers do not fall into such a misunderstanding.

I dunno. Your "if you memset it to NULs, you will break the linked list of the lock and the whole lockfile system and the element cannot even be reused." may be the most important thing you may have wanted to say, but it is not conveyed by that change at all, at least to me.

A small voice in the back of my skull keeps telling me that a rule that is hard to document and explain is a rule that does not make sense. Is it possible to allow commit and rollback to safely remove the structure from the atexit(3) list (aka "no longer owned by the lockfile subsystem")?

Show 9 quoted lines

>> Is it allowed to tell the name of this lock_file to other people and
>> let them read from it?  The answer is yes but it is not obvious from
>> this description.
>> 
>> Also mention how the above interact with reopen_lock_file() here?
>
> I'll take a stab at it, though TBH I haven't really studied the use case
> for reopen_lock_file(). You might be better able to provide insight into
> this topic.

You would want to be able to do this:

 - hold a lock on a file (say, the index);

 - update it in preparation to commit it;

 - write it out and make sure the contents is on the disk platter,
   in preparation to call another program and allow it (and nobody
   else) to inspect the contents you wrote, while still holding the
   lock yourself.  In our set of API functions, close_lock_file is
   what lets us do this.

 - further update it, write it out and commit.  We need to read it
   first, open(2) it to write, write(2), and commit_lock_file().

The set of API functions you described in the document, there is no way to say "I already have a lock on that file, just let me open(2) for writing because I have further updates" and that is where reopen comes in.