Skip to content

Commit

Permalink
branch -d: avoid repeated symref resolution
Browse files Browse the repository at this point in the history 
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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
Jonathan Nieder authored and Junio C Hamano committed Oct 15, 2014
1 parent 014e7db commit 62a2d52
Show file tree
Hide file tree
Showing 6 changed files with 68 additions and 3 deletions.
3 changes: 2 additions & 1 deletion builtin/branch.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,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
Expand Down
6 changes: 6 additions & 0 deletions cache.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -973,6 +973,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
Expand All @@ -985,6 +990,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);

Expand Down
17 changes: 15 additions & 2 deletions refs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1467,6 +1467,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;
}
}
Expand Down Expand Up @@ -1523,13 +1527,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);
}
}

Expand Down Expand Up @@ -2170,6 +2178,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);
Expand Down Expand Up @@ -3664,13 +3674,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)
Expand Down
2 changes: 2 additions & 0 deletions refs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,10 +177,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.
*/
Expand Down
34 changes: 34 additions & 0 deletions t/t1400-update-ref.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
"
Expand Down
9 changes: 9 additions & 0 deletions t/t3200-branch.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand Down

0 comments on commit 62a2d52

Please sign in to comment.