{"thread":{"id":"37143","subject":"[PATCH 03/12] refs.c: add an err argument to delete_ref_loose","startedAt":"2014-07-16T22:23:00Z","lastAt":"2014-07-22T21:46:28Z","messageCount":26,"participants":["Ronnie Sahlberg","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":12},"messages":[{"id":"246195","messageId":"1405549392-27306-1-git-send-email-sahlberg@google.com","threadId":"37143","inReplyTo":null,"subject":"[PATCH 00/12] Use ref transactions part 3","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-16T22:23:00Z","receivedAt":"2014-07-16T22:23:00Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"This is the third and final part of the original 48 patch series for\nbasic transaction support.\n\nIt is used ontop of the previous two series :\n* rs/ref-transaction-0 (2014-07-14) 19 commits\n* rs/ref-transaction-1 (2014-07-16) 20 commits\n\nThis version implements some changes suggested by mhagger for the\nwarn_if_removable changes.\nIt also adds a new patch \"fix handling of badly named refs\" that repairs\nthe handling of badly named refs.\n\n\nRonnie Sahlberg (12):\n wrapper.c: simplify warn_if_unremovable\n wrapper.c: add a new function unlink_or_msg\n refs.c: add an err argument to delete_ref_loose\n refs.c: pass the ref log message to _create/delete/update instead of\n _commit\n refs.c: pass NULL as *flags to read_ref_full\n refs.c: move the check for valid refname to lock_ref_sha1_basic\n refs.c: call lock_ref_sha1_basic directly from commit\n refs.c: pass a skip list to name_conflict_fn\n refs.c: propagate any errno==ENOTDIR from _commit back to the callers\n fetch.c: change s_update_ref to use a ref transaction\n refs.c: make write_ref_sha1 static\n refs.c: fix handling of badly named refs\n\n branch.c | 4 +-\n builtin/blame.c | 2 +-\n builtin/branch.c | 6 +-\n builtin/clone.c | 2 +-\n builtin/commit.c | 4 +-\n builtin/fetch.c | 36 ++++---\n builtin/fmt-merge-msg.c | 2 +-\n builtin/for-each-ref.c | 6 +-\n builtin/log.c | 3 +-\n builtin/receive-pack.c | 5 +-\n builtin/remote.c | 5 +-\n builtin/replace.c | 4 +-\n builtin/show-branch.c | 6 +-\n builtin/tag.c | 4 +-\n builtin/update-ref.c | 13 +--\n bundle.c | 2 +-\n cache.h | 18 ++--\n fast-import.c | 8 +-\n git-compat-util.h | 6 ++\n http-backend.c | 3 +-\n reflog-walk.c | 3 +-\n refs.c | 247 +++++++++++++++++++++++++++++++-----------------\n refs.h | 17 ++--\n remote.c | 6 +-\n sequencer.c | 6 +-\n transport-helper.c | 2 +-\n transport.c | 5 +-\n walker.c | 5 +-\n wrapper.c | 30 ++++--\n 29 files changed, 291 insertions(+), 169 deletions(-)\n\n-- \n2.0.1.527.gc6b782e\n"},{"id":"246199","messageId":"1405549392-27306-2-git-send-email-sahlberg@google.com","threadId":"37143","inReplyTo":"1405549392-27306-1-git-send-email-sahlberg@google.com","subject":"[PATCH 01/12] wrapper.c: simplify warn_if_unremovable","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-16T22:23:01Z","receivedAt":"2014-07-16T22:23:01Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Signed-off-by: Ronnie Sahlberg \n---\n wrapper.c | 14 ++++++--------\n 1 file changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex bc1bfb8..740e193 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -429,14 +429,12 @@ int xmkstemp_mode(char *template, int mode)\n \n static int warn_if_unremovable(const char *op, const char *file, int rc)\n {\n-\tif (rc < 0) {\n-\t\tint err = errno;\n-\t\tif (ENOENT != err) {\n-\t\t\twarning(\"unable to %s %s: %s\",\n-\t\t\t\top, file, strerror(errno));\n-\t\t\terrno = err;\n-\t\t}\n-\t}\n+\tint err;\n+\tif (rc >= 0 || errno == ENOENT)\n+\t\treturn rc;\n+\terr = errno;\n+\twarning(\"unable to %s %s: %s\", op, file, strerror(errno));\n+\terrno = err;\n \treturn rc;\n }\n \n-- \n2.0.1.527.gc6b782e\n"},{"id":"246204","messageId":"1405549392-27306-3-git-send-email-sahlberg@google.com","threadId":"37143","inReplyTo":"1405549392-27306-1-git-send-email-sahlberg@google.com","subject":"[PATCH 02/12] wrapper.c: add a new function unlink_or_msg","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-16T22:23:02Z","receivedAt":"2014-07-16T22:23:02Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Signed-off-by: Ronnie Sahlberg \n---\n git-compat-util.h | 6 ++++++\n wrapper.c | 18 ++++++++++++++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex b6f03b3..426bc98 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -704,12 +704,18 @@ void git_qsort(void *base, size_t nmemb, size_t size,\n #endif\n #endif\n \n+#include \"strbuf.h\"\n+\n /*\n * Preserves errno, prints a message, but gives no warning for ENOENT.\n * Always returns the return value of unlink(2).\n */\n int unlink_or_warn(const char *path);\n /*\n+ * Like unlink_or_warn but populates a strbuf\n+ */\n+int unlink_or_msg(const char *file, struct strbuf *err);\n+/*\n * Likewise for rmdir(2).\n */\n int rmdir_or_warn(const char *path);\ndiff --git a/wrapper.c b/wrapper.c\nindex 740e193..74a0cc0 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -438,6 +438,24 @@ static int warn_if_unremovable(const char *op, const char *file, int rc)\n \treturn rc;\n }\n \n+int unlink_or_msg(const char *file, struct strbuf *err)\n+{\n+\tif (err) {\n+\t\tint rc = unlink(file);\n+\t\tint save_errno = errno;\n+\n+\t\tif (rc < 0 && errno != ENOENT) {\n+\t\t\tstrbuf_addf(err, \"unable to unlink %s: %s\",\n+\t\t\t\t file, strerror(errno));\n+\t\t\terrno = save_errno;\n+\t\t\treturn -1;\n+\t\t}\n+\t\treturn 0;\n+\t}\n+\n+\treturn unlink_or_warn(file);\n+}\n+\n int unlink_or_warn(const char *file)\n {\n \treturn warn_if_unremovable(\"unlink\", file, unlink(file));\n-- \n2.0.1.527.gc6b782e\n"},{"id":"246194","messageId":"1405549392-27306-4-git-send-email-sahlberg@google.com","threadId":"37143","inReplyTo":"1405549392-27306-1-git-send-email-sahlberg@google.com","subject":"[PATCH 03/12] refs.c: add an err argument to delete_ref_loose","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-16T22:23:03Z","receivedAt":"2014-07-16T22:23:03Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Add an err argument to delete_loose_ref so that we can pass a descriptive\nerror string back to the caller. Pass the err argument from transaction\ncommit to this function so that transaction users will have a nice error\nstring if the transaction failed due to delete_loose_ref.\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 0017d9c..24f9546 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2544,16 +2544,16 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n \treturn ret;\n }\n \n-static int delete_ref_loose(struct ref_lock *lock, int flag)\n+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)\n {\n \tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n \t\t/* loose */\n-\t\tint err, i = strlen(lock->lk->filename) - 5; /* .lock */\n+\t\tint res, i = strlen(lock->lk->filename) - 5; /* .lock */\n \n \t\tlock->lk->filename[i] = 0;\n-\t\terr = unlink_or_warn(lock->lk->filename);\n+\t\tres = unlink_or_msg(lock->lk->filename, err);\n \t\tlock->lk->filename[i] = '.';\n-\t\tif (err && errno != ENOENT)\n+\t\tif (res)\n \t\t\treturn 1;\n \t}\n \treturn 0;\n@@ -3602,7 +3602,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif (update->lock) {\n-\t\t\tret |= delete_ref_loose(update->lock, update->type);\n+\t\t\tret |= delete_ref_loose(update->lock, update->type,\n+\t\t\t\t\t\terr);\n \t\t\tif (!(update->flags & REF_ISPRUNING))\n \t\t\t\tdelnames[delnum++] = update->lock->ref_name;\n \t\t}\n-- \n2.0.1.527.gc6b782e\n"},{"id":"246196","messageId":"1405549392-27306-5-git-send-email-sahlberg@google.com","threadId":"37143","inReplyTo":"1405549392-27306-1-git-send-email-sahlberg@google.com","subject":"[PATCH 04/12] refs.c: pass the ref log message to _create/delete/update instead of _commit","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-16T22:23:04Z","receivedAt":"2014-07-16T22:23:04Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change the reference transactions so that we pass the reflog message\nthrough to the create/delete/update function instead of the commit message.\nThis allows for individual messages for each change in a multi ref\ntransaction.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n branch.c | 4 ++--\n builtin/commit.c | 4 ++--\n builtin/fetch.c | 3 +--\n builtin/receive-pack.c | 5 +++--\n builtin/replace.c | 4 ++--\n builtin/tag.c | 4 ++--\n builtin/update-ref.c | 13 +++++++------\n fast-import.c | 8 ++++----\n refs.c | 34 +++++++++++++++++++++-------------\n refs.h | 8 ++++----\n sequencer.c | 4 ++--\n walker.c | 5 ++---\n 12 files changed, 52 insertions(+), 44 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex c1eae00..e0439af 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -301,8 +301,8 @@ void create_branch(const char *head,\n \t\ttransaction = ref_transaction_begin(&err);\n \t\tif (!transaction ||\n \t\t ref_transaction_update(transaction, ref.buf, sha1,\n-\t\t\t\t\t null_sha1, 0, !forcing, &err) ||\n-\t\t ref_transaction_commit(transaction, msg, &err))\n+\t\t\t\t\t null_sha1, 0, !forcing, msg, &err) ||\n+\t\t ref_transaction_commit(transaction, &err))\n \t\t\tdie(\"%s\", err.buf);\n \t\tref_transaction_free(transaction);\n \t}\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 668fa6a..c499826 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1767,8 +1767,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t ref_transaction_update(transaction, \"HEAD\", sha1,\n \t\t\t\t current_head ?\n \t\t\t\t current_head->object.sha1 : NULL,\n-\t\t\t\t 0, !!current_head, &err) ||\n-\t ref_transaction_commit(transaction, sb.buf, &err)) {\n+\t\t\t\t 0, !!current_head, sb.buf, &err) ||\n+\t ref_transaction_commit(transaction, &err)) {\n \t\trollback_index_files();\n \t\tdie(\"%s\", err.buf);\n \t}\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex dd46b61..92fad2d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -702,10 +702,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t}\n \t\t}\n \t}\n-\n \tif (rc & STORE_REF_ERROR_DF_CONFLICT)\n \t\terror(_(\"some local refs could not be updated; try running\\n\"\n-\t\t \" 'git remote prune %s' to remove any old, conflicting \"\n+\t\t \"'git remote prune %s' to remove any old, conflicting \"\n \t\t \"branches\"), remote_name);\n \n abort:\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 91099ad..4752225 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -585,8 +585,9 @@ static char *update(struct command *cmd, struct shallow_info *si)\n \t\ttransaction = ref_transaction_begin(&err);\n \t\tif (!transaction ||\n \t\t ref_transaction_update(transaction, namespaced_name,\n-\t\t\t\t\t new_sha1, old_sha1, 0, 1, &err) ||\n-\t\t ref_transaction_commit(transaction, \"push\", &err)) {\n+\t\t\t\t\t new_sha1, old_sha1, 0, 1, \"push\",\n+\t\t\t\t\t &err) ||\n+\t\t ref_transaction_commit(transaction, &err)) {\n \t\t\tchar *str = strbuf_detach(&err, NULL);\n \t\t\tref_transaction_free(transaction);\n \ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 7528f3d..df060f8 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -170,8 +170,8 @@ static int replace_object_sha1(const char *object_ref,\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t ref_transaction_update(transaction, ref, repl, prev,\n-\t\t\t\t 0, !is_null_sha1(prev), &err) ||\n-\t ref_transaction_commit(transaction, NULL, &err))\n+\t\t\t\t 0, !is_null_sha1(prev), NULL, &err) ||\n+\t ref_transaction_commit(transaction, &err))\n \t\tdie(\"%s\", err.buf);\n \n \tref_transaction_free(transaction);\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 1aa88a2..3834b06 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -705,8 +705,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t ref_transaction_update(transaction, ref.buf, object, prev,\n-\t\t\t\t 0, 1, &err) ||\n-\t ref_transaction_commit(transaction, NULL, &err))\n+\t\t\t\t 0, 1, NULL, &err) ||\n+\t ref_transaction_commit(transaction, &err))\n \t\tdie(\"%s\", err.buf);\n \tref_transaction_free(transaction);\n \tif (force && !is_null_sha1(prev) && hashcmp(prev, object))\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex c6ad0be..28b478a 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -16,6 +16,7 @@ static struct ref_transaction *transaction;\n \n static char line_termination = '\\n';\n static int update_flags;\n+static const char *msg;\n static struct strbuf err = STRBUF_INIT;\n \n /*\n@@ -199,7 +200,7 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)\n \t\tdie(\"update %s: extra input: %s\", refname, next);\n \n \tif (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n-\t\t\t\t update_flags, have_old, &err))\n+\t\t\t\t update_flags, have_old, msg, &err))\n \t\tdie(\"%s\", err.buf);\n \n \tupdate_flags = 0;\n@@ -227,7 +228,7 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)\n \t\tdie(\"create %s: extra input: %s\", refname, next);\n \n \tif (ref_transaction_create(transaction, refname, new_sha1,\n-\t\t\t\t update_flags, &err))\n+\t\t\t\t update_flags, msg, &err))\n \t\tdie(\"%s\", err.buf);\n \n \tupdate_flags = 0;\n@@ -259,7 +260,7 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)\n \t\tdie(\"delete %s: extra input: %s\", refname, next);\n \n \tif (ref_transaction_delete(transaction, refname, old_sha1,\n-\t\t\t\t update_flags, have_old, &err))\n+\t\t\t\t update_flags, have_old, msg, &err))\n \t\tdie(\"%s\", err.buf);\n \n \tupdate_flags = 0;\n@@ -292,7 +293,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n \t\tdie(\"verify %s: extra input: %s\", refname, next);\n \n \tif (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n-\t\t\t\t update_flags, have_old, &err))\n+\t\t\t\t update_flags, have_old, msg, &err))\n \t\tdie(\"%s\", err.buf);\n \n \tupdate_flags = 0;\n@@ -345,7 +346,7 @@ static void update_refs_stdin(void)\n \n int cmd_update_ref(int argc, const char **argv, const char *prefix)\n {\n-\tconst char *refname, *oldval, *msg = NULL;\n+\tconst char *refname, *oldval;\n \tunsigned char sha1[20], oldsha1[20];\n \tint delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;\n \tstruct option options[] = {\n@@ -371,7 +372,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t\tif (end_null)\n \t\t\tline_termination = '\\0';\n \t\tupdate_refs_stdin();\n-\t\tif (ref_transaction_commit(transaction, msg, &err))\n+\t\tif (ref_transaction_commit(transaction, &err))\n \t\t\tdie(\"%s\", err.buf);\n \t\tref_transaction_free(transaction);\n \t\treturn 0;\ndiff --git a/fast-import.c b/fast-import.c\nindex a95e1be..0876db2 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1708,8 +1708,8 @@ static int update_branch(struct branch *b)\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t ref_transaction_update(transaction, b->name, b->sha1, old_sha1,\n-\t\t\t\t 0, 1, &err) ||\n-\t ref_transaction_commit(transaction, msg, &err)) {\n+\t\t\t\t 0, 1, msg, &err) ||\n+\t ref_transaction_commit(transaction, &err)) {\n \t\tref_transaction_free(transaction);\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n@@ -1748,12 +1748,12 @@ static void dump_tags(void)\n \t\tstrbuf_addf(&ref_name, \"refs/tags/%s\", t->name);\n \n \t\tif (ref_transaction_update(transaction, ref_name.buf, t->sha1,\n-\t\t\t\t\t NULL, 0, 0, &err)) {\n+\t\t\t\t\t NULL, 0, 0, msg, &err)) {\n \t\t\tfailure |= error(\"%s\", err.buf);\n \t\t\tgoto cleanup;\n \t\t}\n \t}\n-\tif (ref_transaction_commit(transaction, msg, &err))\n+\tif (ref_transaction_commit(transaction, &err))\n \t\tfailure |= error(\"%s\", err.buf);\n \n cleanup:\ndiff --git a/refs.c b/refs.c\nindex 24f9546..7d65253 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2393,8 +2393,8 @@ static void prune_ref(struct ref_to_prune *r)\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t ref_transaction_delete(transaction, r->name, r->sha1,\n-\t\t\t\t REF_ISPRUNING, 1, &err) ||\n-\t ref_transaction_commit(transaction, NULL, &err)) {\n+\t\t\t\t REF_ISPRUNING, 1, NULL, &err) ||\n+\t ref_transaction_commit(transaction, &err)) {\n \t\tref_transaction_free(transaction);\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n@@ -2567,8 +2567,8 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t ref_transaction_delete(transaction, refname, sha1, delopt,\n-\t\t\t\t sha1 && !is_null_sha1(sha1), &err) ||\n-\t ref_transaction_commit(transaction, NULL, &err)) {\n+\t\t\t\t sha1 && !is_null_sha1(sha1), NULL, &err) ||\n+\t ref_transaction_commit(transaction, &err)) {\n \t\terror(\"%s\", err.buf);\n \t\tref_transaction_free(transaction);\n \t\tstrbuf_release(&err);\n@@ -3345,6 +3345,7 @@ struct ref_update {\n \tint have_old; /* 1 if old_sha1 is valid, 0 otherwise */\n \tstruct ref_lock *lock;\n \tint type;\n+\tchar *msg;\n \tconst char refname[FLEX_ARRAY];\n };\n \n@@ -3391,9 +3392,10 @@ void ref_transaction_free(struct ref_transaction *transaction)\n \tif (!transaction)\n \t\treturn;\n \n-\tfor (i = 0; i < transaction->nr; i++)\n+\tfor (i = 0; i < transaction->nr; i++) {\n+\t\tfree(transaction->updates[i]->msg);\n \t\tfree(transaction->updates[i]);\n-\n+\t}\n \tfree(transaction->updates);\n \tfree(transaction);\n }\n@@ -3414,7 +3416,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t const char *refname,\n \t\t\t const unsigned char *new_sha1,\n \t\t\t const unsigned char *old_sha1,\n-\t\t\t int flags, int have_old,\n+\t\t\t int flags, int have_old, const char *msg,\n \t\t\t struct strbuf *err)\n {\n \tstruct ref_update *update;\n@@ -3431,13 +3433,15 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \tupdate->have_old = have_old;\n \tif (have_old)\n \t\thashcpy(update->old_sha1, old_sha1);\n+\tif (msg)\n+\t\tupdate->msg = xstrdup(msg);\n \treturn 0;\n }\n \n int ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t const char *refname,\n \t\t\t const unsigned char *new_sha1,\n-\t\t\t int flags,\n+\t\t\t int flags, const char *msg,\n \t\t\t struct strbuf *err)\n {\n \tstruct ref_update *update;\n@@ -3454,13 +3458,15 @@ int ref_transaction_create(struct ref_transaction *transaction,\n \thashclr(update->old_sha1);\n \tupdate->flags = flags;\n \tupdate->have_old = 1;\n+\tif (msg)\n+\t\tupdate->msg = xstrdup(msg);\n \treturn 0;\n }\n \n int ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t const char *refname,\n \t\t\t const unsigned char *old_sha1,\n-\t\t\t int flags, int have_old,\n+\t\t\t int flags, int have_old, const char *msg,\n \t\t\t struct strbuf *err)\n {\n \tstruct ref_update *update;\n@@ -3478,6 +3484,8 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n \t\tassert(!is_null_sha1(old_sha1));\n \t\thashcpy(update->old_sha1, old_sha1);\n \t}\n+\tif (msg)\n+\t\tupdate->msg = xstrdup(msg);\n \treturn 0;\n }\n \n@@ -3491,8 +3499,8 @@ int update_ref(const char *action, const char *refname,\n \tt = ref_transaction_begin(&err);\n \tif (!t ||\n \t ref_transaction_update(t, refname, sha1, oldval, flags,\n-\t\t\t\t !!oldval, &err) ||\n-\t ref_transaction_commit(t, action, &err)) {\n+\t\t\t\t !!oldval, action, &err) ||\n+\t ref_transaction_commit(t, &err)) {\n \t\tconst char *str = \"update_ref failed for ref '%s': %s\";\n \n \t\tref_transaction_free(t);\n@@ -3536,7 +3544,7 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n }\n \n int ref_transaction_commit(struct ref_transaction *transaction,\n-\t\t\t const char *msg, struct strbuf *err)\n+\t\t\t struct strbuf *err)\n {\n \tint ret = 0, delnum = 0, i;\n \tconst char **delnames;\n@@ -3585,7 +3593,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t\tif (!is_null_sha1(update->new_sha1)) {\n \t\t\tret = write_ref_sha1(update->lock, update->new_sha1,\n-\t\t\t\t\t msg);\n+\t\t\t\t\t update->msg);\n \t\t\tupdate->lock = NULL; /* freed by write_ref_sha1 */\n \t\t\tif (ret) {\n \t\t\t\tconst char *str = \"Cannot update the ref '%s'.\";\ndiff --git a/refs.h b/refs.h\nindex aad846c..6a4d1a7 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -290,7 +290,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t const char *refname,\n \t\t\t const unsigned char *new_sha1,\n \t\t\t const unsigned char *old_sha1,\n-\t\t\t int flags, int have_old,\n+\t\t\t int flags, int have_old, const char *msg,\n \t\t\t struct strbuf *err);\n \n /*\n@@ -305,7 +305,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n int ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t const char *refname,\n \t\t\t const unsigned char *new_sha1,\n-\t\t\t int flags,\n+\t\t\t int flags, const char *msg,\n \t\t\t struct strbuf *err);\n \n /*\n@@ -319,7 +319,7 @@ int ref_transaction_create(struct ref_transaction *transaction,\n int ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t const char *refname,\n \t\t\t const unsigned char *old_sha1,\n-\t\t\t int flags, int have_old,\n+\t\t\t int flags, int have_old, const char *msg,\n \t\t\t struct strbuf *err);\n \n /*\n@@ -328,7 +328,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n * problem.\n */\n int ref_transaction_commit(struct ref_transaction *transaction,\n-\t\t\t const char *msg, struct strbuf *err);\n+\t\t\t struct strbuf *err);\n \n /*\n * Free an existing transaction and all associated data.\ndiff --git a/sequencer.c b/sequencer.c\nindex cf17c69..284059b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -286,8 +286,8 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \tif (!transaction ||\n \t ref_transaction_update(transaction, \"HEAD\",\n \t\t\t\t to, unborn ? null_sha1 : from,\n-\t\t\t\t 0, 1, &err) ||\n-\t ref_transaction_commit(transaction, sb.buf, &err)) {\n+\t\t\t\t 0, 1, sb.buf, &err) ||\n+\t ref_transaction_commit(transaction, &err)) {\n \t\tref_transaction_free(transaction);\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&sb);\ndiff --git a/walker.c b/walker.c\nindex 60d9f9e..fd9ef87 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -295,15 +295,14 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n \t\tstrbuf_addf(&ref_name, \"refs/%s\", write_ref[i]);\n \t\tif (ref_transaction_update(transaction, ref_name.buf,\n \t\t\t\t\t &sha1[20 * i], NULL, 0, 0,\n+\t\t\t\t\t msg ? msg : \"fetch (unknown)\",\n \t\t\t\t\t &err)) {\n \t\t\terror(\"%s\", err.buf);\n \t\t\tgoto rollback_and_fail;\n \t\t}\n \t}\n \tif (write_ref) {\n-\t\tif (ref_transaction_commit(transaction,\n-\t\t\t\t\t msg ? msg : \"fetch (unknown)\",\n-\t\t\t\t\t &err)) {\n+\t\tif (ref_transaction_commit(transaction, &err)) {\n \t\t\terror(\"%s\", err.buf);\n \t\t\tgoto rollback_and_fail;\n \t\t}\n-- \n2.0.1.527.gc6b782e\n"},{"id":"246198","messageId":"1405549392-27306-6-git-send-email-sahlberg@google.com","threadId":"37143","inReplyTo":"1405549392-27306-1-git-send-email-sahlberg@google.com","subject":"[PATCH 05/12] refs.c: pass NULL as *flags to read_ref_full","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-16T22:23:05Z","receivedAt":"2014-07-16T22:23:05Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"We call read_ref_full with a pointer to flags from rename_ref but since\nwe never actually use the returned flags we can just pass NULL here instead.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 7d65253..0df6894 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2666,7 +2666,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t\tgoto rollback;\n \t}\n \n-\tif (!read_ref_full(newrefname, sha1, 1, &flag) &&\n+\tif (!read_ref_full(newrefname, sha1, 1, NULL) &&\n \t delete_ref(newrefname, sha1, REF_NODEREF)) {\n \t\tif (errno==EISDIR) {\n \t\t\tif (remove_empty_directories(git_path(\"%s\", newrefname))) {\n-- \n2.0.1.527.gc6b782e\n"},{"id":"246202","messageId":"1405549392-27306-7-git-send-email-sahlberg@google.com","threadId":"37143","inReplyTo":"1405549392-27306-1-git-send-email-sahlberg@google.com","subject":"[PATCH 06/12] refs.c: move the check for valid refname to lock_ref_sha1_basic","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-16T22:23:06Z","receivedAt":"2014-07-16T22:23:06Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Move the check for check_refname_format from lock_any_ref_for_update\nto lock_ref_sha1_basic. At some later stage we will get rid of\nlock_any_ref_for_update completely.\n\nIf lock_ref_sha1_basic fails the check_refname_format test, set errno to\nEINVAL before returning NULL. This to guarantee that we will not return an\nerror without updating errno.\n\nThis leaves lock_any_ref_for_updates as a no-op wrapper which could be removed.\nBut this wrapper is also called from an external caller and we will soon\nmake changes to the signature to lock_ref_sha1_basic that we do not want to\nexpose to that caller.\n\nThis changes semantics for lock_ref_sha1_basic slightly. With this change\nit is no longer possible to open a ref that has a badly name which breaks\nany codepaths that tries to open and repair badly named refs. The normal refs\nAPI should not allow neither creating nor accessing refs with invalid names.\nIf we need such recovery code we could add it as an option to git fsck and have\ngit fsck be the only sanctioned way of bypassing the normal API and checks.\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 0df6894..f29f18a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2088,6 +2088,11 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tint missing = 0;\n \tint attempts_remaining = 3;\n \n+\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n+\t\terrno = EINVAL;\n+\t\treturn NULL;\n+\t}\n+\n \tlock = xcalloc(1, sizeof(struct ref_lock));\n \tlock->lock_fd = -1;\n \n@@ -2179,8 +2184,6 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,\n \t\t\t\t\t const unsigned char *old_sha1,\n \t\t\t\t\t int flags, int *type_p)\n {\n-\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))\n-\t\treturn NULL;\n \treturn lock_ref_sha1_basic(refname, old_sha1, flags, type_p);\n }\n \n-- \n2.0.1.527.gc6b782e\n"},{"id":"246197","messageId":"1405549392-27306-8-git-send-email-sahlberg@google.com","threadId":"37143","inReplyTo":"1405549392-27306-1-git-send-email-sahlberg@google.com","subject":"[PATCH 07/12] refs.c: call lock_ref_sha1_basic directly from commit","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-16T22:23:07Z","receivedAt":"2014-07-16T22:23:07Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic\ndirectly from the commit function.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex f29f18a..d3fedbb 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3575,12 +3575,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n-\t\tupdate->lock = lock_any_ref_for_update(update->refname,\n-\t\t\t\t\t\t (update->have_old ?\n-\t\t\t\t\t\t\tupdate->old_sha1 :\n-\t\t\t\t\t\t\tNULL),\n-\t\t\t\t\t\t update->flags,\n-\t\t\t\t\t\t &update->type);\n+\t\tupdate->lock = lock_ref_sha1_basic(update->refname,\n+\t\t\t\t\t\t (update->have_old ?\n+\t\t\t\t\t\t update->old_sha1 :\n+\t\t\t\t\t\t NULL),\n+\t\t\t\t\t\t update->flags,\n+\t\t\t\t\t\t &update->type);\n \t\tif (!update->lock) {\n \t\t\tif (err)\n \t\t\t\tstrbuf_addf(err, \"Cannot lock the ref '%s'.\",\n-- \n2.0.1.527.gc6b782e\n"},{"id":"246201","messageId":"1405549392-27306-9-git-send-email-sahlberg@google.com","threadId":"37143","inReplyTo":"1405549392-27306-1-git-send-email-sahlberg@google.com","subject":"[PATCH 08/12] refs.c: pass a skip list to name_conflict_fn","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-16T22:23:08Z","receivedAt":"2014-07-16T22:23:08Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Allow passing a list of refs to skip checking to name_conflict_fn.\nThere are some conditions where we want to allow a temporary conflict and skip\nchecking those refs. For example if we have a transaction that\n1, guarantees that m is a packed refs and there is no loose ref for m\n2, the transaction will delete m from the packed ref\n3, the transaction will create conflicting m/m\n\nFor this case we want to be able to lock and create m/m since we know that the\nconflict is only transient. I.e. the conflict will be automatically resolved\nby the transaction when it deletes m.\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 41 ++++++++++++++++++++++++++---------------\n 1 file changed, 26 insertions(+), 15 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex d3fedbb..a115478 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -801,15 +801,18 @@ static int names_conflict(const char *refname1, const char *refname2)\n \n struct name_conflict_cb {\n \tconst char *refname;\n-\tconst char *oldrefname;\n \tconst char *conflicting_refname;\n+\tconst char **skip;\n+\tint skipnum;\n };\n \n static int name_conflict_fn(struct ref_entry *entry, void *cb_data)\n {\n \tstruct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;\n-\tif (data->oldrefname && !strcmp(data->oldrefname, entry->name))\n-\t\treturn 0;\n+\tint i;\n+\tfor (i = 0; i < data->skipnum; i++)\n+\t\tif (!strcmp(entry->name, data->skip[i]))\n+\t\t\treturn 0;\n \tif (names_conflict(data->refname, entry->name)) {\n \t\tdata->conflicting_refname = entry->name;\n \t\treturn 1;\n@@ -822,15 +825,18 @@ static int name_conflict_fn(struct ref_entry *entry, void *cb_data)\n * conflicting with the name of an existing reference in dir. If\n * oldrefname is non-NULL, ignore potential conflicts with oldrefname\n * (e.g., because oldrefname is scheduled for deletion in the same\n- * operation).\n+ * operation). skip contains a list of refs we want to skip checking for\n+ * conflicts with.\n */\n-static int is_refname_available(const char *refname, const char *oldrefname,\n-\t\t\t\tstruct ref_dir *dir)\n+static int is_refname_available(const char *refname,\n+\t\t\t\tstruct ref_dir *dir,\n+\t\t\t\tconst char **skip, int skipnum)\n {\n \tstruct name_conflict_cb data;\n \tdata.refname = refname;\n-\tdata.oldrefname = oldrefname;\n \tdata.conflicting_refname = NULL;\n+\tdata.skip = skip;\n+\tdata.skipnum = skipnum;\n \n \tsort_ref_dir(dir);\n \tif (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data)) {\n@@ -2077,7 +2083,8 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n /* This function should make sure errno is meaningful on error */\n static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t\t\t const unsigned char *old_sha1,\n-\t\t\t\t\t int flags, int *type_p)\n+\t\t\t\t\t int flags, int *type_p,\n+\t\t\t\t\t const char **skip, int skipnum)\n {\n \tchar *ref_file;\n \tconst char *orig_refname = refname;\n@@ -2126,7 +2133,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t * name is a proper prefix of our refname.\n \t */\n \tif (missing &&\n-\t !is_refname_available(refname, NULL, get_packed_refs(&ref_cache))) {\n+\t !is_refname_available(refname, get_packed_refs(&ref_cache),\n+\t\t\t\t skip, skipnum)) {\n \t\tlast_errno = ENOTDIR;\n \t\tgoto error_return;\n \t}\n@@ -2184,7 +2192,7 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,\n \t\t\t\t\t const unsigned char *old_sha1,\n \t\t\t\t\t int flags, int *type_p)\n {\n-\treturn lock_ref_sha1_basic(refname, old_sha1, flags, type_p);\n+\treturn lock_ref_sha1_basic(refname, old_sha1, flags, type_p, NULL, 0);\n }\n \n /*\n@@ -2654,10 +2662,12 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tif (!symref)\n \t\treturn error(\"refname %s not found\", oldrefname);\n \n-\tif (!is_refname_available(newrefname, oldrefname, get_packed_refs(&ref_cache)))\n+\tif (!is_refname_available(newrefname, get_packed_refs(&ref_cache),\n+\t\t\t\t &oldrefname, 1))\n \t\treturn 1;\n \n-\tif (!is_refname_available(newrefname, oldrefname, get_loose_refs(&ref_cache)))\n+\tif (!is_refname_available(newrefname, get_loose_refs(&ref_cache),\n+\t\t\t\t &oldrefname, 1))\n \t\treturn 1;\n \n \tif (log && rename(git_path(\"logs/%s\", oldrefname), git_path(TMP_RENAMED_LOG)))\n@@ -2687,7 +2697,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \n \tlogmoved = log;\n \n-\tlock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL);\n+\tlock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL, NULL, 0);\n \tif (!lock) {\n \t\terror(\"unable to lock %s for update\", newrefname);\n \t\tgoto rollback;\n@@ -2702,7 +2712,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \treturn 0;\n \n rollback:\n-\tlock = lock_ref_sha1_basic(oldrefname, NULL, 0, NULL);\n+\tlock = lock_ref_sha1_basic(oldrefname, NULL, 0, NULL, NULL, 0);\n \tif (!lock) {\n \t\terror(\"unable to lock %s for rollback\", oldrefname);\n \t\tgoto rollbacklog;\n@@ -3580,7 +3590,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t\t\t update->old_sha1 :\n \t\t\t\t\t\t NULL),\n \t\t\t\t\t\t update->flags,\n-\t\t\t\t\t\t &update->type);\n+\t\t\t\t\t\t &update->type,\n+\t\t\t\t\t\t delnames, delnum);\n \t\tif (!update->lock) {\n \t\t\tif (err)\n \t\t\t\tstrbuf_addf(err, \"Cannot lock the ref '%s'.\",\n-- \n2.0.1.527.gc6b782e\n"},{"id":"246205","messageId":"1405549392-27306-10-git-send-email-sahlberg@google.com","threadId":"37143","inReplyTo":"1405549392-27306-1-git-send-email-sahlberg@google.com","subject":"[PATCH 09/12] refs.c: propagate any errno==ENOTDIR from _commit back to the callers","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-16T22:23:09Z","receivedAt":"2014-07-16T22:23:09Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"In _commit, ENOTDIR can happen in the call to lock_ref_sha1_basic, either when\nwe lstat the new refname and it returns ENOTDIR or if the name checking\nfunction reports that the same type of conflict happened. In both cases it\nmeans that we can not create the new ref due to a name conflict.\n\nFor these cases, save the errno value and abort and make sure that the caller\ncan see errno==ENOTDIR.\n\nAlso start defining specific return codes for _commit, assign -1 as a generic\nerror and -2 as the error that refers to a name conflict. Callers can (and\nshould) use that return value inspecting errno directly.\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 22 +++++++++++++++-------\n refs.h | 6 ++++++\n 2 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex a115478..69cbca5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3559,7 +3559,7 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n-\tint ret = 0, delnum = 0, i;\n+\tint ret = 0, delnum = 0, i, df_conflict = 0;\n \tconst char **delnames;\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n@@ -3577,9 +3577,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t/* Copy, sort, and reject duplicate refs */\n \tqsort(updates, n, sizeof(*updates), ref_update_compare);\n-\tret = ref_update_reject_duplicates(updates, n, err);\n-\tif (ret)\n+\tif (ref_update_reject_duplicates(updates, n, err)) {\n+\t\tret = -1;\n \t\tgoto cleanup;\n+\t}\n \n \t/* Acquire all locks while verifying old values */\n \tfor (i = 0; i < n; i++) {\n@@ -3593,10 +3594,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t\t\t &update->type,\n \t\t\t\t\t\t delnames, delnum);\n \t\tif (!update->lock) {\n+\t\t\tif (errno == ENOTDIR)\n+\t\t\t\tdf_conflict = 1;\n \t\t\tif (err)\n \t\t\t\tstrbuf_addf(err, \"Cannot lock the ref '%s'.\",\n \t\t\t\t\t update->refname);\n-\t\t\tret = 1;\n+\t\t\tret = -1;\n \t\t\tgoto cleanup;\n \t\t}\n \t}\n@@ -3614,6 +3617,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t\t\t\tif (err)\n \t\t\t\t\tstrbuf_addf(err, str, update->refname);\n+\t\t\t\tret = -1;\n \t\t\t\tgoto cleanup;\n \t\t\t}\n \t\t}\n@@ -3624,14 +3628,16 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif (update->lock) {\n-\t\t\tret |= delete_ref_loose(update->lock, update->type,\n-\t\t\t\t\t\terr);\n+\t\t\tif (delete_ref_loose(update->lock, update->type, err))\n+\t\t\t\tret = -1;\n+\n \t\t\tif (!(update->flags & REF_ISPRUNING))\n \t\t\t\tdelnames[delnum++] = update->lock->ref_name;\n \t\t}\n \t}\n \n-\tret |= repack_without_refs(delnames, delnum, err);\n+\tif (repack_without_refs(delnames, delnum, err))\n+\t\tret = -1;\n \tfor (i = 0; i < delnum; i++)\n \t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n \tclear_loose_ref_cache(&ref_cache);\n@@ -3644,6 +3650,8 @@ cleanup:\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n \tfree(delnames);\n+\tif (df_conflict)\n+\t\tret = -2;\n \treturn ret;\n }\n \ndiff --git a/refs.h b/refs.h\nindex 6a4d1a7..fc7942c 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -326,7 +326,13 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n * Commit all of the changes that have been queued in transaction, as\n * atomically as possible. Return a nonzero value if there is a\n * problem.\n+ * If the transaction is already in failed state this function will return\n+ * an error.\n+ * Function returns 0 on success, -1 for generic failures and\n+ * UPDATE_REFS_NAME_CONFLICT (-2) if the failure was due to a name\n+ * collision (ENOTDIR).\n */\n+#define UPDATE_REFS_NAME_CONFLICT -2\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err);\n \n-- \n2.0.1.527.gc6b782e\n"},{"id":"246200","messageId":"1405549392-27306-11-git-send-email-sahlberg@google.com","threadId":"37143","inReplyTo":"1405549392-27306-1-git-send-email-sahlberg@google.com","subject":"[PATCH 10/12] fetch.c: change s_update_ref to use a ref transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-16T22:23:10Z","receivedAt":"2014-07-16T22:23:10Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change s_update_ref to use a ref transaction for the ref update.\n\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/fetch.c | 33 +++++++++++++++++++++++----------\n 1 file changed, 23 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 92fad2d..383c385 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -404,23 +404,36 @@ static int s_update_ref(const char *action,\n {\n \tchar msg[1024];\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n-\tstatic struct ref_lock *lock;\n+\tstruct ref_transaction *transaction;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tint ret, df_conflict = 0;\n \n \tif (dry_run)\n \t\treturn 0;\n \tif (!rla)\n \t\trla = default_rla.buf;\n \tsnprintf(msg, sizeof(msg), \"%s: %s\", rla, action);\n-\tlock = lock_any_ref_for_update(ref->name,\n-\t\t\t\t check_old ? ref->old_sha1 : NULL,\n-\t\t\t\t 0, NULL);\n-\tif (!lock)\n-\t\treturn errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n-\t\t\t\t\t STORE_REF_ERROR_OTHER;\n-\tif (write_ref_sha1(lock, ref->new_sha1, msg) < 0)\n-\t\treturn errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n-\t\t\t\t\t STORE_REF_ERROR_OTHER;\n+\n+\ttransaction = ref_transaction_begin(&err);\n+\tif (!transaction ||\n+\t ref_transaction_update(transaction, ref->name, ref->new_sha1,\n+\t\t\t\t ref->old_sha1, 0, check_old, msg, &err))\n+\t\tgoto fail;\n+\n+\tret = ref_transaction_commit(transaction, &err);\n+\tif (ret == UPDATE_REFS_NAME_CONFLICT)\n+\t\tdf_conflict = 1;\n+\tif (ret)\n+\t\tgoto fail;\n+\n+\tref_transaction_free(transaction);\n \treturn 0;\n+fail:\n+\tref_transaction_free(transaction);\n+\terror(\"%s\", err.buf);\n+\tstrbuf_release(&err);\n+\treturn df_conflict ? STORE_REF_ERROR_DF_CONFLICT\n+\t\t\t : STORE_REF_ERROR_OTHER;\n }\n \n #define REFCOL_WIDTH 10\n-- \n2.0.1.527.gc6b782e\n"},{"id":"246206","messageId":"1405549392-27306-12-git-send-email-sahlberg@google.com","threadId":"37143","inReplyTo":"1405549392-27306-1-git-send-email-sahlberg@google.com","subject":"[PATCH 11/12] refs.c: make write_ref_sha1 static","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-16T22:23:11Z","receivedAt":"2014-07-16T22:23:11Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"No external users call write_ref_sha1 any more so lets declare it static.\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 10 ++++++++--\n refs.h | 3 ---\n 2 files changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 69cbca5..6c7a9d2 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2643,6 +2643,9 @@ static int rename_tmp_log(const char *newrefname)\n \treturn 0;\n }\n \n+static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,\n+\t\t\t const char *logmsg);\n+\n int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)\n {\n \tunsigned char sha1[20], orig_sha1[20];\n@@ -2892,8 +2895,11 @@ static int is_branch(const char *refname)\n \treturn !strcmp(refname, \"HEAD\") || starts_with(refname, \"refs/heads/\");\n }\n \n-/* This function must return a meaningful errno */\n-int write_ref_sha1(struct ref_lock *lock,\n+/*\n+ * Writes sha1 into the ref specified by the lock. Makes sure that errno\n+ * is sane on error.\n+ */\n+static int write_ref_sha1(struct ref_lock *lock,\n \tconst unsigned char *sha1, const char *logmsg)\n {\n \tstatic char term = '\\n';\ndiff --git a/refs.h b/refs.h\nindex fc7942c..b0476c1 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -196,9 +196,6 @@ extern int commit_ref(struct ref_lock *lock);\n /** Release any lock taken but not written. **/\n extern void unlock_ref(struct ref_lock *lock);\n \n-/** Writes sha1 into the ref specified by the lock. **/\n-extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);\n-\n /*\n * Setup reflog before using. Set errno to something meaningful on failure.\n */\n-- \n2.0.1.527.gc6b782e\n"},{"id":"246203","messageId":"1405549392-27306-13-git-send-email-sahlberg@google.com","threadId":"37143","inReplyTo":"1405549392-27306-1-git-send-email-sahlberg@google.com","subject":"[PATCH 12/12] refs.c: fix handling of badly named refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-16T22:23:12Z","receivedAt":"2014-07-16T22:23:12Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"We currently do not handle badly named refs well :\n$ cp .git/refs/heads/master .git/refs/heads/master.....@\\*@\\\\.\n$ git branch\n fatal: Reference has invalid format: 'refs/heads/master.....@*@\\.'\n$ git branch -D master.....@\\*@\\\\.\n error: branch 'master.....@*@\\.' not found.\n\nBut we can not really recover from a badly named ref with less than\nmanually deleting the .git/refs/heads/ file.\n\nChange resolve_ref_unsafe to take a flags field instead of a 'reading'\nboolean and update all callers that used a non-zero value for reading\nto pass the flag RESOLVE_REF_READING instead.\nAdd another flag RESOLVE_REF_ALLOW_BAD_NAME that will make\nresolve_ref_unsafe skip checking the refname for sanity and use this\nfrom branch.c so that we will be able to call resolve_ref_unsafe on such\nrefs when trying to delete it.\nAdd checks for refname sanity when updating (not deleting) a ref in\nref_transaction_update and in ref_transaction_create to make the transaction\nfail if an attempt is made to create/update a badly named ref.\nSince all ref changes will later go through the transaction layer this means\nwe no longer need to check for and fail for bad refnames in\nlock_ref_sha1_basic.\n\nChange lock_ref_sha1_basic to not fail for bad refnames. Just check the\nrefname, and print an error, and remember that the refname is bad so that\nwe can skip calling verify_lock().\n\nAllow reading refs with bad names from loose refs and packed refs\nbut flag them as broken. This means that the refs will at least show up in\ngit branch even if their name is invalid/broken.\n\nSince we now do the refname checks, when we need to, before the call\nto create_ref_entry we can remove the check_name argument to the function.\n\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/blame.c | 2 +-\n builtin/branch.c | 6 ++-\n builtin/clone.c | 2 +-\n builtin/fmt-merge-msg.c | 2 +-\n builtin/for-each-ref.c | 6 ++-\n builtin/log.c | 3 +-\n builtin/remote.c | 5 +-\n builtin/show-branch.c | 6 ++-\n bundle.c | 2 +-\n cache.h | 18 ++++---\n http-backend.c | 3 +-\n reflog-walk.c | 3 +-\n refs.c | 126 ++++++++++++++++++++++++++++++------------------\n remote.c | 6 ++-\n sequencer.c | 2 +-\n transport-helper.c | 2 +-\n transport.c | 5 +-\n 17 files changed, 123 insertions(+), 76 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 662e3fe..76340e2 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2278,7 +2278,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \tcommit->object.type = OBJ_COMMIT;\n \tparent_tail = &commit->parents;\n \n-\tif (!resolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL))\n+\tif (!resolve_ref_unsafe(\"HEAD\", head_sha1, RESOLVE_REF_READING, NULL))\n \t\tdie(\"no such ref: HEAD\");\n \n \tparent_tail = append_parent(parent_tail, head_sha1);\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 652b1d2..5c95656 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -129,7 +129,8 @@ static int branch_merged(int kind, const char *name,\n \t\t branch->merge[0] &&\n \t\t branch->merge[0]->dst &&\n \t\t (reference_name = reference_name_to_free =\n-\t\t resolve_refdup(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)\n+\t\t resolve_refdup(branch->merge[0]->dst, sha1,\n+\t\t\t\t RESOLVE_REF_READING, NULL)) != NULL)\n \t\t\treference_rev = lookup_commit_reference(sha1);\n \t}\n \tif (!reference_rev)\n@@ -233,7 +234,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tfree(name);\n \n \t\tname = mkpathdup(fmt, bname.buf);\n-\t\ttarget = resolve_ref_unsafe(name, sha1, 0, &flags);\n+\t\ttarget = resolve_ref_unsafe(name, sha1,\n+\t\t\t\t\t RESOLVE_REF_ALLOW_BAD_NAME, &flags);\n \t\tif (!target ||\n \t\t (!(flags & REF_ISSYMREF) && is_null_sha1(sha1))) {\n \t\t\terror(remote_branch\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex b12989d..f7307e6 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -622,7 +622,7 @@ static int checkout(void)\n \tif (option_no_checkout)\n \t\treturn 0;\n \n-\thead = resolve_refdup(\"HEAD\", sha1, 1, NULL);\n+\thead = resolve_refdup(\"HEAD\", sha1, RESOLVE_REF_READING, NULL);\n \tif (!head) {\n \t\twarning(_(\"remote HEAD refers to nonexistent ref, \"\n \t\t\t \"unable to checkout.\\n\"));\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 3906eda..d8ab177 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -602,7 +602,7 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out,\n \n \t/* get current branch */\n \tcurrent_branch = current_branch_to_free =\n-\t\tresolve_refdup(\"HEAD\", head_sha1, 1, NULL);\n+\t\tresolve_refdup(\"HEAD\", head_sha1, RESOLVE_REF_READING, NULL);\n \tif (!current_branch)\n \t\tdie(\"No current branch\");\n \tif (starts_with(current_branch, \"refs/heads/\"))\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 4135980..a5833fd 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -649,7 +649,8 @@ static void populate_value(struct refinfo *ref)\n \n \tif (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {\n \t\tunsigned char unused1[20];\n-\t\tref->symref = resolve_refdup(ref->refname, unused1, 1, NULL);\n+\t\tref->symref = resolve_refdup(ref->refname, unused1,\n+\t\t\t\t\t RESOLVE_REF_READING, NULL);\n \t\tif (!ref->symref)\n \t\t\tref->symref = \"\";\n \t}\n@@ -707,7 +708,8 @@ static void populate_value(struct refinfo *ref)\n \t\t\tconst char *head;\n \t\t\tunsigned char sha1[20];\n \n-\t\t\thead = resolve_ref_unsafe(\"HEAD\", sha1, 1, NULL);\n+\t\t\thead = resolve_ref_unsafe(\"HEAD\", sha1,\n+\t\t\t\t\t\t RESOLVE_REF_READING, NULL);\n \t\t\tif (!strcmp(ref->refname, head))\n \t\t\t\tv->s = \"*\";\n \t\t\telse\ndiff --git a/builtin/log.c b/builtin/log.c\nindex a7ba211..92db809 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1395,7 +1395,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tif (check_head) {\n \t\t\tunsigned char sha1[20];\n \t\t\tconst char *ref;\n-\t\t\tref = resolve_ref_unsafe(\"HEAD\", sha1, 1, NULL);\n+\t\t\tref = resolve_ref_unsafe(\"HEAD\", sha1,\n+\t\t\t\t\t\t RESOLVE_REF_READING, NULL);\n \t\t\tif (ref && starts_with(ref, \"refs/heads/\"))\n \t\t\t\tbranch_name = xstrdup(ref + strlen(\"refs/heads/\"));\n \t\t\telse\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 401feb3..be8ebac 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -568,7 +568,8 @@ static int read_remote_branches(const char *refname,\n \tstrbuf_addf(&buf, \"refs/remotes/%s/\", rename->old);\n \tif (starts_with(refname, buf.buf)) {\n \t\titem = string_list_append(rename->remote_branches, xstrdup(refname));\n-\t\tsymref = resolve_ref_unsafe(refname, orig_sha1, 1, &flag);\n+\t\tsymref = resolve_ref_unsafe(refname, orig_sha1,\n+\t\t\t\t\t RESOLVE_REF_READING, &flag);\n \t\tif (flag & REF_ISSYMREF)\n \t\t\titem->util = xstrdup(symref);\n \t\telse\n@@ -704,7 +705,7 @@ static int mv(int argc, const char **argv)\n \t\tint flag = 0;\n \t\tunsigned char sha1[20];\n \n-\t\tread_ref_full(item->string, sha1, 1, &flag);\n+\t\tread_ref_full(item->string, sha1, RESOLVE_REF_READING, &flag);\n \t\tif (!(flag & REF_ISSYMREF))\n \t\t\tcontinue;\n \t\tif (delete_ref(item->string, NULL, REF_NODEREF))\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex d873172..a9a5eb3 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -727,7 +727,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\tif (ac == 0) {\n \t\t\tstatic const char *fake_av[2];\n \n-\t\t\tfake_av[0] = resolve_refdup(\"HEAD\", sha1, 1, NULL);\n+\t\t\tfake_av[0] = resolve_refdup(\"HEAD\", sha1,\n+\t\t\t\t\t\t RESOLVE_REF_READING, NULL);\n \t\t\tfake_av[1] = NULL;\n \t\t\tav = fake_av;\n \t\t\tac = 1;\n@@ -789,7 +790,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t}\n \t}\n \n-\thead_p = resolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL);\n+\thead_p = resolve_ref_unsafe(\"HEAD\", head_sha1,\n+\t\t\t\t RESOLVE_REF_READING, NULL);\n \tif (head_p) {\n \t\thead_len = strlen(head_p);\n \t\tmemcpy(head, head_p, head_len + 1);\ndiff --git a/bundle.c b/bundle.c\nindex 1222952..8aaf5f8 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -311,7 +311,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \t\t\tcontinue;\n \t\tif (dwim_ref(e->name, strlen(e->name), sha1, &ref) != 1)\n \t\t\tcontinue;\n-\t\tif (read_ref_full(e->name, sha1, 1, &flag))\n+\t\tif (read_ref_full(e->name, sha1, RESOLVE_REF_READING, &flag))\n \t\t\tflag = 0;\n \t\tdisplay_ref = (flag & REF_ISSYMREF) ? e->name : ref;\n \ndiff --git a/cache.h b/cache.h\nindex 4ca4583..aab246d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -948,7 +948,7 @@ extern int get_sha1_hex(const char *hex, unsigned char *sha1);\n \n extern char *sha1_to_hex(const unsigned char *sha1);\t/* static buffer result! */\n extern int read_ref_full(const char *refname, unsigned char *sha1,\n-\t\t\t int reading, int *flags);\n+\t\t\t int flags, int *ref_flag);\n extern int read_ref(const char *refname, unsigned char *sha1);\n \n /*\n@@ -960,17 +960,17 @@ extern int read_ref(const char *refname, unsigned char *sha1);\n * or the input ref.\n *\n * If the reference cannot be resolved to an object, the behavior\n- * depends on the \"reading\" argument:\n+ * depends on the RESOLVE_REF_READING flag:\n *\n- * - If reading is set, return NULL.\n+ * - If RESOLVE_REF_READING is set, return NULL.\n *\n- * - If reading is not set, clear sha1 and return the name of the last\n- * reference name in the chain, which will either be a non-symbolic\n+ * - If RESOLVE_REF_READING is not set, clear sha1 and return the name of\n+ * the last reference name in the chain, which will either be a non-symbolic\n * reference or an undefined reference. If this is a prelude to\n * \"writing\" to the ref, the return value is the name of the ref\n * that will actually be created or changed.\n *\n- * If flag is non-NULL, set the value that it points to the\n+ * If ref_flag is non-NULL, set the value that it points to the\n * combination of REF_ISPACKED (if the reference was found among the\n * packed references) and REF_ISSYMREF (if the initial reference was a\n * symbolic reference).\n@@ -981,8 +981,10 @@ extern int read_ref(const char *refname, unsigned char *sha1);\n *\n * errno is set to something meaningful on error.\n */\n-extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag);\n-extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);\n+#define RESOLVE_REF_READING 0x01\n+#define RESOLVE_REF_ALLOW_BAD_NAME 0x02\n+extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int flags, int *ref_flag);\n+extern char *resolve_refdup(const char *ref, unsigned char *sha1, int flags, int *ref_flag);\n \n extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);\n extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);\ndiff --git a/http-backend.c b/http-backend.c\nindex d2c0a62..059f790 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -417,7 +417,8 @@ static int show_head_ref(const char *refname, const unsigned char *sha1,\n \n \tif (flag & REF_ISSYMREF) {\n \t\tunsigned char unused[20];\n-\t\tconst char *target = resolve_ref_unsafe(refname, unused, 1, NULL);\n+\t\tconst char *target = resolve_ref_unsafe(refname, unused,\n+\t\t\t\t\t\tRESOLVE_REF_READING, NULL);\n \t\tconst char *target_nons = strip_namespace(target);\n \n \t\tstrbuf_addf(buf, \"ref: %s\\n\", target_nons);\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 9ce8b53..d80a42a 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -48,7 +48,8 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)\n \t\tunsigned char sha1[20];\n \t\tconst char *name;\n \t\tvoid *name_to_free;\n-\t\tname = name_to_free = resolve_refdup(ref, sha1, 1, NULL);\n+\t\tname = name_to_free = resolve_refdup(ref, sha1,\n+\t\t\t\t\t\t RESOLVE_REF_READING, NULL);\n \t\tif (name) {\n \t\t\tfor_each_reflog_ent(name, read_one_reflog, reflogs);\n \t\t\tfree(name_to_free);\ndiff --git a/refs.c b/refs.c\nindex 6c7a9d2..6dcb920 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -281,15 +281,11 @@ static struct ref_dir *get_ref_dir(struct ref_entry *entry)\n }\n \n static struct ref_entry *create_ref_entry(const char *refname,\n-\t\t\t\t\t const unsigned char *sha1, int flag,\n-\t\t\t\t\t int check_name)\n+\t\t\t\t\t const unsigned char *sha1, int flag)\n {\n \tint len;\n \tstruct ref_entry *ref;\n \n-\tif (check_name &&\n-\t check_refname_format(refname, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT))\n-\t\tdie(\"Reference has invalid format: '%s'\", refname);\n \tlen = strlen(refname) + 1;\n \tref = xmalloc(sizeof(struct ref_entry) + len);\n \thashcpy(ref->u.value.sha1, sha1);\n@@ -1062,7 +1058,12 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \n \t\trefname = parse_ref_line(refline, sha1);\n \t\tif (refname) {\n-\t\t\tlast = create_ref_entry(refname, sha1, REF_ISPACKED, 1);\n+\t\t\tint flag = REF_ISPACKED;\n+\n+\t\t\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT)) {\n+\t\t\t\tflag |= REF_ISBROKEN;\n+\t\t\t}\n+\t\t\tlast = create_ref_entry(refname, sha1, flag);\n \t\t\tif (peeled == PEELED_FULLY ||\n \t\t\t (peeled == PEELED_TAGS && starts_with(refname, \"refs/tags/\")))\n \t\t\t\tlast->flag |= REF_KNOWS_PEELED;\n@@ -1135,8 +1136,10 @@ void add_packed_ref(const char *refname, const unsigned char *sha1)\n \n \tif (!packed_ref_cache->lock)\n \t\tdie(\"internal error: packed refs not locked\");\n+\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT))\n+\t\tdie(\"Reference has invalid format: '%s'\", refname);\n \tadd_ref(get_packed_ref_dir(packed_ref_cache),\n-\t\tcreate_ref_entry(refname, sha1, REF_ISPACKED, 1));\n+\t\tcreate_ref_entry(refname, sha1, REF_ISPACKED));\n }\n \n /*\n@@ -1194,12 +1197,17 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)\n \t\t\t\t\thashclr(sha1);\n \t\t\t\t\tflag |= REF_ISBROKEN;\n \t\t\t\t}\n-\t\t\t} else if (read_ref_full(refname.buf, sha1, 1, &flag)) {\n+\t\t\t} else if (read_ref_full(refname.buf, sha1,\n+\t\t\t\t\t\t RESOLVE_REF_READING, &flag)) {\n+\t\t\t\thashclr(sha1);\n+\t\t\t\tflag |= REF_ISBROKEN;\n+\t\t\t}\n+\t\t\tif (check_refname_format(refname.buf, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT)) {\n \t\t\t\thashclr(sha1);\n \t\t\t\tflag |= REF_ISBROKEN;\n \t\t\t}\n \t\t\tadd_entry_to_dir(dir,\n-\t\t\t\t\t create_ref_entry(refname.buf, sha1, flag, 1));\n+\t\t\t\t\t create_ref_entry(refname.buf, sha1, flag));\n \t\t}\n \t\tstrbuf_setlen(&refname, dirnamelen);\n \t}\n@@ -1346,21 +1354,21 @@ static const char *handle_missing_loose_ref(const char *refname,\n }\n \n /* This function needs to return a meaningful errno on failure */\n-const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)\n+const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int flags, int *ref_flag)\n {\n \tint depth = MAXDEPTH;\n \tssize_t len;\n \tchar buffer[256];\n \tstatic char refname_buffer[256];\n \n-\tif (flag)\n-\t\t*flag = 0;\n+\tif (ref_flag)\n+\t\t*ref_flag = 0;\n \n-\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n+\tif (!(flags & RESOLVE_REF_ALLOW_BAD_NAME) &&\n+\t check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n \t\terrno = EINVAL;\n \t\treturn NULL;\n \t}\n-\n \tfor (;;) {\n \t\tchar path[PATH_MAX];\n \t\tstruct stat st;\n@@ -1387,7 +1395,8 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\tif (lstat(path, &st) < 0) {\n \t\t\tif (errno == ENOENT)\n \t\t\t\treturn handle_missing_loose_ref(refname, sha1,\n-\t\t\t\t\t\t\t\treading, flag);\n+\t\t\t\t\t\tflags & RESOLVE_REF_READING,\n+\t\t\t\t\t\tref_flag);\n \t\t\telse\n \t\t\t\treturn NULL;\n \t\t}\n@@ -1407,8 +1416,8 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t\t\t\t!check_refname_format(buffer, 0)) {\n \t\t\t\tstrcpy(refname_buffer, buffer);\n \t\t\t\trefname = refname_buffer;\n-\t\t\t\tif (flag)\n-\t\t\t\t\t*flag |= REF_ISSYMREF;\n+\t\t\t\tif (ref_flag)\n+\t\t\t\t\t*ref_flag |= REF_ISSYMREF;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n@@ -1453,21 +1462,21 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t\t */\n \t\t\tif (get_sha1_hex(buffer, sha1) ||\n \t\t\t (buffer[40] != '\\0' && !isspace(buffer[40]))) {\n-\t\t\t\tif (flag)\n-\t\t\t\t\t*flag |= REF_ISBROKEN;\n+\t\t\t\tif (ref_flag)\n+\t\t\t\t\t*ref_flag |= REF_ISBROKEN;\n \t\t\t\terrno = EINVAL;\n \t\t\t\treturn NULL;\n \t\t\t}\n \t\t\treturn refname;\n \t\t}\n-\t\tif (flag)\n-\t\t\t*flag |= REF_ISSYMREF;\n+\t\tif (ref_flag)\n+\t\t\t*ref_flag |= REF_ISSYMREF;\n \t\tbuf = buffer + 4;\n \t\twhile (isspace(*buf))\n \t\t\tbuf++;\n \t\tif (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {\n-\t\t\tif (flag)\n-\t\t\t\t*flag |= REF_ISBROKEN;\n+\t\t\tif (ref_flag)\n+\t\t\t\t*ref_flag |= REF_ISBROKEN;\n \t\t\terrno = EINVAL;\n \t\t\treturn NULL;\n \t\t}\n@@ -1475,9 +1484,9 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t}\n }\n \n-char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag)\n+char *resolve_refdup(const char *ref, unsigned char *sha1, int flags, int *ref_flag)\n {\n-\tconst char *ret = resolve_ref_unsafe(ref, sha1, reading, flag);\n+\tconst char *ret = resolve_ref_unsafe(ref, sha1, flags, ref_flag);\n \treturn ret ? xstrdup(ret) : NULL;\n }\n \n@@ -1488,22 +1497,22 @@ struct ref_filter {\n \tvoid *cb_data;\n };\n \n-int read_ref_full(const char *refname, unsigned char *sha1, int reading, int *flags)\n+int read_ref_full(const char *refname, unsigned char *sha1, int flags, int *ref_flag)\n {\n-\tif (resolve_ref_unsafe(refname, sha1, reading, flags))\n+\tif (resolve_ref_unsafe(refname, sha1, flags, ref_flag))\n \t\treturn 0;\n \treturn -1;\n }\n \n int read_ref(const char *refname, unsigned char *sha1)\n {\n-\treturn read_ref_full(refname, sha1, 1, NULL);\n+\treturn read_ref_full(refname, sha1, RESOLVE_REF_READING, NULL);\n }\n \n int ref_exists(const char *refname)\n {\n \tunsigned char sha1[20];\n-\treturn !!resolve_ref_unsafe(refname, sha1, 1, NULL);\n+\treturn !!resolve_ref_unsafe(refname, sha1, RESOLVE_REF_READING, NULL);\n }\n \n static int filter_refs(const char *refname, const unsigned char *sha1, int flags,\n@@ -1617,7 +1626,7 @@ int peel_ref(const char *refname, unsigned char *sha1)\n \t\treturn 0;\n \t}\n \n-\tif (read_ref_full(refname, base, 1, &flag))\n+\tif (read_ref_full(refname, base, RESOLVE_REF_READING, &flag))\n \t\treturn -1;\n \n \t/*\n@@ -1783,7 +1792,7 @@ static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)\n \t\treturn 0;\n \t}\n \n-\tif (!read_ref_full(\"HEAD\", sha1, 1, &flag))\n+\tif (!read_ref_full(\"HEAD\", sha1, RESOLVE_REF_READING, &flag))\n \t\treturn fn(\"HEAD\", sha1, flag, cb_data);\n \n \treturn 0;\n@@ -1863,7 +1872,7 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)\n \tint flag;\n \n \tstrbuf_addf(&buf, \"%sHEAD\", get_git_namespace());\n-\tif (!read_ref_full(buf.buf, sha1, 1, &flag))\n+\tif (!read_ref_full(buf.buf, sha1, RESOLVE_REF_READING, &flag))\n \t\tret = fn(buf.buf, sha1, flag, cb_data);\n \tstrbuf_release(&buf);\n \n@@ -1958,7 +1967,8 @@ int refname_match(const char *abbrev_name, const char *full_name)\n static struct ref_lock *verify_lock(struct ref_lock *lock,\n \tconst unsigned char *old_sha1, int mustexist)\n {\n-\tif (read_ref_full(lock->ref_name, lock->old_sha1, mustexist, NULL)) {\n+\tif (read_ref_full(lock->ref_name, lock->old_sha1,\n+\t\t\t mustexist ? RESOLVE_REF_READING : 0, NULL)) {\n \t\tint save_errno = errno;\n \t\terror(\"Can't verify ref %s\", lock->ref_name);\n \t\tunlock_ref(lock);\n@@ -2031,7 +2041,8 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n \n \t\tthis_result = refs_found ? sha1_from_ref : sha1;\n \t\tmksnpath(fullref, sizeof(fullref), *p, len, str);\n-\t\tr = resolve_ref_unsafe(fullref, this_result, 1, &flag);\n+\t\tr = resolve_ref_unsafe(fullref, this_result,\n+\t\t\t\t RESOLVE_REF_READING, &flag);\n \t\tif (r) {\n \t\t\tif (!refs_found++)\n \t\t\t\t*ref = xstrdup(r);\n@@ -2060,7 +2071,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \t\tconst char *ref, *it;\n \n \t\tmksnpath(path, sizeof(path), *p, len, str);\n-\t\tref = resolve_ref_unsafe(path, hash, 1, NULL);\n+\t\tref = resolve_ref_unsafe(path, hash, RESOLVE_REF_READING, NULL);\n \t\tif (!ref)\n \t\t\tcontinue;\n \t\tif (reflog_exists(path))\n@@ -2092,18 +2103,22 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tint last_errno = 0;\n \tint type, lflags;\n \tint mustexist = (old_sha1 && !is_null_sha1(old_sha1));\n+\tint resolve_flags;\n \tint missing = 0;\n \tint attempts_remaining = 3;\n-\n-\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n-\t\terrno = EINVAL;\n-\t\treturn NULL;\n-\t}\n+\tint bad_refname;\n \n \tlock = xcalloc(1, sizeof(struct ref_lock));\n \tlock->lock_fd = -1;\n \n-\trefname = resolve_ref_unsafe(refname, lock->old_sha1, mustexist, &type);\n+\tbad_refname = check_refname_format(refname, REFNAME_ALLOW_ONELEVEL);\n+\n+\tresolve_flags = RESOLVE_REF_ALLOW_BAD_NAME;\n+\tif (mustexist)\n+\t\tresolve_flags |= RESOLVE_REF_READING;\n+\n+\trefname = resolve_ref_unsafe(refname, lock->old_sha1, resolve_flags,\n+\t\t\t\t &type);\n \tif (!refname && errno == EISDIR) {\n \t\t/* we are trying to lock foo but we used to\n \t\t * have foo/bar which now does not exist;\n@@ -2116,7 +2131,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\terror(\"there are still refs under '%s'\", orig_refname);\n \t\t\tgoto error_return;\n \t\t}\n-\t\trefname = resolve_ref_unsafe(orig_refname, lock->old_sha1, mustexist, &type);\n+\t\trefname = resolve_ref_unsafe(orig_refname, lock->old_sha1,\n+\t\t\t\t\t resolve_flags, &type);\n \t}\n \tif (type_p)\n \t *type_p = type;\n@@ -2180,6 +2196,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\telse\n \t\t\tunable_to_lock_index_die(ref_file, errno);\n \t}\n+\tif (bad_refname)\n+\t\treturn lock;\n \treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n \n error_return:\n@@ -2343,8 +2361,9 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)\n \t\tpacked_entry->flag = REF_ISPACKED | REF_KNOWS_PEELED;\n \t\thashcpy(packed_entry->u.value.sha1, entry->u.value.sha1);\n \t} else {\n-\t\tpacked_entry = create_ref_entry(entry->name, entry->u.value.sha1,\n-\t\t\t\t\t\tREF_ISPACKED | REF_KNOWS_PEELED, 0);\n+\t\tpacked_entry = create_ref_entry(entry->name,\n+\t\t\t\t\tentry->u.value.sha1,\n+\t\t\t\t\tREF_ISPACKED | REF_KNOWS_PEELED);\n \t\tadd_ref(cb->packed_refs, packed_entry);\n \t}\n \thashcpy(packed_entry->u.value.peeled, entry->u.value.peeled);\n@@ -2658,7 +2677,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tif (log && S_ISLNK(loginfo.st_mode))\n \t\treturn error(\"reflog for %s is a symlink\", oldrefname);\n \n-\tsymref = resolve_ref_unsafe(oldrefname, orig_sha1, 1, &flag);\n+\tsymref = resolve_ref_unsafe(oldrefname, orig_sha1,\n+\t\t\t\t RESOLVE_REF_READING, &flag);\n \tif (flag & REF_ISSYMREF)\n \t\treturn error(\"refname %s is a symbolic ref, renaming it is not supported\",\n \t\t\toldrefname);\n@@ -2682,7 +2702,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t\tgoto rollback;\n \t}\n \n-\tif (!read_ref_full(newrefname, sha1, 1, NULL) &&\n+\tif (!read_ref_full(newrefname, sha1, RESOLVE_REF_READING, NULL) &&\n \t delete_ref(newrefname, sha1, REF_NODEREF)) {\n \t\tif (errno==EISDIR) {\n \t\t\tif (remove_empty_directories(git_path(\"%s\", newrefname))) {\n@@ -2960,7 +2980,8 @@ static int write_ref_sha1(struct ref_lock *lock,\n \t\tunsigned char head_sha1[20];\n \t\tint head_flag;\n \t\tconst char *head_ref;\n-\t\thead_ref = resolve_ref_unsafe(\"HEAD\", head_sha1, 1, &head_flag);\n+\t\thead_ref = resolve_ref_unsafe(\"HEAD\", head_sha1,\n+\t\t\t\t\t RESOLVE_REF_READING, &head_flag);\n \t\tif (head_ref && (head_flag & REF_ISSYMREF) &&\n \t\t !strcmp(head_ref, lock->ref_name))\n \t\t\tlog_ref_write(\"HEAD\", lock->old_sha1, sha1, logmsg);\n@@ -3446,6 +3467,12 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \tif (have_old && !old_sha1)\n \t\tdie(\"BUG: have_old is true but old_sha1 is NULL\");\n \n+\tif (!is_null_sha1(new_sha1) &&\n+\t check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n+\t\tstrbuf_addf(err, \"Bad refname: %s\", refname);\n+\t\treturn -1;\n+\t}\n+\n \tupdate = add_update(transaction, refname);\n \thashcpy(update->new_sha1, new_sha1);\n \tupdate->flags = flags;\n@@ -3471,6 +3498,11 @@ int ref_transaction_create(struct ref_transaction *transaction,\n \tif (!new_sha1 || is_null_sha1(new_sha1))\n \t\tdie(\"BUG: create ref with null new_sha1\");\n \n+\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n+\t\tstrbuf_addf(err, \"Bad refname: %s\", refname);\n+\t\treturn -1;\n+\t}\n+\n \tupdate = add_update(transaction, refname);\n \n \thashcpy(update->new_sha1, new_sha1);\ndiff --git a/remote.c b/remote.c\nindex ae04043..de84ac3 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1121,7 +1121,8 @@ static char *guess_ref(const char *name, struct ref *peer)\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned char sha1[20];\n \n-\tconst char *r = resolve_ref_unsafe(peer->name, sha1, 1, NULL);\n+\tconst char *r = resolve_ref_unsafe(peer->name, sha1,\n+\t\t\t\t\t RESOLVE_REF_READING, NULL);\n \tif (!r)\n \t\treturn NULL;\n \n@@ -1182,7 +1183,8 @@ static int match_explicit(struct ref *src, struct ref *dst,\n \t\tunsigned char sha1[20];\n \t\tint flag;\n \n-\t\tdst_value = resolve_ref_unsafe(matched_src->name, sha1, 1, &flag);\n+\t\tdst_value = resolve_ref_unsafe(matched_src->name, sha1,\n+\t\t\t\t\t RESOLVE_REF_READING, &flag);\n \t\tif (!dst_value ||\n \t\t ((flag & REF_ISSYMREF) &&\n \t\t !starts_with(dst_value, \"refs/heads/\")))\ndiff --git a/sequencer.c b/sequencer.c\nindex 284059b..e1419bf 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -365,7 +365,7 @@ static int is_index_unchanged(void)\n \tunsigned char head_sha1[20];\n \tstruct commit *head_commit;\n \n-\tif (!resolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL))\n+\tif (!resolve_ref_unsafe(\"HEAD\", head_sha1, RESOLVE_REF_READING, NULL))\n \t\treturn error(_(\"Could not resolve HEAD commit\\n\"));\n \n \thead_commit = lookup_commit(head_sha1);\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 84c616f..270ae28 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -889,7 +889,7 @@ static int push_refs_with_export(struct transport *transport,\n \t\t\t\t\tint flag;\n \n \t\t\t\t\t/* Follow symbolic refs (mainly for HEAD). */\n-\t\t\t\t\tname = resolve_ref_unsafe(ref->peer_ref->name, sha1, 1, &flag);\n+\t\t\t\t\tname = resolve_ref_unsafe(ref->peer_ref->name, sha1, RESOLVE_REF_READING, &flag);\n \t\t\t\t\tif (!name || !(flag & REF_ISSYMREF))\n \t\t\t\t\t\tname = ref->peer_ref->name;\n \ndiff --git a/transport.c b/transport.c\nindex 325f03e..f40e950 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -168,7 +168,8 @@ static void set_upstreams(struct transport *transport, struct ref *refs,\n \t\t/* Follow symbolic refs (mainly for HEAD). */\n \t\tlocalname = ref->peer_ref->name;\n \t\tremotename = ref->name;\n-\t\ttmp = resolve_ref_unsafe(localname, sha, 1, &flag);\n+\t\ttmp = resolve_ref_unsafe(localname, sha,\n+\t\t\t\t\t RESOLVE_REF_READING, &flag);\n \t\tif (tmp && flag & REF_ISSYMREF &&\n \t\t\tstarts_with(tmp, \"refs/heads/\"))\n \t\t\tlocalname = tmp;\n@@ -753,7 +754,7 @@ void transport_print_push_status(const char *dest, struct ref *refs,\n \tunsigned char head_sha1[20];\n \tchar *head;\n \n-\thead = resolve_refdup(\"HEAD\", head_sha1, 1, NULL);\n+\thead = resolve_refdup(\"HEAD\", head_sha1, RESOLVE_REF_READING, NULL);\n \n \tif (verbose) {\n \t\tfor (ref = refs; ref; ref = ref->next)\n-- \n2.0.1.527.gc6b782e\n"},{"id":"246344","messageId":"xmqqlhrql345.fsf@gitster.dls.corp.google.com","threadId":"37143","inReplyTo":"1405549392-27306-2-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 01/12] wrapper.c: simplify warn_if_unremovable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-18T22:21:46Z","receivedAt":"2014-07-18T22:21:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg writes:\n\n> Signed-off-by: Ronnie Sahlberg \n> ---\n> wrapper.c | 14 ++++++--------\n> 1 file changed, 6 insertions(+), 8 deletions(-)\n>\n> diff --git a/wrapper.c b/wrapper.c\n> index bc1bfb8..740e193 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -429,14 +429,12 @@ int xmkstemp_mode(char *template, int mode)\n> \n> static int warn_if_unremovable(const char *op, const char *file, int rc)\n> {\n> -\tif (rc < 0) {\n> -\t\tint err = errno;\n> -\t\tif (ENOENT != err) {\n> -\t\t\twarning(\"unable to %s %s: %s\",\n> -\t\t\t\top, file, strerror(errno));\n> -\t\t\terrno = err;\n> -\t\t}\n> -\t}\n> +\tint err;\n> +\tif (rc >= 0 || errno == ENOENT)\n> +\t\treturn rc;\n> +\terr = errno;\n> +\twarning(\"unable to %s %s: %s\", op, file, strerror(errno));\n> +\terrno = err;\n> \treturn rc;\n> }\n\nLooks sensible.\n"},{"id":"246345","messageId":"xmqqha2el2x5.fsf@gitster.dls.corp.google.com","threadId":"37143","inReplyTo":"1405549392-27306-3-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 02/12] wrapper.c: add a new function unlink_or_msg","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-18T22:25:58Z","receivedAt":"2014-07-18T22:25:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg writes:\n\n> Signed-off-by: Ronnie Sahlberg \n> ---\n> git-compat-util.h | 6 ++++++\n> wrapper.c | 18 ++++++++++++++++++\n> 2 files changed, 24 insertions(+)\n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index b6f03b3..426bc98 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -704,12 +704,18 @@ void git_qsort(void *base, size_t nmemb, size_t size,\n> #endif\n> #endif\n> \n> +#include \"strbuf.h\"\n> +\n> /*\n> * Preserves errno, prints a message, but gives no warning for ENOENT.\n> * Always returns the return value of unlink(2).\n> */\n> int unlink_or_warn(const char *path);\n> /*\n> + * Like unlink_or_warn but populates a strbuf\n> + */\n> +int unlink_or_msg(const char *file, struct strbuf *err);\n> +/*\n> * Likewise for rmdir(2).\n> */\n> int rmdir_or_warn(const char *path);\n> diff --git a/wrapper.c b/wrapper.c\n> index 740e193..74a0cc0 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -438,6 +438,24 @@ static int warn_if_unremovable(const char *op, const char *file, int rc)\n> \treturn rc;\n> }\n> \n> +int unlink_or_msg(const char *file, struct strbuf *err)\n> +{\n> +\tif (err) {\n> +\t\tint rc = unlink(file);\n> +\t\tint save_errno = errno;\n> +\n> +\t\tif (rc < 0 && errno != ENOENT) {\n> +\t\t\tstrbuf_addf(err, \"unable to unlink %s: %s\",\n> +\t\t\t\t file, strerror(errno));\n> +\t\t\terrno = save_errno;\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t\treturn 0;\n> +\t}\n> +\n> +\treturn unlink_or_warn(file);\n> +}\n\nIn general, I do not generally like to see messages propagated\nupwards from deeper levels of the callchain to the callers to be\nused later, primarily because that will easily make it harder to\nlocalize the message-lego.\n\nFor this partcular one, shouldn't the caller be doing\n\n\tif (unlink(file) && errno != ENOENT) {\n \t... do its own error message ...\n\t}\n\ninstead of calling any of the unlink_or_whatever() helper?\n\n\n> int unlink_or_warn(const char *file)\n> {\n> \treturn warn_if_unremovable(\"unlink\", file, unlink(file));\n"},{"id":"246346","messageId":"xmqqd2d2l2o7.fsf@gitster.dls.corp.google.com","threadId":"37143","inReplyTo":"1405549392-27306-6-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 05/12] refs.c: pass NULL as *flags to read_ref_full","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-18T22:31:20Z","receivedAt":"2014-07-18T22:31:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg writes:\n\n> We call read_ref_full with a pointer to flags from rename_ref but since\n> we never actually use the returned flags we can just pass NULL here instead.\n\nSensible, at least for the current callers. I had to wonder if\nrename_ref() would never want to take advantage of the flags return\nparameter in the future, though. For example, would it want to act\ndifferently when the given ref turns out to be a symref? Would it\nwant to report something when the ref to be overwritten was a broken\none?\n\n> Reviewed-by: Jonathan Nieder \n> Signed-off-by: Ronnie Sahlberg \n> ---\n> refs.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 7d65253..0df6894 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2666,7 +2666,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n> \t\tgoto rollback;\n> \t}\n> \n> -\tif (!read_ref_full(newrefname, sha1, 1, &flag) &&\n> +\tif (!read_ref_full(newrefname, sha1, 1, NULL) &&\n> \t delete_ref(newrefname, sha1, REF_NODEREF)) {\n> \t\tif (errno==EISDIR) {\n> \t\t\tif (remove_empty_directories(git_path(\"%s\", newrefname))) {\n"},{"id":"246347","messageId":"xmqq8unql2eo.fsf@gitster.dls.corp.google.com","threadId":"37143","inReplyTo":"1405549392-27306-7-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 06/12] refs.c: move the check for valid refname to lock_ref_sha1_basic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-18T22:37:03Z","receivedAt":"2014-07-18T22:37:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg writes:\n\n> Move the check for check_refname_format from lock_any_ref_for_update\n> to lock_ref_sha1_basic. At some later stage we will get rid of\n> lock_any_ref_for_update completely.\n>\n> If lock_ref_sha1_basic fails the check_refname_format test, set errno to\n> EINVAL before returning NULL. This to guarantee that we will not return an\n> error without updating errno.\n>\n> This leaves lock_any_ref_for_updates as a no-op wrapper which could be removed.\n> But this wrapper is also called from an external caller and we will soon\n> make changes to the signature to lock_ref_sha1_basic that we do not want to\n> expose to that caller.\n\nThat might be sensible if our only goal were to remove\nlock-any-ref-for-updates, but I wonder what the impact of this\nchange to other existing callers of lock-ref-sha1-basic. I may be\nrecalling things incorrectly, but I suspect that it was deliberate\nto keep the lowest-level internal helper function (i.e. _basic()) to\nbe lenient so that those who do not want the format checks can\nchoose to pass refnames that are not exactly kosher.\n\n> If we need such recovery code we could add it as an option to git fsck and have\n> git fsck be the only sanctioned way of bypassing the normal API and checks.\n\nBut fsck is about checking and never about recovering, isn't it?\nDoes it offer to remove misnamed refs? Should it?\n\n\n\n> Signed-off-by: Ronnie Sahlberg \n> ---\n> refs.c | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 0df6894..f29f18a 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2088,6 +2088,11 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n> \tint missing = 0;\n> \tint attempts_remaining = 3;\n> \n> +\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n> +\t\terrno = EINVAL;\n> +\t\treturn NULL;\n> +\t}\n> +\n> \tlock = xcalloc(1, sizeof(struct ref_lock));\n> \tlock->lock_fd = -1;\n> \n> @@ -2179,8 +2184,6 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,\n> \t\t\t\t\t const unsigned char *old_sha1,\n> \t\t\t\t\t int flags, int *type_p)\n> {\n> -\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))\n> -\t\treturn NULL;\n> \treturn lock_ref_sha1_basic(refname, old_sha1, flags, type_p);\n> }\n"},{"id":"246348","messageId":"CAPc5daW_6bVg4B4GHA-HCRL7bzmLAdVOF2xOYa9aOOjze-zTdA@mail.gmail.com","threadId":"37143","inReplyTo":"xmqqha2el2x5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 02/12] wrapper.c: add a new function unlink_or_msg","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-18T22:59:21Z","receivedAt":"2014-07-18T22:59:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hmm, the primary reason for this seems to be because you are going to handle\nmultiple refs at a time, some of them might fail to lock due to this\nlowest-level\nhelper to unlink failing, some others may fail to lock due to some other reason,\nand the user may want to be able to differentiate various modes of failure.\n\nBut even if that were the case, would it be necessary to buffer the messages\nlike this and give them all at the end? In the transaction code path,\nit is likely\nthat you would be aborting the whole thing at the first failure, no?\n\nI dunno...\n\n\nOn Fri, Jul 18, 2014 at 3:25 PM, Junio C Hamano wrote:\n> Ronnie Sahlberg writes:\n>\n>> Signed-off-by: Ronnie Sahlberg \n>> ---\n>> git-compat-util.h | 6 ++++++\n>> wrapper.c | 18 ++++++++++++++++++\n>> 2 files changed, 24 insertions(+)\n>>\n>> diff --git a/git-compat-util.h b/git-compat-util.h\n>> index b6f03b3..426bc98 100644\n>> --- a/git-compat-util.h\n>> +++ b/git-compat-util.h\n>> @@ -704,12 +704,18 @@ void git_qsort(void *base, size_t nmemb, size_t size,\n>> #endif\n>> #endif\n>>\n>> +#include \"strbuf.h\"\n>> +\n>> /*\n>> * Preserves errno, prints a message, but gives no warning for ENOENT.\n>> * Always returns the return value of unlink(2).\n>> */\n>> int unlink_or_warn(const char *path);\n>> /*\n>> + * Like unlink_or_warn but populates a strbuf\n>> + */\n>> +int unlink_or_msg(const char *file, struct strbuf *err);\n>> +/*\n>> * Likewise for rmdir(2).\n>> */\n>> int rmdir_or_warn(const char *path);\n>> diff --git a/wrapper.c b/wrapper.c\n>> index 740e193..74a0cc0 100644\n>> --- a/wrapper.c\n>> +++ b/wrapper.c\n>> @@ -438,6 +438,24 @@ static int warn_if_unremovable(const char *op, const char *file, int rc)\n>> return rc;\n>> }\n>>\n>> +int unlink_or_msg(const char *file, struct strbuf *err)\n>> +{\n>> + if (err) {\n>> + int rc = unlink(file);\n>> + int save_errno = errno;\n>> +\n>> + if (rc < 0 && errno != ENOENT) {\n>> + strbuf_addf(err, \"unable to unlink %s: %s\",\n>> + file, strerror(errno));\n>> + errno = save_errno;\n>> + return -1;\n>> + }\n>> + return 0;\n>> + }\n>> +\n>> + return unlink_or_warn(file);\n>> +}\n>\n> In general, I do not generally like to see messages propagated\n> upwards from deeper levels of the callchain to the callers to be\n> used later, primarily because that will easily make it harder to\n> localize the message-lego.\n>\n> For this partcular one, shouldn't the caller be doing\n>\n> if (unlink(file) && errno != ENOENT) {\n> ... do its own error message ...\n> }\n>\n> instead of calling any of the unlink_or_whatever() helper?\n>\n>\n>> int unlink_or_warn(const char *file)\n>> {\n>> return warn_if_unremovable(\"unlink\", file, unlink(file));\n"},{"id":"246509","messageId":"CAL=YDWmHcy+Kf+gJLHyFK7bVjnD+bk7rX22jqHVFmTXoHDCEhQ@mail.gmail.com","threadId":"37143","inReplyTo":"CAPc5daW_6bVg4B4GHA-HCRL7bzmLAdVOF2xOYa9aOOjze-zTdA@mail.gmail.com","subject":"Re: [PATCH 02/12] wrapper.c: add a new function unlink_or_msg","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-22T17:42:12Z","receivedAt":"2014-07-22T17:42:12Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Fri, Jul 18, 2014 at 3:59 PM, Junio C Hamano wrote:\n> Hmm, the primary reason for this seems to be because you are going to handle\n> multiple refs at a time, some of them might fail to lock due to this\n> lowest-level\n> helper to unlink failing, some others may fail to lock due to some other reason,\n> and the user may want to be able to differentiate various modes of failure.\n>\n> But even if that were the case, would it be necessary to buffer the messages\n> like this and give them all at the end? In the transaction code path,\n> it is likely\n> that you would be aborting the whole thing at the first failure, no?\n\nNot necessarily.\nI can think of reasons both for and against \"abort on first failure\".\n\nOne reason for the former could be if there are problems with multiple\nrefs in a single transaction.\nIt would be very annoying to have to do\n$ git \n error: ref foo has a problem\n\n$ \n$ git (try again)\n error: ref bar has a problem\n...\n\nAnd it might be more userfriendly if that instead would be\n$ git \n error: ref foo has a problem\n error: ref bar has a problem\n ...\n\nAnd get all the failures in one go instead of having to iterate.\n\nThe reason for the latter I think is it would be cleaner/simpler/...\nto just have an \"abort on first failure\".\n\n\nOn the past discussions on the list I think I have heard voices for\nboth approaches.\nI don't think we have all that many\nmultiple-refs-in-a-single-transaction yet in what is checked in so far\nso I think we are practically still \"abort on first error\".\n\nI personally do not know yet which approach is the best but would like\nto keep the door open for the \"try all and fail at the end\".\nThat said, I do not feel all that strongly about this.\nIf you have strong feelings about this I can remove the unlink_or_msg\npatch and rework the rest of the series to cope with it.\n\n\nregards\nronnie sahlberg\n\n\n\n>\n> I dunno...\n>\n>\n> On Fri, Jul 18, 2014 at 3:25 PM, Junio C Hamano wrote:\n>> Ronnie Sahlberg writes:\n>>\n>>> Signed-off-by: Ronnie Sahlberg \n>>> ---\n>>> git-compat-util.h | 6 ++++++\n>>> wrapper.c | 18 ++++++++++++++++++\n>>> 2 files changed, 24 insertions(+)\n>>>\n>>> diff --git a/git-compat-util.h b/git-compat-util.h\n>>> index b6f03b3..426bc98 100644\n>>> --- a/git-compat-util.h\n>>> +++ b/git-compat-util.h\n>>> @@ -704,12 +704,18 @@ void git_qsort(void *base, size_t nmemb, size_t size,\n>>> #endif\n>>> #endif\n>>>\n>>> +#include \"strbuf.h\"\n>>> +\n>>> /*\n>>> * Preserves errno, prints a message, but gives no warning for ENOENT.\n>>> * Always returns the return value of unlink(2).\n>>> */\n>>> int unlink_or_warn(const char *path);\n>>> /*\n>>> + * Like unlink_or_warn but populates a strbuf\n>>> + */\n>>> +int unlink_or_msg(const char *file, struct strbuf *err);\n>>> +/*\n>>> * Likewise for rmdir(2).\n>>> */\n>>> int rmdir_or_warn(const char *path);\n>>> diff --git a/wrapper.c b/wrapper.c\n>>> index 740e193..74a0cc0 100644\n>>> --- a/wrapper.c\n>>> +++ b/wrapper.c\n>>> @@ -438,6 +438,24 @@ static int warn_if_unremovable(const char *op, const char *file, int rc)\n>>> return rc;\n>>> }\n>>>\n>>> +int unlink_or_msg(const char *file, struct strbuf *err)\n>>> +{\n>>> + if (err) {\n>>> + int rc = unlink(file);\n>>> + int save_errno = errno;\n>>> +\n>>> + if (rc < 0 && errno != ENOENT) {\n>>> + strbuf_addf(err, \"unable to unlink %s: %s\",\n>>> + file, strerror(errno));\n>>> + errno = save_errno;\n>>> + return -1;\n>>> + }\n>>> + return 0;\n>>> + }\n>>> +\n>>> + return unlink_or_warn(file);\n>>> +}\n>>\n>> In general, I do not generally like to see messages propagated\n>> upwards from deeper levels of the callchain to the callers to be\n>> used later, primarily because that will easily make it harder to\n>> localize the message-lego.\n>>\n>> For this partcular one, shouldn't the caller be doing\n>>\n>> if (unlink(file) && errno != ENOENT) {\n>> ... do its own error message ...\n>> }\n>>\n>> instead of calling any of the unlink_or_whatever() helper?\n>>\n>>\n>>> int unlink_or_warn(const char *file)\n>>> {\n>>> return warn_if_unremovable(\"unlink\", file, unlink(file));\n"},{"id":"246510","messageId":"xmqq4my9gtvj.fsf@gitster.dls.corp.google.com","threadId":"37143","inReplyTo":"CAL=YDWmHcy+Kf+gJLHyFK7bVjnD+bk7rX22jqHVFmTXoHDCEhQ@mail.gmail.com","subject":"Re: [PATCH 02/12] wrapper.c: add a new function unlink_or_msg","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-22T17:56:16Z","receivedAt":"2014-07-22T17:56:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg writes:\n\n> One reason for the former could be if there are problems with multiple\n> refs in a single transaction.\n> It would be very annoying to have to do\n> $ git \n> error: ref foo has a problem\n>\n> $ \n> $ git (try again)\n> error: ref bar has a problem\n> ...\n>\n> And it might be more userfriendly if that instead would be\n> $ git \n> error: ref foo has a problem\n> error: ref bar has a problem\n> ...\n>\n> And get all the failures in one go instead of having to iterate.\n> ...\n> I personally do not know yet which approach is the best but would like\n> to keep the door open for the \"try all and fail at the end\".\n\nYes, and often it is useful (e.g. we allow to push multiple and then\nshow the result for individual refs). But that still does not show\na need for accumulating the error messages to strbuf, does it?\n"},{"id":"246511","messageId":"CAL=YDWnoCqEAN8+XPiVgPqUazAbzKG2oedLGBtEwPGCJMm_ctg@mail.gmail.com","threadId":"37143","inReplyTo":"xmqqd2d2l2o7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 05/12] refs.c: pass NULL as *flags to read_ref_full","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-22T18:19:52Z","receivedAt":"2014-07-22T18:19:52Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Fri, Jul 18, 2014 at 3:31 PM, Junio C Hamano wrote:\n> Ronnie Sahlberg writes:\n>\n>> We call read_ref_full with a pointer to flags from rename_ref but since\n>> we never actually use the returned flags we can just pass NULL here instead.\n>\n> Sensible, at least for the current callers. I had to wonder if\n> rename_ref() would never want to take advantage of the flags return\n> parameter in the future, though. For example, would it want to act\n> differently when the given ref turns out to be a symref?\n\nI don't know.\n\nWe have a check if the old refname was a symref or not since the old\nversion did not have code for how to handle renaming the reflog.\n(That check is removed in a later series when we have enough\ntransaction code and reflog api changes so that we no longer need to\ncall rename() for the reflog handling.)\n\nI can not think of any reason right now why, but if we need it we can\nadd the argument back when the need arises.\n\n> Would it\n> want to report something when the ref to be overwritten was a broken\n> one?\n\nGood point.\n\nThere are two cases where the new ref could be broken.\n1) It could either contain a broken SHA1, like if we do this :\n$ echo \"Broken ref\" > .git/refs/heads/foo-broken-1\n2) or it could be broken due to having a bad/invalid name :\n$ cp .git.refs.heads.master .git/refs/heads/foo-broken-1-\\*...\n\nFor 2) I think this should not be allowed so the rename should just\nfail with something like :\n$ ./git branch -M foo foo-broken-1-\\*...\nfatal: 'foo-broken-1-*...' is not a valid branch name.\n\nFor 1) if the new branch already exists but it has a broken SHA1, for\nthat case I think we should allow rename_ref to overwrite the existing\nbad SHA1 with the new, good, SHA1 value.\nCurrently this does not work in master :\n$ echo \"Broken ref\" > .git/refs/heads/foo-broken-1\n$ ./git branch -m foo foo-broken-1\nerror: unable to resolve reference refs/heads/foo-broken-1: Invalid argument\nerror: unable to lock refs/heads/foo-broken-1 for update\nfatal: Branch rename failed\n\n\nAnd the only way to recover is to first delete the branch as my other\npatch in this series now allows and then trying the rename again.\n\nFor 1), since we are planning to overwrite the current branch with a\nnew SHA1 value, I think that what makes most sense would be to treat\nthe \"branch exist but is broken\" as if the branch did not exist at all\nand just allow overwriting it with the new good value.\n\n\n\nCurrently this does not work in master :\n\n$ echo \"Broken ref\" > .git/refs/heads/foo-broken-1\n$ ./git branch -m foo foo-broken-1\nerror: unable to resolve reference refs/heads/foo-broken-1: Invalid argument\nerror: unable to lock refs/heads/foo-broken-1 for update\nfatal: Branch rename failed\nso since this is not a regression I will not update this particular\npatch series but instead I\ncan add a new patch to the next patch series to allow this so that we can do :\n$ echo \"Broken ref\" > .git/refs/heads/foo-broken-1\n$ ./git branch -m foo foo-broken-1\n\n\n\nComments/opinions?\n\nregards\nronnie sahlberg\n\n\n>\n>> Reviewed-by: Jonathan Nieder \n>> Signed-off-by: Ronnie Sahlberg \n>> ---\n>> refs.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/refs.c b/refs.c\n>> index 7d65253..0df6894 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -2666,7 +2666,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n>> goto rollback;\n>> }\n>>\n>> - if (!read_ref_full(newrefname, sha1, 1, &flag) &&\n>> + if (!read_ref_full(newrefname, sha1, 1, NULL) &&\n>> delete_ref(newrefname, sha1, REF_NODEREF)) {\n>> if (errno==EISDIR) {\n>> if (remove_empty_directories(git_path(\"%s\", newrefname))) {\n"},{"id":"246520","messageId":"xmqqlhrlf7oe.fsf@gitster.dls.corp.google.com","threadId":"37143","inReplyTo":"1405549392-27306-13-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 12/12] refs.c: fix handling of badly named refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-22T20:41:05Z","receivedAt":"2014-07-22T20:41:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg writes:\n\n> We currently do not handle badly named refs well :\n> $ cp .git/refs/heads/master .git/refs/heads/master.....@\\*@\\\\.\n> $ git branch\n> fatal: Reference has invalid format: 'refs/heads/master.....@*@\\.'\n> $ git branch -D master.....@\\*@\\\\.\n> error: branch 'master.....@*@\\.' not found.\n>\n> But we can not really recover from a badly named ref with less than\n> manually deleting the .git/refs/heads/ file.\n>\n> Change resolve_ref_unsafe to take a flags field instead of a 'reading'\n> boolean and update all callers that used a non-zero value for reading\n> to pass the flag RESOLVE_REF_READING instead.\n> Add another flag RESOLVE_REF_ALLOW_BAD_NAME that will make\n> resolve_ref_unsafe skip checking the refname for sanity and use this\n> from branch.c so that we will be able to call resolve_ref_unsafe on such\n> refs when trying to delete it.\n\nMakes sense.\n\n> Add checks for refname sanity when updating (not deleting) a ref in\n> ref_transaction_update and in ref_transaction_create to make the transaction\n> fail if an attempt is made to create/update a badly named ref.\n> Since all ref changes will later go through the transaction layer this means\n> we no longer need to check for and fail for bad refnames in\n> lock_ref_sha1_basic.\n>\n> Change lock_ref_sha1_basic to not fail for bad refnames. Just check the\n> refname, and print an error, and remember that the refname is bad so that\n> we can skip calling verify_lock().\n\nThis is somewhat puzzling, though.\n\n> @@ -2180,6 +2196,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n> \t\telse\n> \t\t\tunable_to_lock_index_die(ref_file, errno);\n> \t}\n> +\tif (bad_refname)\n> +\t\treturn lock;\n\nHmph. If the only offence was that the ref was named badly due to\nhistorically loose code, does the caller not still benefit from the\nverify-lock check to make sure that other people did not muck with\nthe ref while we were planning to update it?\n\n> \treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n> \n> error_return:\n"},{"id":"246526","messageId":"CAL=YDWnTNKGW3AAr7twZ44KUb1Pxu0kms5Lt_3-4LBYGQw2+PQ@mail.gmail.com","threadId":"37143","inReplyTo":"xmqqlhrlf7oe.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 12/12] refs.c: fix handling of badly named refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-22T21:30:36Z","receivedAt":"2014-07-22T21:30:36Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, Jul 22, 2014 at 1:41 PM, Junio C Hamano wrote:\n> Ronnie Sahlberg writes:\n>\n>> We currently do not handle badly named refs well :\n>> $ cp .git/refs/heads/master .git/refs/heads/master.....@\\*@\\\\.\n>> $ git branch\n>> fatal: Reference has invalid format: 'refs/heads/master.....@*@\\.'\n>> $ git branch -D master.....@\\*@\\\\.\n>> error: branch 'master.....@*@\\.' not found.\n>>\n>> But we can not really recover from a badly named ref with less than\n>> manually deleting the .git/refs/heads/ file.\n>>\n>> Change resolve_ref_unsafe to take a flags field instead of a 'reading'\n>> boolean and update all callers that used a non-zero value for reading\n>> to pass the flag RESOLVE_REF_READING instead.\n>> Add another flag RESOLVE_REF_ALLOW_BAD_NAME that will make\n>> resolve_ref_unsafe skip checking the refname for sanity and use this\n>> from branch.c so that we will be able to call resolve_ref_unsafe on such\n>> refs when trying to delete it.\n>\n> Makes sense.\n>\n>> Add checks for refname sanity when updating (not deleting) a ref in\n>> ref_transaction_update and in ref_transaction_create to make the transaction\n>> fail if an attempt is made to create/update a badly named ref.\n>> Since all ref changes will later go through the transaction layer this means\n>> we no longer need to check for and fail for bad refnames in\n>> lock_ref_sha1_basic.\n>>\n>> Change lock_ref_sha1_basic to not fail for bad refnames. Just check the\n>> refname, and print an error, and remember that the refname is bad so that\n>> we can skip calling verify_lock().\n>\n> This is somewhat puzzling, though.\n>\n>> @@ -2180,6 +2196,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>> else\n>> unable_to_lock_index_die(ref_file, errno);\n>> }\n>> + if (bad_refname)\n>> + return lock;\n>\n> Hmph. If the only offence was that the ref was named badly due to\n> historically loose code, does the caller not still benefit from the\n> verify-lock check to make sure that other people did not muck with\n> the ref while we were planning to update it?\n>\n\nI don't think we need to do that.\nThat would imply that we would need to be able to also allow reading\nthe content of a badly named ref.\nCurrently a badly named ref can not be accessed by any function except\n git branch -D which contains the special flag that\nallows locking it eventhough the ref has an illegal name.\n\nSo no one else should be able to read or modify the ref at all.\n\nI think it is sufficient for this case to just have the semantics\n\"just delete it, I don't care what it used to point to.\" for this\nspecial case git branch -D \nso therefore since it is not the content of the ref but the name of\nthe ref itself we have a problem with I don't think we need to read\nthe old value or verify it.\n"},{"id":"246527","messageId":"CAL=YDW=zo2=6rJkZ0rXqe7=1X8j5yegHieHgmmJPO11u_U4d_Q@mail.gmail.com","threadId":"37143","inReplyTo":"CAL=YDWnTNKGW3AAr7twZ44KUb1Pxu0kms5Lt_3-4LBYGQw2+PQ@mail.gmail.com","subject":"Re: [PATCH 12/12] refs.c: fix handling of badly named refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-22T21:36:20Z","receivedAt":"2014-07-22T21:36:20Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, Jul 22, 2014 at 2:30 PM, Ronnie Sahlberg wrote:\n> On Tue, Jul 22, 2014 at 1:41 PM, Junio C Hamano wrote:\n>> Ronnie Sahlberg writes:\n>>\n>>> We currently do not handle badly named refs well :\n>>> $ cp .git/refs/heads/master .git/refs/heads/master.....@\\*@\\\\.\n>>> $ git branch\n>>> fatal: Reference has invalid format: 'refs/heads/master.....@*@\\.'\n>>> $ git branch -D master.....@\\*@\\\\.\n>>> error: branch 'master.....@*@\\.' not found.\n>>>\n>>> But we can not really recover from a badly named ref with less than\n>>> manually deleting the .git/refs/heads/ file.\n>>>\n>>> Change resolve_ref_unsafe to take a flags field instead of a 'reading'\n>>> boolean and update all callers that used a non-zero value for reading\n>>> to pass the flag RESOLVE_REF_READING instead.\n>>> Add another flag RESOLVE_REF_ALLOW_BAD_NAME that will make\n>>> resolve_ref_unsafe skip checking the refname for sanity and use this\n>>> from branch.c so that we will be able to call resolve_ref_unsafe on such\n>>> refs when trying to delete it.\n>>\n>> Makes sense.\n>>\n>>> Add checks for refname sanity when updating (not deleting) a ref in\n>>> ref_transaction_update and in ref_transaction_create to make the transaction\n>>> fail if an attempt is made to create/update a badly named ref.\n>>> Since all ref changes will later go through the transaction layer this means\n>>> we no longer need to check for and fail for bad refnames in\n>>> lock_ref_sha1_basic.\n>>>\n>>> Change lock_ref_sha1_basic to not fail for bad refnames. Just check the\n>>> refname, and print an error, and remember that the refname is bad so that\n>>> we can skip calling verify_lock().\n>>\n>> This is somewhat puzzling, though.\n>>\n>>> @@ -2180,6 +2196,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>>> else\n>>> unable_to_lock_index_die(ref_file, errno);\n>>> }\n>>> + if (bad_refname)\n>>> + return lock;\n>>\n>> Hmph. If the only offence was that the ref was named badly due to\n>> historically loose code, does the caller not still benefit from the\n>> verify-lock check to make sure that other people did not muck with\n>> the ref while we were planning to update it?\n>>\n>\n> I don't think we need to do that.\n> That would imply that we would need to be able to also allow reading\n> the content of a badly named ref.\n> Currently a badly named ref can not be accessed by any function except\n> git branch -D which contains the special flag that\n> allows locking it eventhough the ref has an illegal name.\n>\n> So no one else should be able to read or modify the ref at all.\n>\n> I think it is sufficient for this case to just have the semantics\n> \"just delete it, I don't care what it used to point to.\" for this\n> special case git branch -D \n> so therefore since it is not the content of the ref but the name of\n> the ref itself we have a problem with I don't think we need to read\n> the old value or verify it.\n\nIt is also that prior to this change we could not access these badly\nnamed refs at all.\nThis change tries to be careful to not open up too much as it tries to\nonly allow git branch -D and nothing else to start working for such\nrefs.\n(To avoid accidentally opening things up so that it becomes possible\nto start using/depending on such refs)\n"},{"id":"246529","messageId":"CAL=YDWnBKavodqxeECYAwWpx-wqUDAp7QPWEaZfB819BS70fiw@mail.gmail.com","threadId":"37143","inReplyTo":"CAL=YDWnoCqEAN8+XPiVgPqUazAbzKG2oedLGBtEwPGCJMm_ctg@mail.gmail.com","subject":"Re: [PATCH 05/12] refs.c: pass NULL as *flags to read_ref_full","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-22T21:44:20Z","receivedAt":"2014-07-22T21:44:20Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, Jul 22, 2014 at 11:19 AM, Ronnie Sahlberg wrote:\n> On Fri, Jul 18, 2014 at 3:31 PM, Junio C Hamano wrote:\n>> Ronnie Sahlberg writes:\n>>\n>>> We call read_ref_full with a pointer to flags from rename_ref but since\n>>> we never actually use the returned flags we can just pass NULL here instead.\n>>\n>> Sensible, at least for the current callers. I had to wonder if\n>> rename_ref() would never want to take advantage of the flags return\n>> parameter in the future, though. For example, would it want to act\n>> differently when the given ref turns out to be a symref?\n>\n> I don't know.\n>\n> We have a check if the old refname was a symref or not since the old\n> version did not have code for how to handle renaming the reflog.\n> (That check is removed in a later series when we have enough\n> transaction code and reflog api changes so that we no longer need to\n> call rename() for the reflog handling.)\n>\n> I can not think of any reason right now why, but if we need it we can\n> add the argument back when the need arises.\n>\n>> Would it\n>> want to report something when the ref to be overwritten was a broken\n>> one?\n>\n> Good point.\n>\n> There are two cases where the new ref could be broken.\n> 1) It could either contain a broken SHA1, like if we do this :\n> $ echo \"Broken ref\" > .git/refs/heads/foo-broken-1\n> 2) or it could be broken due to having a bad/invalid name :\n> $ cp .git.refs.heads.master .git/refs/heads/foo-broken-1-\\*...\n>\n> For 2) I think this should not be allowed so the rename should just\n> fail with something like :\n> $ ./git branch -M foo foo-broken-1-\\*...\n> fatal: 'foo-broken-1-*...' is not a valid branch name.\n>\n> For 1) if the new branch already exists but it has a broken SHA1, for\n> that case I think we should allow rename_ref to overwrite the existing\n> bad SHA1 with the new, good, SHA1 value.\n> Currently this does not work in master :\n> $ echo \"Broken ref\" > .git/refs/heads/foo-broken-1\n> $ ./git branch -m foo foo-broken-1\n> error: unable to resolve reference refs/heads/foo-broken-1: Invalid argument\n> error: unable to lock refs/heads/foo-broken-1 for update\n> fatal: Branch rename failed\n>\n>\n> And the only way to recover is to first delete the branch as my other\n> patch in this series now allows and then trying the rename again.\n>\n> For 1), since we are planning to overwrite the current branch with a\n> new SHA1 value, I think that what makes most sense would be to treat\n> the \"branch exist but is broken\" as if the branch did not exist at all\n> and just allow overwriting it with the new good value.\n>\n>\n>\n> Currently this does not work in master :\n>\n> $ echo \"Broken ref\" > .git/refs/heads/foo-broken-1\n> $ ./git branch -m foo foo-broken-1\n> error: unable to resolve reference refs/heads/foo-broken-1: Invalid argument\n> error: unable to lock refs/heads/foo-broken-1 for update\n> fatal: Branch rename failed\n> so since this is not a regression I will not update this particular\n> patch series but instead I\n> can add a new patch to the next patch series to allow this so that we can do :\n> $ echo \"Broken ref\" > .git/refs/heads/foo-broken-1\n> $ ./git branch -m foo foo-broken-1\n> \n>\n\nI have a patch to make it possible to delete a broken ref that can not\nbe resolved in :\nhttps://github.com/rsahlberg/git/commit/763ab16e1874d58a4fc5c37920abc1ea40ccd814\n\nThis patch is scheduled at the end of the next patch series (use\ntransactions for all reflog updates) I plan to send out tomorrow.\n"},{"id":"246531","messageId":"xmqqzjg1dq2z.fsf@gitster.dls.corp.google.com","threadId":"37143","inReplyTo":"CAL=YDWnTNKGW3AAr7twZ44KUb1Pxu0kms5Lt_3-4LBYGQw2+PQ@mail.gmail.com","subject":"Re: [PATCH 12/12] refs.c: fix handling of badly named refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-22T21:46:28Z","receivedAt":"2014-07-22T21:46:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg writes:\n\n> I don't think we need to do that.\n> That would imply that we would need to be able to also allow reading\n> the content of a badly named ref.\n> Currently a badly named ref can not be accessed by any function except\n> git branch -D which contains the special flag that\n> allows locking it eventhough the ref has an illegal name.\n>\n> So no one else should be able to read or modify the ref at all.\n\nOK.\n\n> I think it is sufficient for this case to just have the semantics\n> \"just delete it, I don't care what it used to point to.\" for this\n> special case git branch -D \n> so therefore since it is not the content of the ref but the name of\n> the ref itself we have a problem with I don't think we need to read\n> the old value or verify it.\n"}]}