{"thread":{"id":"31834","subject":"[PATCH 1/5] t1400-update-ref: Add test verifying bug with symrefs in delete_ref()","startedAt":"2012-10-16T13:44:49Z","lastAt":"2012-10-16T16:54:14Z","messageCount":7,"participants":["Johan Herland","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"201337","messageId":"1350395094-11404-1-git-send-email-johan@herland.net","threadId":"31834","inReplyTo":"CAPc5daUws-MfzC9imkytTrLaHyyywE4_OX1jAUVPCTK2WyUF=w@mail.gmail.com","subject":"[PATCH 0/5] Fixing problem with deleting symrefs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2012-10-16T13:44:49Z","receivedAt":"2012-10-16T13:44:49Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"I see that Rene Scharfe has also worked on the same issue, while I was\npreparing these patches...\n\nOn Mon, Oct 15, 2012 at 11:29 AM, Junio C Hamano <jch2355@gmail.com> wrote:\n> Even though update-ref deferences a symref when it updates one to point at a\n> new object, I personally don't think update-ref -d that derefs makes any\n> sense. I'd rather see it error out when given a symref, with or without\n> --no-deref option.\n\nI'm not sure. We have multiple testcases that directly test deleting a ref\nthrough a symref (e.g. t1400), so supporting this seems like a concious\ndecision. Erroring out when given a symref will break the following\ntestcases:\n - t1400 (git update-ref -d)\n - t3310 & t3311 (git notes merge --abort is broken)\n - t5505 (git remote set-head --delete and renaming a remote is broken)\n\n> But even if it did, removing a ref pointed by a symref should really remove\n> it, and forgetting to remove a leftover entry in packed-ref has no excuse\n> bug.\n>\n> I'd say what you observed is a double bug.\n\nPatch #1 - #2 fixes the 2nd bug (removing through a symref should remove\nboth loose and packed versions of the ref).\n\nPatch #3 fixes an associated problem where deleting a symref would remove\nthe dereferenced ref's reflog instead of the symref's reflog.\n\nPatch #4 - #5 introduces solution A presented in my previous mail (i.e.\n'git branch -d' never dereferences symrefs).\n\nJohan Herland (5):\n  t1400-update-ref: Add test verifying bug with symrefs in delete_ref()\n  delete_ref(): Fix deletion of refs through symbolic refs\n  delete_ref(): Remove the correct reflog when deleting symrefs\n  Add tests for using symbolic refs as branch name aliases.\n  branch -d: Fix failure to remove branch aliases (symrefs)\n\n builtin/branch.c                        |  2 +-\n refs.c                                  | 35 +++++++++++-----------\n t/t1400-update-ref.sh                   | 18 +++++++++++\n t/t3220-symbolic-ref-as-branch-alias.sh | 53 +++++++++++++++++++++++++++++++++\n 4 files changed, 90 insertions(+), 18 deletions(-)\n create mode 100755 t/t3220-symbolic-ref-as-branch-alias.sh\n\n--\n1.7.12.1.609.g5cd6968\n"},{"id":"201333","messageId":"1350395094-11404-2-git-send-email-johan@herland.net","threadId":"31834","inReplyTo":"1350395094-11404-1-git-send-email-johan@herland.net","subject":"[PATCH 1/5] t1400-update-ref: Add test verifying bug with symrefs in delete_ref()","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2012-10-16T13:44:50Z","receivedAt":"2012-10-16T13:44:50Z","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":"201334","messageId":"1350395094-11404-3-git-send-email-johan@herland.net","threadId":"31834","inReplyTo":"1350395094-11404-1-git-send-email-johan@herland.net","subject":"[PATCH 2/5] delete_ref(): Fix deletion of refs through symbolic refs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2012-10-16T13:44:51Z","receivedAt":"2012-10-16T13:44:51Z","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), the packed version of that ref would not\nbe deleted, because delete_ref() would pass the symref name (as opposed\nto the dereferenced ref name) to repack_without_ref().\n\nThis patch revamps the logic in delete_ref() to make it easier to read,\nand to clarify how it operates when given a symref.\n\nSigned-off-by: Johan Herland <johan@herland.net>\n---\n refs.c                | 33 +++++++++++++++++----------------\n t/t1400-update-ref.sh |  2 +-\n 2 files changed, 18 insertions(+), 17 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex da74a2b..df4fe20 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1751,34 +1751,35 @@ static int repack_without_ref(const char *refname)\n int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n {\n \tstruct ref_lock *lock;\n-\tint err, i = 0, ret = 0, flag = 0;\n+\tint err, delete_symref, 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) || 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/* The following variables are at play here:\n+\t *  - refname may be a symref (in this case lock->orig_ref_name holds\n+\t *    the symref name, and lock->ref_name holds the dereferenced name)\n+\t *  - The dereferenced ref name (lock->ref_name) may be a loose ref, a\n+\t *    packed ref, or both.\n+\t *  - If REF_NODEREF is enabled - and refname is a symref, we should\n+\t *    delete the symref; otherwise delete the dereferenced ref.\n+\t */\n+\tdelete_symref = (flag & REF_ISSYMREF && delopt & REF_NODEREF);\n+\trefname = delete_symref ? lock->orig_ref_name : lock->ref_name;\n+\n+\tif (!(flag & REF_ISPACKED) || delete_symref) {\n+\t\t/* loose */\n+\t\terr = unlink_or_warn(git_path(\"%s\", refname));\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}\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 \t * without it.\n \t */\n-\tret |= repack_without_ref(refname);\n+\tif (!delete_symref)\n+\t\tret |= repack_without_ref(refname);\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":"201338","messageId":"1350395094-11404-4-git-send-email-johan@herland.net","threadId":"31834","inReplyTo":"1350395094-11404-1-git-send-email-johan@herland.net","subject":"[PATCH 3/5] delete_ref(): Remove the correct reflog when deleting symrefs","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2012-10-16T13:44:52Z","receivedAt":"2012-10-16T13:44:52Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"When deleting a symref (e.g. HEAD), we would incorrectly remove the\nreflog of the dereferenced ref (e.g. .git/logs/refs/heads/master),\ninsted of the symref's reflog (e.g. .git/logs/HEAD).\n\nThis patch ensures that we remove the reflog that corresponds to the\nremoved (sym)ref.\n\nSigned-off-by: Johan Herland <johan@herland.net>\n---\n refs.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex df4fe20..f2508bf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1781,7 +1781,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \tif (!delete_symref)\n \t\tret |= repack_without_ref(refname);\n \n-\tunlink_or_warn(git_path(\"logs/%s\", lock->ref_name));\n+\tunlink_or_warn(git_path(\"logs/%s\", refname));\n \tinvalidate_ref_cache(NULL);\n \tunlock_ref(lock);\n \treturn ret;\n-- \n1.7.12.1.609.g5cd6968\n"},{"id":"201335","messageId":"1350395094-11404-5-git-send-email-johan@herland.net","threadId":"31834","inReplyTo":"1350395094-11404-1-git-send-email-johan@herland.net","subject":"[PATCH 4/5] Add tests for using symbolic refs as branch name aliases.","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2012-10-16T13:44:53Z","receivedAt":"2012-10-16T13:44:53Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"A branch name alias is an alternative name for a branch, that is in most\nrespects equivalent to using the proper branch name. It is implemented as\na symbolic ref from the alias to the proper branch name.\n\nCurrently branch aliases work well up to the point where you try to delete\nthem (with \"git branch -d\"), at which point they incorrectly delete the\nproper branch name instead of the alias. This is reflected in these tests,\nand will be fixed in a subsequent patch.\n\nSigned-off-by: Johan Herland <johan@herland.net>\n---\n t/t3220-symbolic-ref-as-branch-alias.sh | 53 +++++++++++++++++++++++++++++++++\n 1 file changed, 53 insertions(+)\n create mode 100755 t/t3220-symbolic-ref-as-branch-alias.sh\n\ndiff --git a/t/t3220-symbolic-ref-as-branch-alias.sh b/t/t3220-symbolic-ref-as-branch-alias.sh\nnew file mode 100755\nindex 0000000..39f3a33\n--- /dev/null\n+++ b/t/t3220-symbolic-ref-as-branch-alias.sh\n@@ -0,0 +1,53 @@\n+#!/bin/sh\n+\n+test_description='Using a symbolic ref as a branch name alias\n+\n+This test uses refs/heads/alias as a symbolic ref to refs/heads/master, and\n+verifies that it works as a branch name alias, namely:\n+ - commits on \"master\" are automatically reflected on \"alias\"\n+ - creating or deleting \"alias\" does not affect \"master\"\n+ - the \"alias\" is not broken by \"git gc\"\n+'\n+. ./test-lib.sh\n+\n+test_expect_success 'prepare a trivial repository' '\n+\techo Hello > A &&\n+\tgit add A &&\n+\tgit commit -m \"Hello\" &&\n+\tgit rev-parse --verify HEAD > expect\n+'\n+\n+test_expect_success 'create symref: refs/heads/alias -> refs/heads/master' '\n+\tgit symbolic-ref refs/heads/alias refs/heads/master &&\n+\tgit rev-parse --verify master > master &&\n+\tgit rev-parse --verify alias > alias &&\n+\ttest_cmp expect master &&\n+\ttest_cmp expect alias\n+'\n+\n+test_expect_success '\"git gc\" does not change \"alias\" symref' '\n+\tgit gc &&\n+\tgit rev-parse --verify master > master &&\n+\tgit rev-parse --verify alias > alias &&\n+\ttest_cmp expect master &&\n+\ttest_cmp expect alias\n+'\n+\n+test_expect_success 'commits are reflected through \"alias\" symref' '\n+\techo World >> A &&\n+\tgit commit -am \"A\" &&\n+\tgit rev-parse --verify HEAD > expect &&\n+\tgit rev-parse --verify master > master &&\n+\tgit rev-parse --verify alias > alias &&\n+\ttest_cmp expect master &&\n+\ttest_cmp expect alias\n+'\n+\n+test_expect_failure '\"branch -d\" deletes the \"alias\" symref' '\n+\tgit branch -d alias &&\n+\tgit rev-parse --verify master > master &&\n+\ttest_must_fail git rev-parse --verify alias &&\n+\ttest_cmp expect master\n+'\n+\n+test_done\n-- \n1.7.12.1.609.g5cd6968\n"},{"id":"201336","messageId":"1350395094-11404-6-git-send-email-johan@herland.net","threadId":"31834","inReplyTo":"1350395094-11404-1-git-send-email-johan@herland.net","subject":"[PATCH 5/5] branch -d: Fix failure to remove branch aliases (symrefs)","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2012-10-16T13:44:54Z","receivedAt":"2012-10-16T13:44:54Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"With refs/heads/alias set up as a symref to refs/heads/master,\n'git branch -d alias' should remove refs/heads/alias and not\nrefs/heads/master.\n\nSigned-off-by: Johan Herland <johan@herland.net>\n---\n builtin/branch.c                        | 2 +-\n t/t3220-symbolic-ref-as-branch-alias.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\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/t3220-symbolic-ref-as-branch-alias.sh b/t/t3220-symbolic-ref-as-branch-alias.sh\nindex 39f3a33..8ebec7a 100755\n--- a/t/t3220-symbolic-ref-as-branch-alias.sh\n+++ b/t/t3220-symbolic-ref-as-branch-alias.sh\n@@ -43,7 +43,7 @@ test_expect_success 'commits are reflected through \"alias\" symref' '\n \ttest_cmp expect alias\n '\n \n-test_expect_failure '\"branch -d\" deletes the \"alias\" symref' '\n+test_expect_success '\"branch -d\" deletes the \"alias\" symref' '\n \tgit branch -d alias &&\n \tgit rev-parse --verify master > master &&\n \ttest_must_fail git rev-parse --verify alias &&\n-- \n1.7.12.1.609.g5cd6968\n"},{"id":"201356","messageId":"7v4nlutl1l.fsf@alter.siamese.dyndns.org","threadId":"31834","inReplyTo":"1350395094-11404-1-git-send-email-johan@herland.net","subject":"Re: [PATCH 0/5] Fixing problem with deleting symrefs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-16T16:54:14Z","receivedAt":"2012-10-16T16:54:14Z","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> I see that Rene Scharfe has also worked on the same issue, while I was\n> preparing these patches...\n>\n> On Mon, Oct 15, 2012 at 11:29 AM, Junio C Hamano <jch2355@gmail.com> wrote:\n>> Even though update-ref deferences a symref when it updates one to point at a\n>> new object, I personally don't think update-ref -d that derefs makes any\n>> sense. I'd rather see it error out when given a symref, with or without\n>> --no-deref option.\n>\n> I'm not sure. We have multiple testcases that directly test deleting a ref\n> through a symref (e.g. t1400), so supporting this seems like a concious\n> decision. Erroring out when given a symref will break the following\n> testcases:\n>  - t1400 (git update-ref -d)\n>  - t3310 & t3311 (git notes merge --abort is broken)\n>  - t5505 (git remote set-head --delete and renaming a remote is broken)\n\n\"Concious\" does not necessarily mean \"Sane\", though.  But this is\nwater under the bridge.  Too many people must have started relying\non this crazy \"feature\" since mid 2008, and removing it would break\nthem.\n\n - \"update-ref -d --no-deref SYMREF\", even though it is a synonym\n   for \"symbolic-ref -d SYMREF\", makes sense.  Removing it would\n   only buy us breakage.\n\n - \"update-ref -d --no-deref SYMREF OLDSHA1\" does not make *ANY*\n   sense as a request, but again it would not hurt to keep it.\n\n - \"update-ref -d --deref SYMREF [OLDSHA1]\" is questionable.  What\n   is the use case?  What is the next step after doing such an\n   operation, now you have SYMREF that is dangling?\n\n>> But even if it did, removing a ref pointed by a symref should really remove\n>> it, and forgetting to remove a leftover entry in packed-ref has no excuse\n>> bug.\n>>\n>> I'd say what you observed is a double bug.\n>\n> Patch #1 - #2 fixes the 2nd bug (removing through a symref should remove\n> both loose and packed versions of the ref).\n\nOK.  That is surely needed.\n"}]}