Re: [PATCH 22/22] lockfile: allow new file contents to be written while retaining lock

2014-04-02 Thread Junio C Hamano
Michael Haggerty mhag...@alum.mit.edu writes:

 +static int open_staging_file(struct lock_file *lk)
 +{
 + strbuf_setlen(lk-staging_filename, lk-filename.len);
 + strbuf_addstr(lk-staging_filename, .new);
 + lk-fd = open(lk-staging_filename.buf, O_RDWR | O_CREAT | O_EXCL, 
 0666);
 + if (lk-fd  0) {
 + return -1;
 + }

All the other if (lk-fd  0) calls reset_lock_file(lk).  Is it an
intentional omission that this one does not?

If so, please drop the extraneous {} around the single return -1
statement.

I also share the same puzzlement in Peff's review.
--
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 22/22] lockfile: allow new file contents to be written while retaining lock

2014-04-01 Thread Michael Haggerty
Add a new option flag, LOCK_SEPARATE_STAGING_FILE, that can be passed
to hold_lock_file_for_update() or hold_lock_file_for_append() to use a
staging file that is independent of the lock file.

Add a new function activate_staging_file() that activates the contents
that have been written to the staging file without releasing the lock.

This functionality can be used to ensure that changes to two files are
seen by other processes in one order even if correctness requires the
locks to be released in another order.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 Documentation/technical/api-lockfile.txt |  54 
 cache.h  |   4 +
 lockfile.c   | 140 ++-
 3 files changed, 162 insertions(+), 36 deletions(-)

diff --git a/Documentation/technical/api-lockfile.txt 
b/Documentation/technical/api-lockfile.txt
index 95ed03b..06b49f9 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -43,6 +43,13 @@ LOCK_DIE_ON_ERROR::
If a lock is already taken for the file, `die()` with an error
message.  If this option is not specified, return a negative
integer to the caller on failure.
+
+LOCK_SEPARATE_STAGING_FILE::
+
+   Use a separate file (not the lockfile) for staging the new
+   file contents.  This option makes it possible to call
+   activate_staging_file() to overwrite the old file while
+   retaining the file lock.
 --
 
 hold_lock_file_for_append::
@@ -64,23 +71,39 @@ unable_to_lock_index_die::
 
 commit_lock_file::
 
-   Take a pointer to the `struct lock_file` initialized
-   with an earlier call to `hold_lock_file_for_update()`,
-   close the file descriptor and rename the lockfile to its
-   final destination.  Returns 0 upon success, a negative
-   value on failure to close(2) or rename(2).
+   Take a pointer to the `struct lock_file` initialized with an
+   earlier call to `hold_lock_file_for_update()` or
+   `hold_lock_file_for_append()`.  Close the file descriptor.
+   Then, if the lock was created with the
+   LOCK_SEPARATE_STAGING_FILE flag, rename the staging file on
+   top of the locked file (if `activate_staging_file()` hasn't
+   already been called) and delete the lockfile; otherwise,
+   rename the lockfile to its final destination.  Returns 0 upon
+   success, a negative value on failure to close(2) or rename(2).
+
+activate_staging_file::
+
+   Write the contents of the separate staging file over those of
+   the locked file while retaining the lock.  This function may
+   only be called if the lock was created with the
+   LOCK_SEPARATE_STAGING_FILE flag.
 
 rollback_lock_file::
 
-   Take a pointer to the `struct lock_file` initialized
-   with an earlier call to `hold_lock_file_for_update()`,
-   close the file descriptor and remove the lockfile.
+   Take a pointer to the `struct lock_file` initialized with an
+   earlier call to `hold_lock_file_for_update()`, close the file
+   descriptor, remove the separate staging file (if any), and
+   remove the lockfile.  Please note that if
+   `activate_staging_file()` has already been called, then the
+   contents committed at that time will remain active.
 
 close_lock_file::
Take a pointer to the `struct lock_file` initialized
with an earlier call to `hold_lock_file_for_update()`,
and close the file descriptor.  Returns 0 upon success,
-   a negative value on failure to close(2).
+   a negative value on failure to close(2).  This function must
+   only be called if the lock was created without the
+   LOCK_SEPARATE_STAGING_FILE flag.
 
 Because the structure is used in an `atexit(3)` handler, its
 storage has to stay throughout the life of the program.  It
@@ -94,11 +117,10 @@ will close and remove the lockfile.
 If you need to close the file descriptor you obtained from
 `hold_lock_file_for_update` function yourself, do so by calling
 `close_lock_file()`.  You should never call `close(2)` yourself!
-Otherwise the `struct
-lock_file` structure still remembers that the file descriptor
-needs to be closed, and a later call to `commit_lock_file()` or
-`rollback_lock_file()` will result in duplicate calls to
-`close(2)`.  Worse yet, if you `close(2)`, open another file
-descriptor for completely different purpose, and then call
-`commit_lock_file()` or `rollback_lock_file()`, they may close
+Otherwise the `struct lock_file` structure still remembers that the
+file descriptor needs to be closed, and a later call to
+`commit_lock_file()` or `rollback_lock_file()` will result in
+duplicate calls to `close(2)`.  Worse yet, if you `close(2)`, open
+another file descriptor for completely different purpose, and then
+call `commit_lock_file()` or `rollback_lock_file()`, they may close
 that unrelated file descriptor.
diff 

Re: [PATCH 22/22] lockfile: allow new file contents to be written while retaining lock

2014-04-01 Thread Jeff King
On Tue, Apr 01, 2014 at 05:58:30PM +0200, Michael Haggerty wrote:

 Add a new option flag, LOCK_SEPARATE_STAGING_FILE, that can be passed
 to hold_lock_file_for_update() or hold_lock_file_for_append() to use a
 staging file that is independent of the lock file.
 
 Add a new function activate_staging_file() that activates the contents
 that have been written to the staging file without releasing the lock.
 
 This functionality can be used to ensure that changes to two files are
 seen by other processes in one order even if correctness requires the
 locks to be released in another order.

Can you give an example of when this is useful? I'm guessing the
application is for writing out packed-refs before pruning loose refs in
git-pack-refs?

It seems like this makes the API much more confusing.  If I understand
correctly, this is basically allowing us to take a lock, write to
_another_ tmpfile that is not the lock, then rename the tmpfile into
place without releasing the lock (and then we can drop the lock at our
convenience).

I wonder if it would be simpler to build an API for that around the
lock_file API, rather than as part of it. Or am I misunderstanding
what's going on?

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