{"thread":{"id":"16045","subject":"[PATCH 3/3] Fix git update-ref --no-deref -d.","startedAt":"2008-10-26T02:33:55Z","lastAt":"2008-10-27T08:50:55Z","messageCount":6,"participants":["Miklos Vajna","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"93953","messageId":"cover.1224987944.git.vmiklos@frugalware.org","threadId":"16045","inReplyTo":"7v8wsca5ne.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 0/3] symref rename/delete fixes","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-10-26T02:33:55Z","receivedAt":"2008-10-26T02:33:55Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Sat, Oct 25, 2008 at 11:31:01AM -0700, Junio C Hamano <gitster@pobox.com> wrote:\n> > +int delete_ref(const char *refname, const unsigned char *sha1, int\n> > flags)\n> >  {\n> >     struct ref_lock *lock;\n> > -   int err, i, ret = 0, flag = 0;\n> > +   int err, i = 0, ret = 0, flag = 0;\n> > +   char *path;\n\n> Two variables flag vs flags is a bit confusing, isn't it?  How about\n> naming the new one \"delopt\" or something?\n\nRenamed.\n\n> The new variable \"char *path\" at the toplevel can be confined in the\n> scope\n> of this if () {} block and probably can become \"const char *\", right?\n\nYes - moved.\n\n> >     int log = !lstat(git_path(\"logs/%s\", oldref), &loginfo);\n> > +   const char *symref = NULL;\n> > +   int is_symref = 0;\n> >\n> >     if (S_ISLNK(loginfo.st_mode))\n> >             return error(\"reflog for %s is a symlink\", oldref);\n>\n> Possible bug in the context.  When there is no reflog for the ref\n> being\n> renamed, lstat would fail; it doesn't feel right to have this\n> S_ISLNK()\n> before checking the result of the lstat which is in \"log\".\n\nOK, this went to a separate patch.\n\n> > +   symref = resolve_ref(oldref, orig_sha1, 0, &flag);\n> > +   if (flag & REF_ISSYMREF)\n> > +           is_symref = 1;\n> >     if (!resolve_ref(oldref, orig_sha1, 1, &flag))\n> >             return error(\"refname %s not found\", oldref);\n>\n> Do we really need two calls to resolve_ref()?  Your new call calls it\n> without must-exist bit --- why?  Immediately after that, the existing\n> call\n> will barf if it does not exist anyway.\n\nJust having\n\n        if (!symref)\n                return error(\"refname %s not found\", oldref);\n\nfirst looks weird, given that the error message is not \"refname %s is\nnot a symref\", but you are right, changed.\n\n> I agree it is good to have symref aware delete_ref(), but I am not\n> sure\n> supporting symref in rename_ref() is either needed or necessarily a\n> good\n> idea.  You also need to worry about a symref pointing at a branch yet\n> to\n> be born.\n\nThat is currently not supported and the error message of 'git branch -m'\nis (in case foo points to refs/heads/bar and bar is not yet born):\n\nerror: refname refs/heads/foo not found\nfatal: Branch rename failed\n\nwhich is quite acceptable, IMHO[1].\n\n> In the meantime, I think we should just check (flag & REF_ISSYMREF)\n> after\n> the existing resolve_ref() we can see in the context above, and error\n> out\n> saying you cannot rename a symref, and do nothing else.\n\nA symref-aware rename_ref() is needed by git remove rename, since it\ntypically does origin/HEAD -> upstream/HEAD symref renames there.\n\nOf course you can say that this should be handled by git-remote itself,\nwithout using rename_ref() but that not seem to be a good solution to\nme. (Workaround in the wrong layer, instead of a solution in a good\none.)\n\n[1] I mean, I have a real-world scenario (git remove rename) for \"why\nrenaming a symref is a good idea\", but I don't think renaming a ref\npointing to a yet-to-be-born ref has any real world users.\n\nMiklos Vajna (3):\n  Fix git branch -m for symrefs.\n  rename_ref(): handle the case when the reflog of a ref does not exist\n  Fix git update-ref --no-deref -d.\n\n builtin-branch.c       |    2 +-\n builtin-receive-pack.c |    2 +-\n builtin-remote.c       |    4 +-\n builtin-reset.c        |    2 +-\n builtin-send-pack.c    |    2 +-\n builtin-tag.c          |    2 +-\n builtin-update-ref.c   |    8 ++++--\n cache.h                |    2 +-\n refs.c                 |   61 +++++++++++++++++++++++++++++------------------\n t/t1400-update-ref.sh  |    7 +++++\n t/t3200-branch.sh      |    9 +++++++\n 11 files changed, 67 insertions(+), 34 deletions(-)\n\nAlso available in the 'symref-mv' branch of 'git://repo.or.cz/git/vmiklos.git'.\n"},{"id":"93954","messageId":"39b6bd01cf753d48a803e1d5908e543b66899c77.1224987944.git.vmiklos@frugalware.org","threadId":"16045","inReplyTo":"cover.1224987944.git.vmiklos@frugalware.org","subject":"[PATCH 1/3] Fix git branch -m for symrefs.","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-10-26T02:33:56Z","receivedAt":"2008-10-26T02:33:56Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"This had two problems with symrefs. First, it copied the actual sha1\ninstead of the \"pointer\", second it failed to remove the old ref after a\nsuccessful rename.\n\nGiven that till now delete_ref() always dereferenced symrefs, a new\nparameters has been introduced to delete_ref() to allow deleting refs\nwithout a dereference.\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n builtin-branch.c       |    2 +-\n builtin-receive-pack.c |    2 +-\n builtin-remote.c       |    4 +-\n builtin-reset.c        |    2 +-\n builtin-send-pack.c    |    2 +-\n builtin-tag.c          |    2 +-\n builtin-update-ref.c   |    2 +-\n cache.h                |    2 +-\n refs.c                 |   59 ++++++++++++++++++++++++++++++------------------\n t/t3200-branch.sh      |    9 +++++++\n 10 files changed, 55 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin-branch.c b/builtin-branch.c\nindex 8d634ff..2b3613f 100644\n--- a/builtin-branch.c\n+++ b/builtin-branch.c\n@@ -160,7 +160,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (delete_ref(name, sha1)) {\n+\t\tif (delete_ref(name, sha1, 0)) {\n \t\t\terror(\"Error deleting %sbranch '%s'\", remote,\n \t\t\t       argv[i]);\n \t\t\tret = 1;\ndiff --git a/builtin-receive-pack.c b/builtin-receive-pack.c\nindex 45e3cd9..ab5fa1c 100644\n--- a/builtin-receive-pack.c\n+++ b/builtin-receive-pack.c\n@@ -224,7 +224,7 @@ static const char *update(struct command *cmd)\n \t\t\twarning (\"Allowing deletion of corrupt ref.\");\n \t\t\told_sha1 = NULL;\n \t\t}\n-\t\tif (delete_ref(name, old_sha1)) {\n+\t\tif (delete_ref(name, old_sha1, 0)) {\n \t\t\terror(\"failed to delete %s\", name);\n \t\t\treturn \"failed to delete\";\n \t\t}\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex df2be06..e396a3a 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -337,7 +337,7 @@ static int remove_branches(struct string_list *branches)\n \t\tconst char *refname = item->string;\n \t\tunsigned char *sha1 = item->util;\n \n-\t\tif (delete_ref(refname, sha1))\n+\t\tif (delete_ref(refname, sha1, 0))\n \t\t\tresult |= error(\"Could not remove branch %s\", refname);\n \t}\n \treturn result;\n@@ -565,7 +565,7 @@ static int prune(int argc, const char **argv)\n \t\t\tconst char *refname = states.stale.items[i].util;\n \n \t\t\tif (!dry_run)\n-\t\t\t\tresult |= delete_ref(refname, NULL);\n+\t\t\t\tresult |= delete_ref(refname, NULL, 0);\n \n \t\t\tprintf(\" * [%s] %s\\n\", dry_run ? \"would prune\" : \"pruned\",\n \t\t\t       abbrev_ref(refname, \"refs/remotes/\"));\ndiff --git a/builtin-reset.c b/builtin-reset.c\nindex 16e6bb2..9514b77 100644\n--- a/builtin-reset.c\n+++ b/builtin-reset.c\n@@ -279,7 +279,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\tupdate_ref(msg, \"ORIG_HEAD\", orig, old_orig, 0, MSG_ON_ERR);\n \t}\n \telse if (old_orig)\n-\t\tdelete_ref(\"ORIG_HEAD\", old_orig);\n+\t\tdelete_ref(\"ORIG_HEAD\", old_orig, 0);\n \tprepend_reflog_action(\"updating HEAD\", msg, sizeof(msg));\n \tupdate_ref_status = update_ref(msg, \"HEAD\", sha1, orig, 0, MSG_ON_ERR);\n \ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 910db92..bbf6e0a 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -234,7 +234,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)\n \t\tif (args.verbose)\n \t\t\tfprintf(stderr, \"updating local tracking ref '%s'\\n\", rs.dst);\n \t\tif (ref->deletion) {\n-\t\t\tdelete_ref(rs.dst, NULL);\n+\t\t\tdelete_ref(rs.dst, NULL, 0);\n \t\t} else\n \t\t\tupdate_ref(\"update by push\", rs.dst,\n \t\t\t\t\tref->new_sha1, NULL, 0, 0);\ndiff --git a/builtin-tag.c b/builtin-tag.c\nindex b13fa34..1ff7b37 100644\n--- a/builtin-tag.c\n+++ b/builtin-tag.c\n@@ -125,7 +125,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)\n static int delete_tag(const char *name, const char *ref,\n \t\t\t\tconst unsigned char *sha1)\n {\n-\tif (delete_ref(ref, sha1))\n+\tif (delete_ref(ref, sha1, 0))\n \t\treturn 1;\n \tprintf(\"Deleted tag '%s'\\n\", name);\n \treturn 0;\ndiff --git a/builtin-update-ref.c b/builtin-update-ref.c\nindex 56a0b1b..d8f3142 100644\n--- a/builtin-update-ref.c\n+++ b/builtin-update-ref.c\n@@ -48,7 +48,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t\tdie(\"%s: not a valid old SHA1\", oldval);\n \n \tif (delete)\n-\t\treturn delete_ref(refname, oldval ? oldsha1 : NULL);\n+\t\treturn delete_ref(refname, oldval ? oldsha1 : NULL, 0);\n \telse\n \t\treturn update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,\n \t\t\t\t  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);\ndiff --git a/cache.h b/cache.h\nindex b0edbf9..a3c77f0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -434,7 +434,7 @@ extern int commit_locked_index(struct lock_file *);\n extern void set_alternate_index_output(const char *);\n extern int close_lock_file(struct lock_file *);\n extern void rollback_lock_file(struct lock_file *);\n-extern int delete_ref(const char *, const unsigned char *sha1);\n+extern int delete_ref(const char *, const unsigned char *sha1, int delopt);\n \n /* Environment bits from configuration mechanism */\n extern int trust_executable_bit;\ndiff --git a/refs.c b/refs.c\nindex 0a126fa..70c0967 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -921,25 +921,33 @@ static int repack_without_ref(const char *refname)\n \treturn commit_lock_file(&packlock);\n }\n \n-int delete_ref(const char *refname, const unsigned char *sha1)\n+int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n {\n \tstruct ref_lock *lock;\n-\tint err, i, ret = 0, flag = 0;\n+\tint err, i = 0, ret = 0, flag = 0;\n \n \tlock = lock_ref_sha1_basic(refname, sha1, 0, &flag);\n \tif (!lock)\n \t\treturn 1;\n \tif (!(flag & REF_ISPACKED)) {\n \t\t/* loose */\n-\t\ti = strlen(lock->lk->filename) - 5; /* .lock */\n-\t\tlock->lk->filename[i] = 0;\n-\t\terr = unlink(lock->lk->filename);\n+\t\tconst char *path;\n+\n+\t\tif (!(delopt & REF_NODEREF)) {\n+\t\t\ti = strlen(lock->lk->filename) - 5; /* .lock */\n+\t\t\tlock->lk->filename[i] = 0;\n+\t\t\tpath = lock->lk->filename;\n+\t\t} else {\n+\t\t\tpath = git_path(refname);\n+\t\t}\n+\t\terr = unlink(path);\n \t\tif (err && errno != ENOENT) {\n \t\t\tret = 1;\n \t\t\terror(\"unlink(%s) failed: %s\",\n-\t\t\t      lock->lk->filename, strerror(errno));\n+\t\t\t      path, strerror(errno));\n \t\t}\n-\t\tlock->lk->filename[i] = '.';\n+\t\tif (!(delopt & REF_NODEREF))\n+\t\t\tlock->lk->filename[i] = '.';\n \t}\n \t/* removing the loose one could have resurrected an earlier\n \t * packed one.  Also, if it was not loose we need to repack\n@@ -964,11 +972,16 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)\n \tstruct ref_lock *lock;\n \tstruct stat loginfo;\n \tint log = !lstat(git_path(\"logs/%s\", oldref), &loginfo);\n+\tconst char *symref = NULL;\n+\tint is_symref = 0;\n \n \tif (S_ISLNK(loginfo.st_mode))\n \t\treturn error(\"reflog for %s is a symlink\", oldref);\n \n-\tif (!resolve_ref(oldref, orig_sha1, 1, &flag))\n+\tsymref = resolve_ref(oldref, orig_sha1, 1, &flag);\n+\tif (flag & REF_ISSYMREF)\n+\t\tis_symref = 1;\n+\tif (!symref)\n \t\treturn error(\"refname %s not found\", oldref);\n \n \tif (!is_refname_available(newref, oldref, get_packed_refs(), 0))\n@@ -988,12 +1001,12 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)\n \t\treturn error(\"unable to move logfile logs/%s to tmp-renamed-log: %s\",\n \t\t\toldref, strerror(errno));\n \n-\tif (delete_ref(oldref, orig_sha1)) {\n+\tif (delete_ref(oldref, orig_sha1, REF_NODEREF)) {\n \t\terror(\"unable to delete old %s\", oldref);\n \t\tgoto rollback;\n \t}\n \n-\tif (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1)) {\n+\tif (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1, REF_NODEREF)) {\n \t\tif (errno==EISDIR) {\n \t\t\tif (remove_empty_directories(git_path(\"%s\", newref))) {\n \t\t\t\terror(\"Directory not empty: %s\", newref);\n@@ -1031,18 +1044,20 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)\n \t}\n \tlogmoved = log;\n \n-\tlock = lock_ref_sha1_basic(newref, NULL, 0, NULL);\n-\tif (!lock) {\n-\t\terror(\"unable to lock %s for update\", newref);\n-\t\tgoto rollback;\n-\t}\n-\n-\tlock->force_write = 1;\n-\thashcpy(lock->old_sha1, orig_sha1);\n-\tif (write_ref_sha1(lock, orig_sha1, logmsg)) {\n-\t\terror(\"unable to write current sha1 into %s\", newref);\n-\t\tgoto rollback;\n-\t}\n+\tif (!is_symref) {\n+\t\tlock = lock_ref_sha1_basic(newref, NULL, 0, NULL);\n+\t\tif (!lock) {\n+\t\t\terror(\"unable to lock %s for update\", newref);\n+\t\t\tgoto rollback;\n+\t\t}\n+\t\tlock->force_write = 1;\n+\t\thashcpy(lock->old_sha1, orig_sha1);\n+\t\tif (write_ref_sha1(lock, orig_sha1, logmsg)) {\n+\t\t\terror(\"unable to write current sha1 into %s\", newref);\n+\t\t\tgoto rollback;\n+\t\t}\n+\t} else\n+\t\tcreate_symref(newref, symref, logmsg);\n \n \treturn 0;\n \ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 2147eac..fdeb1f5 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -112,6 +112,15 @@ test_expect_success 'config information was renamed, too' \\\n \t\"test $(git config branch.s.dummy) = Hello &&\n \t test_must_fail git config branch.s/s/dummy\"\n \n+test_expect_success 'renaming a symref' \\\n+'\n+\tgit symbolic-ref refs/heads/master2 refs/heads/master &&\n+\tgit branch -m master2 master3 &&\n+\tgit symbolic-ref refs/heads/master3 &&\n+\ttest -f .git/refs/heads/master &&\n+\t! test -f .git/refs/heads/master2\n+'\n+\n test_expect_success \\\n     'git branch -m u v should fail when the reflog for u is a symlink' '\n      git branch -l u &&\n-- \n1.6.0.2\n"},{"id":"93952","messageId":"b06bf89aeeb844903e16f7c231c6add011a910bf.1224987945.git.vmiklos@frugalware.org","threadId":"16045","inReplyTo":"39b6bd01cf753d48a803e1d5908e543b66899c77.1224987944.git.vmiklos@frugalware.org","subject":"[PATCH 2/3] rename_ref(): handle the case when the reflog of a ref does not exist","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-10-26T02:33:57Z","receivedAt":"2008-10-26T02:33:57Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"We tried to check if a reflog of a ref is a symlink without first\nchecking if it exists, which is a bug.\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n refs.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 70c0967..3c39a31 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -975,7 +975,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)\n \tconst char *symref = NULL;\n \tint is_symref = 0;\n \n-\tif (S_ISLNK(loginfo.st_mode))\n+\tif (log && S_ISLNK(loginfo.st_mode))\n \t\treturn error(\"reflog for %s is a symlink\", oldref);\n \n \tsymref = resolve_ref(oldref, orig_sha1, 1, &flag);\n-- \n1.6.0.2\n"},{"id":"93951","messageId":"f10fa3f86cbdf14ae935eaf806ee6b4a1922e0c2.1224987945.git.vmiklos@frugalware.org","threadId":"16045","inReplyTo":"b06bf89aeeb844903e16f7c231c6add011a910bf.1224987945.git.vmiklos@frugalware.org","subject":"[PATCH 3/3] Fix git update-ref --no-deref -d.","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-10-26T02:33:58Z","receivedAt":"2008-10-26T02:33:58Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"Till now --no-deref was just ignored when deleting refs, fix this.\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n builtin-update-ref.c  |    8 +++++---\n t/t1400-update-ref.sh |    7 +++++++\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-update-ref.c b/builtin-update-ref.c\nindex d8f3142..378dc1b 100644\n--- a/builtin-update-ref.c\n+++ b/builtin-update-ref.c\n@@ -13,7 +13,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n {\n \tconst char *refname, *oldval, *msg=NULL;\n \tunsigned char sha1[20], oldsha1[20];\n-\tint delete = 0, no_deref = 0;\n+\tint delete = 0, no_deref = 0, flags = 0;\n \tstruct option options[] = {\n \t\tOPT_STRING( 'm', NULL, &msg, \"reason\", \"reason of the update\"),\n \t\tOPT_BOOLEAN('d', NULL, &delete, \"deletes the reference\"),\n@@ -47,9 +47,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \tif (oldval && *oldval && get_sha1(oldval, oldsha1))\n \t\tdie(\"%s: not a valid old SHA1\", oldval);\n \n+\tif (no_deref)\n+\t\tflags = REF_NODEREF;\n \tif (delete)\n-\t\treturn delete_ref(refname, oldval ? oldsha1 : NULL, 0);\n+\t\treturn delete_ref(refname, oldval ? oldsha1 : NULL, flags);\n \telse\n \t\treturn update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,\n-\t\t\t\t  no_deref ? REF_NODEREF : 0, DIE_ON_ERR);\n+\t\t\t\t  flags, DIE_ON_ERR);\n }\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 04c2b16..8139cd6 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -75,6 +75,13 @@ test_expect_success \"delete $m (by HEAD)\" '\n '\n rm -f .git/$m\n \n+cp -f .git/HEAD .git/HEAD.orig\n+test_expect_success \"delete symref without dereference\" '\n+\tgit update-ref --no-deref -d HEAD &&\n+\t! test -f .git/HEAD\n+'\n+cp -f .git/HEAD.orig .git/HEAD\n+\n test_expect_success '(not) create HEAD with old sha1' \"\n \ttest_must_fail git update-ref HEAD $A $B\n \"\n-- \n1.6.0.2\n"},{"id":"94022","messageId":"7vhc6yioyq.fsf@gitster.siamese.dyndns.org","threadId":"16045","inReplyTo":"cover.1224987944.git.vmiklos@frugalware.org","subject":"Re: [PATCH 0/3] symref rename/delete fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-27T05:31:09Z","receivedAt":"2008-10-27T05:31:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna <vmiklos@frugalware.org> writes:\n\n> A symref-aware rename_ref() is needed by git remove rename, since it\n> typically does origin/HEAD -> upstream/HEAD symref renames there.\n>\n> Of course you can say that this should be handled by git-remote itself,\n> without using rename_ref() but that not seem to be a good solution to\n> me. (Workaround in the wrong layer, instead of a solution in a good\n> one.)\n\nI do not think it is a workaround at all.\n\nI would even say that the renaming of symref that \"git remote rename\"\nneeds to do is fundamentally different from what rename_ref() is about,\nand trying to cram it into rename_ref() is a grave mistake.\n\nIf you \"git remote rename origin upstream\" when origin/HEAD points at\nrefs/remotes/origin/master, you need to make the renamed one point at\nrefs/remotes/upstream/master, as you will be renaming origin/master to\nupstream/master.\n\nNormal \"rename_ref()\" would just rename the ref without touching its\ncontents, and if you used it to implement \"git remote rename\", your\nupstream/HEAD would point at the old name \"origin/master\" that will\ndisappear when rename is finished, wouldn't it?  I do not think it is\nuseful.\n\nThere may be cases where you would really want to rename the symbolic ref\nwithout changing its value (e.g. which other ref it points at), but as you\nmentioned, even \"git branch -m\" is not such a usecase.\n\nI think it is better to simply forbid renaming of a symref _until_ we know\nwhat we want it to mean.  It is a lot easier to start strict and then add\nfeatures, than start loosely and implement an unclean semantics, and then\nhaving to fix that semantics after people start to rely on the initial\n(potentially crazy) semantics.\n"},{"id":"94033","messageId":"20081027085055.GL2273@genesis.frugalware.org","threadId":"16045","inReplyTo":"7vhc6yioyq.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 0/3] symref rename/delete fixes","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-10-27T08:50:55Z","receivedAt":"2008-10-27T08:50:55Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Sun, Oct 26, 2008 at 10:31:09PM -0700, Junio C Hamano <gitster@pobox.com> wrote:\n> > Of course you can say that this should be handled by git-remote itself,\n> > without using rename_ref() but that not seem to be a good solution to\n> > me. (Workaround in the wrong layer, instead of a solution in a good\n> > one.)\n> \n> I do not think it is a workaround at all.\n> \n> I would even say that the renaming of symref that \"git remote rename\"\n> needs to do is fundamentally different from what rename_ref() is about,\n> and trying to cram it into rename_ref() is a grave mistake.\n> \n> If you \"git remote rename origin upstream\" when origin/HEAD points at\n> refs/remotes/origin/master, you need to make the renamed one point at\n> refs/remotes/upstream/master, as you will be renaming origin/master to\n> upstream/master.\n> \n> Normal \"rename_ref()\" would just rename the ref without touching its\n> contents, and if you used it to implement \"git remote rename\", your\n> upstream/HEAD would point at the old name \"origin/master\" that will\n> disappear when rename is finished, wouldn't it?  I do not think it is\n> useful.\n\nAh, I missed that. You convienced me, I'll post updated patches later\ntoday. :)\n"}]}