[PATCH v2 00/33] Yet more preparation for reference backends

classic Classic list List threaded Threaded
48 messages Options
123
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 20/33] ref_transaction_commit(): correctly report close_ref() failure

Michael Haggerty-2
Signed-off-by: Michael Haggerty <[hidden email]>
---
 refs/files-backend.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7cc680a..f4f7953 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3139,6 +3139,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  if (close_ref(update->lock)) {
  strbuf_addf(err, "couldn't close '%s.lock'",
     update->refname);
+ ret = TRANSACTION_GENERIC_ERROR;
  goto cleanup;
  }
  }
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 21/33] delete_branches(): use resolve_refdup()

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
The return value of resolve_ref_unsafe() is not guaranteed to stay
around as long as we need it, so use resolve_refdup() instead.

Signed-off-by: Michael Haggerty <[hidden email]>
---
 builtin/branch.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 0adba62..ae55688 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -212,7 +212,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
  die(_("Couldn't look up commit object for HEAD"));
  }
  for (i = 0; i < argc; i++, strbuf_release(&bname)) {
- const char *target;
+ char *target = NULL;
  int flags = 0;
 
  strbuf_branchname(&bname, argv[i]);
@@ -231,11 +231,11 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
  }
  }
 
- target = resolve_ref_unsafe(name,
-    RESOLVE_REF_READING
-    | RESOLVE_REF_NO_RECURSE
-    | RESOLVE_REF_ALLOW_BAD_NAME,
-    sha1, &flags);
+ target = resolve_refdup(name,
+ RESOLVE_REF_READING
+ | RESOLVE_REF_NO_RECURSE
+ | RESOLVE_REF_ALLOW_BAD_NAME,
+ sha1, &flags);
  if (!target) {
  error(remote_branch
       ? _("remote-tracking branch '%s' not found.")
@@ -248,7 +248,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
     check_branch_commit(bname.buf, name, sha1, head_rev, kinds,
  force)) {
  ret = 1;
- continue;
+ goto next;
  }
 
  if (delete_ref(name, is_null_sha1(sha1) ? NULL : sha1,
@@ -258,7 +258,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
       : _("Error deleting branch '%s'"),
       bname.buf);
  ret = 1;
- continue;
+ goto next;
  }
  if (!quiet) {
  printf(remote_branch
@@ -270,6 +270,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
        : find_unique_abbrev(sha1, DEFAULT_ABBREV));
  }
  delete_branch_config(bname.buf);
+
+ next:
+ free(target);
  }
 
  free(name);
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 22/33] refs: allow log-only updates

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
From: David Turner <[hidden email]>

The refs infrastructure learns about log-only ref updates, which only
update the reflog.  Later, we will use this to separate symbolic
reference resolution from ref updating.

Signed-off-by: David Turner <[hidden email]>
Signed-off-by: Junio C Hamano <[hidden email]>
Signed-off-by: Michael Haggerty <[hidden email]>
---
 refs/files-backend.c | 16 ++++++++++------
 refs/refs-internal.h |  7 +++++++
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f4f7953..3f546d0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2683,7 +2683,7 @@ static int commit_ref_update(struct ref_lock *lock,
  }
  }
  }
