From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Ken Brown Newsgroups: gmane.emacs.devel Subject: Re: [Emacs-diffs] master 2f5e0b1: Improve case-insensitive checks (Bug#24441) Date: Mon, 14 Nov 2016 16:03:00 -0500 Message-ID: References: <20161114170952.20595.32286@vcs.savannah.gnu.org> <20161114170952.6729A220179@vcs.savannah.gnu.org> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Trace: blaine.gmane.org 1479157419 17165 195.159.176.226 (14 Nov 2016 21:03:39 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Mon, 14 Nov 2016 21:03:39 +0000 (UTC) User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 Cc: John Wiegley , Eli Zaretskii To: emacs-devel@gnu.org, Paul Eggert Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Nov 14 22:03:33 2016 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c6OPK-0001Ua-4R for ged-emacs-devel@m.gmane.org; Mon, 14 Nov 2016 22:03:14 +0100 Original-Received: from localhost ([::1]:42592 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6OPN-0006fo-F3 for ged-emacs-devel@m.gmane.org; Mon, 14 Nov 2016 16:03:17 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:42223) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6OPG-0006fW-5F for emacs-devel@gnu.org; Mon, 14 Nov 2016 16:03:11 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6OPE-000880-P2 for emacs-devel@gnu.org; Mon, 14 Nov 2016 16:03:10 -0500 Original-Received: from limerock04.mail.cornell.edu ([128.84.13.244]:55114) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6OPA-00087F-9I; Mon, 14 Nov 2016 16:03:04 -0500 X-CornellRouted: This message has been Routed already. Original-Received: from authusersmtp.mail.cornell.edu (granite4.serverfarm.cornell.edu [10.16.197.9]) by limerock04.mail.cornell.edu (8.14.4/8.14.4_cu) with ESMTP id uAEL31J9019884; Mon, 14 Nov 2016 16:03:02 -0500 Original-Received: from [192.168.1.9] (mta-68-175-148-36.twcny.rr.com [68.175.148.36] (may be forged)) (authenticated bits=0) by authusersmtp.mail.cornell.edu (8.14.4/8.12.10) with ESMTP id uAEL3023004354 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT); Mon, 14 Nov 2016 16:03:00 -0500 In-Reply-To: <20161114170952.6729A220179@vcs.savannah.gnu.org> X-PMX-Cornell-Gauge: Gauge=XXXXX X-PMX-CORNELL-AUTH-RESULTS: dkim-out=none; X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 128.84.13.244 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:209394 Archived-At: 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