{"thread":{"id":"56378","subject":"[PATCH RESEND] branch: allow deleting dangling branches with --force","startedAt":"2021-08-25T20:43:44Z","lastAt":"2021-08-27T18:35:46Z","messageCount":17,"participants":["René Scharfe","Junio C Hamano","Ævar Arnfjörð Bjarmason","Han-Wen Nienhuys","Ulrich Windl"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"433767","messageId":"7894f736-4681-7656-e2d4-5945d2c71d31@web.de","threadId":"56378","inReplyTo":null,"subject":"[PATCH RESEND] branch: allow deleting dangling branches with --force","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-08-25T20:43:38Z","receivedAt":"2021-08-25T20:43:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"git branch only allows deleting branches that point to valid commits.\nSkip that check if --force is given, as the caller is indicating with\nit that they know what they are doing and accept the consequences.\nThis allows deleting dangling branches, which previously had to be\nreset to a valid start-point using --force first.\n\nSigned-off-by: René Scharfe \n---\nOriginal submission:\nhttp://public-inbox.org/git/52847a99-db7c-9634-b3b1-fd9b1342bc32@web.de/\n\n Documentation/git-branch.txt | 3 ++-\n builtin/branch.c | 2 +-\n t/t3200-branch.sh | 7 +++++++\n 3 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 94dc9a54f2..5449767121 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -118,7 +118,8 @@ OPTIONS\n \tReset to , even if exists\n \talready. Without `-f`, 'git branch' refuses to change an existing branch.\n \tIn combination with `-d` (or `--delete`), allow deleting the\n-\tbranch irrespective of its merged status. In combination with\n+\tbranch irrespective of its merged status, or whether it even\n+\tpoints to a valid commit. In combination with\n \t`-m` (or `--move`), allow renaming the branch even if the new\n \tbranch name already exists, the same applies for `-c` (or `--copy`).\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex b23b1d1752..03c7b7253a 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -168,7 +168,7 @@ static int check_branch_commit(const char *branchname, const char *refname,\n \t\t\t int kinds, int force)\n {\n \tstruct commit *rev = lookup_commit_reference(the_repository, oid);\n-\tif (!rev) {\n+\tif (!force && !rev) {\n \t\terror(_(\"Couldn't look up commit object for '%s'\"), refname);\n \t\treturn -1;\n \t}\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex cc4b10236e..ec61a10c29 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1272,6 +1272,13 @@ test_expect_success 'attempt to delete a branch merged to its base' '\n \ttest_must_fail git branch -d my10\n '\n\n+test_expect_success 'branch --delete --force removes dangling branch' '\n+\ttest_when_finished \"rm -f .git/refs/heads/dangling\" &&\n+\techo $ZERO_OID >.git/refs/heads/dangling &&\n+\tgit branch --delete --force dangling &&\n+\ttest_path_is_missing .git/refs/heads/dangling\n+'\n+\n test_expect_success 'use --edit-description' '\n \twrite_script editor <<-\\EOF &&\n \t\techo \"New contents\" >\"$1\"\n--\n2.32.0\n"},{"id":"433770","messageId":"xmqqr1ehgq8t.fsf@gitster.g","threadId":"56378","inReplyTo":"7894f736-4681-7656-e2d4-5945d2c71d31@web.de","subject":"Re: [PATCH RESEND] branch: allow deleting dangling branches with --force","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-25T21:37:38Z","receivedAt":"2021-08-25T21:37:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> git branch only allows deleting branches that point to valid commits.\n> Skip that check if --force is given, as the caller is indicating with\n> it that they know what they are doing and accept the consequences.\n> This allows deleting dangling branches, which previously had to be\n> reset to a valid start-point using --force first.\n>\n> Signed-off-by: René Scharfe \n> ---\n> Original submission:\n> http://public-inbox.org/git/52847a99-db7c-9634-b3b1-fd9b1342bc32@web.de/\n\nThanks.\n\n> +test_expect_success 'branch --delete --force removes dangling branch' '\n> +\ttest_when_finished \"rm -f .git/refs/heads/dangling\" &&\n> +\techo $ZERO_OID >.git/refs/heads/dangling &&\n> +\tgit branch --delete --force dangling &&\n> +\ttest_path_is_missing .git/refs/heads/dangling\n> +'\n\nThis goes against the spirit of the series merged at c9780bb2 (Merge\nbranch 'hn/prep-tests-for-reftable', 2021-07-13).\n\nCan we creat the dangling ref and test the lack of \"dangling\" ref in\nthe end in a less transparent way?\n\nAn escape hatch is to make this test depend on the REFFILES\nprerequisite, just like dc474899 (t4202: mark bogus head hash test\nwith REFFILES, 2021-05-31) did, which may be more appropriate.\n\n> test_expect_success 'use --edit-description' '\n> \twrite_script editor <<-\\EOF &&\n> \t\techo \"New contents\" >\"$1\"\n> --\n> 2.32.0\n"},{"id":"433784","messageId":"87h7fdcdc7.fsf@evledraar.gmail.com","threadId":"56378","inReplyTo":"xmqqr1ehgq8t.fsf@gitster.g","subject":"Re: [PATCH RESEND] branch: allow deleting dangling branches with --force","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-25T23:28:00Z","receivedAt":"2021-08-25T23:30:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Aug 25 2021, Junio C Hamano wrote:\n\n> René Scharfe writes:\n>\n>> git branch only allows deleting branches that point to valid commits.\n>> Skip that check if --force is given, as the caller is indicating with\n>> it that they know what they are doing and accept the consequences.\n>> This allows deleting dangling branches, which previously had to be\n>> reset to a valid start-point using --force first.\n>>\n>> Signed-off-by: René Scharfe \n>> ---\n>> Original submission:\n>> http://public-inbox.org/git/52847a99-db7c-9634-b3b1-fd9b1342bc32@web.de/\n>\n> Thanks.\n>\n>> +test_expect_success 'branch --delete --force removes dangling branch' '\n>> +\ttest_when_finished \"rm -f .git/refs/heads/dangling\" &&\n>> +\techo $ZERO_OID >.git/refs/heads/dangling &&\n>> +\tgit branch --delete --force dangling &&\n>> +\ttest_path_is_missing .git/refs/heads/dangling\n>> +'\n>\n> This goes against the spirit of the series merged at c9780bb2 (Merge\n> branch 'hn/prep-tests-for-reftable', 2021-07-13).\n>\n> Can we creat the dangling ref and test the lack of \"dangling\" ref in\n> the end in a less transparent way?\n>\n> An escape hatch is to make this test depend on the REFFILES\n> prerequisite, just like dc474899 (t4202: mark bogus head hash test\n> with REFFILES, 2021-05-31) did, which may be more appropriate.\n\nI'm not sure, but this may also be a good example of the sort of thing\nthat we should probably go beyond REFFILES with, i.e. is it even\npossible under reftable to run into this sort of situation?\n\nNot really a topic for this series, but something to make a mental note\nof for the reftable topic, i.e. we may eventually want to edit the docs\netc. appropriately if and when the new backend is more mature.\n"},{"id":"433785","messageId":"87eeahcd7f.fsf@evledraar.gmail.com","threadId":"56378","inReplyTo":"7894f736-4681-7656-e2d4-5945d2c71d31@web.de","subject":"Re: [PATCH RESEND] branch: allow deleting dangling branches with --force","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-25T23:30:13Z","receivedAt":"2021-08-25T23:32:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Aug 25 2021, René Scharfe wrote:\n\n> git branch only allows deleting branches that point to valid commits.\n> Skip that check if --force is given, as the caller is indicating with\n> it that they know what they are doing and accept the consequences.\n> This allows deleting dangling branches, which previously had to be\n> reset to a valid start-point using --force first.\n>\n> Signed-off-by: René Scharfe \n> ---\n> Original submission:\n> http://public-inbox.org/git/52847a99-db7c-9634-b3b1-fd9b1342bc32@web.de/\n>\n> Documentation/git-branch.txt | 3 ++-\n> builtin/branch.c | 2 +-\n> t/t3200-branch.sh | 7 +++++++\n> 3 files changed, 10 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\n> index 94dc9a54f2..5449767121 100644\n> --- a/Documentation/git-branch.txt\n> +++ b/Documentation/git-branch.txt\n> @@ -118,7 +118,8 @@ OPTIONS\n> \tReset to , even if exists\n> \talready. Without `-f`, 'git branch' refuses to change an existing branch.\n> \tIn combination with `-d` (or `--delete`), allow deleting the\n> -\tbranch irrespective of its merged status. In combination with\n> +\tbranch irrespective of its merged status, or whether it even\n> +\tpoints to a valid commit. In combination with\n> \t`-m` (or `--move`), allow renaming the branch even if the new\n> \tbranch name already exists, the same applies for `-c` (or `--copy`).\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index b23b1d1752..03c7b7253a 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -168,7 +168,7 @@ static int check_branch_commit(const char *branchname, const char *refname,\n> \t\t\t int kinds, int force)\n> {\n> \tstruct commit *rev = lookup_commit_reference(the_repository, oid);\n> -\tif (!rev) {\n> +\tif (!force && !rev) {\n> \t\terror(_(\"Couldn't look up commit object for '%s'\"), refname);\n> \t\treturn -1;\n> \t}\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index cc4b10236e..ec61a10c29 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -1272,6 +1272,13 @@ test_expect_success 'attempt to delete a branch merged to its base' '\n> \ttest_must_fail git branch -d my10\n> '\n>\n> +test_expect_success 'branch --delete --force removes dangling branch' '\n> +\ttest_when_finished \"rm -f .git/refs/heads/dangling\" &&\n> +\techo $ZERO_OID >.git/refs/heads/dangling &&\n> +\tgit branch --delete --force dangling &&\n> +\ttest_path_is_missing .git/refs/heads/dangling\n> +'\n\nIsn't a more meaningful test here to use a \"real\" SHA, instead of the\n$ZERO_OID? You can use $(test_oid deadbeef) to get one of those.\n\nThat way we know that this this test & logic is really testing that we\ncan delete a branch that's been racily GC'd away or whatever, and not\none in the already-broken state of referring to the $ZERO_OID.\n\nAlso: How does \"git tag -d\" handle this scenario if the same sort of\ndata were added to .git/refs/tags/* ?\n"},{"id":"433812","messageId":"CAFQ2z_Mwj0+uWO-Ly8MTHmidjf=1P-9K1n2f0DSNzN=Or2t=bw@mail.gmail.com","threadId":"56378","inReplyTo":"xmqqr1ehgq8t.fsf@gitster.g","subject":"Re: [PATCH RESEND] branch: allow deleting dangling branches with --force","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-08-26T07:26:01Z","receivedAt":"2021-08-26T07:26:16Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Aug 25, 2021 at 11:37 PM Junio C Hamano wrote:\n\n> > +test_expect_success 'branch --delete --force removes dangling branch' '\n> > + test_when_finished \"rm -f .git/refs/heads/dangling\" &&\n> > + echo $ZERO_OID >.git/refs/heads/dangling &&\n> > + git branch --delete --force dangling &&\n> > + test_path_is_missing .git/refs/heads/dangling\n> > +'\n>\n> This goes against the spirit of the series merged at c9780bb2 (Merge\n> branch 'hn/prep-tests-for-reftable', 2021-07-13).\n>\n> Can we creat the dangling ref and test the lack of \"dangling\" ref in\n> the end in a less transparent way?\n\nagreed. Try the ref-store test-helper's update-ref command?\n\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"433850","messageId":"xmqq5yvsf8p5.fsf@gitster.g","threadId":"56378","inReplyTo":"CAFQ2z_Mwj0+uWO-Ly8MTHmidjf=1P-9K1n2f0DSNzN=Or2t=bw@mail.gmail.com","subject":"Re: [PATCH RESEND] branch: allow deleting dangling branches with --force","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-26T16:54:14Z","receivedAt":"2021-08-26T16:54:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys writes:\n\n> On Wed, Aug 25, 2021 at 11:37 PM Junio C Hamano wrote:\n>\n>> > +test_expect_success 'branch --delete --force removes dangling branch' '\n>> > + test_when_finished \"rm -f .git/refs/heads/dangling\" &&\n>> > + echo $ZERO_OID >.git/refs/heads/dangling &&\n>> > + git branch --delete --force dangling &&\n>> > + test_path_is_missing .git/refs/heads/dangling\n>> > +'\n>>\n>> This goes against the spirit of the series merged at c9780bb2 (Merge\n>> branch 'hn/prep-tests-for-reftable', 2021-07-13).\n>>\n>> Can we creat the dangling ref and test the lack of \"dangling\" ref in\n>> the end in a less transparent way?\n>\n> agreed. Try the ref-store test-helper's update-ref command?\n\nI thought the approach taken by dc474899 (t4202: mark bogus head\nhash test with REFFILES, 2021-05-31) to hide it behind a\nprerequisite was good enough, but if we can ensure the same\nbehaviour under the reftable backend, that is even better.\n\nThanks.\n\n"},{"id":"433851","messageId":"xmqq1r6gf6ne.fsf@gitster.g","threadId":"56378","inReplyTo":"xmqq5yvsf8p5.fsf@gitster.g","subject":"Re: [PATCH RESEND] branch: allow deleting dangling branches with --force","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-26T17:38:29Z","receivedAt":"2021-08-26T17:38:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Han-Wen Nienhuys writes:\n>\n>> On Wed, Aug 25, 2021 at 11:37 PM Junio C Hamano wrote:\n>>\n>>> > +test_expect_success 'branch --delete --force removes dangling branch' '\n>>> > + test_when_finished \"rm -f .git/refs/heads/dangling\" &&\n>>> > + echo $ZERO_OID >.git/refs/heads/dangling &&\n>>> > + git branch --delete --force dangling &&\n>>> > + test_path_is_missing .git/refs/heads/dangling\n>>> > +'\n>>>\n>>> This goes against the spirit of the series merged at c9780bb2 (Merge\n>>> branch 'hn/prep-tests-for-reftable', 2021-07-13).\n>>>\n>>> Can we creat the dangling ref and test the lack of \"dangling\" ref in\n>>> the end in a less transparent way?\n>>\n>> agreed. Try the ref-store test-helper's update-ref command?\n>\n> I thought the approach taken by dc474899 (t4202: mark bogus head\n> hash test with REFFILES, 2021-05-31) to hide it behind a\n> prerequisite was good enough, but if we can ensure the same\n> behaviour under the reftable backend, that is even better.\n>\n> Thanks.\n\nHaving said that, there are a few observations to make about this\ntest script.\n\n * It is hopefully becoming harder and harder to check for behaviour\n in broken repositories in a \"portable\" way, simply because we are\n making it harder to corrupt repository. We hopefully won't point\n a ref to point at a missing object, we hopefully won't prune an\n object away that is still pointed at by a ref, etc.\n\n * This script to test \"branch\" is full of tests that rely on direct\n manipulation of .git/refs/ filesystem hierarchy.\n\nFor these two reasons, it probably is OK to accept this patch as-is\nand leave the \"clean-up\" to a later follow-on series, that would\ncover both \"what's our recommended approach to 'corrupt' the test\nrepository so that we can use different ref (and other) backends?\"\nand \"make sure the tests in the script are happy with both ref\nbackends.\" issues.\n\nThanks.\n"},{"id":"433852","messageId":"f2511e9a-cc0f-20d0-d196-5dacdd637b7d@web.de","threadId":"56378","inReplyTo":"CAFQ2z_Mwj0+uWO-Ly8MTHmidjf=1P-9K1n2f0DSNzN=Or2t=bw@mail.gmail.com","subject":"Re: [PATCH RESEND] branch: allow deleting dangling branches with --force","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-08-26T18:18:53Z","receivedAt":"2021-08-26T18:19:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 26.08.21 um 09:26 schrieb Han-Wen Nienhuys:\n> On Wed, Aug 25, 2021 at 11:37 PM Junio C Hamano wrote:\n>\n>>> +test_expect_success 'branch --delete --force removes dangling branch' '\n>>> + test_when_finished \"rm -f .git/refs/heads/dangling\" &&\n>>> + echo $ZERO_OID >.git/refs/heads/dangling &&\n>>> + git branch --delete --force dangling &&\n>>> + test_path_is_missing .git/refs/heads/dangling\n>>> +'\n>>\n>> This goes against the spirit of the series merged at c9780bb2 (Merge\n>> branch 'hn/prep-tests-for-reftable', 2021-07-13).\n\nI assume the file backend won't go away anytime soon. So I guess the\nidea is that the test suite is supposed to be run with the new backend\nas default and exercise it?\n\n>> Can we creat the dangling ref and test the lack of \"dangling\" ref in\n>> the end in a less transparent way?\n>\n> agreed. Try the ref-store test-helper's update-ref command?\n\nIt requires the new hash to refer to an existing object, so we can't\nuse it in this test.\n\nRené\n"},{"id":"433853","messageId":"611b4216-7650-f4c2-ba46-0ddd98d5d7e5@web.de","threadId":"56378","inReplyTo":"87h7fdcdc7.fsf@evledraar.gmail.com","subject":"Re: [PATCH RESEND] branch: allow deleting dangling branches with --force","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-08-26T18:19:00Z","receivedAt":"2021-08-26T18:19:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 26.08.21 um 01:28 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Wed, Aug 25 2021, Junio C Hamano wrote:\n>\n>> René Scharfe writes:\n>>\n>>> git branch only allows deleting branches that point to valid commits.\n>>> Skip that check if --force is given, as the caller is indicating with\n>>> it that they know what they are doing and accept the consequences.\n>>> This allows deleting dangling branches, which previously had to be\n>>> reset to a valid start-point using --force first.\n>>>\n>>> Signed-off-by: René Scharfe \n>>> ---\n>>> Original submission:\n>>> http://public-inbox.org/git/52847a99-db7c-9634-b3b1-fd9b1342bc32@web.de/\n>>\n>> Thanks.\n>>\n>>> +test_expect_success 'branch --delete --force removes dangling branch' '\n>>> +\ttest_when_finished \"rm -f .git/refs/heads/dangling\" &&\n>>> +\techo $ZERO_OID >.git/refs/heads/dangling &&\n>>> +\tgit branch --delete --force dangling &&\n>>> +\ttest_path_is_missing .git/refs/heads/dangling\n>>> +'\n>>\n>> This goes against the spirit of the series merged at c9780bb2 (Merge\n>> branch 'hn/prep-tests-for-reftable', 2021-07-13).\n>>\n>> Can we creat the dangling ref and test the lack of \"dangling\" ref in\n>> the end in a less transparent way?\n>>\n>> An escape hatch is to make this test depend on the REFFILES\n>> prerequisite, just like dc474899 (t4202: mark bogus head hash test\n>> with REFFILES, 2021-05-31) did, which may be more appropriate.\n>\n> I'm not sure, but this may also be a good example of the sort of thing\n> that we should probably go beyond REFFILES with, i.e. is it even\n> possible under reftable to run into this sort of situation?\n\nProbably yes: A commit can disappear when its object file or pack or\nalternate object database gets lost somehow, and a ref store could\nonly compensate for that loss if it kept a copy of the ref target,\nwhich seems impractical.\n\nRené\n"},{"id":"433854","messageId":"f53313bf-222d-bb0e-d91e-a0359ffdaeff@web.de","threadId":"56378","inReplyTo":"87eeahcd7f.fsf@evledraar.gmail.com","subject":"Re: [PATCH RESEND] branch: allow deleting dangling branches with --force","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-08-26T18:19:06Z","receivedAt":"2021-08-26T18:19:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 26.08.21 um 01:30 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Wed, Aug 25 2021, René Scharfe wrote:\n>\n>> git branch only allows deleting branches that point to valid commits.\n>> Skip that check if --force is given, as the caller is indicating with\n>> it that they know what they are doing and accept the consequences.\n>> This allows deleting dangling branches, which previously had to be\n>> reset to a valid start-point using --force first.\n>>\n>> Signed-off-by: René Scharfe \n>> ---\n>> Original submission:\n>> http://public-inbox.org/git/52847a99-db7c-9634-b3b1-fd9b1342bc32@web.de/\n>>\n>> Documentation/git-branch.txt | 3 ++-\n>> builtin/branch.c | 2 +-\n>> t/t3200-branch.sh | 7 +++++++\n>> 3 files changed, 10 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\n>> index 94dc9a54f2..5449767121 100644\n>> --- a/Documentation/git-branch.txt\n>> +++ b/Documentation/git-branch.txt\n>> @@ -118,7 +118,8 @@ OPTIONS\n>> \tReset to , even if exists\n>> \talready. Without `-f`, 'git branch' refuses to change an existing branch.\n>> \tIn combination with `-d` (or `--delete`), allow deleting the\n>> -\tbranch irrespective of its merged status. In combination with\n>> +\tbranch irrespective of its merged status, or whether it even\n>> +\tpoints to a valid commit. In combination with\n>> \t`-m` (or `--move`), allow renaming the branch even if the new\n>> \tbranch name already exists, the same applies for `-c` (or `--copy`).\n>>\n>> diff --git a/builtin/branch.c b/builtin/branch.c\n>> index b23b1d1752..03c7b7253a 100644\n>> --- a/builtin/branch.c\n>> +++ b/builtin/branch.c\n>> @@ -168,7 +168,7 @@ static int check_branch_commit(const char *branchname, const char *refname,\n>> \t\t\t int kinds, int force)\n>> {\n>> \tstruct commit *rev = lookup_commit_reference(the_repository, oid);\n>> -\tif (!rev) {\n>> +\tif (!force && !rev) {\n>> \t\terror(_(\"Couldn't look up commit object for '%s'\"), refname);\n>> \t\treturn -1;\n>> \t}\n>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n>> index cc4b10236e..ec61a10c29 100755\n>> --- a/t/t3200-branch.sh\n>> +++ b/t/t3200-branch.sh\n>> @@ -1272,6 +1272,13 @@ test_expect_success 'attempt to delete a branch merged to its base' '\n>> \ttest_must_fail git branch -d my10\n>> '\n>>\n>> +test_expect_success 'branch --delete --force removes dangling branch' '\n>> +\ttest_when_finished \"rm -f .git/refs/heads/dangling\" &&\n>> +\techo $ZERO_OID >.git/refs/heads/dangling &&\n>> +\tgit branch --delete --force dangling &&\n>> +\ttest_path_is_missing .git/refs/heads/dangling\n>> +'\n>\n> Isn't a more meaningful test here to use a \"real\" SHA, instead of the\n> $ZERO_OID? You can use $(test_oid deadbeef) to get one of those.\n>\n> That way we know that this this test & logic is really testing that we\n> can delete a branch that's been racily GC'd away or whatever, and not\n> one in the already-broken state of referring to the $ZERO_OID.\n\nRight, git branch --delete could cheat by treating all-zero object IDs\nspecially, and the test would then not exercise the original scenario.\n\n> Also: How does \"git tag -d\" handle this scenario if the same sort of\n> data were added to .git/refs/tags/* ?\n\nIt deletes that tag, no --force needed.\n\nRené\n"},{"id":"433855","messageId":"325d64e9-8a31-6ba0-73f2-5e9d67b8682f@web.de","threadId":"56378","inReplyTo":"7894f736-4681-7656-e2d4-5945d2c71d31@web.de","subject":"[PATCH v2] branch: allow deleting dangling branches with --force","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-08-26T18:19:15Z","receivedAt":"2021-08-26T18:19:23Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"git branch only allows deleting branches that point to valid commits.\nSkip that check if --force is given, as the caller is indicating with\nit that they know what they are doing and accept the consequences.\nThis allows deleting dangling branches, which previously had to be\nreset to a valid start-point using --force first.\n\nReported-by: Ulrich Windl \nHelped-by: Ævar Arnfjörð Bjarmason \nHelped-by: Junio C Hamano \nSigned-off-by: René Scharfe \n---\nChanges since v1:\n- Added Reported-by and Helped-by.\n- Made test independent of ref store.\n\n Documentation/git-branch.txt | 3 ++-\n builtin/branch.c | 2 +-\n t/t3200-branch.sh | 12 ++++++++++++\n 3 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 94dc9a54f2..5449767121 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -118,7 +118,8 @@ OPTIONS\n \tReset to , even if exists\n \talready. Without `-f`, 'git branch' refuses to change an existing branch.\n \tIn combination with `-d` (or `--delete`), allow deleting the\n-\tbranch irrespective of its merged status. In combination with\n+\tbranch irrespective of its merged status, or whether it even\n+\tpoints to a valid commit. In combination with\n \t`-m` (or `--move`), allow renaming the branch even if the new\n \tbranch name already exists, the same applies for `-c` (or `--copy`).\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex b23b1d1752..03c7b7253a 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -168,7 +168,7 @@ static int check_branch_commit(const char *branchname, const char *refname,\n \t\t\t int kinds, int force)\n {\n \tstruct commit *rev = lookup_commit_reference(the_repository, oid);\n-\tif (!rev) {\n+\tif (!force && !rev) {\n \t\terror(_(\"Couldn't look up commit object for '%s'\"), refname);\n \t\treturn -1;\n \t}\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex cc4b10236e..d0d28c8ea7 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1272,6 +1272,18 @@ test_expect_success 'attempt to delete a branch merged to its base' '\n \ttest_must_fail git branch -d my10\n '\n\n+test_expect_success 'branch --delete --force removes dangling branch' '\n+\tgit checkout main &&\n+\ttest_commit unstable &&\n+\thash=$(git rev-parse HEAD) &&\n+\tobjpath=$(echo $hash | sed -e \"s|^..|.git/objects/&/|\") &&\n+\tgit branch --no-track dangling &&\n+\ttest_when_finished \"test -f $objpath.x && mv $objpath.x $objpath\" &&\n+\tmv $objpath $objpath.x &&\n+\tgit branch --delete --force dangling &&\n+\ttest -z \"$(git for-each-ref refs/heads/dangling)\"\n+'\n+\n test_expect_success 'use --edit-description' '\n \twrite_script editor <<-\\EOF &&\n \t\techo \"New contents\" >\"$1\"\n--\n2.33.0\n"},{"id":"433860","messageId":"xmqqk0k8do27.fsf@gitster.g","threadId":"56378","inReplyTo":"325d64e9-8a31-6ba0-73f2-5e9d67b8682f@web.de","subject":"Re: [PATCH v2] branch: allow deleting dangling branches with --force","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-26T19:05:20Z","receivedAt":"2021-08-26T19:05:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> +\thash=$(git rev-parse HEAD) &&\n> +\tobjpath=$(echo $hash | sed -e \"s|^..|.git/objects/&/|\") &&\n> +\tgit branch --no-track dangling &&\n> +\ttest_when_finished \"test -f $objpath.x && mv $objpath.x $objpath\" &&\n\nDo we need test -f here?\n\n> +\tmv $objpath $objpath.x &&\n> +\tgit branch --delete --force dangling &&\n\n> +\ttest -z \"$(git for-each-ref refs/heads/dangling)\"\n\nIt is not wrong per-se, but maybe\n\n\tgit show-ref --quiet refs/heads/dangling\n\nis more straight-forward.\n\nThanks.\n"},{"id":"433862","messageId":"87eeagyq2f.fsf@evledraar.gmail.com","threadId":"56378","inReplyTo":"325d64e9-8a31-6ba0-73f2-5e9d67b8682f@web.de","subject":"Re: [PATCH v2] branch: allow deleting dangling branches with --force","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-26T19:12:24Z","receivedAt":"2021-08-26T19:16:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Aug 26 2021, René Scharfe wrote:\n\n> - Added Reported-by and Helped-by.\n\nThanks, this whole thing looks good to me.\n\n> - Made test independent of ref store.\n\nAlso thanks. Just my 0.02: I think even with v1 this patch is fine to go\nin (but thanks for the re-roll!). I.e. under a full run of the testsuite\nwith reftable a bunch of things are broken currently.\n\nIt's not really that much more effort to just fix up code like in the v1\nof this patch when we get to fixing those with the reftable integration,\nand putting the onus on patch authors on testing that topic in \"seen\"\nwith their tests is probably not a good time investment overall\nv.s. just fixing them in bulk later.\n\nParticularly since in this case we can make it refstore independent,\nsince it's about a disappearing loose object, but in some other cases\nit's either the whole test that needs to be skipped, or we'd be better\noff with some helpers to produce the corruption in one way under\nREFFILES, and in another way under !REFFILES....\n"},{"id":"433879","messageId":"9c999d1f-9263-965d-a0a1-527e1260a6d9@web.de","threadId":"56378","inReplyTo":"xmqqk0k8do27.fsf@gitster.g","subject":"Re: [PATCH v2] branch: allow deleting dangling branches with --force","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-08-26T21:01:30Z","receivedAt":"2021-08-26T21:01:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 26.08.21 um 21:05 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> +\thash=$(git rev-parse HEAD) &&\n>> +\tobjpath=$(echo $hash | sed -e \"s|^..|.git/objects/&/|\") &&\n>> +\tgit branch --no-track dangling &&\n>> +\ttest_when_finished \"test -f $objpath.x && mv $objpath.x $objpath\" &&\n>\n> Do we need test -f here?\n\nIf the mv in the next line fails, then test in the cleanup prevents it\nfrom adding another confusing error. So it's not really needed, but\nkinda nice to have.\n\n>> +\tmv $objpath $objpath.x &&\n>> +\tgit branch --delete --force dangling &&\n>\n>> +\ttest -z \"$(git for-each-ref refs/heads/dangling)\"\n>\n> It is not wrong per-se, but maybe\n>\n> \tgit show-ref --quiet refs/heads/dangling\n>\n> is more straight-forward.\n\nActually it *is* wrong, because that check passes even if the dangling\nref still exists due to for-each-ref checking if the ref target exists\nand just erroring out if it doesn't. I somehow assumed it wouldn't do\nthis needless verification. So we'd need to check its return value:\n\n\tgit for-each-ref refs/heads/dangling >actual &&\n\ttest_must_be_empty actual\n\ngit show-ref fails both if the ref is missing and if it's dangling, so\nwe'd need to check its stderr to distinguish between those cases:\n\n\ttest_must_fail git show-ref --quiet refs/heads/dangling 2>err &&\n\ttest_must_be_empty err\n\nTo avoid these complications we could ask git branch itself:\n\n\ttest -z $(git branch --list dangling)\n\nRené\n"},{"id":"433909","messageId":"61289339020000A100043878@gwsmtp.uni-regensburg.de","threadId":"56378","inReplyTo":"xmqq1r6gf6ne.fsf@gitster.g","subject":"Antw: [EXT] Re: [PATCH RESEND] branch: allow deleting dangling branches with ‑‑force","fromName":"Ulrich Windl","fromEmail":"ulrich.windl@rz.uni-regensburg.de","sentAt":"2021-08-27T07:24:41Z","receivedAt":"2021-08-27T07:30:04Z","isPatch":true,"sender":{"key":"ulrich.windl@rz.uni-regensburg.de","avatar":null},"body":">>> Junio C Hamano schrieb am 26.08.2021 um 19:38 in Nachricht\n:\n\n...\n> * It is hopefully becoming harder and harder to check for behaviour\n> in broken repositories in a \"portable\" way, simply because we are\n> making it harder to corrupt repository. We hopefully won't point\n> a ref to point at a missing object, we hopefully won't prune an\n> object away that is still pointed at by a ref, etc.\n...\n\nMaybe git needs a \"--disarm-safety-belt\" option that disables all those nice checks for testing purposes ;-)\n\n\n"},{"id":"433912","messageId":"875yvrz5ho.fsf@evledraar.gmail.com","threadId":"56378","inReplyTo":"61289339020000A100043878@gwsmtp.uni-regensburg.de","subject":"Re: Antw: [EXT] Re: [PATCH RESEND] branch: allow deleting dangling branches with ‑‑force","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-27T07:53:09Z","receivedAt":"2021-08-27T07:55:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Aug 27 2021, Ulrich Windl wrote:\n\n>>>> Junio C Hamano schrieb am 26.08.2021 um 19:38 in Nachricht\n> :\n>\n> ...\n>> * It is hopefully becoming harder and harder to check for behaviour\n>> in broken repositories in a \"portable\" way, simply because we are\n>> making it harder to corrupt repository. We hopefully won't point\n>> a ref to point at a missing object, we hopefully won't prune an\n>> object away that is still pointed at by a ref, etc.\n> ...\n>\n> Maybe git needs a \"--disarm-safety-belt\" option that disables all those nice checks for testing purposes ;-)\n\nI haven't tested, but I think in both of those cases a way to accomplish\nthis corruption in a way that bypasses the safety of our tooling is also\nto setup an alternate object directory with the relevant object(s), and\nthen simply drop that alternate to simulate the case of an object\ndisappearing or other such corruption.\n"},{"id":"433946","messageId":"c192f438-2eaf-c098-9fe4-c03a9d36cbd0@web.de","threadId":"56378","inReplyTo":"325d64e9-8a31-6ba0-73f2-5e9d67b8682f@web.de","subject":"[PATCH v3] branch: allow deleting dangling branches with --force","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-08-27T18:35:35Z","receivedAt":"2021-08-27T18:35:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"git branch only allows deleting branches that point to valid commits.\nSkip that check if --force is given, as the caller is indicating with\nit that they know what they are doing and accept the consequences.\nThis allows deleting dangling branches, which previously had to be\nreset to a valid start-point using --force first.\n\nReported-by: Ulrich Windl \nHelped-by: Ævar Arnfjörð Bjarmason \nHelped-by: Junio C Hamano \nSigned-off-by: René Scharfe \n---\nChanges since v2:\n- move test_when_finished down to avoid need for test -f\n- check return code of git for-each-ref in test to distinguish\n between a deleted and a still existing, but dangling branch\n\n Documentation/git-branch.txt | 3 ++-\n builtin/branch.c | 2 +-\n t/t3200-branch.sh | 13 +++++++++++++\n 3 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 94dc9a54f2..5449767121 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -118,7 +118,8 @@ OPTIONS\n \tReset to , even if exists\n \talready. Without `-f`, 'git branch' refuses to change an existing branch.\n \tIn combination with `-d` (or `--delete`), allow deleting the\n-\tbranch irrespective of its merged status. In combination with\n+\tbranch irrespective of its merged status, or whether it even\n+\tpoints to a valid commit. In combination with\n \t`-m` (or `--move`), allow renaming the branch even if the new\n \tbranch name already exists, the same applies for `-c` (or `--copy`).\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex b23b1d1752..03c7b7253a 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -168,7 +168,7 @@ static int check_branch_commit(const char *branchname, const char *refname,\n \t\t\t int kinds, int force)\n {\n \tstruct commit *rev = lookup_commit_reference(the_repository, oid);\n-\tif (!rev) {\n+\tif (!force && !rev) {\n \t\terror(_(\"Couldn't look up commit object for '%s'\"), refname);\n \t\treturn -1;\n \t}\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex cc4b10236e..e575ffb4ff 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1272,6 +1272,19 @@ test_expect_success 'attempt to delete a branch merged to its base' '\n \ttest_must_fail git branch -d my10\n '\n\n+test_expect_success 'branch --delete --force removes dangling branch' '\n+\tgit checkout main &&\n+\ttest_commit unstable &&\n+\thash=$(git rev-parse HEAD) &&\n+\tobjpath=$(echo $hash | sed -e \"s|^..|.git/objects/&/|\") &&\n+\tgit branch --no-track dangling &&\n+\tmv $objpath $objpath.x &&\n+\ttest_when_finished \"mv $objpath.x $objpath\" &&\n+\tgit branch --delete --force dangling &&\n+\tgit for-each-ref refs/heads/dangling >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_expect_success 'use --edit-description' '\n \twrite_script editor <<-\\EOF &&\n \t\techo \"New contents\" >\"$1\"\n--\n2.33.0\n"}]}