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

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

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

Michael Haggerty-2
Thanks to David, Junio, and Peff for their comments on v1 of this
patch series [1]. I think I have addressed all of the points that were
brought up. Plus I fixed a pre-existing bug that I noticed myself
while adding some more tests; see the first bullet point below for
more information.

Changes between v1 and v2:

* Prefixed the patch series with three new patches that demonstrate
  and fix some bugs in reference resolution that I noticed while
  inspecting this series:

  * "t1404: demonstrate a bug resolving references" -- this
    demonstrates a bug that has existed since at least 2.5.0.
  * "read_raw_ref(): don't get confused by an empty directory"
  * "commit_ref(): if there is an empty dir in the way, delete it"

* Added a patch "read_raw_ref(): move docstring to header file".

* "ref_transaction_create(): disallow recursive pruning": Add a
  comment that `REF_ISPRUNING` must only be used with `REF_NODEREF`.

* "refs: don't dereference on rename": explain why we can't pass an
  `old_sha1` argument to `delete_ref()`. Inline `resolve_flags`
  constant to make the code more transparent.

* "add_update(): initialize the whole ref_update": move some more
  checks from `ref_transaction_update()` to `add_update()`.

* "refs: resolve symbolic refs first":
  * Remove unused `deleting` parameter from `lock_raw_ref()`
  * Fix a comment, add another.

* "lock_ref_sha1_basic(): only handle REF_NODEREF mode": initialize
  `ref_name` only once, as its value can be reused.

This patch series is also available from my GitHub repo [2] as branch
"split-under-lock".

[1] http://thread.gmane.org/gmane.comp.version-control.git/292772
[2] https://github.com/mhagger/git

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

Michael Haggerty (31):
  t1404: demonstrate a bug resolving references
  commit_ref(): if there is an empty dir in the way, delete it
  read_raw_ref(): don't get confused by an empty directory
  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
  read_raw_ref(): move docstring to header file
  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                             |  96 ++--
 refs/files-backend.c               | 909 ++++++++++++++++++++++++++++---------
 refs/refs-internal.h               |  95 +++-
 t/t1400-update-ref.sh              |  41 +-
 t/t1404-update-ref-df-conflicts.sh |  76 +++-
 t/t1430-bad-ref-name.sh            |   2 +-
 t/t3200-branch.sh                  |   9 +
 10 files changed, 1013 insertions(+), 262 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 v2 01/33] t1404: demonstrate a bug resolving references

Michael Haggerty-2
Add some tests checking that it is possible to work with a reference
even if there is an empty directory where the loose ref would be stored.

One of the new tests demonstrates a bug that has been with us since at
least 2.5.0--single reference lookup gives up when it sees the
directory, even if the reference exists as a packed ref. This probably
hasn't been reported before because Git usually cleans up empty
directories when packing references.

This bug will be fixed shortly.

Signed-off-by: Michael Haggerty <[hidden email]>
---
 t/t1404-update-ref-df-conflicts.sh | 76 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 75 insertions(+), 1 deletion(-)

diff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh
index 66bafb5..16752a9 100755
--- a/t/t1404-update-ref-df-conflicts.sh
+++ b/t/t1404-update-ref-df-conflicts.sh
@@ -28,7 +28,9 @@ Q="'"
 test_expect_success 'setup' '
 
  git commit --allow-empty -m Initial &&
- C=$(git rev-parse HEAD)
+ C=$(git rev-parse HEAD) &&
+ git commit --allow-empty -m Second &&
+ D=$(git rev-parse HEAD)
 
 '
 
@@ -104,4 +106,76 @@ test_expect_success 'one new ref is a simple prefix of another' '
 
 '
 
