Re: [PATCH 1/2] Fix git branch -m for symrefs.

13 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH 1/2] Fix git branch -m for symrefs.

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:45:31

Miklos Vajna [off-list ref] writes:
-int delete_ref(const char *refname, const unsigned char *sha1)
+int delete_ref(const char *refname, const unsigned char *sha1, int flags)
 {
 	struct ref_lock *lock;
-	int err, i, ret = 0, flag = 0;
+	int err, i = 0, ret = 0, flag = 0;
+	char *path;
 
 	lock = lock_ref_sha1_basic(refname, sha1, 0, &flag);
 	if (!lock)
 		return 1;
 	if (!(flag & REF_ISPACKED)) {
Two variables flag vs flags is a bit confusing, isn't it?  How about
naming the new one "delopt" or something?

The new variable "char *path" at the toplevel can be confined in the scope
of this if () {} block and probably can become "const char *", right?
quoted hunk
@@ -964,10 +971,15 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	struct ref_lock *lock;
 	struct stat loginfo;
 	int log = !lstat(git_path("logs/%s", oldref), &loginfo);
+	const char *symref = NULL;
+	int is_symref = 0;
 
 	if (S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldref);
Possible bug in the context.  When there is no reflog for the ref being
renamed, lstat would fail; it doesn't feel right to have this S_ISLNK()
before checking the result of the lstat which is in "log".
+	symref = resolve_ref(oldref, orig_sha1, 0, &flag);
+	if (flag & REF_ISSYMREF)
+		is_symref = 1;
 	if (!resolve_ref(oldref, orig_sha1, 1, &flag))
 		return error("refname %s not found", oldref);
Do we really need two calls to resolve_ref()?  Your new call calls it
without must-exist bit --- why?  Immediately after that, the existing call
will barf if it does not exist anyway.

I agree it is good to have symref aware delete_ref(), but I am not sure
supporting symref in rename_ref() is either needed or necessarily a good
idea.  You also need to worry about a symref pointing at a branch yet to
be born.

In the meantime, I think we should just check (flag & REF_ISSYMREF) after
the existing resolve_ref() we can see in the context above, and error out
saying you cannot rename a symref, and do nothing else.

[PATCH 3/3] Fix git update-ref --no-deref -d.

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:31

Till now --no-deref was just ignored when deleting refs, fix this.

Signed-off-by: Miklos Vajna <redacted>
---
 builtin-update-ref.c  |    8 +++++---
 t/t1400-update-ref.sh |    7 +++++++
 2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index d8f3142..378dc1b 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -13,7 +13,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 {
 	const char *refname, *oldval, *msg=NULL;
 	unsigned char sha1[20], oldsha1[20];
-	int delete = 0, no_deref = 0;
+	int delete = 0, no_deref = 0, flags = 0;
 	struct option options[] = {
 		OPT_STRING( 'm', NULL, &msg, "reason", "reason of the update"),
 		OPT_BOOLEAN('d', NULL, &delete, "deletes the reference"),
@@ -47,9 +47,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	if (oldval && *oldval && get_sha1(oldval, oldsha1))
 		die("%s: not a valid old SHA1", oldval);
 
+	if (no_deref)
+		flags = REF_NODEREF;
 	if (delete)
-		return delete_ref(refname, oldval ? oldsha1 : NULL, 0);
+		return delete_ref(refname, oldval ? oldsha1 : NULL, flags);
 	else
 		return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
-				  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);
+				  flags, DIE_ON_ERR);
 }
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 04c2b16..8139cd6 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -75,6 +75,13 @@ test_expect_success "delete $m (by HEAD)" '
 '
 rm -f .git/$m
 
+cp -f .git/HEAD .git/HEAD.orig
+test_expect_success "delete symref without dereference" '
+	git update-ref --no-deref -d HEAD &&
+	! test -f .git/HEAD
+'
+cp -f .git/HEAD.orig .git/HEAD
+
 test_expect_success '(not) create HEAD with old sha1' "
 	test_must_fail git update-ref HEAD $A $B
 "
-- 
1.6.0.2

[PATCH 2/3] rename_ref(): handle the case when the reflog of a ref does not exist

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:31

We tried to check if a reflog of a ref is a symlink without first
checking if it exists, which is a bug.

Signed-off-by: Miklos Vajna <redacted>
---
 refs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/refs.c b/refs.c
index 70c0967..3c39a31 100644
--- a/refs.c
+++ b/refs.c
@@ -975,7 +975,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	const char *symref = NULL;
 	int is_symref = 0;
 
-	if (S_ISLNK(loginfo.st_mode))
+	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldref);
 
 	symref = resolve_ref(oldref, orig_sha1, 1, &flag);
