[PATCH 00/29] Yet more preparation for reference backends

classic Classic list List threaded Threaded
70 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

[PATCH 20/29] verify_refname_available(): adjust constness in declaration

Michael Haggerty-2
The two string_list arguments can be const.

Signed-off-by: Michael Haggerty <[hidden email]>
---
This is needed because I want to add a new caller that has const
versions of these lists in hand.

 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 e4cdd5a..e0d9fa3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2328,8 +2328,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 6b53ba1..aaf56ea 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 21/29] 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]>
---
In particular, it's nice that it returns the "struct ref_update *"
that it creates, which is useful for refs-internal code that might
want to tweak other field in the ref_update. But we don't want
non-refs code to get its hands on a ref_update.

 refs.c               | 33 +++++++++++++++++----------------
 refs/refs-internal.h | 14 ++++++++++++++
 2 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/refs.c b/refs.c
index 5dc2473..858b6d7 100644
--- a/refs.c
+++ b/refs.c
@@ -766,13 +766,24 @@ 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;
  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,8 +794,6 @@ 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)
@@ -800,18 +809,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 aaf56ea..c46fe67 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 22/29] 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 e0d9fa3..546656a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3048,6 +3048,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)
 {
@@ -3085,74 +3167,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 23/29] 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 546656a..8f2a795 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1545,6 +1545,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
@@ -1703,16 +1713,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 24/29] 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]>
---
There are remaining problems in this area of the code. For example,
check_refname_format() is *less* strict than refname_is_safe(). It
allows almost arbitrary top-level reference names like "foo" and
"refs". (The latter is especially fun because if you manage to create
a reference called "refs", Git stops recognizing the directory as a
Git repository.) On the other hand, some callers call
check_refname_format() with incomplete reference names (e.g., branch
names like "master"), so the functions can't be made stricter until
those callers are changed.

I'll address these problems separately if I find the time.

 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 858b6d7..41eb9e2 100644
--- a/refs.c
+++ b/refs.c
@@ -802,8 +802,9 @@ int ref_transaction_update(struct ref_transaction *transaction,
  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)) {
+ 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 25/29] 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 8f2a795..d0e932f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1556,6 +1556,226 @@ static void unlock_ref(struct ref_lock *lock)
 }
 
 /*
+ * Lock refname, without following symrefs, and set lock 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 deleting, 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) {
+ 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 {
+ /*
+ * There is a directory in the way,
+ * but we don't know of any references
+ * that it should contain. This might
+ * be a directory that used to contain
+ * references but is now empty. Try to
+ * remove it; otherwise it might cause
+ * trouble when we try to rename the
+ * lockfile into place.
+ */
+ 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.
@@ -3049,55 +3269,203 @@ 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,
+   update->flags & REF_DELETING,
+   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);
 
@@ -3121,7 +3489,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;
@@ -3138,6 +3506,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);
 
@@ -3149,9 +3520,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;
@@ -3159,6 +3546,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
@@ -3167,7 +3580,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;
@@ -3176,23 +3589,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];
@@ -3225,7 +3650,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 c46fe67..9839b07 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 26/29] 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 d0e932f..66ceb83 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3427,33 +3427,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 27/29] 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]>
---
There is one remaining race that I know of: the value of HEAD is read
at the start of the transaction to determine whether its referent is
changed in the transaction. If so, the update is also logged in HEAD's
reflog.

The problem is that HEAD is read without acquiring its lock. So in
principle HEAD could change during the time that the transaction is
being processed, leading to inconsistencies in the reflogs.

This problem could be fixed, but it would require HEAD to be locked
for every reference transaction, probably in some kind of an
additional fake ref_update. I don't know that it's worth the effort,
but if it is it can be done within the same framework that I
implemented here.

 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 66ceb83..40ed157 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3366,8 +3366,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;
 
@@ -3375,6 +3382,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.
@@ -3428,44 +3446,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 9839b07..2355735 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 28/29] 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 40ed157..938871b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2568,7 +2568,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)
 {
@@ -2636,7 +2636,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;
@@ -2656,7 +2656,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);
  }
@@ -2870,12 +2870,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);
@@ -2911,7 +2911,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 29/29] 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()
* 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, 21 insertions(+), 33 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 938871b..5dca352 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;
 };
@@ -1551,7 +1550,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);
 }
 
@@ -1605,7 +1603,6 @@ static int lock_raw_ref(const char *refname, int deleting, 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:
@@ -1991,14 +1988,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);
 
@@ -2008,46 +2004,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) {
+ 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)) {
+ strbuf_git_path(&ref_file, "%s", refname);
+ 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
@@ -2064,7 +2053,7 @@ 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_reset(&ref_file);
  strbuf_git_path(&ref_file, "%s", refname);
 
  retry:
@@ -2108,7 +2097,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;
 }
@@ -2873,9 +2861,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);
@@ -2883,7 +2869,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
@@ -2899,6 +2886,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) &&
@@ -2911,6 +2899,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);
@@ -3016,7 +3005,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 05/29] refname_is_safe(): insist that the refname already be normalized

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