+test_expect_failure 'empty directory should not fool rev-parse' '
+ prefix=refs/e-rev-parse &&
+ git update-ref $prefix/foo $C &&
+ git pack-refs --all &&
+ mkdir -p .git/$prefix/foo/bar/baz &&
+ echo "$C" >expected &&
+ git rev-parse $prefix/foo >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'empty directory should not fool for-each-ref' '
+ prefix=refs/e-for-each-ref &&
+ git update-ref $prefix/foo $C &&
+ git for-each-ref $prefix >expected &&
+ git pack-refs --all &&
+ mkdir -p .git/$prefix/foo/bar/baz &&
+ git for-each-ref $prefix >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'empty directory should not fool create' '
+ prefix=refs/e-create &&
+ mkdir -p .git/$prefix/foo/bar/baz &&
+ printf "create %s $C\n" $prefix/foo |
+ git update-ref --stdin
+'
+
+test_expect_success 'empty directory should not fool verify' '
+ prefix=refs/e-verify &&
+ git update-ref $prefix/foo $C &&
+ git pack-refs --all &&
+ mkdir -p .git/$prefix/foo/bar/baz &&
+ printf "verify %s $C\n" $prefix/foo |
+ git update-ref --stdin
+'
+
+test_expect_success 'empty directory should not fool 1-arg update' '
+ prefix=refs/e-update-1 &&
+ git update-ref $prefix/foo $C &&
+ git pack-refs --all &&
+ mkdir -p .git/$prefix/foo/bar/baz &&
+ printf "update %s $D\n" $prefix/foo |
+ git update-ref --stdin
+'
+
+test_expect_success 'empty directory should not fool 2-arg update' '
+ prefix=refs/e-update-2 &&
+ git update-ref $prefix/foo $C &&
+ git pack-refs --all &&
+ mkdir -p .git/$prefix/foo/bar/baz &&
+ printf "update %s $D $C\n" $prefix/foo |
+ git update-ref --stdin
+'
+
+test_expect_success 'empty directory should not fool 0-arg delete' '
+ prefix=refs/e-delete-0 &&
+ git update-ref $prefix/foo $C &&
+ git pack-refs --all &&
+ mkdir -p .git/$prefix/foo/bar/baz &&
+ printf "delete %s\n" $prefix/foo |
+ git update-ref --stdin
+'
+
+test_expect_success 'empty directory should not fool 1-arg delete' '
+ prefix=refs/e-delete-1 &&
+ git update-ref $prefix/foo $C &&
+ git pack-refs --all &&
+ mkdir -p .git/$prefix/foo/bar/baz &&
+ printf "delete %s $C\n" $prefix/foo |
+ git update-ref --stdin
+'
+
 test_done
--
2.8.1

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

[PATCH v2 02/33] commit_ref(): if there is an empty dir in the way, delete it

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
Part of the bug revealed in the last commit is that resolve_ref_unsafe()
incorrectly returns EISDIR if it finds a directory in the place where it
is looking for a loose reference, even if the corresponding packed
reference exists. lock_ref_sha1_basic() notices the bogus EISDIR, and
use it as an indication that it should call remove_empty_directories()
and call resolve_ref_unsafe() again.

But resolve_ref_unsafe() shouldn't report EISDIR in this case. If we
would simply make that change, then remove_empty_directories() wouldn't
get called anymore, and the empty directory would get in the way when
commit_ref() calls commit_lock_file() to rename the lockfile into place.

So instead of relying on lock_ref_sha1_basic() to delete empty
directories, teach commit_ref(), just before calling commit_lock_file(),
to check whether a directory is in the way, and if so, try to delete it.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1f38076..ad9cd86 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2457,6 +2457,30 @@ static int close_ref(struct ref_lock *lock)
 
 static int commit_ref(struct ref_lock *lock)
 {
+ char *path = get_locked_file_path(lock->lk);
+ struct stat st;
+
+ if (!lstat(path, &st) && S_ISDIR(st.st_mode)) {
+ /*
+ * There is a directory at the path we want to rename
+ * the lockfile to. Hopefully it is empty; try to
+ * delete it.
+ */
+ size_t len = strlen(path);
+ struct strbuf sb_path = STRBUF_INIT;
+
+ strbuf_attach(&sb_path, path, len, len);
+
+ /*
+ * If this fails, commit_lock_file() will also fail
+ * and will report the problem.
+ */
+ remove_empty_directories(&sb_path);
+ strbuf_release(&sb_path);
+ } else {
+ free(path);
+ }
+
  if (commit_lock_file(lock->lk))
  return -1;
  return 0;
--
2.8.1

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

[PATCH v2 03/33] read_raw_ref(): don't get confused by an empty directory

Michael Haggerty-2
In reply to this post by Michael Haggerty-2
Even if there is an empty directory where we look for the loose version
of a reference, check for a packed reference before giving up. This
fixes the failing test that was introduced two commits ago.

Signed-off-by: Michael Haggerty <[hidden email]>
---
 refs/files-backend.c               | 11 ++++++++++-
 t/t1404-update-ref-df-conflicts.sh |  2 +-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index ad9cd86..0cc116d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1477,7 +1477,16 @@ stat_ref:
 
  /* Is it a directory? */
  if (S_ISDIR(st.st_mode)) {
- errno = EISDIR;
+ /*
+ * Even though there is a directory where the loose
+ * ref is supposed to be, there could still be a
+ * packed ref:
+ */
+ if (resolve_missing_loose_ref(refname, sha1, flags)) {
+ errno = EISDIR;
+ goto out;
+ }
+ ret = 0;
  goto out;
  }
 
diff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh
index 16752a9..6d869d1 100755
--- a/t/t1404-update-ref-df-conflicts.sh
+++ b/t/t1404-update-ref-df-conflicts.sh
@@ -106,7 +106,7 @@ test_expect_success 'one new ref is a simple prefix of another' '
 
 '
 
-test_expect_failure 'empty directory should not fool rev-parse' '
+test_expect_success 'empty directory should not fool rev-parse' '
  prefix=refs/e-rev-parse &&
  git update-ref $prefix/foo $C &&
  git pack-refs --all &&
--
2.8.1

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

[PATCH v2 04/33] safe_create_leading_directories(): improve docstring

Michael Haggerty-2
In reply to this post by 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 v2 05/33] 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 v2 06/33] 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 v2 07/33] 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]>
---
 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 v2 08/33] 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]>
