On Thu, Dec 4, 2014 at 3:08 PM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:
> We don't actually need the locking functionality, because we already
> hold the lock on the reference itself,
No. You do need the lock.
The ref is locked only during transaction_commit()
If you don't want to lock the reflog file and instead rely on the lock
on the ref itself you will need to
rework your patches so that the lock on the ref is taken already
during, for example, transaction_update_ref() instead.
But without doing those changes and moving the ref locking from
_commit() to _update_ref() you will risk reflog corruption/surprises
if two operations collide and both rewrite the reflog without any lock held.
> which is how the reflog file is
> locked. But the lock_file code still does some of the bookkeeping for
> us and is more careful than the old code here was.
>
> For example:
>
> * Correctly handle the case that the reflog lock file already exists
> for some reason or cannot be opened.
>
> * Correctly clean up the lockfile if the program dies.
>
> Signed-off-by: Michael Haggerty <mhagger@xxxxxxxxxxxx>
> ---
> builtin/reflog.c | 37 ++++++++++++++++++++++---------------
> 1 file changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index a282e60..d344d45 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const unsigned char *sha1, int
> return 0;
> }
>
> +static struct lock_file reflog_lock;
> +
> static int expire_reflog(const char *refname, const unsigned char *sha1, void *cb_data)
> {
> struct cmd_reflog_expire_cb *cmd = cb_data;
> 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;
> @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
> 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");
> + if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
> + goto failure;
> + cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> + if (!cb.newlog)
> + goto failure;
> }
>
> cb.cmd = cmd;
> @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
> }
>
> 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 ||
> @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c
> close_ref(lock) < 0)) {
> 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("cannot rename %s.lock to %s",
> + log_file, log_file);
> } else if (cmd->updateref && commit_ref(lock)) {
> status |= error("Couldn't set %s", lock->ref_name);
> - } else {
> - adjust_shared_perm(log_file);
> }
> }
> - 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
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html