On 11/11/2016 3:27 AM, Eli Zaretskii wrote:
> Thanks, please see a few comments below.
>
>> +			  (if (and (file-name-case-insensitive-p
>> +				    (expand-file-name (car fn-list)))
>
> You shouldn't need to expand-file-name here, as all primitives do that
> internally.  (Yours didn't, but that's a mistake, see below.)

Fixed.

> What about filesystems mounted on Posix hosts, like Samba, NFS-mounted
> Windows volumes, etc. -- do those behave as case-sensitive or not?  If
> the latter, can that be detected?  Just asking, this shouldn't hold
> the patch, unless incorporating that is easy (or you feel like it even
> if it isn't ;-).

I don't have an immediate idea, but I added a "FIXME" comment and will 
think about it.

>> +DEFUN ("file-name-case-insensitive-p", Ffile_name_case_insensitive_p,
>> +       Sfile_name_case_insensitive_p, 1, 1, 0,
>> +       doc: /* Return t if file FILENAME is on a case-insensitive filesystem.
>> +The arg must be a string.  */)
>> +  (Lisp_Object filename)
>> +{
>> +  CHECK_STRING (filename);
>> +  return file_name_case_insensitive_p (SSDATA (filename)) ? Qt : Qnil;
>> +}
>
> This "needs work"™.  First, it should call expand-file-name, as all
> file-related primitives do.  Second, it should see if there's a file
> handler for this operation, and if so, call it instead (Michael,
> please take note ;-).  Finally, it should run the expanded file name
> through ENCODE_FILE before it calls file_name_case_insensitive_p, and
> pass to the latter the result of the encoding, otherwise the call to
> getattrlist will most certainly fail in some cases.

Fixed.

>>  DEFUN ("rename-file", Frename_file, Srename_file, 2, 3,
>>         "fRename file: \nGRename %s to file: \np",
>>         doc: /* Rename FILE as NEWNAME.  Both args must be strings.
>> @@ -2250,12 +2301,11 @@ This is what happens in interactive use with M-x.  */)
>>    file = Fexpand_file_name (file, Qnil);
>>
>>    if ((!NILP (Ffile_directory_p (newname)))
>> -#ifdef DOS_NT
>> -      /* If the file names are identical but for the case,
>> -	 don't attempt to move directory to itself. */
>> -      && (NILP (Fstring_equal (Fdowncase (file), Fdowncase (newname))))
>> -#endif
>> -      )
>> +      /* If the filesystem is case-insensitive and the file names are
>> +	 identical but for the case, don't attempt to move directory
>> +	 to itself.  */
>> +      && (NILP (Ffile_name_case_insensitive_p (file))
>> +	  || NILP (Fstring_equal (Fdowncase (file), Fdowncase (newname)))))
>
> This should call file_name_case_insensitive_p, and pass it the encoded
> file names.  That's because the file handlers for rename-file were
> already called, so we are now sure the file doesn't have any handlers.

Actually, the handlers haven't been called yet at this point in the 
code, so I left this one alone.

>> @@ -2276,14 +2326,12 @@ This is what happens in interactive use with M-x.  */)
>>    encoded_file = ENCODE_FILE (file);
>>    encoded_newname = ENCODE_FILE (newname);
>>
>> -#ifdef DOS_NT
>> -  /* If the file names are identical but for the case, don't ask for
>> -     confirmation: they simply want to change the letter-case of the
>> -     file name.  */
>> -  if (NILP (Fstring_equal (Fdowncase (file), Fdowncase (newname))))
>> -#endif
>> -  if (NILP (ok_if_already_exists)
>> -      || INTEGERP (ok_if_already_exists))
>> +  /* If the filesystem is case-insensitive and the file names are
>> +     identical but for the case, don't ask for confirmation: they
>> +     simply want to change the letter-case of the file name.  */
>> +  if ((NILP (Ffile_name_case_insensitive_p (file))
>
> Likewise here.

Fixed.

> Finally, please provide a NEWS entry for the new primitive and its
> documentation in the ELisp manual.

Done, but I'm not sure I found the best place for it in the manual.

Thanks for the review.  Revised patch attached.

Ken