# [PATCH 2/2] Fix git update-ref --no-deref -d. 4 messages from 2008-10-25 to 2008-10-25. Participants: Miklos Vajna, Junio C Hamano. Thread: https://gitlist.dev/t/16041 ## Miklos Vajna, 2008-10-25 12:58 Subject: [PATCH 0/2] Fixes for git branch -m / update-ref --no-deref -d Message-ID: URL: https://gitlist.dev/e/cover.1224939436.git.vmiklos%40frugalware.org In-Reply-To: <7v63nh1sc7.fsf@gitster.siamese.dyndns.org> ``` On Fri, Oct 24, 2008 at 04:33:28PM -0700, Junio C Hamano wrote: > Hmm, remote_get() can read from all three supported places that you > can > define remotes. Could you explain what happens if the old remote is > read > from say $GIT_DIR/remotes/origin and you are renaming it to "upstream" > with "git remote rename origin upstream"? While trying to answer your question, I noticed that rename_ref()/delete_ref() did not really handled symrefs. Regarding rename_ref() (for users: git branch -m) I think you can't create symrefs in the refs/heads namespace without using plumbing, so most users are not affected. Regarding delete_ref() (for users: git update-ref --no-deref -d) in most repos you just have HEAD as symref and you never want to delete it, but in case the user asks for it, I think we just have to do so. Here are two patches to fix these issues (and in fact they will be required for git remote rename as well). Miklos Vajna (2): Fix git branch -m for symrefs. 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 | 56 ++++++++++++++++++++++++++++++------------------ t/t1400-update-ref.sh | 7 ++++++ t/t3200-branch.sh | 9 +++++++ 11 files changed, 64 insertions(+), 32 deletions(-) ``` ## Miklos Vajna, 2008-10-25 12:58 Subject: [PATCH 1/2] Fix git branch -m for symrefs. Message-ID: URL: https://gitlist.dev/e/a96243124c555cbc4081f733b348252ac200bd53.1224939436.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. 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 | 56 ++++++++++++++++++++++++++++++------------------ t/t3200-branch.sh | 9 +++++++ 10 files changed, 53 insertions(+), 30 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 a5883df..3f2113c 100644 --- a/builtin-remote.c +++ b/builtin-remote.c @@ -441,7 +441,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; @@ -669,7 +669,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..9951952 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 flags); /* Environment bits from configuration mechanism */ extern int trust_executable_bit; diff --git a/refs.c b/refs.c index 0a126fa..93a61e1 100644 --- a/refs.c +++ b/refs.c @@ -921,25 +921,32 @@ 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 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)) { /* loose */ - i = strlen(lock->lk->filename) - 5; /* .lock */ - lock->lk->filename[i] = 0; - err = unlink(lock->lk->filename); + if (!(flags & 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 (!(flags & 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,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); + 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); @@ -988,12 +1000,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 +1043,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, 2008-10-25 12:58 Subject: [PATCH 2/2] Fix git update-ref --no-deref -d. Message-ID: URL: https://gitlist.dev/e/f4adb5a66764e620125adc99abd65b1ca897513d.1224939436.git.vmiklos%40frugalware.org In-Reply-To: ``` 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 ``` ## Junio C Hamano, 2008-10-25 18:31 Subject: Re: [PATCH 1/2] Fix git branch -m for symrefs. Message-ID: <7v8wsca5ne.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8wsca5ne.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Miklos Vajna 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? > @@ -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. ```