> Does anybody have a use case for allowing un-normalized reference
> names like "refs/foo/../bar///baz"? I'm pretty certain they would not
> be handled correctly anyway, especially if they are not stored as
> loose references.

I wondered what codepath this follows:

    $ git rev-parse mhagger/wip//split-under-lock

get_sha1_basic() calls to dwim_ref() to learn real_ref and its
value.

dwim_ref() repeatedly gives the string to a known dwim rule, and the
"refs/remotes/%.*s" rule is expected to find that the user meant to
name "refs/remotes/mhagger/wip//split-under-lock".

This, still with doubled slash, is passed to resolve_ref_unsafe(),
which has a call to !refname_is_safe(refname) to reject the request.

So I think this will move the rejection early in the codepath than
how we reject the ref with doubled slash in the current code, but
the end result would be the same for this example.  The same is true
for heads//master or refs/heads//master.

There is another call to refname_is_safe() in resolve_ref_unsafe(),
which applies the sanity check to the string from a symref.  We seem
to allow

    $ git symbolic-ref refs/heads/SSS refs/heads//master

and we end up storing "ref: refs/heads//master" (or make a symbolic
link with doubled slashes), but the current code considers the
resulting symbolic link as "dangling".  Again, this change moves the
rejection a bit earlier in the codepath, without changing the end
result, I'd think.




--
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 08/29] ref_transaction_commit(): remove local variable n

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

> This microoptimization doesn't make a significant difference in speed.
> And it causes problems if somebody ever wants to modify the function to
> add updates to a transaction as part of processing it, as will happen
> shortly.
>
> Make the same change in initial_ref_transaction_commit().
>
> Signed-off-by: Michael Haggerty <[hidden email]>
> ---

This particular change also makes the end result easier to read.  I
have no objection to officially declaring that we do support
"adding" new transaction update while we are committing (and we do
not support other futzing like "removing" or "reordering"), either.

I expect that somewhere in this series transaction->nr will not stay
constant even if the client code of ref-transaction API makes no
direct call that adds a new update[] element, though, even if it is
not done in this patch.

Thanks.
--
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 12/29] read_raw_ref(): improve docstring

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

>   * Backend-specific flags might be set in type as well, regardless of
>   * outcome.
>   *
> - * sb_path is workspace: the caller should allocate and free it.

All made sense.

A welcome bonus is the removal of this stale comment that 42a38cf7
(read_raw_ref(): manage own scratch space, 2016-04-07) forgot to
remove.


--
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 15/29] ref_transaction_create(): disallow recursive pruning

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

> It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING
> without REF_NODEREF. Forbid it explicitly. Change the one REF_ISPRUNING
> caller to pass REF_NODEREF too.
>
> Signed-off-by: Michael Haggerty <[hidden email]>
> ---
> This also makes later patches a bit clearer.

I wonder if it is more future-proof to solve this by doing

    -#define REF_ISPRUNING 0x04
    +#define REF_ISPRUNING (0x04 | REF_NODEREF)

instead.  It makes the intention clear that pruning is always about
the single level (i.e. no-deref).


>  refs.c               | 3 +++
>  refs/files-backend.c | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index ba14105..5dc2473 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -790,6 +790,9 @@ int ref_transaction_update(struct ref_transaction *transaction,
>   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'",
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 9faf17c..8fcbd7d 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2116,7 +2116,7 @@ static void prune_ref(struct ref_to_prune *r)
>   transaction = ref_transaction_begin(&err);
>   if (!transaction ||
>      ref_transaction_delete(transaction, r->name, r->sha1,
> -   REF_ISPRUNING, NULL, &err) ||
> +   REF_ISPRUNING | REF_NODEREF, NULL, &err) ||
>      ref_transaction_commit(transaction, &err)) {
>   ref_transaction_free(transaction);
>   error("%s", err.buf);
--
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 19/29] refs: don't dereference on rename

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

