{"thread":{"id":"39767","subject":"[PATCH] filter-branch: handle deletion of annotated tags","startedAt":"2015-07-02T12:50:48Z","lastAt":"2015-07-02T12:55:02Z","messageCount":2,"participants":["Clemens Buchacher"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"265394","messageId":"20150702125048.GA15759@musxeris015.imu.intel.com","threadId":"39767","inReplyTo":null,"subject":"[PATCH] filter-branch: handle deletion of annotated tags","fromName":"Clemens Buchacher","fromEmail":"clemens.buchacher@intel.com","sentAt":"2015-07-02T12:50:48Z","receivedAt":"2015-07-02T12:50:48Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"If filter-branch removes a commit which an annotated tag points to,\nand that tag is in the list of refs to be rewritten, we die with an\nerror like this:\n\n    error: Ref refs/tags/a1t is at 699cce2774f79a0830d8c5f631deca12d4b1ee8c but expected ba247450492030b03e3d2a9d5fef7ef67519483e\n    Could not delete refs/tags/a1t\n\nIn order to update refs, we first peel the ref until we find a\ncommit sha1. We then pass the commit sha1 to update-ref as the\n<oldvalue> parameter. Please consider the following scenarios:\n\n a) The ref points to a commit object directly. In this case,\n    update-ref will find that the current value of the ref still\n    matches oldvalue, and succeeds. This check is redundant, since\n    we only just queried the current value.\n\n b) The ref points to a tag object. In this case, update-ref will\n    error out, since the commit sha1 cannot match the current value\n    of the ref. If the commit has been removed, or rewritten into\n    multiple commits, we simply die. If the commit has been\n    rewritten, we output a warning message saying that to rewrite\n    tags one should use --tag-name-filter, and then we continue. If\n    --tag-name-filter is active, the tag will later be rewritten.\n\nThere seems to be no added value in passing the <oldvalue>\nparameter. So remove it.\n\nThis fixes deletion of tag objects. We also do not die any more if\na tag object points to a commit which has been rewritten into\nmultiple commits. However, we probably will die later in the\n--tag-name-filter code, because it does not seem to handle this\ncase.\n\nThis is a minimalist fix which leaves the following issues open:\n\n o In the absence of --tag-name-filter, we rewrite lightweight tags, but\n   not annotated tags, which is not intuitive. We do output a warning,\n   though:\n\n   $ git filter-branch --msg-filter \"cat && echo hi\" -- --all\n   [...]\n   WARNING: You said to rewrite tagged commits, but not the corresponding tag.\n   WARNING: Perhaps use '--tag-name-filter cat' to rewrite the tag.\n\n o Annotated tags are backed up as lightweight tags.\n\n o Annotated tags are backed up even in the absence of\n   --tag-name-filter. But in this case backup is not needed because\n   they are not rewritten.\n\nThese issues could be solved by moving the tag rewriting logic from\ntag-name-filter to the regular ref updating code, and\ntag-name-filter should deal only with renaming tags. However, this\nwould change behavior. Currently, the following command would\nrewrite tags:\n\n    git filter-branch --msg-filter \"cat && echo hi\" \\\n        --tag-name-filter cat -- --branches\n\nWith the suggested behavior, tags would be rewritten only if we\ninclude them in the rev-list options:\n\n    git filter-branch --msg-filter=\"cat && echo hi\" -- --all\n\nI am not sure if we can afford to change behavior like that.\n\nCc: Thomas Rast <trast@student.ethz.ch>\nCc: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Clemens Buchacher <clemens.buchacher@intel.com>\nReviewed-by: Jorge Nunes <jorge.nunes@intel.com>\n---\n git-filter-branch.sh     | 20 +++++++++-----------\n t/t7003-filter-branch.sh | 21 +++++++++++++++++++++\n 2 files changed, 30 insertions(+), 11 deletions(-)\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex 5b3f63d..7ca1d99 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -399,21 +399,19 @@ do\n \tcase \"$rewritten\" in\n \t'')\n \t\techo \"Ref '$ref' was deleted\"\n-\t\tgit update-ref -m \"filter-branch: delete\" -d \"$ref\" $sha1 ||\n+\t\tgit update-ref -m \"filter-branch: delete\" -d \"$ref\" ||\n \t\t\tdie \"Could not delete $ref\"\n \t;;\n \t$_x40)\n \t\techo \"Ref '$ref' was rewritten\"\n-\t\tif ! git update-ref -m \"filter-branch: rewrite\" \\\n-\t\t\t\t\t\"$ref\" $rewritten $sha1 2>/dev/null; then\n-\t\t\tif test $(git cat-file -t \"$ref\") = tag; then\n-\t\t\t\tif test -z \"$filter_tag_name\"; then\n-\t\t\t\t\twarn \"WARNING: You said to rewrite tagged commits, but not the corresponding tag.\"\n-\t\t\t\t\twarn \"WARNING: Perhaps use '--tag-name-filter cat' to rewrite the tag.\"\n-\t\t\t\tfi\n-\t\t\telse\n-\t\t\t\tdie \"Could not rewrite $ref\"\n+\t\tif test $(git cat-file -t \"$ref\") = tag; then\n+\t\t\tif test -z \"$filter_tag_name\"; then\n+\t\t\t\twarn \"WARNING: You said to rewrite tagged commits, but not the corresponding tag.\"\n+\t\t\t\twarn \"WARNING: Perhaps use '--tag-name-filter cat' to rewrite the tag.\"\n \t\t\tfi\n+\t\telse\n+\t\t\tgit update-ref -m \"filter-branch: rewrite\" \"$ref\" $rewritten ||\n+\t\t\t\tdie \"Could not rewrite $ref\"\n \t\tfi\n \t;;\n \t*)\n@@ -423,7 +421,7 @@ do\n \t\twarn \"WARNING: Ref '$ref' points to the first one now.\"\n \t\trewritten=$(echo \"$rewritten\" | head -n 1)\n \t\tgit update-ref -m \"filter-branch: rewrite to first\" \\\n-\t\t\t\t\"$ref\" $rewritten $sha1 ||\n+\t\t\t\t\"$ref\" $rewritten ||\n \t\t\tdie \"Could not rewrite $ref\"\n \t;;\n \tesac\ndiff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh\nindex 855afda..6a34527 100755\n--- a/t/t7003-filter-branch.sh\n+++ b/t/t7003-filter-branch.sh\n@@ -261,6 +261,7 @@ test_expect_success 'Subdirectory filter with disappearing trees' '\n '\n \n test_expect_success 'Tag name filtering retains tag message' '\n+\tgit update-ref -d refs/tags/T &&\n \tgit tag -m atag T &&\n \tgit cat-file tag T > expect &&\n \tgit filter-branch -f --tag-name-filter cat &&\n@@ -268,6 +269,26 @@ test_expect_success 'Tag name filtering retains tag message' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success \"Rewrite commit referenced by annotated tag\" '\n+\tgit update-ref -d refs/tags/T &&\n+\tgit tag -a -m atag T &&\n+\tgit rev-parse refs/tags/T^0 >old_commit &&\n+\tgit filter-branch -f --msg-filter \"cat && echo foo\" --tag-name-filter cat refs/tags/T &&\n+\techo tag >type.expect &&\n+\tgit cat-file -t refs/tags/T >type.actual &&\n+\ttest_cmp type.expect type.actual &&\n+\tgit rev-parse refs/tags/T^0 >new_commit &&\n+\ttest_must_fail test_cmp old_commit new_commit\n+'\n+\n+test_expect_success \"Remove all commits\" '\n+\tgit branch removed-branch &&\n+\tgit tag -a -m atag removed-tag &&\n+\tgit filter-branch -f --commit-filter \"skip_commit \\\"\\$@\\\"\" removed-branch removed-tag &&\n+\ttest_must_fail git rev-parse refs/heads/removed-branch &&\n+\ttest_must_fail git rev-parse refs/tags/removed-tag\n+'\n+\n faux_gpg_tag='object XXXXXX\n type commit\n tag S\n-- \n1.9.4\n"},{"id":"265395","messageId":"20150702125502.GA20534@musxeris015.imu.intel.com","threadId":"39767","inReplyTo":"20150702125048.GA15759@musxeris015.imu.intel.com","subject":"Re: [PATCH] filter-branch: handle deletion of annotated tags","fromName":"Clemens Buchacher","fromEmail":"clemens.buchacher@intel.com","sentAt":"2015-07-02T12:55:02Z","receivedAt":"2015-07-02T12:55:02Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"If filter-branch removes a commit which an annotated tag points to,\nand that tag is in the list of refs to be rewritten, we die with an\nerror like this:\n\n    error: Ref refs/tags/a1t is at 699cce2774f79a0830d8c5f631deca12d4b1ee8c but expected ba247450492030b03e3d2a9d5fef7ef67519483e\n    Could not delete refs/tags/a1t\n\nIn order to update refs, we first peel the ref until we find a\ncommit sha1. We then pass the commit sha1 to update-ref as the\n<oldvalue> parameter. Please consider the following scenarios:\n\n a) The ref points to a commit object directly. In this case,\n    update-ref will find that the current value of the ref still\n    matches oldvalue, and succeeds. This check is redundant, since\n    we only just queried the current value.\n\n b) The ref points to a tag object. In this case, update-ref will\n    error out, since the commit sha1 cannot match the current value\n    of the ref. If the commit has been removed, or rewritten into\n    multiple commits, we simply die. If the commit has been\n    rewritten, we output a warning message saying that to rewrite\n    tags one should use --tag-name-filter, and then we continue. If\n    --tag-name-filter is active, the tag will later be rewritten.\n\nThere seems to be no added value in passing the <oldvalue>\nparameter. So remove it.\n\nThis fixes deletion of tag objects. We also do not die any more if\na tag object points to a commit which has been rewritten into\nmultiple commits. However, we probably will die later in the\n--tag-name-filter code, because it does not seem to handle this\ncase.\n\nThis is a minimalist fix which leaves the following issues open:\n\n o In the absence of --tag-name-filter, we rewrite lightweight tags, but\n   not annotated tags, which is not intuitive. We do output a warning,\n   though:\n\n   $ git filter-branch --msg-filter \"cat && echo hi\" -- --all\n   [...]\n   WARNING: You said to rewrite tagged commits, but not the corresponding tag.\n   WARNING: Perhaps use '--tag-name-filter cat' to rewrite the tag.\n\n o Annotated tags are backed up as lightweight tags.\n\n o Annotated tags are backed up even in the absence of\n   --tag-name-filter. But in this case backup is not needed because\n   they are not rewritten.\n\nThese issues could be solved by moving the tag rewriting logic from\ntag-name-filter to the regular ref updating code, and\ntag-name-filter should deal only with renaming tags. However, this\nwould change behavior. Currently, the following command would\nrewrite tags:\n\n    git filter-branch --msg-filter \"cat && echo hi\" \\\n        --tag-name-filter cat -- --branches\n\nWith the suggested behavior, tags would be rewritten only if we\ninclude them in the rev-list options:\n\n    git filter-branch --msg-filter=\"cat && echo hi\" -- --all\n\nI am not sure if we can afford to change behavior like that.\n\nCc: Thomas Rast <tr@thomasrast.ch>\nCc: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Clemens Buchacher <clemens.buchacher@intel.com>\n---\n\nRe-send with Thomas' email address fixed.\n\n git-filter-branch.sh     | 20 +++++++++-----------\n t/t7003-filter-branch.sh | 21 +++++++++++++++++++++\n 2 files changed, 30 insertions(+), 11 deletions(-)\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex 5b3f63d..7ca1d99 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -399,21 +399,19 @@ do\n \tcase \"$rewritten\" in\n \t'')\n \t\techo \"Ref '$ref' was deleted\"\n-\t\tgit update-ref -m \"filter-branch: delete\" -d \"$ref\" $sha1 ||\n+\t\tgit update-ref -m \"filter-branch: delete\" -d \"$ref\" ||\n \t\t\tdie \"Could not delete $ref\"\n \t;;\n \t$_x40)\n \t\techo \"Ref '$ref' was rewritten\"\n-\t\tif ! git update-ref -m \"filter-branch: rewrite\" \\\n-\t\t\t\t\t\"$ref\" $rewritten $sha1 2>/dev/null; then\n-\t\t\tif test $(git cat-file -t \"$ref\") = tag; then\n-\t\t\t\tif test -z \"$filter_tag_name\"; then\n-\t\t\t\t\twarn \"WARNING: You said to rewrite tagged commits, but not the corresponding tag.\"\n-\t\t\t\t\twarn \"WARNING: Perhaps use '--tag-name-filter cat' to rewrite the tag.\"\n-\t\t\t\tfi\n-\t\t\telse\n-\t\t\t\tdie \"Could not rewrite $ref\"\n+\t\tif test $(git cat-file -t \"$ref\") = tag; then\n+\t\t\tif test -z \"$filter_tag_name\"; then\n+\t\t\t\twarn \"WARNING: You said to rewrite tagged commits, but not the corresponding tag.\"\n+\t\t\t\twarn \"WARNING: Perhaps use '--tag-name-filter cat' to rewrite the tag.\"\n \t\t\tfi\n+\t\telse\n+\t\t\tgit update-ref -m \"filter-branch: rewrite\" \"$ref\" $rewritten ||\n+\t\t\t\tdie \"Could not rewrite $ref\"\n \t\tfi\n \t;;\n \t*)\n@@ -423,7 +421,7 @@ do\n \t\twarn \"WARNING: Ref '$ref' points to the first one now.\"\n \t\trewritten=$(echo \"$rewritten\" | head -n 1)\n \t\tgit update-ref -m \"filter-branch: rewrite to first\" \\\n-\t\t\t\t\"$ref\" $rewritten $sha1 ||\n+\t\t\t\t\"$ref\" $rewritten ||\n \t\t\tdie \"Could not rewrite $ref\"\n \t;;\n \tesac\ndiff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh\nindex 855afda..6a34527 100755\n--- a/t/t7003-filter-branch.sh\n+++ b/t/t7003-filter-branch.sh\n@@ -261,6 +261,7 @@ test_expect_success 'Subdirectory filter with disappearing trees' '\n '\n \n test_expect_success 'Tag name filtering retains tag message' '\n+\tgit update-ref -d refs/tags/T &&\n \tgit tag -m atag T &&\n \tgit cat-file tag T > expect &&\n \tgit filter-branch -f --tag-name-filter cat &&\n@@ -268,6 +269,26 @@ test_expect_success 'Tag name filtering retains tag message' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success \"Rewrite commit referenced by annotated tag\" '\n+\tgit update-ref -d refs/tags/T &&\n+\tgit tag -a -m atag T &&\n+\tgit rev-parse refs/tags/T^0 >old_commit &&\n+\tgit filter-branch -f --msg-filter \"cat && echo foo\" --tag-name-filter cat refs/tags/T &&\n+\techo tag >type.expect &&\n+\tgit cat-file -t refs/tags/T >type.actual &&\n+\ttest_cmp type.expect type.actual &&\n+\tgit rev-parse refs/tags/T^0 >new_commit &&\n+\ttest_must_fail test_cmp old_commit new_commit\n+'\n+\n+test_expect_success \"Remove all commits\" '\n+\tgit branch removed-branch &&\n+\tgit tag -a -m atag removed-tag &&\n+\tgit filter-branch -f --commit-filter \"skip_commit \\\"\\$@\\\"\" removed-branch removed-tag &&\n+\ttest_must_fail git rev-parse refs/heads/removed-branch &&\n+\ttest_must_fail git rev-parse refs/tags/removed-tag\n+'\n+\n faux_gpg_tag='object XXXXXX\n type commit\n tag S\n-- \n1.9.4\n"}]}