Re: [PATCH v5 09/35] lockfile.c: document the various states of lock_file objects
On 09/16/2014 11:03 PM, Jonathan Nieder wrote:
Michael Haggerty wrote:
--- a/lockfile.c
+++ b/lockfile.c
@@ -4,6 +4,63 @@
#include cache.h
#include sigchain.h
+/*
+ * File write-locks as used by Git.
+ *
+ * When a file at $FILENAME needs to be written, it is done as
+ * follows:
This overlaps a lot with the API doc, which makes me worry a little
about it going out of date. Would improving the API doc help, or if
aspects are especially relevant here, is there a way to summarize them
more quickly to avoid some of the redundancy?
[...]
+ * A lock_file object can be in several states:
Would it make sense for this information to go near the definition of
'struct lock_file'? That's where I'd start if I were looking for
information on what states a lock_file can be in.
I agree with your point about overlap. I will split the documentation
into two parts with less redundancy:
* Documentation/technical/api-lockfile.txt: How to use the API.
* lockfile.{c,h}: Internal implementation details.
I think the implementation details would get lost among the thousand
things in cache.h. So instead, I will add a commit on top of the patch
series that splits out a lockfile.h header file (which I think is a good
idea anyway), and moves the internal documentation there. Sound good?
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 09/35] lockfile.c: document the various states of lock_file objects
Michael Haggerty wrote:
I agree with your point about overlap. I will split the documentation
into two parts with less redundancy:
* Documentation/technical/api-lockfile.txt: How to use the API.
* lockfile.{c,h}: Internal implementation details.
I think the implementation details would get lost among the thousand
things in cache.h. So instead, I will add a commit on top of the patch
series that splits out a lockfile.h header file (which I think is a good
idea anyway), and moves the internal documentation there. Sound good?
Yep, a separate lockfile.h header file sounds sensible to me.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 09/35] lockfile.c: document the various states of lock_file objects
Document the valid states of lock_file objects, how they get into each state, and how the state is encoded in the object's fields. Signed-off-by: Michael Haggerty mhag...@alum.mit.edu Reviewed-by: Ronnie Sahlberg sahlb...@google.com --- lockfile.c | 57 + 1 file changed, 57 insertions(+) diff --git a/lockfile.c b/lockfile.c index 00c972c..b6fe848 100644 --- a/lockfile.c +++ b/lockfile.c @@ -4,6 +4,63 @@ #include cache.h #include sigchain.h +/* + * File write-locks as used by Git. + * + * When a file at $FILENAME needs to be written, it is done as + * follows: + * + * 1. Obtain a lock on the file by creating a lockfile at path + *$FILENAME.lock. The file is opened for read/write using O_CREAT + *and O_EXCL mode to ensure that it doesn't already exist. Such a + *lock file is respected by writers *but not by readers*. + * + *Usually, if $FILENAME is a symlink, then it is resolved, and the + *file ultimately pointed to is the one that is locked and later + *replaced. However, if LOCK_NODEREF is used, then $FILENAME + *itself is locked and later replaced, even if it is a symlink. + * + * 2. Write the new file contents to the lockfile. + * + * 3. Move the lockfile to its final destination using rename(2). + * + * Instead of (3), the change can be rolled back by deleting lockfile. + * + * This module keeps track of all locked files in lock_file_list. + * When the first file is locked, it registers an atexit(3) handler; + * when the program exits, the handler rolls back any files that have + * been locked but were never committed or rolled back. + * + * A lock_file is owned by the process that created it. The lock_file + * object has an owner field that records its owner. This field is + * used to prevent a forked process from closing a lock_file of its + * parent. + * + * A lock_file object can be in several states: + * + * - Uninitialized. In this state the object's on_list field must be + * zero but the rest of its contents need not be initialized. As + * soon as the object is used in any way, it is irrevocably + * registered in the lock_file_list, and on_list is set. + * + * - Locked, lockfile open (after hold_lock_file_for_update(), + * hold_lock_file_for_append(), or reopen_lock_file()). In this + * state, the lockfile exists, filename holds the filename of the + * lockfile, fd holds a file descriptor open for writing to the + * lockfile, and owner holds the PID of the process that locked the + * file. + * + * - Locked, lockfile closed (after close_lock_file()). Same as the + * previous state, except that the lockfile is closed and fd is -1. + * + * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a + * failed attempt to lock). In this state, filename[0] == '\0' and + * fd is -1. The object is left registered in the lock_file_list, + * and on_list is set. + * + * See Documentation/api-lockfile.txt for more information. + */ + static struct lock_file *lock_file_list; static void remove_lock_file(void) -- 2.1.0 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 09/35] lockfile.c: document the various states of lock_file objects
Michael Haggerty wrote: --- a/lockfile.c +++ b/lockfile.c @@ -4,6 +4,63 @@ #include cache.h #include sigchain.h +/* + * File write-locks as used by Git. + * + * When a file at $FILENAME needs to be written, it is done as + * follows: This overlaps a lot with the API doc, which makes me worry a little about it going out of date. Would improving the API doc help, or if aspects are especially relevant here, is there a way to summarize them more quickly to avoid some of the redundancy? [...] + * A lock_file object can be in several states: Would it make sense for this information to go near the definition of 'struct lock_file'? That's where I'd start if I were looking for information on what states a lock_file can be in. My two cents, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
