bug#56682: locked narrowing

[Eli Zaretskii]

> Date: Sat, 26 Nov 2022 16:15:34 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: monnier@iro.umontreal.ca, 56682@debbugs.gnu.org, dgutov@yandex.ru
> 
> >> +(defun with-narrowing-1 (start end tag body)
> >> +(defun with-narrowing-2 (start end body)
> >
> > Should these two helpers be internal functions?
> >
> 
> You mean, they should be called with-narrowing--{1,2} (or perhaps 
> with--narrowing-{1,2})?

I actually think these should not be called with-SOMETHING, since they
aren't macros.  And yes, the function names should include "--".

> >> +/* Remove the innermost lock in BUF from the narrowing_lock alist.  */
> >>  static void
> >> -unwind_locked_zv (Lisp_Object point_max)
> >> +narrowing_lock_pop (Lisp_Object buf)
> >>  {
> >> -  SET_BUF_ZV (current_buffer, XFIXNUM (point_max));
> >> +  Lisp_Object buffer_locks = assq_no_quit (buf, narrowing_locks);
> >> +  eassert (! NILP (buffer_locks));
> >
> > Why this assertion?  There's no similar assertions in other functions 
> > that deal with narrowing_locks.
> >
> 
> Because 'pop' on an empty stack is an error

You don't need to pop, just return without doing anything.

> and (more importantly) it would mean that the narrowing_locks alist is
> corrupted.  The cdr's in that alist should never be nil.

A comment to that effect should be in order, then.

> >> +static void
> >> +narrowing_locks_restore (Lisp_Object buf_and_saved_locks)
> >> +{
> >> +  if (NILP (buf_and_saved_locks))
> >> +    return;
> >> +  Lisp_Object buf = Fcar (buf_and_saved_locks);
> >> +  eassert (BUFFERP (buf));
> >> +  Lisp_Object saved_locks = Fcdr (buf_and_saved_locks);
> >> +  eassert (! NILP (saved_locks));
> >
> > Again, I don't understand the need for an assertion here.  Just return 
> > if saved_locks is nil.
> >
> 
> Strictly speaking there is no need for an assertion, but again it would 
> mean that something that isn't supposed to happen happened.  In this case 
> it would mean that narrowing_locks_restore is called with a list of length 
> 1, containing only a buffer, whereas it is supposed to be called with the 
> return value of narrowing_locks_save, which is either nil or a list of 
> length >= 2.

I don't think I understand (you avoid the assertion in other cases which
look similar to me), but if there are good reasons, please add comments
there explaining the rationale.

> > What about some tests?
> 
> I'd have to add some tests, indeed.  Not sure I'll have time to do that 
> today.

We can add the tests after the branch is cut, I just wanted to be sure we
will have them eventually.

Thanks.