# [PATCH 3/3] Fix git update-ref --no-deref -d. 6 messages from 2008-10-27 to 2008-10-29. Participants: Miklos Vajna. Thread: https://gitlist.dev/t/16065 ## Miklos Vajna, 2008-10-27 19:50 Subject: [PATCH 0/3] symref rename/delete fixes Message-ID: URL: https://gitlist.dev/e/cover.1225136865.git.vmiklos%40frugalware.org In-Reply-To: <20081027085055.GL2273@genesis.frugalware.org> ``` On Mon, Oct 27, 2008 at 09:50:55AM +0100, Miklos Vajna 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(-) ``` ## Miklos Vajna, 2008-10-27 19:50 Subject: [PATCH 1/3] Disallow git branch -m for symrefs. Message-ID: URL: https://gitlist.dev/e/a18363084130c4c23279667f6f0bd26d585a061d.1225136865.git.vmiklos%40frugalware.org In-Reply-To: ``` 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 --- 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 ``` ## Miklos Vajna, 2008-10-27 19:50 Subject: [PATCH 2/3] rename_ref(): handle the case when the reflog of a ref does not exist Message-ID: <5eea93aa5f6d35405eafee4fd022dc9f0b5d75b6.1225136865.git.vmiklos@frugalware.org> URL: https://gitlist.dev/e/5eea93aa5f6d35405eafee4fd022dc9f0b5d75b6.1225136865.git.vmiklos%40frugalware.org In-Reply-To: ``` 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 --- 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 ``` ## Miklos Vajna, 2008-10-27 19:50 Subject: [PATCH 3/3] Fix git update-ref --no-deref -d. Message-ID: <509c44856b50b0cf55e72cf8ec56b477a0c89790.1225136865.git.vmiklos@frugalware.org> URL: https://gitlist.dev/e/509c44856b50b0cf55e72cf8ec56b477a0c89790.1225136865.git.vmiklos%40frugalware.org In-Reply-To: <5eea93aa5f6d35405eafee4fd022dc9f0b5d75b6.1225136865.git.vmiklos@frugalware.org> ``` Till now --no-deref was just ignored when deleting refs, fix this. Signed-off-by: Miklos Vajna --- 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 ``` ## Miklos Vajna, 2008-10-28 23:45 Subject: Re: [PATCH 0/3] symref rename/delete fixes Message-ID: <20081028234541.GY24201@genesis.frugalware.org> URL: https://gitlist.dev/e/20081028234541.GY24201%40genesis.frugalware.org In-Reply-To: ``` On Mon, Oct 27, 2008 at 08:50:19PM +0100, Miklos Vajna 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. ``` ## Miklos Vajna, 2008-10-29 00:05 Subject: [PATCH] git branch -m: forbid renaming of a symref Message-ID: <1225238727-12399-1-git-send-email-vmiklos@frugalware.org> URL: https://gitlist.dev/e/1225238727-12399-1-git-send-email-vmiklos%40frugalware.org In-Reply-To: <20081028234541.GY24201@genesis.frugalware.org> ``` 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 --- 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 ```