git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 15/24] branch -d: avoid repeated symref resolution

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Oct 2, 2014, 02:15 UTC
Message-ID
<20141002021510.GH1175@google.com>
In-Reply-To
<20141002014817.GS1175@google.com>
From: Jonathan Nieder <jrnieder@gmail.com>
Date: Wed, 10 Sep 2014 18:22:48 -0700

If a repository gets in a broken state with too much symref nesting, it cannot be repaired with "git branch -d":

 $ git symbolic-ref refs/heads/nonsense refs/heads/nonsense
 $ git branch -d nonsense
 error: branch 'nonsense' not found.

Worse, "git update-ref --no-deref -d" doesn't work for such repairs either:

 $ git update-ref -d refs/heads/nonsense
 error: unable to resolve reference refs/heads/nonsense: Too many levels of symbolic links

Fix both by teaching resolve_ref_unsafe a new RESOLVE_REF_NO_RECURSE flag and passing it when appropriate.

Callers can still read the value of a symref (for example to print a message about it) with that flag set --- resolve_ref_unsafe will resolve one level of symrefs and stop there.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Reviewed-by: Ronnie Sahlberg <sahlberg@google.com>
---
Since v21:
- renamed flag from RESOLVE_REF_NO_DEREF to _NO_RECURSE
- more detail in API docs
- only set NO_RECURSE when deleting refs.  Locking refs for non-deletion
  updates needs to recurse to get old_sha1 for the reflog.
- add more tests (for symrefs to refs with bad names, which should also
  be deletable now)
 builtin/branch.c      |  3 ++-
 cache.h               |  6 ++++++
 refs.c                | 17 +++++++++++++++--
 refs.h                |  2 ++
 t/t1400-update-ref.sh | 34 ++++++++++++++++++++++++++++++++++
 t/t3200-branch.sh     |  9 +++++++++
 6 files changed, 68 insertions(+), 3 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index e5d1377..a334380 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -234,7 +234,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		free(name);
 
 		name = mkpathdup(fmt, bname.buf);
-		target = resolve_ref_unsafe(name, 0, sha1, &flags);
+		target = resolve_ref_unsafe(name, RESOLVE_REF_NO_RECURSE,
+					    sha1, &flags);
 		if (!target ||
 		    (!(flags & REF_ISSYMREF) && is_null_sha1(sha1))) {
 			error(remote_branch
diff --git a/cache.h b/cache.h
index 5b54911..5ca7f2b 100644
--- a/cache.h
+++ b/cache.h
@@ -970,6 +970,11 @@ extern int read_ref(const char *refname, unsigned char *sha1);
  *   "writing" to the ref, the return value is the name of the ref
  *   that will actually be created or changed.
  *
+ * If the RESOLVE_REF_NO_RECURSE flag is passed, only resolves one
+ * level of symbolic reference.  The value stored in sha1 for a symbolic
+ * reference will always be null_sha1 in this case, and the return
+ * value is the reference that the symref refers to directly.
+ *
  * If flags is non-NULL, set the value that it points to the
  * combination of REF_ISPACKED (if the reference was found among the
  * packed references), REF_ISSYMREF (if the initial reference was a
@@ -982,6 +987,7 @@ extern int read_ref(const char *refname, unsigned char *sha1);
  * errno is set to something meaningful on error.
  */
 #define RESOLVE_REF_READING 0x01
+#define RESOLVE_REF_NO_RECURSE 0x02
 extern const char *resolve_ref_unsafe(const char *ref, int resolve_flags, unsigned char *sha1, int *flags);
 extern char *resolve_refdup(const char *ref, int resolve_flags, unsigned char *sha1, int *flags);
 
diff --git a/refs.c b/refs.c
index 4916d16..490e788 100644
--- a/refs.c
+++ b/refs.c
@@ -1407,6 +1407,10 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags, unsigned
 				refname = refname_buffer;
 				if (flags)
 					*flags |= REF_ISSYMREF;
+				if (resolve_flags & RESOLVE_REF_NO_RECURSE) {
+					hashclr(sha1);
+					return refname;
+				}
 				continue;
 			}
 		}
@@ -1463,13 +1467,17 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags, unsigned
 		buf = buffer + 4;
 		while (isspace(*buf))
 			buf++;
+		refname = strcpy(refname_buffer, buf);
+		if (resolve_flags & RESOLVE_REF_NO_RECURSE) {
+			hashclr(sha1);
+			return refname;
+		}
 		if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
 			if (flags)
 				*flags |= REF_ISBROKEN;
 			errno = EINVAL;
 			return NULL;
 		}