- if (commit_ref(lock)) {
+ if (!(flags & REF_LOG_ONLY) && commit_ref(lock)) {
  strbuf_addf(err, "couldn't set '%s'", lock->ref_name);
  unlock_ref(lock);
  return -1;
@@ -3101,7 +3101,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  goto cleanup;
  }
  if ((update->flags & REF_HAVE_NEW) &&
-    !(update->flags & REF_DELETING)) {
+    !(update->flags & REF_DELETING) &&
+    !(update->flags & REF_LOG_ONLY)) {
  int overwriting_symref = ((update->type & REF_ISSYMREF) &&
   (update->flags & REF_NODEREF));
 
@@ -3133,8 +3134,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  }
  if (!(update->flags & REF_NEEDS_COMMIT)) {
  /*
- * We didn't have to write anything to the lockfile.
- * Close it to free up the file descriptor:
+ * We didn't call write_ref_to_lockfile(), so
+ * the lockfile is still open. Close it to
+ * free up the file descriptor:
  */
  if (close_ref(update->lock)) {
  strbuf_addf(err, "couldn't close '%s.lock'",
@@ -3149,7 +3151,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  for (i = 0; i < transaction->nr; i++) {
  struct ref_update *update = updates[i];
 
- if (update->flags & REF_NEEDS_COMMIT) {
+ if (update->flags & REF_NEEDS_COMMIT ||
+    update->flags & REF_LOG_ONLY) {
  if (commit_ref_update(update->lock,
       update->new_sha1, update->msg,
       update->flags, err)) {
@@ -3168,7 +3171,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  for (i = 0; i < transaction->nr; i++) {
  struct ref_update *update = updates[i];
 
- if (update->flags & REF_DELETING) {
+ if (update->flags & REF_DELETING &&
+    !(update->flags & REF_LOG_ONLY)) {
  if (delete_ref_loose(update->lock, update->type, err)) {
  ret = TRANSACTION_GENERIC_ERROR;
  goto cleanup;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 1f94f7a..85f4650 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -43,6 +43,13 @@
  */
 
 /*
+ * Used as a flag in ref_update::flags when we want to log a ref
+ * update but not actually perform it.  This is used when a symbolic
+ * ref update is split up.
+ */
+#define REF_LOG_ONLY 0x80
+
+/*
  * Return true iff refname is minimally safe. "Safe" here means that
  * deleting a loose reference by this name will not do any damage, for
  * example by causing a file that is not a reference to be deleted.
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 23/33] refs: don't dereference on rename

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
From: David Turner <[hidden email]>

When renaming refs, don't dereference either the origin or the destination
before renaming.

The origin does not need to be dereferenced because it is presently
forbidden to rename symbolic refs.

Not dereferencing the destination fixes a bug where renaming on top of
a broken symref would use the pointed-to ref name for the moved
reflog.

Add a test for the reflog bug.

Signed-off-by: David Turner <[hidden email]>
Signed-off-by: Junio C Hamano <[hidden email]>
Signed-off-by: Michael Haggerty <[hidden email]>
---
 refs/files-backend.c | 21 ++++++++++++++++-----
 t/t3200-branch.sh    |  9 +++++++++
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3f546d0..ad53b6e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2333,7 +2333,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
  if (log && S_ISLNK(loginfo.st_mode))
  return error("reflog for %s is a symlink", oldrefname);
 
- if (!resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING, orig_sha1, &flag))
+ if (!resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+ orig_sha1, &flag))
  return error("refname %s not found", oldrefname);
 
  if (flag & REF_ISSYMREF)
@@ -2351,8 +2352,16 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
  goto rollback;
  }
 
- if (!read_ref_full(newrefname, RESOLVE_REF_READING, sha1, NULL) &&
-    delete_ref(newrefname, sha1, REF_NODEREF)) {
+ /*
+ * Since we are doing a shallow lookup, sha1 is not the
+ * correct value to pass to delete_ref as old_sha1. But that
+ * doesn't matter, because an old_sha1 check wouldn't add to
+ * the safety anyway; we want to delete the reference whatever
+ * its current value.
+ */
+ if (!read_ref_full(newrefname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+   sha1, NULL) &&
+    delete_ref(newrefname, NULL, REF_NODEREF)) {
  if (errno==EISDIR) {
  struct strbuf path = STRBUF_INIT;
  int result;
@@ -2376,7 +2385,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 
  logmoved = log;
 
- lock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, 0, NULL, &err);
+ lock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, REF_NODEREF,
+   NULL, &err);
  if (!lock) {
  error("unable to rename '%s' to '%s': %s", oldrefname, newrefname, err.buf);
  strbuf_release(&err);
@@ -2394,7 +2404,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
  return 0;
 
  rollback:
- lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, 0, NULL, &err);
+ lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, REF_NODEREF,
+   NULL, &err);
  if (!lock) {
  error("unable to lock %s for rollback: %s", oldrefname, err.buf);
  strbuf_release(&err);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index f3e3b6c..4281160 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -79,6 +79,15 @@ test_expect_success 'git branch -m dumps usage' '
  test_i18ngrep "branch name required" err
 '
 
+test_expect_success 'git branch -m m broken_symref should work' '
+ test_when_finished "git branch -D broken_symref" &&
+ git branch -l m &&
+ git symbolic-ref refs/heads/broken_symref refs/heads/i_am_broken &&
+ git branch -m m broken_symref &&
+ git reflog exists refs/heads/broken_symref &&
+ test_must_fail git reflog exists refs/heads/i_am_broken
+'
+
 test_expect_success 'git branch -m m m/m should work' '
  git branch -l m &&
  git branch -m m m/m &&
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 24/33] verify_refname_available(): adjust constness in declaration

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
The two string_list arguments can be const.

Signed-off-by: Michael Haggerty <[hidden email]>
---
 refs/files-backend.c | 4 ++--
 refs/refs-internal.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index ad53b6e..f0eef9e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2299,8 +2299,8 @@ out:
 }
 
 int verify_refname_available(const char *newname,
-     struct string_list *extras,
-     struct string_list *skip,
+     const struct string_list *extras,
+     const struct string_list *skip,
      struct strbuf *err)
 {
  struct ref_dir *packed_refs = get_packed_refs(&ref_cache);
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 85f4650..9686e60 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -116,8 +116,8 @@ enum peel_status peel_object(const unsigned char *name, unsigned char *sha1);
  * extras and skip must be sorted.
  */
 int verify_refname_available(const char *newname,
-     struct string_list *extras,
-     struct string_list *skip,
+     const struct string_list *extras,
+     const struct string_list *skip,
      struct strbuf *err);
 
 /*
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 25/33] add_update(): initialize the whole ref_update

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
Change add_update() to initialize all of the fields in the new
ref_update object. Rename the function to ref_transaction_add_update(),
and increase its visibility to all of the refs-related code.

All of this makes the function more useful for other future callers.

Signed-off-by: Michael Haggerty <[hidden email]>
---
 refs.c               | 48 ++++++++++++++++++++++++++----------------------
 refs/refs-internal.h | 14 ++++++++++++++
 2 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/refs.c b/refs.c
index 5dc2473..7c4eeb1 100644
--- a/refs.c
+++ b/refs.c
@@ -766,13 +766,33 @@ void ref_transaction_free(struct ref_transaction *transaction)
  free(transaction);
 }
 
-static struct ref_update *add_update(struct ref_transaction *transaction,
-     const char *refname)
+struct ref_update *ref_transaction_add_update(
+ struct ref_transaction *transaction,
+ const char *refname, unsigned int flags,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
+ const char *msg)
 {
  struct ref_update *update;
+
+ if (transaction->state != REF_TRANSACTION_OPEN)
+ die("BUG: update called for transaction that is not open");
+
+ if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
+ die("BUG: REF_ISPRUNING set without REF_NODEREF");
+
  FLEX_ALLOC_STR(update, refname, refname);
  ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
  transaction->updates[transaction->nr++] = update;
+
+ update->flags = flags;
+
+ if (flags & REF_HAVE_NEW)
+ hashcpy(update->new_sha1, new_sha1);
+ if (flags & REF_HAVE_OLD)
+ hashcpy(update->old_sha1, old_sha1);
+ if (msg)
+ update->msg = xstrdup(msg);
  return update;
 }
 
@@ -783,16 +803,8 @@ int ref_transaction_update(struct ref_transaction *transaction,
    unsigned int flags, const char *msg,
    struct strbuf *err)
 {
- struct ref_update *update;
-
  assert(err);
 
- if (transaction->state != REF_TRANSACTION_OPEN)
- die("BUG: update called for transaction that is not open");
-
- if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
- die("BUG: REF_ISPRUNING set without REF_NODEREF");
-
  if (new_sha1 && !is_null_sha1(new_sha1) &&
     check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
  strbuf_addf(err, "refusing to update ref with bad name '%s'",
@@ -800,18 +812,10 @@ int ref_transaction_update(struct ref_transaction *transaction,
  return -1;
  }
 
- update = add_update(transaction, refname);
- if (new_sha1) {
- hashcpy(update->new_sha1, new_sha1);
- flags |= REF_HAVE_NEW;
- }
- if (old_sha1) {
- hashcpy(update->old_sha1, old_sha1);
- flags |= REF_HAVE_OLD;
- }
- update->flags = flags;
- if (msg)
- update->msg = xstrdup(msg);
+ flags |= (new_sha1 ? REF_HAVE_NEW : 0) | (old_sha1 ? REF_HAVE_OLD : 0);
+
+ ref_transaction_add_update(transaction, refname, flags,
+   new_sha1, old_sha1, msg);
  return 0;
 }
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 9686e60..babdf27 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -158,6 +158,20 @@ struct ref_update {
 };
 
 /*
+ * Add a ref_update with the specified properties to transaction, and
+ * return a pointer to the new object. This function does not verify
+ * that refname is well-formed. new_sha1 and old_sha1 are only
+ * dereferenced if the REF_HAVE_NEW and REF_HAVE_OLD bits,
+ * respectively, are set in flags.
+ */
+struct ref_update *ref_transaction_add_update(
+ struct ref_transaction *transaction,
+ const char *refname, unsigned int flags,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
+ const char *msg);
+
+/*
  * Transaction states.
  * OPEN:   The transaction is in a valid state and can accept new updates.
  *         An OPEN transaction can be committed.
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 26/33] lock_ref_for_update(): new function

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
Extract a new function, lock_ref_for_update(), from
ref_transaction_commit().

Signed-off-by: Michael Haggerty <[hidden email]>
---
 refs/files-backend.c | 152 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 85 insertions(+), 67 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f0eef9e..1c20af5 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3051,6 +3051,88 @@ static int ref_update_reject_duplicates(struct string_list *refnames,
  return 0;
 }
 
+/*
+ * Acquire all locks, verify old values if provided, check
+ * that new values are valid, and write new values to the
+ * lockfiles, ready to be activated. Only keep one lockfile
+ * open at a time to avoid running out of file descriptors.
+ */
+static int lock_ref_for_update(struct ref_update *update,
+       struct ref_transaction *transaction,
+       struct string_list *affected_refnames,
+       struct strbuf *err)
+{
+ int ret;
+
+ if ((update->flags & REF_HAVE_NEW) &&
+    is_null_sha1(update->new_sha1))
+ update->flags |= REF_DELETING;
+ update->lock = lock_ref_sha1_basic(
+ update->refname,
+ ((update->flags & REF_HAVE_OLD) ?
+ update->old_sha1 : NULL),
+ affected_refnames, NULL,
+ update->flags,
+ &update->type,
+ err);
+ if (!update->lock) {
+ char *reason;
+
+ ret = (errno == ENOTDIR)
+ ? TRANSACTION_NAME_CONFLICT
+ : TRANSACTION_GENERIC_ERROR;
+ reason = strbuf_detach(err, NULL);
+ strbuf_addf(err, "cannot lock ref '%s': %s",
+    update->refname, reason);
+ free(reason);
+ return ret;
+ }
+ if ((update->flags & REF_HAVE_NEW) &&
+    !(update->flags & REF_DELETING) &&
+    !(update->flags & REF_LOG_ONLY)) {
+ int overwriting_symref = ((update->type & REF_ISSYMREF) &&
+  (update->flags & REF_NODEREF));
+
+ if (!overwriting_symref &&
+    !hashcmp(update->lock->old_oid.hash, update->new_sha1)) {
+ /*
+ * The reference already has the desired
+ * value, so we don't need to write it.
+ */
+ } else if (write_ref_to_lockfile(update->lock,
+ update->new_sha1,
+ err)) {
+ char *write_err = strbuf_detach(err, NULL);
+
+ /*
+ * The lock was freed upon failure of
+ * write_ref_to_lockfile():
+ */
+ update->lock = NULL;
+ strbuf_addf(err,
+    "cannot update the ref '%s': %s",
+    update->refname, write_err);
+ free(write_err);
+ return TRANSACTION_GENERIC_ERROR;
+ } else {
+ update->flags |= REF_NEEDS_COMMIT;
+ }
+ }
+ if (!(update->flags & REF_NEEDS_COMMIT)) {
+ /*
+ * We didn't call write_ref_to_lockfile(), so
+ * the lockfile is still open. Close it to
+ * free up the file descriptor:
+ */
+ if (close_ref(update->lock)) {
+ strbuf_addf(err, "couldn't close '%s.lock'",
+    update->refname);
+ return TRANSACTION_GENERIC_ERROR;
+ }
+ }
+ return 0;
+}
+
 int ref_transaction_commit(struct ref_transaction *transaction,
    struct strbuf *err)
 {
@@ -3088,74 +3170,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  for (i = 0; i < transaction->nr; i++) {
  struct ref_update *update = updates[i];
 
- if ((update->flags & REF_HAVE_NEW) &&
-    is_null_sha1(update->new_sha1))
- update->flags |= REF_DELETING;
- update->lock = lock_ref_sha1_basic(
- update->refname,
- ((update->flags & REF_HAVE_OLD) ?
- update->old_sha1 : NULL),
- &affected_refnames, NULL,
- update->flags,
- &update->type,
- err);
- if (!update->lock) {
- char *reason;
-
- ret = (errno == ENOTDIR)
- ? TRANSACTION_NAME_CONFLICT
- : TRANSACTION_GENERIC_ERROR;
- reason = strbuf_detach(err, NULL);
- strbuf_addf(err, "cannot lock ref '%s': %s",
-    update->refname, reason);
- free(reason);
+ ret = lock_ref_for_update(update, transaction,
+  &affected_refnames, err);
+ if (ret)
  goto cleanup;
- }
- if ((update->flags & REF_HAVE_NEW) &&
-    !(update->flags & REF_DELETING) &&
-    !(update->flags & REF_LOG_ONLY)) {
- int overwriting_symref = ((update->type & REF_ISSYMREF) &&
-  (update->flags & REF_NODEREF));
-
- if (!overwriting_symref &&
-    !hashcmp(update->lock->old_oid.hash, update->new_sha1)) {
- /*
- * The reference already has the desired
- * value, so we don't need to write it.
- */
- } else if (write_ref_to_lockfile(update->lock,
- update->new_sha1,
- err)) {
- char *write_err = strbuf_detach(err, NULL);
-
- /*
- * The lock was freed upon failure of
- * write_ref_to_lockfile():
- */
- update->lock = NULL;
- strbuf_addf(err,
-    "cannot update the ref '%s': %s",
-    update->refname, write_err);
- free(write_err);
- ret = TRANSACTION_GENERIC_ERROR;
- goto cleanup;
- } else {
- update->flags |= REF_NEEDS_COMMIT;
- }
- }
- if (!(update->flags & REF_NEEDS_COMMIT)) {
- /*
- * We didn't call write_ref_to_lockfile(), so
- * the lockfile is still open. Close it to
- * free up the file descriptor:
- */
- if (close_ref(update->lock)) {
- strbuf_addf(err, "couldn't close '%s.lock'",
-    update->refname);
- ret = TRANSACTION_GENERIC_ERROR;
- goto cleanup;
- }
- }
  }
 
  /* Perform updates first so live commits remain referenced */
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 27/33] unlock_ref(): move definition higher in the file

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
This avoids the need for a forward declaration in the next patch.

Signed-off-by: Michael Haggerty <[hidden email]>
---
 refs/files-backend.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1c20af5..8ff76ed 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1516,6 +1516,16 @@ out:
  return ret;
 }
 
+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);
+ free(lock->ref_name);
+ free(lock->orig_ref_name);
+ free(lock);
+}
+
 /*
  * Peel the entry (if possible) and return its new peel_status.  If
  * repeel is true, re-peel the entry even if there is an old peeled
@@ -1674,16 +1684,6 @@ int do_for_each_ref(const char *submodule, const char *base,
  return do_for_each_entry(refs, base, do_one_ref, &data);
 }
 
-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);
- free(lock->ref_name);
- free(lock->orig_ref_name);
- free(lock);
-}
-
 /*
  * Verify that the reference locked by lock has the value old_sha1.
  * Fail if the reference doesn't exist and mustexist is set. Return 0
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 28/33] ref_transaction_update(): check refname_is_safe() at a minimum

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
If the user has asked that a new value be set for a reference, we use
check_refname_format() to verify that the reference name satisfies all
of the rules. But in other cases, at least check that refname_is_safe().

Signed-off-by: Michael Haggerty <[hidden email]>
---
 refs.c                  | 5 +++--
 t/t1400-update-ref.sh   | 2 +-
 t/t1430-bad-ref-name.sh | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 7c4eeb1..842c5c7 100644
--- a/refs.c
+++ b/refs.c
@@ -805,8 +805,9 @@ int ref_transaction_update(struct ref_transaction *transaction,
 {
  assert(err);
 
- if (new_sha1 && !is_null_sha1(new_sha1) &&
-    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+ if ((new_sha1 && !is_null_sha1(new_sha1)) ?
+    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
+    !refname_is_safe(refname)) {
  strbuf_addf(err, "refusing to update ref with bad name '%s'",
     refname);
  return -1;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 40b0cce..08bd8fd 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -23,7 +23,7 @@ test_expect_success setup '
 m=refs/heads/master
 n_dir=refs/heads/gu
 n=$n_dir/fixes
-outside=foo
+outside=refs/foo
 
 test_expect_success \
  "create $m" \
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index 25ddab4..8937e25 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -285,7 +285,7 @@ test_expect_success 'update-ref -d cannot delete non-ref in .git dir' '
  echo precious >expect &&
  test_must_fail git update-ref -d my-private-file >output 2>error &&
  test_must_be_empty output &&
- test_i18ngrep -e "cannot lock .*: unable to resolve reference" error &&
+ test_i18ngrep -e "refusing to update ref with bad name" error &&
  test_cmp expect .git/my-private-file
 '
 
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 29/33] refs: resolve symbolic refs first

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
Before committing ref updates, split symbolic ref updates into two
parts: an update to the underlying ref, and a log-only update to the
symbolic ref. This ensures that both references are locked correctly
during the transaction, including while their reflogs are updated.

Similarly, if the reference pointed to by HEAD is modified directly, add
a separate log-only update to HEAD, rather than leaving the job of
updating HEAD's reflog to commit_ref_update(). This change ensures that
HEAD is locked correctly while its reflog is being modified, as well as
being cheaper (HEAD only needs to be resolved once).

This makes use of a new function, lock_ref_raw(), which is analogous to
read_ref_raw(), but acquires a lock on the reference before reading it.

This change still has two problems:

* There are redundant read_ref_full() reference lookups.

* It is still possible to get incorrect reflogs for symbolic references
  if there is a concurrent update by another process, since the old_oid
  of a symref is determined before the lock on the pointed-to ref is
  held.

Both problems will soon be fixed.

Signed-off-by: David Turner <[hidden email]>
Signed-off-by: Junio C Hamano <[hidden email]>
Signed-off-by: Michael Haggerty <[hidden email]>
---
 refs/files-backend.c  | 513 +++++++++++++++++++++++++++++++++++++++++++++-----
 refs/refs-internal.h  |  11 +-
 t/t1400-update-ref.sh |  35 ++++
 3 files changed, 514 insertions(+), 45 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8ff76ed..799866f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1527,6 +1527,228 @@ static void unlock_ref(struct ref_lock *lock)
 }
 
 /*
+ * Lock refname, without following symrefs, and set *lock_p to point
+ * at a newly-allocated lock object. Fill in lock->old_oid, referent,
+ * and type similarly to read_raw_ref().
+ *
+ * The caller must verify that refname is a "safe" reference name (in
+ * the sense of refname_is_safe()) before calling this function.
+ *
+ * If the reference doesn't already exist, verify that refname doesn't
+ * have a D/F conflict with any existing references. extras and skip
+ * are passed to verify_refname_available_dir() for this check.
+ *
+ * If mustexist is not set and the reference is not found or is
+ * broken, lock the reference anyway but clear sha1.
+ *
+ * Return 0 on success. On failure, write an error message to err and
+ * return TRANSACTION_NAME_CONFLICT or TRANSACTION_GENERIC_ERROR.
+ *
+ * Implementation note: This function is basically
+ *
+ *     lock reference
+ *     read_raw_ref()
+ *
+ * but it includes a lot more code to
+ * - Deal with possible races with other processes
+ * - Avoid calling verify_refname_available_dir() when it can be
+ *   avoided, namely if we were successfully able to read the ref
+ * - Generate informative error messages in the case of failure
+ */
+static int lock_raw_ref(const char *refname, int mustexist,
+ const struct string_list *extras,
+ const struct string_list *skip,
+ struct ref_lock **lock_p,
+ struct strbuf *referent,
+ unsigned int *type,
+ struct strbuf *err)
+{
+ struct ref_lock *lock;
+ struct strbuf ref_file = STRBUF_INIT;
+ int attempts_remaining = 3;
+ int ret = TRANSACTION_GENERIC_ERROR;
+
+ assert(err);
+ *type = 0;
+
+ /* First lock the file so it can't change out from under us. */
+
+ *lock_p = lock = xcalloc(1, sizeof(*lock));
+
+ lock->ref_name = xstrdup(refname);
+ lock->orig_ref_name = xstrdup(refname);
+ strbuf_git_path(&ref_file, "%s", refname);
+
+retry:
+ switch (safe_create_leading_directories(ref_file.buf)) {
+ case SCLD_OK:
+ break; /* success */
+ case SCLD_EXISTS:
+ /*
+ * Suppose refname is "refs/foo/bar". We just failed
+ * to create the containing directory, "refs/foo",
+ * because there was a non-directory in the way. This
+ * indicates a D/F conflict, probably because of
+ * another reference such as "refs/foo". There is no
+ * reason to expect this error to be transitory.
+ */
+ if (verify_refname_available(refname, extras, skip, err)) {
+ if (mustexist) {
+ /*
+ * To the user the relevant error is
+ * that the "mustexist" reference is
+ * missing:
+ */
+ strbuf_reset(err);
+ strbuf_addf(err, "unable to resolve reference '%s'",
+    refname);
+ } else {
+ /*
+ * The error message set by
+ * verify_refname_available_dir() is OK.
+ */
+ ret = TRANSACTION_NAME_CONFLICT;
+ }
+ } else {
+ /*
+ * The file that is in the way isn't a loose
+ * reference. Report it as a low-level
+ * failure.
+ */
+ strbuf_addf(err, "unable to create lock file %s.lock; "
+    "non-directory in the way",
+    ref_file.buf);
+ }
+ goto error_return;
+ case SCLD_VANISHED:
+ /* Maybe another process was tidying up. Try again. */
+ if (--attempts_remaining > 0)
+ goto retry;
+ /* fall through */
+ default:
+ strbuf_addf(err, "unable to create directory for %s",
+    ref_file.buf);
+ goto error_return;
+ }
+
+ if (!lock->lk)
+ lock->lk = xcalloc(1, sizeof(struct lock_file));
+
+ if (hold_lock_file_for_update(lock->lk, ref_file.buf, LOCK_NO_DEREF) < 0) {
+ if (errno == ENOENT && --attempts_remaining > 0) {
+ /*
+ * Maybe somebody just deleted one of the
+ * directories leading to ref_file.  Try
+ * again:
+ */
+ goto retry;
+ } else {
+ unable_to_lock_message(ref_file.buf, errno, err);
+ goto error_return;
+ }
+ }
+
+ /*
+ * Now we hold the lock and can read the reference without
+ * fear that its value will change.
+ */
+
+ if (read_raw_ref(refname, lock->old_oid.hash, referent, type)) {
+ if (errno == ENOENT) {
+ if (mustexist) {
+ /* Garden variety missing reference. */
+ strbuf_addf(err, "unable to resolve reference '%s'",
+    refname);
+ goto error_return;
+ } else if (verify_refname_available_dir(
+   refname, extras, skip,
+   get_loose_refs(&ref_cache),
+   err)) {
+ /*
+ * The error message set by
+ * verify_refname_available() is OK.
+ */
+ ret = TRANSACTION_NAME_CONFLICT;
+ goto error_return;
+ } else {
+ /* Reference is missing, but that's OK */
+ }
+ } else if (errno == EISDIR) {
+ /*
+ * There is a directory in the way. It might have
+ * contained references that have been deleted. If
+ * we don't require that the reference already
+ * exists, try to remove the directory so that it
+ * doesn't cause trouble when we want to rename the
+ * lockfile into place later.
+ */
+ if (mustexist) {
+ /* Garden variety missing reference. */
+ strbuf_addf(err, "unable to resolve reference '%s'",
+    refname);
+ goto error_return;
+ } else if (remove_dir_recursively(&ref_file,
+  REMOVE_DIR_EMPTY_ONLY)) {
+ if (verify_refname_available_dir(
+    refname, extras, skip,
+    get_loose_refs(&ref_cache),
+    err)) {
+ /*
+ * The error message set by
+ * verify_refname_available() is OK.
+ */
+ ret = TRANSACTION_NAME_CONFLICT;
+ goto error_return;
+ } else {
+ /*
+ * We can't delete the directory,
+ * but we also don't know of any
+ * references that it should
+ * contain.
+ */
+ strbuf_addf(err, "there is a non-empty directory '%s' "
+    "blocking reference '%s'",
+    ref_file.buf, refname);
+ goto error_return;
+ }
+ }
+ } else if (errno == EINVAL && (*type & REF_ISBROKEN)) {
+ strbuf_addf(err, "unable to resolve reference '%s': "
+    "reference broken", refname);
+ goto error_return;
+ } else {
+ strbuf_addf(err, "unable to resolve reference '%s': %s",
+    refname, strerror(errno));
+ goto error_return;
+ }
+
+ /*
+ * If the ref did not exist and we are creating it,
+ * make sure there is no existing packed ref whose
+ * name begins with our refname, nor a packed ref
+ * whose name is a proper prefix of our refname.
+ */
+ if (verify_refname_available_dir(
+    refname, extras, skip,
+    get_packed_refs(&ref_cache),
+    err)) {
+ goto error_return;
+ }
+ }
+
+ ret = 0;
+ goto out;
+
+error_return:
+ unlock_ref(lock);
+ *lock_p = NULL;
+
+out:
+ strbuf_release(&ref_file);
+ return ret;
+}
+
+/*
  * Peel the entry (if possible) and return its new peel_status.  If
  * repeel is true, re-peel the entry even if there is an old peeled
  * value that is already stored in it.
@@ -3052,55 +3274,201 @@ static int ref_update_reject_duplicates(struct string_list *refnames,
 }
 
 /*
- * Acquire all locks, verify old values if provided, check
- * that new values are valid, and write new values to the
- * lockfiles, ready to be activated. Only keep one lockfile
- * open at a time to avoid running out of file descriptors.
+ * 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.
+ */
+static int split_head_update(struct ref_update *update,
+     struct ref_transaction *transaction,
+     const char *head_ref,
+     struct string_list *affected_refnames,
+     struct strbuf *err)
+{
+ struct string_list_item *item;
+ struct ref_update *new_update;
+
+ if ((update->flags & REF_LOG_ONLY) ||
+    (update->flags & REF_ISPRUNING) ||
+    (update->flags & REF_UPDATE_VIA_HEAD))
+ return 0;
+
+ if (strcmp(update->refname, head_ref))
+ return 0;
+
+ /*
+ * First make sure that HEAD is not already in the
+ * transaction. This insertion is O(N) in the transaction
+ * size, but it happens at most once per transaction.
+ */
+ item = string_list_insert(affected_refnames, "HEAD");
+ if (item->util) {
+ /* An entry already existed */
+ strbuf_addf(err,
+    "multiple updates for 'HEAD' (including one "
+    "via its referent '%s') are not allowed",
+    update->refname);
+ return TRANSACTION_NAME_CONFLICT;
+ }
+
+ new_update = ref_transaction_add_update(
+ transaction, "HEAD",
+ update->flags | REF_LOG_ONLY | REF_NODEREF,
+ update->new_sha1, update->old_sha1,
+ update->msg);
+
+ item->util = new_update;
+
+ return 0;
+}
+
+/*
+ * update is for a symref that points at referent and doesn't have
+ * REF_NODEREF set. Split it into two updates:
+ * - The original update, but with REF_LOG_ONLY and REF_NODEREF set
+ * - A new, separate update for the referent reference
+ * Note that the new update will itself be subject to splitting when
+ * the iteration gets to it.
+ */
+static int split_symref_update(struct ref_update *update,
+       const char *referent,
+       struct ref_transaction *transaction,
+       struct string_list *affected_refnames,
+       struct strbuf *err)
+{
+ struct string_list_item *item;
+ struct ref_update *new_update;
+ unsigned int new_flags;
+
+ /*
+ * First make sure that referent is not already in the
+ * transaction. This insertion is O(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 */
+ strbuf_addf(err,
+    "multiple updates for '%s' (including one "
+    "via symref '%s') are not allowed",
+    referent, update->refname);
+ return TRANSACTION_NAME_CONFLICT;
+ }
+
+ new_flags = update->flags;
+ if (!strcmp(update->refname, "HEAD")) {
+ /*
+ * Record that the new update came via HEAD, so that
+ * when we process it, split_head_update() doesn't try
+ * to add another reflog update for HEAD. Note that
+ * this bit will be propagated if the new_update
+ * itself needs to be split.
+ */
+ new_flags |= REF_UPDATE_VIA_HEAD;
+ }
+
+ new_update = ref_transaction_add_update(
+ transaction, referent, new_flags,
+ update->new_sha1, update->old_sha1,
+ update->msg);
+
+ /* Change the symbolic ref update to log only: */
+ update->flags |= REF_LOG_ONLY | REF_NODEREF;
+
+ item->util = new_update;
+
+ return 0;
+}
+
+/*
+ * Prepare for carrying out update:
+ * - Lock the reference referred to by update.
+ * - Read the reference under lock.
+ * - Check that its old SHA-1 value (if specified) is correct, and in
+ *   any case record it for later use in the reflog.
+ * - If it is a symref update without REF_NODEREF, split it up into a
+ *   REF_LOG_ONLY update of the symref and add a separate update for
+ *   the referent to transaction.
+ * - If it is an update of head_ref, add a corresponding REF_LOG_ONLY
+ *   update of HEAD.
  */
 static int lock_ref_for_update(struct ref_update *update,
        struct ref_transaction *transaction,
+       const char *head_ref,
        struct string_list *affected_refnames,
        struct strbuf *err)
 {
+ struct strbuf referent = STRBUF_INIT;
+ int mustexist = (update->flags & REF_HAVE_OLD) &&
+ !is_null_sha1(update->old_sha1);
  int ret;
+ struct ref_lock *lock;
 
- if ((update->flags & REF_HAVE_NEW) &&
-    is_null_sha1(update->new_sha1))
+ if ((update->flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1))
  update->flags |= REF_DELETING;
- update->lock = lock_ref_sha1_basic(
- update->refname,
- ((update->flags & REF_HAVE_OLD) ?
- update->old_sha1 : NULL),
- affected_refnames, NULL,
- update->flags,
- &update->type,
- err);
- if (!update->lock) {
+
+ if (head_ref) {
+ ret = split_head_update(update, transaction, head_ref,
+ affected_refnames, err);
+ if (ret)
+ return ret;
+ }
+
+ ret = lock_raw_ref(update->refname, mustexist,
+   affected_refnames, NULL,
+   &update->lock, &referent,
+   &update->type, err);
+
+ if (ret) {
  char *reason;
 
- ret = (errno == ENOTDIR)
- ? TRANSACTION_NAME_CONFLICT
- : TRANSACTION_GENERIC_ERROR;
  reason = strbuf_detach(err, NULL);
  strbuf_addf(err, "cannot lock ref '%s': %s",
     update->refname, reason);
  free(reason);
  return ret;
  }
+
+ lock = update->lock;
+
+ if (read_ref_full(update->refname,
+  mustexist ? RESOLVE_REF_READING : 0,
+  lock->old_oid.hash, NULL)) {
+ if (update->flags & REF_HAVE_OLD) {
+ strbuf_addf(err, "cannot lock ref '%s': can't resolve old value",
+    update->refname);
+ return TRANSACTION_GENERIC_ERROR;
+ } else {
+ hashclr(lock->old_oid.hash);
+ }
+ }
+ if ((update->flags & REF_HAVE_OLD) &&
+    hashcmp(lock->old_oid.hash, update->old_sha1)) {
+ strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
+    update->refname,
+    sha1_to_hex(lock->old_oid.hash),
+    sha1_to_hex(update->old_sha1));
+ return TRANSACTION_GENERIC_ERROR;
+ }
+
+ if (update->type & REF_ISSYMREF) {
+ if (!(update->flags & REF_NODEREF)) {
+ ret = split_symref_update(update, referent.buf, transaction,
+  affected_refnames, err);
+ if (ret)
+ return ret;
+ }
+ }
+
  if ((update->flags & REF_HAVE_NEW) &&
     !(update->flags & REF_DELETING) &&
     !(update->flags & REF_LOG_ONLY)) {
- int overwriting_symref = ((update->type & REF_ISSYMREF) &&
-  (update->flags & REF_NODEREF));
-
- if (!overwriting_symref &&
-    !hashcmp(update->lock->old_oid.hash, update->new_sha1)) {
+ if (!(update->type & REF_ISSYMREF) &&
+    !hashcmp(lock->old_oid.hash, update->new_sha1)) {
  /*
  * The reference already has the desired
  * value, so we don't need to write it.
  */
- } else if (write_ref_to_lockfile(update->lock,
- update->new_sha1,
+ } else if (write_ref_to_lockfile(lock, update->new_sha1,
  err)) {
  char *write_err = strbuf_detach(err, NULL);
 
@@ -3124,7 +3492,7 @@ static int lock_ref_for_update(struct ref_update *update,
  * the lockfile is still open. Close it to
  * free up the file descriptor:
  */
- if (close_ref(update->lock)) {
+ if (close_ref(lock)) {
  strbuf_addf(err, "couldn't close '%s.lock'",
     update->refname);
  return TRANSACTION_GENERIC_ERROR;
@@ -3141,6 +3509,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
  struct string_list_item *ref_to_delete;
  struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
+ char *head_ref = NULL;
+ int head_type;
+ struct object_id head_oid;
 
  assert(err);
 
@@ -3152,9 +3523,25 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  return 0;
  }
 
- /* Fail if a refname appears more than once in the transaction: */
- for (i = 0; i < transaction->nr; i++)
- string_list_append(&affected_refnames, updates[i]->refname);
+ /*
+ * Fail if a refname appears more than once in the
+ * transaction. (If we end up splitting up any updates using
+ * split_symref_update() or split_head_update(), those
+ * functions will check that the new updates don't have the
+ * same refname as any existing ones.)
+ */
+ for (i = 0; i < transaction->nr; i++) {
+ struct ref_update *update = updates[i];
+ struct string_list_item *item =
+ string_list_append(&affected_refnames, update->refname);
+
+ /*
+ * We store a pointer to update in item->util, but at
+ * the moment we never use the value of this field
+ * except to check whether it is non-NULL.
+ */
+ item->util = update;
+ }
  string_list_sort(&affected_refnames);
  if (ref_update_reject_duplicates(&affected_refnames, err)) {
  ret = TRANSACTION_GENERIC_ERROR;
@@ -3162,6 +3549,32 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  }
 
  /*
+ * Special hack: If a branch is updated directly and HEAD
+ * points to it (may happen on the remote side of a push
+ * for example) then logically the HEAD reflog should be
+ * updated too.
+ *
+ * A generic solution would require reverse symref lookups,
+ * but finding all symrefs pointing to a given branch would be
+ * rather costly for this rare event (the direct update of a
+ * branch) to be worth it. So let's cheat and check with HEAD
+ * only, which should cover 99% of all usage scenarios (even
+ * 100% of the default ones).
+ *
+ * So if HEAD is a symbolic reference, then record the name of
+ * the reference that it points to. If we see an update of
+ * head_ref within the transaction, then split_head_update()
+ * arranges for the reflog of HEAD to be updated, too.
+ */
+ head_ref = resolve_refdup("HEAD", RESOLVE_REF_NO_RECURSE,
+  head_oid.hash, &head_type);
+
+ if (head_ref && !(head_type & REF_ISSYMREF)) {
+ free(head_ref);
+ head_ref = NULL;
+ }
+
+ /*
  * Acquire all locks, verify old values if provided, check
  * that new values are valid, and write new values to the
  * lockfiles, ready to be activated. Only keep one lockfile
@@ -3170,7 +3583,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  for (i = 0; i < transaction->nr; i++) {
  struct ref_update *update = updates[i];
 
- ret = lock_ref_for_update(update, transaction,
+ ret = lock_ref_for_update(update, transaction, head_ref,
   &affected_refnames, err);
  if (ret)
  goto cleanup;
@@ -3179,23 +3592,35 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  /* Perform updates first so live commits remain referenced */
  for (i = 0; i < transaction->nr; i++) {
  struct ref_update *update = updates[i];
+ struct ref_lock *lock = update->lock;
 
  if (update->flags & REF_NEEDS_COMMIT ||
     update->flags & REF_LOG_ONLY) {
- if (commit_ref_update(update->lock,
-      update->new_sha1, update->msg,
-      update->flags, err)) {
- /* freed by commit_ref_update(): */
- update->lock = NULL;
- ret = TRANSACTION_GENERIC_ERROR;
- goto cleanup;
- } else {
- /* freed by commit_ref_update(): */
- update->lock = NULL;
- }
- }
- }
+ if (log_ref_write(lock->ref_name, lock->old_oid.hash,
+  update->new_sha1,
+  update->msg, update->flags, err)) {
+ char *old_msg = strbuf_detach(err, NULL);
 
+ strbuf_addf(err, "cannot update the ref '%s': %s",
+    lock->ref_name, old_msg);
+ free(old_msg);
+ unlock_ref(lock);
+ update->lock = NULL;
+ ret = TRANSACTION_GENERIC_ERROR;
+ goto cleanup;
+ }
+ }
+ if (update->flags & REF_NEEDS_COMMIT) {
+ clear_loose_ref_cache(&ref_cache);
+ if (commit_ref(lock)) {
+ strbuf_addf(err, "couldn't set '%s'", lock->ref_name);
+ unlock_ref(lock);
+ update->lock = NULL;
+ ret = TRANSACTION_GENERIC_ERROR;
+ goto cleanup;
+ }
+ }
+ }
  /* Perform deletes now that updates are safely completed */
  for (i = 0; i < transaction->nr; i++) {
  struct ref_update *update = updates[i];
@@ -3228,7 +3653,9 @@ cleanup:
  if (updates[i]->lock)
  unlock_ref(updates[i]->lock);
  string_list_clear(&refs_to_delete, 0);
+ free(head_ref);
  string_list_clear(&affected_refnames, 0);
+
  return ret;
 }
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index babdf27..cccd76b 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -50,6 +50,12 @@
 #define REF_LOG_ONLY 0x80
 
 /*
+ * Internal flag, meaning that the containing ref_update was via an
+ * update to HEAD.
+ */
+#define REF_UPDATE_VIA_HEAD 0x100
+
+/*
  * Return true iff refname is minimally safe. "Safe" here means that
  * deleting a loose reference by this name will not do any damage, for
  * example by causing a file that is not a reference to be deleted.
@@ -148,11 +154,12 @@ struct ref_update {
  unsigned char old_sha1[20];
  /*
  * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,
- * REF_DELETING, and REF_ISPRUNING:
+ * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, and
+ * REF_UPDATE_VIA_HEAD:
  */
  unsigned int flags;
  struct ref_lock *lock;
- int type;
+ unsigned int type;
  char *msg;
  const char refname[FLEX_ARRAY];
 };
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 08bd8fd..d226930 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1102,6 +1102,41 @@ test_expect_success 'stdin -z delete refs works with packed and loose refs' '
  test_must_fail git rev-parse --verify -q $c
 '
 
+test_expect_success 'fails with duplicate HEAD update' '
+ git branch target1 $A &&
+ git checkout target1 &&
+ cat >stdin <<-EOF &&
+ update refs/heads/target1 $C
+ option no-deref
+ update HEAD $B
+ EOF
+ test_must_fail git update-ref --stdin <stdin 2>err &&
+ grep "fatal: multiple updates for '\''HEAD'\'' (including one via its referent .refs/heads/target1.) are not allowed" err &&
+ echo "refs/heads/target1" >expect &&
+ git symbolic-ref HEAD >actual &&
+ test_cmp expect actual &&
+ echo "$A" >expect &&
+ git rev-parse refs/heads/target1 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'fails with duplicate ref update via symref' '
+ git branch target2 $A &&
+ git symbolic-ref refs/heads/symref2 refs/heads/target2 &&
+ cat >stdin <<-EOF &&
+ update refs/heads/target2 $C
+ update refs/heads/symref2 $B
+ EOF
+ test_must_fail git update-ref --stdin <stdin 2>err &&
+ grep "fatal: multiple updates for '\''refs/heads/target2'\'' (including one via symref .refs/heads/symref2.) are not allowed" err &&
+ echo "refs/heads/target2" >expect &&
+ git symbolic-ref refs/heads/symref2 >actual &&
+ test_cmp expect actual &&
+ echo "$A" >expect &&
+ git rev-parse refs/heads/target2 >actual &&
+ test_cmp expect actual
+'
+
 run_with_limited_open_files () {
  (ulimit -n 32 && "$@")
 }
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 30/33] lock_ref_for_update(): don't re-read non-symbolic references

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
Before the previous patch, our first read of the reference happened
before the reference was locked, so we couldn't trust its value and had
to read it again. But now that our first read of the reference happens
after acquiring the lock, there is no need to read it a second time. So
move the read_ref_full() call into the (update->type & REF_ISSYMREF)
block.

Signed-off-by: Michael Haggerty <[hidden email]>
---
 refs/files-backend.c | 52 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 20 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 799866f..a9066ba 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3430,33 +3430,45 @@ static int lock_ref_for_update(struct ref_update *update,
 
  lock = update->lock;
 
- if (read_ref_full(update->refname,
-  mustexist ? RESOLVE_REF_READING : 0,
-  lock->old_oid.hash, NULL)) {
- if (update->flags & REF_HAVE_OLD) {
- strbuf_addf(err, "cannot lock ref '%s': can't resolve old value",
-    update->refname);
- return TRANSACTION_GENERIC_ERROR;
- } else {
- hashclr(lock->old_oid.hash);
- }
- }
- if ((update->flags & REF_HAVE_OLD) &&
-    hashcmp(lock->old_oid.hash, update->old_sha1)) {
- strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
-    update->refname,
-    sha1_to_hex(lock->old_oid.hash),
-    sha1_to_hex(update->old_sha1));
- return TRANSACTION_GENERIC_ERROR;
- }
-
  if (update->type & REF_ISSYMREF) {
+ if (read_ref_full(update->refname,
+  mustexist ? RESOLVE_REF_READING : 0,
+  lock->old_oid.hash, NULL)) {
+ if (update->flags & REF_HAVE_OLD) {
+ strbuf_addf(err, "cannot lock ref '%s': can't resolve old value",
+    update->refname);
+ return TRANSACTION_GENERIC_ERROR;
+ } else {
+ hashclr(lock->old_oid.hash);
+ }
+ }
+ if ((update->flags & REF_HAVE_OLD) &&
+    hashcmp(lock->old_oid.hash, update->old_sha1)) {
+ strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
+    update->refname,
+    sha1_to_hex(lock->old_oid.hash),
+    sha1_to_hex(update->old_sha1));
+ return TRANSACTION_GENERIC_ERROR;
+ }
+
  if (!(update->flags & REF_NODEREF)) {
  ret = split_symref_update(update, referent.buf, transaction,
   affected_refnames, err);
  if (ret)
  return ret;
  }
+ } else if ((update->flags & REF_HAVE_OLD) &&
+   hashcmp(lock->old_oid.hash, update->old_sha1)) {
+ if (is_null_sha1(update->old_sha1))
+ strbuf_addf(err, "cannot lock ref '%s': reference already exists",
+    update->refname);
+ else
+ strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
+    update->refname,
+    sha1_to_hex(lock->old_oid.hash),
+    sha1_to_hex(update->old_sha1));
+
+ return TRANSACTION_GENERIC_ERROR;
  }
 
  if ((update->flags & REF_HAVE_NEW) &&
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 31/33] lock_ref_for_update(): don't resolve symrefs

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
If a transaction includes a non-NODEREF update to a symbolic reference,
we don't have to look it up in lock_ref_for_update(). The reference will
be dereferenced anyway when the split-off update is processed.

This change requires that we store a backpointer from the split-off
update to its parent update, for two reasons:

* We still want to report the original reference name in error messages.
  So if an error occurs when checking the split-off update's old_sha1,
  walk the parent_update pointers back to find the original reference
  name, and report that one.

* We still need to write the old_sha1 of the symref to its reflog. So
  after we read the split-off update's reference value, walk the
  parent_update pointers back and fill in their old_sha1 fields.

Aside from eliminating unnecessary reads, this change fixes a
subtle (though not very serious) race condition: in the old code, the
old_sha1 of the symref was resolved before the reference that it pointed
at was locked. So it was possible that the old_sha1 value logged to the
symref's reflog could be wrong if another process changed the downstream
reference before it was locked.

Signed-off-by: Michael Haggerty <[hidden email]>
---
 refs/files-backend.c | 108 +++++++++++++++++++++++++++++++++++++--------------
 refs/refs-internal.h |  17 ++++++++
 2 files changed, 95 insertions(+), 30 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index a9066ba..08ec293 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3371,8 +3371,15 @@ static int split_symref_update(struct ref_update *update,
  update->new_sha1, update->old_sha1,
  update->msg);
 
- /* Change the symbolic ref update to log only: */
+ new_update->parent_update = update;
+
+ /*
+ * Change the symbolic ref update to log only. Also, it
+ * doesn't need to check its old SHA-1 value, as that will be
+ * done when new_update is processed.
+ */
  update->flags |= REF_LOG_ONLY | REF_NODEREF;
+ update->flags &= ~REF_HAVE_OLD;
 
  item->util = new_update;
 
@@ -3380,6 +3387,17 @@ static int split_symref_update(struct ref_update *update,
 }
 
 /*
+ * Return the refname under which update was originally requested.
+ */
+static const char *original_update_refname(struct ref_update *update)
+{
+ while (update->parent_update)
+ update = update->parent_update;
+
+ return update->refname;
+}
+
+/*
  * Prepare for carrying out update:
  * - Lock the reference referred to by update.
  * - Read the reference under lock.
@@ -3431,44 +3449,74 @@ static int lock_ref_for_update(struct ref_update *update,
  lock = update->lock;
 
  if (update->type & REF_ISSYMREF) {
- if (read_ref_full(update->refname,
-  mustexist ? RESOLVE_REF_READING : 0,
-  lock->old_oid.hash, NULL)) {
- if (update->flags & REF_HAVE_OLD) {
- strbuf_addf(err, "cannot lock ref '%s': can't resolve old value",
-    update->refname);
+ if (update->flags & REF_NODEREF) {
+ /*
+ * We won't be reading the referent as part of
+ * the transaction, so we have to read it here
+ * to record and possibly check old_sha1:
+ */
+ if (read_ref_full(update->refname,
+  mustexist ? RESOLVE_REF_READING : 0,
+  lock->old_oid.hash, NULL)) {
+ if (update->flags & REF_HAVE_OLD) {
+ strbuf_addf(err, "cannot lock ref '%s': "
+    "can't resolve old value",
+    update->refname);
+ return TRANSACTION_GENERIC_ERROR;
+ } else {
+ hashclr(lock->old_oid.hash);
+ }
+ }
+ if ((update->flags & REF_HAVE_OLD) &&
+    hashcmp(lock->old_oid.hash, update->old_sha1)) {
+ strbuf_addf(err, "cannot lock ref '%s': "
+    "is at %s but expected %s",
+    update->refname,
+    sha1_to_hex(lock->old_oid.hash),
+    sha1_to_hex(update->old_sha1));
  return TRANSACTION_GENERIC_ERROR;
- } else {
- hashclr(lock->old_oid.hash);
  }
- }
- if ((update->flags & REF_HAVE_OLD) &&
-    hashcmp(lock->old_oid.hash, update->old_sha1)) {
- strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
-    update->refname,
-    sha1_to_hex(lock->old_oid.hash),
-    sha1_to_hex(update->old_sha1));
- return TRANSACTION_GENERIC_ERROR;
- }
 
- if (!(update->flags & REF_NODEREF)) {
+ } else {
+ /*
+ * Create a new update for the reference this
+ * symref is pointing at. Also, we will record
+ * and verify old_sha1 for this update as part
+ * of processing the split-off update, so we
+ * don't have to do it here.
+ */
  ret = split_symref_update(update, referent.buf, transaction,
   affected_refnames, err);
  if (ret)
  return ret;
  }
- } else if ((update->flags & REF_HAVE_OLD) &&
-   hashcmp(lock->old_oid.hash, update->old_sha1)) {
- if (is_null_sha1(update->old_sha1))
- strbuf_addf(err, "cannot lock ref '%s': reference already exists",
-    update->refname);
- else
- strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
-    update->refname,
-    sha1_to_hex(lock->old_oid.hash),
-    sha1_to_hex(update->old_sha1));
+ } else {
+ struct ref_update *parent_update;
 
- return TRANSACTION_GENERIC_ERROR;
+ /*
+ * If this update is happening indirectly because of a
+ * symref update, record the old SHA-1 in the parent
+ * update:
+ */
+ for (parent_update = update->parent_update;
+     parent_update;
+     parent_update = parent_update->parent_update) {
+ oidcpy(&parent_update->lock->old_oid, &lock->old_oid);
+ }
+
+ if ((update->flags & REF_HAVE_OLD) &&
+    hashcmp(lock->old_oid.hash, update->old_sha1)) {
+ if (is_null_sha1(update->old_sha1))
+ strbuf_addf(err, "cannot lock ref '%s': reference already exists",
+    original_update_refname(update));
+ else
+ strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
+    original_update_refname(update),
+    sha1_to_hex(lock->old_oid.hash),
+    sha1_to_hex(update->old_sha1));
+
+ return TRANSACTION_GENERIC_ERROR;
+ }
  }
 
  if ((update->flags & REF_HAVE_NEW) &&
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index cccd76b..1bb3d87 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -143,24 +143,41 @@ int should_autocreate_reflog(const char *refname);
  * not exist before update.
  */
 struct ref_update {
+
  /*
  * If (flags & REF_HAVE_NEW), set the reference to this value:
  */
  unsigned char new_sha1[20];
+
  /*
  * If (flags & REF_HAVE_OLD), check that the reference
  * previously had this value:
  */
  unsigned char old_sha1[20];
+
  /*
  * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,
  * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, and
  * REF_UPDATE_VIA_HEAD:
  */
  unsigned int flags;
+
  struct ref_lock *lock;
  unsigned int type;
  char *msg;
+
+ /*
+ * If this ref_update was split off of a symref update via
+ * split_symref_update(), then this member points at that
+ * update. This is used for two purposes:
+ * 1. When reporting errors, we report the refname under which
+ *    the update was originally requested.
+ * 2. When we read the old value of this reference, we
+ *    propagate it back to its parent update for recording in
+ *    the latter's reflog.
+ */
+ struct ref_update *parent_update;
+
  const char refname[FLEX_ARRAY];
 };
 
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 32/33] commit_ref_update(): remove the flags parameter

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
commit_ref_update() is now only called with flags=0. So remove the flags
parameter entirely.

Signed-off-by: Michael Haggerty <[hidden email]>
---
 refs/files-backend.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 08ec293..a180b9e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2541,7 +2541,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
  const unsigned char *sha1, struct strbuf *err);
 static int commit_ref_update(struct ref_lock *lock,
      const unsigned char *sha1, const char *logmsg,
-     int flags, struct strbuf *err);
+     struct strbuf *err);
 
 int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)
 {
@@ -2617,7 +2617,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
  hashcpy(lock->old_oid.hash, orig_sha1);
 
  if (write_ref_to_lockfile(lock, orig_sha1, &err) ||
-    commit_ref_update(lock, orig_sha1, logmsg, 0, &err)) {
+    commit_ref_update(lock, orig_sha1, logmsg, &err)) {
  error("unable to write current sha1 into %s: %s", newrefname, err.buf);
  strbuf_release(&err);
  goto rollback;
@@ -2637,7 +2637,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
  flag = log_all_ref_updates;
  log_all_ref_updates = 0;
  if (write_ref_to_lockfile(lock, orig_sha1, &err) ||
-    commit_ref_update(lock, orig_sha1, NULL, 0, &err)) {
+    commit_ref_update(lock, orig_sha1, NULL, &err)) {
  error("unable to write current sha1 into %s: %s", oldrefname, err.buf);
  strbuf_release(&err);
  }
@@ -2875,12 +2875,12 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
  */
 static int commit_ref_update(struct ref_lock *lock,
      const unsigned char *sha1, const char *logmsg,
-     int flags, struct strbuf *err)
+     struct strbuf *err)
 {
  clear_loose_ref_cache(&ref_cache);
- if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, flags, err) < 0 ||
+ if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err) < 0 ||
     (strcmp(lock->ref_name, lock->orig_ref_name) &&
-     log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, flags, err) < 0)) {
+     log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, 0, err) < 0)) {
  char *old_msg = strbuf_detach(err, NULL);
  strbuf_addf(err, "cannot update the ref '%s': %s",
     lock->ref_name, old_msg);
@@ -2916,7 +2916,7 @@ static int commit_ref_update(struct ref_lock *lock,
  }
  }
  }
