Package: emacs;
Reported by: Sean Allred <allred.sean <at> gmail.com>
Date: Tue, 17 Sep 2024 16:57:02 UTC
Severity: normal
Tags: patch
Done: Dmitry Gutov <dmitry <at> gutov.dev>
Bug is archived. No further changes may be made.
Message #17 received at 73320 <at> debbugs.gnu.org (full text, mbox):
From: Sean Allred <allred.sean <at> gmail.com> To: Dmitry Gutov <dmitry <at> gutov.dev> Cc: 73320 <at> debbugs.gnu.org Subject: Re: bug#73320: [PATCH] project--vc-list-files: use Git's sparse-index Date: Wed, 18 Sep 2024 23:25:57 -0500
Dmitry Gutov <dmitry <at> gutov.dev> writes:
>>> Yeah, I expect project-find-regexp, project-search,
>>> project-query-replace-regexp might start misbehaving without
>>> additional filtering -- either throwing up errors or, best case,
>>> continuing to search through the "hidden" directories.
>> Not sure how best to track that we should come back to this, but
>> yeah.
>> It seems like the right place to add some sort of switch would be in the
>> `project-files` defmethod. From here, it looks like all the functions
>> you mention could choose the behavior right for them. (Based on the
>> function names alone -- it seems they would /also/ be interested in
>> operating on only those files which exist on disk.)
>
> I think we can just remove the names ending with '/'. The built-in
> commands don't seem to error out on them right now - probably because
> there is some protection against nonexistent files - but those files
> are (were) still shown as completions for project-find-file. Try out
> this addition please. The performance here seems about the same even
> with a large list (something I was worried about):
>
> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index b29d5ed5404..a2e3f3f52e6 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -663,7 +663,7 @@ project--vc-list-files
> (pcase backend
> (`Git
> (let* ((default-directory (expand-file-name
> (file-name-as-directory dir)))
> - (args '("-z"))
> + (args '("-z" "--sparse"))
> (vc-git-use-literal-pathspecs nil)
> (include-untracked (project--value-in-dir
> 'project-vc-include-untracked
> @@ -703,7 +703,8 @@ project--vc-list-files
> (delq nil
> (mapcar
> (lambda (file)
> - (unless (member file submodules)
> + (unless (or (member file submodules)
> + (eq ?/ (aref file (1- (length file)))))
> (if project-files-relative-names
> file
> (concat default-directory file))))
Works fine for me :-) Though I've added an additional version check
inlined below.
>> Incidentally looking at the version check within `project-files`, it's
>> worthwhile to point out that `--sparse` is likely /not/ compatible with
>> ancient versions of Git. [...]
>
> [...]
>
> We can call vc-git--program-version the same way it's used in
> vc-git-state. Which version should we make the minimum?
The `--sparse` option was introduced in 2.35. The following seems to
work well for me:
diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index b29d5ed5404..873bc92729d 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -663,7 +663,8 @@ project--vc-list-files
(pcase backend
(`Git
(let* ((default-directory (expand-file-name (file-name-as-directory dir)))
- (args '("-z"))
+ (args `("-z" ,@(when (version<= "2.35" (vc-git--program-version))
+ '("--sparse"))))
(vc-git-use-literal-pathspecs nil)
(include-untracked (project--value-in-dir
'project-vc-include-untracked
@@ -703,7 +704,8 @@ project--vc-list-files
(delq nil
(mapcar
(lambda (file)
- (unless (member file submodules)
+ (unless (or (member file submodules)
+ (eq ?/ (aref file (1- (length file)))))
(if project-files-relative-names
file
(concat default-directory file))))
Since we're getting a bit busy with our conditions, though, it might be
better to start using `cond`:
diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 873bc92729d..b42415154e3 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -704,11 +704,11 @@ project--vc-list-files
(delq nil
(mapcar
(lambda (file)
- (unless (or (member file submodules)
- (eq ?/ (aref file (1- (length file)))))
- (if project-files-relative-names
- file
- (concat default-directory file))))
+ (cond
+ ((member file submodules) nil)
+ ((eq ?/ (aref file (1- (length file)))) nil)
+ (project-files-relative-names file)
+ (t (concat default-directory file))))
(split-string
(with-output-to-string
(apply #'vc-git-command standard-output 0 nil "ls-files" args))
This seems to help readability -- at least to me. There's probably also
a nominal performance benefit since `cond` is a special form.
I've pushed this as branch `sa/sparse-index-2` to my repository. (This
is in addition to the `sa/sparse-index` branch, which contains the
`file-exists-p` check mentioned below plus what might be, I take it, an
ultimately unneeded opt-out parameter in `project-files`.)
It's worth noting that actually performing a `file-exists-p` check here
would have the added benefit of handling the awkward state between Git
2.25 (where sparse-checkout was introduced) and 2.35 (where git-ls-files
learned --sparse) where ls-files could still report things that _look_
like files but are not present. This would be fixed by just replacing
the (eq ..) form with (not (file-exists-p file)).
--
Sean Allred
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.