[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 00/29] Yet more preparation for reference backends

Michael Haggerty-2
This started as a modest attempt to move the last couple of patches
mentioned here [1] to before the vtable patches. I wanted to do that
because having real changes mixed with the vtable refactoring was
making rebasing and stuff awkward.

But then it snowballed. A lot of what's new is pretty trivial (though
in some cases fixes minor bugs). But a few of the later patches are
pretty major.

The two main problems addressed by this series are:

1. Reference transactions will, in the future, sometimes span two
   completely different reference backends. For example, we already
   have worktree-specific refs like `HEAD` and `refs/bisect/*`, which
   are stored within the worktree, plus shared refs, which live in the
   main repository. It is even possible for a symref in one reference
   backend to point at a reference in another reference backend. Thus
   we need to be able to split one main transaction into one
   transaction per backend.

2. Currently, reference transactions that involve symbolic refs are
   not atomic: the symref is not locked at all, even when its reflog
   is being updated. This is a no-no. It also means that its referent
   can change between the time that the symref is resolved to find out
   its old_oid and the time that the referent is locked. These aren't
   super serious races because symrefs are usually not modified very
   often (especially on Git servers, which is mostly where concurrent
   modifications are an issue). But still...

The approach taken to solve these problems is inspired by David
Turner's patch [2], whose approach was first discussed here [3]. David
wrote a separate function, dereference_symrefs(transaction, ...),
which scanned through the whole transaction and split up any symref
updates into one symref update with the REF_LOG_ONLY option (to update
the symrefs reflog), plus a second update that changes the underlying
reference. This approach solves problem 1 but not problem 2.

This patch series takes a slightly different approach. For each
reference update during the "lock references" part of
ref_transaction_commit(), it

1. Locks the named reference. (If it is a symref, lock *the symref*,
   not its referent!)

2. Reads the reference non-recursively

3. If it is a symref, change the update to REF_LOG_ONLY and add a
   second update to the transaction for the underlying reference.

4. If it is not a symref but was derived from a symref update, record
   the reference's old_oid in the ref_update structure for the
   original symref.

In this way, each reference in a symref chain is locked down *before*
we read it and the lock is held throughout the transaction. As a
bonus, we don't have to use resolve_ref_full() to find old_oid for the
references; it is enough to read the references *once*, because we do
it under lock.

The three patches starting with "refs: resolve symbolic refs first"
involve a lot of new code in an area that is pretty intricate. I've
reviewed them a few times, but it's quite possible I've made some
mistakes (especially in the error-handling code). Careful review here
would be much appreciated.

It's possible that people will think this is too much new code for
fixing a relatively unlikely race. I'm not absolutely convinced myself
that it's not overengineered. But splitting ref_updates is definitely
needed for supporting multiple backends, and I think the approach of
locking then following one reference at a time is more or less a
prerequisite for making symref locking work correctly with multiple
reference backends. So (I think) our choices are to accept this code
or something like it, or to give up the hope of correct atomicity of
transactions that span backends.

This patch series is also available from my GitHub repository [4] as
branch "split-under-lock". It applies to Junio's current master.

Incidentally, a draft of the *next* patch series, which adds a
ref_store vtable abstraction for managing reference backends, is
available as branch "ref-store" in my GitHub repo. That branch passes
all tests but is not yet carefully reviewed.

Following is a table of contents for this patch series...

Chapter 1: Prologue

  This chapter consists of small cleanups that I noticed while working
  on the code.

  * safe_create_leading_directories(): improve docstring
  * remove_dir_recursively(): add docstring
  * refname_is_safe(): use skip_prefix()

  This patch fixes a bug in refname_is_safe():

  * refname_is_safe(): don't allow the empty string

  This one makes refname_is_safe() a little stricter...it shouldn't
  allow refnames like "refs/foo/../bar///baz" because the downstream
  code isn't smart enough to handle them anyway. (At the latest if the
  reference has to be sought in packed-refs, things would fail):

  * refname_is_safe(): insist that the refname already be normalized

Chapter 2: Character development

  Make sure error output goes the right place:

  * commit_ref_update(): write error message to *err, not stderr

  Tighten up some code, rename some parameters:

  * rename_ref(): remove unneeded local variable
  * ref_transaction_commit(): remove local variable n
  * read_raw_ref(): rename flags argument to type
  * read_raw_ref(): clear *type at start of function
  * read_raw_ref(): rename symref argument to referent
  * read_raw_ref(): improve docstring
  * lock_ref_sha1_basic(): remove unneeded local variable

Chapter 3: Small adventures

  Change error messages to adhere to our new policy of starting with
  lower-case letters:

  * refs: make error messages more consistent

  Require REF_NODEREF if REF_ISPRUNING is set:

  * ref_transaction_create(): disallow recursive pruning

  Correctly handle an (unlikely) error path:

  * ref_transaction_commit(): correctly report close_ref() failure

  Oops, here's a problem that could have bit us later. (In fact, it
  did bite me when I implemented one of the later patches):

  * delete_branches(): use resolve_refdup()

