Lee Hopkins [off-list ref] 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.
A new configuration
refs.caseInsensitive = {warn|error|allow}
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.
Am 03.03.2014 18:51, schrieb Junio C Hamano:
Lee Hopkins [off-list ref] writes:
quoted
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.
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.
On 2014-03-04 14.23, Karsten Blees wrote:
Am 03.03.2014 18:51, schrieb Junio C Hamano:
quoted
Lee Hopkins [off-list ref] writes:
quoted
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.
quoted
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.
I start to agree here.
The case-insensitive file system does not allow branches foo and Foo at the same time,
and the packed refs should simply follow this convention/restriction/behaviour.
(and everything else could and should go into another patch:
If we ever want Linux to ignore the case in refs,
to ease the cross-platform development with Windows.
Or if we allow Windows/Mac OS to handle case insensitive refs (by always packing them)
to ease the co-working with e.g. Linux.
)
Lee, could you improve your change in refs.c into a real patch, with a commit message?
(And please have a look at the indentation with TABs)
A test case could be good, if time allows I can make a suggestion.
Thanks for all comments
/Torsten
Lee, could you improve your change in refs.c into a real patch, with a commit message?
(And please have a look at the indentation with TABs)
A test case could be good, if time allows I can make a suggestion.
I will remove the refs.ignorecase flag and work on a test care or two,
it will have to wait a few days tho.
(and everything else could and should go into another patch:
If we ever want Linux to ignore the case in refs,
to ease the cross-platform development with Windows.
Or if we allow Windows/Mac OS to handle case insensitive refs (by always packing them)
to ease the co-working with e.g. Linux.
)
I was actually planning on tying to add this to my changes if they
gained any traction. Why is another patch desirable?
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.
I also would not mind working on either of Junio's suggestions if one
is more desirable than what I already have.
-Lee