git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 07/24] expire_reflog(): use a lock_file for rewriting the reflog file

From
Stefan Beller <sbeller@google.com>
Date
Dec 12, 2014, 18:57 UTC
Message-ID
<CAGZ79kaPzMPcxMqsTeW5LjYNc7LbpjHE5eBPHWKvUkBU3NvGJw@mail.gmail.com>
In-Reply-To
<1418374623-5566-8-git-send-email-mhagger@alum.mit.edu>
On Fri, Dec 12, 2014 at 12:56 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 89 quoted lines
> We don't actually need the locking functionality, because we already
> hold the lock on the reference itself, which is how the reflog file is
> locked. But the lock_file code can do some of the bookkeeping for us,
> and it is more careful than the old code here was. For example:
>
> * It correctly handles the case that the reflog lock file already
>   exists for some reason or cannot be opened.
>
> * It correctly cleans up the lockfile if the program dies.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  builtin/reflog.c | 60 ++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 41 insertions(+), 19 deletions(-)
>
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 37b33c9..ba5b3d3 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -352,9 +352,10 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
>  static int expire_reflog(const char *refname, const unsigned char *sha1,
>                          struct cmd_reflog_expire_cb *cmd)
>  {
> +       static struct lock_file reflog_lock;
>         struct expire_reflog_cb cb;
>         struct ref_lock *lock;
> -       char *log_file, *newlog_path = NULL;
> +       char *log_file;
>         struct commit *tip_commit;
>         struct commit_list *tips;
>         int status = 0;
> @@ -362,8 +363,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>         memset(&cb, 0, sizeof(cb));
>
>         /*
> -        * we take the lock for the ref itself to prevent it from
> -        * getting updated.
> +        * The reflog file is locked by holding the lock on the
> +        * reference itself, plus we might need to update the
> +        * reference if --updateref was specified:
>          */
>         lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
>         if (!lock)
> @@ -372,10 +374,29 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>                 unlock_ref(lock);
>                 return 0;
>         }
> +
>         log_file = git_pathdup("logs/%s", refname);
>         if (!cmd->dry_run) {
> -               newlog_path = git_pathdup("logs/%s.lock", refname);
> -               cb.newlog = fopen(newlog_path, "w");
> +               /*
> +                * Even though holding $GIT_DIR/logs/$reflog.lock has
> +                * no locking implications, we use the lock_file
> +                * machinery here anyway because it does a lot of the
> +                * work we need, including cleaning up if the program
> +                * exits unexpectedly.
> +                */
> +               if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {
> +                       struct strbuf err = STRBUF_INIT;
> +                       unable_to_lock_message(log_file, errno, &err);
> +                       error("%s", err.buf);
> +                       strbuf_release(&err);
> +                       goto failure;
> +               }
> +               cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> +               if (!cb.newlog) {
> +                       error("cannot fdopen %s (%s)",
> +                             reflog_lock.filename.buf, strerror(errno));
> +                       goto failure;
> +               }
>         }
>
>         cb.cmd = cmd;
> @@ -423,32 +444,33 @@ static int expire_reflog(const char *refname, const unsigned char *sha1,
>         }
>
>         if (cb.newlog) {
> -               if (fclose(cb.newlog)) {
> -                       status |= error("%s: %s", strerror(errno),
> -                                       newlog_path);
> -                       unlink(newlog_path);
> +               if (close_lock_file(&reflog_lock)) {
> +                       status |= error("couldn't write %s: %s", log_file,
> +                                       strerror(errno));
>                 } else if (cmd->updateref &&
>                         (write_in_full(lock->lock_fd,
>                                 sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||

optional nit: While being here, you may fix the indentation of the sha1_to_hex to align to the opening parentheses of write_in_full the line before.

Show 18 quoted lines
>                          write_str_in_full(lock->lock_fd, "\n") != 1 ||
>                          close_ref(lock) < 0)) {
> -                       status |= error("Couldn't write %s",
> +                       status |= error("couldn't write %s",
>                                         lock->lk->filename.buf);
> -                       unlink(newlog_path);
> -               } else if (rename(newlog_path, log_file)) {
> -                       status |= error("cannot rename %s to %s",
> -                                       newlog_path, log_file);
> -                       unlink(newlog_path);
> +                       rollback_lock_file(&reflog_lock);
> +               } else if (commit_lock_file(&reflog_lock)) {
> +                       status |= error("unable to commit reflog '%s' (%s)",
> +                                       log_file, strerror(errno));
>                 } else if (cmd->updateref && commit_ref(lock)) {
> -                       status |= error("Couldn't set %s", lock->ref_name);
> -               } else {
> -                       adjust_shared_perm(log_file);
So we don't need adjust_shared_perm anymore?
I have been looking at this excerpt for a while now and I am confused by the
first and third statement in the origin/master state as well as after
your changes.
So it's essentially:
  * write out the reflog lock file
  * update ref in the lock file
  * rename the reflog lock into place
  * rename the ref lock into place
  * (in origin/master) if everything went fine, call adjust_shared_perm.
  * In your patch we actually don't need it as on locking the lock
file there is a call
    to  adjust_shared_perm already.

So I think it's clear to me now and looks good. It took me sometime to understand it. Maybe that's because I just got up or it's in fact hard to understand. Would comments help in laying out the sequence of actions beforehand?

I think the most confusing thing was the close_lock_file as a first statement in the if chain. close_lock_file is primarily used here to have the file written out to disk, but the function name suggests that the primary functionality is the closing of the fd.

Show 19 quoted lines
> +                       status |= error("couldn't set %s", lock->ref_name);
>                 }
>         }
> -       free(newlog_path);
>         free(log_file);
>         unlock_ref(lock);
>         return status;
> +
> + failure:
> +       rollback_lock_file(&reflog_lock);
> +       free(log_file);
> +       unlock_ref(lock);
> +       return -1;
>  }
>
>  static int collect_reflog(const char *ref, const unsigned char *sha1, int unused, void *cb_data)
> --
> 2.1.3
>
Previous: Michael HaggertyNext: Michael Haggerty
Message 11 of 31 in “Add reflog_expire() to the references API”
  1. 00/24 Add reflog_expire() to the references APIMichael Haggerty, Dec 12, 2014
  2. 01/24 refs.c: make ref_transaction_create a wrapper for ref_transaction_updateMichael Haggerty, Dec 12, 2014
  3. 02/24 refs.c: make ref_transaction_delete a wrapper for ref_transaction_updateMichael Haggerty, Dec 12, 2014
  4. 03/24 refs.c: add a function to append a reflog entry to a fdMichael Haggerty, Dec 12, 2014
  5. 04/24 expire_reflog(): it's not an each_ref_fn anymoreMichael Haggerty, Dec 12, 2014
  6. Stefan BellerDec 12, 2014
  7. 05/24 expire_reflog(): rename "ref" parameter to "refname"Michael Haggerty, Dec 12, 2014
  8. 06/24 expire_reflog(): return early if the reference has no reflogMichael Haggerty, Dec 12, 2014
  9. Stefan BellerDec 12, 2014
  10. 07/24 expire_reflog(): use a lock_file for rewriting the reflog fileMichael Haggerty, Dec 12, 2014
  11. Stefan BellerDec 12, 2014
  12. 08/24 Extract function should_expire_reflog_ent()Michael Haggerty, Dec 12, 2014
  13. Stefan BellerDec 12, 2014
  14. 09/24 expire_reflog(): extract two policy-related functionsMichael Haggerty, Dec 12, 2014
  15. 10/24 expire_reflog(): add a "flags" argumentMichael Haggerty, Dec 12, 2014
  16. 11/24 expire_reflog(): move dry_run to flags argumentMichael Haggerty, Dec 12, 2014
  17. 12/24 expire_reflog(): move updateref to flags argumentMichael Haggerty, Dec 12, 2014
  18. 13/24 Rename expire_reflog_cb to expire_reflog_policy_cbMichael Haggerty, Dec 12, 2014
  19. 14/24 struct expire_reflog_cb: a new callback data typeMichael Haggerty, Dec 12, 2014
  20. 15/24 expire_reflog(): pass flags through to expire_reflog_ent()Michael Haggerty, Dec 12, 2014
  21. 16/24 expire_reflog(): move verbose to flags argumentMichael Haggerty, Dec 12, 2014
  22. 17/24 expire_reflog(): move rewrite to flags argumentMichael Haggerty, Dec 12, 2014
  23. 18/24 Move newlog and last_kept_sha1 to "struct expire_reflog_cb"Michael Haggerty, Dec 12, 2014
  24. 19/24 expire_reflog(): treat the policy callback data as opaqueMichael Haggerty, Dec 12, 2014
  25. 20/24 reflog_expire(): new function in the reference APIMichael Haggerty, Dec 12, 2014
  26. 21/24 refs.c: remove unlock_ref/close_ref/commit_ref from the refs apiMichael Haggerty, Dec 12, 2014
  27. 22/24 lock_any_ref_for_update(): inline functionMichael Haggerty, Dec 12, 2014
  28. Stefan BellerDec 12, 2014
  29. 23/24 refs.c: don't expose the internal struct ref_lock in the header fileMichael Haggerty, Dec 12, 2014
  30. 24/24 refs.c: let fprintf handle the formattingMichael Haggerty, Dec 12, 2014
  31. Junio C HamanoDec 12, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.