Chapter 4: Flashback

  The following two patches are split out of one of David Turner's
  patches mentioned above:

  * refs: allow log-only updates
  * refs: don't dereference on rename

Chapter 5: The tension builds

  The protagonist clearly has something planned...but what?

  * verify_refname_available(): adjust constness in declaration
  * add_update(): initialize the whole ref_update
  * lock_ref_for_update(): new function
  * unlock_ref(): move definition higher in the file

  Here we force a little more refname safety while building up
  ref_transactions:

  * ref_transaction_update(): check refname_is_safe() at a minimum

Chapter 6: The climax

  This is the guts of the patch series. Everything up to this point
  should be pretty uncontroversial. These three patches are the main
  subject of the discussion above. They are pretty large patches, but
  I don't see a reasonable way to break them up more:

  * refs: resolve symbolic refs first
  * lock_ref_for_update(): don't re-read non-symbolic references
  * lock_ref_for_update(): don't resolve symrefs

Chapter 7: Denouement

  Remove some stuff that's not needed anymore.

  * commit_ref_update(): remove the flags parameter
  * lock_ref_sha1_basic(): only handle REF_NODEREF mode

Michael

[1] http://article.gmane.org/gmane.comp.version-control.git/289994
[2] http://thread.gmane.org/gmane.comp.version-control.git/287949/focus=287958
[3] http://article.gmane.org/gmane.comp.version-control.git/282927
[4] https://github.com/mhagger/git

David Turner (2):
  refs: allow log-only updates
  refs: don't dereference on rename

Michael Haggerty (27):
  safe_create_leading_directories(): improve docstring
  remove_dir_recursively(): add docstring
  refname_is_safe(): use skip_prefix()
  refname_is_safe(): don't allow the empty string
  refname_is_safe(): insist that the refname already be normalized
  commit_ref_update(): write error message to *err, not stderr
  rename_ref(): remove unneeded local variable
  ref_transaction_commit(): remove local variable n
  read_raw_ref(): rename flags argument to type
  read_raw_ref(): clear *type at start of function
  read_raw_ref(): rename symref argument to referent
  read_raw_ref(): improve docstring
  lock_ref_sha1_basic(): remove unneeded local variable
  refs: make error messages more consistent
  ref_transaction_create(): disallow recursive pruning
  ref_transaction_commit(): correctly report close_ref() failure
  delete_branches(): use resolve_refdup()
  verify_refname_available(): adjust constness in declaration
  add_update(): initialize the whole ref_update
  lock_ref_for_update(): new function
  unlock_ref(): move definition higher in the file
  ref_transaction_update(): check refname_is_safe() at a minimum
  refs: resolve symbolic refs first
  lock_ref_for_update(): don't re-read non-symbolic references
  lock_ref_for_update(): don't resolve symrefs
  commit_ref_update(): remove the flags parameter
  lock_ref_sha1_basic(): only handle REF_NODEREF mode

 builtin/branch.c        |  19 +-
 cache.h                 |   5 +
 dir.h                   |  23 ++
 refs.c                  |  67 ++--
 refs/files-backend.c    | 882 +++++++++++++++++++++++++++++++++++++-----------
 refs/refs-internal.h    |  55 ++-
 t/t1400-update-ref.sh   |  41 ++-
 t/t1430-bad-ref-name.sh |   2 +-
 t/t3200-branch.sh       |   9 +
 9 files changed, 869 insertions(+), 234 deletions(-)

--
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 01/29] safe_create_leading_directories(): improve docstring

Michael Haggerty-2
Document the difference between this function and
safe_create_leading_directories_const(), and that the former restores
path before returning.

Signed-off-by: Michael Haggerty <[hidden email]>
---
 cache.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/cache.h b/cache.h
index 2711048..4134f64 100644
--- a/cache.h
+++ b/cache.h
@@ -993,6 +993,11 @@ int adjust_shared_perm(const char *path);
  * directory while we were working.  To be robust against this kind of
  * race, callers might want to try invoking the function again when it
  * returns SCLD_VANISHED.
+ *
+ * safe_create_leading_directories() temporarily changes path while it
+ * is working but restores it before returning.
+ * safe_create_leading_directories_const() doesn't modify path, even
+ * temporarily.
  */
 enum scld_error {
  SCLD_OK = 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 02/29] remove_dir_recursively(): add docstring

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
Add a docstring for the remove_dir_recursively() function and the
REMOVE_DIR_* flags that can be passed to it.

Signed-off-by: Michael Haggerty <[hidden email]>
---
 dir.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/dir.h b/dir.h
index 301b737..5f19acc 100644
--- a/dir.h
+++ b/dir.h
@@ -262,9 +262,32 @@ extern int is_empty_dir(const char *dir);
 
 extern void setup_standard_excludes(struct dir_struct *dir);
 