-- 
1.6.0.2

[PATCH 0/3] symref rename/delete fixes

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:31

On Sat, Oct 25, 2008 at 11:31:01AM -0700, Junio C Hamano [off-list ref] wrote:
quoted
+int delete_ref(const char *refname, const unsigned char *sha1, int
flags)
 {
    struct ref_lock *lock;
-   int err, i, ret = 0, flag = 0;
+   int err, i = 0, ret = 0, flag = 0;
+   char *path;
Two variables flag vs flags is a bit confusing, isn't it?  How about
naming the new one "delopt" or something?
Renamed.
The new variable "char *path" at the toplevel can be confined in the
scope
of this if () {} block and probably can become "const char *", right?
Yes - moved.
quoted
    int log = !lstat(git_path("logs/%s", oldref), &loginfo);
+   const char *symref = NULL;
+   int is_symref = 0;

    if (S_ISLNK(loginfo.st_mode))
            return error("reflog for %s is a symlink", oldref);
Possible bug in the context.  When there is no reflog for the ref
being
renamed, lstat would fail; it doesn't feel right to have this
S_ISLNK()
before checking the result of the lstat which is in "log".
OK, this went to a separate patch.
quoted
+   symref = resolve_ref(oldref, orig_sha1, 0, &flag);
+   if (flag & REF_ISSYMREF)
+           is_symref = 1;
    if (!resolve_ref(oldref, orig_sha1, 1, &flag))
            return error("refname %s not found", oldref);
Do we really need two calls to resolve_ref()?  Your new call calls it
without must-exist bit --- why?  Immediately after that, the existing
call
will barf if it does not exist anyway.
Just having

        if (!symref)
                return error("refname %s not found", oldref);

first looks weird, given that the error message is not "refname %s is
not a symref", but you are right, changed.
I agree it is good to have symref aware delete_ref(), but I am not
sure
supporting symref in rename_ref() is either needed or necessarily a
good
idea.  You also need to worry about a symref pointing at a branch yet
to
be born.
That is currently not supported and the error message of 'git branch -m'
is (in case foo points to refs/heads/bar and bar is not yet born):

error: refname refs/heads/foo not found
fatal: Branch rename failed

which is quite acceptable, IMHO[1].
In the meantime, I think we should just check (flag & REF_ISSYMREF)
after
the existing resolve_ref() we can see in the context above, and error
out
saying you cannot rename a symref, and do nothing else.
A symref-aware rename_ref() is needed by git remove rename, since it
typically does origin/HEAD -> upstream/HEAD symref renames there.

Of course you can say that this should be handled by git-remote itself,
without using rename_ref() but that not seem to be a good solution to
me. (Workaround in the wrong layer, instead of a solution in a good
one.)

[1] I mean, I have a real-world scenario (git remove rename) for "why
renaming a symref is a good idea", but I don't think renaming a ref
pointing to a yet-to-be-born ref has any real world users.

Miklos Vajna (3):
  Fix git branch -m for symrefs.
  rename_ref(): handle the case when the reflog of a ref does not exist
  Fix git update-ref --no-deref -d.

 builtin-branch.c       |    2 +-
 builtin-receive-pack.c |    2 +-
 builtin-remote.c       |    4 +-
 builtin-reset.c        |    2 +-
 builtin-send-pack.c    |    2 +-
 builtin-tag.c          |    2 +-
 builtin-update-ref.c   |    8 ++++--
 cache.h                |    2 +-
 refs.c                 |   61 +++++++++++++++++++++++++++++------------------
 t/t1400-update-ref.sh  |    7 +++++
 t/t3200-branch.sh      |    9 +++++++
 11 files changed, 67 insertions(+), 34 deletions(-)

Also available in the 'symref-mv' branch of 'git://repo.or.cz/git/vmiklos.git'.

[PATCH 1/3] Fix git branch -m for symrefs.

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:31

This had two problems with symrefs. First, it copied the actual sha1
instead of the "pointer", second it failed to remove the old ref after a
successful rename.

Given that till now delete_ref() always dereferenced symrefs, a new
parameters has been introduced to delete_ref() to allow deleting refs
without a dereference.

Signed-off-by: Miklos Vajna <redacted>
---
 builtin-branch.c       |    2 +-
 builtin-receive-pack.c |    2 +-
 builtin-remote.c       |    4 +-
 builtin-reset.c        |    2 +-
 builtin-send-pack.c    |    2 +-
 builtin-tag.c          |    2 +-
 builtin-update-ref.c   |    2 +-
 cache.h                |    2 +-
 refs.c                 |   59 ++++++++++++++++++++++++++++++------------------
 t/t3200-branch.sh      |    9 +++++++
 10 files changed, 55 insertions(+), 31 deletions(-)
diff --git a/builtin-branch.c b/builtin-branch.c
index 8d634ff..2b3613f 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -160,7 +160,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
 			continue;
 		}
 
