bug#67687: Feature request: automatic tags management

[Dmitry Gutov]

On 30/12/2023 09:33, Eli Zaretskii wrote:
> > Date: Sat, 30 Dec 2023 05:05:01 +0200
> > Cc: 67687@debbugs.gnu.org, eskinjp@gmail.com, stefankangas@gmail.com
> > From: Dmitry Gutov <dmitry@gutov.dev>
> >
> > > > +(defcustom etags-regen-tags-file "TAGS"
> > > > +  "Name of the tags file to create inside the project.
> > >
> > > This and the other defcustom's here should say in the first line of
> > > the doc string that they are for etags-regen-mode.  This will help
> > > discoverability and also produce a more helpful display with the
> > > various apropos commands.
> >
> > I've tried, but it seems hard to fit into most of them while keeping to
> > the requisite max number of columns. Only managed to fit that into
> > etags-regen-program and etags-regen-file-extensions.
> >
> > TBH, most of the time it would seem superfluous, given the namespaced
> > names. But it's probably good to mention in 'etags-regen-program', on
> > balance.
>
> I suggest some minor improvements in this area below.

All right.

> > > > +;;;###autoload
> > > > +(put 'etags-regen-file-extensions 'safe-local-variable
> > > > +     (lambda (value) (and (listp value) (seq-every-p #'stringp value))))
> > >
> > > Why not use list-of-strings-p here?
> >
> > Again, that "core ELPA" consideration. We could deploy this feature to a
> > number of released Emacs versions, if we don't introduce such dependencies.
>
> Isn't this covered by the compat package on ELPA?  If not, I think it
> should be.

These forms go into generated autoloads file for each installed package 
(*-autoloads.el). I think compat doesn't make list-of-string-p 
autoloaded, and autoloads files don't usually have (require ...) forms.

So while I haven't really tested this and could be missing something, it 
seems brittle to rely on 'compat' for this function (if at all possible).

> > > > +     (lambda (f) (or (not (string-match-p match-re f))
> > > > +                (string-match-p "/\\.#" f)
> > >
> > > Is that '/' there to detect regexps for absolute file names?  If so,
> > > that won't work for Windows.
> >
> > It's to detect backup files.
>
> Can you add a comment there to that effect?

Added.

> > > > +(defun etags-regen--ignore-regexp (ignore)
> > > > +  (require 'dired)
> > > > +  ;; It's somewhat brittle to rely on Dired.
> > > > +  (let ((re (dired-glob-regexp ignore)))
> > > > +    ;; We could implement root anchoring here, but \\= doesn't work in
> > > > +    ;; string-match :-(.
> > > > +    (concat (unless (eq ?/ (aref re 3)) "/")
> > > > +            ;; Cutting off the anchors.
> > > > +            (substring re 2 (- (length re) 2))
> > > > +            (unless (eq ?/ (aref re (- (length re) 3)))
> > > > +              ;; Either match a full name segment, or eos.
> > > > +              "\\(?:/\\|\\'\\)"))))
> > >
> > > Same here: what is the purpose of comparisons with a slash?  I think
> > > we need some more comments there explaining the logic of the code.
> >
> > We compare with a slash to see whether the glob was matching against a
> > directory (in which case it's already anchored to the name of a file
> > name segment), otherwise we add such anchoring to either the end of a
> > file name segment or eos (thus allowing a glob match both directory
> > names and file names).
> >
> > Added a shorter comment saying the same.
>
> Thanks, but I miss in that comment explanations of the "magic"
> constants 2 and 3.  Could we add that, please?

2 is the length of both anchors, 3 is the index of the character right 
after the anchor. There is already a comment about cutting off the 
anchors, I've expanded it a bit.

> > > > +(defun etags-regen--append-tags (&rest file-names)
> > > > +  (goto-char (point-max))
> > > > +  (let ((options (etags-regen--build-program-options (etags-regen--ctags-p)))
> > > > +        (inhibit-read-only t))
> > > > +    ;; XXX: call-process is significantly faster, though.
> > > > +    ;; Like 10ms vs 20ms here.
> > > > +    (shell-command
> > > > +     (format "%s %s %s -o -"
> > > > +             etags-regen-program (mapconcat #'identity options " ")
> > > > +             (mapconcat #'identity file-names " "))
> > > > +     t etags-regen--errors-buffer-name))
> > >
> > > Should we indeed use call-process?
> >
> > Something for later improvement.
> >
> > Looking at the code, I believe I decided to use 'shell-command' for the
> > first version because of how easy it makes to output stderr to a
> > separate buffer. call-process only offers writing them to a file.
>
> How about mentioning this issue in that comment?

Added.

> > +(defcustom etags-regen-tags-file "TAGS"
> > +  "Name of the tags file to create inside the project.
>
> I suggest
>
>    Name of the tags file to create inside the project by `etags-regen-mode'.
>
> > +(defcustom etags-regen-program-options nil
> > +  "List of additional options to pass to the etags program."
>
> I suggest
>
>    List of additional options for etags program invoked by `etags-regen-mode'.
>
> > +(defcustom etags-regen-regexp-alist nil
> > +  "Mapping of languages to additional regexps for tags.
>
> I suggest
>
>    Mapping of languages to etags regexps for `etags-regen-mode'.
>
> > +(define-minor-mode etags-regen-mode
> > +  "Generate and update the tags automatically.
>
> I suggest
>
>    Minor mode to automatically generate and update tags tables.

Replaced, thanks.

Latest revision attached. Any further comments?

etags-regen-v5.diff
Description: Text Data