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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 26, 2014, 18:33 UTC
Message-ID: <xmqqlhp68c69.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1411726119-31598-3-git-send-email-mhagger@alum.mit.edu>

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

Show 9 quoted lines

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

Show 6 quoted lines

> +* 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.

> +* Automatic cruft removal. If the program exits after we lock a file
> +  but before the changes have been committed, we want to make sure
> +  that we remove the lockfile.

Show 11 quoted lines

> +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).

Show 24 quoted lines

> +* 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 //;

Show 7 quoted lines

> +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.

Show 11 quoted lines

> +unable_to_lock_message::
> +
> +	Append an appropriate error message to a `strbuf`.
> +
> +unable_to_lock_error::
> +
> +	Emit an appropriate error message using `error()`.
> +
> +unable_to_lock_die::
> +
> +	Emit an appropriate error message and `die()`.