threads / bug / 41659

Bug: git branch -D can be used to delete branch which is currently checked out - Part 2

Subject: Bug: git branch -D can be used to delete branch which is currently checked out - Part 2

## tl;dr

6 messages between Mar 10, 2016 and Mar 10, 2016.

replies: 5people: 3as markdown or json

Marcus Kida· Mar 10, 2016, 08:39 UTC · lore

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· Mar 10, 2016, 12:15 UTC · re: Marcus Kida · lore

Re: Bug: git branch -D can be used to delete branch which is currently checked out - Part 2

Hi Marcus,

On Thu, 10 Mar 2016, Marcus Kida wrote:

Show 5 quoted lines

> 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· Mar 10, 2016, 18:30 UTC · re: Johannes Schindelin · lore

Re: Bug: git branch -D can be used to delete branch which is currently checked out - Part 2

Hi Johannes,

thank you for the feedback. I will fix this, test it and send a patch.

Cheers, Marcus

Show 21 quoted lines

> On 10 Mar 2016, at 11:15 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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· Mar 10, 2016, 22:41 UTC · re: Marcus Kida · lore

Re: Bug: git branch -D can be used to delete branch which is currently checked out - Part 2

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· Mar 10, 2016, 22:43 UTC · re: Jeff King · lore

Re: Bug: git branch -D can be used to delete branch which is currently checked out - Part 2

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.

Show 27 quoted lines

> On 11 Mar 2016, at 9:41 AM, Jeff King <peff@peff.net> 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

Marcus Kida· Mar 10, 2016, 21:20 UTC · re: Johannes Schindelin · lore

Re: Bug: git branch -D can be used to delete branch which is currently checked out - Part 2

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 <Johannes.Schindelin@gmx.de> 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