Subject: Re: [PATCH v5 02/35] api-lockfile: expand the documentation

From: Michael Haggerty <[email-masked]>
Date: Mon, 22 Sep 2014 16:13:39 +0200

On 09/16/2014 10:25 PM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> Document a couple more functions and the flags argument as used by
>> hold_lock_file_for_update() and hold_lock_file_for_append().
> 
> Thanks.
> 
> [...]
>> --- a/Documentation/technical/api-lockfile.txt
>> +++ b/Documentation/technical/api-lockfile.txt
>> @@ -28,9 +28,39 @@ hold_lock_file_for_update::
>>  	the final destination (e.g. `$GIT_DIR/index`) and a flag
>>  	`die_on_error`.  Attempt to create a lockfile for the
>>  	destination and return the file descriptor for writing
>> -	to the file.  If `die_on_error` flag is true, it dies if
>> -	a lock is already taken for the file; otherwise it
>> -	returns a negative integer to the caller on failure.
>> +	to the file.  The flags parameter is a combination of
>> ++
>> +--
> 
> Context: this document has structure
> 
> 	lockfile API
> 	============
> 
> 	Explanation of purpose (nice!).
> 
> 	The functions
> 	-------------
> 
> 	Quick descriptions of each of the four functions
> 	`hold_lock_file_for_update`, `commit_lock_file`,
> 	`rollback_lock_file`, `close_lock_file`.
> 
> 	Reminder about lifetime of the lock_file structure.
> 
> 	Description of cleanup convention (thou shalt either
> 	commit or roll back; if you forget to, the atexit
> 	handler will roll back for you).
> 
> 	Long warning about the harder use cases.  The above
> 	"thou shalt" was a lie --- you can also
> 	close_lock_file if you know what you're doing
> 	[jn: why is that function part of the public API?].
> 
> What's nice about the existing structure is that you can get
> a sense of how to use the API at a glance.  Would there be a
> way to add this extra information while preserving that property?
> 
> E.g.:
> 
> 	lockfile API
> 	============
> 
> 	Nice brief explanation of purpose ("is this the API
> 	I want to use?"), as before.
> 
> 	Calling sequence
> 	----------------
> 
> 	The caller:
> 
> 	* Allocates a variable `struct lock_file lock` in the bss
> 	section or heap.  Because the `lock_file` structure is used
> 	in an `atexit(3)` handler, its storage has to stay
> 	throughout the life of the program.  It cannot be an auto
> 	variable allocated on the stack.
> 
> 	* Attempts to create a lockfile by passing that variable and
> 	the filename of the final destination (e.g. `$GIT_DIR/index`)
> 	to `hold_lock_file_for_update` or `hold_lock_file_for_append`.
> 	+
> 	If the `die_on_error` flag is true, git dies if a lock is
> 	already taken for the file.
> 
> 	* Writes new content for the destination file by writing to
> 	`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`.
> 
> 	If you do not call one of `commit_lock_file`,
> 	`rollback_lock_file`, and `close_lock_file` and instead
> 	simply `exit(3)` from the program, an `atexit(3)` handler will
> 	close and remove the lockfile.
> 
> 	You should never call `close(2)` on `lock->fd` yourself~
> 	Otherwise the ...
> 
> 	Error handling
> 	--------------
> 
> 	Functions return 0 on success, -1 on failure.  errno is?
> 	isn't? meaningful on error.
> 
> 	... description of unable_to_lock_error and unable_to_lock_die
> 	here ...
> 
> 	Flags
> 	-----
> 
> 	LOCK_NODEREF::
> 
> 		Usually symbolic links in the destination path are
> 		resolved and the lockfile is created by adding ".lock"
> 		to the resolved path.  If `LOCK_NODEREF` is set, then
> 		the lockfile is created by adding ".lock" to the path
> 		argument itself.
> 
> What is the user-visible effect of that flag?  When would I want
> to pass that flag, and when wouldn't I?
> 
> 	LOCK_DIE_ON_ERROR::
> 
> 		If a lock is already taken for the file, `die()` with
> 		an error message.  If this option is not specified,
> 		trying to hold a lock file that is already taken will
> 		return -1 to the caller.
> 
> Sensible?
> Jonathan

OK, in the next reroll I will revise the documentation pretty
thoroughly. Please let me know what you think.

Michael

-- 
Michael Haggerty
[email-masked]