bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> If this is going to be called from C, it should probably use
> save-match-data, because no one will expect that just modifying a file
> from some Lisp program could clobber the match data.

Right; now added.

> Also, do we ever need this during loadup?  Because before files.el is
> loaded by loadup, this function will not be available, so
> unconditionally calling it from C without protection, not even
> safe_call or somesuch, is not safe.

I'll try doing a "make bootstrap"...

>> +static Lisp_Object
>> +make_lock_file_name (Lisp_Object fn)
>> +{
>> +  return ENCODE_FILE (call1 (intern ("make-lock-file-name"), fn));
>> +}
>
> I'd prefer not to have a function return an encoded file name, it's
> unusual and unexpected.  It is better to leave that to the caller.
> (And if you do that, the rationale for having a separate function for
> this will all but disappear, I think.)

Right.  But it seemed like all the callers wanted an encoded file name
here, so it was marginally cleaner.

>> -  fn = Fexpand_file_name (fn, Qnil);
>> +  fn = make_lock_file_name (Fexpand_file_name (fn, Qnil));
>
> In the original code, 'fn' was an un-encoded file name, but your
> changes made it encoded.  Why not keep the code more similar by having
> a separate variable with the encoded file name?  E.g., this would
> avoid potential trouble here:

Yup; I've now rewritten this to not reuse variables in this way, because
it was pretty confusing.  (See the version of the patch I posted some
minutes ago.)

>>    if (!NILP (subject_buf)
>>        && NILP (Fverify_visited_file_modtime (subject_buf))
>>        && !NILP (Ffile_exists_p (fn)) <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>
> Ffile_exists_p was passed an un-encoded file name in the original
> code.  It calls file handlers, and encodes local file names by itself,
> so it is better to pass it un-encoded file names.

[...]

> Same problem here: 'filename' is now an encoded file name, so you call
> report_file_errno with an encoded file name, which is a no-no.

Right; I'll fix up the un-encoded/encoded file name confusion here.

> Last, but not least: do we care that now locking a file will cons
> strings, even with the default value of lock-file-name-transforms?
> That sounds like we are punishing the vast majority of users for the
> benefit of the few who need the opt-in behavior.  Should we perhaps
> avoid consing strings, or maybe even calling Lisp altogether, under
> the default value of that option?

Hm...  I think for simplicity's sake, it makes sense to always call the
Lisp code.  Having two places where we insert ".#" into a file name just
seems error prone, long term.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no