> @@ -2380,8 +2381,8 @@ 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)) {
> + if (!read_ref_full(newrefname, resolve_flags, sha1, NULL) &&
> +    delete_ref(newrefname, NULL, REF_NODEREF)) {

Could you explain s/sha1/NULL/ here in the proposed log message?

>   if (errno==EISDIR) {
>   struct strbuf path = STRBUF_INIT;
>   int result;
--
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 05/29] refname_is_safe(): insist that the refname already be normalized

David Turner
In reply to this post by Junio C Hamano
On Wed, 2016-04-27 at 10:59 -0700, Junio C Hamano wrote:

> There is another call to refname_is_safe() in resolve_ref_unsafe(),
> which applies the sanity check to the string from a symref.  We seem
> to allow
>
>     $ git symbolic-ref refs/heads/SSS refs/heads//master
>
> and we end up storing "ref: refs/heads//master" (or make a symbolic
> link with doubled slashes), but the current code considers the
> resulting symbolic link as "dangling".  Again, this change moves the
> rejection a bit earlier in the codepath, without changing the end
> result, I'd think.

I think we should disallow that -- refname_is_safe should probably call
(or be replaced with calls to) check_refname_format.  
--
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 24/29] ref_transaction_update(): check refname_is_safe() at a minimum

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

> 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().

It isn't clear to me what "in other cases" exactly refers to.  A
request to delete a ref would obviously one of those that do not
"ask that a new value be set", but are there other cases?

> Signed-off-by: Michael Haggerty <[hidden email]>
> ---
> There are remaining problems in this area of the code. For example,
> check_refname_format() is *less* strict than refname_is_safe(). It
> allows almost arbitrary top-level reference names like "foo" and
> "refs". (The latter is especially fun because if you manage to create
> a reference called "refs", Git stops recognizing the directory as a
> Git repository.) On the other hand, some callers call
> check_refname_format() with incomplete reference names (e.g., branch
> names like "master"), so the functions can't be made stricter until
> those callers are changed.
>
> I'll address these problems separately if I find the time.

Thanks.

>  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 858b6d7..41eb9e2 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -802,8 +802,9 @@ int ref_transaction_update(struct ref_transaction *transaction,
>   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)) {
> + 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
>  '
--
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 05/29] refname_is_safe(): insist that the refname already be normalized

Jeff King
In reply to this post by David Turner
On Wed, Apr 27, 2016 at 04:10:32PM -0400, David Turner wrote:

> On Wed, 2016-04-27 at 10:59 -0700, Junio C Hamano wrote:
>
> > There is another call to refname_is_safe() in resolve_ref_unsafe(),
> > which applies the sanity check to the string from a symref.  We seem
> > to allow
> >
> >     $ git symbolic-ref refs/heads/SSS refs/heads//master
> >
> > and we end up storing "ref: refs/heads//master" (or make a symbolic
> > link with doubled slashes), but the current code considers the
> > resulting symbolic link as "dangling".  Again, this change moves the
> > rejection a bit earlier in the codepath, without changing the end
> > result, I'd think.
>
> I think we should disallow that -- refname_is_safe should probably call
> (or be replaced with calls to) check_refname_format.  

I thought the point is that one is a lesser check than the other, and we
need different rules for different situations. So we might allow
deletion on a refname that does not pass check_refname_format(), but we
must make sure it is not going to cause any mischief (e.g., escaping
".git" and deleting random files).

But anything writing a _new_ refname (whether the actual ref, or
referencing it via a symref) should be using check_refname_format()
before writing.

-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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

David Turner
In reply to this post by Junio C Hamano
On Wed, 2016-04-27 at 11:47 -0700, Junio C Hamano wrote:

> Michael Haggerty <[hidden email]> writes:
>
> > It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING
> > without REF_NODEREF. Forbid it explicitly. Change the one
> > REF_ISPRUNING
> > caller to pass REF_NODEREF too.
> >
> > Signed-off-by: Michael Haggerty <[hidden email]>
> > ---
> > This also makes later patches a bit clearer.
>
> I wonder if it is more future-proof to solve this by doing
>
>     -#define REF_ISPRUNING 0x04
>     +#define REF_ISPRUNING (0x04 | REF_NODEREF)
>
> instead.  It makes the intention clear that pruning is always about
> the single level (i.e. no-deref).

I think the approach in Michael's patch might be clearer than yours,
since someone reading the code doesn't have to look at the definition
of REF_ISPRUNING to understand what is going on.
--
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 05/29] refname_is_safe(): insist that the refname already be normalized

David Turner
In reply to this post by Jeff King
On Wed, 2016-04-27 at 16:15 -0400, Jeff King wrote:

> On Wed, Apr 27, 2016 at 04:10:32PM -0400, David Turner wrote:
>
> > On Wed, 2016-04-27 at 10:59 -0700, Junio C Hamano wrote:
> >
> > > There is another call to refname_is_safe() in
> > > resolve_ref_unsafe(),
> > > which applies the sanity check to the string from a symref.  We
> > > seem
> > > to allow
> > >
> > >     $ git symbolic-ref refs/heads/SSS refs/heads//master
> > >
> > > and we end up storing "ref: refs/heads//master" (or make a
> > > symbolic
> > > link with doubled slashes), but the current code considers the
> > > resulting symbolic link as "dangling".  Again, this change moves
> > > the
> > > rejection a bit earlier in the codepath, without changing the end
> > > result, I'd think.
> >
> > I think we should disallow that -- refname_is_safe should probably
> > call
> > (or be replaced with calls to) check_refname_format.  
>
> I thought the point is that one is a lesser check than the other, and
> we
> need different rules for different situations. So we might allow
> deletion on a refname that does not pass check_refname_format(), but
> we
> must make sure it is not going to cause any mischief (e.g., escaping
> ".git" and deleting random files).
>
> But anything writing a _new_ refname (whether the actual ref, or
> referencing it via a symref) should be using check_refname_format()
> before writing.

Unfortunately, neither check is lesser -- refname_is_safe allows
refs/heads//foo but not a/b while check_refname_format allows a/b but
not refs/heads//foo.  So sometimes we need both, while other times we
just need one :(
--
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
1234