-		refname = strcpy(refname_buffer, buf);
 	}
 }
 
@@ -2111,6 +2119,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 
 	if (mustexist)
 		resolve_flags |= RESOLVE_REF_READING;
+	if (flags & REF_NODEREF && flags & REF_DELETING)
+		resolve_flags |= RESOLVE_REF_NO_RECURSE;
 
 	refname = resolve_ref_unsafe(refname, resolve_flags,
 				     lock->old_sha1, &type);
@@ -3610,13 +3620,16 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	/* Acquire all locks while verifying old values */
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
+		int flags = update->flags;
 
+		if (is_null_sha1(update->new_sha1))
+			flags |= REF_DELETING;
 		update->lock = lock_ref_sha1_basic(update->refname,
 						   (update->have_old ?
 						    update->old_sha1 :
 						    NULL),
 						   NULL,
-						   update->flags,
+						   flags,
 						   &update->type);
 		if (!update->lock) {
 			ret = (errno == ENOTDIR)
diff --git a/refs.h b/refs.h
index 3bb16db..b62c4c4 100644
--- a/refs.h
+++ b/refs.h
@@ -175,10 +175,12 @@ extern int peel_ref(const char *refname, unsigned char *sha1);
  * ref_transaction_create(), etc.
  * REF_NODEREF: act on the ref directly, instead of dereferencing
  *              symbolic references.
+ * REF_DELETING: tolerate broken refs
  *
  * Flags >= 0x100 are reserved for internal use.
  */
 #define REF_NODEREF	0x01
+#define REF_DELETING	0x02
 /*
  * This function sets errno to something meaningful on failure.
  */
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 0218e96..7c8c41a 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -110,6 +110,40 @@ test_expect_success "delete symref without dereference when the referred ref is
 cp -f .git/HEAD.orig .git/HEAD
 git update-ref -d $m
 
+test_expect_success 'update-ref -d is not confused by self-reference' '
+	git symbolic-ref refs/heads/self refs/heads/self &&
+	test_when_finished "rm -f .git/refs/heads/self" &&
+	test_path_is_file .git/refs/heads/self &&
+	test_must_fail git update-ref -d refs/heads/self &&
+	test_path_is_file .git/refs/heads/self
+'
+
+test_expect_success 'update-ref --no-deref -d can delete self-reference' '
+	git symbolic-ref refs/heads/self refs/heads/self &&
+	test_when_finished "rm -f .git/refs/heads/self" &&
+	test_path_is_file .git/refs/heads/self &&
+	git update-ref --no-deref -d refs/heads/self &&
+	test_path_is_missing .git/refs/heads/self
+'
+
+test_expect_success 'update-ref --no-deref -d can delete reference to bad ref' '
+	>.git/refs/heads/bad &&
+	test_when_finished "rm -f .git/refs/heads/bad" &&
+	git symbolic-ref refs/heads/ref-to-bad refs/heads/bad &&
+	test_when_finished "rm -f .git/refs/heads/ref-to-bad" &&
+	test_path_is_file .git/refs/heads/ref-to-bad &&
+	git update-ref --no-deref -d refs/heads/ref-to-bad &&
+	test_path_is_missing .git/refs/heads/ref-to-bad
+'
+
+test_expect_success 'update-ref --no-deref -d can delete reference to broken name' '
+	git symbolic-ref refs/heads/badname refs/heads/broken...ref &&
+	test_when_finished "rm -f .git/refs/heads/badname" &&
+	test_path_is_file .git/refs/heads/badname &&
+	git update-ref --no-deref -d refs/heads/badname &&
+	test_path_is_missing .git/refs/heads/badname
+'
+
 test_expect_success '(not) create HEAD with old sha1' "
 	test_must_fail git update-ref HEAD $A $B
 "
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index ac31b71..432921b 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -285,6 +285,15 @@ test_expect_success 'deleting a dangling symref' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'deleting a self-referential symref' '
+	git symbolic-ref refs/heads/self-reference refs/heads/self-reference &&
+	test_path_is_file .git/refs/heads/self-reference &&
+	echo "Deleted branch self-reference (was refs/heads/self-reference)." >expect &&
+	git branch -d self-reference >actual &&
+	test_path_is_missing .git/refs/heads/self-reference &&
+	test_i18ncmp expect actual
+'
+
 test_expect_success 'renaming a symref is not allowed' '
 	git symbolic-ref refs/heads/master2 refs/heads/master &&
 	test_must_fail git branch -m master2 master3 &&

-- 
2.1.0.rc2.206.gedb03e5
Previous: Jonathan NiederNext: Jonathan Nieder
Message 116 of 129 in “Transaction patch series overview”
  1. Ronnie SahlbergJul 30, 2014
  2. Ronnie SahlbergJul 31, 2014
  3. Ronnie SahlbergAug 8, 2014
  4. Ronnie SahlbergAug 19, 2014
  5. Junio C HamanoAug 19, 2014
  6. Jonathan NiederAug 20, 2014
  7. Jonathan NiederAug 26, 2014
  8. Junio C HamanoAug 26, 2014
  9. Jonathan NiederAug 26, 2014
  10. 0/20 rs/ref-transaction-1 (Re: Transaction patch series overview)Jonathan Nieder, Aug 27, 2014
  11. 01/20 refs.c: change ref_transaction_create to do error checking and return statusJonathan Nieder, Aug 27, 2014
  12. 02/20 refs.c: update ref_transaction_delete to check for error and return statusJonathan Nieder, Aug 27, 2014
  13. 03/20 refs.c: make ref_transaction_begin take an err argumentJonathan Nieder, Aug 27, 2014
  14. 04/20 refs.c: add transaction.status and track OPEN/CLOSEDJonathan Nieder, Aug 27, 2014
  15. 05/20 tag.c: use ref transactions when doing updatesJonathan Nieder, Aug 27, 2014
  16. 06/20 replace.c: use the ref transaction functions for updatesJonathan Nieder, Aug 27, 2014
  17. 07/20 commit.c: use ref transactions for updatesJonathan Nieder, Aug 27, 2014
  18. 08/20 sequencer.c: use ref transactions for all ref updatesJonathan Nieder, Aug 27, 2014
  19. 09/20 fast-import.c: change update_branch to use ref transactionsJonathan Nieder, Aug 27, 2014
  20. 10/20 branch.c: use ref transaction for all ref updatesJonathan Nieder, Aug 27, 2014
  21. 11/20 refs.c: change update_ref to use a transactionJonathan Nieder, Aug 27, 2014
  22. 12/20 receive-pack.c: use a reference transaction for updating the refsJonathan Nieder, Aug 27, 2014
  23. 13/20 fast-import.c: use a ref transaction when dumping tagsJonathan Nieder, Aug 27, 2014
  24. 14/20 walker.c: use ref transaction for ref updatesJonathan Nieder, Aug 27, 2014
  25. 15/20 refs.c: make lock_ref_sha1 staticJonathan Nieder, Aug 27, 2014
  26. 16/20 refs.c: remove the update_ref_lock functionJonathan Nieder, Aug 27, 2014
  27. 17/20 refs.c: remove the update_ref_write functionJonathan Nieder, Aug 27, 2014
  28. 18/20 refs.c: remove lock_ref_sha1Jonathan Nieder, Aug 27, 2014
  29. 19/20 refs.c: make prune_ref use a transaction to delete the refJonathan Nieder, Aug 27, 2014
  30. 20/20 refs.c: make delete_ref use a transactionJonathan Nieder, Aug 27, 2014
  31. Junio C HamanoAug 27, 2014
  32. Junio C HamanoAug 27, 2014
  33. 0/22 rs/ref-transaction-1 (Re: Transaction patch series overview)Jonathan Nieder, Sep 2, 2014
  34. 01/22 refs.c: change ref_transaction_create to do error checking and return statusJonathan Nieder, Sep 2, 2014
  35. 02/22 refs.c: update ref_transaction_delete to check for error and return statusJonathan Nieder, Sep 2, 2014
  36. 03/22 refs.c: make ref_transaction_begin take an err argumentJonathan Nieder, Sep 2, 2014
  37. 04/22 refs.c: add transaction.status and track OPEN/CLOSEDJonathan Nieder, Sep 2, 2014
  38. 05/22 tag.c: use ref transactions when doing updatesJonathan Nieder, Sep 2, 2014
  39. 06/22 replace.c: use the ref transaction functions for updatesJonathan Nieder, Sep 2, 2014
  40. 07/22 commit.c: use ref transactions for updatesJonathan Nieder, Sep 2, 2014
  41. 08/22 sequencer.c: use ref transactions for all ref updatesJonathan Nieder, Sep 2, 2014
  42. 09/22 fast-import.c: change update_branch to use ref transactionsJonathan Nieder, Sep 2, 2014
  43. 10/22 branch.c: use ref transaction for all ref updatesJonathan Nieder, Sep 2, 2014
  44. 11/22 refs.c: change update_ref to use a transactionJonathan Nieder, Sep 2, 2014
  45. 12/22 receive-pack.c: use a reference transaction for updating the refsJonathan Nieder, Sep 2, 2014
  46. 13/22 fast-import.c: use a ref transaction when dumping tagsJonathan Nieder, Sep 2, 2014
  47. 14/22 walker.c: use ref transaction for ref updatesJonathan Nieder, Sep 2, 2014
  48. 15/22 refs.c: make lock_ref_sha1 staticJonathan Nieder, Sep 2, 2014
  49. 16/22 refs.c: remove the update_ref_lock functionJonathan Nieder, Sep 2, 2014
  50. 17/22 refs.c: remove the update_ref_write functionJonathan Nieder, Sep 2, 2014
  51. 18/22 refs.c: remove lock_ref_sha1Jonathan Nieder, Sep 2, 2014
  52. 19/22 refs.c: make prune_ref use a transaction to delete the refJonathan Nieder, Sep 2, 2014
  53. 20/22 refs.c: make delete_ref use a transactionJonathan Nieder, Sep 2, 2014
  54. 21/22 update-ref --stdin: narrow scope of err strbufJonathan Nieder, Sep 2, 2014
  55. 22/22 update-ref --stdin: pass transaction around explicitlyJonathan Nieder, Sep 2, 2014
  56. Using Gerrit to review Git patches (was: Re: Transaction patch series overview)Michael Haggerty, Aug 27, 2014
  57. Shawn PearceAug 28, 2014
  58. 0/19 rs/ref-transaction (Re: Transaction patch series overview)Jonathan Nieder, Sep 11, 2014
  59. 01/19 mv test: recreate mod/ directory instead of relying on stale copyJonathan Nieder, Sep 11, 2014
  60. 02/19 wrapper.c: remove/unlink_or_warn: simplify, treat ENOENT as successJonathan Nieder, Sep 11, 2014
  61. 03/19 wrapper.c: add a new function unlink_or_msgJonathan Nieder, Sep 11, 2014
  62. 04/19 refs.c: add an err argument to delete_ref_looseJonathan Nieder, Sep 11, 2014
  63. 05/19 refs.c: pass the ref log message to _create/delete/update instead of _commitJonathan Nieder, Sep 11, 2014
  64. 06/19 rename_ref: don't ask read_ref_full where the ref came fromJonathan Nieder, Sep 11, 2014
  65. 07/19 refs.c: move the check for valid refname to lock_ref_sha1_basicJonathan Nieder, Sep 11, 2014
  66. 08/19 refs.c: call lock_ref_sha1_basic directly from commitJonathan Nieder, Sep 11, 2014
  67. 09/19 refs.c: pass a skip list to name_conflict_fnJonathan Nieder, Sep 11, 2014
  68. 10/19 refs.c: ref_transaction_commit: distinguish name conflicts from other errorsJonathan Nieder, Sep 11, 2014
  69. 11/19 fetch.c: change s_update_ref to use a ref transactionJonathan Nieder, Sep 11, 2014
  70. 12/19 refs.c: make write_ref_sha1 staticJonathan Nieder, Sep 11, 2014
  71. 13/19 refs.c: change resolve_ref_unsafe reading argument to be a flags fieldJonathan Nieder, Sep 11, 2014
  72. 14/19 branch -d: avoid repeated symref resolutionJonathan Nieder, Sep 11, 2014
  73. 15/19 refs.c: fix handling of badly named refsJonathan Nieder, Sep 11, 2014
  74. 16/19 for-each-ref.c: improve message before aborting on broken refJonathan Nieder, Sep 11, 2014
  75. 17/19 refs.c: do not permit err == NULLJonathan Nieder, Sep 11, 2014
  76. 18/19 lockfile: remove unable_to_lock_errorJonathan Nieder, Sep 11, 2014
  77. 19/19 ref_transaction_commit: bail out on failure to remove a refJonathan Nieder, Sep 11, 2014
  78. Junio C HamanoSep 11, 2014
  79. Junio C HamanoSep 11, 2014
  80. Jonathan NiederSep 12, 2014
  81. Junio C HamanoSep 12, 2014
  82. Jonathan NiederSep 12, 2014
  83. Junio C HamanoSep 12, 2014
  84. Junio C HamanoSep 12, 2014
  85. Junio C HamanoSep 13, 2014
  86. Michael HaggertySep 12, 2014
  87. Jonathan NiederSep 12, 2014
  88. Michael HaggertySep 17, 2014
  89. Junio C HamanoSep 18, 2014
  90. Jonathan NiederSep 18, 2014
  91. Junio C HamanoSep 18, 2014
  92. Jonathan NiederSep 18, 2014
  93. Junio C HamanoSep 25, 2014
  94. Jonathan NiederSep 25, 2014
  95. Junio C HamanoSep 25, 2014
  96. 0/24 rs/ref-transactionJonathan Nieder, Oct 2, 2014
  97. 01/24 mv test: recreate mod/ directory instead of relying on stale copyJonathan Nieder, Oct 2, 2014
  98. 02/24 wrapper.c: remove/unlink_or_warn: simplify, treat ENOENT as successJonathan Nieder, Oct 2, 2014
  99. 03/24 wrapper.c: add a new function unlink_or_msgJonathan Nieder, Oct 2, 2014
  100. 04/24 refs.c: add an err argument to delete_ref_looseJonathan Nieder, Oct 2, 2014
  101. 05/24 refs.c: pass the ref log message to _create/delete/update instead of _commitJonathan Nieder, Oct 2, 2014
  102. 06/24 rename_ref: don't ask read_ref_full where the ref came fromJonathan Nieder, Oct 2, 2014
  103. 07/24 refs.c: refuse to lock badly named refs in lock_ref_sha1_basicJonathan Nieder, Oct 2, 2014
  104. 08/24 refs.c: call lock_ref_sha1_basic directly from commitJonathan Nieder, Oct 2, 2014
  105. 09/24 refs.c: pass a list of names to skip to is_refname_availableJonathan Nieder, Oct 2, 2014
  106. Junio C HamanoOct 2, 2014
  107. Jonathan NiederOct 3, 2014
  108. Junio C HamanoOct 3, 2014
  109. 09/24 refs.c: pass a list of names to skip to is_refname_availableJonathan Nieder, Oct 3, 2014
  110. Junio C HamanoOct 7, 2014
  111. 10/24 refs.c: ref_transaction_commit: distinguish name conflicts from other errorsJonathan Nieder, Oct 2, 2014
  112. 11/24 fetch.c: change s_update_ref to use a ref transactionJonathan Nieder, Oct 2, 2014
  113. 12/24 refs.c: make write_ref_sha1 staticJonathan Nieder, Oct 2, 2014
  114. 13/24 refs.c: change resolve_ref_unsafe reading argument to be a flags fieldJonathan Nieder, Oct 2, 2014
  115. 14/24 reflog test: test interaction with detached HEADJonathan Nieder, Oct 2, 2014
  116. 15/24 branch -d: avoid repeated symref resolutionJonathan Nieder, Oct 2, 2014
  117. 16/24 branch -d: simplify by using RESOLVE_REF_READING flagJonathan Nieder, Oct 2, 2014
  118. 17/24 packed-ref cache: forbid dot-components in refnamesJonathan Nieder, Oct 2, 2014
  119. 18/24 test: put tests for handling of bad ref names in one placeJonathan Nieder, Oct 2, 2014
  120. 19/24 refs.c: allow listing and deleting badly named refsJonathan Nieder, Oct 2, 2014
  121. Junio C HamanoOct 2, 2014
  122. Ronnie SahlbergOct 3, 2014
  123. Ronnie SahlbergOct 3, 2014
  124. Junio C HamanoOct 3, 2014
  125. 20/24 for-each-ref.c: improve message before aborting on broken refJonathan Nieder, Oct 2, 2014
  126. 21/24 remote rm/prune: print a message when writing packed-refs failsJonathan Nieder, Oct 2, 2014
  127. 22/24 refs.c: do not permit err == NULLJonathan Nieder, Oct 2, 2014
  128. 23/24 lockfile: remove unable_to_lock_errorJonathan Nieder, Oct 2, 2014
  129. 24/24 ref_transaction_commit: bail out on failure to remove a refJonathan Nieder, Oct 2, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.