bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

classic Classic list List threaded Threaded
40 messages Options
12
Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Juri Linkov-2
Severity: wishlist

For example, typing 'C-x v L' (vc-print-root-log) outside of a project
displays the minibuffer with the prompt "Directory for VC revision log: ".
It should provide a list of the default values from the project list
saved in ~/.emacs.d/projects



Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Dmitry Gutov
On 21.06.2020 02:51, Juri Linkov wrote:

>>> Maybe something like:
>>
>> Not too bad.
>>
>> Though I'd rather not extend the public contract of project.el with
>> a function that special-cases VC projects.
>>
>> So maybe something like this instead:
>>
>> +;; Or use project-try-vc after all. But this should be faster in the
>> +;; event when there actually are non-VC based projects in the list.
>> +(defun vc--known-vc-roots ()
>> +  (require 'project)
>> +  (defvar project--list)
>> +  (project--ensure-read-project-list)
>
> Calling internal project.el functions from vc.el?  Really?

The alternative would be to create a function like that in project.el.

But I hesitate to make it public, sorry.

>> Personally, though, when I want behavior like this, I would probably just
>> type 'C-x p v'.
>
> 'C-x p v' is not a replacement for 'C-x v L'.

'C-x p v L', then?

>> The directory name reading with completion performed by
>> project-prompt-project-dir is more quick and handy (though I'll confess
>> to using Ivy as the completion UI for this and one other function; vertical
>> completion fits these long string values best).
>
> 'M-n' works fine without Ivy to select a recent project dir.

Just saying what works better for me.

>> It also puts the selected project on the top of the list, which
>> vc--known-vc-roots (or your function) don't do.
>
> I don't understand what is the selected project.  The current project?

When you select a project to use, the prompting subroutine
(project-prompt-project-dir) re-sorts the saved list of project. It is
somewhat of an argument to only go through this UI when one needs to
select a project to use.

> Then neither 'C-x p v', nor 'C-x v L' should ask for a project directory
> when called from default-directory of the current project.

Not sure what you mean. I think they don't? When called inside a project.



Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Juri Linkov-2
>>> +;; Or use project-try-vc after all. But this should be faster in the
>>> +;; event when there actually are non-VC based projects in the list.
>>> +(defun vc--known-vc-roots ()
>>> +  (require 'project)
>>> +  (defvar project--list)
>>> +  (project--ensure-read-project-list)
>>
>> Calling internal project.el functions from vc.el?  Really?
>
> The alternative would be to create a function like that in project.el.
>
> But I hesitate to make it public, sorry.

Why not?

We have to decide which of them should be dependent.  It would be fine
either way: project.el to use vc.el public functions, or vice versa.

>>> Personally, though, when I want behavior like this, I would probably just
>>> type 'C-x p v'.
>> 'C-x p v' is not a replacement for 'C-x v L'.
>
> 'C-x p v L', then?

'C-x v L' needs to provide a list of recently used repositories.
Actually, this feature doesn't depend on project.el, so recently
used repositories could be recorded independently without project.el.
But it would be nice to share these directories between project.el and vc.el.



Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Dmitry Gutov
On 22.06.2020 01:49, Juri Linkov wrote:

>>>> +;; Or use project-try-vc after all. But this should be faster in the
>>>> +;; event when there actually are non-VC based projects in the list.
>>>> +(defun vc--known-vc-roots ()
>>>> +  (require 'project)
>>>> +  (defvar project--list)
>>>> +  (project--ensure-read-project-list)
>>>
>>> Calling internal project.el functions from vc.el?  Really?
>>
>> The alternative would be to create a function like that in project.el.
>>
>> But I hesitate to make it public, sorry.
>
> Why not?

Because I'd rather not have external code depend on what exact kind of
project backend is in use.

> We have to decide which of them should be dependent.  It would be fine
> either way: project.el to use vc.el public functions, or vice versa.

There are other private functions shared between project.el and xref.el.
So I this is fine, kind of.

But you have noted an alternative option below:

>>>> Personally, though, when I want behavior like this, I would probably just
>>>> type 'C-x p v'.
>>> 'C-x p v' is not a replacement for 'C-x v L'.
>>
>> 'C-x p v L', then?
>
> 'C-x v L' needs to provide a list of recently used repositories.

If you like.

> Actually, this feature doesn't depend on project.el, so recently
> used repositories could be recorded independently without project.el.

This is the alternative option. It would create a yet another file in
the user's dir, though.

> But it would be nice to share these directories between project.el and vc.el.

Perhaps there could be a public function in project.el called
project-known-roots.



Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Juri Linkov-2
>> But it would be nice to share these directories between project.el and vc.el.
>
> Perhaps there could be a public function in project.el called
> project-known-roots.

Like this?


diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 2f213dab8b..f489145e92 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -935,6 +935,12 @@ project-prompt-project-dir
         (read-directory-name "Select directory: " default-directory nil t)
       pr-dir)))
 
+;;;###autoload
+(defun project-known-roots ()
+  "Return a list of known roots."
+  (project--ensure-read-project-list)
+  (mapcar #'project-try-vc (mapcar #'car project--list)))
+
 
 ;;; Project switching
 
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 9b12d44978..cc83d9a7a1 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2547,7 +2555,10 @@ vc-print-root-log
  rootdir)
     (if backend
  (setq rootdir (vc-call-backend backend 'root default-directory))
-      (setq rootdir (read-directory-name "Directory for VC revision log: "))
+      (setq rootdir (read-directory-name "Directory for VC revision log: " nil
+                                         (mapcan (lambda (r) (when (eq (car r) 'vc)
+                                                               (list (cdr r))))
+                                                 (project-known-roots))))
       (setq backend (vc-responsible-backend rootdir))
       (unless backend
         (error "Directory is not version controlled")))
Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Juri Linkov-2
tags 41821 fixed
close 41821 28.0.50
quit

> Close.

I interpreted this as a verb in the imperative mood, so closed the report :)

> But without mapping through project-try-vc. The function will return _all_
> known roots, in VC repositories or not.
>
> vc-print-root-log can call it with seq-filter (project-try-vc is a public
> function). No need to deconstruct the returned values either.

Now pushed to master with these fixes.  Thanks for the help.



Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Eli Zaretskii
> From: Juri Linkov <[hidden email]>
> Date: Wed, 24 Jun 2020 02:59:49 +0300
> Cc: [hidden email]
>
> I interpreted this as a verb in the imperative mood, so closed the report :)
>
> > But without mapping through project-try-vc. The function will return _all_
> > known roots, in VC repositories or not.
> >
> > vc-print-root-log can call it with seq-filter (project-try-vc is a public
> > function). No need to deconstruct the returned values either.
>
> Now pushed to master with these fixes.  Thanks for the help.

This changeset causes byte-compilation warnings:

  In end of data:
  vc/vc-hooks.el:1013:1: Warning: the function `project-known-roots' is not
      known to be defined.

More importantly, this is not very clean, IMO: vc-hooks.el is
preloaded, so any attempt to call a project.el function in it runs a
very real risk to force loading project.el, something that we didn't
yet decide to do.

Why does this have to be in vc-hooks.el, and if it has to be there,
can it please not call functions that re not preloaded?

Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Eli Zaretskii
> From: "Basil L. Contovounesios" <[hidden email]>
> Cc: Juri Linkov <[hidden email]>,  [hidden email],  [hidden email]
> Date: Wed, 24 Jun 2020 15:52:20 +0100
>
> >   In end of data:
> >   vc/vc-hooks.el:1013:1: Warning: the function `project-known-roots' is not
> >       known to be defined.
>
> Since project-known-roots is autoloaded, will this go away when
> ldefs-boot.el is updated?

You mean loaddefs.el, I presume (ldefs-boot is rarely used during the
build).

Probably, but that's not my main problem with that change.



Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Eli Zaretskii
In reply to this post by Eli Zaretskii
> Cc: [hidden email]
> From: Dmitry Gutov <[hidden email]>
> Date: Wed, 24 Jun 2020 18:10:52 +0300
>
> On 24.06.2020 17:39, Eli Zaretskii wrote:
> > vc-hooks.el is
> > preloaded, so any attempt to call a project.el function in it runs a
> > very real risk to force loading project.el, something that we didn't
> > yet decide to do.
>
> I don't quite understand. Is it a problem to call a non-preloded
> function at runtime?

As I tried to explain, I'd like to avoid calling such functions from
preloaded code.

Is that possible, please?



Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Dmitry Gutov
On 24.06.2020 18:42, Eli Zaretskii wrote:
> So is it possible to have vc-known-roots defined in vc.el instead of
> in vc-hooks.el?

Okay, but that new function delegates to code in project.el anyway, what
would be the practical difference?

OTOH, if you just said we shouldn't use project.el in vc/*, I could
understand that.



Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Eli Zaretskii
> Cc: [hidden email], [hidden email]
> From: Dmitry Gutov <[hidden email]>
> Date: Wed, 24 Jun 2020 21:13:07 +0300
>
> On 24.06.2020 18:42, Eli Zaretskii wrote:
> > So is it possible to have vc-known-roots defined in vc.el instead of
> > in vc-hooks.el?
>
> Okay, but that new function delegates to code in project.el anyway, what
> would be the practical difference?

It would be somewhat cleaner, I think.  Doesn't vc.el already have
some calls to project.el?

> OTOH, if you just said we shouldn't use project.el in vc/*, I could
> understand that.

That'd be unreasonably harsh, I think.

Actually, I have a question: isn't project.el conceptually a
higher-level feature than VC?  If so, how come VC wants to call
project.el?



Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Dmitry Gutov
In reply to this post by Juri Linkov-2
On 24.06.2020 02:59, Juri Linkov wrote:

>> Close.
>
> I interpreted this as a verb in the imperative mood, so closed the report :)

A bit premature: both the name and the docstring need to be changed. And
the return value currently returns a private structure, instead of a
list of strings, as promised by the name.

But since we might revert this commit either way, let's shelve
discussing these details for now.



Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Dmitry Gutov
In reply to this post by Eli Zaretskii
On 24.06.2020 21:29, Eli Zaretskii wrote:

>> Cc: [hidden email], [hidden email]
>> From: Dmitry Gutov <[hidden email]>
>> Date: Wed, 24 Jun 2020 21:13:07 +0300
>>
>> On 24.06.2020 18:42, Eli Zaretskii wrote:
>>> So is it possible to have vc-known-roots defined in vc.el instead of
>>> in vc-hooks.el?
>>
>> Okay, but that new function delegates to code in project.el anyway, what
>> would be the practical difference?
>
> It would be somewhat cleaner, I think.

That's the whole reason? I mean, I'm not going to protest against an
extra wrapper, but that doesn't sound like it would solve any practical
problems. "Cleaner" solutions often have those.

> Doesn't vc.el already have some calls to project.el?

Nope.

>> OTOH, if you just said we shouldn't use project.el in vc/*, I could
>> understand that.
>
> That'd be unreasonably harsh, I think.

But that would be a limitation I could understand (don't use
non-preloaded code from preloaded code, period).

> Actually, I have a question: isn't project.el conceptually a
> higher-level feature than VC?  If so, how come VC wants to call
> project.el?

VC doesn't serve project.el only. project.el doesn't solely use VC.

Apparently Juri wants to use certain data collected and saved by
project.el UI, for convenience.

The alternative would be to introduce some separate history-keeping
feature for the cases when VC code needs to ask the user to point to a
VC repository.



Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Juri Linkov-2
> Apparently Juri wants to use certain data collected and saved by project.el
> UI, for convenience.

This feature was implemented to address Stephen's concerns in
https://debbugs.gnu.org/38044#233

> The alternative would be to introduce some separate history-keeping feature
> for the cases when VC code needs to ask the user to point to
> a VC repository.

It wouldn't be rational to duplicate this feature.
Rather I think the project-list should be updated
even on using vc commands such as 'C-x v d', i.e.
project and vc should be more tightly integrated.



Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Eli Zaretskii
In reply to this post by Dmitry Gutov
> Cc: [hidden email], [hidden email]
> From: Dmitry Gutov <[hidden email]>
> Date: Wed, 24 Jun 2020 21:44:01 +0300
>
> >> Okay, but that new function delegates to code in project.el anyway, what
> >> would be the practical difference?
> >
> > It would be somewhat cleaner, I think.
>
> That's the whole reason?

Well, the whole issue at hand is a rather minor one.

> I mean, I'm not going to protest against an
> extra wrapper, but that doesn't sound like it would solve any practical
> problems. "Cleaner" solutions often have those.

In general, code that doesn't _have_ to be preloaded, shouldn't be.
If nothing else, it keeps the memory footprint of a bare Emacs
smaller, and thus prevents us from slipping down the slippery slope of
memory bloat.

> > Actually, I have a question: isn't project.el conceptually a
> > higher-level feature than VC?  If so, how come VC wants to call
> > project.el?
>
> VC doesn't serve project.el only. project.el doesn't solely use VC.

Yes, but that's not what I asked.  I have the impression that
project.el builds on VC as one project back-end, so it sounds strange
to me that VC turns around and calls project.el for something.

> Apparently Juri wants to use certain data collected and saved by
> project.el UI, for convenience.

After reading the original complaint that Juri says he wanted to
resolve, I still don't understand why we use project.el for that.  No
one says that every relevant VC repository must have been visited as a
project as part of the current Emacs session.  Why not have some
relevant history in VC itself?

> The alternative would be to introduce some separate history-keeping
> feature for the cases when VC code needs to ask the user to point to a
> VC repository.

Exactly.  Why not?

Juri answers:

> It wouldn't be rational to duplicate this feature.
> Rather I think the project-list should be updated
> even on using vc commands such as 'C-x v d', i.e.
> project and vc should be more tightly integrated.

I don't think I agree.  First, I don't see any duplication here:
people could (and do) use VC directly, not through project.el, in
which case project.el history wouldn't help.  Moreover, using a
command such as "C-x v d" doesn't mean I want project.el record that,
again because I might be using project.el commands for projects that
aren't based on VC.

And if the history is collected by VC, it could be made available to
project.el commands that call into VC, right?

Anyway, if you-two feel strongly about keeping the current solution,
i.e. having VC commands use project.el-collected history, I'd
appreciate if that function could be moved to vc.el from vc-hooks.el,
thanks in advance.



Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Eli Zaretskii
> Cc: [hidden email], [hidden email]
> From: Dmitry Gutov <[hidden email]>
> Date: Thu, 25 Jun 2020 16:50:24 +0300
>
> > In general, code that doesn't _have_ to be preloaded, shouldn't be.
> > If nothing else, it keeps the memory footprint of a bare Emacs
> > smaller, and thus prevents us from slipping down the slippery slope of
> > memory bloat.
>
> And having vc-hooks call project.el functions at runtime would somehow
> force us to preload it? How?

It's enough that someone makes the function a macro, or does something
else similarly innocent.

> > Anyway, if you-two feel strongly about keeping the current solution,
> > i.e. having VC commands use project.el-collected history, I'd
> > appreciate if that function could be moved to vc.el from vc-hooks.el,
> > thanks in advance.
>
> We can't just move it: it accesses information private to project.el.
> The best we could do is a wrapper function.

Sorry, I don't understand: why can't we move it from vc-hooks.el to
vc.el, and why doing that would need a wrapper?




Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Dmitry Gutov
On 25.06.2020 19:31, Eli Zaretskii wrote:

>> And having vc-hooks call project.el functions at runtime would somehow
>> force us to preload it? How?
>
> It's enough that someone makes the function a macro, or does something
> else similarly innocent.

Nobody will make it a macro, or a defsubst. And I wouldn't say making it
a macro would be "innocent".

>>> Anyway, if you-two feel strongly about keeping the current solution,
>>> i.e. having VC commands use project.el-collected history, I'd
>>> appreciate if that function could be moved to vc.el from vc-hooks.el,
>>> thanks in advance.
>>
>> We can't just move it: it accesses information private to project.el.
>> The best we could do is a wrapper function.
>
> Sorry, I don't understand: why can't we move it from vc-hooks.el to
> vc.el, and why doing that would need a wrapper?

It would be a wrapper for a project.el function. But if it works for
you, okay.



Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Eli Zaretskii
> Cc: [hidden email], [hidden email]
> From: Dmitry Gutov <[hidden email]>
> Date: Thu, 25 Jun 2020 20:19:27 +0300
>
> > We are going in circles again.  Can you explain why vc-known-roots
> > cannot be moved to vc.el in its entirety?
>
> To re-iterate:
>
> - We can't just move it: it accesses information private to project.el.

What does project.el has to do with this?  I didn't ask to move the
function to project.el.  What am I missing?

> - We can't move said information (the variable and mechanism to
> save/restore it)

Which information? what variable?  Please be more specific.



Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Dmitry Gutov
On 25.06.2020 20:45, Eli Zaretskii wrote:
>> To re-iterate:
>>
>> - We can't just move it: it accesses information private to project.el.
> What does project.el has to do with this?  I didn't ask to move the
> function to project.el.  What am I missing?

You asked to move it *from* project.el.

If that sounds confusing, perhaps try to answer this question: which
function do you want to see moved?

>> - We can't move said information (the variable and mechanism to
>> save/restore it)
> Which information? what variable?  Please be more specific.

project--list.



Reply | Threaded
Open this post in threaded view
|

bug#41821: 28.0.50; read-directory-name in vc commands should provide defaults from projects

Eli Zaretskii
> Cc: [hidden email], [hidden email]
> From: Dmitry Gutov <[hidden email]>
> Date: Thu, 25 Jun 2020 20:50:47 +0300
>
> On 25.06.2020 20:45, Eli Zaretskii wrote:
> >> To re-iterate:
> >>
> >> - We can't just move it: it accesses information private to project.el.
> > What does project.el has to do with this?  I didn't ask to move the
> > function to project.el.  What am I missing?
>
> You asked to move it *from* project.el.

This is a huge misunderstanding.

> If that sounds confusing, perhaps try to answer this question: which
> function do you want to see moved?

As I said before: I'm talking about moving vc-known-roots from
vc-hooks.el to vc.el.  Quoting myself:

> > We are going in circles again.  Can you explain why vc-known-roots
> > cannot be moved to vc.el in its entirety?



12