-		if (delete_ref(name, sha1)) {
+		if (delete_ref(name, sha1, 0)) {
 			error("Error deleting %sbranch '%s'", remote,
 			       argv[i]);
 			ret = 1;
diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 45e3cd9..ab5fa1c 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -224,7 +224,7 @@ static const char *update(struct command *cmd)
 			warning ("Allowing deletion of corrupt ref.");
 			old_sha1 = NULL;
 		}
-		if (delete_ref(name, old_sha1)) {
+		if (delete_ref(name, old_sha1, 0)) {
 			error("failed to delete %s", name);
 			return "failed to delete";
 		}
diff --git a/builtin-remote.c b/builtin-remote.c
index df2be06..e396a3a 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -337,7 +337,7 @@ static int remove_branches(struct string_list *branches)
 		const char *refname = item->string;
 		unsigned char *sha1 = item->util;
 
-		if (delete_ref(refname, sha1))
+		if (delete_ref(refname, sha1, 0))
 			result |= error("Could not remove branch %s", refname);
 	}
 	return result;
@@ -565,7 +565,7 @@ static int prune(int argc, const char **argv)
 			const char *refname = states.stale.items[i].util;
 
 			if (!dry_run)
-				result |= delete_ref(refname, NULL);
+				result |= delete_ref(refname, NULL, 0);
 
 			printf(" * [%s] %s\n", dry_run ? "would prune" : "pruned",
 			       abbrev_ref(refname, "refs/remotes/"));
diff --git a/builtin-reset.c b/builtin-reset.c
index 16e6bb2..9514b77 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -279,7 +279,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		update_ref(msg, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
 	}
 	else if (old_orig)
-		delete_ref("ORIG_HEAD", old_orig);
+		delete_ref("ORIG_HEAD", old_orig, 0);
 	prepend_reflog_action("updating HEAD", msg, sizeof(msg));
 	update_ref_status = update_ref(msg, "HEAD", sha1, orig, 0, MSG_ON_ERR);
 
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 910db92..bbf6e0a 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -234,7 +234,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 		if (args.verbose)
 			fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
 		if (ref->deletion) {
-			delete_ref(rs.dst, NULL);
+			delete_ref(rs.dst, NULL, 0);
 		} else
 			update_ref("update by push", rs.dst,
 					ref->new_sha1, NULL, 0, 0);
diff --git a/builtin-tag.c b/builtin-tag.c
index b13fa34..1ff7b37 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -125,7 +125,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
 static int delete_tag(const char *name, const char *ref,
 				const unsigned char *sha1)
 {
-	if (delete_ref(ref, sha1))
+	if (delete_ref(ref, sha1, 0))
 		return 1;
 	printf("Deleted tag '%s'\n", name);
 	return 0;
diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index 56a0b1b..d8f3142 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -48,7 +48,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		die("%s: not a valid old SHA1", oldval);
 
 	if (delete)
-		return delete_ref(refname, oldval ? oldsha1 : NULL);
+		return delete_ref(refname, oldval ? oldsha1 : NULL, 0);
 	else
 		return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
 				  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);
diff --git a/cache.h b/cache.h
index b0edbf9..a3c77f0 100644
--- a/cache.h
+++ b/cache.h
@@ -434,7 +434,7 @@ extern int commit_locked_index(struct lock_file *);
 extern void set_alternate_index_output(const char *);
 extern int close_lock_file(struct lock_file *);
 extern void rollback_lock_file(struct lock_file *);
-extern int delete_ref(const char *, const unsigned char *sha1);
+extern int delete_ref(const char *, const unsigned char *sha1, int delopt);
 
 /* Environment bits from configuration mechanism */
 extern int trust_executable_bit;
diff --git a/refs.c b/refs.c
index 0a126fa..70c0967 100644
--- a/refs.c
+++ b/refs.c
@@ -921,25 +921,33 @@ static int repack_without_ref(const char *refname)
 	return commit_lock_file(&packlock);
 }
 
