[Emacs-diffs] master 2f5e0b1: Improve case-insensitive checks (Bug#24441)

[Emacs-diffs] master 2f5e0b1: Improve case-insensitive checks (Bug#24441)

[Eli Zaretskii]

I've reverted that commit.  I see no reason in your changes, which
effectively removed code and useful documentation and comments, on
which Ken labored, without any discussion.

This is not how we should treat work done by others.

Re: [Emacs-diffs] master 2f5e0b1: Improve case-insensitive checks (Bug#24441)

[Ken Brown]

On 11/14/2016 2:30 PM, Eli Zaretskii wrote:
> I've reverted that commit.  I see no reason in your changes, which
> effectively removed code and useful documentation and comments, on
> which Ken labored, without any discussion.

Thanks.  I think there are some useful ideas in Paul's commit, but I 
also see some problems.  I'll write specific comments in reply to his 
commit message.

Ken

Re: [Emacs-diffs] master 2f5e0b1: Improve case-insensitive checks (Bug#24441)

[Eli Zaretskii]

> From: Ken Brown <kbrown@cornell.edu>
> Date: Mon, 14 Nov 2016 15:22:03 -0500
> Cc: emacs-devel@gnu.org
> 
> On 11/14/2016 2:30 PM, Eli Zaretskii wrote:
> > I've reverted that commit.  I see no reason in your changes, which
> > effectively removed code and useful documentation and comments, on
> > which Ken labored, without any discussion.
> 
> Thanks.  I think there are some useful ideas in Paul's commit, but I 
> also see some problems.  I'll write specific comments in reply to his 
> commit message.

I'd be glad to see the solution improved, of course.  But the ways to
improve it should be discussed first and committed only after that,
especially when the changes effectively disable portions of the
previous commit and remove other portions.

Re: [Emacs-diffs] master 2f5e0b1: Improve case-insensitive checks (Bug#24441)

[Paul Eggert]

On 11/14/2016 12:43 PM, Eli Zaretskii wrote:
> I'd be glad to see the solution improved, of course.

I thought those changes were relatively minor technical improvements. 
Evidently they were taken as intrusive and stepping on other peoples' 
toes. Sorry about that; that was certainly not the intent. Please feel 
free to ignore the changes.

Re: [Emacs-diffs] master 2f5e0b1: Improve case-insensitive checks (Bug#24441)

[Ken Brown]

On 11/14/2016 12:09 PM, Paul Eggert wrote:
> --- a/doc/lispref/files.texi
> +++ b/doc/lispref/files.texi
> @@ -1144,17 +1144,9 @@ return value is unspecified.
>  Sometimes file names or their parts need to be compared as strings, in
>  which case it's important to know whether the underlying filesystem is
>  case-insensitive.  This function returns @code{t} if file
> -@var{filename} is on a case-insensitive filesystem.  It always returns
> -@code{t} on MS-DOS and MS-Windows.  On Cygwin and Mac OS X,
> -filesystems may or may not be case-insensitive, and the function tries
> -to determine case-sensitivity by a runtime test.  If the test is
> -inconclusive, the function returns @code{t} on Cygwin and @code{nil}
> -on Mac OS X.
> -
> -Currently this function always returns @code{nil} on platforms other
> -than MS-DOS, MS-Windows, Cygwin, and Mac OS X.  It does not detect
> -case-insensitivity of mounted filesystems, such as Samba shares or
> -NFS-mounted Windows volumes.
> +@var{filename} is on a case-insensitive filesystem.  On platforms where
> +this information is not available, this function guesses based on
> +common practice.

I like my more verbose version better.  Users who don't know what common 
practice is might find the platform-specific information useful.  For 
example, I'll bet there are many Cygwin users who don't know that Cygwin 
can be figured to be case-sensitive.  And I wouldn't be surprised (based 
on bugs 24441 and 22300) if there are OS X users who don't know that OS 
X volumes can be configured to be case-insensitive.

> -/* Filesystems are case-sensitive on all supported systems except
> -   MS-Windows, MS-DOS, Cygwin, and Mac OS X.  They are always
> -   case-insensitive on the first two, but they may or may not be
> -   case-insensitive on Cygwin and OS X.  The following function
> -   attempts to provide a runtime test on those two systems.  If the
> -   test is not conclusive, we assume case-insensitivity on Cygwin and
> -   case-sensitivity on Mac OS X.
> +/* Return true if FILENAME is on a case-insensitive file system.
> +   Use a runtime test if available.  Otherwise, assume the file system
> +   is case-insensitive on Microsoft-based platforms and case-sensitive
> +   elsewhere.

Once again, I would rather keep the platform-specific information.

> -#elif defined CYGWIN
> -/* As of Cygwin-2.6.1, pathconf supports _PC_CASE_INSENSITIVE.  */
> -# ifdef _PC_CASE_INSENSITIVE
> +#ifdef _PC_CASE_INSENSITIVE

I have no problem with the removal of "#elif defined CYGWIN", but AFAIK 
Cygwin (as of version 2.6.1) is the only platform that defines 
_PC_CASE_INSENSITIVE.  So I think the comment should be retained in some 
form, for clarity.

>    int res = pathconf (filename, _PC_CASE_INSENSITIVE);
> -  if (res < 0)
> -    return 1;
> -  return res > 0;
> -# else
> -  return 1;
> +  if (0 < res)
> +    return true;
> +  if (res == 0 || errno != EINVAL)
> +    return false;

What's the rationale for returning false on an error?  The only relevant 
platform here is Cygwin, so we should just fall through to the "#if 
defined CYGWIN || defined DOS_NT" if there's an error.

> +#elif defined _PC_CASE_SENSITIVE

Here AFAIK Darwin is the only platform that defines _PC_CASE_SENSITIVE, 
so there should be a comment to that effect.  Otherwise readers may not 
even know that we're now treating the Darwin case.

> +  int res = pathconf (filename, _PC_CASE_SENSITIVE);
> +  if (res == 0)
> +    return true;
> +  if (0 < res || errno != EINVAL)
> +    return false;

Again, what's the rationale for returning false on error?  Why not just 
fall through (which happens to have the same effect)?

> +#ifdef DARWIN_OS
> +  /* It is not clear whether this section is needed.  For now, rely on
> +     pathconf and skip this section.  If pathconf does not work,
> +     please recompile Emacs with -DDARWIN_OS_CASE_SENSITIVE_FIXME=1 or
> +     -DDARWIN_OS_CASE_SENSITIVE_FIXME=2, and file a bug report saying
> +     whether this fixed your problem.  */
> +# ifndef DARWIN_OS_CASE_SENSITIVE_FIXME
> +  int DARWIN_OS_CASE_SENSITIVE_FIXME = 0;
>  # endif
> -#elif defined DARWIN_OS
> -  /* The following is based on
> -     http://lists.apple.com/archives/darwin-dev/2007/Apr/msg00010.html.  */
> -  struct attrlist alist;
> -  unsigned char buffer[sizeof (vol_capabilities_attr_t) + sizeof (size_t)];
> -
> -  memset (&alist, 0, sizeof (alist));
> -  alist.volattr = ATTR_VOL_CAPABILITIES;
> -  if (getattrlist (filename, &alist, buffer, sizeof (buffer), 0)
> -      || !(alist.volattr & ATTR_VOL_CAPABILITIES))
> -    return 0;
> -  vol_capabilities_attr_t *vcaps = buffer;
> -  return !(vcaps->capabilities[0] & VOL_CAP_FMT_CASE_SENSITIVE);
> +
> +  if (DARWIN_OS_CASE_SENSITIVE_FIXME == 1)
> +    {
> +      /* This is based on developer.apple.com's getattrlist man page.  */
> +      struct attrlist alist = {.volattr = ATTR_VOL_CAPABILITIES};
> +      struct vol_capabilities_attr_t vcaps;
> +      if (getattrlist (filename, &alist, &vcaps, sizeof vcaps, 0) == 0)
> +	{
> +	  if (vcaps.valid[VOL_CAPABILITIES_FORMAT] & VOL_CAP_FMT_CASE_SENSITIVE)
> +	    return ! (vcaps.capabilities[VOL_CAPABILITIES_FORMAT]
> +		      & VOL_CAP_FMT_CASE_SENSITIVE);
> +	}
> +      else if (errno != EINVAL)
> +	return false;
> +    }
> +  else if (DARWIN_OS_CASE_SENSITIVE_FIXME == 2)
> +    {
> +      /* The following is based on
> +	 http://lists.apple.com/archives/darwin-dev/2007/Apr/msg00010.html.  */
> +      struct attrlist alist;
> +      unsigned char buffer[sizeof (vol_capabilities_attr_t) + sizeof (size_t)];
> +
> +      memset (&alist, 0, sizeof (alist));
> +      alist.volattr = ATTR_VOL_CAPABILITIES;
> +      if (getattrlist (filename, &alist, buffer, sizeof (buffer), 0)
> +	  || !(alist.volattr & ATTR_VOL_CAPABILITIES))
> +	return 0;
> +      vol_capabilities_attr_t *vcaps = buffer;
> +      return !(vcaps->capabilities[0] & VOL_CAP_FMT_CASE_SENSITIVE);
> +    }
> +#endif

I have no opinion on this hunk.  I'd like to see some OS X experts chime 
in.  I've added John to the CC.

> -  if ((!(file_name_case_insensitive_p (SSDATA (encoded_file)))
> +  if ((! file_name_case_insensitive_p (SSDATA (encoded_file))

I think you just wanted to add a space after !, but you also removed a 
parenthesis at the end of the line.  (But this doesn't matter any more, 
since Eli already reverted the commit.)

Ken

Re: [Emacs-diffs] master 2f5e0b1: Improve case-insensitive checks (Bug#24441)

[Ken Brown]

On 11/14/2016 3:54 PM, Paul Eggert wrote:
> On 11/14/2016 12:43 PM, Eli Zaretskii wrote:
>> I'd be glad to see the solution improved, of course.
>
> I thought those changes were relatively minor technical improvements.
> Evidently they were taken as intrusive and stepping on other peoples'
> toes. Sorry about that; that was certainly not the intent. Please feel
> free to ignore the changes.

I don't want to ignore the changes.  I'll send a new patch that 
incorporates some of your changes.

Ken

Re: [Emacs-diffs] master 2f5e0b1: Improve case-insensitive checks (Bug#24441)

[John Wiegley]

>>>>> Ken Brown <kbrown@cornell.edu> writes:

> I have no opinion on this hunk. I'd like to see some OS X experts chime in.
> I've added John to the CC.

Hi Ken, I really don't write OS X-specific C code, so I'm not qualified to
judge the merits of that hunk.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: [Emacs-diffs] master 2f5e0b1: Improve case-insensitive checks (Bug#24441)

[Ken Brown]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

On 11/14/2016 4:07 PM, Ken Brown wrote:
> On 11/14/2016 3:54 PM, Paul Eggert wrote:
>> On 11/14/2016 12:43 PM, Eli Zaretskii wrote:
>>> I'd be glad to see the solution improved, of course.
>>
>> I thought those changes were relatively minor technical improvements.
>> Evidently they were taken as intrusive and stepping on other peoples'
>> toes. Sorry about that; that was certainly not the intent. Please feel
>> free to ignore the changes.
>
> I don't want to ignore the changes.  I'll send a new patch that
> incorporates some of your changes.

Here it is.

Ken


[-- Attachment #2: 0001-Simplify-case-insensitivity-checks-on-Mac-OS-X.patch --]
[-- Type: text/plain, Size: 3932 bytes --]

From 93cd4ee1fc261bca5d9d54f6e1366e74668ccc29 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Mon, 14 Nov 2016 17:26:12 -0500
Subject: [PATCH] Simplify case-insensitivity checks on Mac OS X

* src/fileio.c (file_name_case_insensitive_p): Try skipping the
Darwin code and instead using pathconf with _PC_CASE_SENSITIVE.
Leave in two alternatives conditionally compiled based on
DARWIN_OS_CASE_SENSITIVE_FIXME in case pathconf doesn't work.
---
 src/fileio.c | 81 ++++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 57 insertions(+), 24 deletions(-)

diff --git a/src/fileio.c b/src/fileio.c
index f3f8f42..70799eb 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -2251,33 +2251,66 @@ internal_delete_file (Lisp_Object filename)
 static bool
 file_name_case_insensitive_p (const char *filename)
 {
-#ifdef DOS_NT
-  return 1;
-#elif defined CYGWIN
-/* As of Cygwin-2.6.1, pathconf supports _PC_CASE_INSENSITIVE.  */
-# ifdef _PC_CASE_INSENSITIVE
+  /* Use pathconf with _PC_CASE_INSENSITIVE or _PC_CASE_SENSITIVE if
+     those flags are available.  As of this writing (2016-11-14),
+     Cygwin is the only platform known to support the former (starting
+     with Cygwin-2.6.1), and Mac OS X is the only platform known to
+     support the latter.
+
+     There have been reports that pathconf with _PC_CASE_SENSITIVE
+     does not work reliably on Mac OS X.  If you have a problem,
+     please recompile Emacs with -DDARWIN_OS_CASE_SENSITIVE_FIXME=1 or
+     -DDARWIN_OS_CASE_SENSITIVE_FIXME=2, and file a bug report saying
+     whether this fixed your problem.  */
+
+#ifdef _PC_CASE_INSENSITIVE
   int res = pathconf (filename, _PC_CASE_INSENSITIVE);
-  if (res < 0)
-    return 1;
-  return res > 0;
-# else
-  return 1;
+  if (res >= 0)
+    return res > 0;
+#elif defined _PC_CASE_SENSITIVE && !defined DARWIN_OS_CASE_SENSITIVE_FIXME
+  int res = pathconf (filename, _PC_CASE_SENSITIVE);
+  if (res >= 0)
+    return res == 0;
+#endif
+
+#ifdef DARWIN_OS
+# ifndef DARWIN_OS_CASE_SENSITIVE_FIXME
+  int DARWIN_OS_CASE_SENSITIVE_FIXME = 0;
 # endif
-#elif defined DARWIN_OS
-  /* The following is based on
-     http://lists.apple.com/archives/darwin-dev/2007/Apr/msg00010.html.  */
-  struct attrlist alist;
-  unsigned char buffer[sizeof (vol_capabilities_attr_t) + sizeof (size_t)];
-
-  memset (&alist, 0, sizeof (alist));
-  alist.volattr = ATTR_VOL_CAPABILITIES;
-  if (getattrlist (filename, &alist, buffer, sizeof (buffer), 0)
-      || !(alist.volattr & ATTR_VOL_CAPABILITIES))
-    return 0;
-  vol_capabilities_attr_t *vcaps = buffer;
-  return !(vcaps->capabilities[0] & VOL_CAP_FMT_CASE_SENSITIVE);
+
+  if (DARWIN_OS_CASE_SENSITIVE_FIXME == 1)
+    {
+      /* This is based on developer.apple.com's getattrlist man page.  */
+      struct attrlist alist = {.volattr = ATTR_VOL_CAPABILITIES};
+      struct vol_capabilities_attr_t vcaps;
+      if (getattrlist (filename, &alist, &vcaps, sizeof vcaps, 0) == 0)
+	{
+	  if (vcaps.valid[VOL_CAPABILITIES_FORMAT] & VOL_CAP_FMT_CASE_SENSITIVE)
+	    return ! (vcaps.capabilities[VOL_CAPABILITIES_FORMAT]
+		      & VOL_CAP_FMT_CASE_SENSITIVE);
+	}
+    }
+  else if (DARWIN_OS_CASE_SENSITIVE_FIXME == 2)
+    {
+      /* The following is based on
+	 http://lists.apple.com/archives/darwin-dev/2007/Apr/msg00010.html.  */
+      struct attrlist alist;
+      unsigned char buffer[sizeof (vol_capabilities_attr_t)  sizeof (size_t)];
+
+      memset (&alist, 0, sizeof (alist));
+      alist.volattr = ATTR_VOL_CAPABILITIES;
+      if (getattrlist (filename, &alist, buffer, sizeof (buffer), 0)
+	  || !(alist.volattr & ATTR_VOL_CAPABILITIES))
+	return 0;
+      vol_capabilities_attr_t *vcaps = buffer;
+      return !(vcaps->capabilities[0] & VOL_CAP_FMT_CASE_SENSITIVE);
+    }
+#endif	/* DARWIN_OS */
+
+#if defined CYGWIN || defined DOS_NT
+  return true;
 #else
-  return 0;
+  return false;
 #endif
 }
 
-- 
2.8.3

Re: [Emacs-diffs] master 2f5e0b1: Improve case-insensitive checks (Bug#24441)

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Ken Brown <kbrown@cornell.edu>
> Date: Mon, 14 Nov 2016 17:33:18 -0500
> 
> +     There have been reports that pathconf with _PC_CASE_SENSITIVE
> +     does not work reliably on Mac OS X.  If you have a problem,
> +     please recompile Emacs with -DDARWIN_OS_CASE_SENSITIVE_FIXME=1 or
> +     -DDARWIN_OS_CASE_SENSITIVE_FIXME=2, and file a bug report saying
> +     whether this fixed your problem.  */

I think this text (with some context) should also be in etc/PROBLEMS.

Otherwise, LGTM, thanks.

Re: [Emacs-diffs] master 2f5e0b1: Improve case-insensitive checks (Bug#24441)

[Ken Brown]

On 11/15/2016 11:07 AM, Eli Zaretskii wrote:
> I think this text (with some context) should also be in etc/PROBLEMS.

OK.  Is Mac OS X considered to be a Unix variant?  If so, I would put 
this under

* Runtime problems specific to individual Unix variants
** Mac OS/X    <<<<<<<<<<< new subheading

Otherwise, I would create a new heading

* Runtime problems specific to Mac OS X

Ken

Re: [Emacs-diffs] master 2f5e0b1: Improve case-insensitive checks (Bug#24441)

[Eli Zaretskii]

> Cc: eggert@cs.ucla.edu, emacs-devel@gnu.org
> From: Ken Brown <kbrown@cornell.edu>
> Date: Tue, 15 Nov 2016 14:15:16 -0500
> 
> Is Mac OS X considered to be a Unix variant?

I think it should be separate, as NEWS makes it so.

Re: [Emacs-diffs] master 2f5e0b1: Improve case-insensitive checks (Bug#24441)

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 383 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am Di., 15. Nov. 2016 um 21:26 Uhr:

> > Cc: eggert@cs.ucla.edu, emacs-devel@gnu.org
> > From: Ken Brown <kbrown@cornell.edu>
> > Date: Tue, 15 Nov 2016 14:15:16 -0500
> >
> > Is Mac OS X considered to be a Unix variant?
>
> I think it should be separate, as NEWS makes it so.
>
>
Also please call it "macOS", as that's the current product name.

[-- Attachment #2: Type: text/html, Size: 1075 bytes --]