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

[Junio C Hamano]

Michael Haggerty  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

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

[Eric Sunshine]

On Tue, Apr 1, 2014 at 11:58 AM, 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.
>
> Signed-off-by: Michael Haggerty 
> ---
> diff --git a/lockfile.c b/lockfile.c
> index c06e134..336b914 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -271,19 +325,54 @@ int hold_lock_file_for_append(struct lock_file *lk, 
> const char *path, int flags)
> return fd;
>  }
>
> -int close_lock_file(struct lock_file *lk)
> +static int close_staging_file(struct lock_file *lk)
>  {
> int fd = lk->fd;
> +
> lk->fd = -1;
> return close(fd);
>  }
>
> -int commit_lock_file(struct lock_file *lk)
> +int close_lock_file(struct lock_file *lk)
>  {
> -   if (lk->fd >= 0 && close_lock_file(lk))
> -   return -1;
> -   if (rename(lk->staging_filename.buf, lk->filename.buf))
> +   assert(!(lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE));
> +   return close_staging_file(lk);
> +}
> +
> +int activate_staging_file(struct lock_file *lk)
> +{
> +   int err;
> +
> +   assert(lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE);
> +   assert(lk->fd >= 0);
> +   assert(lk->staging_filename.len);
> +
> +   if (close_staging_file(lk))
> return -1;
> +
> +   err = rename(lk->staging_filename.buf, lk->filename.buf);
> +   strbuf_setlen(&lk->staging_filename, 0);

strbuf_reset()?

> +
> +   return err;
> +}
> +
> +int commit_lock_file(struct lock_file *lk)
> +{
> +   if (lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE) {
> +   if (lk->staging_filename.len) {
> +   assert(lk->fd >= 0);
> +   if (activate_staging_file(lk))
> +   return -1;
> +   }
> +   strbuf_addbuf(&lk->staging_filename, &lk->filename);
> +   strbuf_addstr(&lk->staging_filename, ".lock");
> +   unlink_or_warn(lk->staging_filename.buf);
> +   } else {
> +   if (lk->fd >= 0 && close_lock_file(lk))
> +   return -1;
> +   if (rename(lk->staging_filename.buf, lk->filename.buf))
> +   return -1;
> +   }
> reset_lock_file(lk);
> return 0;
>  }
> @@ -318,10 +407,21 @@ int commit_locked_index(struct lock_file *lk)
>
>  void rollback_lock_file(struct lock_file *lk)
>  {
> -   if (lk->filename.len) {
> -   if (lk->fd >= 0)
> -   close_lock_file(lk);
> -   unlink_or_warn(lk->staging_filename.buf);
> -   reset_lock_file(lk);
> +   if (!lk->filename.len)
> +   return;
> +
> +   if (lk->fd >= 0)
> +   close_staging_file(lk);
> +
> +   if (lk->flags & LOCK_FLAGS_SEPARATE_STAGING_FILE) {
> +   if (lk->staging_filename.len) {
> +   unlink_or_warn(lk->staging_filename.buf);
> +   strbuf_setlen(&lk->staging_filename, 0);

strbuf_reset()?

> +   }
> +   strbuf_addbuf(&lk->staging_filename, &lk->filename);
> +   strbuf_addstr(&lk->staging_filename, ".lock");
> }
> +
> +   unlink_or_warn(lk->staging_filename.buf);
> +   reset_lock_file(lk);
>  }
> --
> 1.9.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 22/22] lockfile: allow new file contents to be written while retaining lock

[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