-int delete_ref(const char *refname, const unsigned char *sha1)
+int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 {
 	struct ref_lock *lock;
-	int err, i, ret = 0, flag = 0;
+	int err, i = 0, ret = 0, flag = 0;
 
 	lock = lock_ref_sha1_basic(refname, sha1, 0, &flag);
 	if (!lock)
 		return 1;
 	if (!(flag & REF_ISPACKED)) {
 		/* loose */
-		i = strlen(lock->lk->filename) - 5; /* .lock */
-		lock->lk->filename[i] = 0;
-		err = unlink(lock->lk->filename);
+		const char *path;
+
+		if (!(delopt & REF_NODEREF)) {
+			i = strlen(lock->lk->filename) - 5; /* .lock */
+			lock->lk->filename[i] = 0;
+			path = lock->lk->filename;
+		} else {
+			path = git_path(refname);
+		}
+		err = unlink(path);
 		if (err && errno != ENOENT) {
 			ret = 1;
 			error("unlink(%s) failed: %s",
-			      lock->lk->filename, strerror(errno));
+			      path, strerror(errno));
 		}
-		lock->lk->filename[i] = '.';
+		if (!(delopt & REF_NODEREF))
+			lock->lk->filename[i] = '.';
 	}
 	/* removing the loose one could have resurrected an earlier
 	 * packed one.  Also, if it was not loose we need to repack
@@ -964,11 +972,16 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	struct ref_lock *lock;
 	struct stat loginfo;
 	int log = !lstat(git_path("logs/%s", oldref), &loginfo);
+	const char *symref = NULL;
+	int is_symref = 0;
 
 	if (S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldref);
 
-	if (!resolve_ref(oldref, orig_sha1, 1, &flag))
+	symref = resolve_ref(oldref, orig_sha1, 1, &flag);
+	if (flag & REF_ISSYMREF)
+		is_symref = 1;
+	if (!symref)
 		return error("refname %s not found", oldref);
 
 	if (!is_refname_available(newref, oldref, get_packed_refs(), 0))
@@ -988,12 +1001,12 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 		return error("unable to move logfile logs/%s to tmp-renamed-log: %s",
 			oldref, strerror(errno));
 
-	if (delete_ref(oldref, orig_sha1)) {
+	if (delete_ref(oldref, orig_sha1, REF_NODEREF)) {
 		error("unable to delete old %s", oldref);
 		goto rollback;
 	}
 
-	if (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1)) {
+	if (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1, REF_NODEREF)) {
 		if (errno==EISDIR) {
 			if (remove_empty_directories(git_path("%s", newref))) {
 				error("Directory not empty: %s", newref);
@@ -1031,18 +1044,20 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	}
 	logmoved = log;
 
-	lock = lock_ref_sha1_basic(newref, NULL, 0, NULL);
-	if (!lock) {
-		error("unable to lock %s for update", newref);
-		goto rollback;
-	}
-
-	lock->force_write = 1;
-	hashcpy(lock->old_sha1, orig_sha1);
-	if (write_ref_sha1(lock, orig_sha1, logmsg)) {
-		error("unable to write current sha1 into %s", newref);
-		goto rollback;
-	}
+	if (!is_symref) {
+		lock = lock_ref_sha1_basic(newref, NULL, 0, NULL);
+		if (!lock) {
+			error("unable to lock %s for update", newref);
+			goto rollback;
+		}
+		lock->force_write = 1;
+		hashcpy(lock->old_sha1, orig_sha1);
+		if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+			error("unable to write current sha1 into %s", newref);
+			goto rollback;
+		}
+	} else
+		create_symref(newref, symref, logmsg);
 
 	return 0;
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 2147eac..fdeb1f5 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -112,6 +112,15 @@ test_expect_success 'config information was renamed, too' \
 	"test $(git config branch.s.dummy) = Hello &&
 	 test_must_fail git config branch.s/s/dummy"
 
+test_expect_success 'renaming a symref' \
+'
+	git symbolic-ref refs/heads/master2 refs/heads/master &&
+	git branch -m master2 master3 &&
+	git symbolic-ref refs/heads/master3 &&
+	test -f .git/refs/heads/master &&
+	! test -f .git/refs/heads/master2
+'
+
 test_expect_success \
     'git branch -m u v should fail when the reflog for u is a symlink' '
      git branch -l u &&
-- 
1.6.0.2

Re: [PATCH 0/3] symref rename/delete fixes

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:45:32

Miklos Vajna [off-list ref] writes:
A symref-aware rename_ref() is needed by git remove rename, since it
typically does origin/HEAD -> upstream/HEAD symref renames there.

Of course you can say that this should be handled by git-remote itself,
without using rename_ref() but that not seem to be a good solution to
me. (Workaround in the wrong layer, instead of a solution in a good
one.)
I do not think it is a workaround at all.

I would even say that the renaming of symref that "git remote rename"
needs to do is fundamentally different from what rename_ref() is about,
and trying to cram it into rename_ref() is a grave mistake.

If you "git remote rename origin upstream" when origin/HEAD points at
refs/remotes/origin/master, you need to make the renamed one point at
refs/remotes/upstream/master, as you will be renaming origin/master to
upstream/master.

Normal "rename_ref()" would just rename the ref without touching its
contents, and if you used it to implement "git remote rename", your
upstream/HEAD would point at the old name "origin/master" that will
disappear when rename is finished, wouldn't it?  I do not think it is
useful.

There may be cases where you would really want to rename the symbolic ref
without changing its value (e.g. which other ref it points at), but as you
mentioned, even "git branch -m" is not such a usecase.

I think it is better to simply forbid renaming of a symref _until_ we know
what we want it to mean.  It is a lot easier to start strict and then add
features, than start loosely and implement an unclean semantics, and then
having to fix that semantics after people start to rely on the initial
(potentially crazy) semantics.

Re: [PATCH 0/3] symref rename/delete fixes

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:32

On Sun, Oct 26, 2008 at 10:31:09PM -0700, Junio C Hamano [off-list ref] wrote:
quoted
Of course you can say that this should be handled by git-remote itself,
without using rename_ref() but that not seem to be a good solution to
me. (Workaround in the wrong layer, instead of a solution in a good
one.)
I do not think it is a workaround at all.

I would even say that the renaming of symref that "git remote rename"
needs to do is fundamentally different from what rename_ref() is about,
and trying to cram it into rename_ref() is a grave mistake.

If you "git remote rename origin upstream" when origin/HEAD points at
refs/remotes/origin/master, you need to make the renamed one point at
refs/remotes/upstream/master, as you will be renaming origin/master to
upstream/master.

Normal "rename_ref()" would just rename the ref without touching its
contents, and if you used it to implement "git remote rename", your
upstream/HEAD would point at the old name "origin/master" that will
disappear when rename is finished, wouldn't it?  I do not think it is
useful.
Ah, I missed that. You convienced me, I'll post updated patches later
today. :)

[PATCH 3/3] Fix git update-ref --no-deref -d.

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:32

Till now --no-deref was just ignored when deleting refs, fix this.

Signed-off-by: Miklos Vajna <redacted>
---
 builtin-update-ref.c  |    8 +++++---
 t/t1400-update-ref.sh |    7 +++++++
 2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index d8f3142..378dc1b 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -13,7 +13,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 {
 	const char *refname, *oldval, *msg=NULL;
 	unsigned char sha1[20], oldsha1[20];
-	int delete = 0, no_deref = 0;
+	int delete = 0, no_deref = 0, flags = 0;
 	struct option options[] = {
 		OPT_STRING( 'm', NULL, &msg, "reason", "reason of the update"),
 		OPT_BOOLEAN('d', NULL, &delete, "deletes the reference"),
@@ -47,9 +47,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	if (oldval && *oldval && get_sha1(oldval, oldsha1))
 		die("%s: not a valid old SHA1", oldval);
 
+	if (no_deref)
+		flags = REF_NODEREF;
 	if (delete)
-		return delete_ref(refname, oldval ? oldsha1 : NULL, 0);
+		return delete_ref(refname, oldval ? oldsha1 : NULL, flags);
 	else
 		return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
-				  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);
+				  flags, DIE_ON_ERR);
 }
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 04c2b16..8139cd6 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -75,6 +75,13 @@ test_expect_success "delete $m (by HEAD)" '
 '
 rm -f .git/$m
 
+cp -f .git/HEAD .git/HEAD.orig
+test_expect_success "delete symref without dereference" '
+	git update-ref --no-deref -d HEAD &&
+	! test -f .git/HEAD
+'
+cp -f .git/HEAD.orig .git/HEAD
+
 test_expect_success '(not) create HEAD with old sha1' "
 	test_must_fail git update-ref HEAD $A $B
 "
-- 
1.6.0.2

[PATCH 2/3] rename_ref(): handle the case when the reflog of a ref does not exist

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:32

We tried to check if a reflog of a ref is a symlink without first
checking if it exists, which is a bug.

Signed-off-by: Miklos Vajna <redacted>
---
 refs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/refs.c b/refs.c
index 31a2057..116eb59 100644
--- a/refs.c
+++ b/refs.c
@@ -974,7 +974,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	int log = !lstat(git_path("logs/%s", oldref), &loginfo);
 	const char *symref = NULL;
 
-	if (S_ISLNK(loginfo.st_mode))
+	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldref);
 
 	symref = resolve_ref(oldref, orig_sha1, 1, &flag);
-- 
1.6.0.2

[PATCH 0/3] symref rename/delete fixes

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:32

On Mon, Oct 27, 2008 at 09:50:55AM +0100, Miklos Vajna [off-list ref] wrote:
Ah, I missed that. You convienced me, I'll post updated patches later
today. :)
Here they are. The first is updated: the delete_ref() part is unchanged,
but the rename_ref() part is dropped and it just bails out with an error
if it detects a symref. The testcase is updated according to this as
well.

The two others are just rebased on top of the new first one, no
functional changes.

Miklos Vajna (3):
  Disallow git branch -m for symrefs.
  rename_ref(): handle the case when the reflog of a ref does not exist
  Fix git update-ref --no-deref -d.

 builtin-branch.c       |    2 +-
 builtin-receive-pack.c |    2 +-
 builtin-remote.c       |    4 ++--
 builtin-reset.c        |    2 +-
 builtin-send-pack.c    |    2 +-
 builtin-tag.c          |    2 +-
 builtin-update-ref.c   |    8 +++++---
 cache.h                |    2 +-
 refs.c                 |   35 ++++++++++++++++++++++++-----------
 t/t1400-update-ref.sh  |    7 +++++++
 t/t3200-branch.sh      |    9 +++++++++
 11 files changed, 53 insertions(+), 22 deletions(-)

[PATCH 1/3] Disallow git branch -m for symrefs.

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:32

This had two problems with symrefs. First, it copied the actual sha1
instead of the "pointer", second it failed to remove the old ref after a
successful rename.

Given that till now delete_ref() always dereferenced symrefs, a new
parameters has been introduced to delete_ref() to allow deleting refs
without a dereference.

For now, we just don't allow renaming symref as no real-word workflow
needs it and doing so would be just an accent.

Signed-off-by: Miklos Vajna <redacted>
---
 builtin-branch.c       |    2 +-
 builtin-receive-pack.c |    2 +-
 builtin-remote.c       |    4 ++--
 builtin-reset.c        |    2 +-
 builtin-send-pack.c    |    2 +-
 builtin-tag.c          |    2 +-
 builtin-update-ref.c   |    2 +-
 cache.h                |    2 +-
 refs.c                 |   33 +++++++++++++++++++++++----------
 t/t3200-branch.sh      |    9 +++++++++
 10 files changed, 41 insertions(+), 19 deletions(-)
diff --git a/builtin-branch.c b/builtin-branch.c
index 8d634ff..2b3613f 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -160,7 +160,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
 			continue;
 		}
 
-		if (delete_ref(name, sha1)) {
+		if (delete_ref(name, sha1, 0)) {
 			error("Error deleting %sbranch '%s'", remote,
 			       argv[i]);
 			ret = 1;
diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 45e3cd9..ab5fa1c 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -224,7 +224,7 @@ static const char *update(struct command *cmd)
 			warning ("Allowing deletion of corrupt ref.");
 			old_sha1 = NULL;
 		}
-		if (delete_ref(name, old_sha1)) {
+		if (delete_ref(name, old_sha1, 0)) {
 			error("failed to delete %s", name);
 			return "failed to delete";
 		}
diff --git a/builtin-remote.c b/builtin-remote.c
index df2be06..e396a3a 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -337,7 +337,7 @@ static int remove_branches(struct string_list *branches)
 		const char *refname = item->string;
 		unsigned char *sha1 = item->util;
 
-		if (delete_ref(refname, sha1))
+		if (delete_ref(refname, sha1, 0))
 			result |= error("Could not remove branch %s", refname);
 	}
 	return result;
@@ -565,7 +565,7 @@ static int prune(int argc, const char **argv)
 			const char *refname = states.stale.items[i].util;
 
 			if (!dry_run)
-				result |= delete_ref(refname, NULL);
+				result |= delete_ref(refname, NULL, 0);
 
 			printf(" * [%s] %s\n", dry_run ? "would prune" : "pruned",
 			       abbrev_ref(refname, "refs/remotes/"));
diff --git a/builtin-reset.c b/builtin-reset.c
index 16e6bb2..9514b77 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -279,7 +279,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		update_ref(msg, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
 	}
 	else if (old_orig)
-		delete_ref("ORIG_HEAD", old_orig);
+		delete_ref("ORIG_HEAD", old_orig, 0);
 	prepend_reflog_action("updating HEAD", msg, sizeof(msg));
 	update_ref_status = update_ref(msg, "HEAD", sha1, orig, 0, MSG_ON_ERR);
 
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 910db92..bbf6e0a 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -234,7 +234,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 		if (args.verbose)
 			fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
 		if (ref->deletion) {
-			delete_ref(rs.dst, NULL);
+			delete_ref(rs.dst, NULL, 0);
 		} else
 			update_ref("update by push", rs.dst,
 					ref->new_sha1, NULL, 0, 0);
diff --git a/builtin-tag.c b/builtin-tag.c
index b13fa34..1ff7b37 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -125,7 +125,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
 static int delete_tag(const char *name, const char *ref,
 				const unsigned char *sha1)
 {
-	if (delete_ref(ref, sha1))
+	if (delete_ref(ref, sha1, 0))
 		return 1;
 	printf("Deleted tag '%s'\n", name);
 	return 0;
diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index 56a0b1b..d8f3142 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -48,7 +48,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		die("%s: not a valid old SHA1", oldval);
 
 	if (delete)
-		return delete_ref(refname, oldval ? oldsha1 : NULL);
+		return delete_ref(refname, oldval ? oldsha1 : NULL, 0);
 	else
 		return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
 				  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);
diff --git a/cache.h b/cache.h
index b0edbf9..a3c77f0 100644
--- a/cache.h
+++ b/cache.h
@@ -434,7 +434,7 @@ extern int commit_locked_index(struct lock_file *);
 extern void set_alternate_index_output(const char *);
 extern int close_lock_file(struct lock_file *);
 extern void rollback_lock_file(struct lock_file *);
-extern int delete_ref(const char *, const unsigned char *sha1);
+extern int delete_ref(const char *, const unsigned char *sha1, int delopt);
 
 /* Environment bits from configuration mechanism */
 extern int trust_executable_bit;