+
+/* Constants for remove_dir_recursively: */
+
+/*
+ * If a non-directory is found within path, stop and return an error.
+ * (In this case some empty directories might already have been
+ * removed.)
+ */
 #define REMOVE_DIR_EMPTY_ONLY 01
+
+/*
+ * If any Git work trees are found within path, skip them without
+ * considering it an error.
+ */
 #define REMOVE_DIR_KEEP_NESTED_GIT 02
+
+/* Remove the contents of path, but leave path itself. */
 #define REMOVE_DIR_KEEP_TOPLEVEL 04
+
+/*
+ * Remove path and its contents, recursively. flags is a combination
+ * of the above REMOVE_DIR_* constants. Return 0 on success.
+ *
+ * This function uses path as temporary scratch space, but restores it
+ * before returning.
+ */
 extern int remove_dir_recursively(struct strbuf *path, int flag);
 
 /* tries to remove the path with empty directories along it, ignores ENOENT */
--
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 03/29] refname_is_safe(): use skip_prefix()

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
Signed-off-by: Michael Haggerty <[hidden email]>
---
 refs.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 87dc82f..5789152 100644
--- a/refs.c
+++ b/refs.c
@@ -120,17 +120,19 @@ int check_refname_format(const char *refname, int flags)
 
 int refname_is_safe(const char *refname)
 {
- if (starts_with(refname, "refs/")) {
+ const char *rest;
+
+ if (skip_prefix(refname, "refs/", &rest)) {
  char *buf;
  int result;
 
- buf = xmallocz(strlen(refname));
  /*
  * Does the refname try to escape refs/?
  * For example: refs/foo/../bar is safe but refs/foo/../../bar
  * is not.
  */
- result = !normalize_path_copy(buf, refname + strlen("refs/"));
+ buf = xmallocz(strlen(rest));
+ result = !normalize_path_copy(buf, rest);
  free(buf);
  return result;
  }
--
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 04/29] refname_is_safe(): don't allow the empty string

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
Signed-off-by: Michael Haggerty <[hidden email]>
---
This fixes a coding error from the original implementation.

 refs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 5789152..ca0280f 100644
--- a/refs.c
+++ b/refs.c
@@ -136,11 +136,12 @@ int refname_is_safe(const char *refname)
  free(buf);
  return result;
  }
- while (*refname) {
+
+ do {
  if (!isupper(*refname) && *refname != '_')
  return 0;
  refname++;
- }
+ } while (*refname);
  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 05/29] refname_is_safe(): insist that the refname already be normalized

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
The reference name is going to be compared to other reference names, so
it should be in its normalized form.

Signed-off-by: Michael Haggerty <[hidden email]>
---
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.

 refs.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index ca0280f..b18d995 100644
--- a/refs.c
+++ b/refs.c
@@ -125,14 +125,19 @@ int refname_is_safe(const char *refname)
  if (skip_prefix(refname, "refs/", &rest)) {
  char *buf;
  int result;
+ size_t restlen = strlen(rest);
+
+ /* rest must not be empty, or start or end with "/" */
+ if (!restlen || *rest == '/' || rest[restlen - 1] == '/')
+ return 0;
 
  /*
  * Does the refname try to escape refs/?
  * For example: refs/foo/../bar is safe but refs/foo/../../bar
  * is not.
  */
- buf = xmallocz(strlen(rest));
- result = !normalize_path_copy(buf, rest);
+ buf = xmallocz(restlen);
+ result = !normalize_path_copy(buf, rest) && !strcmp(buf, rest);
  free(buf);
  return result;
  }
--
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 06/29] commit_ref_update(): write error message to *err, not stderr

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1f38076..de38517 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2686,7 +2686,7 @@ static int commit_ref_update(struct ref_lock *lock,
  }
  }
  if (commit_ref(lock)) {
- error("Couldn't set %s", lock->ref_name);
+ 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 07/29] rename_ref(): remove unneeded local variable

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
Signed-off-by: Michael Haggerty <[hidden email]>
---
 refs/files-backend.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index de38517..0ade681 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2351,20 +2351,17 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
  struct ref_lock *lock;
  struct stat loginfo;
  int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
- const char *symref = NULL;
  struct strbuf err = STRBUF_INIT;
 
  if (log && S_ISLNK(loginfo.st_mode))
  return error("reflog for %s is a symlink", oldrefname);
 
- symref = resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING,
-    orig_sha1, &flag);
+ if (!resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING, orig_sha1, &flag))
+ return error("refname %s not found", oldrefname);
+
  if (flag & REF_ISSYMREF)
  return error("refname %s is a symbolic ref, renaming it is not supported",
  oldrefname);
