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

[Ken Brown <kbrown@cornell.edu>]

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