---
 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 v2 09/33] 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 0cc116d..2d3a8c6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2719,7 +2719,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 v2 10/33] 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 2d3a8c6..80d346f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2360,20 +2360,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 v2 11/33] 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 80d346f..93a94af 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3076,7 +3076,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;
@@ -3087,13 +3086,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)) {
@@ -3107,7 +3106,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) &&
@@ -3178,7 +3177,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) {
@@ -3197,7 +3196,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) {
@@ -3223,7 +3222,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);
@@ -3243,7 +3242,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;
 
@@ -3253,7 +3251,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)) {
@@ -3276,7 +3274,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) &&
@@ -3297,7 +3295,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 v2 12/33] 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 | 18 +++++++++---------
 refs/refs-internal.h |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 93a94af..fa8bbd6 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;
  }
@@ -1482,7 +1482,7 @@ stat_ref:
  * ref is supposed to be, there could still be a
  * packed ref:
  */
- if (resolve_missing_loose_ref(refname, sha1, flags)) {
+ if (resolve_missing_loose_ref(refname, sha1, type)) {
  errno = EISDIR;
  goto out;
  }
@@ -1519,7 +1519,7 @@ stat_ref:
 
  strbuf_reset(symref);
  strbuf_addstr(symref, buf);
- *flags |= REF_ISSYMREF;
+ *type |= REF_ISSYMREF;
  ret = 0;
  goto out;
  }
@@ -1530,7 +1530,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 v2 13/33] 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 fa8bbd6..8ced104 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 v2 14/33] 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 8ced104..fbbd48f 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;
@@ -1518,8 +1519,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 v2 15/33] 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 fbbd48f..73717dd 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 v2 16/33] read_raw_ref(): move docstring to header file

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 73717dd..fc0c7c1 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1388,44 +1388,6 @@ static int resolve_missing_loose_ref(const char *refname,
  return -1;
 }
 
-/*
- * Read the specified reference from the filesystem or packed refs
- * file, non-recursively. Set type to describe the reference, and:
- *
- * - If refname is the name of a normal reference, fill in sha1
- *   (leaving referent unchanged).
- *
- * - 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.
- *
- * 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.
- *
- * 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.
- *
- * It is OK for refname to point into referent. If so:
- *
- * - if the function succeeds with REF_ISSYMREF, referent will be
- *   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.
- */
 int read_raw_ref(const char *refname, unsigned char *sha1,
  struct strbuf *referent, unsigned int *type)
 {
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 37a1a37..de7722e 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -209,6 +209,44 @@ int rename_ref_available(const char *oldname, const char *newname);
 int do_for_each_ref(const char *submodule, const char *base,
     each_ref_fn fn, int trim, int flags, void *cb_data);
 
+/*
+ * Read the specified reference from the filesystem or packed refs
+ * file, non-recursively. Set type to describe the reference, and:
+ *
+ * - If refname is the name of a normal reference, fill in sha1
+ *   (leaving referent unchanged).
+ *
+ * - 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.
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * It is OK for refname to point into referent. If so:
+ *
+ * - if the function succeeds with REF_ISSYMREF, referent will be
+ *   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.
+ */
 int read_raw_ref(const char *refname, unsigned char *sha1,
  struct strbuf *referent, unsigned int *type);
 
--
2.8.1

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

[PATCH v2 17/33] 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 fc0c7c1..97377c7 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1738,7 +1738,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;
@@ -1746,7 +1746,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;
@@ -1766,7 +1765,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
@@ -1784,10 +1783,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 v2 18/33] 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]>
---
 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 97377c7..5a597bb 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1701,7 +1701,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 {
@@ -1710,7 +1710,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));
@@ -1790,7 +1790,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;
@@ -1828,7 +1828,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;
  }
@@ -2473,7 +2473,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;
  }
@@ -2487,7 +2487,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;
  }
@@ -2495,7 +2495,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;
  }
@@ -2564,13 +2564,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;
  }
@@ -2611,14 +2611,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;
@@ -2628,7 +2628,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;
  }
@@ -2649,7 +2649,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);
@@ -2684,7 +2684,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;
  }
@@ -3033,7 +3033,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;
  }
@@ -3137,7 +3137,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 v2 19/33] 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]>
---
 refs.c               | 3 +++
 refs/files-backend.c | 2 +-
 refs/refs-internal.h | 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

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 5a597bb..7cc680a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2087,7 +2087,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);
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index de7722e..1f94f7a 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -15,7 +15,7 @@
 
 /*
  * Used as a flag in ref_update::flags when a loose ref is being
- * pruned.
+ * pruned. This flag must only be used when REF_NODEREF is set.
  */
 #define REF_ISPRUNING 0x04
 
--
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
123