- if (!symref)
- return error("refname %s not found", oldrefname);
-
  if (!rename_ref_available(oldrefname, newrefname))
  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 08/29] ref_transaction_commit(): remove local variable n

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
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]>
---
 refs/files-backend.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0ade681..05797cb 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3043,7 +3043,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
    struct strbuf *err)
 {
  int ret = 0, i;
- int n = transaction->nr;
  struct ref_update **updates = transaction->updates;
  struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
  struct string_list_item *ref_to_delete;
@@ -3054,13 +3053,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  if (transaction->state != REF_TRANSACTION_OPEN)
  die("BUG: commit called for transaction that is not open");
 
- if (!n) {
+ if (!transaction->nr) {
  transaction->state = REF_TRANSACTION_CLOSED;
  return 0;
  }
 
  /* Fail if a refname appears more than once in the transaction: */
- for (i = 0; i < n; i++)
+ for (i = 0; i < transaction->nr; i++)
  string_list_append(&affected_refnames, updates[i]->refname);
  string_list_sort(&affected_refnames);
  if (ref_update_reject_duplicates(&affected_refnames, err)) {
@@ -3074,7 +3073,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  * lockfiles, ready to be activated. Only keep one lockfile
  * open at a time to avoid running out of file descriptors.
  */
- for (i = 0; i < n; i++) {
+ for (i = 0; i < transaction->nr; i++) {
  struct ref_update *update = updates[i];
 
  if ((update->flags & REF_HAVE_NEW) &&
@@ -3145,7 +3144,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  }
 
  /* Perform updates first so live commits remain referenced */
- for (i = 0; i < n; i++) {
+ for (i = 0; i < transaction->nr; i++) {
  struct ref_update *update = updates[i];
 
  if (update->flags & REF_NEEDS_COMMIT) {
@@ -3164,7 +3163,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  }
 
  /* Perform deletes now that updates are safely completed */
- for (i = 0; i < n; i++) {
+ for (i = 0; i < transaction->nr; i++) {
  struct ref_update *update = updates[i];
 
  if (update->flags & REF_DELETING) {
@@ -3190,7 +3189,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 cleanup:
  transaction->state = REF_TRANSACTION_CLOSED;
 
- for (i = 0; i < n; i++)
+ for (i = 0; i < transaction->nr; i++)
  if (updates[i]->lock)
  unlock_ref(updates[i]->lock);
  string_list_clear(&refs_to_delete, 0);
@@ -3210,7 +3209,6 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
    struct strbuf *err)
 {
  int ret = 0, i;
- int n = transaction->nr;
  struct ref_update **updates = transaction->updates;
  struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
 
@@ -3220,7 +3218,7 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
  die("BUG: commit called for transaction that is not open");
 
  /* Fail if a refname appears more than once in the transaction: */
- for (i = 0; i < n; i++)
+ for (i = 0; i < transaction->nr; i++)
  string_list_append(&affected_refnames, updates[i]->refname);
  string_list_sort(&affected_refnames);
  if (ref_update_reject_duplicates(&affected_refnames, err)) {
@@ -3243,7 +3241,7 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
  if (for_each_rawref(ref_present, &affected_refnames))
  die("BUG: initial ref transaction called with existing refs");
 
- for (i = 0; i < n; i++) {
+ for (i = 0; i < transaction->nr; i++) {
  struct ref_update *update = updates[i];
 
  if ((update->flags & REF_HAVE_OLD) &&
@@ -3264,7 +3262,7 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
  goto cleanup;
  }
 
- for (i = 0; i < n; i++) {
+ for (i = 0; i < transaction->nr; i++) {
  struct ref_update *update = updates[i];
 
  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 09/29] read_raw_ref(): rename flags argument to type

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
This will hopefully reduce confusion with the "flags" arguments that are
used in many functions in this module as an input parameter to choose
how the function should operate.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 05797cb..303c43b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1395,18 +1395,18 @@ static int resolve_missing_loose_ref(const char *refname,
  *
  * If the ref is symbolic, fill in *symref with the referrent
  * (e.g. "refs/heads/master") and return 0.  The caller is responsible
- * for validating the referrent.  Set REF_ISSYMREF in flags.
+ * for validating the referrent.  Set REF_ISSYMREF in type.
  *
  * If the ref doesn't exist, set errno to ENOENT and return -1.
  *
  * If the ref exists but is neither a symbolic ref nor a sha1, it is
- * broken. Set REF_ISBROKEN in flags, set errno to EINVAL, and return
+ * broken. Set REF_ISBROKEN in type, set errno to EINVAL, and return
  * -1.
  *
  * If there is another error reading the ref, set errno appropriately and
  * return -1.
  *
- * Backend-specific flags might be set in flags as well, regardless of
+ * Backend-specific flags might be set in type as well, regardless of
  * outcome.
  *
  * sb_path is workspace: the caller should allocate and free it.
@@ -1419,7 +1419,7 @@ static int resolve_missing_loose_ref(const char *refname,
  *   refname will still be valid and unchanged.
  */
 int read_raw_ref(const char *refname, unsigned char *sha1,
- struct strbuf *symref, unsigned int *flags)
+ struct strbuf *symref, unsigned int *type)
 {
  struct strbuf sb_contents = STRBUF_INIT;
  struct strbuf sb_path = STRBUF_INIT;
@@ -1448,7 +1448,7 @@ stat_ref:
  if (lstat(path, &st) < 0) {
  if (errno != ENOENT)
  goto out;
- if (resolve_missing_loose_ref(refname, sha1, flags)) {
+ if (resolve_missing_loose_ref(refname, sha1, type)) {
  errno = ENOENT;
  goto out;
  }
@@ -1469,7 +1469,7 @@ stat_ref:
  if (starts_with(sb_contents.buf, "refs/") &&
     !check_refname_format(sb_contents.buf, 0)) {
  strbuf_swap(&sb_contents, symref);
- *flags |= REF_ISSYMREF;
+ *type |= REF_ISSYMREF;
  ret = 0;
  goto out;
  }
@@ -1510,7 +1510,7 @@ stat_ref:
 
  strbuf_reset(symref);
  strbuf_addstr(symref, buf);
- *flags |= REF_ISSYMREF;
+ *type |= REF_ISSYMREF;
  ret = 0;
  goto out;
  }
@@ -1521,7 +1521,7 @@ stat_ref:
  */
  if (get_sha1_hex(buf, sha1) ||
     (buf[40] != '\0' && !isspace(buf[40]))) {
- *flags |= REF_ISBROKEN;
+ *type |= REF_ISBROKEN;
  errno = EINVAL;
  goto out;
  }
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 3a4f634..0b047f6 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -210,6 +210,6 @@ int do_for_each_ref(const char *submodule, const char *base,
     each_ref_fn fn, int trim, int flags, void *cb_data);
 
 int read_raw_ref(const char *refname, unsigned char *sha1,
- struct strbuf *symref, unsigned int *flags);
+ struct strbuf *symref, unsigned int *type);
 
 #endif /* REFS_REFS_INTERNAL_H */
--
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 10/29] read_raw_ref(): clear *type at start of function

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
This is more convenient and less error-prone for callers.

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 303c43b..f10c80f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1430,6 +1430,7 @@ int read_raw_ref(const char *refname, unsigned char *sha1,
  int ret = -1;
  int save_errno;
 
+ *type = 0;
  strbuf_reset(&sb_path);
  strbuf_git_path(&sb_path, "%s", refname);
  path = sb_path.buf;
--
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 11/29] read_raw_ref(): rename symref argument to referent

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
After all, it doesn't hold the symbolic reference, but rather the
reference referred to.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f10c80f..364425c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1393,9 +1393,10 @@ static int resolve_missing_loose_ref(const char *refname,
  *
  * If the ref is a sha1, fill in sha1 and return 0.
  *
- * If the ref is symbolic, fill in *symref with the referrent
- * (e.g. "refs/heads/master") and return 0.  The caller is responsible
- * for validating the referrent.  Set REF_ISSYMREF in type.
+ * If the ref is symbolic, fill in *referent with the name of the
+ * branch to which it refers (e.g. "refs/heads/master") and return 0.
+ * The caller is responsible for validating the referent. Set
+ * REF_ISSYMREF in type.
  *
  * If the ref doesn't exist, set errno to ENOENT and return -1.
  *
@@ -1411,15 +1412,15 @@ static int resolve_missing_loose_ref(const char *refname,
  *
  * sb_path is workspace: the caller should allocate and free it.
  *
- * It is OK for refname to point into symref. In this case:
- * - if the function succeeds with REF_ISSYMREF, symref will be
+ * It is OK for refname to point into referent. In this case:
+ * - if the function succeeds with REF_ISSYMREF, referent will be
  *   overwritten and the memory pointed to by refname might be changed
  *   or even freed.
- * - in all other cases, symref will be untouched, and therefore
+ * - in all other cases, referent will be untouched, and therefore
  *   refname will still be valid and unchanged.
  */
 int read_raw_ref(const char *refname, unsigned char *sha1,
- struct strbuf *symref, unsigned int *type)
+ struct strbuf *referent, unsigned int *type)
 {
  struct strbuf sb_contents = STRBUF_INIT;
  struct strbuf sb_path = STRBUF_INIT;
@@ -1469,7 +1470,7 @@ stat_ref:
  }
  if (starts_with(sb_contents.buf, "refs/") &&
     !check_refname_format(sb_contents.buf, 0)) {
- strbuf_swap(&sb_contents, symref);
+ strbuf_swap(&sb_contents, referent);
  *type |= REF_ISSYMREF;
  ret = 0;
  goto out;
@@ -1509,8 +1510,8 @@ stat_ref:
  while (isspace(*buf))
  buf++;
 
- strbuf_reset(symref);
- strbuf_addstr(symref, buf);
+ strbuf_reset(referent);
+ strbuf_addstr(referent, buf);
  *type |= REF_ISSYMREF;
  ret = 0;
  goto out;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 0b047f6..37a1a37 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -210,6 +210,6 @@ int do_for_each_ref(const char *submodule, const char *base,
     each_ref_fn fn, int trim, int flags, void *cb_data);
 
 int read_raw_ref(const char *refname, unsigned char *sha1,
- struct strbuf *symref, unsigned int *type);
+ struct strbuf *referent, unsigned int *type);
 
 #endif /* REFS_REFS_INTERNAL_H */
--
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 12/29] read_raw_ref(): improve docstring

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
Among other things, document the (important!) requirement that input
refname be checked for safety before calling this function.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 364425c..1d2ef2a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1389,33 +1389,40 @@ static int resolve_missing_loose_ref(const char *refname,
 }
 
 /*
- * Read a raw ref from the filesystem or packed refs file.
+ * Read the specified reference from the filesystem or packed refs
+ * file, non-recursively. Set type to describe the reference, and:
  *
- * If the ref is a sha1, fill in sha1 and return 0.
+ * - If refname is the name of a normal reference, fill in sha1
+ *   (leaving referent unchanged).
  *
- * If the ref is symbolic, fill in *referent with the name of the
- * branch to which it refers (e.g. "refs/heads/master") and return 0.
- * The caller is responsible for validating the referent. Set
- * REF_ISSYMREF in type.
+ * - If refname is the name of a symbolic reference, write the full
+ *   name of the reference to which it refers (e.g.
+ *   "refs/heads/master") to referent and set the REF_ISSYMREF bit in
+ *   type (leaving sha1 unchanged). The caller is responsible for
+ *   validating that referent is a valid reference name.
  *
- * If the ref doesn't exist, set errno to ENOENT and return -1.
+ * WARNING: refname might be used as part of a filename, so it is
+ * important from a security standpoint that it be safe in the sense
+ * of refname_is_safe(). Moreover, for symrefs this function sets
+ * referent to whatever the repository says, which might not be a
+ * properly-formatted or even safe reference name. NEITHER INPUT NOR
+ * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION.
  *
- * If the ref exists but is neither a symbolic ref nor a sha1, it is
- * broken. Set REF_ISBROKEN in type, set errno to EINVAL, and return
- * -1.
- *
- * If there is another error reading the ref, set errno appropriately and
- * return -1.
+ * Return 0 on success. If the ref doesn't exist, set errno to ENOENT
+ * and return -1. If the ref exists but is neither a symbolic ref nor
+ * a sha1, it is broken; set REF_ISBROKEN in type, set errno to
+ * EINVAL, and return -1. If there is another error reading the ref,
+ * set errno appropriately and return -1.
  *
  * Backend-specific flags might be set in type as well, regardless of
  * outcome.
  *
- * sb_path is workspace: the caller should allocate and free it.
+ * It is OK for refname to point into referent. If so:
  *
- * It is OK for refname to point into referent. In this case:
  * - if the function succeeds with REF_ISSYMREF, referent will be
- *   overwritten and the memory pointed to by refname might be changed
- *   or even freed.
+ *   overwritten and the memory formerly pointed to by it might be
+ *   changed or even freed.
+ *
  * - in all other cases, referent will be untouched, and therefore
  *   refname will still be valid and unchanged.
  */
--
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 13/29] lock_ref_sha1_basic(): remove unneeded local variable

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
resolve_ref_unsafe() can cope with being called with NULL passed to its
flags argument. So lock_ref_sha1_basic() can just hand its own type
parameter through.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1d2ef2a..b8c1779 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1767,7 +1767,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
     const unsigned char *old_sha1,
     const struct string_list *extras,
     const struct string_list *skip,
-    unsigned int flags, int *type_p,
+    unsigned int flags, int *type,
     struct strbuf *err)
 {
  struct strbuf ref_file = STRBUF_INIT;
@@ -1775,7 +1775,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
  const char *orig_refname = refname;
  struct ref_lock *lock;
  int last_errno = 0;
- int type;
  int lflags = 0;
  int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
  int resolve_flags = 0;
@@ -1795,7 +1794,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
  }
 
  refname = resolve_ref_unsafe(refname, resolve_flags,
-     lock->old_oid.hash, &type);
+     lock->old_oid.hash, type);
  if (!refname && errno == EISDIR) {
  /*
  * we are trying to lock foo but we used to
@@ -1813,10 +1812,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
  goto error_return;
  }
  refname = resolve_ref_unsafe(orig_refname, resolve_flags,
-     lock->old_oid.hash, &type);
+     lock->old_oid.hash, type);
  }
- if (type_p)
-    *type_p = type;
  if (!refname) {
  last_errno = errno;
  if (last_errno != ENOTDIR ||
--
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 14/29] refs: make error messages more consistent

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
* Always start error messages with a lower-case letter.

* Always enclose reference names in single quotes.

Signed-off-by: Michael Haggerty <[hidden email]>
---
This change is not strictly needed, but I wanted to fix the old error
messages before I started adding new ones (otherwise, should the new
error messages be made to look like the old ones or should they be
corrected?) By the way, should these be internationalized?

 refs.c                |  8 ++++----
 refs/files-backend.c  | 32 ++++++++++++++++----------------
 t/t1400-update-ref.sh |  4 ++--
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/refs.c b/refs.c
index b18d995..ba14105 100644
--- a/refs.c
+++ b/refs.c
@@ -504,7 +504,7 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
  filename = git_path("%s", pseudoref);
  fd = hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);
  if (fd < 0) {
- strbuf_addf(err, "Could not open '%s' for writing: %s",
+ strbuf_addf(err, "could not open '%s' for writing: %s",
     filename, strerror(errno));
  return -1;
  }
@@ -515,14 +515,14 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
  if (read_ref(pseudoref, actual_old_sha1))
  die("could not read ref '%s'", pseudoref);
  if (hashcmp(actual_old_sha1, old_sha1)) {
- strbuf_addf(err, "Unexpected sha1 when writing %s", pseudoref);
+ strbuf_addf(err, "unexpected sha1 when writing '%s'", pseudoref);
  rollback_lock_file(&lock);
  goto done;
  }
  }
 
  if (write_in_full(fd, buf.buf, buf.len) != buf.len) {
- strbuf_addf(err, "Could not write to '%s'", filename);
+ strbuf_addf(err, "could not write to '%s'", filename);
  rollback_lock_file(&lock);
  goto done;
  }
@@ -792,7 +792,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
 
  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",
+ strbuf_addf(err, "refusing to update ref with bad name '%s'",
     refname);
  return -1;
  }
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b8c1779..9faf17c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1730,7 +1730,7 @@ static int verify_lock(struct ref_lock *lock,
   lock->old_oid.hash, NULL)) {
  if (old_sha1) {
  int save_errno = errno;
- strbuf_addf(err, "can't verify ref %s", lock->ref_name);
+ strbuf_addf(err, "can't verify ref '%s'", lock->ref_name);
  errno = save_errno;
  return -1;
  } else {
@@ -1739,7 +1739,7 @@ static int verify_lock(struct ref_lock *lock,
  }
  }
  if (old_sha1 && hashcmp(lock->old_oid.hash, old_sha1)) {
- strbuf_addf(err, "ref %s is at %s but expected %s",
+ strbuf_addf(err, "ref '%s' is at %s but expected %s",
     lock->ref_name,
     sha1_to_hex(lock->old_oid.hash),
     sha1_to_hex(old_sha1));
@@ -1819,7 +1819,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
  if (last_errno != ENOTDIR ||
     !verify_refname_available_dir(orig_refname, extras, skip,
   get_loose_refs(&ref_cache), err))
- strbuf_addf(err, "unable to resolve reference %s: %s",
+ strbuf_addf(err, "unable to resolve reference '%s': %s",
     orig_refname, strerror(last_errno));
 
  goto error_return;
@@ -1857,7 +1857,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
  /* fall through */
  default:
  last_errno = errno;
- strbuf_addf(err, "unable to create directory for %s",
+ strbuf_addf(err, "unable to create directory for '%s'",
     ref_file.buf);
  goto error_return;
  }
@@ -2478,7 +2478,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
  strbuf_git_path(logfile, "logs/%s", refname);
  if (force_create || should_autocreate_reflog(refname)) {
  if (safe_create_leading_directories(logfile->buf) < 0) {
- strbuf_addf(err, "unable to create directory for %s: "
+ strbuf_addf(err, "unable to create directory for '%s': "
     "%s", logfile->buf, strerror(errno));
  return -1;
  }
@@ -2492,7 +2492,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
 
  if (errno == EISDIR) {
  if (remove_empty_directories(logfile)) {
- strbuf_addf(err, "There are still logs under "
+ strbuf_addf(err, "there are still logs under "
     "'%s'", logfile->buf);
  return -1;
  }
@@ -2500,7 +2500,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
  }
 
  if (logfd < 0) {
- strbuf_addf(err, "unable to append to %s: %s",
+ strbuf_addf(err, "unable to append to '%s': %s",
     logfile->buf, strerror(errno));
  return -1;
  }
@@ -2569,13 +2569,13 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
  result = log_ref_write_fd(logfd, old_sha1, new_sha1,
   git_committer_info(0), msg);
  if (result) {
- strbuf_addf(err, "unable to append to %s: %s", logfile->buf,
+ strbuf_addf(err, "unable to append to '%s': %s", logfile->buf,
     strerror(errno));
  close(logfd);
  return -1;
  }
  if (close(logfd)) {
- strbuf_addf(err, "unable to append to %s: %s", logfile->buf,
+ strbuf_addf(err, "unable to append to '%s': %s", logfile->buf,
     strerror(errno));
  return -1;
  }
@@ -2616,14 +2616,14 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
  o = parse_object(sha1);
  if (!o) {
  strbuf_addf(err,
-    "Trying to write ref %s with nonexistent object %s",
+    "trying to write ref '%s' with nonexistent object %s",
     lock->ref_name, sha1_to_hex(sha1));
  unlock_ref(lock);
  return -1;
  }
  if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {
  strbuf_addf(err,
-    "Trying to write non-commit object %s to branch %s",
+    "trying to write non-commit object %s to branch '%s'",
     sha1_to_hex(sha1), lock->ref_name);
  unlock_ref(lock);
  return -1;
@@ -2633,7 +2633,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
     write_in_full(fd, &term, 1) != 1 ||
     close_ref(lock) < 0) {
  strbuf_addf(err,
-    "Couldn't write %s", get_lock_file_path(lock->lk));
+    "couldn't write '%s'", get_lock_file_path(lock->lk));
  unlock_ref(lock);
  return -1;
  }
@@ -2654,7 +2654,7 @@ static int commit_ref_update(struct ref_lock *lock,
     (strcmp(lock->ref_name, lock->orig_ref_name) &&
      log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, flags, err) < 0)) {
  char *old_msg = strbuf_detach(err, NULL);
- strbuf_addf(err, "Cannot update the ref '%s': %s",
+ strbuf_addf(err, "cannot update the ref '%s': %s",
     lock->ref_name, old_msg);
  free(old_msg);
  unlock_ref(lock);
@@ -2689,7 +2689,7 @@ static int commit_ref_update(struct ref_lock *lock,
  }
  }
  if (commit_ref(lock)) {
- strbuf_addf(err, "Couldn't set %s", lock->ref_name);
+ strbuf_addf(err, "couldn't set '%s'", lock->ref_name);
  unlock_ref(lock);
  return -1;
  }
@@ -3038,7 +3038,7 @@ static int ref_update_reject_duplicates(struct string_list *refnames,
  for (i = 1; i < n; i++)
  if (!strcmp(refnames->items[i - 1].string, refnames->items[i].string)) {
  strbuf_addf(err,
-    "Multiple updates for ref '%s' not allowed.",
+    "multiple updates for ref '%s' not allowed.",
     refnames->items[i].string);
  return 1;
  }
@@ -3142,7 +3142,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
  * Close it to free up the file descriptor:
  */
  if (close_ref(update->lock)) {
- strbuf_addf(err, "Couldn't close %s.lock",
+ strbuf_addf(err, "couldn't close '%s.lock'",
     update->refname);
  goto cleanup;
  }
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index af1b20d..40b0cce 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -479,7 +479,7 @@ test_expect_success 'stdin fails with duplicate refs' '
  create $a $m
  EOF
  test_must_fail git update-ref --stdin <stdin 2>err &&
- grep "fatal: Multiple updates for ref '"'"'$a'"'"' not allowed." err
+ grep "fatal: multiple updates for ref '"'"'$a'"'"' not allowed." err
 '
 
 test_expect_success 'stdin create ref works' '
@@ -880,7 +880,7 @@ test_expect_success 'stdin -z fails option with unknown name' '
 test_expect_success 'stdin -z fails with duplicate refs' '
  printf $F "create $a" "$m" "create $b" "$m" "create $a" "$m" >stdin &&
  test_must_fail git update-ref -z --stdin <stdin 2>err &&
- grep "fatal: Multiple updates for ref '"'"'$a'"'"' not allowed." err
+ grep "fatal: multiple updates for ref '"'"'$a'"'"' not allowed." err
 '
 
 test_expect_success 'stdin -z create ref works' '
--
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 15/29] ref_transaction_create(): disallow recursive pruning

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
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.

 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);
--
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 16/29] ref_transaction_commit(): correctly report close_ref() failure

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
Signed-off-by: Michael Haggerty <[hidden email]>
---
close() rarely fails, but it is possible.

 refs/files-backend.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8fcbd7d..e86e3de 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3144,6 +3144,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 17/29] 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 18/29] 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 e86e3de..91416db 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2688,7 +2688,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;
@@ -3106,7 +3106,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));
 
@@ -3138,8 +3139,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'",
@@ -3154,7 +3156,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)) {
@@ -3173,7 +3176,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 37a1a37..6b53ba1 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 19/29] 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 | 13 ++++++++-----
 t/t3200-branch.sh    |  9 +++++++++
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 91416db..e4cdd5a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2358,11 +2358,12 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
  struct stat loginfo;
  int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
  struct strbuf err = STRBUF_INIT;
+ const int resolve_flags = RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE;
 
  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_flags, orig_sha1, &flag))
  return error("refname %s not found", oldrefname);
 
  if (flag & REF_ISSYMREF)
@@ -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)) {
  if (errno==EISDIR) {
  struct strbuf path = STRBUF_INIT;
  int result;
@@ -2405,7 +2406,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);
@@ -2423,7 +2425,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
1234