{"thread":{"id":"39295","subject":"[PATCH v2 02/18] is_refname_available(): revamp the comments","startedAt":"2015-05-11T15:25:02Z","lastAt":"2015-05-23T00:52:47Z","messageCount":33,"participants":["Michael Haggerty","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":2,"patchTotal":18},"messages":[{"id":"260973","messageId":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":null,"subject":"[PATCH v2 00/18] Improve handling of D/F conflicts","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:02Z","receivedAt":"2015-05-11T15:25:02Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is a re-roll of [1]. This patch series improves the handling of\nconflicts between references like \"refs/foo\" and \"refs/foo/bar\" that\nare not allowed to co-exist. It also changes some functions in this\narea to record their error messages in a \"struct strbuf *err\" argument\nrather than sometimes emitting errors directly to stderr.\n\nChanges relative to v1:\n\n* Rebase to master rather than depending on\n mh/ref-lock-avoid-running-out-of-fds, as did v1. There were no\n significant dependencies between the two patch series, and now that\n mh/ref-lock-avoid-running-out-of-fds (now renamed to\n mh/write-refs-sooner.*) has been backported to 2.2, the dependency\n makes even less sense.\n\n* Fix leak of \"reason\" string in ref_transaction_commit().\n\n* Fix a broken &&-chain in t1404 and fix the function definition as\n per Junio's \"SQUASH???\" commit.\n\nThanks to Stefan, Junio, and Eric for their comments regarding v1.\n\nAs usual, this patch series is also available from my GitHub\nrepository [2], as branch \"check-df-conflicts-earlier\".\n\nMichael\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/268117\n[2] https://github.com/mhagger/git\n\nMichael Haggerty (18):\n t1404: new tests of ref D/F conflicts within transactions\n is_refname_available(): revamp the comments\n is_refname_available(): avoid shadowing \"dir\" variable\n is_refname_available(): convert local variable \"dirname\" to strbuf\n entry_matches(): inline function\n report_refname_conflict(): inline function\n struct nonmatching_ref_data: store a refname instead of a ref_entry\n is_refname_available(): use dirname in first loop\n ref_transaction_commit(): use a string_list for detecting duplicates\n refs: check for D/F conflicts among refs created in a transaction\n verify_refname_available(): rename function\n verify_refname_available(): report errors via a \"struct strbuf *err\"\n lock_ref_sha1_basic(): report errors via a \"struct strbuf *err\"\n lock_ref_sha1_basic(): improve diagnostics for ref D/F conflicts\n rename_ref(): integrate lock_ref_sha1_basic() errors into ours\n ref_transaction_commit(): provide better error messages\n ref_transaction_commit(): delete extra \"the\" from error message\n reflog_expire(): integrate lock_ref_sha1_basic() errors into ours\n\n refs.c | 309 ++++++++++++++++++++++++-------------\n t/t1400-update-ref.sh | 14 +-\n t/t1404-update-ref-df-conflicts.sh | 107 +++++++++++++\n 3 files changed, 312 insertions(+), 118 deletions(-)\n create mode 100755 t/t1404-update-ref-df-conflicts.sh\n\n-- \n2.1.4\n"},{"id":"260975","messageId":"1431357920-25090-2-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:03Z","receivedAt":"2015-05-11T15:25:03Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add some tests of reference D/F conflicts (by which I mean the fact\nthat references like \"refs/foo\" and \"refs/foo/bar\" are not allowed to\ncoexist) in the context of reference transactions.\n\nThe test of creating two conflicting references in the same\ntransaction fails, leaving the transaction half-completed. This will\nbe fixed later in this patch series.\n\nPlease note that the error messages emitted in the case of conflicts\nare not very user-friendly. In particular, when the conflicts involve\nloose references, then the errors are reported as\n\n error: there are still refs under 'refs/foo'\n fatal: Cannot lock the ref 'refs/foo'.\n\nor\n\n error: unable to resolve reference refs/foo/bar: Not a directory\n fatal: Cannot lock the ref 'refs/foo/bar'.\n\nThis is because lock_ref_sha1_basic() fails while trying to lock the\nnew reference, before it even gets to the is_refname_available()\ncheck. This situation will also be improved later in this patch\nseries.\n\nSigned-off-by: Michael Haggerty \n---\n t/t1404-update-ref-df-conflicts.sh | 107 +++++++++++++++++++++++++++++++++++++\n 1 file changed, 107 insertions(+)\n create mode 100755 t/t1404-update-ref-df-conflicts.sh\n\ndiff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh\nnew file mode 100755\nindex 0000000..ed1b93e\n--- /dev/null\n+++ b/t/t1404-update-ref-df-conflicts.sh\n@@ -0,1 +1,107 @@\n+#!/bin/sh\n+\n+test_description='Test git update-ref with D/F conflicts'\n+. ./test-lib.sh\n+\n+test_update_rejected () {\n+\tprefix=\"$1\" &&\n+\tbefore=\"$2\" &&\n+\tpack=\"$3\" &&\n+\tcreate=\"$4\" &&\n+\terror=\"$5\" &&\n+\tprintf \"create $prefix/%s $C\\n\" $before |\n+\tgit update-ref --stdin &&\n+\tgit for-each-ref $prefix >unchanged &&\n+\tif $pack\n+\tthen\n+\t\tgit pack-refs --all\n+\tfi &&\n+\tprintf \"create $prefix/%s $C\\n\" $create >input &&\n+\ttest_must_fail git update-ref --stdin output.err &&\n+\tgrep -F \"$error\" output.err &&\n+\tgit for-each-ref $prefix >actual &&\n+\ttest_cmp unchanged actual\n+}\n+\n+Q=\"'\"\n+\n+test_expect_success 'setup' '\n+\n+\tgit commit --allow-empty -m Initial &&\n+\tC=$(git rev-parse HEAD)\n+\n+'\n+\n+test_expect_success 'existing loose ref is a simple prefix of new' '\n+\n+\tprefix=refs/1l &&\n+\ttest_update_rejected $prefix \"a c e\" false \"b c/x d\" \\\n+\t\t\"unable to resolve reference $prefix/c/x: Not a directory\"\n+\n+'\n+\n+test_expect_success 'existing packed ref is a simple prefix of new' '\n+\n+\tprefix=refs/1p &&\n+\ttest_update_rejected $prefix \"a c e\" true \"b c/x d\" \\\n+\t\t\"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x$Q\"\n+\n+'\n+\n+test_expect_success 'existing loose ref is a deeper prefix of new' '\n+\n+\tprefix=refs/2l &&\n+\ttest_update_rejected $prefix \"a c e\" false \"b c/x/y d\" \\\n+\t\t\"unable to resolve reference $prefix/c/x/y: Not a directory\"\n+\n+'\n+\n+test_expect_success 'existing packed ref is a deeper prefix of new' '\n+\n+\tprefix=refs/2p &&\n+\ttest_update_rejected $prefix \"a c e\" true \"b c/x/y d\" \\\n+\t\t\"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x/y$Q\"\n+\n+'\n+\n+test_expect_success 'new ref is a simple prefix of existing loose' '\n+\n+\tprefix=refs/3l &&\n+\ttest_update_rejected $prefix \"a c/x e\" false \"b c d\" \\\n+\t\t\"there are still refs under $Q$prefix/c$Q\"\n+\n+'\n+\n+test_expect_success 'new ref is a simple prefix of existing packed' '\n+\n+\tprefix=refs/3p &&\n+\ttest_update_rejected $prefix \"a c/x e\" true \"b c d\" \\\n+\t\t\"$Q$prefix/c/x$Q exists; cannot create $Q$prefix/c$Q\"\n+\n+'\n+\n+test_expect_success 'new ref is a deeper prefix of existing loose' '\n+\n+\tprefix=refs/4l &&\n+\ttest_update_rejected $prefix \"a c/x/y e\" false \"b c d\" \\\n+\t\t\"there are still refs under $Q$prefix/c$Q\"\n+\n+'\n+\n+test_expect_success 'new ref is a deeper prefix of existing packed' '\n+\n+\tprefix=refs/4p &&\n+\ttest_update_rejected $prefix \"a c/x/y e\" true \"b c d\" \\\n+\t\t\"$Q$prefix/c/x/y$Q exists; cannot create $Q$prefix/c$Q\"\n+\n+'\n+\n+test_expect_failure 'one new ref is a simple prefix of another' '\n+\n+\tprefix=refs/5 &&\n+\ttest_update_rejected $prefix \"a e\" false \"b c c/x d\" \\\n+\t\t\"cannot process $Q$prefix/c$Q and $Q$prefix/c/x$Q at the same time\"\n+\n+'\n+\n+test_done\n-- \n2.1.4\n"},{"id":"260972","messageId":"1431357920-25090-3-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 02/18] is_refname_available(): revamp the comments","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:04Z","receivedAt":"2015-05-11T15:25:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change the comments to a running example of running the function with\nrefname set to \"refs/foo/bar\". Add some more explanation of the logic.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 69 +++++++++++++++++++++++++++++++++++++++++++++---------------------\n 1 file changed, 47 insertions(+), 22 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 47e4e53..776bbce 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -876,9 +876,9 @@ static void report_refname_conflict(struct ref_entry *entry,\n * operation).\n *\n * Two reference names conflict if one of them exactly matches the\n- * leading components of the other; e.g., \"foo/bar\" conflicts with\n- * both \"foo\" and with \"foo/bar/baz\" but not with \"foo/bar\" or\n- * \"foo/barbados\".\n+ * leading components of the other; e.g., \"refs/foo/bar\" conflicts\n+ * with both \"refs/foo\" and with \"refs/foo/bar/baz\" but not with\n+ * \"refs/foo/bar\" or \"refs/foo/barbados\".\n *\n * skip must be sorted.\n */\n@@ -891,19 +891,39 @@ static int is_refname_available(const char *refname,\n \tint pos;\n \tchar *dirname;\n \n+\t/*\n+\t * For the sake of comments in this function, suppose that\n+\t * refname is \"refs/foo/bar\".\n+\t */\n+\n \tfor (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {\n \t\t/*\n-\t\t * We are still at a leading dir of the refname; we are\n-\t\t * looking for a conflict with a leaf entry.\n-\t\t *\n-\t\t * If we find one, we still must make sure it is\n-\t\t * not in \"skip\".\n+\t\t * We are still at a leading dir of the refname (e.g.,\n+\t\t * \"refs/foo\"; if there is a reference with that name,\n+\t\t * it is a conflict, *unless* it is in skip.\n \t\t */\n \t\tpos = search_ref_dir(dir, refname, slash - refname);\n \t\tif (pos >= 0) {\n+\t\t\t/*\n+\t\t\t * We found a reference whose name is a proper\n+\t\t\t * prefix of refname; e.g., \"refs/foo\".\n+\t\t\t */\n \t\t\tstruct ref_entry *entry = dir->entries[pos];\n-\t\t\tif (entry_matches(entry, skip))\n+\t\t\tif (entry_matches(entry, skip)) {\n+\t\t\t\t/*\n+\t\t\t\t * The reference we just found, e.g.,\n+\t\t\t\t * \"refs/foo\", is also in skip, so it\n+\t\t\t\t * is not considered a conflict.\n+\t\t\t\t * Moreover, the fact that \"refs/foo\"\n+\t\t\t\t * exists means that there cannot be\n+\t\t\t\t * any references anywhere under the\n+\t\t\t\t * \"refs/foo/\" namespace (because they\n+\t\t\t\t * would have conflicted with\n+\t\t\t\t * \"refs/foo\"). So we can stop looking\n+\t\t\t\t * now and return true.\n+\t\t\t\t */\n \t\t\t\treturn 1;\n+\t\t\t}\n \t\t\treport_refname_conflict(entry, refname);\n \t\t\treturn 0;\n \t\t}\n@@ -911,19 +931,29 @@ static int is_refname_available(const char *refname,\n \n \t\t/*\n \t\t * Otherwise, we can try to continue our search with\n-\t\t * the next component; if we come up empty, we know\n-\t\t * there is nothing under this whole prefix.\n+\t\t * the next component. So try to look up the\n+\t\t * directory, e.g., \"refs/foo/\".\n \t\t */\n \t\tpos = search_ref_dir(dir, refname, slash + 1 - refname);\n-\t\tif (pos < 0)\n+\t\tif (pos < 0) {\n+\t\t\t/*\n+\t\t\t * There was no directory \"refs/foo/\", so\n+\t\t\t * there is nothing under this whole prefix,\n+\t\t\t * and we are OK.\n+\t\t\t */\n \t\t\treturn 1;\n+\t\t}\n \n \t\tdir = get_ref_dir(dir->entries[pos]);\n \t}\n \n \t/*\n-\t * We are at the leaf of our refname; we want to\n-\t * make sure there are no directories which match it.\n+\t * We are at the leaf of our refname (e.g., \"refs/foo/bar\").\n+\t * There is no point in searching for a reference with that\n+\t * name, because a refname isn't considered to conflict with\n+\t * itself. But we still need to check for references whose\n+\t * names are in the \"refs/foo/bar/\" namespace, because they\n+\t * *do* conflict.\n \t */\n \tlen = strlen(refname);\n \tdirname = xmallocz(len + 1);\n@@ -933,9 +963,9 @@ static int is_refname_available(const char *refname,\n \n \tif (pos >= 0) {\n \t\t/*\n-\t\t * We found a directory named \"refname\". It is a\n-\t\t * problem iff it contains any ref that is not\n-\t\t * in \"skip\".\n+\t\t * We found a directory named \"$refname/\" (e.g.,\n+\t\t * \"refs/foo/bar/\"). It is a problem iff it contains\n+\t\t * any ref that is not in \"skip\".\n \t\t */\n \t\tstruct ref_entry *entry = dir->entries[pos];\n \t\tstruct ref_dir *dir = get_ref_dir(entry);\n@@ -950,11 +980,6 @@ static int is_refname_available(const char *refname,\n \t\treturn 0;\n \t}\n \n-\t/*\n-\t * There is no point in searching for another leaf\n-\t * node which matches it; such an entry would be the\n-\t * ref we are looking for, not a conflict.\n-\t */\n \treturn 1;\n }\n \n-- \n2.1.4\n"},{"id":"260977","messageId":"1431357920-25090-4-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 03/18] is_refname_available(): avoid shadowing \"dir\" variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:05Z","receivedAt":"2015-05-11T15:25:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The function had a \"dir\" parameter that was shadowed by a local \"dir\"\nvariable within a code block. Use the former in place of the latter.\n(This is consistent with \"dir\"'s use elsewhere in the function.)\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 776bbce..9d87e84 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -967,10 +967,10 @@ static int is_refname_available(const char *refname,\n \t\t * \"refs/foo/bar/\"). It is a problem iff it contains\n \t\t * any ref that is not in \"skip\".\n \t\t */\n-\t\tstruct ref_entry *entry = dir->entries[pos];\n-\t\tstruct ref_dir *dir = get_ref_dir(entry);\n \t\tstruct nonmatching_ref_data data;\n+\t\tstruct ref_entry *entry = dir->entries[pos];\n \n+\t\tdir = get_ref_dir(entry);\n \t\tdata.skip = skip;\n \t\tsort_ref_dir(dir);\n \t\tif (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data))\n-- \n2.1.4\n"},{"id":"260976","messageId":"1431357920-25090-5-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 04/18] is_refname_available(): convert local variable \"dirname\" to strbuf","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:06Z","receivedAt":"2015-05-11T15:25:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This change wouldn't be worth it by itself, but in a moment we will\nuse the strbuf for more string juggling.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 12 +++++-------\n 1 file changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9d87e84..faabd68 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -887,9 +887,8 @@ static int is_refname_available(const char *refname,\n \t\t\t\tstruct ref_dir *dir)\n {\n \tconst char *slash;\n-\tsize_t len;\n \tint pos;\n-\tchar *dirname;\n+\tstruct strbuf dirname = STRBUF_INIT;\n \n \t/*\n \t * For the sake of comments in this function, suppose that\n@@ -955,11 +954,10 @@ static int is_refname_available(const char *refname,\n \t * names are in the \"refs/foo/bar/\" namespace, because they\n \t * *do* conflict.\n \t */\n-\tlen = strlen(refname);\n-\tdirname = xmallocz(len + 1);\n-\tsprintf(dirname, \"%s/\", refname);\n-\tpos = search_ref_dir(dir, dirname, len + 1);\n-\tfree(dirname);\n+\tstrbuf_addstr(&dirname, refname);\n+\tstrbuf_addch(&dirname, '/');\n+\tpos = search_ref_dir(dir, dirname.buf, dirname.len);\n+\tstrbuf_release(&dirname);\n \n \tif (pos >= 0) {\n \t\t/*\n-- \n2.1.4\n"},{"id":"260974","messageId":"1431357920-25090-6-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 05/18] entry_matches(): inline function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:07Z","receivedAt":"2015-05-11T15:25:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It wasn't pulling its weight. And in a moment we will need similar\ntests that take a refname rather than a ref_entry as parameter, which\nwould have made entry_matches() even less useful.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex faabd68..6bdd34f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -841,11 +841,6 @@ static void prime_ref_dir(struct ref_dir *dir)\n \t}\n }\n \n-static int entry_matches(struct ref_entry *entry, const struct string_list *list)\n-{\n-\treturn list && string_list_has_string(list, entry->name);\n-}\n-\n struct nonmatching_ref_data {\n \tconst struct string_list *skip;\n \tstruct ref_entry *found;\n@@ -855,7 +850,7 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)\n {\n \tstruct nonmatching_ref_data *data = vdata;\n \n-\tif (entry_matches(entry, data->skip))\n+\tif (data->skip && string_list_has_string(data->skip, entry->name))\n \t\treturn 0;\n \n \tdata->found = entry;\n@@ -908,7 +903,7 @@ static int is_refname_available(const char *refname,\n \t\t\t * prefix of refname; e.g., \"refs/foo\".\n \t\t\t */\n \t\t\tstruct ref_entry *entry = dir->entries[pos];\n-\t\t\tif (entry_matches(entry, skip)) {\n+\t\t\tif (skip && string_list_has_string(skip, entry->name)) {\n \t\t\t\t/*\n \t\t\t\t * The reference we just found, e.g.,\n \t\t\t\t * \"refs/foo\", is also in skip, so it\n-- \n2.1.4\n"},{"id":"260987","messageId":"1431357920-25090-7-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 06/18] report_refname_conflict(): inline function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:08Z","receivedAt":"2015-05-11T15:25:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It wasn't pulling its weight. And we are about to need code similar to\nthis where no ref_entry is available and with more diverse error\nmessages. Rather than try to generalize the function, just inline it.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 10 ++--------\n 1 file changed, 2 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6bdd34f..7422594 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -857,12 +857,6 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)\n \treturn 1;\n }\n \n-static void report_refname_conflict(struct ref_entry *entry,\n-\t\t\t\t const char *refname)\n-{\n-\terror(\"'%s' exists; cannot create '%s'\", entry->name, refname);\n-}\n-\n /*\n * Return true iff a reference named refname could be created without\n * conflicting with the name of an existing reference in dir. If\n@@ -918,7 +912,7 @@ static int is_refname_available(const char *refname,\n \t\t\t\t */\n \t\t\t\treturn 1;\n \t\t\t}\n-\t\t\treport_refname_conflict(entry, refname);\n+\t\t\terror(\"'%s' exists; cannot create '%s'\", entry->name, refname);\n \t\t\treturn 0;\n \t\t}\n \n@@ -969,7 +963,7 @@ static int is_refname_available(const char *refname,\n \t\tif (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data))\n \t\t\treturn 1;\n \n-\t\treport_refname_conflict(data.found, refname);\n+\t\terror(\"'%s' exists; cannot create '%s'\", data.found->name, refname);\n \t\treturn 0;\n \t}\n \n-- \n2.1.4\n"},{"id":"260989","messageId":"1431357920-25090-8-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 07/18] struct nonmatching_ref_data: store a refname instead of a ref_entry","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:09Z","receivedAt":"2015-05-11T15:25:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now that we don't need a ref_entry to pass to\nreport_refname_conflict(), it is sufficient to store the refname of\nthe conflicting reference.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 7422594..effd91a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -843,7 +843,7 @@ static void prime_ref_dir(struct ref_dir *dir)\n \n struct nonmatching_ref_data {\n \tconst struct string_list *skip;\n-\tstruct ref_entry *found;\n+\tconst char *conflicting_refname;\n };\n \n static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)\n@@ -853,7 +853,7 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)\n \tif (data->skip && string_list_has_string(data->skip, entry->name))\n \t\treturn 0;\n \n-\tdata->found = entry;\n+\tdata->conflicting_refname = entry->name;\n \treturn 1;\n }\n \n@@ -963,7 +963,8 @@ static int is_refname_available(const char *refname,\n \t\tif (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data))\n \t\t\treturn 1;\n \n-\t\terror(\"'%s' exists; cannot create '%s'\", data.found->name, refname);\n+\t\terror(\"'%s' exists; cannot create '%s'\",\n+\t\t data.conflicting_refname, refname);\n \t\treturn 0;\n \t}\n \n-- \n2.1.4\n"},{"id":"260978","messageId":"1431357920-25090-9-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 08/18] is_refname_available(): use dirname in first loop","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:10Z","receivedAt":"2015-05-11T15:25:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"In the first loop (over prefixes of refname), use dirname to keep\ntrack of the current prefix. This is not an improvement in itself, but\nin a moment we will start using dirname for a role where a\nNUL-terminated string is needed.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 40 ++++++++++++++++++++++++++--------------\n 1 file changed, 26 insertions(+), 14 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex effd91a..8316bb1 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -878,26 +878,30 @@ static int is_refname_available(const char *refname,\n \tconst char *slash;\n \tint pos;\n \tstruct strbuf dirname = STRBUF_INIT;\n+\tint ret = 0;\n \n \t/*\n \t * For the sake of comments in this function, suppose that\n \t * refname is \"refs/foo/bar\".\n \t */\n \n+\tstrbuf_grow(&dirname, strlen(refname) + 1);\n \tfor (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {\n+\t\t/* Expand dirname to the new prefix, not including the trailing slash: */\n+\t\tstrbuf_add(&dirname, refname + dirname.len, slash - refname - dirname.len);\n+\n \t\t/*\n \t\t * We are still at a leading dir of the refname (e.g.,\n \t\t * \"refs/foo\"; if there is a reference with that name,\n \t\t * it is a conflict, *unless* it is in skip.\n \t\t */\n-\t\tpos = search_ref_dir(dir, refname, slash - refname);\n+\t\tpos = search_ref_dir(dir, dirname.buf, dirname.len);\n \t\tif (pos >= 0) {\n \t\t\t/*\n \t\t\t * We found a reference whose name is a proper\n \t\t\t * prefix of refname; e.g., \"refs/foo\".\n \t\t\t */\n-\t\t\tstruct ref_entry *entry = dir->entries[pos];\n-\t\t\tif (skip && string_list_has_string(skip, entry->name)) {\n+\t\t\tif (skip && string_list_has_string(skip, dirname.buf)) {\n \t\t\t\t/*\n \t\t\t\t * The reference we just found, e.g.,\n \t\t\t\t * \"refs/foo\", is also in skip, so it\n@@ -910,10 +914,11 @@ static int is_refname_available(const char *refname,\n \t\t\t\t * \"refs/foo\"). So we can stop looking\n \t\t\t\t * now and return true.\n \t\t\t\t */\n-\t\t\t\treturn 1;\n+\t\t\t\tret = 1;\n+\t\t\t\tgoto cleanup;\n \t\t\t}\n-\t\t\terror(\"'%s' exists; cannot create '%s'\", entry->name, refname);\n-\t\t\treturn 0;\n+\t\t\terror(\"'%s' exists; cannot create '%s'\", dirname.buf, refname);\n+\t\t\tgoto cleanup;\n \t\t}\n \n \n@@ -922,14 +927,16 @@ static int is_refname_available(const char *refname,\n \t\t * the next component. So try to look up the\n \t\t * directory, e.g., \"refs/foo/\".\n \t\t */\n-\t\tpos = search_ref_dir(dir, refname, slash + 1 - refname);\n+\t\tstrbuf_addch(&dirname, '/');\n+\t\tpos = search_ref_dir(dir, dirname.buf, dirname.len);\n \t\tif (pos < 0) {\n \t\t\t/*\n \t\t\t * There was no directory \"refs/foo/\", so\n \t\t\t * there is nothing under this whole prefix,\n \t\t\t * and we are OK.\n \t\t\t */\n-\t\t\treturn 1;\n+\t\t\tret = 1;\n+\t\t\tgoto cleanup;\n \t\t}\n \n \t\tdir = get_ref_dir(dir->entries[pos]);\n@@ -943,10 +950,9 @@ static int is_refname_available(const char *refname,\n \t * names are in the \"refs/foo/bar/\" namespace, because they\n \t * *do* conflict.\n \t */\n-\tstrbuf_addstr(&dirname, refname);\n+\tstrbuf_addstr(&dirname, refname + dirname.len);\n \tstrbuf_addch(&dirname, '/');\n \tpos = search_ref_dir(dir, dirname.buf, dirname.len);\n-\tstrbuf_release(&dirname);\n \n \tif (pos >= 0) {\n \t\t/*\n@@ -960,15 +966,21 @@ static int is_refname_available(const char *refname,\n \t\tdir = get_ref_dir(entry);\n \t\tdata.skip = skip;\n \t\tsort_ref_dir(dir);\n-\t\tif (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data))\n-\t\t\treturn 1;\n+\t\tif (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data)) {\n+\t\t\tret = 1;\n+\t\t\tgoto cleanup;\n+\t\t}\n \n \t\terror(\"'%s' exists; cannot create '%s'\",\n \t\t data.conflicting_refname, refname);\n-\t\treturn 0;\n+\t\tgoto cleanup;\n \t}\n \n-\treturn 1;\n+\tret = 1;\n+\n+cleanup:\n+\tstrbuf_release(&dirname);\n+\treturn ret;\n }\n \n struct packed_ref_cache {\n-- \n2.1.4\n"},{"id":"260988","messageId":"1431357920-25090-10-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 09/18] ref_transaction_commit(): use a string_list for detecting duplicates","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:11Z","receivedAt":"2015-05-11T15:25:11Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Detect duplicates by storing the reference names in a string_list and\nsorting that, instead of sorting the ref_updates directly.\n\n* In a moment the string_list will be used for another purpose, too.\n\n* This removes the need for the custom comparison function\n ref_update_compare().\n\n* This means that we can carry out the updates in the order that the\n user specified them instead of reordering them. This might be handy\n someday if, we want to permit multiple updates to a single reference\n as long as they are compatible with each other.\n\nNote: we can't use string_list_remove_duplicates() to check for\nduplicates, because we need to know the name of the reference that\nappeared multiple times, to be used in the error message.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 25 +++++++++++--------------\n 1 file changed, 11 insertions(+), 14 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8316bb1..6bb65ab 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3720,25 +3720,18 @@ int update_ref(const char *msg, const char *refname,\n \treturn 0;\n }\n \n-static int ref_update_compare(const void *r1, const void *r2)\n-{\n-\tconst struct ref_update * const *u1 = r1;\n-\tconst struct ref_update * const *u2 = r2;\n-\treturn strcmp((*u1)->refname, (*u2)->refname);\n-}\n-\n-static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n+static int ref_update_reject_duplicates(struct string_list *refnames,\n \t\t\t\t\tstruct strbuf *err)\n {\n-\tint i;\n+\tint i, n = refnames->nr;\n \n \tassert(err);\n \n \tfor (i = 1; i < n; i++)\n-\t\tif (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {\n+\t\tif (!strcmp(refnames->items[i - 1].string, refnames->items[i].string)) {\n \t\t\tstrbuf_addf(err,\n \t\t\t\t \"Multiple updates for ref '%s' not allowed.\",\n-\t\t\t\t updates[i]->refname);\n+\t\t\t\t refnames->items[i].string);\n \t\t\treturn 1;\n \t\t}\n \treturn 0;\n@@ -3752,6 +3745,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tstruct ref_update **updates = transaction->updates;\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *ref_to_delete;\n+\tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n \n \tassert(err);\n \n@@ -3763,9 +3757,11 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\treturn 0;\n \t}\n \n-\t/* Copy, sort, and reject duplicate refs */\n-\tqsort(updates, n, sizeof(*updates), ref_update_compare);\n-\tif (ref_update_reject_duplicates(updates, n, err)) {\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@@ -3857,6 +3853,7 @@ cleanup:\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n \tstring_list_clear(&refs_to_delete, 0);\n+\tstring_list_clear(&affected_refnames, 0);\n \treturn ret;\n }\n \n-- \n2.1.4\n"},{"id":"260979","messageId":"1431357920-25090-11-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 10/18] refs: check for D/F conflicts among refs created in a transaction","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:12Z","receivedAt":"2015-05-11T15:25:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If two references that D/F conflict (e.g., \"refs/foo\" and\n\"refs/foo/bar\") are created in a single transaction, the old code\ndiscovered the problem only after the \"commit\" phase of\nref_transaction_commit() had already begun. This could leave some\nreferences updated and others not, which violates the promise of\natomicity.\n\nInstead, check for such conflicts during the \"locking\" phase:\n\n* Teach is_refname_available() to take an \"extras\" parameter that can\n contain extra reference names with which the specified refname must\n not conflict.\n\n* Change lock_ref_sha1_basic() to take an \"extras\" parameter, which it\n passes through to is_refname_available().\n\n* Change ref_transaction_commit() to pass \"affected_refnames\" to\n lock_ref_sha1_basic() as its \"extras\" argument.\n\nThis change fixes a test case in t1404.\n\nThis code is a bit stricter than it needs to be. We could conceivably\nallow reference \"refs/foo/bar\" to be created in the same transaction\nas \"refs/foo\" is deleted (or vice versa). But that would be\ncomplicated to implement, because it is not possible to lock\n\"refs/foo/bar\" while \"refs/foo\" exists as a loose reference, but on\nthe other hand we don't want to delete some references before adding\nothers (because that could leave a gap during which required objects\nare unreachable). There is also a complication that reflog files'\npaths can conflict.\n\nAny less-strict implementation would probably require tricks like the\npacking of all references before the start of the real transaction, or\nthe use of temporary intermediate reference names.\n\nSo for now let's accept too-strict checks. Some reference update\ntransactions will be rejected unnecessarily, but they will be rejected\nin their entirety rather than leaving the repository in an\nintermediate state, as would happen now.\n\nPlease note that there is still one kind of D/F conflict that is *not*\nhandled correctly. If two processes are running at the same time, and\none tries to create \"refs/foo\" at the same time that the other tries\nto create \"refs/foo/bar\", then they can race with each other. Both\nprocesses can obtain their respective locks (\"refs/foo.lock\" and\n\"refs/foo/bar.lock\"), proceed to the \"commit\" phase of\nref_transaction_commit(), and then the slower process will discover\nthat it cannot rename its lockfile into place (after possibly having\ncommitted changes to other references). There appears to be no way to\nfix this race without changing the locking policy, which in turn would\nrequire a change to *all* Git clients.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 156 ++++++++++++++++++++++---------------\n t/t1404-update-ref-df-conflicts.sh | 2 +-\n 2 files changed, 95 insertions(+), 63 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6bb65ab..0fa70fb 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -859,19 +859,22 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)\n \n /*\n * Return true iff a reference named refname could be created without\n- * conflicting with the name of an existing reference in dir. If\n- * skip is non-NULL, ignore potential conflicts with refs in skip\n- * (e.g., because they are scheduled for deletion in the same\n- * operation).\n+ * conflicting with the name of an existing reference in dir. If\n+ * extras is non-NULL, it is a list of additional refnames with which\n+ * refname is not allowed to conflict. If skip is non-NULL, ignore\n+ * potential conflicts with refs in skip (e.g., because they are\n+ * scheduled for deletion in the same operation). Behavior is\n+ * undefined if the same name is listed in both extras and skip.\n *\n * Two reference names conflict if one of them exactly matches the\n * leading components of the other; e.g., \"refs/foo/bar\" conflicts\n * with both \"refs/foo\" and with \"refs/foo/bar/baz\" but not with\n * \"refs/foo/bar\" or \"refs/foo/barbados\".\n *\n- * skip must be sorted.\n+ * extras and skip must be sorted.\n */\n static int is_refname_available(const char *refname,\n+\t\t\t\tconst struct string_list *extras,\n \t\t\t\tconst struct string_list *skip,\n \t\t\t\tstruct ref_dir *dir)\n {\n@@ -895,51 +898,53 @@ static int is_refname_available(const char *refname,\n \t\t * \"refs/foo\"; if there is a reference with that name,\n \t\t * it is a conflict, *unless* it is in skip.\n \t\t */\n-\t\tpos = search_ref_dir(dir, dirname.buf, dirname.len);\n-\t\tif (pos >= 0) {\n-\t\t\t/*\n-\t\t\t * We found a reference whose name is a proper\n-\t\t\t * prefix of refname; e.g., \"refs/foo\".\n-\t\t\t */\n-\t\t\tif (skip && string_list_has_string(skip, dirname.buf)) {\n+\t\tif (dir) {\n+\t\t\tpos = search_ref_dir(dir, dirname.buf, dirname.len);\n+\t\t\tif (pos >= 0 &&\n+\t\t\t (!skip || !string_list_has_string(skip, dirname.buf))) {\n \t\t\t\t/*\n-\t\t\t\t * The reference we just found, e.g.,\n-\t\t\t\t * \"refs/foo\", is also in skip, so it\n-\t\t\t\t * is not considered a conflict.\n-\t\t\t\t * Moreover, the fact that \"refs/foo\"\n-\t\t\t\t * exists means that there cannot be\n-\t\t\t\t * any references anywhere under the\n-\t\t\t\t * \"refs/foo/\" namespace (because they\n-\t\t\t\t * would have conflicted with\n-\t\t\t\t * \"refs/foo\"). So we can stop looking\n-\t\t\t\t * now and return true.\n+\t\t\t\t * We found a reference whose name is\n+\t\t\t\t * a proper prefix of refname; e.g.,\n+\t\t\t\t * \"refs/foo\", and is not in skip.\n \t\t\t\t */\n-\t\t\t\tret = 1;\n+\t\t\t\terror(\"'%s' exists; cannot create '%s'\",\n+\t\t\t\t dirname.buf, refname);\n \t\t\t\tgoto cleanup;\n \t\t\t}\n-\t\t\terror(\"'%s' exists; cannot create '%s'\", dirname.buf, refname);\n-\t\t\tgoto cleanup;\n \t\t}\n \n+\t\tif (extras && string_list_has_string(extras, dirname.buf) &&\n+\t\t (!skip || !string_list_has_string(skip, dirname.buf))) {\n+\t\t\terror(\"cannot process '%s' and '%s' at the same time\",\n+\t\t\t refname, dirname.buf);\n+\t\t\tgoto cleanup;\n+\t\t}\n \n \t\t/*\n \t\t * Otherwise, we can try to continue our search with\n \t\t * the next component. So try to look up the\n-\t\t * directory, e.g., \"refs/foo/\".\n+\t\t * directory, e.g., \"refs/foo/\". If we come up empty,\n+\t\t * we know there is nothing under this whole prefix,\n+\t\t * but even in that case we still have to continue the\n+\t\t * search for conflicts with extras.\n \t\t */\n \t\tstrbuf_addch(&dirname, '/');\n-\t\tpos = search_ref_dir(dir, dirname.buf, dirname.len);\n-\t\tif (pos < 0) {\n-\t\t\t/*\n-\t\t\t * There was no directory \"refs/foo/\", so\n-\t\t\t * there is nothing under this whole prefix,\n-\t\t\t * and we are OK.\n-\t\t\t */\n-\t\t\tret = 1;\n-\t\t\tgoto cleanup;\n+\t\tif (dir) {\n+\t\t\tpos = search_ref_dir(dir, dirname.buf, dirname.len);\n+\t\t\tif (pos < 0) {\n+\t\t\t\t/*\n+\t\t\t\t * There was no directory \"refs/foo/\",\n+\t\t\t\t * so there is nothing under this\n+\t\t\t\t * whole prefix. So there is no need\n+\t\t\t\t * to continue looking for conflicting\n+\t\t\t\t * references. But we need to continue\n+\t\t\t\t * looking for conflicting extras.\n+\t\t\t\t */\n+\t\t\t\tdir = NULL;\n+\t\t\t} else {\n+\t\t\t\tdir = get_ref_dir(dir->entries[pos]);\n+\t\t\t}\n \t\t}\n-\n-\t\tdir = get_ref_dir(dir->entries[pos]);\n \t}\n \n \t/*\n@@ -952,30 +957,56 @@ static int is_refname_available(const char *refname,\n \t */\n \tstrbuf_addstr(&dirname, refname + dirname.len);\n \tstrbuf_addch(&dirname, '/');\n-\tpos = search_ref_dir(dir, dirname.buf, dirname.len);\n \n-\tif (pos >= 0) {\n+\tif (dir) {\n+\t\tpos = search_ref_dir(dir, dirname.buf, dirname.len);\n+\n+\t\tif (pos >= 0) {\n+\t\t\t/*\n+\t\t\t * We found a directory named \"$refname/\"\n+\t\t\t * (e.g., \"refs/foo/bar/\"). It is a problem\n+\t\t\t * iff it contains any ref that is not in\n+\t\t\t * \"skip\".\n+\t\t\t */\n+\t\t\tstruct nonmatching_ref_data data;\n+\n+\t\t\tdata.skip = skip;\n+\t\t\tdata.conflicting_refname = NULL;\n+\t\t\tdir = get_ref_dir(dir->entries[pos]);\n+\t\t\tsort_ref_dir(dir);\n+\t\t\tif (do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data)) {\n+\t\t\t\terror(\"'%s' exists; cannot create '%s'\",\n+\t\t\t\t data.conflicting_refname, refname);\n+\t\t\t\tgoto cleanup;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\tif (extras) {\n \t\t/*\n-\t\t * We found a directory named \"$refname/\" (e.g.,\n-\t\t * \"refs/foo/bar/\"). It is a problem iff it contains\n-\t\t * any ref that is not in \"skip\".\n+\t\t * Check for entries in extras that start with\n+\t\t * \"$refname/\". We do that by looking for the place\n+\t\t * where \"$refname/\" would be inserted in extras. If\n+\t\t * there is an entry at that position that starts with\n+\t\t * \"$refname/\" and is not in skip, then we have a\n+\t\t * conflict.\n \t\t */\n-\t\tstruct nonmatching_ref_data data;\n-\t\tstruct ref_entry *entry = dir->entries[pos];\n-\n-\t\tdir = get_ref_dir(entry);\n-\t\tdata.skip = skip;\n-\t\tsort_ref_dir(dir);\n-\t\tif (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data)) {\n-\t\t\tret = 1;\n-\t\t\tgoto cleanup;\n-\t\t}\n+\t\tfor (pos = string_list_find_insert_index(extras, dirname.buf, 0);\n+\t\t pos < extras->nr; pos++) {\n+\t\t\tconst char *extra_refname = extras->items[pos].string;\n \n-\t\terror(\"'%s' exists; cannot create '%s'\",\n-\t\t data.conflicting_refname, refname);\n-\t\tgoto cleanup;\n+\t\t\tif (!starts_with(extra_refname, dirname.buf))\n+\t\t\t\tbreak;\n+\n+\t\t\tif (!skip || !string_list_has_string(skip, extra_refname)) {\n+\t\t\t\terror(\"cannot process '%s' and '%s' at the same time\",\n+\t\t\t\t refname, extra_refname);\n+\t\t\t\tgoto cleanup;\n+\t\t\t}\n+\t\t}\n \t}\n \n+\t/* No conflicts were found */\n \tret = 1;\n \n cleanup:\n@@ -2296,6 +2327,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n */\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 const struct string_list *extras,\n \t\t\t\t\t const struct string_list *skip,\n \t\t\t\t\t unsigned int flags, int *type_p)\n {\n@@ -2351,7 +2383,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t * our refname.\n \t */\n \tif (is_null_sha1(lock->old_sha1) &&\n-\t !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) {\n+\t !is_refname_available(refname, extras, skip, get_packed_refs(&ref_cache))) {\n \t\tlast_errno = ENOTDIR;\n \t\tgoto error_return;\n \t}\n@@ -2792,8 +2824,8 @@ static int rename_ref_available(const char *oldname, const char *newname)\n \tint ret;\n \n \tstring_list_insert(&skip, oldname);\n-\tret = is_refname_available(newname, &skip, get_packed_refs(&ref_cache))\n-\t && is_refname_available(newname, &skip, get_loose_refs(&ref_cache));\n+\tret = is_refname_available(newname, NULL, &skip, get_packed_refs(&ref_cache))\n+\t\t&& is_refname_available(newname, NULL, &skip, get_loose_refs(&ref_cache));\n \tstring_list_clear(&skip, 0);\n \treturn ret;\n }\n@@ -2851,7 +2883,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, NULL, 0, NULL);\n+\tlock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, 0, NULL);\n \tif (!lock) {\n \t\terror(\"unable to lock %s for update\", newrefname);\n \t\tgoto rollback;\n@@ -2865,7 +2897,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, NULL, 0, NULL);\n+\tlock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, 0, NULL);\n \tif (!lock) {\n \t\terror(\"unable to lock %s for rollback\", oldrefname);\n \t\tgoto rollbacklog;\n@@ -3777,7 +3809,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\tupdate->refname,\n \t\t\t\t((update->flags & REF_HAVE_OLD) ?\n \t\t\t\t update->old_sha1 : NULL),\n-\t\t\t\tNULL,\n+\t\t\t\t&affected_refnames, NULL,\n \t\t\t\tflags,\n \t\t\t\t&update->type);\n \t\tif (!update->lock) {\n@@ -4054,7 +4086,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n \t * reference itself, plus we might need to update the\n \t * reference if --updateref was specified:\n \t */\n-\tlock = lock_ref_sha1_basic(refname, sha1, NULL, 0, &type);\n+\tlock = lock_ref_sha1_basic(refname, sha1, NULL, NULL, 0, &type);\n \tif (!lock)\n \t\treturn error(\"cannot lock ref '%s'\", refname);\n \tif (!reflog_exists(refname)) {\ndiff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh\nindex ed1b93e..b0bb7d4 100755\n--- a/t/t1404-update-ref-df-conflicts.sh\n+++ b/t/t1404-update-ref-df-conflicts.sh\n@@ -96,7 +96,7 @@ test_expect_success 'new ref is a deeper prefix of existing packed' '\n \n '\n \n-test_expect_failure 'one new ref is a simple prefix of another' '\n+test_expect_success 'one new ref is a simple prefix of another' '\n \n \tprefix=refs/5 &&\n \ttest_update_rejected $prefix \"a e\" false \"b c c/x d\" \\\n-- \n2.1.4\n"},{"id":"260980","messageId":"1431357920-25090-12-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 11/18] verify_refname_available(): rename function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:13Z","receivedAt":"2015-05-11T15:25:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Rename is_refname_available() to verify_refname_available() and change\nits return value from 1 for success to 0 for success, to be consistent\nwith our error-handling convention. In a moment it will also get a\n\"struct strbuf *err\" parameter.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 37 ++++++++++++++++++++-----------------\n 1 file changed, 20 insertions(+), 17 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 0fa70fb..703e93d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -263,7 +263,7 @@ struct ref_dir {\n * presence of an empty subdirectory does not block the creation of a\n * similarly-named reference. (The fact that reference names with the\n * same leading components can conflict *with each other* is a\n- * separate issue that is regulated by is_refname_available().)\n+ * separate issue that is regulated by verify_refname_available().)\n *\n * Please note that the name field contains the fully-qualified\n * reference (or subdirectory) name. Space could be saved by only\n@@ -858,13 +858,14 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)\n }\n \n /*\n- * Return true iff a reference named refname could be created without\n- * conflicting with the name of an existing reference in dir. If\n- * extras is non-NULL, it is a list of additional refnames with which\n- * refname is not allowed to conflict. If skip is non-NULL, ignore\n- * potential conflicts with refs in skip (e.g., because they are\n- * scheduled for deletion in the same operation). Behavior is\n- * undefined if the same name is listed in both extras and skip.\n+ * Return 0 if a reference named refname could be created without\n+ * conflicting with the name of an existing reference in dir.\n+ * Otherwise, return a negative value. If extras is non-NULL, it is a\n+ * list of additional refnames with which refname is not allowed to\n+ * conflict. If skip is non-NULL, ignore potential conflicts with refs\n+ * in skip (e.g., because they are scheduled for deletion in the same\n+ * operation). Behavior is undefined if the same name is listed in\n+ * both extras and skip.\n *\n * Two reference names conflict if one of them exactly matches the\n * leading components of the other; e.g., \"refs/foo/bar\" conflicts\n@@ -873,15 +874,15 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)\n *\n * extras and skip must be sorted.\n */\n-static int is_refname_available(const char *refname,\n-\t\t\t\tconst struct string_list *extras,\n-\t\t\t\tconst struct string_list *skip,\n-\t\t\t\tstruct ref_dir *dir)\n+static int verify_refname_available(const char *refname,\n+\t\t\t\t const struct string_list *extras,\n+\t\t\t\t const struct string_list *skip,\n+\t\t\t\t struct ref_dir *dir)\n {\n \tconst char *slash;\n \tint pos;\n \tstruct strbuf dirname = STRBUF_INIT;\n-\tint ret = 0;\n+\tint ret = -1;\n \n \t/*\n \t * For the sake of comments in this function, suppose that\n@@ -1007,7 +1008,7 @@ static int is_refname_available(const char *refname,\n \t}\n \n \t/* No conflicts were found */\n-\tret = 1;\n+\tret = 0;\n \n cleanup:\n \tstrbuf_release(&dirname);\n@@ -2383,7 +2384,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t * our refname.\n \t */\n \tif (is_null_sha1(lock->old_sha1) &&\n-\t !is_refname_available(refname, extras, skip, get_packed_refs(&ref_cache))) {\n+\t verify_refname_available(refname, extras, skip, get_packed_refs(&ref_cache))) {\n \t\tlast_errno = ENOTDIR;\n \t\tgoto error_return;\n \t}\n@@ -2824,8 +2825,10 @@ static int rename_ref_available(const char *oldname, const char *newname)\n \tint ret;\n \n \tstring_list_insert(&skip, oldname);\n-\tret = is_refname_available(newname, NULL, &skip, get_packed_refs(&ref_cache))\n-\t\t&& is_refname_available(newname, NULL, &skip, get_loose_refs(&ref_cache));\n+\tret = !verify_refname_available(newname, NULL, &skip,\n+\t\t\t\t\tget_packed_refs(&ref_cache))\n+\t\t&& !verify_refname_available(newname, NULL, &skip,\n+\t\t\t\t\t get_loose_refs(&ref_cache));\n \tstring_list_clear(&skip, 0);\n \treturn ret;\n }\n-- \n2.1.4\n"},{"id":"260985","messageId":"1431357920-25090-13-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 12/18] verify_refname_available(): report errors via a \"struct strbuf *err\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:14Z","receivedAt":"2015-05-11T15:25:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It shouldn't be spewing errors directly to stderr.\n\nFor now, change its callers to spew the errors to stderr.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 50 ++++++++++++++++++++++++++++++--------------------\n 1 file changed, 30 insertions(+), 20 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 703e93d..43e7bdd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -860,12 +860,12 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)\n /*\n * Return 0 if a reference named refname could be created without\n * conflicting with the name of an existing reference in dir.\n- * Otherwise, return a negative value. If extras is non-NULL, it is a\n- * list of additional refnames with which refname is not allowed to\n- * conflict. If skip is non-NULL, ignore potential conflicts with refs\n- * in skip (e.g., because they are scheduled for deletion in the same\n- * operation). Behavior is undefined if the same name is listed in\n- * both extras and skip.\n+ * Otherwise, return a negative value and write an explanation to err.\n+ * If extras is non-NULL, it is a list of additional refnames with\n+ * which refname is not allowed to conflict. If skip is non-NULL,\n+ * ignore potential conflicts with refs in skip (e.g., because they\n+ * are scheduled for deletion in the same operation). Behavior is\n+ * undefined if the same name is listed in both extras and skip.\n *\n * Two reference names conflict if one of them exactly matches the\n * leading components of the other; e.g., \"refs/foo/bar\" conflicts\n@@ -877,7 +877,8 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)\n static int verify_refname_available(const char *refname,\n \t\t\t\t const struct string_list *extras,\n \t\t\t\t const struct string_list *skip,\n-\t\t\t\t struct ref_dir *dir)\n+\t\t\t\t struct ref_dir *dir,\n+\t\t\t\t struct strbuf *err)\n {\n \tconst char *slash;\n \tint pos;\n@@ -889,6 +890,8 @@ static int verify_refname_available(const char *refname,\n \t * refname is \"refs/foo/bar\".\n \t */\n \n+\tassert(err);\n+\n \tstrbuf_grow(&dirname, strlen(refname) + 1);\n \tfor (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {\n \t\t/* Expand dirname to the new prefix, not including the trailing slash: */\n@@ -908,16 +911,16 @@ static int verify_refname_available(const char *refname,\n \t\t\t\t * a proper prefix of refname; e.g.,\n \t\t\t\t * \"refs/foo\", and is not in skip.\n \t\t\t\t */\n-\t\t\t\terror(\"'%s' exists; cannot create '%s'\",\n-\t\t\t\t dirname.buf, refname);\n+\t\t\t\tstrbuf_addf(err, \"'%s' exists; cannot create '%s'\",\n+\t\t\t\t\t dirname.buf, refname);\n \t\t\t\tgoto cleanup;\n \t\t\t}\n \t\t}\n \n \t\tif (extras && string_list_has_string(extras, dirname.buf) &&\n \t\t (!skip || !string_list_has_string(skip, dirname.buf))) {\n-\t\t\terror(\"cannot process '%s' and '%s' at the same time\",\n-\t\t\t refname, dirname.buf);\n+\t\t\tstrbuf_addf(err, \"cannot process '%s' and '%s' at the same time\",\n+\t\t\t\t refname, dirname.buf);\n \t\t\tgoto cleanup;\n \t\t}\n \n@@ -976,8 +979,8 @@ static int verify_refname_available(const char *refname,\n \t\t\tdir = get_ref_dir(dir->entries[pos]);\n \t\t\tsort_ref_dir(dir);\n \t\t\tif (do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data)) {\n-\t\t\t\terror(\"'%s' exists; cannot create '%s'\",\n-\t\t\t\t data.conflicting_refname, refname);\n+\t\t\t\tstrbuf_addf(err, \"'%s' exists; cannot create '%s'\",\n+\t\t\t\t\t data.conflicting_refname, refname);\n \t\t\t\tgoto cleanup;\n \t\t\t}\n \t\t}\n@@ -1000,8 +1003,8 @@ static int verify_refname_available(const char *refname,\n \t\t\t\tbreak;\n \n \t\t\tif (!skip || !string_list_has_string(skip, extra_refname)) {\n-\t\t\t\terror(\"cannot process '%s' and '%s' at the same time\",\n-\t\t\t\t refname, extra_refname);\n+\t\t\t\tstrbuf_addf(err, \"cannot process '%s' and '%s' at the same time\",\n+\t\t\t\t\t refname, extra_refname);\n \t\t\t\tgoto cleanup;\n \t\t\t}\n \t\t}\n@@ -2340,6 +2343,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tint mustexist = (old_sha1 && !is_null_sha1(old_sha1));\n \tint resolve_flags = 0;\n \tint attempts_remaining = 3;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tlock = xcalloc(1, sizeof(struct ref_lock));\n \tlock->lock_fd = -1;\n@@ -2384,7 +2388,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t * our refname.\n \t */\n \tif (is_null_sha1(lock->old_sha1) &&\n-\t verify_refname_available(refname, extras, skip, get_packed_refs(&ref_cache))) {\n+\t verify_refname_available(refname, extras, skip,\n+\t\t\t\t get_packed_refs(&ref_cache), &err)) {\n+\t\terror(\"%s\", err.buf);\n \t\tlast_errno = ENOTDIR;\n \t\tgoto error_return;\n \t}\n@@ -2425,10 +2431,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t */\n \t\t\tgoto retry;\n \t\telse {\n-\t\t\tstruct strbuf err = STRBUF_INIT;\n \t\t\tunable_to_lock_message(ref_file, errno, &err);\n \t\t\terror(\"%s\", err.buf);\n-\t\t\tstrbuf_release(&err);\n \t\t\tgoto error_return;\n \t\t}\n \t}\n@@ -2436,6 +2440,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \n error_return:\n \tunlock_ref(lock);\n+\tstrbuf_release(&err);\n \terrno = last_errno;\n \treturn NULL;\n }\n@@ -2822,14 +2827,19 @@ static int rename_tmp_log(const char *newrefname)\n static int rename_ref_available(const char *oldname, const char *newname)\n {\n \tstruct string_list skip = STRING_LIST_INIT_NODUP;\n+\tstruct strbuf err = STRBUF_INIT;\n \tint ret;\n \n \tstring_list_insert(&skip, oldname);\n \tret = !verify_refname_available(newname, NULL, &skip,\n-\t\t\t\t\tget_packed_refs(&ref_cache))\n+\t\t\t\t\tget_packed_refs(&ref_cache), &err)\n \t\t&& !verify_refname_available(newname, NULL, &skip,\n-\t\t\t\t\t get_loose_refs(&ref_cache));\n+\t\t\t\t\t get_loose_refs(&ref_cache), &err);\n+\tif (!ret)\n+\t\terror(\"%s\", err.buf);\n+\n \tstring_list_clear(&skip, 0);\n+\tstrbuf_release(&err);\n \treturn ret;\n }\n \n-- \n2.1.4\n"},{"id":"260981","messageId":"1431357920-25090-14-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 13/18] lock_ref_sha1_basic(): report errors via a \"struct strbuf *err\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:15Z","receivedAt":"2015-05-11T15:25:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"For now, change the callers to spew the error to stderr like before.\nBut soon we will change them to incorporate the reason for the failure\ninto their own error messages.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 44 ++++++++++++++++++++++++++++----------------\n 1 file changed, 28 insertions(+), 16 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 43e7bdd..ff9b9cf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2333,7 +2333,8 @@ 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 const struct string_list *extras,\n \t\t\t\t\t const struct string_list *skip,\n-\t\t\t\t\t unsigned int flags, int *type_p)\n+\t\t\t\t\t unsigned int flags, int *type_p,\n+\t\t\t\t\t struct strbuf *err)\n {\n \tchar *ref_file;\n \tconst char *orig_refname = refname;\n@@ -2343,7 +2344,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tint mustexist = (old_sha1 && !is_null_sha1(old_sha1));\n \tint resolve_flags = 0;\n \tint attempts_remaining = 3;\n-\tstruct strbuf err = STRBUF_INIT;\n+\n+\tassert(err);\n \n \tlock = xcalloc(1, sizeof(struct ref_lock));\n \tlock->lock_fd = -1;\n@@ -2367,7 +2369,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\tref_file = git_path(\"%s\", orig_refname);\n \t\tif (remove_empty_directories(ref_file)) {\n \t\t\tlast_errno = errno;\n-\t\t\terror(\"there are still refs under '%s'\", orig_refname);\n+\t\t\tstrbuf_addf(err, \"there are still refs under '%s'\",\n+\t\t\t\t orig_refname);\n \t\t\tgoto error_return;\n \t\t}\n \t\trefname = resolve_ref_unsafe(orig_refname, resolve_flags,\n@@ -2377,8 +2380,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t *type_p = type;\n \tif (!refname) {\n \t\tlast_errno = errno;\n-\t\terror(\"unable to resolve reference %s: %s\",\n-\t\t\torig_refname, strerror(errno));\n+\t\tstrbuf_addf(err, \"unable to resolve reference %s: %s\",\n+\t\t\t orig_refname, strerror(errno));\n \t\tgoto error_return;\n \t}\n \t/*\n@@ -2389,8 +2392,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t */\n \tif (is_null_sha1(lock->old_sha1) &&\n \t verify_refname_available(refname, extras, skip,\n-\t\t\t\t get_packed_refs(&ref_cache), &err)) {\n-\t\terror(\"%s\", err.buf);\n+\t\t\t\t get_packed_refs(&ref_cache), err)) {\n \t\tlast_errno = ENOTDIR;\n \t\tgoto error_return;\n \t}\n@@ -2416,7 +2418,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t/* fall through */\n \tdefault:\n \t\tlast_errno = errno;\n-\t\terror(\"unable to create directory for %s\", ref_file);\n+\t\tstrbuf_addf(err, \"unable to create directory for %s\", ref_file);\n \t\tgoto error_return;\n \t}\n \n@@ -2431,8 +2433,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t */\n \t\t\tgoto retry;\n \t\telse {\n-\t\t\tunable_to_lock_message(ref_file, errno, &err);\n-\t\t\terror(\"%s\", err.buf);\n+\t\t\tunable_to_lock_message(ref_file, errno, err);\n \t\t\tgoto error_return;\n \t\t}\n \t}\n@@ -2440,7 +2441,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \n error_return:\n \tunlock_ref(lock);\n-\tstrbuf_release(&err);\n \terrno = last_errno;\n \treturn NULL;\n }\n@@ -2854,6 +2854,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tstruct stat loginfo;\n \tint log = !lstat(git_path(\"logs/%s\", oldrefname), &loginfo);\n \tconst char *symref = NULL;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tif (log && S_ISLNK(loginfo.st_mode))\n \t\treturn error(\"reflog for %s is a symlink\", oldrefname);\n@@ -2896,8 +2897,10 @@ 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, NULL, NULL, 0, NULL);\n+\tlock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, 0, NULL, &err);\n \tif (!lock) {\n+\t\terror(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n \t\terror(\"unable to lock %s for update\", newrefname);\n \t\tgoto rollback;\n \t}\n@@ -2910,8 +2913,10 @@ 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, NULL, NULL, 0, NULL);\n+\tlock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, 0, NULL, &err);\n \tif (!lock) {\n+\t\terror(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n \t\terror(\"unable to lock %s for rollback\", oldrefname);\n \t\tgoto rollbacklog;\n \t}\n@@ -3824,11 +3829,14 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t update->old_sha1 : NULL),\n \t\t\t\t&affected_refnames, NULL,\n \t\t\t\tflags,\n-\t\t\t\t&update->type);\n+\t\t\t\t&update->type,\n+\t\t\t\terr);\n \t\tif (!update->lock) {\n \t\t\tret = (errno == ENOTDIR)\n \t\t\t\t? TRANSACTION_NAME_CONFLICT\n \t\t\t\t: TRANSACTION_GENERIC_ERROR;\n+\t\t\terror(\"%s\", err->buf);\n+\t\t\tstrbuf_reset(err);\n \t\t\tstrbuf_addf(err, \"Cannot lock the ref '%s'.\",\n \t\t\t\t update->refname);\n \t\t\tgoto cleanup;\n@@ -4088,6 +4096,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n \tchar *log_file;\n \tint status = 0;\n \tint type;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tmemset(&cb, 0, sizeof(cb));\n \tcb.flags = flags;\n@@ -4099,9 +4108,12 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n \t * reference itself, plus we might need to update the\n \t * reference if --updateref was specified:\n \t */\n-\tlock = lock_ref_sha1_basic(refname, sha1, NULL, NULL, 0, &type);\n-\tif (!lock)\n+\tlock = lock_ref_sha1_basic(refname, sha1, NULL, NULL, 0, &type, &err);\n+\tif (!lock) {\n+\t\terror(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n \t\treturn error(\"cannot lock ref '%s'\", refname);\n+\t}\n \tif (!reflog_exists(refname)) {\n \t\tunlock_ref(lock);\n \t\treturn 0;\n-- \n2.1.4\n"},{"id":"260986","messageId":"1431357920-25090-15-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 14/18] lock_ref_sha1_basic(): improve diagnostics for ref D/F conflicts","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:16Z","receivedAt":"2015-05-11T15:25:16Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If there is a failure to lock a reference that is likely caused by a\nD/F conflict (e.g., trying to lock \"refs/foo/bar\" when reference\n\"refs/foo\" already exists), invoke verify_refname_available() to try\nto generate a more helpful error message.\n\nThat function might not detect an error. For example, some\nnon-reference file might be blocking the deletion of an\notherwise-empty directory tree, or there might be a race with another\nprocess that just deleted the offending reference. In such cases,\ngenerate the strerror-based error message like before.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 16 ++++++++++++----\n t/t1404-update-ref-df-conflicts.sh | 8 ++++----\n 2 files changed, 16 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ff9b9cf..ce993bd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2369,8 +2369,12 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\tref_file = git_path(\"%s\", orig_refname);\n \t\tif (remove_empty_directories(ref_file)) {\n \t\t\tlast_errno = errno;\n-\t\t\tstrbuf_addf(err, \"there are still refs under '%s'\",\n-\t\t\t\t orig_refname);\n+\n+\t\t\tif (!verify_refname_available(orig_refname, extras, skip,\n+\t\t\t\t\t\t get_loose_refs(&ref_cache), err))\n+\t\t\t\tstrbuf_addf(err, \"there are still refs under '%s'\",\n+\t\t\t\t\t orig_refname);\n+\n \t\t\tgoto error_return;\n \t\t}\n \t\trefname = resolve_ref_unsafe(orig_refname, resolve_flags,\n@@ -2380,8 +2384,12 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t *type_p = type;\n \tif (!refname) {\n \t\tlast_errno = errno;\n-\t\tstrbuf_addf(err, \"unable to resolve reference %s: %s\",\n-\t\t\t orig_refname, strerror(errno));\n+\t\tif (last_errno != ENOTDIR ||\n+\t\t !verify_refname_available(orig_refname, extras, skip,\n+\t\t\t\t\t get_loose_refs(&ref_cache), err))\n+\t\t\tstrbuf_addf(err, \"unable to resolve reference %s: %s\",\n+\t\t\t\t orig_refname, strerror(last_errno));\n+\n \t\tgoto error_return;\n \t}\n \t/*\ndiff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh\nindex b0bb7d4..348a07a 100755\n--- a/t/t1404-update-ref-df-conflicts.sh\n+++ b/t/t1404-update-ref-df-conflicts.sh\n@@ -36,7 +36,7 @@ test_expect_success 'existing loose ref is a simple prefix of new' '\n \n \tprefix=refs/1l &&\n \ttest_update_rejected $prefix \"a c e\" false \"b c/x d\" \\\n-\t\t\"unable to resolve reference $prefix/c/x: Not a directory\"\n+\t\t\"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x$Q\"\n \n '\n \n@@ -52,7 +52,7 @@ test_expect_success 'existing loose ref is a deeper prefix of new' '\n \n \tprefix=refs/2l &&\n \ttest_update_rejected $prefix \"a c e\" false \"b c/x/y d\" \\\n-\t\t\"unable to resolve reference $prefix/c/x/y: Not a directory\"\n+\t\t\"$Q$prefix/c$Q exists; cannot create $Q$prefix/c/x/y$Q\"\n \n '\n \n@@ -68,7 +68,7 @@ test_expect_success 'new ref is a simple prefix of existing loose' '\n \n \tprefix=refs/3l &&\n \ttest_update_rejected $prefix \"a c/x e\" false \"b c d\" \\\n-\t\t\"there are still refs under $Q$prefix/c$Q\"\n+\t\t\"$Q$prefix/c/x$Q exists; cannot create $Q$prefix/c$Q\"\n \n '\n \n@@ -84,7 +84,7 @@ test_expect_success 'new ref is a deeper prefix of existing loose' '\n \n \tprefix=refs/4l &&\n \ttest_update_rejected $prefix \"a c/x/y e\" false \"b c d\" \\\n-\t\t\"there are still refs under $Q$prefix/c$Q\"\n+\t\t\"$Q$prefix/c/x/y$Q exists; cannot create $Q$prefix/c$Q\"\n \n '\n \n-- \n2.1.4\n"},{"id":"260990","messageId":"1431357920-25090-16-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 15/18] rename_ref(): integrate lock_ref_sha1_basic() errors into ours","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:17Z","receivedAt":"2015-05-11T15:25:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now that lock_ref_sha1_basic() gives us back its error messages via a\nstrbuf, incorporate its error message into our error message rather\nthan emitting two separate error messages.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ce993bd..87c1ad1 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2907,9 +2907,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \n \tlock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, 0, NULL, &err);\n \tif (!lock) {\n-\t\terror(\"%s\", err.buf);\n+\t\terror(\"unable to rename '%s' to '%s': %s\", oldrefname, newrefname, err.buf);\n \t\tstrbuf_release(&err);\n-\t\terror(\"unable to lock %s for update\", newrefname);\n \t\tgoto rollback;\n \t}\n \thashcpy(lock->old_sha1, orig_sha1);\n@@ -2923,9 +2922,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n rollback:\n \tlock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, 0, NULL, &err);\n \tif (!lock) {\n-\t\terror(\"%s\", err.buf);\n+\t\terror(\"unable to lock %s for rollback: %s\", oldrefname, err.buf);\n \t\tstrbuf_release(&err);\n-\t\terror(\"unable to lock %s for rollback\", oldrefname);\n \t\tgoto rollbacklog;\n \t}\n \n-- \n2.1.4\n"},{"id":"260982","messageId":"1431357920-25090-17-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 16/18] ref_transaction_commit(): provide better error messages","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:18Z","receivedAt":"2015-05-11T15:25:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now that lock_ref_sha1_basic() gives us back its error messages via a\nstrbuf, incorporate its error message into our error message rather\nthan emitting one error messages to stderr immediately and returning a\nsecond to our caller.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 87c1ad1..ecaf804 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3838,13 +3838,15 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t&update->type,\n \t\t\t\terr);\n \t\tif (!update->lock) {\n+\t\t\tchar *reason;\n+\n \t\t\tret = (errno == ENOTDIR)\n \t\t\t\t? TRANSACTION_NAME_CONFLICT\n \t\t\t\t: TRANSACTION_GENERIC_ERROR;\n-\t\t\terror(\"%s\", err->buf);\n-\t\t\tstrbuf_reset(err);\n-\t\t\tstrbuf_addf(err, \"Cannot lock the ref '%s'.\",\n-\t\t\t\t update->refname);\n+\t\t\treason = strbuf_detach(err, NULL);\n+\t\t\tstrbuf_addf(err, \"Cannot lock the ref '%s': %s\",\n+\t\t\t\t update->refname, reason);\n+\t\t\tfree(reason);\n \t\t\tgoto cleanup;\n \t\t}\n \t}\n-- \n2.1.4\n"},{"id":"260984","messageId":"1431357920-25090-18-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 17/18] ref_transaction_commit(): delete extra \"the\" from error message","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:19Z","receivedAt":"2015-05-11T15:25:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"While we are in the area, let's remove a superfluous definite article\nfrom the error message that is emitted when the reference cannot be\nlocked. This improves how it reads and makes it a bit shorter.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 2 +-\n t/t1400-update-ref.sh | 14 +++++++-------\n 2 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ecaf804..bc4b1ab 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3844,7 +3844,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t? TRANSACTION_NAME_CONFLICT\n \t\t\t\t: TRANSACTION_GENERIC_ERROR;\n \t\t\treason = strbuf_detach(err, NULL);\n-\t\t\tstrbuf_addf(err, \"Cannot lock the ref '%s': %s\",\n+\t\t\tstrbuf_addf(err, \"Cannot lock ref '%s': %s\",\n \t\t\t\t update->refname, reason);\n \t\t\tfree(reason);\n \t\t\tgoto cleanup;\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 6805b9e..86fa468 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -519,7 +519,7 @@ test_expect_success 'stdin create ref works with path with space to blob' '\n test_expect_success 'stdin update ref fails with wrong old value' '\n \techo \"update $c $m $m~1\" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: Cannot lock the ref '\"'\"'$c'\"'\"'\" err &&\n+\tgrep \"fatal: Cannot lock ref '\"'\"'$c'\"'\"'\" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n@@ -555,7 +555,7 @@ test_expect_success 'stdin update ref works with right old value' '\n test_expect_success 'stdin delete ref fails with wrong old value' '\n \techo \"delete $a $m~1\" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: Cannot lock the ref '\"'\"'$a'\"'\"'\" err &&\n+\tgrep \"fatal: Cannot lock ref '\"'\"'$a'\"'\"'\" err &&\n \tgit rev-parse $m >expect &&\n \tgit rev-parse $a >actual &&\n \ttest_cmp expect actual\n@@ -688,7 +688,7 @@ test_expect_success 'stdin update refs fails with wrong old value' '\n \tupdate $c ''\n \tEOF\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: Cannot lock the ref '\"'\"'$c'\"'\"'\" err &&\n+\tgrep \"fatal: Cannot lock ref '\"'\"'$c'\"'\"'\" err &&\n \tgit rev-parse $m >expect &&\n \tgit rev-parse $a >actual &&\n \ttest_cmp expect actual &&\n@@ -883,7 +883,7 @@ test_expect_success 'stdin -z create ref works with path with space to blob' '\n test_expect_success 'stdin -z update ref fails with wrong old value' '\n \tprintf $F \"update $c\" \"$m\" \"$m~1\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: Cannot lock the ref '\"'\"'$c'\"'\"'\" err &&\n+\tgrep \"fatal: Cannot lock ref '\"'\"'$c'\"'\"'\" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n@@ -899,7 +899,7 @@ test_expect_success 'stdin -z create ref fails when ref exists' '\n \tgit rev-parse \"$c\" >expect &&\n \tprintf $F \"create $c\" \"$m~1\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: Cannot lock the ref '\"'\"'$c'\"'\"'\" err &&\n+\tgrep \"fatal: Cannot lock ref '\"'\"'$c'\"'\"'\" err &&\n \tgit rev-parse \"$c\" >actual &&\n \ttest_cmp expect actual\n '\n@@ -930,7 +930,7 @@ test_expect_success 'stdin -z update ref works with right old value' '\n test_expect_success 'stdin -z delete ref fails with wrong old value' '\n \tprintf $F \"delete $a\" \"$m~1\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: Cannot lock the ref '\"'\"'$a'\"'\"'\" err &&\n+\tgrep \"fatal: Cannot lock ref '\"'\"'$a'\"'\"'\" err &&\n \tgit rev-parse $m >expect &&\n \tgit rev-parse $a >actual &&\n \ttest_cmp expect actual\n@@ -1045,7 +1045,7 @@ test_expect_success 'stdin -z update refs fails with wrong old value' '\n \tgit update-ref $c $m &&\n \tprintf $F \"update $a\" \"$m\" \"$m\" \"update $b\" \"$m\" \"$m\" \"update $c\" \"$m\" \"$Z\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: Cannot lock the ref '\"'\"'$c'\"'\"'\" err &&\n+\tgrep \"fatal: Cannot lock ref '\"'\"'$c'\"'\"'\" err &&\n \tgit rev-parse $m >expect &&\n \tgit rev-parse $a >actual &&\n \ttest_cmp expect actual &&\n-- \n2.1.4\n"},{"id":"260983","messageId":"1431357920-25090-19-git-send-email-mhagger@alum.mit.edu","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 18/18] reflog_expire(): integrate lock_ref_sha1_basic() errors into ours","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-11T15:25:20Z","receivedAt":"2015-05-11T15:25:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now that lock_ref_sha1_basic() gives us back its error messages via a\nstrbuf, incorporate its error message into our error message rather\nthan emitting two separate error messages.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex bc4b1ab..97043fd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -4118,9 +4118,9 @@ int reflog_expire(const char *refname, const unsigned char *sha1,\n \t */\n \tlock = lock_ref_sha1_basic(refname, sha1, NULL, NULL, 0, &type, &err);\n \tif (!lock) {\n-\t\terror(\"%s\", err.buf);\n+\t\terror(\"cannot lock ref '%s': %s\", refname, err.buf);\n \t\tstrbuf_release(&err);\n-\t\treturn error(\"cannot lock ref '%s'\", refname);\n+\t\treturn -1;\n \t}\n \tif (!reflog_exists(refname)) {\n \t\tunlock_ref(lock);\n-- \n2.1.4\n"},{"id":"261004","messageId":"xmqqwq0fngk9.fsf@gitster.dls.corp.google.com","threadId":"39295","inReplyTo":"1431357920-25090-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 00/18] Improve handling of D/F conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-11T17:56:54Z","receivedAt":"2015-05-11T17:56:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> * Rebase to master rather than depending on\n> mh/ref-lock-avoid-running-out-of-fds, as did v1. There were no\n> significant dependencies between the two patch series, and now that\n> mh/ref-lock-avoid-running-out-of-fds (now renamed to\n> mh/write-refs-sooner.*) has been backported to 2.2, the dependency\n> makes even less sense.\n\nThanks, that makes sense.\n"},{"id":"261006","messageId":"CAGZ79kbAq81B-bvCQsn0a6nMbbVKqWFh1TrD2pbNggH34EgKvA@mail.gmail.com","threadId":"39295","inReplyTo":"xmqqwq0fngk9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 00/18] Improve handling of D/F conflicts","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-11T18:10:48Z","receivedAt":"2015-05-11T18:10:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 11, 2015 at 10:56 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n>\n>> * Rebase to master rather than depending on\n>> mh/ref-lock-avoid-running-out-of-fds, as did v1. There were no\n>> significant dependencies between the two patch series, and now that\n>> mh/ref-lock-avoid-running-out-of-fds (now renamed to\n>> mh/write-refs-sooner.*) has been backported to 2.2, the dependency\n>> makes even less sense.\n>\n> Thanks, that makes sense.\n>\n\nThe series is Reviewed-by: Stefan Beller \n\nThanks,\nStefan\n"},{"id":"261010","messageId":"xmqqpp672bh2.fsf@gitster.dls.corp.google.com","threadId":"39295","inReplyTo":"1431357920-25090-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-11T18:52:25Z","receivedAt":"2015-05-11T18:52:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> t/t1404-update-ref-df-conflicts.sh | 107 +++++++++++++++++++++++++++++++++++++\n> 1 file changed, 107 insertions(+)\n> create mode 100755 t/t1404-update-ref-df-conflicts.sh\n>\n> diff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh\n> new file mode 100755\n> index 0000000..ed1b93e\n> --- /dev/null\n> +++ b/t/t1404-update-ref-df-conflicts.sh\n> @@ -0,1 +1,107 @@\n\nWhere does this \"-0,1\" come from???\n\nAfter fixing it up and then running \"format-patch\" (or \"show\"), I\nwould get\n\n diff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh\n new file mode 100755\n index 0000000..2541a23\n --- /dev/null\n +++ b/t/t1404-update-ref-df-conflicts.sh\n @@ -0,0 +1,107 @@\n ...\n\nwhich is more in line with what I would expect.\n"},{"id":"261013","messageId":"xmqqlhgu3nyl.fsf@gitster.dls.corp.google.com","threadId":"39295","inReplyTo":"1431357920-25090-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-11T19:37:22Z","receivedAt":"2015-05-11T19:37:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> +test_update_rejected () {\n> +\tprefix=\"$1\" &&\n> +\tbefore=\"$2\" &&\n> +\tpack=\"$3\" &&\n> +\tcreate=\"$4\" &&\n> +\terror=\"$5\" &&\n> +\tprintf \"create $prefix/%s $C\\n\" $before |\n> +\tgit update-ref --stdin &&\n> +\tgit for-each-ref $prefix >unchanged &&\n> +\tif $pack\n> +\tthen\n> +\t\tgit pack-refs --all\n> +\tfi &&\n> +\tprintf \"create $prefix/%s $C\\n\" $create >input &&\n> +\ttest_must_fail git update-ref --stdin output.err &&\n> +\tgrep -F \"$error\" output.err &&\n\nI am not 100% confident that \"grep -F\" is the right thing to use\nhere. I checked all the error message these tests are expecting and\nnone seems to go through _(), but is it fundamental that these\nerrors should not be translated?\n"},{"id":"261041","messageId":"5551BA88.3090900@alum.mit.edu","threadId":"39295","inReplyTo":"xmqqlhgu3nyl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-12T08:32:08Z","receivedAt":"2015-05-12T08:32:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/11/2015 09:37 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> +test_update_rejected () {\n>> +\tprefix=\"$1\" &&\n>> +\tbefore=\"$2\" &&\n>> +\tpack=\"$3\" &&\n>> +\tcreate=\"$4\" &&\n>> +\terror=\"$5\" &&\n>> +\tprintf \"create $prefix/%s $C\\n\" $before |\n>> +\tgit update-ref --stdin &&\n>> +\tgit for-each-ref $prefix >unchanged &&\n>> +\tif $pack\n>> +\tthen\n>> +\t\tgit pack-refs --all\n>> +\tfi &&\n>> +\tprintf \"create $prefix/%s $C\\n\" $create >input &&\n>> +\ttest_must_fail git update-ref --stdin output.err &&\n>> +\tgrep -F \"$error\" output.err &&\n> \n> I am not 100% confident that \"grep -F\" is the right thing to use\n> here. I checked all the error message these tests are expecting and\n> none seems to go through _(), but is it fundamental that these\n> errors should not be translated?\n\nAren't tests run in the \"C\" locale so that tests don't have to worry\nabout i18n?\n\nRegarding \"grep -F\", what would be a better alternative, given that I\nwant to test that the conflict is being reported correctly?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"261042","messageId":"5551BD94.4050200@alum.mit.edu","threadId":"39295","inReplyTo":"xmqqpp672bh2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-12T08:45:08Z","receivedAt":"2015-05-12T08:45:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/11/2015 08:52 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> t/t1404-update-ref-df-conflicts.sh | 107 +++++++++++++++++++++++++++++++++++++\n>> 1 file changed, 107 insertions(+)\n>> create mode 100755 t/t1404-update-ref-df-conflicts.sh\n>>\n>> diff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh\n>> new file mode 100755\n>> index 0000000..ed1b93e\n>> --- /dev/null\n>> +++ b/t/t1404-update-ref-df-conflicts.sh\n>> @@ -0,1 +1,107 @@\n> \n> Where does this \"-0,1\" come from???\n\nGood question. I had forgotten to insert the space in\n\n test_update_rejected () {\n ^ space was missing when I ran format-patch\n\nso I edited the email file in emacs. Emacs detected that the file\ncontained a patch, so when I edited the hunk, it tried to adjust the\nhunk line numbers to keep them in agreement with the patch. Obviously,\nit screwed up. So the incorrect line numbers were caused by a bug in\nemacs diff-mode.\n\nSorry for the confusion.\n\n> [...]\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"261052","messageId":"CAPc5daVQEMv-QPDpyNVGzcjrjrw20QLzm8dkC0ZYAX=Y4zGaBQ@mail.gmail.com","threadId":"39295","inReplyTo":"5551BA88.3090900@alum.mit.edu","subject":"Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-12T15:45:19Z","receivedAt":"2015-05-12T15:45:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Tue, May 12, 2015 at 1:32 AM, Michael Haggerty wrote:\n> On 05/11/2015 09:37 PM, Junio C Hamano wrote:\n>> Michael Haggerty writes:\n>> ...\n>>> + test_must_fail git update-ref --stdin output.err &&\n>>> + grep -F \"$error\" output.err &&\n>>\n>> I am not 100% confident that \"grep -F\" is the right thing to use\n>> here. I checked all the error message these tests are expecting and\n>> none seems to go through _(), but is it fundamental that these\n>> errors should not be translated?\n>\n> Aren't tests run in the \"C\" locale so that tests don't have to worry\n> about i18n?\n\nBut there is a i18n markings test, for which test-i18ngrep was invented for.\n"},{"id":"261078","messageId":"xmqq8uctfqy5.fsf@gitster.dls.corp.google.com","threadId":"39295","inReplyTo":"1431357920-25090-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 02/18] is_refname_available(): revamp the comments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-12T21:04:18Z","receivedAt":"2015-05-12T21:04:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Change the comments to a running example of running the function with\n> refname set to \"refs/foo/bar\". Add some more explanation of the logic.\n\nI initially had a hard time understanding what these two \"running\"s\nwere before reading the changes. They all look good.\n\nThanks.\n\n\n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 69 +++++++++++++++++++++++++++++++++++++++++++++---------------------\n> 1 file changed, 47 insertions(+), 22 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 47e4e53..776bbce 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -876,9 +876,9 @@ static void report_refname_conflict(struct ref_entry *entry,\n> * operation).\n> *\n> * Two reference names conflict if one of them exactly matches the\n> - * leading components of the other; e.g., \"foo/bar\" conflicts with\n> - * both \"foo\" and with \"foo/bar/baz\" but not with \"foo/bar\" or\n> - * \"foo/barbados\".\n> + * leading components of the other; e.g., \"refs/foo/bar\" conflicts\n> + * with both \"refs/foo\" and with \"refs/foo/bar/baz\" but not with\n> + * \"refs/foo/bar\" or \"refs/foo/barbados\".\n> *\n> * skip must be sorted.\n> */\n> @@ -891,19 +891,39 @@ static int is_refname_available(const char *refname,\n> \tint pos;\n> \tchar *dirname;\n> \n> +\t/*\n> +\t * For the sake of comments in this function, suppose that\n> +\t * refname is \"refs/foo/bar\".\n> +\t */\n> +\n> \tfor (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {\n> \t\t/*\n> -\t\t * We are still at a leading dir of the refname; we are\n> -\t\t * looking for a conflict with a leaf entry.\n> -\t\t *\n> -\t\t * If we find one, we still must make sure it is\n> -\t\t * not in \"skip\".\n> +\t\t * We are still at a leading dir of the refname (e.g.,\n> +\t\t * \"refs/foo\"; if there is a reference with that name,\n> +\t\t * it is a conflict, *unless* it is in skip.\n> \t\t */\n> \t\tpos = search_ref_dir(dir, refname, slash - refname);\n> \t\tif (pos >= 0) {\n> +\t\t\t/*\n> +\t\t\t * We found a reference whose name is a proper\n> +\t\t\t * prefix of refname; e.g., \"refs/foo\".\n> +\t\t\t */\n> \t\t\tstruct ref_entry *entry = dir->entries[pos];\n> -\t\t\tif (entry_matches(entry, skip))\n> +\t\t\tif (entry_matches(entry, skip)) {\n> +\t\t\t\t/*\n> +\t\t\t\t * The reference we just found, e.g.,\n> +\t\t\t\t * \"refs/foo\", is also in skip, so it\n> +\t\t\t\t * is not considered a conflict.\n> +\t\t\t\t * Moreover, the fact that \"refs/foo\"\n> +\t\t\t\t * exists means that there cannot be\n> +\t\t\t\t * any references anywhere under the\n> +\t\t\t\t * \"refs/foo/\" namespace (because they\n> +\t\t\t\t * would have conflicted with\n> +\t\t\t\t * \"refs/foo\"). So we can stop looking\n> +\t\t\t\t * now and return true.\n> +\t\t\t\t */\n> \t\t\t\treturn 1;\n> +\t\t\t}\n> \t\t\treport_refname_conflict(entry, refname);\n> \t\t\treturn 0;\n> \t\t}\n> @@ -911,19 +931,29 @@ static int is_refname_available(const char *refname,\n> \n> \t\t/*\n> \t\t * Otherwise, we can try to continue our search with\n> -\t\t * the next component; if we come up empty, we know\n> -\t\t * there is nothing under this whole prefix.\n> +\t\t * the next component. So try to look up the\n> +\t\t * directory, e.g., \"refs/foo/\".\n> \t\t */\n> \t\tpos = search_ref_dir(dir, refname, slash + 1 - refname);\n> -\t\tif (pos < 0)\n> +\t\tif (pos < 0) {\n> +\t\t\t/*\n> +\t\t\t * There was no directory \"refs/foo/\", so\n> +\t\t\t * there is nothing under this whole prefix,\n> +\t\t\t * and we are OK.\n> +\t\t\t */\n> \t\t\treturn 1;\n> +\t\t}\n> \n> \t\tdir = get_ref_dir(dir->entries[pos]);\n> \t}\n> \n> \t/*\n> -\t * We are at the leaf of our refname; we want to\n> -\t * make sure there are no directories which match it.\n> +\t * We are at the leaf of our refname (e.g., \"refs/foo/bar\").\n> +\t * There is no point in searching for a reference with that\n> +\t * name, because a refname isn't considered to conflict with\n> +\t * itself. But we still need to check for references whose\n> +\t * names are in the \"refs/foo/bar/\" namespace, because they\n> +\t * *do* conflict.\n> \t */\n> \tlen = strlen(refname);\n> \tdirname = xmallocz(len + 1);\n> @@ -933,9 +963,9 @@ static int is_refname_available(const char *refname,\n> \n> \tif (pos >= 0) {\n> \t\t/*\n> -\t\t * We found a directory named \"refname\". It is a\n> -\t\t * problem iff it contains any ref that is not\n> -\t\t * in \"skip\".\n> +\t\t * We found a directory named \"$refname/\" (e.g.,\n> +\t\t * \"refs/foo/bar/\"). It is a problem iff it contains\n> +\t\t * any ref that is not in \"skip\".\n> \t\t */\n> \t\tstruct ref_entry *entry = dir->entries[pos];\n> \t\tstruct ref_dir *dir = get_ref_dir(entry);\n> @@ -950,11 +980,6 @@ static int is_refname_available(const char *refname,\n> \t\treturn 0;\n> \t}\n> \n> -\t/*\n> -\t * There is no point in searching for another leaf\n> -\t * node which matches it; such an entry would be the\n> -\t * ref we are looking for, not a conflict.\n> -\t */\n> \treturn 1;\n> }\n"},{"id":"261079","messageId":"xmqq4mnhfque.fsf@gitster.dls.corp.google.com","threadId":"39295","inReplyTo":"1431357920-25090-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 03/18] is_refname_available(): avoid shadowing \"dir\" variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-12T21:06:33Z","receivedAt":"2015-05-12T21:06:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> The function had a \"dir\" parameter that was shadowed by a local \"dir\"\n> variable within a code block. Use the former in place of the latter.\n> (This is consistent with \"dir\"'s use elsewhere in the function.)\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 776bbce..9d87e84 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -967,10 +967,10 @@ static int is_refname_available(const char *refname,\n> \t\t * \"refs/foo/bar/\"). It is a problem iff it contains\n> \t\t * any ref that is not in \"skip\".\n> \t\t */\n> -\t\tstruct ref_entry *entry = dir->entries[pos];\n> -\t\tstruct ref_dir *dir = get_ref_dir(entry);\n> \t\tstruct nonmatching_ref_data data;\n\nWow, the original is a tricky code, but that helps us be confident\nthat this change is not breaking anything by \"unmasking\" the value\nof incoming \"dir\" ;-)\n\n> +\t\tstruct ref_entry *entry = dir->entries[pos];\n> \n> +\t\tdir = get_ref_dir(entry);\n> \t\tdata.skip = skip;\n> \t\tsort_ref_dir(dir);\n> \t\tif (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data))\n"},{"id":"261083","messageId":"xmqqzj59eblj.fsf@gitster.dls.corp.google.com","threadId":"39295","inReplyTo":"1431357920-25090-7-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 06/18] report_refname_conflict(): inline function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-12T21:21:12Z","receivedAt":"2015-05-12T21:21:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> It wasn't pulling its weight. And we are about to need code similar to\n> this where no ref_entry is available and with more diverse error\n> messages. Rather than try to generalize the function, just inline it.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n\nI am not sure \"not pulling its weight\" in this and also 05/18 is a\nfair judgement. A short helper function sometimes helps readability\nby giving an extra abstraction.\n\nBut I agree 100% with these two steps that they weren't making them\neasier to read. A raw call to error() shows clearly that we are\nreporting an error and more importantly by losing an intermediary\nwe do not have to wonder at the call site if there is anything else\ngoing on in that \"report\" function (where there isn't).\n\nThanks.\n\n\n> refs.c | 10 ++--------\n> 1 file changed, 2 insertions(+), 8 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 6bdd34f..7422594 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -857,12 +857,6 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)\n> \treturn 1;\n> }\n> \n> -static void report_refname_conflict(struct ref_entry *entry,\n> -\t\t\t\t const char *refname)\n> -{\n> -\terror(\"'%s' exists; cannot create '%s'\", entry->name, refname);\n> -}\n> -\n> /*\n> * Return true iff a reference named refname could be created without\n> * conflicting with the name of an existing reference in dir. If\n> @@ -918,7 +912,7 @@ static int is_refname_available(const char *refname,\n> \t\t\t\t */\n> \t\t\t\treturn 1;\n> \t\t\t}\n> -\t\t\treport_refname_conflict(entry, refname);\n> +\t\t\terror(\"'%s' exists; cannot create '%s'\", entry->name, refname);\n> \t\t\treturn 0;\n> \t\t}\n> \n> @@ -969,7 +963,7 @@ static int is_refname_available(const char *refname,\n> \t\tif (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data))\n> \t\t\treturn 1;\n> \n> -\t\treport_refname_conflict(data.found, refname);\n> +\t\terror(\"'%s' exists; cannot create '%s'\", data.found->name, refname);\n> \t\treturn 0;\n> \t}\n"},{"id":"261201","messageId":"5553B1E0.9080008@alum.mit.edu","threadId":"39295","inReplyTo":"CAPc5daVQEMv-QPDpyNVGzcjrjrw20QLzm8dkC0ZYAX=Y4zGaBQ@mail.gmail.com","subject":"Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-13T20:19:44Z","receivedAt":"2015-05-13T20:19:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/12/2015 05:45 PM, Junio C Hamano wrote:\n> On Tue, May 12, 2015 at 1:32 AM, Michael Haggerty wrote:\n>> On 05/11/2015 09:37 PM, Junio C Hamano wrote:\n>>> Michael Haggerty writes:\n>>> ...\n>>>> + test_must_fail git update-ref --stdin output.err &&\n>>>> + grep -F \"$error\" output.err &&\n>>>\n>>> I am not 100% confident that \"grep -F\" is the right thing to use\n>>> here. I checked all the error message these tests are expecting and\n>>> none seems to go through _(), but is it fundamental that these\n>>> errors should not be translated?\n>>\n>> Aren't tests run in the \"C\" locale so that tests don't have to worry\n>> about i18n?\n> \n> But there is a i18n markings test, for which test-i18ngrep was invented for.\n\nThanks for the info. I wasn't aware of that facility.\n\nSo if I understand correctly, s/grep/test_i18ngrep/ will address your\nconcern? That's fine with me.\n\nMeanwhile, I realized that verify_lock() also writes errors directly to\nstderr rather than storing them to a strbuf. I have a few patches that\nfix this [1], which would thematically fit well on the end of this patch\nseries. But maybe this patch series is too big already?\n\nIn any case I'll be away for a 4-day weekend so it will have to wait\nuntil Monday.\n\nMichael\n\n[1] They are available as branch \"verify-lock-strbuf-err\" on\n\n https://github.com/mhagger/git\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"261239","messageId":"xmqqr3qj857t.fsf@gitster.dls.corp.google.com","threadId":"39295","inReplyTo":"5553B1E0.9080008@alum.mit.edu","subject":"Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-14T17:00:06Z","receivedAt":"2015-05-14T17:00:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n>> But there is a i18n markings test, for which test-i18ngrep was invented for.\n>\n> Thanks for the info. I wasn't aware of that facility.\n>\n> So if I understand correctly, s/grep/test_i18ngrep/ will address your\n> concern? That's fine with me.\n\nThinking about it again, should these messages ever be translated,\nor are they plumbing messages that should never get translated?\n\nIf the latter, then 'grep' is the right thing to do; in fact, it\nwould be a bug in the test if we used test_i18ngrep.\n\n\tSide note: besides, I think gettext-poison tests have bitrot\n\tand are no longer very useful (if they ever were, that is).\n\n> In any case I'll be away for a 4-day weekend so it will have to wait\n> until Monday.\n\nEnjoy your long weekend ;-)\n"},{"id":"261957","messageId":"555FB219.3090300@alum.mit.edu","threadId":"39295","inReplyTo":"xmqqr3qj857t.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-22T22:47:53Z","receivedAt":"2015-05-22T22:47:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/14/2015 07:00 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>>> But there is a i18n markings test, for which test-i18ngrep was invented for.\n>>\n>> Thanks for the info. I wasn't aware of that facility.\n>>\n>> So if I understand correctly, s/grep/test_i18ngrep/ will address your\n>> concern? That's fine with me.\n> \n> Thinking about it again, should these messages ever be translated,\n> or are they plumbing messages that should never get translated?\n> \n> If the latter, then 'grep' is the right thing to do; in fact, it\n> would be a bug in the test if we used test_i18ngrep.\n> \n> \tSide note: besides, I think gettext-poison tests have bitrot\n> \tand are no longer very useful (if they ever were, that is).\n\nI don't really know how to decide which messages should be translatable\nand which not.\n\nObviously 'update-ref' is a plumbing command, but these same error\nmessages can be emitted by any reference transaction, such as when a\npush fails, in which case they are visible to the user.\n\nSome other errors that can result from ref transaction failures are\ntested in t1400 with 'grep'.\n\nI think using 'grep' is OK for now, and if they are internationalized in\nthe future the breakage will be pretty obvious and straightforward to fix.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"261972","messageId":"xmqqegm8dsio.fsf@gitster.dls.corp.google.com","threadId":"39295","inReplyTo":"555FB219.3090300@alum.mit.edu","subject":"Re: [PATCH v2 01/18] t1404: new tests of ref D/F conflicts within transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-23T00:52:47Z","receivedAt":"2015-05-23T00:52:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> I think using 'grep' is OK for now, and if they are internationalized in\n> the future the breakage will be pretty obvious and straightforward to fix.\n\nYeah, I think so, too.\n\nThanks.\n"}]}