{"thread":{"id":"39201","subject":"[PATCH 02/18] is_refname_available(): explain the reason for an early exit","startedAt":"2015-05-01T12:25:40Z","lastAt":"2015-05-05T16:12:38Z","messageCount":25,"participants":["Michael Haggerty","Stefan Beller","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":18},"messages":[{"id":"260315","messageId":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":null,"subject":"[PATCH 00/18] Improve handling of D/F conflicts","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:40Z","receivedAt":"2015-05-01T12:25:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"We currently don't handle D/F conflicts very well.\n\nA \"D/F conflict\" is what we call a conflict between references like\n\"refs/foo\" and \"refs/foo/bar\". Such references cannot coexist because,\nwhen stored as loose references, \"refs/foo\" would have to be both a\ndirectory and a file.\n\nThe problems fixed by this series:\n\n* We had no tests of D/F conflict handling within transactions.\n\n* D/F conflicts between references being created in a single\n transaction used to be detected too late, possibly after part of the\n transaction had already been committed.\n\n* D/F errors against loose references were typically reported as\n locking errors, which was not very illuminating. Also, D/F errors\n were typically reported in two separate error messages, like\n\n error: there are still refs under 'refs/foo'\n fatal: Cannot lock the ref 'refs/foo'.\n\n or\n\n error: unable to resolve reference refs/foo/bar: Not a directory\n fatal: Cannot lock the ref 'refs/foo/bar'.\n\n which was confusing.\n\nThere is probably still room for improving the error messages.\n\nThis patch series applies on top of\nmh/ref-lock-avoid-running-out-of-fds. I did it that way because I\nexpected significant conflicts between the series, and the older\nseries is simple/mature enough that I expect it to be merged early in\nthe post-2.4 cycle. But in retrospect it turns out that there are only\nminor conflicts between the two series. So if you would like me to\nrebase this series to another starting point, please let me know.\n\nThis series is also available from my GitHub account,\n\n https://github.com/mhagger/git branch check-df-conflicts-earlier\n\nMichael Haggerty (18):\n t1404: new tests of D/F conflicts within ref transactions\n is_refname_available(): explain the reason for an early exit\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 processed 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 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 | 280 +++++++++++++++++++++++--------------\n t/t1400-update-ref.sh | 14 +-\n t/t1404-update-ref-df-conflicts.sh | 107 ++++++++++++++\n 3 files changed, 286 insertions(+), 115 deletions(-)\n create mode 100755 t/t1404-update-ref-df-conflicts.sh\n\n-- \n2.1.4\n"},{"id":"260316","messageId":"1430483158-14349-2-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 01/18] t1404: new tests of D/F conflicts within ref transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:41Z","receivedAt":"2015-05-01T12:25:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add some tests of D/F conflicts (as in, the type of directory vs. file\nconflict that exists between references \"refs/foo\" and \"refs/foo/bar\")\nin 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..2fc2ac6\n--- /dev/null\n+++ b/t/t1404-update-ref-df-conflicts.sh\n@@ -0,0 +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":"260314","messageId":"1430483158-14349-3-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 02/18] is_refname_available(): explain the reason for an early exit","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:42Z","receivedAt":"2015-05-01T12:25:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The reason why we can exit early if we find a reference in skip whose\nname is a prefix of refname is a bit subtle, so explain it in a\ncomment.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 14 +++++++++++++-\n 1 file changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 2bdd93c..ab438a5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -907,8 +907,20 @@ static int is_refname_available(const char *refname,\n \t\tpos = search_ref_dir(dir, refname, slash - refname);\n \t\tif (pos >= 0) {\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 fact that entry is a ref whose\n+\t\t\t\t * name is a prefix of refname means\n+\t\t\t\t * that there cannot be any other ref\n+\t\t\t\t * whose name starts with that prefix\n+\t\t\t\t * (because it would have been a D/F\n+\t\t\t\t * conflict with entry). So, since we\n+\t\t\t\t * don't care about entry (because it\n+\t\t\t\t * is in skip), 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-- \n2.1.4\n"},{"id":"260319","messageId":"1430483158-14349-4-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 03/18] is_refname_available(): avoid shadowing \"dir\" variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:43Z","receivedAt":"2015-05-01T12:25:43Z","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 ab438a5..09d753f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -954,10 +954,10 @@ static int is_refname_available(const char *refname,\n \t\t * problem iff it contains any ref that is not\n \t\t * 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":"260330","messageId":"1430483158-14349-5-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 04/18] is_refname_available(): convert local variable \"dirname\" to strbuf","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:44Z","receivedAt":"2015-05-01T12:25:44Z","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 09d753f..2ef1c5e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -892,9 +892,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 \tfor (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {\n \t\t/*\n@@ -942,11 +941,10 @@ static int is_refname_available(const char *refname,\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 */\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":"260317","messageId":"1430483158-14349-6-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 05/18] entry_matches(): inline function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:45Z","receivedAt":"2015-05-01T12:25:45Z","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 2ef1c5e..e9c9146 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -846,11 +846,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@@ -860,7 +855,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@@ -906,7 +901,7 @@ static int is_refname_available(const char *refname,\n \t\tpos = search_ref_dir(dir, refname, slash - refname);\n \t\tif (pos >= 0) {\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 fact that entry is a ref whose\n \t\t\t\t * name is a prefix of refname means\n-- \n2.1.4\n"},{"id":"260318","messageId":"1430483158-14349-7-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 06/18] report_refname_conflict(): inline function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:46Z","receivedAt":"2015-05-01T12:25:46Z","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 e9c9146..f1da87b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -862,12 +862,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@@ -915,7 +909,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@@ -956,7 +950,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":"260324","messageId":"1430483158-14349-8-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 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-01T12:25:47Z","receivedAt":"2015-05-01T12:25:47Z","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 f1da87b..0ec3f0a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -848,7 +848,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@@ -858,7 +858,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@@ -950,7 +950,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":"260326","messageId":"1430483158-14349-9-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 08/18] is_refname_available(): use dirname in first loop","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:48Z","receivedAt":"2015-05-01T12:25:48Z","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 | 43 ++++++++++++++++++++++++++++---------------\n 1 file changed, 28 insertions(+), 15 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 0ec3f0a..8e929c7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -883,8 +883,13 @@ 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+\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; we are\n \t\t * looking for a conflict with a leaf entry.\n@@ -892,10 +897,9 @@ static int is_refname_available(const char *refname,\n \t\t * If we find one, we still must make sure it is\n \t\t * not 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\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 fact that entry is a ref whose\n \t\t\t\t * name is a prefix of refname means\n@@ -907,10 +911,11 @@ static int is_refname_available(const char *refname,\n \t\t\t\t * is in skip), 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@@ -919,9 +924,12 @@ static int is_refname_available(const char *refname,\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 */\n-\t\tpos = search_ref_dir(dir, refname, slash + 1 - refname);\n-\t\tif (pos < 0)\n-\t\t\treturn 1;\n+\t\tstrbuf_addch(&dirname, '/');\n+\t\tpos = search_ref_dir(dir, dirname.buf, dirname.len);\n+\t\tif (pos < 0) {\n+\t\t\tret = 1;\n+\t\t\tgoto cleanup;\n+\t\t}\n \n \t\tdir = get_ref_dir(dir->entries[pos]);\n \t}\n@@ -930,10 +938,9 @@ static int is_refname_available(const char *refname,\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 */\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@@ -947,12 +954,14 @@ 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 \t/*\n@@ -960,7 +969,11 @@ static int is_refname_available(const char *refname,\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+\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":"260332","messageId":"1430483158-14349-10-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 09/18] ref_transaction_commit(): use a string_list for detecting duplicates","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:49Z","receivedAt":"2015-05-01T12:25:49Z","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 8e929c7..fca3ef1 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3718,25 +3718,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@@ -3750,6 +3743,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@@ -3761,9 +3755,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@@ -3879,6 +3875,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":"260328","messageId":"1430483158-14349-11-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 10/18] refs: check for D/F conflicts among refs processed in a transaction","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:50Z","receivedAt":"2015-05-01T12:25:50Z","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, 90 insertions(+), 68 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex fca3ef1..81fbb33 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -864,19 +864,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., \"foo/bar\" conflicts with\n * both \"foo\" and with \"foo/bar/baz\" but not with \"foo/bar\" or\n * \"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@@ -891,84 +894,101 @@ static int is_refname_available(const char *refname,\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; we are\n-\t\t * looking for a conflict with a leaf entry.\n+\t\t * We are still at a leading dir of refname; look for\n+\t\t * 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 */\n-\t\tpos = search_ref_dir(dir, dirname.buf, dirname.len);\n-\t\tif (pos >= 0) {\n-\t\t\tif (skip && string_list_has_string(skip, dirname.buf)) {\n-\t\t\t\t/*\n-\t\t\t\t * The fact that entry is a ref whose\n-\t\t\t\t * name is a prefix of refname means\n-\t\t\t\t * that there cannot be any other ref\n-\t\t\t\t * whose name starts with that prefix\n-\t\t\t\t * (because it would have been a D/F\n-\t\t\t\t * conflict with entry). So, since we\n-\t\t\t\t * don't care about entry (because it\n-\t\t\t\t * is in skip), we can stop looking\n-\t\t\t\t * now and return true.\n-\t\t\t\t */\n-\t\t\t\tret = 1;\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\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; if we come up empty, we know\n-\t\t * there is nothing under this whole prefix.\n+\t\t * the next component. If we come up empty, we know\n+\t\t * there is nothing under this whole prefix, but even\n+\t\t * in that case we still have to continue the search\n+\t\t * 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\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\tdir = (pos < 0) ? NULL : get_ref_dir(dir->entries[pos]);\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; we want to make sure\n+\t * that there are no directories that match it. (There is no\n+\t * point in searching for another leaf node that matches it;\n+\t * such an entry would be the ref we are looking for, which is\n+\t * not a conflict.)\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\". It is\n+\t\t\t * a problem if it contains any ref that is\n+\t\t\t * not in \"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\". It is a\n-\t\t * problem iff it contains any ref that is not\n-\t\t * 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/*\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+\t/* No conflicts were found */\n \tret = 1;\n \n cleanup:\n@@ -2289,6 +2309,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@@ -2343,7 +2364,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@@ -2783,8 +2804,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@@ -2844,7 +2865,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@@ -2859,7 +2880,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@@ -3766,9 +3787,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t/*\n \t * Acquire all locks, verify old values if provided, check\n-\t * that new values are valid, and write new values to the\n-\t * lockfiles, ready to be activated. Only keep one lockfile\n-\t * open at a time to avoid running out of file descriptors.\n+\t * that new values are valid, check for D/F conflicts, and\n+\t * write new values to the lockfiles, ready to be activated.\n+\t * Only keep one lockfile open at a time to avoid running out\n+\t * of file descriptors.\n \t */\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n@@ -3779,7 +3801,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\tupdate->flags,\n \t\t\t\t&update->type);\n \t\tif (!update->lock) {\n@@ -4076,7 +4098,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 2fc2ac6..136fb48 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":"260325","messageId":"1430483158-14349-12-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 11/18] verify_refname_available(): rename function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:51Z","receivedAt":"2015-05-01T12:25:51Z","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 81fbb33..cd8aeed 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -268,7 +268,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@@ -863,13 +863,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., \"foo/bar\" conflicts with\n@@ -878,15 +879,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 \tstrbuf_grow(&dirname, strlen(refname) + 1);\n \tfor (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {\n@@ -989,7 +990,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@@ -2364,7 +2365,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@@ -2804,8 +2805,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":"260329","messageId":"1430483158-14349-13-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 12/18] verify_refname_available(): report errors via a \"struct strbuf *err\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:52Z","receivedAt":"2015-05-01T12:25:52Z","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 cd8aeed..730e248 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -865,12 +865,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., \"foo/bar\" conflicts with\n@@ -882,13 +882,16 @@ 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 \tstruct strbuf dirname = STRBUF_INIT;\n \tint ret = -1;\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@@ -905,16 +908,16 @@ static int verify_refname_available(const char *refname,\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\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@@ -958,8 +961,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@@ -982,8 +985,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@@ -2322,6 +2325,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 \n@@ -2365,7 +2369,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@@ -2405,10 +2411,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@@ -2416,6 +2420,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@@ -2802,14 +2807,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":"260327","messageId":"1430483158-14349-14-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 13/18] lock_ref_sha1_basic(): report errors via a \"struct strbuf *err\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:53Z","receivedAt":"2015-05-01T12:25:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"For now, change the callers spew the error to stderr like before. But\neventually, they should incorporate the reason for the failure into\ntheir 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 730e248..1b466bc 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2315,7 +2315,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@@ -2325,7 +2326,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 \n@@ -2348,7 +2350,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@@ -2358,8 +2361,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@@ -2370,8 +2373,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@@ -2397,7 +2399,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@@ -2411,8 +2413,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@@ -2420,7 +2421,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@@ -2836,6 +2836,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@@ -2878,8 +2879,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@@ -2893,8 +2896,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@@ -3816,11 +3821,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\tupdate->flags,\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@@ -4100,6 +4108,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@@ -4111,9 +4120,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":"260320","messageId":"1430483158-14349-15-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 14/18] lock_ref_sha1_basic(): improve diagnostics for D/F conflicts","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:54Z","receivedAt":"2015-05-01T12:25:54Z","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 that is likely caused by a D/F conflict,\ninvoke verify_refname_available() to try to generate a more helpful\nerror message. If that function doesn't detect an error (which could\nhappen if, for example, some non-reference file is blocking the\ndeletion of an otherwise-empty directory tree or if there was a race\nwith another process that just deleted the offending reference), then\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 1b466bc..cdbd31a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2350,8 +2350,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@@ -2361,8 +2365,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 136fb48..11b6d5b 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":"260331","messageId":"1430483158-14349-16-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 15/18] rename_ref(): integrate lock_ref_sha1_basic() errors into ours","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:55Z","receivedAt":"2015-05-01T12:25:55Z","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 cdbd31a..6077e83 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2889,9 +2889,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@@ -2906,9 +2905,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":"260323","messageId":"1430483158-14349-17-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 16/18] ref_transaction_commit(): provide better error messages","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:56Z","receivedAt":"2015-05-01T12:25:56Z","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 | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6077e83..2d0d8bd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3830,13 +3830,14 @@ 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\tconst char *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\tgoto cleanup;\n \t\t}\n \t\tif ((update->flags & REF_HAVE_NEW) && !(update->flags & REF_DELETING)) {\n-- \n2.1.4\n"},{"id":"260321","messageId":"1430483158-14349-18-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 17/18] ref_transaction_commit(): delete extra \"the\" from error message","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:57Z","receivedAt":"2015-05-01T12:25:57Z","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 2d0d8bd..f64f6ae 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3836,7 +3836,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\tgoto cleanup;\n \t\t}\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 636d3a1..ba89f4c 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":"260322","messageId":"1430483158-14349-19-git-send-email-mhagger@alum.mit.edu","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 18/18] reflog_expire(): integrate lock_ref_sha1_basic() errors into ours","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-01T12:25:58Z","receivedAt":"2015-05-01T12:25:58Z","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 f64f6ae..0bff903 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -4129,9 +4129,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":"260359","messageId":"CAGZ79kZDCMj1P1H3+LiW-Lk8S8TS91yJ_G4scmVSvg++3n7dvw@mail.gmail.com","threadId":"39201","inReplyTo":"1430483158-14349-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 02/18] is_refname_available(): explain the reason for an early exit","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-01T17:21:26Z","receivedAt":"2015-05-01T17:21:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 1, 2015 at 5:25 AM, Michael Haggerty wrote:\n> The reason why we can exit early if we find a reference in skip whose\n> name is a prefix of refname is a bit subtle, so explain it in a\n> comment.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 14 +++++++++++++-\n> 1 file changed, 13 insertions(+), 1 deletion(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 2bdd93c..ab438a5 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -907,8 +907,20 @@ static int is_refname_available(const char *refname,\n> pos = search_ref_dir(dir, refname, slash - refname);\n> if (pos >= 0) {\n> struct ref_entry *entry = dir->entries[pos];\n> - if (entry_matches(entry, skip))\n> + if (entry_matches(entry, skip)) {\n> + /*\n> + * The fact that entry is a ref whose\n> + * name is a prefix of refname means\n> + * that there cannot be any other ref\n> + * whose name starts with that prefix\n> + * (because it would have been a D/F\n> + * conflict with entry). So, since we\n> + * don't care about entry (because it\n> + * is in skip), we can stop looking\n> + * now and return true.\n\nAt first I thought this is not true, what about:\nrefs/heads/foo\nrefs/heads/foobar\nThey go well together and one is a prefix of the other.\nWhat is crucial is the existence of a '/' just between the\nprefix and the ending part, so\nrefs/heads/foo/bar doesn't fly here.\n\nThe assumption may be the case if the prefix itself always\nends with a /, which is probably the case here?\nI don't know if that is worth noting as well.\n\n> + */\n> return 1;\n> + }\n> report_refname_conflict(entry, refname);\n> return 0;\n> }\n> --\n> 2.1.4\n>\n"},{"id":"260446","messageId":"xmqqy4l6wgwm.fsf@gitster.dls.corp.google.com","threadId":"39201","inReplyTo":"1430483158-14349-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 00/18] Improve handling of D/F conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-03T02:09:45Z","receivedAt":"2015-05-03T02:09:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> We currently don't handle D/F conflicts very well.\n>\n> A \"D/F conflict\" is what we call a conflict between references like\n> \"refs/foo\" and \"refs/foo/bar\".\n\nCould you phrase that slightly differently, so that readers can tell\nbetween the usual D/F conflict that is between directory and file in\nthe tracked contents (i.e. conflict between the branches being\nmerged, killing of a directory necessary when checking out a file,\netc.) and this thing? They are similar in nature, but \"D/F\nconflict\" has been used to refer to the clashes that happen to the\nuser contents, not refnames. Starting a paragraph with \"... don't\nhandle D/F conflicts in refnames very well\" and then using \"D/F\nconflict\" as a short-hand for \"D/F conflict in refnames\" throughout\nthe rest of the message is perfectly fine, as long as the message\nnever talks about the D/F conflict in the traditional sense.\n\n> * D/F conflicts between references being created in a single\n> transaction used to be detected too late, possibly after part of the\n> transaction had already been committed.\n>\n> * D/F errors against loose references were typically reported as\n\nBe consistent by s/errors/conflicts/ perhaps?\n\n> This patch series applies on top of\n> mh/ref-lock-avoid-running-out-of-fds. I did it that way because I\n> expected significant conflicts between the series, and the older\n> series is simple/mature enough that I expect it to be merged early in\n> the post-2.4 cycle. But in retrospect it turns out that there are only\n> minor conflicts between the two series. So if you would like me to\n> rebase this series to another starting point, please let me know.\n\nThanks for being considerate about back-porting necessity.\n\nAs I have written, I actually would prefer to see that the other\ntopic, ref-lock-avoid-running-out-of-fds, to be made applicable to\nolder maintenance tracks. So...\n"},{"id":"260581","messageId":"CAPig+cQabRiiMhNyYJM4_bZsQ4OTfrSbJ51XAido3V49baLhHA@mail.gmail.com","threadId":"39201","inReplyTo":"1430483158-14349-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 01/18] t1404: new tests of D/F conflicts within ref transactions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-05T05:12:39Z","receivedAt":"2015-05-05T05:12:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 1, 2015 at 8:25 AM, Michael Haggerty wrote:\n> Add some tests of D/F conflicts (as in, the type of directory vs. file\n> conflict that exists between references \"refs/foo\" and \"refs/foo/bar\")\n> in the context of reference transactions.\n> [...]\n> Signed-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>\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..2fc2ac6\n> --- /dev/null\n> +++ b/t/t1404-update-ref-df-conflicts.sh\n> @@ -0,0 +1,107 @@\n> +#!/bin/sh\n> +\n> +test_description='Test git update-ref with D/F conflicts'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +\n> + git commit --allow-empty -m Initial\n\nBroken &&-chain.\n\n> + C=$(git rev-parse HEAD)\n> +\n> +'\n"},{"id":"260602","messageId":"5548DBDA.1000009@alum.mit.edu","threadId":"39201","inReplyTo":"CAGZ79kZDCMj1P1H3+LiW-Lk8S8TS91yJ_G4scmVSvg++3n7dvw@mail.gmail.com","subject":"Re: [PATCH 02/18] is_refname_available(): explain the reason for an early exit","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-05T15:03:54Z","receivedAt":"2015-05-05T15:03:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/01/2015 07:21 PM, Stefan Beller wrote:\n> On Fri, May 1, 2015 at 5:25 AM, Michael Haggerty wrote:\n>> The reason why we can exit early if we find a reference in skip whose\n>> name is a prefix of refname is a bit subtle, so explain it in a\n>> comment.\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n>> refs.c | 14 +++++++++++++-\n>> 1 file changed, 13 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/refs.c b/refs.c\n>> index 2bdd93c..ab438a5 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -907,8 +907,20 @@ static int is_refname_available(const char *refname,\n>> pos = search_ref_dir(dir, refname, slash - refname);\n>> if (pos >= 0) {\n>> struct ref_entry *entry = dir->entries[pos];\n>> - if (entry_matches(entry, skip))\n>> + if (entry_matches(entry, skip)) {\n>> + /*\n>> + * The fact that entry is a ref whose\n>> + * name is a prefix of refname means\n>> + * that there cannot be any other ref\n>> + * whose name starts with that prefix\n>> + * (because it would have been a D/F\n>> + * conflict with entry). So, since we\n>> + * don't care about entry (because it\n>> + * is in skip), we can stop looking\n>> + * now and return true.\n> \n> At first I thought this is not true, what about:\n> refs/heads/foo\n> refs/heads/foobar\n> They go well together and one is a prefix of the other.\n> What is crucial is the existence of a '/' just between the\n> prefix and the ending part, so\n> refs/heads/foo/bar doesn't fly here.\n> \n> The assumption may be the case if the prefix itself always\n> ends with a /, which is probably the case here?\n> I don't know if that is worth noting as well.\n\nYes, this is all rather subtle. Here we have found a reference whose\nname is *exactly* a proper prefix of refname; for example, refname is\n\"refs/foo/bar\" and we just found \"refs/foo\". But \"refs/foo\" is in skip,\nso it is not a conflict. Moreover, its existence means that there cannot\nbe any other references in the \"refs/foo/*\" namespace (e.g.,\n\"refs/foo/bar/banana\") because such a reference would have conflicted\nwith \"refs/foo\".\n\nIf it's any consolation, this logic will be simplified a bit later in\nthe patch series. Nevertheless, I will try to explain this better in v2.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"260600","messageId":"5548E154.7020506@alum.mit.edu","threadId":"39201","inReplyTo":"CAPig+cQabRiiMhNyYJM4_bZsQ4OTfrSbJ51XAido3V49baLhHA@mail.gmail.com","subject":"Re: [PATCH 01/18] t1404: new tests of D/F conflicts within ref transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-05T15:27:16Z","receivedAt":"2015-05-05T15:27:16Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/05/2015 07:12 AM, Eric Sunshine wrote:\n> On Fri, May 1, 2015 at 8:25 AM, Michael Haggerty wrote:\n>> Add some tests of D/F conflicts (as in, the type of directory vs. file\n>> conflict that exists between references \"refs/foo\" and \"refs/foo/bar\")\n>> in the context of reference transactions.\n>> [...]\n>> Signed-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>>\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..2fc2ac6\n>> --- /dev/null\n>> +++ b/t/t1404-update-ref-df-conflicts.sh\n>> @@ -0,0 +1,107 @@\n>> +#!/bin/sh\n>> +\n>> +test_description='Test git update-ref with D/F conflicts'\n>> +. ./test-lib.sh\n>> +\n>> +test_expect_success 'setup' '\n>> +\n>> + git commit --allow-empty -m Initial\n> \n> Broken &&-chain.\n\nThanks, Eric. I'll fix it in v2.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"260592","messageId":"5548EBF6.1030209@alum.mit.edu","threadId":"39201","inReplyTo":"xmqqy4l6wgwm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 00/18] Improve handling of D/F conflicts","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-05T16:12:38Z","receivedAt":"2015-05-05T16:12:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/03/2015 04:09 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> We currently don't handle D/F conflicts very well.\n>>\n>> A \"D/F conflict\" is what we call a conflict between references like\n>> \"refs/foo\" and \"refs/foo/bar\".\n> \n> Could you phrase that slightly differently, so that readers can tell\n> between the usual D/F conflict that is between directory and file in\n> the tracked contents (i.e. conflict between the branches being\n> merged, killing of a directory necessary when checking out a file,\n> etc.) and this thing? They are similar in nature, but \"D/F\n> conflict\" has been used to refer to the clashes that happen to the\n> user contents, not refnames. Starting a paragraph with \"... don't\n> handle D/F conflicts in refnames very well\" and then using \"D/F\n> conflict\" as a short-hand for \"D/F conflict in refnames\" throughout\n> the rest of the message is perfectly fine, as long as the message\n> never talks about the D/F conflict in the traditional sense.\n\nGood point. I will clarify this.\n\n>> * D/F conflicts between references being created in a single\n>> transaction used to be detected too late, possibly after part of the\n>> transaction had already been committed.\n>>\n>> * D/F errors against loose references were typically reported as\n> \n> Be consistent by s/errors/conflicts/ perhaps?\n\nYes, thanks.\n\n>> This patch series applies on top of\n>> mh/ref-lock-avoid-running-out-of-fds. I did it that way because I\n>> expected significant conflicts between the series, and the older\n>> series is simple/mature enough that I expect it to be merged early in\n>> the post-2.4 cycle. But in retrospect it turns out that there are only\n>> minor conflicts between the two series. So if you would like me to\n>> rebase this series to another starting point, please let me know.\n> \n> Thanks for being considerate about back-porting necessity.\n> \n> As I have written, I actually would prefer to see that the other\n> topic, ref-lock-avoid-running-out-of-fds, to be made applicable to\n> older maintenance tracks. So...\n\nOK, I will rebase this branch onto \"master\", and simultaneously check\nhow much work it would be to implement a version of\nref-lock-avoid-running-out-of-fds on top of 2.3 and/or 2.2.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"}]}