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

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

[Marcus Kida]

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

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

[Johannes Schindelin]

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

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

[Marcus Kida]

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 <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

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

[Marcus Kida]

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

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

[Jeff King]

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

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

[Marcus Kida]

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 <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