# Bug: git branch -D can be used to delete branch which is currently checked out - Part 2 6 messages from 2016-03-10 to 2016-03-10. Participants: Marcus Kida, Johannes Schindelin, Jeff King. Thread: https://gitlist.dev/t/41659 ## Marcus Kida, 2016-03-10 08:39 Subject: Bug: git branch -D can be used to delete branch which is currently checked out - Part 2 Message-ID: URL: https://gitlist.dev/e/F6588CBF-D13E-475F-847F-6154370F4A2E%40gmail.com ``` Proposed solution: Use `strcasecmp`, `stricmp`, `strcmpi` here: https://github.com/git/git/blob/f02fbc4f9433937ee0463d0342d6d7d97e1f6f1e/builtin/branch.c#L218 Not sure if/which one of this will work on POSIX as well as MS too though. Thank you. Cheers, Marcus ``` ## Johannes Schindelin, 2016-03-10 12:15 Subject: Re: Bug: git branch -D can be used to delete branch which is currently checked out - Part 2 Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1603101313220.4690%40virtualbox In-Reply-To: ``` Hi Marcus, On Thu, 10 Mar 2016, Marcus Kida wrote: > Proposed solution: > > Use `strcasecmp`, `stricmp`, `strcmpi` here: https://github.com/git/git/blob/f02fbc4f9433937ee0463d0342d6d7d97e1f6f1e/builtin/branch.c#L218 > > Not sure if/which one of this will work on POSIX as well as MS too though. This is not quite a solution (it is not a patch). And you *definitely* want to make the use of strcasecmp() contingent on ignore_case. You are not alone on this world, after all, and other people have case-sensitive filesystems. It's totally doable, of course. So, do you feel up to the task? Ciao, Johannes ``` ## Marcus Kida, 2016-03-10 18:30 Subject: Re: Bug: git branch -D can be used to delete branch which is currently checked out - Part 2 Message-ID: <9EFA8642-FA02-4ADB-B37B-530E611C9C1F@gmail.com> URL: https://gitlist.dev/e/9EFA8642-FA02-4ADB-B37B-530E611C9C1F%40gmail.com In-Reply-To: ``` Hi Johannes, thank you for the feedback. I will fix this, test it and send a patch. Cheers, Marcus > On 10 Mar 2016, at 11:15 PM, Johannes Schindelin wrote: > > Hi Marcus, > >> On Thu, 10 Mar 2016, Marcus Kida wrote: >> >> Proposed solution: >> >> Use `strcasecmp`, `stricmp`, `strcmpi` here: https://github.com/git/git/blob/f02fbc4f9433937ee0463d0342d6d7d97e1f6f1e/builtin/branch.c#L218 >> >> Not sure if/which one of this will work on POSIX as well as MS too though. > > This is not quite a solution (it is not a patch). And you *definitely* > want to make the use of strcasecmp() contingent on ignore_case. You are > not alone on this world, after all, and other people have case-sensitive > filesystems. It's totally doable, of course. > > So, do you feel up to the task? > > Ciao, > Johannes ``` ## Marcus Kida, 2016-03-10 21:20 Subject: Re: Bug: git branch -D can be used to delete branch which is currently checked out - Part 2 Message-ID: <41E8F374-9E8A-48AE-A5D5-A3C24B79ED7E@gmail.com> URL: https://gitlist.dev/e/41E8F374-9E8A-48AE-A5D5-A3C24B79ED7E%40gmail.com In-Reply-To: ``` Hi Johannes, Hi Junio, here you’ll find a patch to hotfix the “delete-a-branch-you’re-on" issue. As Junio already stated there’s many more places where case (in)sensitivity is not handled correctly but this patch would at least prevent you from deleting the branch you’re currently working on (which happened to me yesterday and lead to quite some additional stress). If you think this isn’t worth patching now, please discard my patch. If you think it’s worth patching and implementing a more advanced filesystem backend at a later time, please apply it. If tested this and validate it works @ 2.8.0-rc1. Thank you. --- diff --git a/builtin/branch.c b/builtin/branch.c index 7b45b6b..46bde61 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -215,7 +215,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, int flags = 0; strbuf_branchname(&bname, argv[i]); - if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) { + if (kinds == FILTER_REFS_BRANCHES && !strcasecmp(head, bname.buf)) { error(_("Cannot delete the branch '%s' " "which you are currently on."), bname.buf); ret = 1; --- Cheers, Marcus > On 10 Mar 2016, at 11:15 PM, Johannes Schindelin wrote: > > Hi Marcus, > > On Thu, 10 Mar 2016, Marcus Kida wrote: > >> Proposed solution: >> >> Use `strcasecmp`, `stricmp`, `strcmpi` here: https://github.com/git/git/blob/f02fbc4f9433937ee0463d0342d6d7d97e1f6f1e/builtin/branch.c#L218 >> >> Not sure if/which one of this will work on POSIX as well as MS too though. > > This is not quite a solution (it is not a patch). And you *definitely* > want to make the use of strcasecmp() contingent on ignore_case. You are > not alone on this world, after all, and other people have case-sensitive > filesystems. It's totally doable, of course. > > So, do you feel up to the task? > > Ciao, > Johannes ``` ## Jeff King, 2016-03-10 22:41 Subject: Re: Bug: git branch -D can be used to delete branch which is currently checked out - Part 2 Message-ID: <20160310224158.GA2772@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160310224158.GA2772%40sigill.intra.peff.net In-Reply-To: <9EFA8642-FA02-4ADB-B37B-530E611C9C1F@gmail.com> ``` On Fri, Mar 11, 2016 at 05:30:00AM +1100, Marcus Kida wrote: > thank you for the feedback. > I will fix this, test it and send a patch. Unfortunately, I think this issue is a little more complicated. There's some prior discussion in http://thread.gmane.org/gmane.comp.version-control.git/284022 and http://thread.gmane.org/gmane.comp.version-control.git/276456/focus=276506 The latter, in particular, shows a case where this approach will do the wrong thing. The fundamental issue is that refs are potentially stored in _two_ places: the filesystem, and the packed-refs file. And the latter is always case-sensitive, while the former sometimes is and sometimes isn't. But because the storage all happens behind the scenes, the user has no way of reliably disambiguating (e.g., does "foo" refer to your checked-out "FOO", or are you intentionally trying to delete an extraneous "FOO" that ended up in the packed-refs file?). -Peff ``` ## Marcus Kida, 2016-03-10 22:43 Subject: Re: Bug: git branch -D can be used to delete branch which is currently checked out - Part 2 Message-ID: <02FB9587-5849-4B3A-A199-6483377C3EED@gmail.com> URL: https://gitlist.dev/e/02FB9587-5849-4B3A-A199-6483377C3EED%40gmail.com In-Reply-To: <20160310224158.GA2772@sigill.intra.peff.net> ``` Fair enough, thank you. I’m going to take a look at the previous threads. I’d also be keen to help working on those issues. > On 11 Mar 2016, at 9:41 AM, Jeff King wrote: > > On Fri, Mar 11, 2016 at 05:30:00AM +1100, Marcus Kida wrote: > >> thank you for the feedback. >> I will fix this, test it and send a patch. > > Unfortunately, I think this issue is a little more complicated. > > There's some prior discussion in > > http://thread.gmane.org/gmane.comp.version-control.git/284022 > > and > > http://thread.gmane.org/gmane.comp.version-control.git/276456/focus=276506 > > The latter, in particular, shows a case where this approach will do the > wrong thing. The fundamental issue is that refs are potentially stored > in _two_ places: the filesystem, and the packed-refs file. And the > latter is always case-sensitive, while the former sometimes is and > sometimes isn't. But because the storage all happens behind the scenes, > the user has no way of reliably disambiguating (e.g., does "foo" refer > to your checked-out "FOO", or are you intentionally trying to delete an > extraneous "FOO" that ended up in the packed-refs file?). > > -Peff ```