- if (!(flags & REF_LOG_ONLY) && commit_ref(lock)) {
+ if (commit_ref(lock)) {
  strbuf_addf(err, "couldn't set '%s'", lock->ref_name);
  unlock_ref(lock);
  return -1;
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 33/33] lock_ref_sha1_basic(): only handle REF_NODEREF mode

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
Now lock_ref_sha1_basic() is only called with flags==REF_NODEREF. So we
don't have to handle other cases anymore.

This enables several simplifications, the most interesting of which come
from the fact that ref_lock::orig_ref_name is now always the same as
ref_lock::ref_name:

* Remove ref_lock::orig_ref_name
* Remove local variable orig_refname from lock_ref_sha1_basic()
* ref_name can be initialize once and its value reused
* commit_ref_update() never has to write to the reflog for
  lock->orig_ref_name

Signed-off-by: Michael Haggerty <[hidden email]>
---
 refs/files-backend.c | 54 +++++++++++++++++++---------------------------------
 1 file changed, 20 insertions(+), 34 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index a180b9e..71e068b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -7,7 +7,6 @@
 
 struct ref_lock {
  char *ref_name;
- char *orig_ref_name;
  struct lock_file *lk;
  struct object_id old_oid;
 };
@@ -1522,7 +1521,6 @@ static void unlock_ref(struct ref_lock *lock)
  if (lock->lk)
  rollback_lock_file(lock->lk);
  free(lock->ref_name);
- free(lock->orig_ref_name);
  free(lock);
 }
 
