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

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Document a couple more functions and the flags argument as used by
>> hold_lock_file_for_update() and hold_lock_file_for_append().
>> Reorganize the document to make it more accessible.
>>
>> Helped-by: Jonathan Nieder <jrnieder@gmail.com>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  Documentation/technical/api-lockfile.txt | 199 ++++++++++++++++++++++---------
>>  1 file changed, 145 insertions(+), 54 deletions(-)
> 
> Nicely done.
> 
>> +* Mutual exclusion and atomic file updates. When we want to change a
>> +  file, we create a lockfile `<filename>.lock`, write the new file
>> +  contents into it, and then rename the lockfile to its final
>> +  destination `<filename>`. We create the `<filename>.lock` file with
>> +  `O_CREAT|O_EXCL` so that we can notice and fail if somebody else has
>> +  already locked the file.
> 
> 
> You may want to say
> 
> 	then atomically rename the lockfile to its final destination
> 	to commit the changes and unlock the file.
> 
> here; that way, your mention of "commit" in the next paragraph would
> become easier to understand.

>> [...]
>> +Calling sequence
>> +----------------
>> +
>> +The caller:
>> +
>> +* Allocates a variable `struct lock_file lock` in the bss section or
>> +  heap, initialized to zeros. It cannot be an auto variable allocated
>> +  on the stack. Because the `lock_file` structure is used in an
>> +  `atexit(3)` handler, its storage has to stay throughout the life of
>> +  the program, even after the file changes have been committed or
>> +  rolled back.
> 
> It is easier to read if you pushed the second sentence (which is a
> rephrase of the first one) and third sentence (which explains why
> the second sentence is true) out of line as a side-note, I think, or
> probably remove them from here (see below).

>> +* Attempts to create a lockfile by passing that variable and the path
>> +  of the final destination (e.g. `$GIT_DIR/index`) to
>> +  `hold_lock_file_for_update` or `hold_lock_file_for_append`.
>> +
>> +* Writes new content for the destination file by writing to the file
>> +  descriptor returned by those functions (also available via
>> +  `lock->fd`).
>> +
>> +When finished writing, the caller can:
>> +
>> +* Close the file descriptor and rename the lockfile to its final
>> +  destination by calling `commit_lock_file`.
>> +
>> +* Close the file descriptor and remove the lockfile by calling
>> +  `rollback_lock_file`.
>> +
>> +* Close the file descriptor without removing or renaming the lockfile
>> +  by calling `close_lock_file`, and later call `commit_lock_file` or
>> +  `rollback_lock_file`.
>> +
>> +At this point, the `lock_file` object must not be freed or altered,
>> +because it is still registered in the `lock_file_list`. However, it
>> +may be reused; just pass it to another call of
>> +`hold_lock_file_for_update` or `hold_lock_file_for_append`.
> 
> 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).

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

>> +If the program exits before you have called one of `commit_lock_file`,
>> +`rollback_lock_file`, or `close_lock_file`, an `atexit(3)` handler
>> +will close and remove the lockfile, essentially rolling back any
>> +uncommitted changes.
> 
> s/essentially //;

>> +Error handling
>> +--------------
>> +
>> +The `hold_lock_file_*` functions return a file descriptor on success
>> +or -1 on failure (unless `LOCK_DIE_ON_ERROR` is used; see below). On
>> +errors, `errno` describes the reason for failure. Errors can be
>> +handled by passing `errno` to one of the following helper functions:
> 
> s/handled by/reported by/; probably.  None of these will help you
> "handle" errors in the sense to (attempt to) recover from it.