bug#67687: Feature request: automatic tags management

[Dmitry Gutov]

On 31/12/2023 03:02, Stefan Kangas wrote:
> My review below.  I wasn't paying attention to the full discussion, so
> please just ignore any points that you have already discussed with Eli.

Thanks.

> Dmitry Gutov <dmitry@gutov.dev> writes:
>
> > diff --git a/etc/NEWS b/etc/NEWS
> > index f82564946b7..6d6bca187de 100644
> > --- a/etc/NEWS
> > +++ b/etc/NEWS
> > @@ -1243,6 +1243,11 @@ the needs of users with red-green or blue-yellow color 
> > deficiency.
> >   The Info manual "(modus-themes) Top" describes the details and
> >   showcases all their customization options.
> >
> > +** New global minor mode 'etags-regen-mode'.
> > +This minor mode generates the tags table automatically based on the
> > +current project configuration, and later updates it as you edit the
> > +files and save the changes.
>
> Consider referring to the relevant section in the info manual.

I guess we'll add that reference later when/if we document it in the manual.

> > +(require 'cl-lib)
> > +
> > +(defgroup etags-regen nil
> > +  "Auto-(re)generating tags."
> > +  :group 'tools)
>
> How about:
>
>      "Auto-generate \"tags\" file."

Sorry, I don't understand the meaning of the quotes. The file name is 
TAGS, if we wanted to refer to it. You also dropped the bit in quotes 
that implies the automatic updates, too.

We can say "Auto-(re)generate tags file". Though knowing how the tags 
are stored might not be that necessary, if the generation and 
loading/refresh happens automatically.

> > +(defcustom etags-regen-tags-file "TAGS"
> > +  "Name of the tags file to create inside the project by `etags-regen-mode'.
> > +
> > +The value should either be a simple file name (no directory
> > +specified), or a function that accepts the project root directory
> > +and returns a distinct absolute file name for its tags file.  The
> > +latter possibility is useful when you prefer to store the tag
> > +files somewhere else, for example in `temporary-file-directory'."
> > +  :type '(choice (string :tag "File name")
> > +                 (function :tag "Function that returns file name"))
> > +  :version "30.1")
>
> Did you consider making it always store the TAGS files somewhere in
> `temporary-file-directory'?  They should be rather ephemereal in any
> case, I think?

This was my approach originally, but the downside was always having to 
generate the tags fully when Emacs is restarted. The current code also 
refreshes existing tags file when there are not too many changed files 
to process.

Now that etags-regen-tags-file can be a function that returns stable 
file names corresponding to project root even in /tmp, that feature can 
still work (until /tmp is cleared).

Whether that should be the default, I don't know. Perhaps people will 
like the easier access to the generated file that the current default 
allows.

> In that case, we could perhaps even ignore an existing TAGS file, if
> this mode is enabled.  Perhaps as an option.

This will happen if no tags file is visited and (funcall 
etags-regen-tags-file root) returns a different file.

> > +
> > +(defcustom etags-regen-program-options nil
> > +  "List of additional options for etags program invoked by `etags-regen-mode'."
> > +  :type '(repeat string)
> > +  :version "30.1")
>
> Perhaps add:
>
>      See the full list of options that `etags' accepts in Info node
>      `(emacs) Create Tags Table'.
>
> Should this be marked unsafe?  Actually, same question for all of the
> above defcustoms, given that we use `shell-command-to-string'.

Given that this option doesn't have the 'safe-local-variable' property, 
do we need to do something else?

> Speaking of which, should we have more `shell-quote-argument' below?
> I didn't look at every example, but maybe you did.

Indeed, the processing of etags-regen-program-options was missing the 
conversion through shell-quote-argument. In case, for example. someone 
adds an option with a value that includes a space.

> > +
> > +(defcustom etags-regen-regexp-alist nil
> > +  "Mapping of languages to etags regexps for `etags-regen-mode'.
> > +
> > +These regexps are used in addition to the tags made with the
> > +standard parsing based on the language.
> > +
> > +The value must be a list of conses (LANGUAGES . TAG-REGEXPS)
> > +where both car and cdr are lists of strings.
>
> I think that should better be:
>
>      where both LANGUAGES and TAG-REGEXPS are lists of strings.
>
> I'm not sure we should say "conses" there, or should we?  I think we
> usually prefer something like:
>
>      The value is a list where each element has the form
>      (LANGUAGES . TAG-REGEXPS)
>
> I think this is better because it sounds less foreign to random users
> with no background in ELisp.
>
> But Eli is much better than me at this.  :-)

Why not, I've replaced the text with your versions.

> > +
> > +Each language should be one of the recognized by etags, see
> > +`etags --help'.  Each tag regexp should be a string in the format
> > +as documented for the `--regex' arguments (without `{language}').
>     ^^
>
> Nit, but I think "as" could be scratched.

Ok.

> > +
> > +We currently support only Emacs's etags program with this option."
> > +  :type '(repeat
> > +          (cons
> > +           :tag "Languages group"
> > +           (repeat (string :tag "Language name"))
> > +           (repeat (string :tag "Tag Regexp"))))
> > +  :version "30.1")
> > +
> > +;;;###autoload
> > +(put 'etags-regen-regexp-alist 'safe-local-variable
> > +     (lambda (value)
> > +       (and (listp value)
> > +            (seq-every-p
> > +             (lambda (group)
> > +               (and (consp group)
> > +                    (listp (car group))
> > +                    (listp (cdr group))
> > +                    (seq-every-p #'stringp (car group))
> > +                    (seq-every-p #'stringp (cdr group))))
> > +             value))))
> > +
> > +;; We have to list all extensions: etags falls back to Fortran
> > +;; when it cannot determine the type of the file.
>
> (A battle-tested default, if nothing else.  ;-)

I'd rather etags skipped unknown files, but there is no such option.

> > +;;;###autoload
> > +(put 'etags-regen-file-extensions 'safe-local-variable
> > +     (lambda (value) (and (listp value) (seq-every-p #'stringp value))))
> > +
> > +;; FIXME: We don't support root anchoring yet.
>
> What is root anchoring?  Does it deserve a sentence that explains what
> it is?

It's the "to root an entry" thing described in the docstring for 
`project-ignores', which is referenced.

> > +(defvar etags-regen--errors-buffer-name "*etags-regen-tags-errors*")
> > +
> > +(defvar etags-regen--rescan-files-limit 100)
> > +
> > +(defun etags-regen--all-mtimes (proj)
> > +  (let ((files (etags-regen--all-files proj))
> > +        (mtimes (make-hash-table :test 'equal))
> > +        file-name-handler-alist)
> > +    (dolist (f files)
> > +      (condition-case nil
> > +          (puthash f
> > +                   (file-attribute-modification-time
> > +                    (file-attributes f))
> > +                   mtimes)
> > +        (file-missing nil)))
> > +    mtimes))
>
> Could we use file notifications for this?  Maybe as a future
> improvement.

File notifications are mentioned in the Commentary. Someday.

The thread about expected problems with our filenotify integration was 
sometime between these two, but I can't find it now :-(

https://lists.gnu.org/archive/html/emacs-devel/2018-01/msg00369.html

https://lists.gnu.org/archive/html/emacs-devel/2020-12/msg00680.html
https://lists.gnu.org/archive/html/emacs-devel/2021-01/msg00428.html

Perhaps filenotify-recursively will help 
(https://lists.gnu.org/archive/html/emacs-devel/2021-08/msg00876.html).

> Other than that, LGTM.
>
> Thanks again for working on this.

Thanks!

Next revision attached.

etags-regen-v6.diff
Description: Text Data