diff --git a/refs.c b/refs.c
index 0a126fa..31a2057 100644
--- a/refs.c
+++ b/refs.c
@@ -921,25 +921,33 @@ static int repack_without_ref(const char *refname)
 	return commit_lock_file(&packlock);
 }
 
-int delete_ref(const char *refname, const unsigned char *sha1)
+int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 {
 	struct ref_lock *lock;
-	int err, i, ret = 0, flag = 0;
+	int err, i = 0, ret = 0, flag = 0;
 
 	lock = lock_ref_sha1_basic(refname, sha1, 0, &flag);
 	if (!lock)
 		return 1;
 	if (!(flag & REF_ISPACKED)) {
 		/* loose */
-		i = strlen(lock->lk->filename) - 5; /* .lock */
-		lock->lk->filename[i] = 0;
-		err = unlink(lock->lk->filename);
+		const char *path;
+
+		if (!(delopt & REF_NODEREF)) {
+			i = strlen(lock->lk->filename) - 5; /* .lock */
+			lock->lk->filename[i] = 0;
+			path = lock->lk->filename;
+		} else {
+			path = git_path(refname);
+		}
+		err = unlink(path);
 		if (err && errno != ENOENT) {
 			ret = 1;
 			error("unlink(%s) failed: %s",
-			      lock->lk->filename, strerror(errno));
+			      path, strerror(errno));
 		}
-		lock->lk->filename[i] = '.';
+		if (!(delopt & REF_NODEREF))
+			lock->lk->filename[i] = '.';
 	}
 	/* removing the loose one could have resurrected an earlier
 	 * packed one.  Also, if it was not loose we need to repack
@@ -964,11 +972,16 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	struct ref_lock *lock;
 	struct stat loginfo;
 	int log = !lstat(git_path("logs/%s", oldref), &loginfo);
+	const char *symref = NULL;
 
 	if (S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldref);
 
-	if (!resolve_ref(oldref, orig_sha1, 1, &flag))
+	symref = resolve_ref(oldref, orig_sha1, 1, &flag);
+	if (flag & REF_ISSYMREF)
+		return error("refname %s is a symbolic ref, renaming it is not supported",
+				oldref);
+	if (!symref)
 		return error("refname %s not found", oldref);
 
 	if (!is_refname_available(newref, oldref, get_packed_refs(), 0))
@@ -988,12 +1001,12 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 		return error("unable to move logfile logs/%s to tmp-renamed-log: %s",
 			oldref, strerror(errno));
 
-	if (delete_ref(oldref, orig_sha1)) {
+	if (delete_ref(oldref, orig_sha1, 0)) {
 		error("unable to delete old %s", oldref);
 		goto rollback;
 	}
 
-	if (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1)) {
+	if (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1, 0)) {
 		if (errno==EISDIR) {
 			if (remove_empty_directories(git_path("%s", newref))) {
 				error("Directory not empty: %s", newref);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 2147eac..20b6c18 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -112,6 +112,15 @@ test_expect_success 'config information was renamed, too' \
 	"test $(git config branch.s.dummy) = Hello &&
 	 test_must_fail git config branch.s/s/dummy"
 
+test_expect_success 'renaming a symref should fail' \
+'
+	git symbolic-ref refs/heads/master2 refs/heads/master &&
+	test_must_fail git branch -m master2 master3 &&
+	git symbolic-ref refs/heads/master2 &&
+	test -f .git/refs/heads/master &&
+	! test -f .git/refs/heads/master3
+'
+
 test_expect_success \
     'git branch -m u v should fail when the reflog for u is a symlink' '
      git branch -l u &&
-- 
1.6.0.2

Re: [PATCH 0/3] symref rename/delete fixes

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:32

On Mon, Oct 27, 2008 at 08:50:19PM +0100, Miklos Vajna [off-list ref] wrote:
Here they are. The first is updated: the delete_ref() part is unchanged,
but the rename_ref() part is dropped and it just bails out with an error
if it detects a symref. The testcase is updated according to this as
well.

The two others are just rebased on top of the new first one, no
functional changes.
I forgot to fetch and did not notice the previous round is already in
'next'. Just forget about these patches then, I'll rebase the relevant
part against mv/maint-branch-m-symref and resend.

[PATCH] git branch -m: forbid renaming of a symref

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:32

There may be cases where one would really want to rename the symbolic
ref without changing its value, but "git branch -m" is not such a
use-case.

Signed-off-by: Miklos Vajna <redacted>
---

This applies on top of 'mv/maint-branch-m-symref'.

 refs.c            |   29 +++++++++++++----------------
 t/t3200-branch.sh |    8 ++++----
 2 files changed, 17 insertions(+), 20 deletions(-)
diff --git a/refs.c b/refs.c
index b39e6f2..8a38e08 100644
--- a/refs.c
+++ b/refs.c
@@ -964,14 +964,14 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	struct stat loginfo;
 	int log = !lstat(git_path("logs/%s", oldref), &loginfo);
 	const char *symref = NULL;
-	int is_symref = 0;
 
 	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldref);
 
 	symref = resolve_ref(oldref, orig_sha1, 1, &flag);
 	if (flag & REF_ISSYMREF)
-		is_symref = 1;
+		return error("refname %s is a symbolic ref, renaming it is not supported",
+			oldref);
 	if (!symref)
 		return error("refname %s not found", oldref);
 
@@ -1035,20 +1035,17 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	}
 	logmoved = log;
 
-	if (!is_symref) {
-		lock = lock_ref_sha1_basic(newref, NULL, 0, NULL);
-		if (!lock) {
-			error("unable to lock %s for update", newref);
-			goto rollback;
-		}
-		lock->force_write = 1;
-		hashcpy(lock->old_sha1, orig_sha1);
-		if (write_ref_sha1(lock, orig_sha1, logmsg)) {
-			error("unable to write current sha1 into %s", newref);
-			goto rollback;
-		}
-	} else
-		create_symref(newref, symref, logmsg);
+	lock = lock_ref_sha1_basic(newref, NULL, 0, NULL);
+	if (!lock) {
+		error("unable to lock %s for update", newref);
+		goto rollback;
+	}
+	lock->force_write = 1;
+	hashcpy(lock->old_sha1, orig_sha1);
+	if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+		error("unable to write current sha1 into %s", newref);
+		goto rollback;
+	}
 
 	return 0;
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index fdeb1f5..25e9971 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -112,13 +112,13 @@ test_expect_success 'config information was renamed, too' \
 	"test $(git config branch.s.dummy) = Hello &&
 	 test_must_fail git config branch.s/s/dummy"
 
-test_expect_success 'renaming a symref' \
+test_expect_success 'renaming a symref is not allowed' \
 '
 	git symbolic-ref refs/heads/master2 refs/heads/master &&
-	git branch -m master2 master3 &&
-	git symbolic-ref refs/heads/master3 &&
+	test_must_fail git branch -m master2 master3 &&
+	git symbolic-ref refs/heads/master2 &&
 	test -f .git/refs/heads/master &&
-	! test -f .git/refs/heads/master2
+	! test -f .git/refs/heads/master3
 '
 
 test_expect_success \
-- 
1.6.0.2
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help