{"thread":{"id":"39616","subject":"[PATCH v2 01/12] delete_ref(): move declaration to refs.h","startedAt":"2015-06-13T14:42:03Z","lastAt":"2015-06-20T03:30:35Z","messageCount":30,"participants":["Michael Haggerty","Stefan Beller","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":12},"messages":[{"id":"263728","messageId":"cover.1434206062.git.mhagger@alum.mit.edu","threadId":"39616","inReplyTo":null,"subject":"[PATCH v2 00/12] Improve \"refs\" module encapsulation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-13T14:42:03Z","receivedAt":"2015-06-13T14:42:03Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is v2 of this patch series. I think I have addressed all of the\nfeedback from v1 [1]. Thanks to Stefan, Peff and Junio for their\nfeedback.\n\nChanges since v1:\n\n* Change docstring for delete_ref() and a comment within the function\n definition.\n\n* Squash together two commits dealing with the error message in\n delete_refs().\n\nThese patches are also available from my GitHub account [2] as branch\n\"init-delete-refs-api\".\n\nMichael\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/271017\n[2] https://github.com/mhagger/git\n\nMichael Haggerty (12):\n delete_ref(): move declaration to refs.h\n remove_branches(): remove temporary\n delete_ref(): handle special case more explicitly\n delete_refs(): new function for the refs API\n delete_refs(): improve error message\n prune_remote(): use delete_refs()\n repack_without_refs(): make function private\n initial_ref_transaction_commit(): function for initial ref creation\n refs: remove some functions from the module's public interface\n initial_ref_transaction_commit(): check for duplicate refs\n initial_ref_transaction_commit(): check for ref D/F conflicts\n refs: move the remaining ref module declarations to refs.h\n\n archive.c | 1 +\n builtin/blame.c | 1 +\n builtin/clone.c | 19 ++++-\n builtin/fast-export.c | 1 +\n builtin/fmt-merge-msg.c | 1 +\n builtin/init-db.c | 1 +\n builtin/log.c | 1 +\n builtin/remote.c | 33 +-------\n cache.h | 68 ----------------\n refs.c | 171 ++++++++++++++++++++++++++++++++++++---\n refs.h | 211 +++++++++++++++++++++++++++++++-----------------\n remote-testsvn.c | 1 +\n 12 files changed, 321 insertions(+), 188 deletions(-)\n\n-- \n2.1.4\n"},{"id":"263722","messageId":"0c49ee45d76303320c197f10270601683ed27cf4.1434206062.git.mhagger@alum.mit.edu","threadId":"39616","inReplyTo":"cover.1434206062.git.mhagger@alum.mit.edu","subject":"[PATCH v2 01/12] delete_ref(): move declaration to refs.h","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-13T14:42:04Z","receivedAt":"2015-06-13T14:42:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Also\n\n* Add a docstring\n\n* Rename the second parameter to \"old_sha1\", to be consistent with the\n convention used elsewhere in the refs module\n\nSigned-off-by: Michael Haggerty \n---\n cache.h | 2 --\n refs.c | 5 +++--\n refs.h | 10 ++++++++++\n 3 files changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 571c98f..be92121 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -585,8 +585,6 @@ extern void update_index_if_able(struct index_state *, struct lock_file *);\n extern int hold_locked_index(struct lock_file *, int);\n extern void set_alternate_index_output(const char *);\n \n-extern int delete_ref(const char *, const unsigned char *sha1, unsigned int flags);\n-\n /* Environment bits from configuration mechanism */\n extern int trust_executable_bit;\n extern int trust_ctime;\ndiff --git a/refs.c b/refs.c\nindex a742d79..b575bb8 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2789,7 +2789,8 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)\n \treturn 0;\n }\n \n-int delete_ref(const char *refname, const unsigned char *sha1, unsigned int flags)\n+int delete_ref(const char *refname, const unsigned char *old_sha1,\n+\t unsigned int flags)\n {\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n@@ -2797,7 +2798,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, unsigned int flag\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t ref_transaction_delete(transaction, refname,\n-\t\t\t\t (sha1 && !is_null_sha1(sha1)) ? sha1 : NULL,\n+\t\t\t\t (old_sha1 && !is_null_sha1(old_sha1)) ? old_sha1 : NULL,\n \t\t\t\t flags, NULL, &err) ||\n \t ref_transaction_commit(transaction, &err)) {\n \t\terror(\"%s\", err.buf);\ndiff --git a/refs.h b/refs.h\nindex 8c3d433..68b5e81 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -202,6 +202,16 @@ extern int read_ref_at(const char *refname, unsigned int flags,\n /** Check if a particular reflog exists */\n extern int reflog_exists(const char *refname);\n \n+/*\n+ * Delete the specified reference. If old_sha1 is non-NULL and not\n+ * NULL_SHA1, then verify that the current value of the reference is\n+ * old_sha1 before deleting it. If old_sha1 is NULL or NULL_SHA1,\n+ * delete the reference if it exists, regardless of its old value.\n+ * flags is passed through to ref_transaction_delete().\n+ */\n+extern int delete_ref(const char *refname, const unsigned char *old_sha1,\n+\t\t unsigned int flags);\n+\n /** Delete a reflog */\n extern int delete_reflog(const char *refname);\n \n-- \n2.1.4\n"},{"id":"263726","messageId":"0c78010d9f06e2bfc8136c1ac11d5bc107a7043a.1434206062.git.mhagger@alum.mit.edu","threadId":"39616","inReplyTo":"cover.1434206062.git.mhagger@alum.mit.edu","subject":"[PATCH v2 02/12] remove_branches(): remove temporary","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-13T14:42:05Z","receivedAt":"2015-06-13T14:42:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\n builtin/remote.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex f4a6ec9..53b8e13 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -756,8 +756,7 @@ static int remove_branches(struct string_list *branches)\n \tstrbuf_release(&err);\n \n \tfor (i = 0; i < branches->nr; i++) {\n-\t\tstruct string_list_item *item = branches->items + i;\n-\t\tconst char *refname = item->string;\n+\t\tconst char *refname = branches->items[i].string;\n \n \t\tif (delete_ref(refname, NULL, 0))\n \t\t\tresult |= error(_(\"Could not remove branch %s\"), refname);\n-- \n2.1.4\n"},{"id":"263723","messageId":"0c66a6bd6e49b9c28fa3617ee50de746ef4ea9a3.1434206062.git.mhagger@alum.mit.edu","threadId":"39616","inReplyTo":"cover.1434206062.git.mhagger@alum.mit.edu","subject":"[PATCH v2 03/12] delete_ref(): handle special case more explicitly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-13T14:42:06Z","receivedAt":"2015-06-13T14:42:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"delete_ref() uses a different convention for its old_sha1 parameter\nthan, say, ref_transaction_delete(): NULL_SHA1 means not to check the\nold value. Make this fact a little bit clearer in the code by handling\nit in explicit, commented code rather than burying it in a conditional\nexpression.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex b575bb8..f0b6cec 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2795,10 +2795,17 @@ int delete_ref(const char *refname, const unsigned char *old_sha1,\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \n+\t/*\n+\t * Treat NULL_SHA1 and NULL alike, to mean \"we don't care what\n+\t * the old value of the reference was (or even if it didn't\n+\t * exist)\":\n+\t */\n+\tif (old_sha1 && is_null_sha1(old_sha1))\n+\t\told_sha1 = NULL;\n+\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n-\t ref_transaction_delete(transaction, refname,\n-\t\t\t\t (old_sha1 && !is_null_sha1(old_sha1)) ? old_sha1 : NULL,\n+\t ref_transaction_delete(transaction, refname, old_sha1,\n \t\t\t\t flags, NULL, &err) ||\n \t ref_transaction_commit(transaction, &err)) {\n \t\terror(\"%s\", err.buf);\n-- \n2.1.4\n"},{"id":"263724","messageId":"9a6c35bd603edfc46da241b8c762f1a269ac44f8.1434206062.git.mhagger@alum.mit.edu","threadId":"39616","inReplyTo":"cover.1434206062.git.mhagger@alum.mit.edu","subject":"[PATCH v2 04/12] delete_refs(): new function for the refs API","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-13T14:42:07Z","receivedAt":"2015-06-13T14:42:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Move the function remove_branches() from builtin/remote.c to refs.c,\nrename it to delete_refs(), and make it public.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/remote.c | 21 +--------------------\n refs.c | 19 +++++++++++++++++++\n refs.h | 7 +++++++\n 3 files changed, 27 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 53b8e13..c8dc724 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -746,25 +746,6 @@ static int mv(int argc, const char **argv)\n \treturn 0;\n }\n \n-static int remove_branches(struct string_list *branches)\n-{\n-\tstruct strbuf err = STRBUF_INIT;\n-\tint i, result = 0;\n-\n-\tif (repack_without_refs(branches, &err))\n-\t\tresult |= error(\"%s\", err.buf);\n-\tstrbuf_release(&err);\n-\n-\tfor (i = 0; i < branches->nr; i++) {\n-\t\tconst char *refname = branches->items[i].string;\n-\n-\t\tif (delete_ref(refname, NULL, 0))\n-\t\t\tresult |= error(_(\"Could not remove branch %s\"), refname);\n-\t}\n-\n-\treturn result;\n-}\n-\n static int rm(int argc, const char **argv)\n {\n \tstruct option options[] = {\n@@ -821,7 +802,7 @@ static int rm(int argc, const char **argv)\n \tstrbuf_release(&buf);\n \n \tif (!result)\n-\t\tresult = remove_branches(&branches);\n+\t\tresult = delete_refs(&branches);\n \tstring_list_clear(&branches, 0);\n \n \tif (skipped.nr) {\ndiff --git a/refs.c b/refs.c\nindex f0b6cec..6f62bd1 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2818,6 +2818,25 @@ int delete_ref(const char *refname, const unsigned char *old_sha1,\n \treturn 0;\n }\n \n+int delete_refs(struct string_list *refnames)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tint i, result = 0;\n+\n+\tif (repack_without_refs(refnames, &err))\n+\t\tresult |= error(\"%s\", err.buf);\n+\tstrbuf_release(&err);\n+\n+\tfor (i = 0; i < refnames->nr; i++) {\n+\t\tconst char *refname = refnames->items[i].string;\n+\n+\t\tif (delete_ref(refname, NULL, 0))\n+\t\t\tresult |= error(_(\"Could not remove branch %s\"), refname);\n+\t}\n+\n+\treturn result;\n+}\n+\n /*\n * People using contrib's git-new-workdir have .git/logs/refs ->\n * /some/other/path/.git/logs/refs, and that may live on another device.\ndiff --git a/refs.h b/refs.h\nindex 68b5e81..1a5d44a 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -212,6 +212,13 @@ extern int reflog_exists(const char *refname);\n extern int delete_ref(const char *refname, const unsigned char *old_sha1,\n \t\t unsigned int flags);\n \n+/*\n+ * Delete the specified references. If there are any problems, emit\n+ * errors but attempt to keep going (i.e., the deletes are not done in\n+ * an all-or-nothing transaction).\n+ */\n+extern int delete_refs(struct string_list *refnames);\n+\n /** Delete a reflog */\n extern int delete_reflog(const char *refname);\n \n-- \n2.1.4\n"},{"id":"263727","messageId":"64949bba1031b19abf86629267b8bc40666a4631.1434206062.git.mhagger@alum.mit.edu","threadId":"39616","inReplyTo":"cover.1434206062.git.mhagger@alum.mit.edu","subject":"[PATCH v2 05/12] delete_refs(): improve error message","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-13T14:42:08Z","receivedAt":"2015-06-13T14:42:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change the error message from\n\n Could not remove branch %s\n\nto\n\n could not remove reference %s\n\n* s/branch/reference/. This change makes sense even for the existing\n caller, which uses the function to delete remote-tracking\n branches.\n\n* Convert it to lower case, as per our usual convention.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6f62bd1..5386077 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2831,7 +2831,7 @@ int delete_refs(struct string_list *refnames)\n \t\tconst char *refname = refnames->items[i].string;\n \n \t\tif (delete_ref(refname, NULL, 0))\n-\t\t\tresult |= error(_(\"Could not remove branch %s\"), refname);\n+\t\t\tresult |= error(_(\"could not remove reference %s\"), refname);\n \t}\n \n \treturn result;\n-- \n2.1.4\n"},{"id":"263725","messageId":"aab9d191609b34fbfee0b68883a5b14fba2421ad.1434206062.git.mhagger@alum.mit.edu","threadId":"39616","inReplyTo":"cover.1434206062.git.mhagger@alum.mit.edu","subject":"[PATCH v2 06/12] prune_remote(): use delete_refs()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-13T14:42:09Z","receivedAt":"2015-06-13T14:42:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This will result in errors being emitted for references that can't be\ndeleted, but that is a good thing.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/remote.c | 11 ++---------\n 1 file changed, 2 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex c8dc724..cc3c741 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1314,19 +1314,12 @@ static int prune_remote(const char *remote, int dry_run)\n \t\tstring_list_append(&refs_to_prune, item->util);\n \tstring_list_sort(&refs_to_prune);\n \n-\tif (!dry_run) {\n-\t\tstruct strbuf err = STRBUF_INIT;\n-\t\tif (repack_without_refs(&refs_to_prune, &err))\n-\t\t\tresult |= error(\"%s\", err.buf);\n-\t\tstrbuf_release(&err);\n-\t}\n+\tif (!dry_run)\n+\t\tresult |= delete_refs(&refs_to_prune);\n \n \tfor_each_string_list_item(item, &states.stale) {\n \t\tconst char *refname = item->util;\n \n-\t\tif (!dry_run)\n-\t\t\tresult |= delete_ref(refname, NULL, 0);\n-\n \t\tif (dry_run)\n \t\t\tprintf_ln(_(\" * [would prune] %s\"),\n \t\t\t abbrev_ref(refname, \"refs/remotes/\"));\n-- \n2.1.4\n"},{"id":"263730","messageId":"4857117df34d5d554688cae25a8e005aac20842d.1434206062.git.mhagger@alum.mit.edu","threadId":"39616","inReplyTo":"cover.1434206062.git.mhagger@alum.mit.edu","subject":"[PATCH v2 07/12] repack_without_refs(): make function private","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-13T14:42:10Z","receivedAt":"2015-06-13T14:42:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is no longer called from outside of the refs module. Also move its\ndocstring and change it to imperative voice.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 9 ++++++++-\n refs.h | 11 -----------\n 2 files changed, 8 insertions(+), 12 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5386077..832d628 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2724,7 +2724,14 @@ int pack_refs(unsigned int flags)\n \treturn 0;\n }\n \n-int repack_without_refs(struct string_list *refnames, struct strbuf *err)\n+/*\n+ * Rewrite the packed-refs file, omitting any refs listed in\n+ * 'refnames'. On error, leave packed-refs unchanged, write an error\n+ * message to 'err', and return a nonzero value.\n+ *\n+ * The refs in 'refnames' needn't be sorted. `err` must not be NULL.\n+ */\n+static int repack_without_refs(struct string_list *refnames, struct strbuf *err)\n {\n \tstruct ref_dir *packed;\n \tstruct string_list_item *refname;\ndiff --git a/refs.h b/refs.h\nindex 1a5d44a..5f3bea7 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -154,17 +154,6 @@ extern void rollback_packed_refs(void);\n */\n int pack_refs(unsigned int flags);\n \n-/*\n- * Rewrite the packed-refs file, omitting any refs listed in\n- * 'refnames'. On error, packed-refs will be unchanged, the return\n- * value is nonzero, and a message about the error is written to the\n- * 'err' strbuf.\n- *\n- * The refs in 'refnames' needn't be sorted. `err` must not be NULL.\n- */\n-extern int repack_without_refs(struct string_list *refnames,\n-\t\t\t struct strbuf *err);\n-\n extern int ref_exists(const char *);\n \n extern int is_branch(const char *refname);\n-- \n2.1.4\n"},{"id":"263729","messageId":"19f421f504ae22538fd2a902840209c0799e67cb.1434206062.git.mhagger@alum.mit.edu","threadId":"39616","inReplyTo":"cover.1434206062.git.mhagger@alum.mit.edu","subject":"[PATCH v2 08/12] initial_ref_transaction_commit(): function for initial ref creation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-13T14:42:11Z","receivedAt":"2015-06-13T14:42:11Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\"git clone\" uses shortcuts when creating the initial set of\nreferences:\n\n* It writes them directly to packed-refs.\n\n* It doesn't lock the individual references (though it does lock the\n packed-refs file).\n\n* It doesn't check for refname conflicts between two new references or\n between one new reference and any hypothetical old ones.\n\n* It doesn't create reflog entries for the reference creations.\n\nThis functionality was implemented in builtin/clone.c. But really that\nfile shouldn't have such intimate knowledge of how references are\nstored. So provide a new function in the refs API,\ninitial_ref_transaction_commit(), which can be used for initial\nreference creation. The new function is based on the ref_transaction\ninterface.\n\nThis means that we can make some other functions private to the refs\nmodule. That will be done in a followup commit.\n\nIt would seem to make sense to add a test here that there are no\nexisting references, because that is how the function *should* be\nused. But in fact, the \"testgit\" remote helper appears to call it\n*after* having set up refs/remotes//HEAD and\nrefs/remotes//master, so we can't be so strict. For now, the\nfunction trusts its caller to only call it when it makes sense. Future\ncommits will add some more limited sanity checks.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/clone.c | 19 +++++++++++++++----\n refs.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n refs.h | 14 ++++++++++++++\n 3 files changed, 76 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex b878252..bd2a50a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -500,16 +500,27 @@ static void write_remote_refs(const struct ref *local_refs)\n {\n \tconst struct ref *r;\n \n-\tlock_packed_refs(LOCK_DIE_ON_ERROR);\n+\tstruct ref_transaction *t;\n+\tstruct strbuf err = STRBUF_INIT;\n+\n+\tt = ref_transaction_begin(&err);\n+\tif (!t)\n+\t\tdie(err.buf);\n \n \tfor (r = local_refs; r; r = r->next) {\n \t\tif (!r->peer_ref)\n \t\t\tcontinue;\n-\t\tadd_packed_ref(r->peer_ref->name, r->old_sha1);\n+\t\tif (ref_transaction_create(t, r->peer_ref->name, r->old_sha1,\n+\t\t\t\t\t 0, NULL, &err))\n+\t\t\tdie(err.buf);\n+\t}\n+\n+\tif (initial_ref_transaction_commit(t, &err)) {\n+\t\tdie(err.buf);\n \t}\n \n-\tif (commit_packed_refs())\n-\t\tdie_errno(\"unable to overwrite old ref-pack file\");\n+\tstrbuf_release(&err);\n+\tref_transaction_free(t);\n }\n \n static void write_followtags(const struct ref *refs, const char *msg)\ndiff --git a/refs.c b/refs.c\nindex 832d628..05e5d42 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -4045,6 +4045,53 @@ cleanup:\n \treturn ret;\n }\n \n+int initial_ref_transaction_commit(struct ref_transaction *transaction,\n+\t\t\t\t struct strbuf *err)\n+{\n+\tint ret = 0, i;\n+\tint n = transaction->nr;\n+\tstruct ref_update **updates = transaction->updates;\n+\n+\tassert(err);\n+\n+\tif (transaction->state != REF_TRANSACTION_OPEN)\n+\t\tdie(\"BUG: commit called for transaction that is not open\");\n+\n+\tfor (i = 0; i < n; i++) {\n+\t\tstruct ref_update *update = updates[i];\n+\n+\t\tif ((update->flags & REF_HAVE_OLD) &&\n+\t\t !is_null_sha1(update->old_sha1))\n+\t\t\tdie(\"BUG: initial ref transaction with old_sha1 set\");\n+\t}\n+\n+\tif (lock_packed_refs(0)) {\n+\t\tstrbuf_addf(err, \"unable to lock packed-refs file: %s\",\n+\t\t\t strerror(errno));\n+\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\tgoto cleanup;\n+\t}\n+\n+\tfor (i = 0; i < n; i++) {\n+\t\tstruct ref_update *update = updates[i];\n+\n+\t\tif ((update->flags & REF_HAVE_NEW) &&\n+\t\t !is_null_sha1(update->new_sha1))\n+\t\t\tadd_packed_ref(update->refname, update->new_sha1);\n+\t}\n+\n+\tif (commit_packed_refs()) {\n+\t\tstrbuf_addf(err, \"unable to commit packed-refs file: %s\",\n+\t\t\t strerror(errno));\n+\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\tgoto cleanup;\n+\t}\n+\n+cleanup:\n+\ttransaction->state = REF_TRANSACTION_CLOSED;\n+\treturn ret;\n+}\n+\n char *shorten_unambiguous_ref(const char *refname, int strict)\n {\n \tint i;\ndiff --git a/refs.h b/refs.h\nindex 5f3bea7..9602889 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -366,6 +366,20 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err);\n \n /*\n+ * Like ref_transaction_commit(), but optimized for creating\n+ * references when originally initializing a repository (e.g., by \"git\n+ * clone\"). It writes the new references directly to packed-refs\n+ * without locking the individual references.\n+ *\n+ * It is a bug to call this function when there might be other\n+ * processes accessing the repository or if there are existing\n+ * references that might conflict with the ones being created. All\n+ * old_sha1 values must either be absent or NULL_SHA1.\n+ */\n+int initial_ref_transaction_commit(struct ref_transaction *transaction,\n+\t\t\t\t struct strbuf *err);\n+\n+/*\n * Free an existing transaction and all associated data.\n */\n void ref_transaction_free(struct ref_transaction *transaction);\n-- \n2.1.4\n"},{"id":"263731","messageId":"0ecf1eb194018d069348722ef679de3ef807af88.1434206062.git.mhagger@alum.mit.edu","threadId":"39616","inReplyTo":"cover.1434206062.git.mhagger@alum.mit.edu","subject":"[PATCH v2 09/12] refs: remove some functions from the module's public interface","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-13T14:42:12Z","receivedAt":"2015-06-13T14:42:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The following functions are no longer used from outside the refs\nmodule:\n\n* lock_packed_refs()\n* add_packed_ref()\n* commit_packed_refs()\n* rollback_packed_refs()\n\nSo make these functions private.\n\nThis is an important step, because it means that nobody outside of the\nrefs module needs to know the difference between loose and packed\nreferences.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 31 ++++++++++++++++++++++++-------\n refs.h | 30 ------------------------------\n 2 files changed, 24 insertions(+), 37 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 05e5d42..a715524 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1314,7 +1314,13 @@ static struct ref_dir *get_packed_refs(struct ref_cache *refs)\n \treturn get_packed_ref_dir(get_packed_ref_cache(refs));\n }\n \n-void add_packed_ref(const char *refname, const unsigned char *sha1)\n+/*\n+ * Add a reference to the in-memory packed reference cache. This may\n+ * only be called while the packed-refs file is locked (see\n+ * lock_packed_refs()). To actually write the packed-refs file, call\n+ * commit_packed_refs().\n+ */\n+static void add_packed_ref(const char *refname, const unsigned char *sha1)\n {\n \tstruct packed_ref_cache *packed_ref_cache =\n \t\tget_packed_ref_cache(&ref_cache);\n@@ -2503,8 +2509,12 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n-/* This should return a meaningful errno on failure */\n-int lock_packed_refs(int flags)\n+/*\n+ * Lock the packed-refs file for writing. Flags is passed to\n+ * hold_lock_file_for_update(). Return 0 on success. On errors, set\n+ * errno appropriately and return a nonzero value.\n+ */\n+static int lock_packed_refs(int flags)\n {\n \tstatic int timeout_configured = 0;\n \tstatic int timeout_value = 1000;\n@@ -2534,10 +2544,12 @@ int lock_packed_refs(int flags)\n }\n \n /*\n- * Commit the packed refs changes.\n- * On error we must make sure that errno contains a meaningful value.\n+ * Write the current version of the packed refs cache from memory to\n+ * disk. The packed-refs file must already be locked for writing (see\n+ * lock_packed_refs()). Return zero on success. On errors, set errno\n+ * and return a nonzero value\n */\n-int commit_packed_refs(void)\n+static int commit_packed_refs(void)\n {\n \tstruct packed_ref_cache *packed_ref_cache =\n \t\tget_packed_ref_cache(&ref_cache);\n@@ -2566,7 +2578,12 @@ int commit_packed_refs(void)\n \treturn error;\n }\n \n-void rollback_packed_refs(void)\n+/*\n+ * Rollback the lockfile for the packed-refs file, and discard the\n+ * in-memory packed reference cache. (The packed-refs file will be\n+ * read anew if it is needed again after this function is called.)\n+ */\n+static void rollback_packed_refs(void)\n {\n \tstruct packed_ref_cache *packed_ref_cache =\n \t\tget_packed_ref_cache(&ref_cache);\ndiff --git a/refs.h b/refs.h\nindex 9602889..cd87f2f 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -111,36 +111,6 @@ extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refn\n extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct string_list *refnames);\n \n /*\n- * Lock the packed-refs file for writing. Flags is passed to\n- * hold_lock_file_for_update(). Return 0 on success.\n- * Errno is set to something meaningful on error.\n- */\n-extern int lock_packed_refs(int flags);\n-\n-/*\n- * Add a reference to the in-memory packed reference cache. This may\n- * only be called while the packed-refs file is locked (see\n- * lock_packed_refs()). To actually write the packed-refs file, call\n- * commit_packed_refs().\n- */\n-extern void add_packed_ref(const char *refname, const unsigned char *sha1);\n-\n-/*\n- * Write the current version of the packed refs cache from memory to\n- * disk. The packed-refs file must already be locked for writing (see\n- * lock_packed_refs()). Return zero on success.\n- * Sets errno to something meaningful on error.\n- */\n-extern int commit_packed_refs(void);\n-\n-/*\n- * Rollback the lockfile for the packed-refs file, and discard the\n- * in-memory packed reference cache. (The packed-refs file will be\n- * read anew if it is needed again after this function is called.)\n- */\n-extern void rollback_packed_refs(void);\n-\n-/*\n * Flags for controlling behaviour of pack_refs()\n * PACK_REFS_PRUNE: Prune loose refs after packing\n * PACK_REFS_ALL: Pack _all_ refs, not just tags and already packed refs\n-- \n2.1.4\n"},{"id":"263734","messageId":"73ebe2bdae4e7e38a2349db4c5be783085971b19.1434206062.git.mhagger@alum.mit.edu","threadId":"39616","inReplyTo":"cover.1434206062.git.mhagger@alum.mit.edu","subject":"[PATCH v2 10/12] initial_ref_transaction_commit(): check for duplicate refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-13T14:42:13Z","receivedAt":"2015-06-13T14:42:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Error out if the ref_transaction includes more than one update for any\nrefname.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex a715524..dfe9090 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -4068,12 +4068,22 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \tint ret = 0, i;\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n+\tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n \n \tassert(err);\n \n \tif (transaction->state != REF_TRANSACTION_OPEN)\n \t\tdie(\"BUG: commit called for transaction that is not open\");\n \n+\t/* Fail if a refname appears more than once in the transaction: */\n+\tfor (i = 0; i < n; i++)\n+\t\tstring_list_append(&affected_refnames, updates[i]->refname);\n+\tstring_list_sort(&affected_refnames);\n+\tif (ref_update_reject_duplicates(&affected_refnames, err)) {\n+\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\tgoto cleanup;\n+\t}\n+\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n@@ -4106,6 +4116,7 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \n cleanup:\n \ttransaction->state = REF_TRANSACTION_CLOSED;\n+\tstring_list_clear(&affected_refnames, 0);\n \treturn ret;\n }\n \n-- \n2.1.4\n"},{"id":"263732","messageId":"95cc41220378be062b4f8406f2e1adf6da529abc.1434206062.git.mhagger@alum.mit.edu","threadId":"39616","inReplyTo":"cover.1434206062.git.mhagger@alum.mit.edu","subject":"[PATCH v2 11/12] initial_ref_transaction_commit(): check for ref D/F conflicts","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-13T14:42:14Z","receivedAt":"2015-06-13T14:42:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"In initial_ref_transaction_commit(), check for D/F conflicts (i.e.,\nthe type of conflict that exists between \"refs/foo\" and\n\"refs/foo/bar\") among the references being created and between the\nreferences being created and any hypothetical existing references.\n\nIdeally, there shouldn't *be* any existing references when this\nfunction is called. But, at least in the case of the \"testgit\" remote\nhelper, \"clone\" can be called after the remote-tracking \"HEAD\" and\n\"master\" branches have already been created. So let's just do the\nfull-blown check.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 34 ++++++++++++++++++++++++++++++++++\n 1 file changed, 34 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex dfe9090..340d67f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -4062,9 +4062,19 @@ cleanup:\n \treturn ret;\n }\n \n+static int ref_present(const char *refname,\n+\t\t const struct object_id *oid, int flags, void *cb_data)\n+{\n+\tstruct string_list *affected_refnames = cb_data;\n+\n+\treturn string_list_has_string(affected_refnames, refname);\n+}\n+\n int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t struct strbuf *err)\n {\n+\tstruct ref_dir *loose_refs = get_loose_refs(&ref_cache);\n+\tstruct ref_dir *packed_refs = get_packed_refs(&ref_cache);\n \tint ret = 0, i;\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n@@ -4084,12 +4094,36 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \t\tgoto cleanup;\n \t}\n \n+\t/*\n+\t * It's really undefined to call this function in an active\n+\t * repository or when there are existing references: we are\n+\t * only locking and changing packed-refs, so (1) any\n+\t * simultaneous processes might try to change a reference at\n+\t * the same time we do, and (2) any existing loose versions of\n+\t * the references that we are setting would have precedence\n+\t * over our values. But some remote helpers create the remote\n+\t * \"HEAD\" and \"master\" branches before calling this function,\n+\t * so here we really only check that none of the references\n+\t * that we are creating already exists.\n+\t */\n+\tif (for_each_rawref(ref_present, &affected_refnames))\n+\t\tdie(\"BUG: initial ref transaction called with existing refs\");\n+\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif ((update->flags & REF_HAVE_OLD) &&\n \t\t !is_null_sha1(update->old_sha1))\n \t\t\tdie(\"BUG: initial ref transaction with old_sha1 set\");\n+\t\tif (verify_refname_available(update->refname,\n+\t\t\t\t\t &affected_refnames, NULL,\n+\t\t\t\t\t loose_refs, err) ||\n+\t\t verify_refname_available(update->refname,\n+\t\t\t\t\t &affected_refnames, NULL,\n+\t\t\t\t\t packed_refs, err)) {\n+\t\t\tret = TRANSACTION_NAME_CONFLICT;\n+\t\t\tgoto cleanup;\n+\t\t}\n \t}\n \n \tif (lock_packed_refs(0)) {\n-- \n2.1.4\n"},{"id":"263733","messageId":"79947baebd702f8e9fe9b667e2eeb69afa73094b.1434206062.git.mhagger@alum.mit.edu","threadId":"39616","inReplyTo":"cover.1434206062.git.mhagger@alum.mit.edu","subject":"[PATCH v2 12/12] refs: move the remaining ref module declarations to refs.h","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-13T14:42:15Z","receivedAt":"2015-06-13T14:42:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Some functions from the refs module were still declared in cache.h.\nMove them to refs.h.\n\nAdd some parameter names where they were missing.\n\nSigned-off-by: Michael Haggerty \n---\n archive.c | 1 +\n builtin/blame.c | 1 +\n builtin/fast-export.c | 1 +\n builtin/fmt-merge-msg.c | 1 +\n builtin/init-db.c | 1 +\n builtin/log.c | 1 +\n cache.h | 66 -----------------------\n refs.c | 6 ++-\n refs.h | 139 +++++++++++++++++++++++++++++++++++++-----------\n remote-testsvn.c | 1 +\n 10 files changed, 118 insertions(+), 100 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex d37c41d..936a594 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"refs.h\"\n #include \"commit.h\"\n #include \"tree-walk.h\"\n #include \"attr.h\"\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex b3e948e..1c998cb 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -6,6 +6,7 @@\n */\n \n #include \"cache.h\"\n+#include \"refs.h\"\n #include \"builtin.h\"\n #include \"blob.h\"\n #include \"commit.h\"\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex b8182c2..d23f3be 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -5,6 +5,7 @@\n */\n #include \"builtin.h\"\n #include \"cache.h\"\n+#include \"refs.h\"\n #include \"commit.h\"\n #include \"object.h\"\n #include \"tag.h\"\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 05f4c26..4ba7f28 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -1,5 +1,6 @@\n #include \"builtin.h\"\n #include \"cache.h\"\n+#include \"refs.h\"\n #include \"commit.h\"\n #include \"diff.h\"\n #include \"revision.h\"\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 4335738..49df78d 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -4,6 +4,7 @@\n * Copyright (C) Linus Torvalds, 2005\n */\n #include \"cache.h\"\n+#include \"refs.h\"\n #include \"builtin.h\"\n #include \"exec_cmd.h\"\n #include \"parse-options.h\"\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e67671e..3caa917 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -5,6 +5,7 @@\n *\t\t 2006 Junio Hamano\n */\n #include \"cache.h\"\n+#include \"refs.h\"\n #include \"color.h\"\n #include \"commit.h\"\n #include \"diff.h\"\ndiff --git a/cache.h b/cache.h\nindex be92121..1c00098 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1009,76 +1009,10 @@ extern int get_oid_hex(const char *hex, struct object_id *sha1);\n \n extern char *sha1_to_hex(const unsigned char *sha1);\t/* static buffer result! */\n extern char *oid_to_hex(const struct object_id *oid);\t/* same static buffer as sha1_to_hex */\n-extern int read_ref_full(const char *refname, int resolve_flags,\n-\t\t\t unsigned char *sha1, int *flags);\n-extern int read_ref(const char *refname, unsigned char *sha1);\n \n-/*\n- * Resolve a reference, recursively following symbolic refererences.\n- *\n- * Store the referred-to object's name in sha1 and return the name of\n- * the non-symbolic reference that ultimately pointed at it. The\n- * return value, if not NULL, is a pointer into either a static buffer\n- * or the input ref.\n- *\n- * If the reference cannot be resolved to an object, the behavior\n- * depends on the RESOLVE_REF_READING flag:\n- *\n- * - If RESOLVE_REF_READING is set, return NULL.\n- *\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 the RESOLVE_REF_NO_RECURSE flag is passed, only resolves one\n- * level of symbolic reference. The value stored in sha1 for a symbolic\n- * reference will always be null_sha1 in this case, and the return\n- * value is the reference that the symref refers to directly.\n- *\n- * If flags 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), REF_ISSYMREF (if the initial reference was a\n- * symbolic reference), REF_BAD_NAME (if the reference name is ill\n- * formed --- see RESOLVE_REF_ALLOW_BAD_NAME below), and REF_ISBROKEN\n- * (if the ref is malformed or has a bad name). See refs.h for more detail\n- * on each flag.\n- *\n- * If ref is not a properly-formatted, normalized reference, return\n- * NULL. If more than MAXDEPTH recursive symbolic lookups are needed,\n- * give up and return NULL.\n- *\n- * RESOLVE_REF_ALLOW_BAD_NAME allows resolving refs even when their\n- * name is invalid according to git-check-ref-format(1). If the name\n- * is bad then the value stored in sha1 will be null_sha1 and the two\n- * flags REF_ISBROKEN and REF_BAD_NAME will be set.\n- *\n- * Even with RESOLVE_REF_ALLOW_BAD_NAME, names that escape the refs/\n- * directory and do not consist of all caps and underscores cannot be\n- * resolved. The function returns NULL for such ref names.\n- * Caps and underscores refers to the special refs, such as HEAD,\n- * FETCH_HEAD and friends, that all live outside of the refs/ directory.\n- */\n-#define RESOLVE_REF_READING 0x01\n-#define RESOLVE_REF_NO_RECURSE 0x02\n-#define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n-extern const char *resolve_ref_unsafe(const char *ref, int resolve_flags, unsigned char *sha1, int *flags);\n-extern char *resolve_refdup(const char *ref, int resolve_flags, unsigned char *sha1, int *flags);\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);\n extern int interpret_branch_name(const char *str, int len, struct strbuf *);\n extern int get_sha1_mb(const char *str, unsigned char *sha1);\n \n-/*\n- * Return true iff abbrev_name is a possible abbreviation for\n- * full_name according to the rules defined by ref_rev_parse_rules in\n- * refs.c.\n- */\n-extern int refname_match(const char *abbrev_name, const char *full_name);\n-\n-extern int create_symref(const char *ref, const char *refs_heads_master, const char *logmsg);\n extern int validate_headref(const char *ref);\n \n extern int base_name_compare(const char *name1, int len1, int mode1, const char *name2, int len2, int mode2);\ndiff --git a/refs.c b/refs.c\nindex 340d67f..37a679a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1732,9 +1732,11 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n \treturn ret;\n }\n \n-char *resolve_refdup(const char *ref, int resolve_flags, unsigned char *sha1, int *flags)\n+char *resolve_refdup(const char *refname, int resolve_flags,\n+\t\t unsigned char *sha1, int *flags)\n {\n-\treturn xstrdup_or_null(resolve_ref_unsafe(ref, resolve_flags, sha1, flags));\n+\treturn xstrdup_or_null(resolve_ref_unsafe(refname, resolve_flags,\n+\t\t\t\t\t\t sha1, flags));\n }\n \n /* The argument to filter_refs */\ndiff --git a/refs.h b/refs.h\nindex cd87f2f..c9a3388 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -2,6 +2,98 @@\n #define REFS_H\n \n /*\n+ * Resolve a reference, recursively following symbolic refererences.\n+ *\n+ * Store the referred-to object's name in sha1 and return the name of\n+ * the non-symbolic reference that ultimately pointed at it. The\n+ * return value, if not NULL, is a pointer into either a static buffer\n+ * or the input ref.\n+ *\n+ * If the reference cannot be resolved to an object, the behavior\n+ * depends on the RESOLVE_REF_READING flag:\n+ *\n+ * - If RESOLVE_REF_READING is set, return NULL.\n+ *\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 the RESOLVE_REF_NO_RECURSE flag is passed, only resolves one\n+ * level of symbolic reference. The value stored in sha1 for a symbolic\n+ * reference will always be null_sha1 in this case, and the return\n+ * value is the reference that the symref refers to directly.\n+ *\n+ * If flags 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), REF_ISSYMREF (if the initial reference was a\n+ * symbolic reference), REF_BAD_NAME (if the reference name is ill\n+ * formed --- see RESOLVE_REF_ALLOW_BAD_NAME below), and REF_ISBROKEN\n+ * (if the ref is malformed or has a bad name). See refs.h for more detail\n+ * on each flag.\n+ *\n+ * If ref is not a properly-formatted, normalized reference, return\n+ * NULL. If more than MAXDEPTH recursive symbolic lookups are needed,\n+ * give up and return NULL.\n+ *\n+ * RESOLVE_REF_ALLOW_BAD_NAME allows resolving refs even when their\n+ * name is invalid according to git-check-ref-format(1). If the name\n+ * is bad then the value stored in sha1 will be null_sha1 and the two\n+ * flags REF_ISBROKEN and REF_BAD_NAME will be set.\n+ *\n+ * Even with RESOLVE_REF_ALLOW_BAD_NAME, names that escape the refs/\n+ * directory and do not consist of all caps and underscores cannot be\n+ * resolved. The function returns NULL for such ref names.\n+ * Caps and underscores refers to the special refs, such as HEAD,\n+ * FETCH_HEAD and friends, that all live outside of the refs/ directory.\n+ */\n+#define RESOLVE_REF_READING 0x01\n+#define RESOLVE_REF_NO_RECURSE 0x02\n+#define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n+\n+extern const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n+\t\t\t\t unsigned char *sha1, int *flags);\n+\n+extern char *resolve_refdup(const char *refname, int resolve_flags,\n+\t\t\t unsigned char *sha1, int *flags);\n+\n+extern int read_ref_full(const char *refname, int resolve_flags,\n+\t\t\t unsigned char *sha1, int *flags);\n+extern int read_ref(const char *refname, unsigned char *sha1);\n+\n+extern int ref_exists(const char *refname);\n+\n+extern int is_branch(const char *refname);\n+\n+/*\n+ * If refname is a non-symbolic reference that refers to a tag object,\n+ * and the tag can be (recursively) dereferenced to a non-tag object,\n+ * store the SHA1 of the referred-to object to sha1 and return 0. If\n+ * any of these conditions are not met, return a non-zero value.\n+ * Symbolic references are considered unpeelable, even if they\n+ * ultimately resolve to a peelable tag.\n+ */\n+extern int peel_ref(const char *refname, unsigned char *sha1);\n+\n+/**\n+ * Resolve refname in the nested \"gitlink\" repository that is located\n+ * at path. If the resolution is successful, return 0 and set sha1 to\n+ * the name of the object; otherwise, return a non-zero value.\n+ */\n+extern int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sha1);\n+\n+/*\n+ * Return true iff abbrev_name is a possible abbreviation for\n+ * full_name according to the rules defined by ref_rev_parse_rules in\n+ * refs.c.\n+ */\n+extern int refname_match(const char *abbrev_name, const char *full_name);\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);\n+\n+/*\n * A ref_transaction represents a collection of ref updates\n * that should succeed or fail together.\n *\n@@ -78,15 +170,15 @@ typedef int each_ref_fn(const char *refname,\n * modifies the reference also returns a nonzero value to immediately\n * stop the iteration.\n */\n-extern int head_ref(each_ref_fn, void *);\n-extern int for_each_ref(each_ref_fn, void *);\n-extern int for_each_ref_in(const char *, each_ref_fn, void *);\n-extern int for_each_tag_ref(each_ref_fn, void *);\n-extern int for_each_branch_ref(each_ref_fn, void *);\n-extern int for_each_remote_ref(each_ref_fn, void *);\n-extern int for_each_replace_ref(each_ref_fn, void *);\n-extern int for_each_glob_ref(each_ref_fn, const char *pattern, void *);\n-extern int for_each_glob_ref_in(each_ref_fn, const char *pattern, const char* prefix, void *);\n+extern int head_ref(each_ref_fn fn, void *cb_data);\n+extern int for_each_ref(each_ref_fn fn, void *cb_data);\n+extern int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);\n+extern int for_each_tag_ref(each_ref_fn fn, void *cb_data);\n+extern int for_each_branch_ref(each_ref_fn fn, void *cb_data);\n+extern int for_each_remote_ref(each_ref_fn fn, void *cb_data);\n+extern int for_each_replace_ref(each_ref_fn fn, void *cb_data);\n+extern int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);\n+extern int for_each_glob_ref_in(each_ref_fn fn, const char *pattern, const char* prefix, void *cb_data);\n \n extern int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);\n extern int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);\n@@ -99,14 +191,14 @@ extern int for_each_remote_ref_submodule(const char *submodule, each_ref_fn fn,\n extern int head_ref_namespaced(each_ref_fn fn, void *cb_data);\n extern int for_each_namespaced_ref(each_ref_fn fn, void *cb_data);\n \n+/* can be used to learn about broken ref and symref */\n+extern int for_each_rawref(each_ref_fn fn, void *cb_data);\n+\n static inline const char *has_glob_specials(const char *pattern)\n {\n \treturn strpbrk(pattern, \"?*[\");\n }\n \n-/* can be used to learn about broken ref and symref */\n-extern int for_each_rawref(each_ref_fn, void *);\n-\n extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname);\n extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct string_list *refnames);\n \n@@ -124,20 +216,6 @@ extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct st\n */\n int pack_refs(unsigned int flags);\n \n-extern int ref_exists(const char *);\n-\n-extern int is_branch(const char *refname);\n-\n-/*\n- * If refname is a non-symbolic reference that refers to a tag object,\n- * and the tag can be (recursively) dereferenced to a non-tag object,\n- * store the SHA1 of the referred-to object to sha1 and return 0. If\n- * any of these conditions are not met, return a non-zero value.\n- * Symbolic references are considered unpeelable, even if they\n- * ultimately resolve to a peelable tag.\n- */\n-extern int peel_ref(const char *refname, unsigned char *sha1);\n-\n /*\n * Flags controlling ref_transaction_update(), ref_transaction_create(), etc.\n * REF_NODEREF: act on the ref directly, instead of dereferencing\n@@ -206,17 +284,13 @@ extern int for_each_reflog(each_ref_fn, void *);\n extern int check_refname_format(const char *refname, int flags);\n \n extern const char *prettify_refname(const char *refname);\n+\n extern char *shorten_unambiguous_ref(const char *refname, int strict);\n \n /** rename ref, return 0 on success **/\n extern int rename_ref(const char *oldref, const char *newref, const char *logmsg);\n \n-/**\n- * Resolve refname in the nested \"gitlink\" repository that is located\n- * at path. If the resolution is successful, return 0 and set sha1 to\n- * the name of the object; otherwise, return a non-zero value.\n- */\n-extern int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sha1);\n+extern int create_symref(const char *ref, const char *refs_heads_master, const char *logmsg);\n \n enum action_on_err {\n \tUPDATE_REFS_MSG_ON_ERR,\n@@ -367,6 +441,7 @@ int update_ref(const char *msg, const char *refname,\n \t unsigned int flags, enum action_on_err onerr);\n \n extern int parse_hide_refs_config(const char *var, const char *value, const char *);\n+\n extern int ref_is_hidden(const char *);\n \n enum expire_reflog_flags {\ndiff --git a/remote-testsvn.c b/remote-testsvn.c\nindex 48bf6eb..f599c37 100644\n--- a/remote-testsvn.c\n+++ b/remote-testsvn.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"refs.h\"\n #include \"remote.h\"\n #include \"strbuf.h\"\n #include \"url.h\"\n-- \n2.1.4\n"},{"id":"263835","messageId":"CAGZ79kbx4Cc3hfd2jnvOWt1eUfLNGSjVu0oJ8vR2gnyf9m6QWA@mail.gmail.com","threadId":"39616","inReplyTo":"cover.1434206062.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 00/12] Improve \"refs\" module encapsulation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-06-15T05:20:04Z","receivedAt":"2015-06-15T05:20:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Jun 13, 2015 at 7:42 AM, Michael Haggerty wrote:\n> This is v2 of this patch series. I think I have addressed all of the\n> feedback from v1 [1]. Thanks to Stefan, Peff and Junio for their\n> feedback.\n\nThis version looks good to me.\n\n>\n> Changes since v1:\n>\n> * Change docstring for delete_ref() and a comment within the function\n> definition.\n>\n> * Squash together two commits dealing with the error message in\n> delete_refs().\n>\n> These patches are also available from my GitHub account [2] as branch\n> \"init-delete-refs-api\".\n>\n> Michael\n>\n> [1] http://thread.gmane.org/gmane.comp.version-control.git/271017\n> [2] https://github.com/mhagger/git\n>\n> Michael Haggerty (12):\n> delete_ref(): move declaration to refs.h\n> remove_branches(): remove temporary\n> delete_ref(): handle special case more explicitly\n> delete_refs(): new function for the refs API\n> delete_refs(): improve error message\n> prune_remote(): use delete_refs()\n> repack_without_refs(): make function private\n> initial_ref_transaction_commit(): function for initial ref creation\n> refs: remove some functions from the module's public interface\n> initial_ref_transaction_commit(): check for duplicate refs\n> initial_ref_transaction_commit(): check for ref D/F conflicts\n> refs: move the remaining ref module declarations to refs.h\n>\n> archive.c | 1 +\n> builtin/blame.c | 1 +\n> builtin/clone.c | 19 ++++-\n> builtin/fast-export.c | 1 +\n> builtin/fmt-merge-msg.c | 1 +\n> builtin/init-db.c | 1 +\n> builtin/log.c | 1 +\n> builtin/remote.c | 33 +-------\n> cache.h | 68 ----------------\n> refs.c | 171 ++++++++++++++++++++++++++++++++++++---\n> refs.h | 211 +++++++++++++++++++++++++++++++-----------------\n> remote-testsvn.c | 1 +\n> 12 files changed, 321 insertions(+), 188 deletions(-)\n>\n> --\n> 2.1.4\n>\n"},{"id":"263871","messageId":"xmqqk2v4alvn.fsf@gitster.dls.corp.google.com","threadId":"39616","inReplyTo":"19f421f504ae22538fd2a902840209c0799e67cb.1434206062.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 08/12] initial_ref_transaction_commit(): function for initial ref creation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-15T18:04:44Z","receivedAt":"2015-06-15T18:04:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> +\tstruct ref_transaction *t;\n> +\tstruct strbuf err = STRBUF_INIT;\n> +\n> +\tt = ref_transaction_begin(&err);\n> +\tif (!t)\n> +\t\tdie(err.buf);\n> \n> \tfor (r = local_refs; r; r = r->next) {\n> \t\tif (!r->peer_ref)\n> \t\t\tcontinue;\n> -\t\tadd_packed_ref(r->peer_ref->name, r->old_sha1);\n> +\t\tif (ref_transaction_create(t, r->peer_ref->name, r->old_sha1,\n> +\t\t\t\t\t 0, NULL, &err))\n> +\t\t\tdie(err.buf);\n> +\t}\n> +\n> +\tif (initial_ref_transaction_commit(t, &err)) {\n> +\t\tdie(err.buf);\n> \t}\n\nI'll remove {} around this as you do not seem to touch this part to\nadd more statement here.\n\nThe series makes sense so far ;-)\n"},{"id":"263872","messageId":"xmqqfv5salh9.fsf@gitster.dls.corp.google.com","threadId":"39616","inReplyTo":"79947baebd702f8e9fe9b667e2eeb69afa73094b.1434206062.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 12/12] refs: move the remaining ref module declarations to refs.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-15T18:13:22Z","receivedAt":"2015-06-15T18:13:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Some functions from the refs module were still declared in cache.h.\n> Move them to refs.h.\n\nYay. But...\n\n> Add some parameter names where they were missing.\n\n... I do not think some of this is a good change.\n\n> @@ -78,15 +170,15 @@ typedef int each_ref_fn(const char *refname,\n> * modifies the reference also returns a nonzero value to immediately\n> * stop the iteration.\n> */\n> -extern int head_ref(each_ref_fn, void *);\n> +extern int head_ref(each_ref_fn fn, void *cb_data);\n\nFor example, between these two, what did we gain?\n\nBecause of their types, it already was clear what the two parameters\nare in the original, without noisewords like \"fn\" (which obviously\nstands for a \"function\", but that is clear due to \"each_ref_fn\").\n\n> -extern int for_each_glob_ref_in(each_ref_fn, const char *pattern, const char* prefix, void *);\n> +extern int for_each_glob_ref_in(each_ref_fn fn, const char *pattern, const char* prefix, void *cb_data);\n\nLikewise for addition of fn and cb_data.\n\nIf you really want to make unrelated changes to this file, what you\nshould fix is to update \"const char* prefix\" to \"const char *prefix\"\n;-)\n"},{"id":"263873","messageId":"xmqqbnggal2a.fsf@gitster.dls.corp.google.com","threadId":"39616","inReplyTo":"0c66a6bd6e49b9c28fa3617ee50de746ef4ea9a3.1434206062.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 03/12] delete_ref(): handle special case more explicitly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-15T18:22:21Z","receivedAt":"2015-06-15T18:22:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> delete_ref() uses a different convention for its old_sha1 parameter\n> than, say, ref_transaction_delete(): NULL_SHA1 means not to check the\n> old value. Make this fact a little bit clearer in the code by handling\n> it in explicit, commented code rather than burying it in a conditional\n> expression.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n\nI think this is a very sensible first step, but I wonder if\ndelete_ref() should be taught the difference between \"we do not\nexpect anything\" and \"we expect it not to exist\".\n\n> refs.c | 11 +++++++++--\n> 1 file changed, 9 insertions(+), 2 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index b575bb8..f0b6cec 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2795,10 +2795,17 @@ int delete_ref(const char *refname, const unsigned char *old_sha1,\n> \tstruct ref_transaction *transaction;\n> \tstruct strbuf err = STRBUF_INIT;\n> \n> +\t/*\n> +\t * Treat NULL_SHA1 and NULL alike, to mean \"we don't care what\n> +\t * the old value of the reference was (or even if it didn't\n> +\t * exist)\":\n> +\t */\n> +\tif (old_sha1 && is_null_sha1(old_sha1))\n> +\t\told_sha1 = NULL;\n> +\n> \ttransaction = ref_transaction_begin(&err);\n> \tif (!transaction ||\n> -\t ref_transaction_delete(transaction, refname,\n> -\t\t\t\t (old_sha1 && !is_null_sha1(old_sha1)) ? old_sha1 : NULL,\n> +\t ref_transaction_delete(transaction, refname, old_sha1,\n> \t\t\t\t flags, NULL, &err) ||\n> \t ref_transaction_commit(transaction, &err)) {\n> \t\terror(\"%s\", err.buf);\n"},{"id":"263876","messageId":"xmqq7fr4akr7.fsf@gitster.dls.corp.google.com","threadId":"39616","inReplyTo":"64949bba1031b19abf86629267b8bc40666a4631.1434206062.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 05/12] delete_refs(): improve error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-15T18:29:00Z","receivedAt":"2015-06-15T18:29:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Subject: Re: [PATCH v2 05/12] delete_refs(): improve error message\n\nI'd call this \"make error message more generic\".\n\n> Change the error message from\n>\n> Could not remove branch %s\n>\n> to\n>\n> could not remove reference %s\n>\n> * s/branch/reference/. This change makes sense even for the existing\n> caller, which uses the function to delete remote-tracking\n> branches.\n\nand replace this bullet with something like:\n\n* Originally 'branch -d' was the only caller of this, but as part\n of the refs API, we will allow it to be called on refs that is not\n a branch or a remote-tracking branch.\n\nas calling a remote-tracking branch a 'branch' is not incorrect\nper-se. What would count as true improvement is ...\n\n> * Convert it to lower case, as per our usual convention.\n\n... this one. If somebody eventually chooses to make the message\nfiner grained by switching on the prefix refs/{what}, so that the\nuser can differentiate between branches, remote-tracking branches,\ntags, etc., that would also count as improvement as well.\n\n>\n> Signed-off-by: Michael Haggerty \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 6f62bd1..5386077 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2831,7 +2831,7 @@ int delete_refs(struct string_list *refnames)\n> \t\tconst char *refname = refnames->items[i].string;\n> \n> \t\tif (delete_ref(refname, NULL, 0))\n> -\t\t\tresult |= error(_(\"Could not remove branch %s\"), refname);\n> +\t\t\tresult |= error(_(\"could not remove reference %s\"), refname);\n> \t}\n> \n> \treturn result;\n"},{"id":"263877","messageId":"20150615183504.GB4041@peff.net","threadId":"39616","inReplyTo":"xmqqfv5salh9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 12/12] refs: move the remaining ref module declarations to refs.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-15T18:35:05Z","receivedAt":"2015-06-15T18:35:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 15, 2015 at 11:13:22AM -0700, Junio C Hamano wrote:\n\n> > @@ -78,15 +170,15 @@ typedef int each_ref_fn(const char *refname,\n> > * modifies the reference also returns a nonzero value to immediately\n> > * stop the iteration.\n> > */\n> > -extern int head_ref(each_ref_fn, void *);\n> > +extern int head_ref(each_ref_fn fn, void *cb_data);\n> \n> For example, between these two, what did we gain?\n> \n> Because of their types, it already was clear what the two parameters\n> are in the original, without noisewords like \"fn\" (which obviously\n> stands for a \"function\", but that is clear due to \"each_ref_fn\").\n\nI think the real benefit of naming parameters is that you can talk about\n\"fn\" and \"cb_data\" by name in the docstring[1]. Of course we do not do\nthat here, so we could clearly wait until \"if-and-when\" we do so. But I\ndo not think it is a big deal for our style guide to say \"we always name\nparameters in declarations\", and to bring things in line there (though\nperhaps it should be a separate patch in that case).\n\n[1] For instance, in the docstring here, which is just outside the\n context, we use the awkward phrase \"the specified callback\n function\". That would be much simpler as just `fn`, though having so\n few parameters to these functions, it is fairly clear already.\n\n> > -extern int for_each_glob_ref_in(each_ref_fn, const char *pattern, const char* prefix, void *);\n> > +extern int for_each_glob_ref_in(each_ref_fn fn, const char *pattern, const char* prefix, void *cb_data);\n> \n> Likewise for addition of fn and cb_data.\n> \n> If you really want to make unrelated changes to this file, what you\n> should fix is to update \"const char* prefix\" to \"const char *prefix\"\n> ;-)\n\nIMHO they are in the same boat (style fixes), and I would be happy to\nsee both improved. :)\n\n-Peff\n"},{"id":"263878","messageId":"xmqq381sakgf.fsf@gitster.dls.corp.google.com","threadId":"39616","inReplyTo":"aab9d191609b34fbfee0b68883a5b14fba2421ad.1434206062.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 06/12] prune_remote(): use delete_refs()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-15T18:35:28Z","receivedAt":"2015-06-15T18:35:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> This will result in errors being emitted for references that can't be\n> deleted, but that is a good thing.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> builtin/remote.c | 11 ++---------\n> 1 file changed, 2 insertions(+), 9 deletions(-)\n>\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index c8dc724..cc3c741 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -1314,19 +1314,12 @@ static int prune_remote(const char *remote, int dry_run)\n> \t\tstring_list_append(&refs_to_prune, item->util);\n> \tstring_list_sort(&refs_to_prune);\n> \n> -\tif (!dry_run) {\n> -\t\tstruct strbuf err = STRBUF_INIT;\n> -\t\tif (repack_without_refs(&refs_to_prune, &err))\n> -\t\t\tresult |= error(\"%s\", err.buf);\n> -\t\tstrbuf_release(&err);\n> -\t}\n> +\tif (!dry_run)\n> +\t\tresult |= delete_refs(&refs_to_prune);\n> \n> \tfor_each_string_list_item(item, &states.stale) {\n> \t\tconst char *refname = item->util;\n> \n> -\t\tif (!dry_run)\n> -\t\t\tresult |= delete_ref(refname, NULL, 0);\n> -\n> \t\tif (dry_run)\n> \t\t\tprintf_ln(_(\" * [would prune] %s\"),\n> \t\t\t abbrev_ref(refname, \"refs/remotes/\"));\n\nThe resulting code reads better by making the for-each-string-list-item\nloop only about reporting and not actually doing anything.\n\nBut the log message puzzles me. Didn't refs that cannot be deleted\ncause the original to fail? After repacking without these refs, it\ncalled delete-ref, and a failure to delete or commit the deletion\nwould have hit the error() down there, no?\n"},{"id":"263880","messageId":"20150615183906.GA10080@peff.net","threadId":"39616","inReplyTo":"xmqq381sakgf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 06/12] prune_remote(): use delete_refs()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-15T18:39:06Z","receivedAt":"2015-06-15T18:39:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 15, 2015 at 11:35:28AM -0700, Junio C Hamano wrote:\n\n> But the log message puzzles me. Didn't refs that cannot be deleted\n> cause the original to fail? After repacking without these refs, it\n> called delete-ref, and a failure to delete or commit the deletion\n> would have hit the error() down there, no?\n\nI think this discussion:\n\n http://thread.gmane.org/gmane.comp.version-control.git/271017/focus=271164\n\nfrom v1 is relevant.\n\n-Peff\n"},{"id":"263879","messageId":"xmqqy4jk95pw.fsf@gitster.dls.corp.google.com","threadId":"39616","inReplyTo":"19f421f504ae22538fd2a902840209c0799e67cb.1434206062.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 08/12] initial_ref_transaction_commit(): function for initial ref creation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-15T18:39:07Z","receivedAt":"2015-06-15T18:39:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> It would seem to make sense to add a test here that there are no\n> existing references, because that is how the function *should* be\n> used. But in fact, the \"testgit\" remote helper appears to call it\n> *after* having set up refs/remotes//HEAD and\n> refs/remotes//master, so we can't be so strict.\n\nIs \"testgit\" so hard to fix (or so sacred to break) that we want to\nsacrifice the quality of this part that is nearer to the core?\n\n> For now, the\n> function trusts its caller to only call it when it makes sense. Future\n> commits will add some more limited sanity checks.\n\nOK.\n\nThanks.\n"},{"id":"263883","messageId":"xmqqioao952n.fsf@gitster.dls.corp.google.com","threadId":"39616","inReplyTo":"19f421f504ae22538fd2a902840209c0799e67cb.1434206062.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 08/12] initial_ref_transaction_commit(): function for initial ref creation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-15T18:53:04Z","receivedAt":"2015-06-15T18:53:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> +\tstruct ref_transaction *t;\n> +\tstruct strbuf err = STRBUF_INIT;\n> +\n> +\tt = ref_transaction_begin(&err);\n> +\tif (!t)\n> +\t\tdie(err.buf);\n\nYikes, and sorry for sending three messages without consolidating\nagainst the same patch, but\n\n\tdie(\"%s\", err.buf);\n\nbecause\n\nextern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\n\nin its declaration causes -Werror=format-security to barf.\n\nLikewise for a few other instances of the same construct all in \nthe same file.\n"},{"id":"263884","messageId":"20150615185746.GB10080@peff.net","threadId":"39616","inReplyTo":"xmqqy4jk95pw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 08/12] initial_ref_transaction_commit(): function for initial ref creation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-15T18:57:46Z","receivedAt":"2015-06-15T18:57:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 15, 2015 at 11:39:07AM -0700, Junio C Hamano wrote:\n\n> Michael Haggerty writes:\n> \n> > It would seem to make sense to add a test here that there are no\n> > existing references, because that is how the function *should* be\n> > used. But in fact, the \"testgit\" remote helper appears to call it\n> > *after* having set up refs/remotes//HEAD and\n> > refs/remotes//master, so we can't be so strict.\n> \n> Is \"testgit\" so hard to fix (or so sacred to break) that we want to\n> sacrifice the quality of this part that is nearer to the core?\n\nI do not think \"testgit\" is sacred; it is there to serve the needs of\ngit and not the other way around. But we do have to stop and think\nwhether what \"testgit\" is doing is representative of what a real\nremote-helper might do.\n\nIt has been a while since I touched the remote-helper code (and it has\nalways been under-documented and confusing, from my recollection), but I\nthink it is not \"testgit\" that is making the refs, but rather that\n\"import\"-style helpers will feed a data stream to git-fast-import\n(actually the transport code does it on behalf of the remote).\n\nSo \"fast-import\" creates the refs, and then \"clone\" later wants to\nrecreate them. IMHO this is not ideal[1]; we should have fast-import create\nthe objects and report the ref tips, which can then be relayed to clone.\nBut with the way the code works now, this is not about fixing \"testgit\",\nbut rather would break any \"import\"-style helper.\n\nTake all of that with a grain of salt, though. I might be totally wrong\nabout how this part of the code works.\n\n-Peff\n\n[1] This may also be buggy for regular fetches. E.g., do we correctly\n respect \"--force\" (or lack thereof) and \"+\"-refspecs when importing?\n I am not sure, as I think we may pass enough information to the\n helper to handle this itself. But certainly if the responsibility\n for updating refs was always in the caller (clone or fetch), then it\n would follow the regular code path and Just Work.\n"},{"id":"263893","messageId":"CAPc5daXJ-FJ_1=EJ8+5t+-fY-w1cQraKYUrvqTu_rXptx24frw@mail.gmail.com","threadId":"39616","inReplyTo":"20150615183906.GA10080@peff.net","subject":"Re: [PATCH v2 06/12] prune_remote(): use delete_refs()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-15T19:45:30Z","receivedAt":"2015-06-15T19:45:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Mon, Jun 15, 2015 at 11:39 AM, Jeff King wrote:\n> On Mon, Jun 15, 2015 at 11:35:28AM -0700, Junio C Hamano wrote:\n>\n>> But the log message puzzles me. Didn't refs that cannot be deleted\n>> cause the original to fail? After repacking without these refs, it\n>> called delete-ref, and a failure to delete or commit the deletion\n>> would have hit the error() down there, no?\n>\n> I think this discussion:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/271017/focus=271164\n>\n> from v1 is relevant.\n\nAh, OK, that makes sense; the log message may want to explain that though.\n"},{"id":"263894","messageId":"CAPc5daXrHiRjZ=dFH3Mp4tcoRAEABS5_1vz7J+vgGgBmxyiqXw@mail.gmail.com","threadId":"39616","inReplyTo":"20150615183504.GB4041@peff.net","subject":"Re: [PATCH v2 12/12] refs: move the remaining ref module declarations to refs.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-15T19:48:02Z","receivedAt":"2015-06-15T19:48:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Mon, Jun 15, 2015 at 11:35 AM, Jeff King wrote:\n>\n> I think the real benefit of naming parameters is that you can talk about\n> \"fn\" and \"cb_data\" by name in the docstring[1].\n\nYes, I would think it is a very sensible thing to do, if a patch that adds\ndocstring adds names so that the documentation can refer to them.\n\nBut otherwise, the patch presented here adds only noise without\nadding much value.\n"},{"id":"264332","messageId":"5584253E.9030001@alum.mit.edu","threadId":"39616","inReplyTo":"xmqq7fr4akr7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 05/12] delete_refs(): improve error message","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-19T14:20:46Z","receivedAt":"2015-06-19T14:20:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/15/2015 08:29 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> Subject: Re: [PATCH v2 05/12] delete_refs(): improve error message\n> \n> I'd call this \"make error message more generic\".\n> \n>> Change the error message from\n>>\n>> Could not remove branch %s\n>>\n>> to\n>>\n>> could not remove reference %s\n>>\n>> * s/branch/reference/. This change makes sense even for the existing\n>> caller, which uses the function to delete remote-tracking\n>> branches.\n> \n> and replace this bullet with something like:\n> \n> * Originally 'branch -d' was the only caller of this, but as part\n> of the refs API, we will allow it to be called on refs that is not\n> a branch or a remote-tracking branch.\n> \n> as calling a remote-tracking branch a 'branch' is not incorrect\n> per-se.\n\nTrue, but \"branch refs/remotes/origin/foo\" is not how we would usually\nrefer to that reference. Usually we would say \"reference\nrefs/remotes/origin/foo\" or \"remote-tracking branch origin/foo\". But I\nwill make approximately the change that you suggest.\n\n> What would count as true improvement is ...\n> \n>> * Convert it to lower case, as per our usual convention.\n> \n> ... this one. If somebody eventually chooses to make the message\n> finer grained by switching on the prefix refs/{what}, so that the\n> user can differentiate between branches, remote-tracking branches,\n> tags, etc., that would also count as improvement as well.\n\nI thought about proposing a function that changes a reference name into\nits prose equivalent, but I'm pretty sure that the result would not be\ninternationalizable. Probably such a function would have to look like\n\nenum refname_kind { REFNAME_KIND_BRANCH, REFNAME_KIND_TAG,\nREFNAME_KIND_REMOTE, ...(?), REFNAME_KIND_OTHER};\n\nenum refname_kind shorten_refname(const char *refname, const char\n**short_refname);\n\nand callers would still need logic to pick the correct\n(internationalized) formatting string based on the return value of the\nfunction.\n\nSomeday...\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"264392","messageId":"5584C789.7020209@alum.mit.edu","threadId":"39616","inReplyTo":"20150615183504.GB4041@peff.net","subject":"Re: [PATCH v2 12/12] refs: move the remaining ref module declarations to refs.h","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-20T01:53:13Z","receivedAt":"2015-06-20T01:53:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/15/2015 08:35 PM, Jeff King wrote:\n> On Mon, Jun 15, 2015 at 11:13:22AM -0700, Junio C Hamano wrote:\n> \n>>> @@ -78,15 +170,15 @@ typedef int each_ref_fn(const char *refname,\n>>> * modifies the reference also returns a nonzero value to immediately\n>>> * stop the iteration.\n>>> */\n>>> -extern int head_ref(each_ref_fn, void *);\n>>> +extern int head_ref(each_ref_fn fn, void *cb_data);\n>>\n>> For example, between these two, what did we gain?\n>>\n>> Because of their types, it already was clear what the two parameters\n>> are in the original, without noisewords like \"fn\" (which obviously\n>> stands for a \"function\", but that is clear due to \"each_ref_fn\").\n> \n> I think the real benefit of naming parameters is that you can talk about\n> \"fn\" and \"cb_data\" by name in the docstring[1]. Of course we do not do\n> that here, so we could clearly wait until \"if-and-when\" we do so. But I\n> do not think it is a big deal for our style guide to say \"we always name\n> parameters in declarations\", and to bring things in line there (though\n> perhaps it should be a separate patch in that case).\n\nI also like the idea that all parameters in declarations should be\nnamed. It is true that sometimes the purpose of a parameter is easy to\nguess from its type without a name. But giving them names make them\neasier to talk about in docstrings, in commit messages, or even on the\nmailing list when reviewing patches or discussing bugs.\n\nMoreover, I don't see the value of wasting mental energy wondering\n\"hmmm, in this case is the meaning sufficiently obvious from the type?\"\nevery time I write a function declaration. It is easier to have a\nconsistent policy of putting them in.\n\nFinally, when I'm inventing new functions (which I admit isn't the case\nhere), I usually write the declaration first and then copy-paste it to\nthe C file as the first step in writing the definition. If I name the\nparameters in the declaration then (a) I don't have to go back and edit\nthem at the definition and (b) if parameter names are needed later at\nthe declaration (e.g., for a docstring), I don't have to edit the\ndeclaration again, cross-referencing back to the C file to make sure I\nname them consistently.\n\nI will split the \"add names in declarations\" changes into a separate\npatch. Also, that way Junio can ignore the patch if he still disagrees :-)\n\n>>> -extern int for_each_glob_ref_in(each_ref_fn, const char *pattern, const char* prefix, void *);\n>>> +extern int for_each_glob_ref_in(each_ref_fn fn, const char *pattern, const char* prefix, void *cb_data);\n>>\n>> Likewise for addition of fn and cb_data.\n>>\n>> If you really want to make unrelated changes to this file, what you\n>> should fix is to update \"const char* prefix\" to \"const char *prefix\"\n>> ;-)\n> \n> IMHO they are in the same boat (style fixes), and I would be happy to\n> see both improved. :)\n\nI will fix this too.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"264393","messageId":"5584CCD9.9030107@alum.mit.edu","threadId":"39616","inReplyTo":"xmqqioao952n.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 08/12] initial_ref_transaction_commit(): function for initial ref creation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-06-20T02:15:53Z","receivedAt":"2015-06-20T02:15:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/15/2015 08:53 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> +\tstruct ref_transaction *t;\n>> +\tstruct strbuf err = STRBUF_INIT;\n>> +\n>> +\tt = ref_transaction_begin(&err);\n>> +\tif (!t)\n>> +\t\tdie(err.buf);\n> \n> Yikes, and sorry for sending three messages without consolidating\n> against the same patch, but\n> \n> \tdie(\"%s\", err.buf);\n> \n> because\n> \n> extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\n> \n> in its declaration causes -Werror=format-security to barf.\n> \n> Likewise for a few other instances of the same construct all in \n> the same file.\n\nThanks for catching this. I'll fix it in the re-roll (and also add that\ngcc option to my config.mak for the future).\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"264396","messageId":"xmqq381n5a5g.fsf@gitster.dls.corp.google.com","threadId":"39616","inReplyTo":"5584C789.7020209@alum.mit.edu","subject":"Re: [PATCH v2 12/12] refs: move the remaining ref module declarations to refs.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-20T03:30:35Z","receivedAt":"2015-06-20T03:30:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Finally, when I'm inventing new functions (which I admit isn't the case\n> here), I usually write the declaration first and then copy-paste it to\n> the C file as the first step in writing the definition. If I name the\n> parameters in the declaration then (a) I don't have to go back and edit\n> them at the definition and (b) if parameter names are needed later at\n> the declaration (e.g., for a docstring), I don't have to edit the\n> declaration again, cross-referencing back to the C file to make sure I\n> name them consistently.\n\nThat cuts both ways, though. After somebody decides to rename\nparameters that used to be \"ref\" after adding tons of other\nfunctions and names parameters to these new functions \"refname\", you\nhave two places to update \"ref\" to \"refname\" for existing ones, not\none.\n\n> I will split the \"add names in declarations\" changes into a separate\n> patch.\n\nYeah, please do so. I do not mind \"add docstring and add names in\ndeclarations so that docstring can refer to them\" patch at all, but\nthat is a step separate from the \"move the remaining declarations\".\n"}]}