bug#58784: 28.2; project-buffers incorrect under let-bound default-directory

[Sean Devlin]

Hi Dmitry,

> On Nov 1, 2022, at 7:35 PM, Dmitry Gutov <dgutov@yandex.ru> wrote:
> 
> Hi Sean,
> 
> On 31.10.2022 19:47, Sean Devlin wrote:
> 
>>>> In the last form, project-buffers includes the current buffer (i.e. the
>>>> scratch buffer in our example) with the results. (This is true even if
>>>> the current buffer is visiting a file in some unrelated directory.)
>>>> This matters because the command project-switch-project let-binds
>>>> default-directory before calling project-switch-commands. This means
>>>> that if you set project-switch-commands to some function that calls
>>>> project-buffers, you will get incorrect results.
>>>> For example, evaluate the following forms in order:
>>>> (defun my-list-project-buffers ()
>>>>   "List the current project's buffers."
>>>>   (interactive)
>>>>   (let ((buffer-list (project-buffers (project-current t)))
>>>>    (buffer-name (project-prefixed-buffer-name "my-project-buffer-list")))
>>>>     (with-current-buffer (get-buffer-create buffer-name)
>>>>       (erase-buffer)
>>>>       (save-excursion
>>>>    (dolist (buffer buffer-list)
>>>>      (insert (buffer-name buffer))
>>>>      (insert ?\n))))
>>>>     (switch-to-buffer buffer-name)))
>>>> (setq project-switch-commands #'my-list-project-buffers)
>>> 
>>> Looks like a reimplementation of projectile-ibuffer, seems useful.
>> Yeah, in my real configuration, I defined this as an ibuffer filter, but I 
>> thought a simpler example would be better for the bug report.
> 
> Right, thanks.
> 
>>>> ;; list tmpfile but also scratch
>>>> (project-switch-project "/tmp/")
>>> 
>>> Not sure how to fix this, though. In bug#53626 we discussed a somewhat 
>>> similar problem, and a let-binding seems impossible to "escape".
>>> 
>>> What else can we do? One option is to change the signature of every 
>>> compatible command to take the project object as its first argument. Might 
>>> have been more realistic when the package was first written, too much 
>>> breakage now, probably.
>>> 
>>> Another would be to add a new var to help override the project choice 
>>> without touch default-directory.
>>> 
>>> Something like the attached. Please try it 
>>> out.<project-current-directory-override.diff>
>> I agree, the fix is not obvious.
>> A workaround I used in my configuration is to add an advice to 
>> project-switch-project to wrap it in a with-temp-buffer form. This way, it’s 
>> only the default-directory of the temp buffer that gets corrupted. Not a 
>> very clean solution, but it seems to work. Unfortunately, you need to 
>> remember to do this in many places; for example, I had to do the same thing 
>> in my project-ibuffer command.
>> Your solution looks cleaner. I gave it a try (along with disabling my 
>> advice), and it seems to work pretty well. Thanks for the fix!
> 
> Actually I like your solution better. It might be less obvious when reading, 
> but it's shorter and fully backward compatible.
> 
> So I just pushed that fix to master.

Great, thanks!

> 
>> There might be some more places where it needs to be applied. For example, 
>> project-prefixed-buffer-name still inspects the default-directory. (Maybe 
>> project-prefixed-buffer-name should just call project-root or similar?)
> 
> In both places where project-prefixed-buffer-name is currently invoked, 
> default-directory is set a specific binding just before that. So I think it 
> would have been fine even with the other fix.
> 
> Or on a higher level, it acts on the current buffer, so referencing 
> default-directory seems okay.

Sounds good.

> 
>> I think there’s still some fragility in the project-buffers function, since 
>> any callers need to be careful not to bind default-directory. It might be 
>> useful to call this out in the doc string or in the manual.
> 
> I suppose it could use improvement, but I'm not sure what phrasing would stop 
> someone from making such a mistake. After all, I knew its implementation and 
> made it anyway.
> 
> Perhaps the docstring should simply say that the buffers are matched on the 
> basis of their default-directory value. In the default implementation, that 
> is (custom backends could choose their own strategy). Would that help?

Yeah, I think a high-level description of the default strategy would be useful.

Thanks again for your help!