{"thread":{"id":"38051","subject":"[PATCH] t1402: check to delete broken refs","startedAt":"2014-11-25T22:56:06Z","lastAt":"2014-11-26T18:43:24Z","messageCount":8,"participants":["Stefan Beller","Jonathan Nieder","Torsten Bögershausen","Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"252549","messageId":"1416956166-20341-1-git-send-email-sbeller@google.com","threadId":"38051","inReplyTo":null,"subject":"[PATCH] t1402: check to delete broken refs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-25T22:56:06Z","receivedAt":"2014-11-25T22:56:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg <sahlberg@google.com>\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n \n This was also part of the ongoing series from Ronnie.\n But I think the patch in this form is rather independant, \n documenting the current state of \"git branch -d\", so it's \n fine to have it in now.\n\n t/t1402-check-ref-format.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\nindex e5dc62e..08af156 100755\n--- a/t/t1402-check-ref-format.sh\n+++ b/t/t1402-check-ref-format.sh\n@@ -197,4 +197,12 @@ invalid_ref_normalized 'heads///foo.lock'\n invalid_ref_normalized 'foo.lock/bar'\n invalid_ref_normalized 'foo.lock///bar'\n \n+test_expect_failure 'git branch -d can delete ref with broken sha1' '\n+\techo \"pointing nowhere\" > .git/refs/heads/brokensha1 &&\n+\ttest_when_finished \"rm -f .git/refs/heads/brokensha1\" &&\n+\tgit branch -d brokensha1 &&\n+\tgit branch >output &&\n+\t! grep -e \"brokensha1\" output\n+'\n+\n test_done\n-- \n2.2.0.rc3\n"},{"id":"252554","messageId":"20141126003506.GA19239@google.com","threadId":"38051","inReplyTo":"1416956166-20341-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] t1402: check to delete broken refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-26T00:35:06Z","receivedAt":"2014-11-26T00:35:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> This was also part of the ongoing series from Ronnie.\n> But I think the patch in this form is rather independant,\n> documenting the current state of \"git branch -d\", so it's\n> fine to have it in now.\n\nIs there a patch adding the feature this patch describes that this\ncould be squashed into?\n\n>  t/t1402-check-ref-format.sh | 8 ++++++++\n>  1 file changed, 8 insertions(+)\n\nThis doesn't have anything to do with check-ref-format --- it's about\nhow easy it is to recover from a repository with corrupt files in it.\nWould it fit somewhere like t3200-branch.sh?\n"},{"id":"252555","messageId":"CAGZ79kao0CpeidBzVL4O-4jfNd7nGbOJwtvsJCEgOjb370z8uw@mail.gmail.com","threadId":"38051","inReplyTo":"20141126003506.GA19239@google.com","subject":"Re: [PATCH] t1402: check to delete broken refs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-26T00:42:20Z","receivedAt":"2014-11-26T00:42:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 25, 2014 at 4:35 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Hi,\n>\n> Stefan Beller wrote:\n>\n>> This was also part of the ongoing series from Ronnie.\n>> But I think the patch in this form is rather independant,\n>> documenting the current state of \"git branch -d\", so it's\n>> fine to have it in now.\n>\n> Is there a patch adding the feature this patch describes that this\n> could be squashed into?\n\nI have extracted it actually from a patch, which it was part of.\nI want to send as much uncontroversial stuff to the list, before I try another\nround of the larger reflog transactions series.\n\nFor now this test just documents, how git branch is behaving differently\nfrom its desired  behavior. So it is also a subtle proposal how the desired\nbehavior should look like. (Deleting broken branches without further\nquestioning, flags or other magic incantations)\n\n\n>\n>>  t/t1402-check-ref-format.sh | 8 ++++++++\n>>  1 file changed, 8 insertions(+)\n>\n> This doesn't have anything to do with check-ref-format --- it's about\n> how easy it is to recover from a repository with corrupt files in it.\n> Would it fit somewhere like t3200-branch.sh?\n\nI'll look into placing this test in a more appropriate spot.\n"},{"id":"252557","messageId":"1416963565-32318-1-git-send-email-sbeller@google.com","threadId":"38051","inReplyTo":"CAGZ79kao0CpeidBzVL4O-4jfNd7nGbOJwtvsJCEgOjb370z8uw@mail.gmail.com","subject":"[PATCHv2] branch -d: test if we can delete broken refs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-26T00:59:25Z","receivedAt":"2014-11-26T00:59:25Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg <sahlberg@google.com>\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\nChanges v1->v2\n * relocated the test from t1402 to t3200\n * reword the commit message title to fit in with similar commits touching \n   t/t3200-branch.sh \n\n t/t3200-branch.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 432921b..fa7d7bd 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -163,6 +163,14 @@ test_expect_success 'git branch --list -d t should fail' '\n \ttest_path_is_missing .git/refs/heads/t\n '\n \n+test_expect_failure 'git branch -d can delete ref with broken sha1' '\n+\techo \"pointing nowhere\" > .git/refs/heads/brokensha1 &&\n+\ttest_when_finished \"rm -f .git/refs/heads/brokensha1\" &&\n+\tgit branch -d brokensha1 &&\n+\tgit branch >output &&\n+\t! grep -e \"brokensha1\" output\n+'\n+\n test_expect_success 'git branch --column' '\n \tCOLUMNS=81 git branch --column=column >actual &&\n \tcat >expected <<\\EOF &&\n-- \n2.2.0.rc3\n"},{"id":"252572","messageId":"54755F7C.3010803@web.de","threadId":"38051","inReplyTo":"1416956166-20341-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] t1402: check to delete broken refs","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-11-26T05:05:00Z","receivedAt":"2014-11-26T05:05:00Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 11/25/2014 11:56 PM, Stefan Beller wrote:\n> From: Ronnie Sahlberg <sahlberg@google.com>\n>\n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>   \n>   This was also part of the ongoing series from Ronnie.\n>   But I think the patch in this form is rather independant,\n>   documenting the current state of \"git branch -d\", so it's\n>   fine to have it in now.\n>\n>   t/t1402-check-ref-format.sh | 8 ++++++++\n>   1 file changed, 8 insertions(+)\n>\n> diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\n> index e5dc62e..08af156 100755\n> --- a/t/t1402-check-ref-format.sh\n> +++ b/t/t1402-check-ref-format.sh\n> @@ -197,4 +197,12 @@ invalid_ref_normalized 'heads///foo.lock'\n>   invalid_ref_normalized 'foo.lock/bar'\n>   invalid_ref_normalized 'foo.lock///bar'\n>   \n> +test_expect_failure 'git branch -d can delete ref with broken sha1' '\n> +\techo \"pointing nowhere\" > .git/refs/heads/brokensha1 &&\n> +\ttest_when_finished \"rm -f .git/refs/heads/brokensha1\" &&\n> +\tgit branch -d brokensha1 &&\n> +\tgit branch >output &&\n> +\t! grep -e \"brokensha1\" output\n> +'\nDo we need grep -e here ?\nIt does not give us anything in the pattern we are using.\n\n   -e PATTERN, --regexp=PATTERN\n               Use  PATTERN  as  the pattern.  This can be used to \nspecify multiple search patterns, or to protect a pattern beginning\n               with a hyphen (-).  (-e is specified by POSIX.)\n\n(And we do not need the \"\" either, there is no space in brokensha1)\nWe can simply use:\n! grep brokensha1 output\n\n\n\n\n\n> +\n>   test_done\n"},{"id":"252574","messageId":"547593FF.6040507@alum.mit.edu","threadId":"38051","inReplyTo":"1416963565-32318-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2] branch -d: test if we can delete broken refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-26T08:49:03Z","receivedAt":"2014-11-26T08:49:03Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Aside from one tiny formatting nit (see below), the test looks good to\nme. On the other hand, this is kind of an \"aspirational test\"; I don't\nknow that the tested functionality has ever worked or that anybody has\never claimed that it works. So my feeling is that the addition of the\ntest would feel more natural in the patch series that implements the new\nfeature. But I don't feel strongly about it.\n\nMichael\n\nOn 11/26/2014 01:59 AM, Stefan Beller wrote:\n> From: Ronnie Sahlberg <sahlberg@google.com>\n> \n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> Changes v1->v2\n>  * relocated the test from t1402 to t3200\n>  * reword the commit message title to fit in with similar commits touching \n>    t/t3200-branch.sh \n> \n>  t/t3200-branch.sh | 8 ++++++++\n>  1 file changed, 8 insertions(+)\n> \n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index 432921b..fa7d7bd 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -163,6 +163,14 @@ test_expect_success 'git branch --list -d t should fail' '\n>  \ttest_path_is_missing .git/refs/heads/t\n>  '\n>  \n> +test_expect_failure 'git branch -d can delete ref with broken sha1' '\n> +\techo \"pointing nowhere\" > .git/refs/heads/brokensha1 &&\n\nPlease no space between the \">\" and the filename.\n\n> +\ttest_when_finished \"rm -f .git/refs/heads/brokensha1\" &&\n> +\tgit branch -d brokensha1 &&\n> +\tgit branch >output &&\n> +\t! grep -e \"brokensha1\" output\n> +'\n> +\n>  test_expect_success 'git branch --column' '\n>  \tCOLUMNS=81 git branch --column=column >actual &&\n>  \tcat >expected <<\\EOF &&\n> \n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"252589","messageId":"1417027024-17054-1-git-send-email-sbeller@google.com","threadId":"38051","inReplyTo":"54755F7C.3010803@web.de","subject":"[PATCHv3] branch -d: test if we can delete broken refs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-26T18:37:04Z","receivedAt":"2014-11-26T18:37:04Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg <sahlberg@google.com>\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\nChanges v1->v2\n * relocated the test from t1402 to t3200\n * reword the commit message title to fit in with similar commits touching \n   t/t3200-branch.sh \n\nChanges v2 -> v3\n * remove space nit pointed out by Michael\n * simplified grep expression as pointed out by Torsten\n \n I don't mind applying this one later as part of the series, where we actually\n introduce the feature. Thanks for all the feedback! \n\nMichael writes \n> On the other hand, this is kind of an \"aspirational test\"; I don't\n> know that the tested functionality has ever worked or that anybody has\n> ever claimed that it works. So my feeling is that the addition of the\n> test would feel more natural in the patch series that implements the new\n> feature. But I don't feel strongly about it.\n\n I don't have a strong oppinion on this one either. It's just my impression, that\n small single patches are digested easier by the mailing list rather than large series.\n So as the feedback is in, I'll put this patch at the end of the series, where the actual\n feature will be introduced.\n \n\n t/t3200-branch.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 432921b..fa7d7bd 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -163,6 +163,14 @@ test_expect_success 'git branch --list -d t should fail' '\n \ttest_path_is_missing .git/refs/heads/t\n '\n \n+test_expect_failure 'git branch -d can delete ref with broken sha1' '\n+\techo \"pointing nowhere\" >.git/refs/heads/brokensha1 &&\n+\ttest_when_finished \"rm -f .git/refs/heads/brokensha1\" &&\n+\tgit branch -d brokensha1 &&\n+\tgit branch >output &&\n+\t! grep brokensha1 output\n+'\n+\n test_expect_success 'git branch --column' '\n \tCOLUMNS=81 git branch --column=column >actual &&\n \tcat >expected <<\\EOF &&\n-- \n2.2.0.rc3\n"},{"id":"252590","messageId":"xmqqbnntreer.fsf@gitster.dls.corp.google.com","threadId":"38051","inReplyTo":"547593FF.6040507@alum.mit.edu","subject":"Re: [PATCHv2] branch -d: test if we can delete broken refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-26T18:43:24Z","receivedAt":"2014-11-26T18:43:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> ... On the other hand, this is kind of an \"aspirational test\"; I don't\n> know that the tested functionality has ever worked or that anybody has\n> ever claimed that it works. So my feeling is that the addition of the\n> test would feel more natural in the patch series that implements the new\n> feature. But I don't feel strongly about it.\n\nI share your feeling.  \n\nIn the \"aspiration followed by realization\" pattern, the realization\ncommit shows a change in t/ hierarchy that turns test_expect_failure\ninto test_expect_success and it is likely that what is being tested\nwill fall outside the context.  Unless the test title is phrased\nvery well, it would not be obvious from the patch text to the t/\nhierarchy alone what behaviour is being corrected when looking at\nthe realization commit.\n\nIf aspiration and realization are in a same series, that would not\nbe a problem, but it is if the commits that add \"aspirational test\"\nand \"realization\" are too far apart.\n\nIf it is pretty clear to everybody that another topic to realize the\naspiration will be coming in a not so distant future, I think it is\nfine, though.\n"}]}