threads / patch / 16045

patch, 3 partsFix git update-ref --no-deref -d.

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

## tl;dr

6 messages between Oct 26, 2008 and Oct 27, 2008. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Miklos Vajna· Oct 26, 2008, 02:33 UTC · lore

[PATCH 0/3] symref rename/delete fixes

On Sat, Oct 25, 2008 at 11:31:01AM -0700, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> > +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.
Show 12 quoted lines
> >     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.
Show 10 quoted lines
> > +   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.

Show 7 quoted lines
> 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].
Show 5 quoted lines
> 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'.
Miklos Vajna· Oct 26, 2008, 02:33 UTC · re: Miklos Vajna · lore

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

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 <vmiklos@frugalware.org>
---
 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(-)
Show changes to 10 files +55 −31

builtin-branch.c, builtin-receive-pack.c, builtin-remote.c, builtin-reset.c, builtin-send-pack.c, builtin-tag.c, builtin-update-ref.c, cache.h, refs.c, t/t3200-branch.sh

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
Miklos Vajna· Oct 26, 2008, 02:33 UTC · re: Miklos Vajna · lore

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

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 <vmiklos@frugalware.org>
---
 refs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to refs.c +1 −1
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
Miklos Vajna· Oct 26, 2008, 02:33 UTC · re: Miklos Vajna · lore
Till now --no-deref was just ignored when deleting refs, fix this.
Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-update-ref.c  |    8 +++++---
 t/t1400-update-ref.sh |    7 +++++++
 2 files changed, 12 insertions(+), 3 deletions(-)
Show changes to 2 files +12 −3

builtin-update-ref.c, t/t1400-update-ref.sh

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
Junio C Hamano· Oct 27, 2008, 05:31 UTC · re: Miklos Vajna · lore

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

Miklos Vajna <vmiklos@frugalware.org> writes:
Show 7 quoted lines
> 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.

Miklos Vajna· Oct 27, 2008, 08:50 UTC · re: Junio C Hamano · lore

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

On Sun, Oct 26, 2008 at 10:31:09PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
Show 21 quoted lines
> > 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. :)

← back to recent threads