Re: Branch Name Case Sensitivity
- From
Karsten Blees <karsten.blees@gmail.com>
- Date
- Mar 4, 2014, 13:23 UTC
- Message-ID
- <5315D3B9.6050602@gmail.com>
- In-Reply-To
- <xmqqsiqzrwzr.fsf@gitster.dls.corp.google.com>
Am 03.03.2014 18:51, schrieb Junio C Hamano:
Show 18 quoted lines
> Lee Hopkins <leerhop@gmail.com> writes: > >> I went ahead and took a stab at a solution. My solution is more >> aggressive than a warning, I actually prevent the creation of >> ambiguous refs. My changes are also in refs.c, which may not be >> appropriate, but it seemed like the natural place. >> >> I have never contributed to Git (in fact this is my first dive into >> the source) and my C is a bit rusty, so bear with me, this is just a >> suggestion: >> >> --- >> refs.c | 31 ++++++++++++++++++++++++------- >> 1 files changed, 24 insertions(+), 7 deletions(-) > > Starting something like this from forbidding is likely to turn out > to be a very bad idea that can break existing repositories. >
Its sure worth considering what should be done with pre-existing duplicates. However, repositories with such refs are already broken on case-insensitive filesystems, and allowing something that's known to be broken is even more dangerous, IMO.
An alternative approach could be to encode upper-case letters in loose refs if core.ignorecase == true (e.g. "Foo" -> "%46oo"). Although this may pose a problem for commands that bypass the refs API / plumbing for whatever reason.
> A new configuration
>
> refs.caseInsensitive = {warn|error|allow}
> s/caseInsensitive/caseSensitive/ Its case-sensitive refs that cause trouble, case-insensitive refs would be fine on all platforms.
I still don't see why we need an extra setting for this. The problems are inherently caused by case-insensitive filesystems, and we already have 'core.ignorecase' for that (its even automatically configured). Having an extra setting for refs is somewhat like making 'core.ignorecase' configurable per sub-directory.
Show 18 quoted lines
> that defaults to "warn" and the user can choose to set to "error" to
> forbid, would be more palatable, I would say.
>
> If the variable is not in 'core.' namespace, you should implement
> this check at the Porcelain level, allowing lower-level tools like
> update-ref as an escape hatch that let users bypass the restriction
> to be used to correct breakages; it would mean an unconditional "if
> !stricmp(), it is an error" in refs.c will not work well.
>
> I think it might be OK to have
>
> core.allowCaseInsentitiveRefs = {yes|no|warn}
>
> which defaults to 'warn' (and 'yes' corresponds to 'allow', 'no'
> corresponds to 'error', in the previous suggestion), instead. If we
> wanted to prevent even lower-level tools like update-ref from
> bypassing the check, that is.
> Its the plumbing that's broken, so implementing checks at the porcelain level won't help much. In particular, git-update-ref currently drops branches (or resets them to an earlier state) and messes up reflogs.