{"thread":{"id":"31820","subject":"BUG when trying to delete symbolic refs","startedAt":"2012-10-15T08:50:58Z","lastAt":"2012-10-21T19:09:01Z","messageCount":16,"participants":["Johan Herland","René Scharfe","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"201235","messageId":"CALKQrgfnvV+1XHjeSytj+LxkAabZJK3hewxH7WT0nkX-ewOKUA@mail.gmail.com","threadId":"31820","inReplyTo":null,"subject":"BUG when trying to delete symbolic refs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2012-10-15T08:50:58Z","receivedAt":"2012-10-15T08:50:58Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Hi,\n\nAt $dayjob we renamed a branch, and for a grace period, we kept the\nold name as a symref/alias to the new name, to give our users a window\nfor switching. This has worked well, until we tried to remove the\nsymref/alias. The following script demonstrates what we discovered:\n\n $ git --version\n git version 1.8.0.rc2.249.g6cc8227\n $ git init symref_delete_test/\n Initialized empty Git repository in .../symref_delete_test/.git/\n $ cd symref_delete_test/\n $ echo foo > foo && git add foo && git commit -m foo\n [master (root-commit) c7ae77e] foo\n  1 file changed, 1 insertion(+)\n  create mode 100644 foo\n $ git gc\n Counting objects: 3, done.\n Writing objects: 100% (3/3), done.\n Total 3 (delta 0), reused 0 (delta 0)\n $ cat .git/packed-refs\n # pack-refs with: peeled\n c7ae77e537138bee3f722e57e1af87a7011466cb refs/heads/master\n $ echo bar > foo && git commit -am bar\n [master 7451bf0] bar\n  1 file changed, 1 insertion(+), 1 deletion(-)\n $ git symbolic-ref refs/heads/alias refs/heads/master\n $ git rev-parse master\n 7451bf08b7aacedc9e88a9fa37a6c1f701071bbe\n $ git rev-parse alias\n 7451bf08b7aacedc9e88a9fa37a6c1f701071bbe\n $ git branch -d alias\n Deleted branch alias (was 7451bf0).\n $ git rev-parse master\n c7ae77e537138bee3f722e57e1af87a7011466cb\n $ git rev-parse alias\n c7ae77e537138bee3f722e57e1af87a7011466cb\n $ cat .git/packed-refs\n # pack-refs with: peeled\n c7ae77e537138bee3f722e57e1af87a7011466cb refs/heads/master\n $ ls .git/refs/heads/\n alias\n\nBasically, there is a \"master\" branch, and an \"alias\" symref to\n\"master\". When we naively try to delete the symref with \"git branch -d\nalias\", it ends up:\n\n - NOT deleting the \"alias\" symref\n - DELETING the \"master\" loose ref\n - NOT deleting the \"master\" packed ref\n\nSo, from the user perspective, \"git branch -d alias\" ends up resetting\n\"master\" (and \"alias\") back to the last time we happened to run \"git\ngc\". Needless to say, this is not quite what we had in mind...\n\nAFAICS, there may be three possible \"acceptable\" outcomes when we run\n\"git branch -d alias\" in the above scenario:\n\n A. The symbolic ref is deleted. This is obviously what we expected...\n\n B. The command fails because \"alias\" is a symref. This would be\nunderstandable if we don't want to teach \"branch -d\" about symrefs.\nBut then, the error message should ideally explain which command we\nshould use to remove the symref.\n\n C. The \"master\" ref (BOTH loose and packed versions of it) is\ndeleted. This would be less helpful for us, but Git would at least be\ninternally consistent (in that the symref would be resolved, and the\ncommand would become \"git branch -d master\").\n\nObviously, I would advocate for option A. What say you?\n\n\nHave fun! :)\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"201318","messageId":"507D315E.8040101@lsrfire.ath.cx","threadId":"31820","inReplyTo":"CALKQrgfnvV+1XHjeSytj+LxkAabZJK3hewxH7WT0nkX-ewOKUA@mail.gmail.com","subject":"Re: BUG when trying to delete symbolic refs","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-10-16T10:05:18Z","receivedAt":"2012-10-16T10:05:18Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.10.2012 10:50, schrieb Johan Herland:\n> Basically, there is a \"master\" branch, and an \"alias\" symref to\n> \"master\". When we naively try to delete the symref with \"git branch -d\n> alias\", it ends up:\n> \n>   - NOT deleting the \"alias\" symref\n>   - DELETING the \"master\" loose ref\n>   - NOT deleting the \"master\" packed ref\n> \n> So, from the user perspective, \"git branch -d alias\" ends up resetting\n> \"master\" (and \"alias\") back to the last time we happened to run \"git\n> gc\". Needless to say, this is not quite what we had in mind...\n> \n> AFAICS, there may be three possible \"acceptable\" outcomes when we run\n> \"git branch -d alias\" in the above scenario:\n> \n>   A. The symbolic ref is deleted. This is obviously what we expected...\n\nBelow is a patch to do that.\n\n>   B. The command fails because \"alias\" is a symref. This would be\n> understandable if we don't want to teach \"branch -d\" about symrefs.\n> But then, the error message should ideally explain which command we\n> should use to remove the symref.\n\nRenaming of symrefs with branch -m is disallowed because it's more\ncomplicated than it looks at first; this was discussed here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/98825/focus=99206\n\nI can't imagine why deletion should be prohibited as well, though.\n\n>   C. The \"master\" ref (BOTH loose and packed versions of it) is\n> deleted. This would be less helpful for us, but Git would at least be\n> internally consistent (in that the symref would be resolved, and the\n> command would become \"git branch -d master\").\n\nAre there use cases for this behaviour?\n\nWhile I don't use symrefs, I'd somehow expect them to behave like\nsymbolic links on Unix do, where rm removes a link, not its target.\n\nBut I wonder why most delete_ref() calls in the code actually don't use\nthe flag REF_NODEREF, thus deleting symref targets instead of the\nsymrefs themselves.  I may be missing something important here.\n\n---\n builtin/branch.c  |  2 +-\n t/t3200-branch.sh | 10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex ffd2684..31af114 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -221,7 +221,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, 0)) {\n+\t\tif (delete_ref(name, sha1, REF_NODEREF)) {\n \t\t\terror(remote_branch\n \t\t\t      ? _(\"Error deleting remote branch '%s'\")\n \t\t\t      : _(\"Error deleting branch '%s'\"),\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 79c8d01..4b73406 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -262,6 +262,16 @@ 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 'deleting a symref' '\n+\tgit branch target &&\n+\tgit symbolic-ref refs/heads/symlink refs/heads/target &&\n+\n+\tgit branch -d symlink &&\n+\n+\ttest_path_is_file .git/refs/heads/target &&\n+\ttest_path_is_missing .git/refs/heads/symlink\n+'\n+\n test_expect_success 'renaming a symref is not allowed' \\\n '\n \tgit symbolic-ref refs/heads/master2 refs/heads/master &&\n-- \n1.7.12\n"},{"id":"201319","messageId":"507D3557.1090006@lsrfire.ath.cx","threadId":"31820","inReplyTo":"507D315E.8040101@lsrfire.ath.cx","subject":"[PATCH] refs: lock symref that is to be deleted, not its target","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-10-16T10:22:15Z","receivedAt":"2012-10-16T10:22:15Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"When delete_ref is called on a symref then it locks its target and then\neither deletes the target or the symref, depending on whether the flag\nREF_NODEREF was set in the parameter delopt.\n\nInstead, simply pass the flag to lock_ref_sha1_basic, which will then\neither lock the target or the symref, and delete the locked ref.\n\nThis reimplements part of eca35a25 (Fix git branch -m for symrefs.).\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\nIndependent patch, kind of related.\n\n refs.c | 18 +++++-------------\n 1 file changed, 5 insertions(+), 13 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex da74a2b..9d1685b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1753,26 +1753,18 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \tstruct ref_lock *lock;\n \tint err, i = 0, ret = 0, flag = 0;\n \n-\tlock = lock_ref_sha1_basic(refname, sha1, 0, &flag);\n+\tlock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);\n \tif (!lock)\n \t\treturn 1;\n \tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n \t\t/* loose */\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(\"%s\", refname);\n-\t\t}\n-\t\terr = unlink_or_warn(path);\n+\t\ti = strlen(lock->lk->filename) - 5; /* .lock */\n+\t\tlock->lk->filename[i] = 0;\n+\t\terr = unlink_or_warn(lock->lk->filename);\n \t\tif (err && errno != ENOENT)\n \t\t\tret = 1;\n \n-\t\tif (!(delopt & REF_NODEREF))\n-\t\t\tlock->lk->filename[i] = '.';\n+\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-- \n1.7.12\n"},{"id":"201349","messageId":"7vr4oytn4q.fsf@alter.siamese.dyndns.org","threadId":"31820","inReplyTo":"507D315E.8040101@lsrfire.ath.cx","subject":"Re: BUG when trying to delete symbolic refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-16T16:09:09Z","receivedAt":"2012-10-16T16:09:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> Am 15.10.2012 10:50, schrieb Johan Herland:\n>> Basically, there is a \"master\" branch, and an \"alias\" symref to\n>> \"master\". When we naively try to delete the symref with \"git branch -d\n>> alias\", it ends up:\n>> \n>>   - NOT deleting the \"alias\" symref\n>>   - DELETING the \"master\" loose ref\n>>   - NOT deleting the \"master\" packed ref\n>> \n>> So, from the user perspective, \"git branch -d alias\" ends up resetting\n>> \"master\" (and \"alias\") back to the last time we happened to run \"git\n>> gc\". Needless to say, this is not quite what we had in mind...\n>> \n>> AFAICS, there may be three possible \"acceptable\" outcomes when we run\n>> \"git branch -d alias\" in the above scenario:\n>> \n>>   A. The symbolic ref is deleted. This is obviously what we expected...\n>\n> Below is a patch to do that.\n>\n>>   B. The command fails because \"alias\" is a symref. This would be\n>> understandable if we don't want to teach \"branch -d\" about symrefs.\n>> But then, the error message should ideally explain which command we\n>> should use to remove the symref.\n>\n> Renaming of symrefs with branch -m is disallowed because it's more\n> complicated than it looks at first; this was discussed here:\n> http://thread.gmane.org/gmane.comp.version-control.git/98825/focus=99206\n\nThanks for a reminder.\n\n> I can't imagine why deletion should be prohibited as well, though.\n\nI am not sure if it is a good idea to let \"update-ref -d\" work on a\nsymref, with or without --no-deref.  There are cases where you want\nto remove the pointer (\"symbolic-ref -d\" is there for that), and\nthere are cases where you want to remove the underlying ref (but of\ncourse you can \"update-ref -d\" the underlying ref yourself).  If\n\"update-ref -d\" refused to work on a symref, we do not have to worry\nabout the confusion \"which one is removed---the pointer, or the\npointee?\"\n\n> But I wonder why most delete_ref() calls in the code actually don't use\n> the flag REF_NODEREF, thus deleting symref targets instead of the\n> symrefs themselves.  I may be missing something important here.\n\nI suspect that is primarily because using a symref to represent\nanything other than $GIT_DIR/HEAD and $GIT_DIR/refs/remotes/*/HEAD\nis outside the normally supported use case and in the \"may happen to\nwork\" territory.\n\nHaving said all that, I think your patch is going in the right\ndirection.  If somebody had a symbolic ref in refs/heads/, the\nremoval should remove it, not the pointee, which may not even\nexist.  Does \"branch -d sym\" work correctly with your patch when\nrefs/heads/sym is pointing at something that does not exist?\n\n> ---\n>  builtin/branch.c  |  2 +-\n>  t/t3200-branch.sh | 10 ++++++++++\n>  2 files changed, 11 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index ffd2684..31af114 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -221,7 +221,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, 0)) {\n> +\t\tif (delete_ref(name, sha1, REF_NODEREF)) {\n>  \t\t\terror(remote_branch\n>  \t\t\t      ? _(\"Error deleting remote branch '%s'\")\n>  \t\t\t      : _(\"Error deleting branch '%s'\"),\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index 79c8d01..4b73406 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -262,6 +262,16 @@ 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 'deleting a symref' '\n> +\tgit branch target &&\n> +\tgit symbolic-ref refs/heads/symlink refs/heads/target &&\n> +\n> +\tgit branch -d symlink &&\n> +\n> +\ttest_path_is_file .git/refs/heads/target &&\n> +\ttest_path_is_missing .git/refs/heads/symlink\n> +'\n> +\n>  test_expect_success 'renaming a symref is not allowed' \\\n>  '\n>  \tgit symbolic-ref refs/heads/master2 refs/heads/master &&\n"},{"id":"201503","messageId":"507FEF0B.1060309@lsrfire.ath.cx","threadId":"31820","inReplyTo":"7vr4oytn4q.fsf@alter.siamese.dyndns.org","subject":"Re: BUG when trying to delete symbolic refs","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-10-18T11:59:07Z","receivedAt":"2012-10-18T11:59:07Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.10.2012 18:09, schrieb Junio C Hamano:\n> Having said all that, I think your patch is going in the right\n> direction.  If somebody had a symbolic ref in refs/heads/, the\n> removal should remove it, not the pointee, which may not even\n> exist.  Does \"branch -d sym\" work correctly with your patch when\n> refs/heads/sym is pointing at something that does not exist?\n\nNo, it doesn't, neither with nor without the patch.  But we can make it\nwork, and also address a UI issue.  This series starts with two patches\nthat only move code around, then follows the patch you commented on, a\npatch addressing dangling symrefs and finally a change to the way\ndeleted symrefs are reported by git branch.\n\n  branch: factor out check_branch_commit()\n  branch: factor out delete_branch_config()\n  branch: delete symref branch, not its target\n  branch: skip commit checks when deleting symref branches\n  branch: show targets of deleted symrefs, not sha1s\n\n builtin/branch.c  | 75 ++++++++++++++++++++++++++++++++++++-------------------\n t/t3200-branch.sh | 19 ++++++++++++++\n 2 files changed, 68 insertions(+), 26 deletions(-)\n\n-- \n1.7.12\n"},{"id":"201504","messageId":"507FEFEB.4030406@lsrfire.ath.cx","threadId":"31820","inReplyTo":"507FEF0B.1060309@lsrfire.ath.cx","subject":"[PATCH 1/5] branch: factor out check_branch_commit()","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-10-18T12:02:51Z","receivedAt":"2012-10-18T12:02:51Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Move the code to perform checks on the tip commit of a branch\nto its own function.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n builtin/branch.c | 33 +++++++++++++++++++++------------\n 1 file changed, 21 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex ffd2684..852019e 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -154,10 +154,28 @@ static int branch_merged(int kind, const char *name,\n \treturn merged;\n }\n \n+static int check_branch_commit(const char *branchname, const char *refname,\n+\t\t\t       unsigned char *sha1, struct commit *head_rev,\n+\t\t\t       int kinds, int force)\n+{\n+\tstruct commit *rev = lookup_commit_reference(sha1);\n+\tif (!rev) {\n+\t\terror(_(\"Couldn't look up commit object for '%s'\"), refname);\n+\t\treturn -1;\n+\t}\n+\tif (!force && !branch_merged(kinds, branchname, rev, head_rev)) {\n+\t\terror(_(\"The branch '%s' is not fully merged.\\n\"\n+\t\t      \"If you are sure you want to delete it, \"\n+\t\t      \"run 'git branch -D %s'.\"), branchname, branchname);\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n+}\n+\n static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t   int quiet)\n {\n-\tstruct commit *rev, *head_rev = NULL;\n+\tstruct commit *head_rev = NULL;\n \tunsigned char sha1[20];\n \tchar *name = NULL;\n \tconst char *fmt;\n@@ -206,17 +224,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\tcontinue;\n \t\t}\n \n-\t\trev = lookup_commit_reference(sha1);\n-\t\tif (!rev) {\n-\t\t\terror(_(\"Couldn't look up commit object for '%s'\"), name);\n-\t\t\tret = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!force && !branch_merged(kinds, bname.buf, rev, head_rev)) {\n-\t\t\terror(_(\"The branch '%s' is not fully merged.\\n\"\n-\t\t\t      \"If you are sure you want to delete it, \"\n-\t\t\t      \"run 'git branch -D %s'.\"), bname.buf, bname.buf);\n+\t\tif (check_branch_commit(bname.buf, name, sha1, head_rev, kinds,\n+\t\t\t\t\tforce)) {\n \t\t\tret = 1;\n \t\t\tcontinue;\n \t\t}\n-- \n1.7.12\n"},{"id":"201505","messageId":"507FF038.6090202@lsrfire.ath.cx","threadId":"31820","inReplyTo":"507FEF0B.1060309@lsrfire.ath.cx","subject":"[PATCH 2/5] branch: factor out delete_branch_config()","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-10-18T12:04:08Z","receivedAt":"2012-10-18T12:04:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Provide a small helper function for deleting branch config sections.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n builtin/branch.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 852019e..97c7361 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -172,6 +172,15 @@ static int check_branch_commit(const char *branchname, const char *refname,\n \treturn 0;\n }\n \n+static void delete_branch_config(const char *branchname)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstrbuf_addf(&buf, \"branch.%s\", branchname);\n+\tif (git_config_rename_section(buf.buf, NULL) < 0)\n+\t\twarning(_(\"Update of config-file failed\"));\n+\tstrbuf_release(&buf);\n+}\n+\n static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t   int quiet)\n {\n@@ -237,17 +246,13 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t      bname.buf);\n \t\t\tret = 1;\n \t\t} else {\n-\t\t\tstruct strbuf buf = STRBUF_INIT;\n \t\t\tif (!quiet)\n \t\t\t\tprintf(remote_branch\n \t\t\t\t       ? _(\"Deleted remote branch %s (was %s).\\n\")\n \t\t\t\t       : _(\"Deleted branch %s (was %s).\\n\"),\n \t\t\t\t       bname.buf,\n \t\t\t\t       find_unique_abbrev(sha1, DEFAULT_ABBREV));\n-\t\t\tstrbuf_addf(&buf, \"branch.%s\", bname.buf);\n-\t\t\tif (git_config_rename_section(buf.buf, NULL) < 0)\n-\t\t\t\twarning(_(\"Update of config-file failed\"));\n-\t\t\tstrbuf_release(&buf);\n+\t\t\tdelete_branch_config(bname.buf);\n \t\t}\n \t}\n \n-- \n1.7.12\n"},{"id":"201506","messageId":"507FF07D.8040608@lsrfire.ath.cx","threadId":"31820","inReplyTo":"507FEF0B.1060309@lsrfire.ath.cx","subject":"[PATCH 3/5] branch: delete symref branch, not its target","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-10-18T12:05:17Z","receivedAt":"2012-10-18T12:05:17Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If a branch that is to be deleted happens to be a symref to another\nbranch, the current code removes the targeted branch instead of the\none it was called for.\n\nChange this surprising behaviour and delete the symref branch\ninstead.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n builtin/branch.c  |  2 +-\n t/t3200-branch.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 97c7361..5e1e5b4 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -239,7 +239,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, 0)) {\n+\t\tif (delete_ref(name, sha1, REF_NODEREF)) {\n \t\t\terror(remote_branch\n \t\t\t      ? _(\"Error deleting remote branch '%s'\")\n \t\t\t      : _(\"Error deleting branch '%s'\"),\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 79c8d01..ec5f70e 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -262,6 +262,17 @@ 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 'deleting a symref' '\n+\tgit branch target &&\n+\tgit symbolic-ref refs/heads/symref refs/heads/target &&\n+\tsha1=$(git rev-parse symref | cut -c 1-7) &&\n+\techo \"Deleted branch symref (was $sha1).\" >expect &&\n+\tgit branch -d symref >actual &&\n+\ttest_path_is_file .git/refs/heads/target &&\n+\ttest_path_is_missing .git/refs/heads/symref &&\n+\ttest_i18ncmp expect actual\n+'\n+\n test_expect_success 'renaming a symref is not allowed' \\\n '\n \tgit symbolic-ref refs/heads/master2 refs/heads/master &&\n-- \n1.7.12\n"},{"id":"201507","messageId":"507FF0EF.3070702@lsrfire.ath.cx","threadId":"31820","inReplyTo":"507FEF0B.1060309@lsrfire.ath.cx","subject":"[PATCH 4/5] branch: skip commit checks when deleting symref branches","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-10-18T12:07:11Z","receivedAt":"2012-10-18T12:07:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Before a branch is deleted, we check that it points to a valid\ncommit.  With -d we also check that the commit is a merged; this\ncheck is not done with -D.\n\nThe reason for that is that commits pointed to by branches should\nnever go missing; if they do then something broke and it's better\nto stop instead of adding to the mess.  And a non-merged commit\nmay contain changes that are worth preserving, so we require the\nstronger option -D instead of -d to get rid of them.\n\nIf a branch consists of a symref, these concerns don't apply.\nDeleting such a branch can't make a commit become unreferenced,\nso we don't need to check if it is merged, or even if it is\nactually a valid commit.  Skip them in that case.  This allows\nus to delete dangling symref branches.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n builtin/branch.c  | 10 ++++++++--\n t/t3200-branch.sh |  9 +++++++++\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 5e1e5b4..d87035a 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -214,6 +214,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n \t}\n \tfor (i = 0; i < argc; i++, strbuf_release(&bname)) {\n+\t\tconst char *target;\n+\t\tint flags = 0;\n+\n \t\tstrbuf_branchname(&bname, argv[i]);\n \t\tif (kinds == REF_LOCAL_BRANCH && !strcmp(head, bname.buf)) {\n \t\t\terror(_(\"Cannot delete the branch '%s' \"\n@@ -225,7 +228,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tfree(name);\n \n \t\tname = mkpathdup(fmt, bname.buf);\n-\t\tif (read_ref(name, sha1)) {\n+\t\ttarget = resolve_ref_unsafe(name, sha1, 0, &flags);\n+\t\tif (!target ||\n+\t\t    (!(flags & REF_ISSYMREF) && is_null_sha1(sha1))) {\n \t\t\terror(remote_branch\n \t\t\t      ? _(\"remote branch '%s' not found.\")\n \t\t\t      : _(\"branch '%s' not found.\"), bname.buf);\n@@ -233,7 +238,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (check_branch_commit(bname.buf, name, sha1, head_rev, kinds,\n+\t\tif (!(flags & REF_ISSYMREF) &&\n+\t\t    check_branch_commit(bname.buf, name, sha1, head_rev, kinds,\n \t\t\t\t\tforce)) {\n \t\t\tret = 1;\n \t\t\tcontinue;\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex ec5f70e..1323f6f 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -273,6 +273,15 @@ test_expect_success 'deleting a symref' '\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'deleting a dangling symref' '\n+\tgit symbolic-ref refs/heads/dangling-symref nowhere &&\n+\ttest_path_is_file .git/refs/heads/dangling-symref &&\n+\techo \"Deleted branch dangling-symref (was 0000000).\" >expect &&\n+\tgit branch -d dangling-symref >actual &&\n+\ttest_path_is_missing .git/refs/heads/dangling-symref &&\n+\ttest_i18ncmp expect actual\n+'\n+\n test_expect_success 'renaming a symref is not allowed' \\\n '\n \tgit symbolic-ref refs/heads/master2 refs/heads/master &&\n-- \n1.7.12\n"},{"id":"201508","messageId":"507FF123.2030709@lsrfire.ath.cx","threadId":"31820","inReplyTo":"507FEF0B.1060309@lsrfire.ath.cx","subject":"[PATCH 5/5] branch: show targets of deleted symrefs, not sha1s","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-10-18T12:08:03Z","receivedAt":"2012-10-18T12:08:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"git branch reports the abbreviated hash of the head commit of\na deleted branch to make it easier for a user to undo the\noperation.  For symref branches this doesn't help.  Print the\nsymref target instead for them.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n builtin/branch.c  | 19 +++++++++++--------\n t/t3200-branch.sh |  5 ++---\n 2 files changed, 13 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex d87035a..1ec9c02 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -251,15 +251,18 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t      : _(\"Error deleting branch '%s'\"),\n \t\t\t      bname.buf);\n \t\t\tret = 1;\n-\t\t} else {\n-\t\t\tif (!quiet)\n-\t\t\t\tprintf(remote_branch\n-\t\t\t\t       ? _(\"Deleted remote branch %s (was %s).\\n\")\n-\t\t\t\t       : _(\"Deleted branch %s (was %s).\\n\"),\n-\t\t\t\t       bname.buf,\n-\t\t\t\t       find_unique_abbrev(sha1, DEFAULT_ABBREV));\n-\t\t\tdelete_branch_config(bname.buf);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!quiet) {\n+\t\t\tprintf(remote_branch\n+\t\t\t       ? _(\"Deleted remote branch %s (was %s).\\n\")\n+\t\t\t       : _(\"Deleted branch %s (was %s).\\n\"),\n+\t\t\t       bname.buf,\n+\t\t\t       (flags & REF_ISSYMREF)\n+\t\t\t       ? target\n+\t\t\t       : find_unique_abbrev(sha1, DEFAULT_ABBREV));\n \t\t}\n+\t\tdelete_branch_config(bname.buf);\n \t}\n \n \tfree(name);\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 1323f6f..80e6be3 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -265,8 +265,7 @@ test_expect_success 'config information was renamed, too' \\\n test_expect_success 'deleting a symref' '\n \tgit branch target &&\n \tgit symbolic-ref refs/heads/symref refs/heads/target &&\n-\tsha1=$(git rev-parse symref | cut -c 1-7) &&\n-\techo \"Deleted branch symref (was $sha1).\" >expect &&\n+\techo \"Deleted branch symref (was refs/heads/target).\" >expect &&\n \tgit branch -d symref >actual &&\n \ttest_path_is_file .git/refs/heads/target &&\n \ttest_path_is_missing .git/refs/heads/symref &&\n@@ -276,7 +275,7 @@ test_expect_success 'deleting a symref' '\n test_expect_success 'deleting a dangling symref' '\n \tgit symbolic-ref refs/heads/dangling-symref nowhere &&\n \ttest_path_is_file .git/refs/heads/dangling-symref &&\n-\techo \"Deleted branch dangling-symref (was 0000000).\" >expect &&\n+\techo \"Deleted branch dangling-symref (was nowhere).\" >expect &&\n \tgit branch -d dangling-symref >actual &&\n \ttest_path_is_missing .git/refs/heads/dangling-symref &&\n \ttest_i18ncmp expect actual\n-- \n1.7.12\n"},{"id":"201528","messageId":"7vpq4flb1c.fsf@alter.siamese.dyndns.org","threadId":"31820","inReplyTo":"507FF0EF.3070702@lsrfire.ath.cx","subject":"Re: [PATCH 4/5] branch: skip commit checks when deleting symref branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-18T21:34:23Z","receivedAt":"2012-10-18T21:34:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> Before a branch is deleted, we check that it points to a valid\n> commit.  With -d we also check that the commit is a merged; this\n> check is not done with -D.\n>\n> The reason for that is that commits pointed to by branches should\n> never go missing; if they do then something broke and it's better\n> to stop instead of adding to the mess.  And a non-merged commit\n> may contain changes that are worth preserving, so we require the\n> stronger option -D instead of -d to get rid of them.\n>\n> If a branch consists of a symref, these concerns don't apply.\n> Deleting such a branch can't make a commit become unreferenced,\n> so we don't need to check if it is merged, or even if it is\n> actually a valid commit.  Skip them in that case.  This allows\n> us to delete dangling symref branches.\n\nPurist in me tells me that we should be using \"symbolic-ref -d\" to\ncorrect from such a misconfiguration, but ignoring that, the above\nlogic makes perfect sense to me, especially together with the next\nchange to tell what branch the symref was pointing at, which can be\nused by the user when the user removes such a symbolic ref by\nmistake.\n\nThanks.  Will queue all five.\n"},{"id":"201623","messageId":"1350816032-16312-1-git-send-email-johan@herland.net","threadId":"31820","inReplyTo":"7vpq4flb1c.fsf@alter.siamese.dyndns.org","subject":"[PATCH 0/2] Fix remaining issue when deleting symrefs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2012-10-21T10:40:30Z","receivedAt":"2012-10-21T10:40:30Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"The following two patches are based on rs/branch-del-symref, and fixes\nthe remaining failure to delete a packed ref through a symref.\n\nThe first patch demonstrates the bug with a testcase, and the second\npatch fixes the bug.\n\nFeel free to squash the two patches into one, if you prefer to keep\nboth the testcase and subsequent fix in a single commit.\n\n\nHave fun! :)\n\n...Johan\n\nJohan Herland (2):\n  t1400-update-ref: Add test verifying bug with symrefs in delete_ref()\n  Fix failure to delete a packed ref through a symref\n\n refs.c                |  2 +-\n t/t1400-update-ref.sh | 18 ++++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\n--\n1.7.12.1.609.g5cd6968\n"},{"id":"201625","messageId":"1350816032-16312-2-git-send-email-johan@herland.net","threadId":"31820","inReplyTo":"1350816032-16312-1-git-send-email-johan@herland.net","subject":"[PATCH 1/2] t1400-update-ref: Add test verifying bug with symrefs in delete_ref()","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2012-10-21T10:40:31Z","receivedAt":"2012-10-21T10:40:31Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"When deleting a ref through a symref (e.g. using 'git update-ref -d HEAD'\nto delete refs/heads/master), we currently fail to remove the packed\nversion of that ref. This testcase demonstrates the bug.\n\nSigned-off-by: Johan Herland <johan@herland.net>\n---\n t/t1400-update-ref.sh | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 4fd83a6..f7ec203 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -74,6 +74,24 @@ test_expect_success \"delete $m (by HEAD)\" '\n '\n rm -f .git/$m\n \n+test_expect_success \\\n+\t\"create $m (by HEAD)\" \\\n+\t\"git update-ref HEAD $A &&\n+\t test $A\"' = $(cat .git/'\"$m\"')'\n+test_expect_success \\\n+\t\"pack refs\" \\\n+\t\"git pack-refs --all\"\n+test_expect_success \\\n+\t\"move $m (by HEAD)\" \\\n+\t\"git update-ref HEAD $B $A &&\n+\t test $B\"' = $(cat .git/'\"$m\"')'\n+test_expect_failure \"delete $m (by HEAD) should remove both packed and loose $m\" '\n+\tgit update-ref -d HEAD $B &&\n+\t! grep \"$m\" .git/packed-refs &&\n+\t! test -f .git/$m\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-- \n1.7.12.1.609.g5cd6968\n"},{"id":"201624","messageId":"1350816032-16312-3-git-send-email-johan@herland.net","threadId":"31820","inReplyTo":"1350816032-16312-1-git-send-email-johan@herland.net","subject":"[PATCH 2/2] Fix failure to delete a packed ref through a symref","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2012-10-21T10:40:32Z","receivedAt":"2012-10-21T10:40:32Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"When deleting a ref through a symref (e.g. using 'git update-ref -d HEAD'\nto delete refs/heads/master), we would remove the loose ref, but a packed\nversion of the same ref would remain, the end result being that instead of\ndeleting refs/heads/master we would appear to reset it to its state as of\nthe last repack.\n\nThis patch fixes the issue, by making sure we pass the correct ref name\nwhen invoking repack_without_ref() from within delete_ref().\n\nSigned-off-by: Johan Herland <johan@herland.net>\n---\n refs.c                | 2 +-\n t/t1400-update-ref.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 726c53c..6cec1c8 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1779,7 +1779,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \t * packed one.  Also, if it was not loose we need to repack\n \t * without it.\n \t */\n-\tret |= repack_without_ref(refname);\n+\tret |= repack_without_ref(lock->ref_name);\n \n \tunlink_or_warn(git_path(\"logs/%s\", lock->ref_name));\n \tinvalidate_ref_cache(NULL);\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex f7ec203..e415ee0 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -85,7 +85,7 @@ test_expect_success \\\n \t\"move $m (by HEAD)\" \\\n \t\"git update-ref HEAD $B $A &&\n \t test $B\"' = $(cat .git/'\"$m\"')'\n-test_expect_failure \"delete $m (by HEAD) should remove both packed and loose $m\" '\n+test_expect_success \"delete $m (by HEAD) should remove both packed and loose $m\" '\n \tgit update-ref -d HEAD $B &&\n \t! grep \"$m\" .git/packed-refs &&\n \t! test -f .git/$m\n-- \n1.7.12.1.609.g5cd6968\n"},{"id":"201633","messageId":"50843505.3040509@lsrfire.ath.cx","threadId":"31820","inReplyTo":"1350816032-16312-3-git-send-email-johan@herland.net","subject":"Re: [PATCH 2/2] Fix failure to delete a packed ref through a symref","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-10-21T17:46:45Z","receivedAt":"2012-10-21T17:46:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.10.2012 12:40, schrieb Johan Herland:\n> When deleting a ref through a symref (e.g. using 'git update-ref -d HEAD'\n> to delete refs/heads/master), we would remove the loose ref, but a packed\n> version of the same ref would remain, the end result being that instead of\n> deleting refs/heads/master we would appear to reset it to its state as of\n> the last repack.\n>\n> This patch fixes the issue, by making sure we pass the correct ref name\n> when invoking repack_without_ref() from within delete_ref().\n>\n> Signed-off-by: Johan Herland <johan@herland.net>\n> ---\n>   refs.c                | 2 +-\n>   t/t1400-update-ref.sh | 2 +-\n>   2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 726c53c..6cec1c8 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1779,7 +1779,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n>   \t * packed one.  Also, if it was not loose we need to repack\n>   \t * without it.\n>   \t */\n> -\tret |= repack_without_ref(refname);\n> +\tret |= repack_without_ref(lock->ref_name);\n>\n>   \tunlink_or_warn(git_path(\"logs/%s\", lock->ref_name));\n>   \tinvalidate_ref_cache(NULL);\n\nLooks reasonable.\n\nFWIW, this is independent of 547d058f in next (refs: lock symref that is \nto be deleted, not its target), which only affects behaviour when \nREF_NODEREF is set, while this one here only makes a difference with \nsymrefs and REF_NODEREF unset.\n\nRené\n"},{"id":"201643","messageId":"7vsj97fxrm.fsf@alter.siamese.dyndns.org","threadId":"31820","inReplyTo":"1350816032-16312-3-git-send-email-johan@herland.net","subject":"Re: [PATCH 2/2] Fix failure to delete a packed ref through a symref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-21T19:09:01Z","receivedAt":"2012-10-21T19:09:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland <johan@herland.net> writes:\n\n> When deleting a ref through a symref (e.g. using 'git update-ref -d HEAD'\n> to delete refs/heads/master), we would remove the loose ref, but a packed\n> version of the same ref would remain, the end result being that instead of\n> deleting refs/heads/master we would appear to reset it to its state as of\n> the last repack.\n>\n> This patch fixes the issue, by making sure we pass the correct ref name\n> when invoking repack_without_ref() from within delete_ref().\n>\n> Signed-off-by: Johan Herland <johan@herland.net>\n> ---\n\nThanks.  Will queue.\n\n>  refs.c                | 2 +-\n>  t/t1400-update-ref.sh | 2 +-\n>  2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 726c53c..6cec1c8 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1779,7 +1779,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n>  \t * packed one.  Also, if it was not loose we need to repack\n>  \t * without it.\n>  \t */\n> -\tret |= repack_without_ref(refname);\n> +\tret |= repack_without_ref(lock->ref_name);\n>  \n>  \tunlink_or_warn(git_path(\"logs/%s\", lock->ref_name));\n>  \tinvalidate_ref_cache(NULL);\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index f7ec203..e415ee0 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -85,7 +85,7 @@ test_expect_success \\\n>  \t\"move $m (by HEAD)\" \\\n>  \t\"git update-ref HEAD $B $A &&\n>  \t test $B\"' = $(cat .git/'\"$m\"')'\n> -test_expect_failure \"delete $m (by HEAD) should remove both packed and loose $m\" '\n> +test_expect_success \"delete $m (by HEAD) should remove both packed and loose $m\" '\n>  \tgit update-ref -d HEAD $B &&\n>  \t! grep \"$m\" .git/packed-refs &&\n>  \t! test -f .git/$m\n"}]}