@@ -1576,7 +1574,6 @@ static int lock_raw_ref(const char *refname, int mustexist,
  *lock_p = lock = xcalloc(1, sizeof(*lock));
 
  lock->ref_name = xstrdup(refname);
- lock->orig_ref_name = xstrdup(refname);
  strbuf_git_path(&ref_file, "%s", refname);
 
 retry:
@@ -1964,14 +1961,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
     struct strbuf *err)
 {
  struct strbuf ref_file = STRBUF_INIT;
- struct strbuf orig_ref_file = STRBUF_INIT;
- const char *orig_refname = refname;
  struct ref_lock *lock;
  int last_errno = 0;
- int lflags = 0;
+ int lflags = LOCK_NO_DEREF;
  int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
- int resolve_flags = 0;
+ int resolve_flags = RESOLVE_REF_NO_RECURSE;
  int attempts_remaining = 3;
+ int resolved;
 
  assert(err);
 
@@ -1981,46 +1977,39 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
  resolve_flags |= RESOLVE_REF_READING;
  if (flags & REF_DELETING)
  resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
- if (flags & REF_NODEREF) {
- resolve_flags |= RESOLVE_REF_NO_RECURSE;
- lflags |= LOCK_NO_DEREF;
- }
 
- refname = resolve_ref_unsafe(refname, resolve_flags,
-     lock->old_oid.hash, type);
- if (!refname && errno == EISDIR) {
+ strbuf_git_path(&ref_file, "%s", refname);
+ resolved = !!resolve_ref_unsafe(refname, resolve_flags,
+ lock->old_oid.hash, type);
+ if (!resolved && errno == EISDIR) {
  /*
  * we are trying to lock foo but we used to
  * have foo/bar which now does not exist;
  * it is normal for the empty directory 'foo'
  * to remain.
  */
- strbuf_git_path(&orig_ref_file, "%s", orig_refname);
- if (remove_empty_directories(&orig_ref_file)) {
+ if (remove_empty_directories(&ref_file)) {
  last_errno = errno;
- if (!verify_refname_available_dir(orig_refname, extras, skip,
+ if (!verify_refname_available_dir(refname, extras, skip,
   get_loose_refs(&ref_cache), err))
  strbuf_addf(err, "there are still refs under '%s'",
-    orig_refname);
+    refname);
  goto error_return;
  }
- refname = resolve_ref_unsafe(orig_refname, resolve_flags,
-     lock->old_oid.hash, type);
+ resolved = !!resolve_ref_unsafe(refname, resolve_flags,
+ lock->old_oid.hash, type);
  }
- if (!refname) {
+ if (!resolved) {
  last_errno = errno;
  if (last_errno != ENOTDIR ||
-    !verify_refname_available_dir(orig_refname, extras, skip,
+    !verify_refname_available_dir(refname, extras, skip,
   get_loose_refs(&ref_cache), err))
  strbuf_addf(err, "unable to resolve reference '%s': %s",
-    orig_refname, strerror(last_errno));
+    refname, strerror(last_errno));
 
  goto error_return;
  }
 
- if (flags & REF_NODEREF)
- refname = orig_refname;
-
  /*
  * If the ref did not exist and we are creating it, make sure
  * there is no existing packed ref whose name begins with our
@@ -2037,8 +2026,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
  lock->lk = xcalloc(1, sizeof(struct lock_file));
 
  lock->ref_name = xstrdup(refname);
- lock->orig_ref_name = xstrdup(orig_refname);
- strbuf_git_path(&ref_file, "%s", refname);
 
  retry:
  switch (safe_create_leading_directories_const(ref_file.buf)) {
@@ -2081,7 +2068,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 
  out:
  strbuf_release(&ref_file);
- strbuf_release(&orig_ref_file);
  errno = last_errno;
  return lock;
 }
@@ -2878,9 +2864,7 @@ static int commit_ref_update(struct ref_lock *lock,
      struct strbuf *err)
 {
  clear_loose_ref_cache(&ref_cache);
- if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err) < 0 ||
-    (strcmp(lock->ref_name, lock->orig_ref_name) &&
-     log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, 0, err) < 0)) {
+ if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err)) {
  char *old_msg = strbuf_detach(err, NULL);
  strbuf_addf(err, "cannot update the ref '%s': %s",
     lock->ref_name, old_msg);
@@ -2888,7 +2872,8 @@ static int commit_ref_update(struct ref_lock *lock,
  unlock_ref(lock);
  return -1;
  }
- if (strcmp(lock->orig_ref_name, "HEAD") != 0) {
+
+ if (strcmp(lock->ref_name, "HEAD") != 0) {
  /*
  * Special hack: If a branch is updated directly and HEAD
  * points to it (may happen on the remote side of a push
@@ -2904,6 +2889,7 @@ static int commit_ref_update(struct ref_lock *lock,
  unsigned char head_sha1[20];
  int head_flag;
  const char *head_ref;
+
  head_ref = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
       head_sha1, &head_flag);
  if (head_ref && (head_flag & REF_ISSYMREF) &&
@@ -2916,6 +2902,7 @@ static int commit_ref_update(struct ref_lock *lock,
  }
  }
  }
+
  if (commit_ref(lock)) {
  strbuf_addf(err, "couldn't set '%s'", lock->ref_name);
  unlock_ref(lock);
@@ -3021,7 +3008,6 @@ int set_worktree_head_symref(const char *gitdir, const char *target)
  lock = xcalloc(1, sizeof(struct ref_lock));
  lock->lk = &head_lock;
  lock->ref_name = xstrdup(head_rel);
- lock->orig_ref_name = xstrdup(head_rel);
 
  ret = create_symref_locked(lock, head_rel, target, NULL);
 
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 00/33] Yet more preparation for reference backends

David Turner
In reply to this post by Michael Haggerty-2
On Fri, 2016-05-06 at 18:13 +0200, Michael Haggerty wrote:

> Thanks to David, Junio, and Peff for their comments on v1 of this
> patch series [1]. I think I have addressed all of the points that
> were
> brought up. Plus I fixed a pre-existing bug that I noticed myself
> while adding some more tests; see the first bullet point below for
> more information.
>
> Changes between v1 and v2:
>
> * Prefixed the patch series with three new patches that demonstrate
>   and fix some bugs in reference resolution that I noticed while
>   inspecting this series:
>
>   * "t1404: demonstrate a bug resolving references" -- this
>     demonstrates a bug that has existed since at least 2.5.0.
>   * "read_raw_ref(): don't get confused by an empty directory"
>   * "commit_ref(): if there is an empty dir in the way, delete it"

I generally like to put the bug fixes before the tests for those fixes
(so that bisect on the complete suite works).  But maybe the git policy
is different.

Other than that, these changes look good to me.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 00/33] Yet more preparation for reference backends

Junio C Hamano
David Turner <[hidden email]> writes:

> On Fri, 2016-05-06 at 18:13 +0200, Michael Haggerty wrote:
>> Thanks to David, Junio, and Peff for their comments on v1 of this
>> patch series [1]. I think I have addressed all of the points that
>> were
>> brought up. Plus I fixed a pre-existing bug that I noticed myself
>> while adding some more tests; see the first bullet point below for
>> more information.
>>
>> Changes between v1 and v2:
>>
>> * Prefixed the patch series with three new patches that demonstrate
>>   and fix some bugs in reference resolution that I noticed while
>>   inspecting this series:
>>
>>   * "t1404: demonstrate a bug resolving references" -- this
>>     demonstrates a bug that has existed since at least 2.5.0.
>>   * "read_raw_ref(): don't get confused by an empty directory"
>>   * "commit_ref(): if there is an empty dir in the way, delete it"
>
> I generally like to put the bug fixes before the tests for those fixes
> (so that bisect on the complete suite works).  But maybe the git policy
> is different.

The Git policy only asks not to break bisection.

As long as patch that adds a new test that comes before a patch that
fixes the issue marks the new test with test_expect_failure, and a
later patch that fixes the issue turns it into test_expect_success,
bisection would not break.

The "demonstrate an existing breakage first" order makes it slightly
easier to review and follow a long series, as it forces the reviewer
to see the issue first and think about possible avenues to solve it
for themselves, before seeing a paticular solution.  For a trivial
single-issue fix, it is not necessary (including a fix and a test to
protect the fix from future breakage in the same patch is a norm).

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 00/33] Yet more preparation for reference backends

Michael Haggerty-2
On 05/09/2016 11:05 PM, Junio C Hamano wrote:

> David Turner <[hidden email]> writes:
>> [...]
>> I generally like to put the bug fixes before the tests for those fixes
>> (so that bisect on the complete suite works).  But maybe the git policy
>> is different.
>
> The Git policy only asks not to break bisection.
>
> As long as patch that adds a new test that comes before a patch that
> fixes the issue marks the new test with test_expect_failure, and a
> later patch that fixes the issue turns it into test_expect_success,
> bisection would not break.
>
> The "demonstrate an existing breakage first" order makes it slightly
> easier to review and follow a long series, as it forces the reviewer
> to see the issue first and think about possible avenues to solve it
> for themselves, before seeing a paticular solution.  For a trivial
> single-issue fix, it is not necessary (including a fix and a test to
> protect the fix from future breakage in the same patch is a norm).

I find it useful to add the broken test in a separate patch, because it
is then easy to cherry pick that patch to other versions of Git to
discover which ones are also affected by the problem. If the addition of
the test is combined with the fix, then the patch would more often fail
to apply to other versions due to conflicts at the locations of the fix,
and even if it applied, you wouldn't learn whether that version of git
was broken but the breakage was fixed by the same fix, or whether it
wasn't broken in the first place and the fix was unnecessary.

Michael

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 00/33] Yet more preparation for reference backends

Junio C Hamano
Michael Haggerty <[hidden email]> writes:

>> The "demonstrate an existing breakage first" order makes it slightly
>> easier to review and follow a long series, as it forces the reviewer
>> to see the issue first and think about possible avenues to solve it
>> for themselves, before seeing a paticular solution.  For a trivial
>> single-issue fix, it is not necessary (including a fix and a test to
>> protect the fix from future breakage in the same patch is a norm).
>
> I find it useful to add the broken test in a separate patch, because it
> is then easy to cherry pick that patch to other versions of Git to
> discover which ones are also affected by the problem. If the addition of
> the test is combined with the fix, then the patch would more often fail
> to apply to other versions due to conflicts at the locations of the fix,
> and even if it applied, you wouldn't learn whether that version of git
> was broken but the breakage was fixed by the same fix, or whether it
> wasn't broken in the first place and the fix was unnecessary.

Note that I only said "it is not necessary", and did not say "it is
not desirable".  I wouldn't automatically reject a two patch series
with demonstration followed by fix, only because they are not in a
single patch.

Even when I know the maintenance track a particular fix and test
targets at, I'd do the "only try to test to see how it is broken
currently" step manually anyway as part of the initial "acceptance"
check when applying [*1*], so trying the same procedure for older
maintenance tracks is no big burden for me.  Having these as two
separate patches is easier to split them apart, which unfortunately
makes it easier to lose one of them while cherry-picking.

This is of course subjective.


[Footnote]

*1* There is a bit of white-lie here.  Instead of applying only t/
    part, I tend to just do "git am" the whole thing, and then pipe
    "git show" to "git apply -R" to in-work-tree revert only the
    code that fixes it.  But the result I get is the same.

    And the "cherry-picking" would also involve "show only the t/
    part and pipe that to "git apply", which is even simpler than
    actually cherry-picking and creating a commit (I do not have to
    be very careful not to leave such a "test cherry-pick" commit in
    the real history).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 00/33] Yet more preparation for reference backends

Junio C Hamano
In reply to this post by Michael Haggerty-2
Michael Haggerty <[hidden email]> writes:

> ... I think I have addressed all of the points that were
> brought up. Plus I fixed a pre-existing bug that I noticed myself
> while adding some more tests; see the first bullet point below for
> more information.
>
> Changes between v1 and v2:
>
> * Prefixed the patch series with three new patches that demonstrate
>   and fix some bugs in reference resolution that I noticed while
>   inspecting this series:

I'd propose to wait for further comments a few more days and merge
this to 'next' by the end of the week.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 29/33] refs: resolve symbolic refs first

Jeff King
In reply to this post by Michael Haggerty-2
On Fri, May 06, 2016 at 06:14:10PM +0200, Michael Haggerty wrote:

> This makes use of a new function, lock_ref_raw(), which is analogous to
> read_ref_raw(), but acquires a lock on the reference before reading it.

Minor nit: the new function is actually called lock_raw_ref(). I don't
care which is used, just an inconsistency.

But my much bigger (non-)nit is that this seems to make large ref
updates much slower. You can see this by running t5551 with "--long".
In t5551.26, we fetch 48000 new tags into a repository that already has
2000 tags. Before this patch, it takes about 2 seconds. After, it chews
CPU for several minutes (I never actually let it finish).

The perf output isn't all that instructive. We seem to spend a lot of
time reading directory entries. Attaching with gdb shows:

#0  0x00007f35e00c2670 in __open_nocancel () at ../sysdeps/unix/syscall-template.S:84
#1  0x0000000000533982 in read_raw_ref (
    refname=0x4e899f0 "refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-11520", sha1=0x7fff7c5aff30 "\002\357\373\070\332{\341\005\366츖\265G\276\332\f\025\271\276\377\177",
    referent=0x836300 <sb_refname>, type=0x7fff7c5afe34) at refs/files-backend.c:1468
#2  0x0000000000530bf3 in resolve_ref_unsafe (
    refname=0x4e899f0 "refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-11520", resolve_flags=1,
    sha1=0x7fff7c5aff30 "\002\357\373\070\332{\341\005\366츖\265G\276\332\f\025\271\276\377\177",
    flags=0x7fff7c5aff2c) at refs.c:1209
#3  0x000000000052e56f in read_ref_full (
    refname=0x4e899f0 "refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-11520", resolve_flags=1,
    sha1=0x7fff7c5aff30 "\002\357\373\070\332{\341\005\366츖\265G\276\332\f\025\271\276\377\177",
    flags=0x7fff7c5aff2c) at refs.c:169
#4  0x000000000053316e in read_loose_refs (dirname=0x4e30f80 "refs/tags/", dir=0x4e30f58) at refs/files-backend.c:1216
#5  0x0000000000531435 in get_ref_dir (entry=0x4e30f50) at refs/files-backend.c:174
#6  0x000000000053265c in verify_refname_available_dir (
    refname=0x1cd9438 "refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-12016", extras=0x7fff7c5b01d0, skip=0x0, dir=0x4dd6e98, err=0x7fff7c5b02a0) at refs/files-backend.c:789
#7  0x0000000000533e44 in lock_raw_ref (
    refname=0x1cd9438 "refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-12016", mustexist=0, extras=0x7fff7c5b01d0, skip=0x0, lock_p=0x1cd9420, referent=0x7fff7c5b0140, type=0x1cd9428,
    err=0x7fff7c5b02a0) at refs/files-backend.c:1663
#8  0x00000000005379d7 in lock_ref_for_update (update=0x1cd93f0, transaction=0x4db0150,
    head_ref=0x4db0000 "refs/heads/master", affected_refnames=0x7fff7c5b01d0, err=0x7fff7c5b02a0)
    at refs/files-backend.c:3416
[...]

So I'd expect us to hit that lock_ref_for_update() for each of the new
refs. But then we end up in verify_refname_available_dir(), which wants
to read all of the loose refs again. So we end up with a quadratic
number of calls to read_ref_full().

I haven't found the actual bug yet. It may be something as simple as not
clearing REF_INCOMPLETE from the loose-ref cache when we ought to. But
that's a wild (optimistic) guess.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
123