Merge branch 'mh/ref-locking-fix'
authorJunio C Hamano <gitster@pobox.com>
Thu, 26 Oct 2017 03:29:23 +0000 (12:29 +0900)
committerJunio C Hamano <gitster@pobox.com>
Thu, 26 Oct 2017 03:29:23 +0000 (12:29 +0900)
Transactions to update multiple references that involves a deletion
was quite broken in an error codepath and did not abort everything
correctly.

* mh/ref-locking-fix:
files_transaction_prepare(): fix handling of ref lock failure
t1404: add a bunch of tests of D/F conflicts

1  2 
refs/files-backend.c
diff --combined refs/files-backend.c
index 014dabb0bf3647b9392d6087ebf6c40e1bc68108,a8c29b7865e327e9a09dc832fb0ea7994ce82ce5..8cc1e07fdb7204d13c99826304317c66d4249cf5
@@@ -12,7 -12,7 +12,7 @@@
  
  struct ref_lock {
        char *ref_name;
 -      struct lock_file *lk;
 +      struct lock_file lk;
        struct object_id old_oid;
  };
  
@@@ -106,6 -106,15 +106,6 @@@ static void files_reflog_path(struct fi
                              struct strbuf *sb,
                              const char *refname)
  {
 -      if (!refname) {
 -              /*
 -               * FIXME: of course this is wrong in multi worktree
 -               * setting. To be fixed real soon.
 -               */
 -              strbuf_addf(sb, "%s/logs", refs->gitcommondir);
 -              return;
 -      }
 -
        switch (ref_type(refname)) {
        case REF_TYPE_PER_WORKTREE:
        case REF_TYPE_PSEUDOREF:
@@@ -409,7 -418,9 +409,7 @@@ out
  
  static void unlock_ref(struct ref_lock *lock)
  {
 -      /* Do not free lock->lk -- atexit() still looks at them */
 -      if (lock->lk)
 -              rollback_lock_file(lock->lk);
 +      rollback_lock_file(&lock->lk);
        free(lock->ref_name);
        free(lock);
  }
@@@ -523,8 -534,11 +523,8 @@@ retry
                goto error_return;
        }
  
 -      if (!lock->lk)
 -              lock->lk = xcalloc(1, sizeof(struct lock_file));
 -
        if (hold_lock_file_for_update_timeout(
 -                          lock->lk, ref_file.buf, LOCK_NO_DEREF,
 +                          &lock->lk, ref_file.buf, LOCK_NO_DEREF,
                            get_files_ref_lock_timeout_ms()) < 0) {
                if (errno == ENOENT && --attempts_remaining > 0) {
                        /*
@@@ -641,6 -655,43 +641,6 @@@ out
        return ret;
  }
  
 -static int files_peel_ref(struct ref_store *ref_store,
 -                        const char *refname, unsigned char *sha1)
 -{
 -      struct files_ref_store *refs =
 -              files_downcast(ref_store, REF_STORE_READ | REF_STORE_ODB,
 -                             "peel_ref");
 -      int flag;
 -      unsigned char base[20];
 -
 -      if (current_ref_iter && current_ref_iter->refname == refname) {
 -              struct object_id peeled;
 -
 -              if (ref_iterator_peel(current_ref_iter, &peeled))
 -                      return -1;
 -              hashcpy(sha1, peeled.hash);
 -              return 0;
 -      }
 -
 -      if (refs_read_ref_full(ref_store, refname,
 -                             RESOLVE_REF_READING, base, &flag))
 -              return -1;
 -
 -      /*
 -       * If the reference is packed, read its ref_entry from the
 -       * cache in the hope that we already know its peeled value.
 -       * We only try this optimization on packed references because
 -       * (a) forcing the filling of the loose reference cache could
 -       * be expensive and (b) loose references anyway usually do not
 -       * have REF_KNOWS_PEELED.
 -       */
 -      if (flag & REF_ISPACKED &&
 -          !refs_peel_ref(refs->packed_ref_store, refname, sha1))
 -              return 0;
 -
 -      return peel_object(base, sha1);
 -}
 -
  struct files_ref_iterator {
        struct ref_iterator base;
  
@@@ -711,7 -762,7 +711,7 @@@ static struct ref_iterator *files_ref_i
                const char *prefix, unsigned int flags)
  {
        struct files_ref_store *refs;
 -      struct ref_iterator *loose_iter, *packed_iter;
 +      struct ref_iterator *loose_iter, *packed_iter, *overlay_iter;
        struct files_ref_iterator *iter;
        struct ref_iterator *ref_iterator;
        unsigned int required_flags = REF_STORE_READ;
  
        refs = files_downcast(ref_store, required_flags, "ref_iterator_begin");
  
 -      iter = xcalloc(1, sizeof(*iter));
 -      ref_iterator = &iter->base;
 -      base_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable);
 -
        /*
         * We must make sure that all loose refs are read before
         * accessing the packed-refs file; this avoids a race
                        refs->packed_ref_store, prefix, 0,
                        DO_FOR_EACH_INCLUDE_BROKEN);
  
 -      iter->iter0 = overlay_ref_iterator_begin(loose_iter, packed_iter);
 +      overlay_iter = overlay_ref_iterator_begin(loose_iter, packed_iter);
 +
 +      iter = xcalloc(1, sizeof(*iter));
 +      ref_iterator = &iter->base;
 +      base_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable,
 +                             overlay_iter->ordered);
 +      iter->iter0 = overlay_iter;
        iter->flags = flags;
  
        return ref_iterator;
@@@ -900,9 -949,11 +900,9 @@@ static struct ref_lock *lock_ref_sha1_b
                goto error_return;
        }
  
 -      lock->lk = xcalloc(1, sizeof(struct lock_file));
 -
        lock->ref_name = xstrdup(refname);
  
 -      if (raceproof_create_file(ref_file.buf, create_reflock, lock->lk)) {
 +      if (raceproof_create_file(ref_file.buf, create_reflock, &lock->lk)) {
                last_errno = errno;
                unable_to_lock_message(ref_file.buf, errno, err);
                goto error_return;
@@@ -1223,9 -1274,9 +1223,9 @@@ static int commit_ref_update(struct fil
                             const struct object_id *oid, const char *logmsg,
                             struct strbuf *err);
  
 -static int files_rename_ref(struct ref_store *ref_store,
 +static int files_copy_or_rename_ref(struct ref_store *ref_store,
                            const char *oldrefname, const char *newrefname,
 -                          const char *logmsg)
 +                          const char *logmsg, int copy)
  {
        struct files_ref_store *refs =
                files_downcast(ref_store, REF_STORE_WRITE, "rename_ref");
        }
  
        if (flag & REF_ISSYMREF) {
 -              ret = error("refname %s is a symbolic ref, renaming it is not supported",
 -                          oldrefname);
 +              if (copy)
 +                      ret = error("refname %s is a symbolic ref, copying it is not supported",
 +                                  oldrefname);
 +              else
 +                      ret = error("refname %s is a symbolic ref, renaming it is not supported",
 +                                  oldrefname);
                goto out;
        }
        if (!refs_rename_ref_available(&refs->base, oldrefname, newrefname)) {
                goto out;
        }
  
 -      if (log && rename(sb_oldref.buf, tmp_renamed_log.buf)) {
 +      if (!copy && log && rename(sb_oldref.buf, tmp_renamed_log.buf)) {
                ret = error("unable to move logfile logs/%s to logs/"TMP_RENAMED_LOG": %s",
                            oldrefname, strerror(errno));
                goto out;
        }
  
 -      if (refs_delete_ref(&refs->base, logmsg, oldrefname,
 +      if (copy && log && copy_file(tmp_renamed_log.buf, sb_oldref.buf, 0644)) {
 +              ret = error("unable to copy logfile logs/%s to logs/"TMP_RENAMED_LOG": %s",
 +                          oldrefname, strerror(errno));
 +              goto out;
 +      }
 +
 +      if (!copy && refs_delete_ref(&refs->base, logmsg, oldrefname,
                            orig_oid.hash, REF_NODEREF)) {
                error("unable to delete old %s", oldrefname);
                goto rollback;
         * the safety anyway; we want to delete the reference whatever
         * its current value.
         */
 -      if (!refs_read_ref_full(&refs->base, newrefname,
 +      if (!copy && !refs_read_ref_full(&refs->base, newrefname,
                                RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
                                oid.hash, NULL) &&
            refs_delete_ref(&refs->base, NULL, newrefname,
        lock = lock_ref_sha1_basic(refs, newrefname, NULL, NULL, NULL,
                                   REF_NODEREF, NULL, &err);
        if (!lock) {
 -              error("unable to rename '%s' to '%s': %s", oldrefname, newrefname, err.buf);
 +              if (copy)
 +                      error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
 +              else
 +                      error("unable to rename '%s' to '%s': %s", oldrefname, newrefname, err.buf);
                strbuf_release(&err);
                goto rollback;
        }
        return ret;
  }
  
 -static int close_ref(struct ref_lock *lock)
 +static int files_rename_ref(struct ref_store *ref_store,
 +                          const char *oldrefname, const char *newrefname,
 +                          const char *logmsg)
 +{
 +      return files_copy_or_rename_ref(ref_store, oldrefname,
 +                               newrefname, logmsg, 0);
 +}
 +
 +static int files_copy_ref(struct ref_store *ref_store,
 +                          const char *oldrefname, const char *newrefname,
 +                          const char *logmsg)
 +{
 +      return files_copy_or_rename_ref(ref_store, oldrefname,
 +                               newrefname, logmsg, 1);
 +}
 +
 +static int close_ref_gently(struct ref_lock *lock)
  {
 -      if (close_lock_file(lock->lk))
 +      if (close_lock_file_gently(&lock->lk))
                return -1;
        return 0;
  }
  
  static int commit_ref(struct ref_lock *lock)
  {
 -      char *path = get_locked_file_path(lock->lk);
 +      char *path = get_locked_file_path(&lock->lk);
        struct stat st;
  
        if (!lstat(path, &st) && S_ISDIR(st.st_mode)) {
                free(path);
        }
  
 -      if (commit_lock_file(lock->lk))
 +      if (commit_lock_file(&lock->lk))
                return -1;
        return 0;
  }
@@@ -1543,7 -1565,7 +1543,7 @@@ static int log_ref_write_fd(int fd, con
  
        written = len <= maxlen ? write_in_full(fd, logrec, len) : -1;
        free(logrec);
 -      if (written != len)
 +      if (written < 0)
                return -1;
  
        return 0;
@@@ -1621,12 -1643,12 +1621,12 @@@ static int write_ref_to_lockfile(struc
                unlock_ref(lock);
                return -1;
        }
 -      fd = get_lock_file_fd(lock->lk);
 -      if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
 -          write_in_full(fd, &term, 1) != 1 ||
 -          close_ref(lock) < 0) {
 +      fd = get_lock_file_fd(&lock->lk);
 +      if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) < 0 ||
 +          write_in_full(fd, &term, 1) < 0 ||
 +          close_ref_gently(lock) < 0) {
                strbuf_addf(err,
 -                          "couldn't write '%s'", get_lock_file_path(lock->lk));
 +                          "couldn't write '%s'", get_lock_file_path(&lock->lk));
                unlock_ref(lock);
                return -1;
        }
@@@ -1670,12 -1692,13 +1670,12 @@@ static int commit_ref_update(struct fil
                 * check with HEAD only which should cover 99% of all usage
                 * scenarios (even 100% of the default ones).
                 */
 -              struct object_id head_oid;
                int head_flag;
                const char *head_ref;
  
                head_ref = refs_resolve_ref_unsafe(&refs->base, "HEAD",
                                                   RESOLVE_REF_READING,
 -                                                 head_oid.hash, &head_flag);
 +                                                 NULL, &head_flag);
                if (head_ref && (head_flag & REF_ISSYMREF) &&
                    !strcmp(head_ref, lock->ref_name)) {
                        struct strbuf log_err = STRBUF_INIT;
@@@ -1702,7 -1725,7 +1702,7 @@@ static int create_ref_symlink(struct re
  {
        int ret = -1;
  #ifndef NO_SYMLINK_HEAD
 -      char *ref_path = get_locked_file_path(lock->lk);
 +      char *ref_path = get_locked_file_path(&lock->lk);
        unlink(ref_path);
        ret = symlink(target, ref_path);
        free(ref_path);
@@@ -1738,14 -1761,14 +1738,14 @@@ static int create_symref_locked(struct 
                return 0;
        }
  
 -      if (!fdopen_lock_file(lock->lk, "w"))
 +      if (!fdopen_lock_file(&lock->lk, "w"))
                return error("unable to fdopen %s: %s",
 -                           lock->lk->tempfile.filename.buf, strerror(errno));
 +                           lock->lk.tempfile->filename.buf, strerror(errno));
  
        update_symref_reflog(refs, lock, refname, target, logmsg);
  
        /* no error check; commit_ref will check ferror */
 -      fprintf(lock->lk->tempfile.fp, "ref: %s\n", target);
 +      fprintf(lock->lk.tempfile->fp, "ref: %s\n", target);
        if (commit_ref(lock) < 0)
                return error("unable to write symref for %s: %s", refname,
                             strerror(errno));
@@@ -2052,64 -2075,23 +2052,64 @@@ static struct ref_iterator_vtable files
        files_reflog_iterator_abort
  };
  
 -static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store)
 +static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
 +                                                const char *gitdir)
  {
 -      struct files_ref_store *refs =
 -              files_downcast(ref_store, REF_STORE_READ,
 -                             "reflog_iterator_begin");
        struct files_reflog_iterator *iter = xcalloc(1, sizeof(*iter));
        struct ref_iterator *ref_iterator = &iter->base;
        struct strbuf sb = STRBUF_INIT;
  
 -      base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);
 -      files_reflog_path(refs, &sb, NULL);
 +      base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0);
 +      strbuf_addf(&sb, "%s/logs", gitdir);
        iter->dir_iterator = dir_iterator_begin(sb.buf);
        iter->ref_store = ref_store;
        strbuf_release(&sb);
 +
        return ref_iterator;
  }
  
 +static enum iterator_selection reflog_iterator_select(
 +      struct ref_iterator *iter_worktree,
 +      struct ref_iterator *iter_common,
 +      void *cb_data)
 +{
 +      if (iter_worktree) {
 +              /*
 +               * We're a bit loose here. We probably should ignore
 +               * common refs if they are accidentally added as
 +               * per-worktree refs.
 +               */
 +              return ITER_SELECT_0;
 +      } else if (iter_common) {
 +              if (ref_type(iter_common->refname) == REF_TYPE_NORMAL)
 +                      return ITER_SELECT_1;
 +
 +              /*
 +               * The main ref store may contain main worktree's
 +               * per-worktree refs, which should be ignored
 +               */
 +              return ITER_SKIP_1;
 +      } else
 +              return ITER_DONE;
 +}
 +
 +static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store)
 +{
 +      struct files_ref_store *refs =
 +              files_downcast(ref_store, REF_STORE_READ,
 +                             "reflog_iterator_begin");
 +
 +      if (!strcmp(refs->gitdir, refs->gitcommondir)) {
 +              return reflog_iterator_begin(ref_store, refs->gitcommondir);
 +      } else {
 +              return merge_ref_iterator_begin(
 +                      0,
 +                      reflog_iterator_begin(ref_store, refs->gitdir),
 +                      reflog_iterator_begin(ref_store, refs->gitcommondir),
 +                      reflog_iterator_select, refs);
 +      }
 +}
 +
  /*
   * If update is a direct update of head_ref (the reference pointed to
   * by HEAD), then add an extra REF_LOG_ONLY update for HEAD.
@@@ -2133,10 -2115,11 +2133,10 @@@ static int split_head_update(struct ref
  
        /*
         * First make sure that HEAD is not already in the
 -       * transaction. This insertion is O(N) in the transaction
 +       * transaction. This check is O(lg N) in the transaction
         * size, but it happens at most once per transaction.
         */
 -      item = string_list_insert(affected_refnames, "HEAD");
 -      if (item->util) {
 +      if (string_list_has_string(affected_refnames, "HEAD")) {
                /* An entry already existed */
                strbuf_addf(err,
                            "multiple updates for 'HEAD' (including one "
                        update->new_oid.hash, update->old_oid.hash,
                        update->msg);
  
 +      /*
 +       * Add "HEAD". This insertion is O(N) in the transaction
 +       * size, but it happens at most once per transaction.
 +       * Add new_update->refname instead of a literal "HEAD".
 +       */
 +      if (strcmp(new_update->refname, "HEAD"))
 +              BUG("%s unexpectedly not 'HEAD'", new_update->refname);
 +      item = string_list_insert(affected_refnames, new_update->refname);
        item->util = new_update;
  
        return 0;
@@@ -2185,12 -2160,13 +2185,12 @@@ static int split_symref_update(struct f
  
        /*
         * First make sure that referent is not already in the
 -       * transaction. This insertion is O(N) in the transaction
 +       * transaction. This check is O(lg N) in the transaction
         * size, but it happens at most once per symref in a
         * transaction.
         */
 -      item = string_list_insert(affected_refnames, referent);
 -      if (item->util) {
 -              /* An entry already existed */
 +      if (string_list_has_string(affected_refnames, referent)) {
 +              /* An entry already exists */
                strbuf_addf(err,
                            "multiple updates for '%s' (including one "
                            "via symref '%s') are not allowed",
        update->flags |= REF_LOG_ONLY | REF_NODEREF;
        update->flags &= ~REF_HAVE_OLD;
  
 +      /*
 +       * Add the referent. This insertion is O(N) in the transaction
 +       * size, but it happens at most once per symref in a
 +       * transaction. Make sure to add new_update->refname, which will
 +       * be valid as long as affected_refnames is in use, and NOT
 +       * referent, which might soon be freed by our caller.
 +       */
 +      item = string_list_insert(affected_refnames, new_update->refname);
 +      if (item->util)
 +              BUG("%s unexpectedly found in affected_refnames",
 +                  new_update->refname);
        item->util = new_update;
  
        return 0;
@@@ -2307,7 -2272,7 +2307,7 @@@ static int lock_ref_for_update(struct f
        struct strbuf referent = STRBUF_INIT;
        int mustexist = (update->flags & REF_HAVE_OLD) &&
                !is_null_oid(&update->old_oid);
 -      int ret;
 +      int ret = 0;
        struct ref_lock *lock;
  
        files_assert_main_repository(refs, "lock_ref_for_update");
                ret = split_head_update(update, transaction, head_ref,
                                        affected_refnames, err);
                if (ret)
 -                      return ret;
 +                      goto out;
        }
  
        ret = lock_raw_ref(refs, update->refname, mustexist,
                strbuf_addf(err, "cannot lock ref '%s': %s",
                            original_update_refname(update), reason);
                free(reason);
 -              return ret;
 +              goto out;
        }
  
        update->backend_data = lock;
                                        strbuf_addf(err, "cannot lock ref '%s': "
                                                    "error reading reference",
                                                    original_update_refname(update));
 -                                      return -1;
 +                                      ret = TRANSACTION_GENERIC_ERROR;
 +                                      goto out;
                                }
                        } else if (check_old_oid(update, &lock->old_oid, err)) {
 -                              return TRANSACTION_GENERIC_ERROR;
 +                              ret = TRANSACTION_GENERIC_ERROR;
 +                              goto out;
                        }
                } else {
                        /*
                                                  referent.buf, transaction,
                                                  affected_refnames, err);
                        if (ret)
 -                              return ret;
 +                              goto out;
                }
        } else {
                struct ref_update *parent_update;
  
 -              if (check_old_oid(update, &lock->old_oid, err))
 -                      return TRANSACTION_GENERIC_ERROR;
 +              if (check_old_oid(update, &lock->old_oid, err)) {
 +                      ret = TRANSACTION_GENERIC_ERROR;
 +                      goto out;
 +              }
  
                /*
                 * If this update is happening indirectly because of a
                                    "cannot update ref '%s': %s",
                                    update->refname, write_err);
                        free(write_err);
 -                      return TRANSACTION_GENERIC_ERROR;
 +                      ret = TRANSACTION_GENERIC_ERROR;
 +                      goto out;
                } else {
                        update->flags |= REF_NEEDS_COMMIT;
                }
                 * the lockfile is still open. Close it to
                 * free up the file descriptor:
                 */
 -              if (close_ref(lock)) {
 +              if (close_ref_gently(lock)) {
                        strbuf_addf(err, "couldn't close '%s.lock'",
                                    update->refname);
 -                      return TRANSACTION_GENERIC_ERROR;
 +                      ret = TRANSACTION_GENERIC_ERROR;
 +                      goto out;
                }
        }
 -      return 0;
 +
 +out:
 +      strbuf_release(&referent);
 +      return ret;
  }
  
  struct files_transaction_backend_data {
@@@ -2494,6 -2450,7 +2494,6 @@@ static int files_transaction_prepare(st
        struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
        char *head_ref = NULL;
        int head_type;
 -      struct object_id head_oid;
        struct files_transaction_backend_data *backend_data;
        struct ref_transaction *packed_transaction = NULL;
  
         */
        head_ref = refs_resolve_refdup(ref_store, "HEAD",
                                       RESOLVE_REF_NO_RECURSE,
 -                                     head_oid.hash, &head_type);
 +                                     NULL, &head_type);
  
        if (head_ref && !(head_type & REF_ISSYMREF)) {
                FREE_AND_NULL(head_ref);
                ret = lock_ref_for_update(refs, update, transaction,
                                          head_ref, &affected_refnames, err);
                if (ret)
-                       break;
+                       goto cleanup;
  
                if (update->flags & REF_DELETING &&
                    !(update->flags & REF_LOG_ONLY) &&
@@@ -2993,17 -2950,16 +2993,17 @@@ static int files_reflog_expire(struct r
                        !(type & REF_ISSYMREF) &&
                        !is_null_oid(&cb.last_kept_oid);
  
 -              if (close_lock_file(&reflog_lock)) {
 +              if (close_lock_file_gently(&reflog_lock)) {
                        status |= error("couldn't write %s: %s", log_file,
                                        strerror(errno));
 +                      rollback_lock_file(&reflog_lock);
                } else if (update &&
 -                         (write_in_full(get_lock_file_fd(lock->lk),
 -                              oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
 -                          write_str_in_full(get_lock_file_fd(lock->lk), "\n") != 1 ||
 -                          close_ref(lock) < 0)) {
 +                         (write_in_full(get_lock_file_fd(&lock->lk),
 +                              oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) < 0 ||
 +                          write_str_in_full(get_lock_file_fd(&lock->lk), "\n") < 0 ||
 +                          close_ref_gently(lock) < 0)) {
                        status |= error("couldn't write %s",
 -                                      get_lock_file_path(lock->lk));
 +                                      get_lock_file_path(&lock->lk));
                        rollback_lock_file(&reflog_lock);
                } else if (commit_lock_file(&reflog_lock)) {
                        status |= error("unable to write reflog '%s' (%s)",
@@@ -3054,10 -3010,10 +3054,10 @@@ struct ref_storage_be refs_be_files = 
        files_initial_transaction_commit,
  
        files_pack_refs,
 -      files_peel_ref,
        files_create_symref,
        files_delete_refs,
        files_rename_ref,
 +      files_copy_ref,
  
        files_ref_iterator_begin,
        files_read_raw_ref,