bug#68579: [PATCH] Support a local repo as URL in treesit-language-source-alist

bug#68579: [PATCH] Support a local repo as URL in treesit-language-source-alist

[Konstantin Kharlamov]

[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]

TL;DR: this just to simplify debugging/rebuilding treesitter grammars,
because they are often broken (both the build system and the infamous
"unversioning" part) and only Emacs knows what to do with them. So
gotta have some way to point Emacs to a local grammar repo.

-----------

Long story:

I've been recently stumbling upon the infamous "Debug the query with
treesit-query-validate" problem, and found that it's not the only
problem in that context. The workaround to this problem currently is
finding the commit in the offending treesitter-grammar that works and
re-building the grammar from it.

Well, that "re-building" part of the advice turns out to be very
convoluted. Let's take for example typescript¹. It has no "building"
docs, but I guess they are not needed because it's obvious by
`Cargo.toml` presence that it's written in Rust, and hence building is
just `cargo bulid --release`, right? Well, no. Doing that does not
produce the shared libs Emacs expects. Now a user starts reverse-
engineering, trying to find what's wrong. At some point they realize
that Emacs builds code from the specific directory `tsx/src`, but…
There's no build system! What do you do with these files…? Well, the
answer is that Emacs somehow knows what to do, so now you have to
modify `treesit-language-source-alist` to point at the local repo and
make Emacs do the job of building the grammar.

However, Emacs does not support local paths as part of that grammar.
What it does instead is it clones FROM the local path (which is
confusing on itself, because everything completes successfully, but it
didn't do what you expected it to).

So what this patch does is it adds detection of a full path to the
treesit-language-source-alist processing to make sure we avoid cloning
the repo in that case and just proceed building it.

1: https://github.com/tree-sitter/tree-sitter-typescript

[-- Attachment #2: 1.patch --]
[-- Type: text/x-patch, Size: 4505 bytes --]

From c9de59a39566a5df46aa39ef93348487bb2948aa Mon Sep 17 00:00:00 2001
From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
Date: Fri, 19 Jan 2024 10:33:47 +0300
Subject: [PATCH] Support a local repo as URL in treesit-language-source-alist

Sometimes people may need to bisect to find specific revision in a
grammar repo. In this case they'd want to point the URL to the local
repo to avoid cloning it on every rebuild. So add support for full
path in treesit-language-source-alist.

* lisp/treesit.el (treesit--install-language-grammar-1): test if URL
starts with / meaning that the URL is a local path. Then if it is,
avoid cloning the repo and removing the path on success.
(treesit--git-clone-repo): factor out the code for cloning to a separate
function.
(treesit--git-checkout-branch): a helper to checkout the revision for
cases where we didn't clone the repo but want it to point the
revision.
---
 lisp/treesit.el | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/lisp/treesit.el b/lisp/treesit.el
index c8b473c7bb8..11631e68793 100644
--- a/lisp/treesit.el
+++ b/lisp/treesit.el
@@ -3417,7 +3417,7 @@ treesit-language-source-alist
     (LANG . (URL REVISION SOURCE-DIR CC C++))
 
 Only LANG and URL are mandatory.  LANG is the language symbol.
-URL is the Git repository URL for the grammar.
+URL is the Git repository URL or full path for the grammar.
 
 REVISION is the Git tag or branch of the desired version,
 defaulting to the latest default branch.
@@ -3551,6 +3551,23 @@ treesit--call-process-signal
                                  (buffer-string)))
     (erase-buffer)))
 
+(defun treesit--git-checkout-branch (repo-path revision)
+  "Checkout `revision' in a repo located in `repo-path'"
+  (treesit--call-process-signal
+   "git" nil t nil "-C" repo-path "checkout" revision))
+
+(defun treesit--git-clone-repo (url revision workdir)
+  "Clone repo pointed by `url' at commit `revision' to `workdir'"
+  (message "Cloning repository")
+  ;; git clone xxx --depth 1 --quiet [-b yyy] workdir
+  (if revision
+      (treesit--call-process-signal
+       "git" nil t nil "clone" url "--depth" "1" "--quiet"
+       "-b" revision workdir)
+    (treesit--call-process-signal
+     "git" nil t nil "clone" url "--depth" "1" "--quiet"
+     workdir)))
+
 (defun treesit--install-language-grammar-1
     (out-dir lang url &optional revision source-dir cc c++)
   "Install and compile a tree-sitter language grammar library.
@@ -3564,8 +3581,10 @@ treesit--install-language-grammar-1
 `treesit-language-source-alist'.  If anything goes wrong, this
 function signals an error."
   (let* ((lang (symbol-name lang))
+         ;; don't clone if url is a local path
+         (url-is-path (string-prefix-p "/" url))
          (default-directory (make-temp-file "treesit-workdir" t))
-         (workdir (expand-file-name "repo"))
+         (workdir (if url-is-path url (expand-file-name "repo")))
          (source-dir (expand-file-name (or source-dir "src") workdir))
          (cc (or cc (seq-find #'executable-find '("cc" "gcc" "c99"))
                  ;; If no C compiler found, just use cc and let
@@ -3580,15 +3599,10 @@ treesit--install-language-grammar-1
          (lib-name (concat "libtree-sitter-" lang soext)))
     (unwind-protect
         (with-temp-buffer
-          (message "Cloning repository")
-          ;; git clone xxx --depth 1 --quiet [-b yyy] workdir
-          (if revision
-              (treesit--call-process-signal
-               "git" nil t nil "clone" url "--depth" "1" "--quiet"
-               "-b" revision workdir)
-            (treesit--call-process-signal
-             "git" nil t nil "clone" url "--depth" "1" "--quiet"
-             workdir))
+          (if url-is-path
+              (when revision
+                (treesit--git-checkout-branch url revision))
+            (treesit--git-clone-repo url revision workdir))
           ;; We need to go into the source directory because some
           ;; header files use relative path (#include "../xxx").
           ;; cd "${sourcedir}"
@@ -3635,7 +3649,7 @@ treesit--install-language-grammar-1
             ;; Ignore errors, in case the old version is still used.
             (ignore-errors (delete-file old-fname)))
           (message "Library installed to %s/%s" out-dir lib-name))
-      (when (file-exists-p workdir)
+      (when (and (not url-is-path) (file-exists-p workdir))
         (delete-directory workdir t)))))
 
 ;;; Etc
-- 
2.43.0

bug#68579: [PATCH] Support a local repo as URL in treesit-language-source-alist

[Eli Zaretskii]

> From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> Date: Fri, 19 Jan 2024 11:08:00 +0300
> 
> So what this patch does is it adds detection of a full path to the
> treesit-language-source-alist processing to make sure we avoid cloning
> the repo in that case and just proceed building it.

Thanks.  Some comments below, and I added Yuan to the discussion.

> From c9de59a39566a5df46aa39ef93348487bb2948aa Mon Sep 17 00:00:00 2001
> From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> Date: Fri, 19 Jan 2024 10:33:47 +0300
> Subject: [PATCH] Support a local repo as URL in treesit-language-source-alist
> 
> Sometimes people may need to bisect to find specific revision in a
> grammar repo. In this case they'd want to point the URL to the local
> repo to avoid cloning it on every rebuild. So add support for full
> path in treesit-language-source-alist.
> 
> * lisp/treesit.el (treesit--install-language-grammar-1): test if URL
> starts with / meaning that the URL is a local path. Then if it is,
> avoid cloning the repo and removing the path on success.
> (treesit--git-clone-repo): factor out the code for cloning to a separate
> function.
> (treesit--git-checkout-branch): a helper to checkout the revision for
> cases where we didn't clone the repo but want it to point the
> revision.

These descriptions should be full sentences: start with a capital
letter and end in a period.  Also, please leave two spaces between
sentences.

>  Only LANG and URL are mandatory.  LANG is the language symbol.
> -URL is the Git repository URL for the grammar.
> +URL is the Git repository URL or full path for the grammar.

GNU Coding Standards frown on using "path" for anything but PATH-style
directory lists.  In this case, you mean "directory name".

> +(defun treesit--git-checkout-branch (repo-path revision)
> +  "Checkout `revision' in a repo located in `repo-path'"

References to arguments in doc strings should be capitalized and not
quoted: REVISION and REPO-PATH (which should be renamed to REPO-DIR).

> +(defun treesit--git-clone-repo (url revision workdir)
> +  "Clone repo pointed by `url' at commit `revision' to `workdir'"

Likewise.

Also, please document in the doc string what happens if REVISION is
nil.

> +         ;; don't clone if url is a local path

Comments should be complete sentences: begin with a capital letter and
end with a period.

> +         (url-is-path (string-prefix-p "/" url))

"Path" used wrongly again.  Also, the string-prefix-p test is too
naïve and unportable.  I think file-name-absolute-p is a better test
(assuming we expect an absolute file name there), perhaps also
augmented by file-accessible-directory-p.

> -         (workdir (expand-file-name "repo"))
> +         (workdir (if url-is-path url (expand-file-name "repo")))

Not sure about this hunk: why do we not need to expand-file-name if
URL is not a local directory but a real URL?

> +          (if url-is-path
> +              (when revision
> +                (treesit--git-checkout-branch url revision))

Isn't the above equivalent to

             (and url-is-path revision
                  (treesit--git-checkout-branch url revision))

?

> -      (when (file-exists-p workdir)
> +      (when (and (not url-is-path) (file-exists-p workdir))
>          (delete-directory workdir t)))))

Why?  Does workdir have different semantics in these two use cases?
Isn't it the directory where we cloned the repository?

Finally, this needs a NEWS entry.

bug#68579: [PATCH] Support a local repo as URL in treesit-language-source-alist

[Konstantin Kharlamov]

On Fri, 2024-01-19 at 10:33 +0200, Eli Zaretskii wrote:
[…]

Thank you!

> 
> > +         (url-is-path (string-prefix-p "/" url))
> 
> "Path" used wrongly again.  Also, the string-prefix-p test is too
> naïve and unportable.  I think file-name-absolute-p is a better test
> (assuming we expect an absolute file name there), perhaps also
> augmented by file-accessible-directory-p.

I would presume if the directory inaccessible some later commands such
as `git checkout` will fail anyway, so no point in adding the `file-
accessible-directory-p` check on Emacs side…?

> > -         (workdir (expand-file-name "repo"))
> > +         (workdir (if url-is-path url (expand-file-name "repo")))
> 
> Not sure about this hunk: why do we not need to expand-file-name if
> URL is not a local directory but a real URL?

Idk, that was there 😅 But yeah, I can remove if it's not needed. I
presume if it's really needed, there needs to be an explanation comment

> > +          (if url-is-path
> > +              (when revision
> > +                (treesit--git-checkout-branch url revision))
> 
> Isn't the above equivalent to
> 
>              (and url-is-path revision
>                   (treesit--git-checkout-branch url revision))
> 
> ?

Good point!

> > -      (when (file-exists-p workdir)
> > +      (when (and (not url-is-path) (file-exists-p workdir))
> >          (delete-directory workdir t)))))
> 
> Why?  Does workdir have different semantics in these two use cases?
> Isn't it the directory where we cloned the repository?

When an absolute path is passed as URL, that means the user have cloned
the repo, not us. So we do not want to remove the directory. This is a
different semantics from the case where we created a temporary clone
ourselves, and want to get rid of the temporary directory after
everything is done.

I can add a comment explaining that there

bug#68579: [PATCH] Support a local repo as URL in treesit-language-source-alist

[Eli Zaretskii]

> From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> Cc: 68579@debbugs.gnu.org
> Date: Fri, 19 Jan 2024 11:57:48 +0300
> 
> On Fri, 2024-01-19 at 10:33 +0200, Eli Zaretskii wrote:
> […]
> 
> Thank you!
> 
> > 
> > > +         (url-is-path (string-prefix-p "/" url))
> > 
> > "Path" used wrongly again.  Also, the string-prefix-p test is too
> > naïve and unportable.  I think file-name-absolute-p is a better test
> > (assuming we expect an absolute file name there), perhaps also
> > augmented by file-accessible-directory-p.
> 
> I would presume if the directory inaccessible some later commands such
> as `git checkout` will fail anyway, so no point in adding the `file-
> accessible-directory-p` check on Emacs side…?

I'm talking about distinguishing between a URL and a local file name.
Are we guaranteed to get an absolute file name there?  If not, how do
you know that something like "http://foo.bar" cannot be a local file
name?

> > > -         (workdir (expand-file-name "repo"))
> > > +         (workdir (if url-is-path url (expand-file-name "repo")))
> > 
> > Not sure about this hunk: why do we not need to expand-file-name if
> > URL is not a local directory but a real URL?
> 
> Idk, that was there 😅

The expand-file-name was there, yes.  But why do you think we should
avoid calling expand-file-name if URL is a local file name?

> > > -      (when (file-exists-p workdir)
> > > +      (when (and (not url-is-path) (file-exists-p workdir))
> > >          (delete-directory workdir t)))))
> > 
> > Why?  Does workdir have different semantics in these two use cases?
> > Isn't it the directory where we cloned the repository?
> 
> When an absolute path is passed as URL, that means the user have cloned
> the repo, not us.

But you still clone from it into workdir, no?  treesit--git-clone-repo
invokes "git clone" in both cases, according to my reading of the patch.

bug#68579: [PATCH] Support a local repo as URL in treesit-language-source-alist

[Konstantin Kharlamov]

On Fri, 2024-01-19 at 13:46 +0200, Eli Zaretskii wrote:
> > From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> > Cc: 68579@debbugs.gnu.org
> > Date: Fri, 19 Jan 2024 11:57:48 +0300
> > 
> > On Fri, 2024-01-19 at 10:33 +0200, Eli Zaretskii wrote:
> > […]
> > 
> > Thank you!
> > 
> > > 
> > > > +         (url-is-path (string-prefix-p "/" url))
> > > 
> > > "Path" used wrongly again.  Also, the string-prefix-p test is too
> > > naïve and unportable.  I think file-name-absolute-p is a better
> > > test
> > > (assuming we expect an absolute file name there), perhaps also
> > > augmented by file-accessible-directory-p.
> > 
> > I would presume if the directory inaccessible some later commands
> > such
> > as `git checkout` will fail anyway, so no point in adding the
> > `file-
> > accessible-directory-p` check on Emacs side…?
> 
> I'm talking about distinguishing between a URL and a local file name.
> Are we guaranteed to get an absolute file name there?  If not, how do
> you know that something like "http://foo.bar" cannot be a local file
> name?

Ah, gotcha!

> > > > -         (workdir (expand-file-name "repo"))
> > > > +         (workdir (if url-is-path url (expand-file-name
> > > > "repo")))
> > > 
> > > Not sure about this hunk: why do we not need to expand-file-name
> > > if
> > > URL is not a local directory but a real URL?
> > 
> > Idk, that was there 😅
> 
> The expand-file-name was there, yes.  But why do you think we should
> avoid calling expand-file-name if URL is a local file name?

Ah, sorry, I got that reversed. Yeah, it makes sense calling that for a
local filename. I was wondering why was it there for a URL, which is
partially why I read you wrong.

> > > > -      (when (file-exists-p workdir)
> > > > +      (when (and (not url-is-path) (file-exists-p workdir))
> > > >          (delete-directory workdir t)))))
> > > 
> > > Why?  Does workdir have different semantics in these two use
> > > cases?
> > > Isn't it the directory where we cloned the repository?
> > 
> > When an absolute path is passed as URL, that means the user have
> > cloned
> > the repo, not us.
> 
> But you still clone from it into workdir, no?  treesit--git-clone-
> repo
> invokes "git clone" in both cases, according to my reading of the
> patch.

No, the `treesit--git-clone-repo` is located on the "else" branch of
the `(if url-is-path`. That is, we do not call it when it's a local
path

bug#68579: [PATCH] Support a local repo as URL in treesit-language-source-alist

[Eli Zaretskii]

> From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> Cc: casouri@gmail.com, 68579@debbugs.gnu.org
> Date: Fri, 19 Jan 2024 17:33:03 +0300
> 
> > > > > -      (when (file-exists-p workdir)
> > > > > +      (when (and (not url-is-path) (file-exists-p workdir))
> > > > >          (delete-directory workdir t)))))
> > > > 
> > > > Why?  Does workdir have different semantics in these two use
> > > > cases?
> > > > Isn't it the directory where we cloned the repository?
> > > 
> > > When an absolute path is passed as URL, that means the user have
> > > cloned
> > > the repo, not us.
> > 
> > But you still clone from it into workdir, no?  treesit--git-clone-
> > repo
> > invokes "git clone" in both cases, according to my reading of the
> > patch.
> 
> No, the `treesit--git-clone-repo` is located on the "else" branch of
> the `(if url-is-path`. That is, we do not call it when it's a local
> path

Then this assumes some kind of workflow, doesn't it?  The user must
first clone the repository, either via treesit.el or manually, and
then they can use this new feature, right?  So I guess we should
document this workflow somewhere?

bug#68579: [PATCH] Support a local repo as URL in treesit-language-source-alist

[Konstantin Kharlamov]

On Fri, 2024-01-19 at 17:06 +0200, Eli Zaretskii wrote:
> > From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> > Cc: casouri@gmail.com, 68579@debbugs.gnu.org
> > Date: Fri, 19 Jan 2024 17:33:03 +0300
> > 
> > > > > > -      (when (file-exists-p workdir)
> > > > > > +      (when (and (not url-is-path) (file-exists-p
> > > > > > workdir))
> > > > > >          (delete-directory workdir t)))))
> > > > > 
> > > > > Why?  Does workdir have different semantics in these two use
> > > > > cases?
> > > > > Isn't it the directory where we cloned the repository?
> > > > 
> > > > When an absolute path is passed as URL, that means the user
> > > > have
> > > > cloned
> > > > the repo, not us.
> > > 
> > > But you still clone from it into workdir, no?  treesit--git-
> > > clone-
> > > repo
> > > invokes "git clone" in both cases, according to my reading of the
> > > patch.
> > 
> > No, the `treesit--git-clone-repo` is located on the "else" branch
> > of
> > the `(if url-is-path`. That is, we do not call it when it's a local
> > path
> 
> Then this assumes some kind of workflow, doesn't it?  The user must
> first clone the repository, either via treesit.el or manually, and
> then they can use this new feature, right?  So I guess we should
> document this workflow somewhere?

Well, I can add some docs if you want, but the workflow per se seems
obvious to me: you have a function that accepts a local path to repo
and builds from it. You'd never want someone to delete your repo just
because you asked that someone to compile sources in the repo, because
you might have some local commits in there. Removing it would be a
completely surprising side-effect.

OTOH, when you pass a URL to the same function, you ask "pls build from
the sources, I don't care how", so the function clones it to a
temporary dir. It may or may not remove the dir, it's doesn't matter
that much. But the temporary directory becomes a junk after the build
completed, so why not remove it as well.

To me personally these two workflows is something that immediately
comes to mind when I see that the parameter is a URL or a local path.

bug#68579: [PATCH] Support a local repo as URL in treesit-language-source-alist

[Eli Zaretskii]

> From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> Cc: casouri@gmail.com, 68579@debbugs.gnu.org
> Date: Fri, 19 Jan 2024 19:06:59 +0300
> 
> On Fri, 2024-01-19 at 17:06 +0200, Eli Zaretskii wrote:
> > > From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> > > Cc: casouri@gmail.com, 68579@debbugs.gnu.org
> > > Date: Fri, 19 Jan 2024 17:33:03 +0300
> > > 
> > > > > > > -      (when (file-exists-p workdir)
> > > > > > > +      (when (and (not url-is-path) (file-exists-p
> > > > > > > workdir))
> > > > > > >          (delete-directory workdir t)))))
> > > > > > 
> > > > > > Why?  Does workdir have different semantics in these two use
> > > > > > cases?
> > > > > > Isn't it the directory where we cloned the repository?
> > > > > 
> > > > > When an absolute path is passed as URL, that means the user
> > > > > have
> > > > > cloned
> > > > > the repo, not us.
> > > > 
> > > > But you still clone from it into workdir, no?  treesit--git-
> > > > clone-
> > > > repo
> > > > invokes "git clone" in both cases, according to my reading of the
> > > > patch.
> > > 
> > > No, the `treesit--git-clone-repo` is located on the "else" branch
> > > of
> > > the `(if url-is-path`. That is, we do not call it when it's a local
> > > path
> > 
> > Then this assumes some kind of workflow, doesn't it?  The user must
> > first clone the repository, either via treesit.el or manually, and
> > then they can use this new feature, right?  So I guess we should
> > document this workflow somewhere?
> 
> Well, I can add some docs if you want, but the workflow per se seems
> obvious to me: you have a function that accepts a local path to repo
> and builds from it.

What I had in mind is some short description in the doc string of the
interactive command that invokes all this stuff.

bug#68579: [PATCH v2] Support a local repo as URL in treesit-language-source-alist

[Konstantin Kharlamov]

Sometimes people may need to bisect to find specific revision in a
grammar repo. In this case they'd want to point the URL to the local
repo to avoid cloning it on every rebuild. So add support for full
path in treesit-language-source-alist.

* lisp/treesit.el (treesit--install-language-grammar-1): Test if URL
starts with / meaning that the URL is a local path.  Then if it is,
avoid cloning the repo and removing the path on success.
(treesit--git-clone-repo): Factor out the code for cloning to a separate
function.
(treesit--git-checkout-branch): A helper to checkout the revision for
cases where we didn't clone the repo but want it to point the
revision.
---

v2:

1. Regarding discussion about (file-name-absolute-p) and (file-accessible-directory-p),
I figured that if we only test whether the file is accessible, we can lift the
restriction of it being an absolute directory. So now I test for accessibility.

2. I also removed my comment in variable initialization because it's obsolete and no
longer useful.

3. I also couldn't squash the

   (if url-is-path
       (when revision
         (treesit--git-checkout-branch url revision))

  to

   (and url-is-path revision
        (treesit--git-checkout-branch url revision))

  because that would change the semantics: note that there's an `(if` which checks
  whether we're dealing with a local path, and then in "else" branch (which is not in
  the snippet) we clone the repo. Changing the code as requested would result in the
  code cloning the repo when it's a local path but the revision wasn't set.

4. I hope it's not a problem: I made use of the word `path` inside documentation for
`treesit-language-source-alist`. It just seems to sound awkward repeating the word
"directory" twice, and I think it's clear from the context that these are synonyms.

Hopefully I didn't miss anything 😅

(Eli Zaretskii):
   * start sentences with capital letter and put 2 spaces after the dot
   * replace "path" with "dir" and "directory" in code and docs
   * capitalize and unquote arg names in the docs
   * (treesit--git-clone-repo): document REVISION being nil
   * call (expand-file-name) on the URL
   * added a NEWS entry
   * changed (treesit-language-source-alist) docs to shortly mention the workflow of
     building the grammar from a local path

 etc/NEWS        |  8 ++++++++
 lisp/treesit.el | 49 ++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 735a05f6579..19b06ea4c66 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1832,6 +1832,14 @@ The 'test' parameter is omitted if it is 'eql' (the default), as is
 'data' if empty.  'rehash-size', 'rehash-threshold' and 'size' are
 always omitted, and ignored if present when the object is read back in.
 
++++
+** 'treesit-install-language-grammar' can handle local directory as URL.
+It is now possible to pass a directory of a local repository as URL
+inside 'treesit-language-source-alist', so that calling
+'treesit-install-language-grammar' would avoid cloning the repository.
+It may be useful for example for the purposes of bisecting a
+treesitter grammar.
+
 \f
 * Changes in Emacs 30.1 on Non-Free Operating Systems
 
diff --git a/lisp/treesit.el b/lisp/treesit.el
index c8b473c7bb8..19dcbe324aa 100644
--- a/lisp/treesit.el
+++ b/lisp/treesit.el
@@ -3410,14 +3410,16 @@ treesit-explore-mode
 ;;; Install & build language grammar
 
 (defvar treesit-language-source-alist nil
-  "Configuration for downloading and installing tree-sitter language grammars.
+  "Configuration for downloading and installing tree-sitter language
+grammars. The grammar can also be built from a local directory if
+URL is an existing local path to the repo.
 
 The value should be an alist where each element has the form
 
     (LANG . (URL REVISION SOURCE-DIR CC C++))
 
 Only LANG and URL are mandatory.  LANG is the language symbol.
-URL is the Git repository URL for the grammar.
+URL is the Git repository URL or directory name for the grammar.
 
 REVISION is the Git tag or branch of the desired version,
 defaulting to the latest default branch.
@@ -3551,6 +3553,26 @@ treesit--call-process-signal
                                  (buffer-string)))
     (erase-buffer)))
 
+(defun treesit--git-checkout-branch (repo-dir revision)
+  "Checkout REVISION in a repo located in REPO-DIR."
+  (treesit--call-process-signal
+   "git" nil t nil "-C" repo-dir "checkout" revision))
+
+(defun treesit--git-clone-repo (url revision workdir)
+  "Clone repo pointed by URL at commit REVISION to WORKDIR.
+
+REVISION may be nil, in which case the cloned repo will be at its
+default branch."
+  (message "Cloning repository")
+  ;; git clone xxx --depth 1 --quiet [-b yyy] workdir
+  (if revision
+      (treesit--call-process-signal
+       "git" nil t nil "clone" url "--depth" "1" "--quiet"
+       "-b" revision workdir)
+    (treesit--call-process-signal
+     "git" nil t nil "clone" url "--depth" "1" "--quiet"
+     workdir)))
+
 (defun treesit--install-language-grammar-1
     (out-dir lang url &optional revision source-dir cc c++)
   "Install and compile a tree-sitter language grammar library.
@@ -3564,8 +3586,12 @@ treesit--install-language-grammar-1
 `treesit-language-source-alist'.  If anything goes wrong, this
 function signals an error."
   (let* ((lang (symbol-name lang))
+         (maybe-repo-dir (expand-file-name url))
+         (url-is-dir (file-accessible-directory-p maybe-repo-dir))
          (default-directory (make-temp-file "treesit-workdir" t))
-         (workdir (expand-file-name "repo"))
+         (workdir (if url-is-dir
+                      maybe-repo-dir
+                    (expand-file-name "repo")))
          (source-dir (expand-file-name (or source-dir "src") workdir))
          (cc (or cc (seq-find #'executable-find '("cc" "gcc" "c99"))
                  ;; If no C compiler found, just use cc and let
@@ -3580,15 +3606,10 @@ treesit--install-language-grammar-1
          (lib-name (concat "libtree-sitter-" lang soext)))
     (unwind-protect
         (with-temp-buffer
-          (message "Cloning repository")
-          ;; git clone xxx --depth 1 --quiet [-b yyy] workdir
-          (if revision
-              (treesit--call-process-signal
-               "git" nil t nil "clone" url "--depth" "1" "--quiet"
-               "-b" revision workdir)
-            (treesit--call-process-signal
-             "git" nil t nil "clone" url "--depth" "1" "--quiet"
-             workdir))
+          (if url-is-dir
+              (when revision
+                (treesit--git-checkout-branch workdir revision))
+            (treesit--git-clone-repo url revision workdir))
           ;; We need to go into the source directory because some
           ;; header files use relative path (#include "../xxx").
           ;; cd "${sourcedir}"
@@ -3635,7 +3656,9 @@ treesit--install-language-grammar-1
             ;; Ignore errors, in case the old version is still used.
             (ignore-errors (delete-file old-fname)))
           (message "Library installed to %s/%s" out-dir lib-name))
-      (when (file-exists-p workdir)
+      ;; Remove workdir if it's not a repo owned by user and we
+      ;; managed to create it in the first place.
+      (when (and (not url-is-dir) (file-exists-p workdir))
         (delete-directory workdir t)))))
 
 ;;; Etc
-- 
2.43.0

bug#68579: [PATCH v2] Support a local repo as URL in treesit-language-source-alist

[Konstantin Kharlamov]

Omg, I'm sorry, I forgot to test the code. I see one of the cases is
broken, let me fix that.

I just had to do so much, I had completely forgotten to test that…

bug#68579: [PATCH v2] Support a local repo as URL in treesit-language-source-alist

[Konstantin Kharlamov]

On Sat, 2024-01-20 at 15:00 +0300, Konstantin Kharlamov wrote:
> Omg, I'm sorry, I forgot to test the code. I see one of the cases is
> broken, let me fix that.
> 
> I just had to do so much, I had completely forgotten to test that…

Ah, never mind, everything is fine, I just forgot to evaluate it.

Tested: github URL, local path with a commit, local path with nil as a
commit.

Everything seems to work 👍

bug#68579: [PATCH v2] Support a local repo as URL in treesit-language-source-alist

[Eli Zaretskii]

> From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> Date: Sat, 20 Jan 2024 14:56:29 +0300
> 
> Sometimes people may need to bisect to find specific revision in a
> grammar repo. In this case they'd want to point the URL to the local
> repo to avoid cloning it on every rebuild. So add support for full
> path in treesit-language-source-alist.
> 
> * lisp/treesit.el (treesit--install-language-grammar-1): Test if URL
> starts with / meaning that the URL is a local path.  Then if it is,
> avoid cloning the repo and removing the path on success.
> (treesit--git-clone-repo): Factor out the code for cloning to a separate
> function.
> (treesit--git-checkout-branch): A helper to checkout the revision for
> cases where we didn't clone the repo but want it to point the
> revision.
> ---

Thanks, but could you please send the patch as attachments created
with "git format-patch", and include only the stuff you think should
be in the patch and the commit log message?

Also, I have a few minor comments below.

> ++++
> +** 'treesit-install-language-grammar' can handle local directory as URL.
> +It is now possible to pass a directory of a local repository as URL
                                                                ^^^^^^
I think you mean "instead of a URL" there.

> +inside 'treesit-language-source-alist', so that calling
> +'treesit-install-language-grammar' would avoid cloning the repository.
> +It may be useful for example for the purposes of bisecting a
                   ^           ^
Commas missing there.

> --- a/lisp/treesit.el
> +++ b/lisp/treesit.el
> @@ -3410,14 +3410,16 @@ treesit-explore-mode
>  ;;; Install & build language grammar
>  
>  (defvar treesit-language-source-alist nil
> -  "Configuration for downloading and installing tree-sitter language grammars.
> +  "Configuration for downloading and installing tree-sitter language
> +grammars. The grammar can also be built from a local directory if
> +URL is an existing local path to the repo.

The first line of a doc string should be a single complete sentence
(because various apropos commands display only the first lines of doc
strings).

>  The value should be an alist where each element has the form
>  
>      (LANG . (URL REVISION SOURCE-DIR CC C++))
>  
>  Only LANG and URL are mandatory.  LANG is the language symbol.
> -URL is the Git repository URL for the grammar.
> +URL is the Git repository URL or directory name for the grammar.

Suggest to reword:

  URL is the URL of the grammar's Git repository or a directory where
  the repository has been cloned.

Thanks.

bug#68579: [PATCH v2] Support a local repo as URL in treesit-language-source-alist

[Konstantin Kharlamov]

[-- Attachment #1: Type: text/plain, Size: 3629 bytes --]

On Sat, 2024-01-27 at 11:37 +0200, Eli Zaretskii wrote:
> > From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> > Date: Sat, 20 Jan 2024 14:56:29 +0300
> > 
> > Sometimes people may need to bisect to find specific revision in a
> > grammar repo. In this case they'd want to point the URL to the
> > local
> > repo to avoid cloning it on every rebuild. So add support for full
> > path in treesit-language-source-alist.
> > 
> > * lisp/treesit.el (treesit--install-language-grammar-1): Test if
> > URL
> > starts with / meaning that the URL is a local path.  Then if it is,
> > avoid cloning the repo and removing the path on success.
> > (treesit--git-clone-repo): Factor out the code for cloning to a
> > separate
> > function.
> > (treesit--git-checkout-branch): A helper to checkout the revision
> > for
> > cases where we didn't clone the repo but want it to point the
> > revision.
> > ---
> 
> Thanks, but could you please send the patch as attachments created
> with "git format-patch", and include only the stuff you think should
> be in the patch and the commit log message?
> 
> Also, I have a few minor comments below.
> 
> > ++++
> > +** 'treesit-install-language-grammar' can handle local directory
> > as URL.
> > +It is now possible to pass a directory of a local repository as
> > URL
>                                                                
> ^^^^^^
> I think you mean "instead of a URL" there.

Right, sorry, I was thinking of "URL" being the name of the argument,
but your text sure is clearer.

> > +inside 'treesit-language-source-alist', so that calling
> > +'treesit-install-language-grammar' would avoid cloning the
> > repository.
> > +It may be useful for example for the purposes of bisecting a
>                    ^           ^
> Commas missing there.
> 
> > --- a/lisp/treesit.el
> > +++ b/lisp/treesit.el
> > @@ -3410,14 +3410,16 @@ treesit-explore-mode
> >  ;;; Install & build language grammar
> >  
> >  (defvar treesit-language-source-alist nil
> > -  "Configuration for downloading and installing tree-sitter
> > language grammars.
> > +  "Configuration for downloading and installing tree-sitter
> > language
> > +grammars. The grammar can also be built from a local directory if
> > +URL is an existing local path to the repo.
> 
> The first line of a doc string should be a single complete sentence
> (because various apropos commands display only the first lines of doc
> strings).

Ah, okay, in this case I'm a bit confused regarding what you wanted me
to do by asking to provide a small description of the workflow. I've
documented that URL arg may accept a local dir. I can put this new
sentence to a separate paragraph, but I'm not sure it reads too well,
given the same information is provided one paragraph below.

I've reverted this change for now.

> >  The value should be an alist where each element has the form
> >  
> >      (LANG . (URL REVISION SOURCE-DIR CC C++))
> >  
> >  Only LANG and URL are mandatory.  LANG is the language symbol.
> > -URL is the Git repository URL for the grammar.
> > +URL is the Git repository URL or directory name for the grammar.
> 
> Suggest to reword:
> 
>   URL is the URL of the grammar's Git repository or a directory where
>   the repository has been cloned.
> 
> Thanks.

Thanks! Patch is attached, everything is addressed except one question
above. I also fixed "one space" to two spaces" in one place in the
commit description.

[-- Attachment #2: 1.patch --]
[-- Type: text/x-patch, Size: 5623 bytes --]

From 87a8984366ab843f6a28a30a74cf519301978bd2 Mon Sep 17 00:00:00 2001
From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
Date: Fri, 19 Jan 2024 10:33:47 +0300
Subject: [PATCH] Support a local repo as URL in treesit-language-source-alist

Sometimes people may need to bisect to find specific revision in a
grammar repo.  In this case they'd want to point the URL to the local
repo to avoid cloning it on every rebuild.  So add support for full
path in treesit-language-source-alist.

* lisp/treesit.el (treesit--install-language-grammar-1): Test if URL
starts with / meaning that the URL is a local path.  Then if it is,
avoid cloning the repo and removing the path on success.
(treesit--git-clone-repo): Factor out the code for cloning to a separate
function.
(treesit--git-checkout-branch): A helper to checkout the revision for
cases where we didn't clone the repo but want it to point the
revision.
---
 etc/NEWS        |  8 ++++++++
 lisp/treesit.el | 46 ++++++++++++++++++++++++++++++++++------------
 2 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 735a05f6579..9bdc3af5e71 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1832,6 +1832,14 @@ The 'test' parameter is omitted if it is 'eql' (the default), as is
 'data' if empty.  'rehash-size', 'rehash-threshold' and 'size' are
 always omitted, and ignored if present when the object is read back in.
 
++++
+** 'treesit-install-language-grammar' can handle local directory instead of URL.
+It is now possible to pass a directory of a local repository as URL
+inside 'treesit-language-source-alist', so that calling
+'treesit-install-language-grammar' would avoid cloning the repository.
+It may be useful, for example, for the purposes of bisecting a
+treesitter grammar.
+
 \f
 * Changes in Emacs 30.1 on Non-Free Operating Systems
 
diff --git a/lisp/treesit.el b/lisp/treesit.el
index c8b473c7bb8..b1f9d551442 100644
--- a/lisp/treesit.el
+++ b/lisp/treesit.el
@@ -3417,7 +3417,8 @@ treesit-language-source-alist
     (LANG . (URL REVISION SOURCE-DIR CC C++))
 
 Only LANG and URL are mandatory.  LANG is the language symbol.
-URL is the Git repository URL for the grammar.
+URL is the URL of the grammar's Git repository or a directory
+where the repository has been cloned.
 
 REVISION is the Git tag or branch of the desired version,
 defaulting to the latest default branch.
@@ -3551,6 +3552,26 @@ treesit--call-process-signal
                                  (buffer-string)))
     (erase-buffer)))
 
+(defun treesit--git-checkout-branch (repo-dir revision)
+  "Checkout REVISION in a repo located in REPO-DIR."
+  (treesit--call-process-signal
+   "git" nil t nil "-C" repo-dir "checkout" revision))
+
+(defun treesit--git-clone-repo (url revision workdir)
+  "Clone repo pointed by URL at commit REVISION to WORKDIR.
+
+REVISION may be nil, in which case the cloned repo will be at its
+default branch."
+  (message "Cloning repository")
+  ;; git clone xxx --depth 1 --quiet [-b yyy] workdir
+  (if revision
+      (treesit--call-process-signal
+       "git" nil t nil "clone" url "--depth" "1" "--quiet"
+       "-b" revision workdir)
+    (treesit--call-process-signal
+     "git" nil t nil "clone" url "--depth" "1" "--quiet"
+     workdir)))
+
 (defun treesit--install-language-grammar-1
     (out-dir lang url &optional revision source-dir cc c++)
   "Install and compile a tree-sitter language grammar library.
@@ -3564,8 +3585,12 @@ treesit--install-language-grammar-1
 `treesit-language-source-alist'.  If anything goes wrong, this
 function signals an error."
   (let* ((lang (symbol-name lang))
+         (maybe-repo-dir (expand-file-name url))
+         (url-is-dir (file-accessible-directory-p maybe-repo-dir))
          (default-directory (make-temp-file "treesit-workdir" t))
-         (workdir (expand-file-name "repo"))
+         (workdir (if url-is-dir
+                      maybe-repo-dir
+                    (expand-file-name "repo")))
          (source-dir (expand-file-name (or source-dir "src") workdir))
          (cc (or cc (seq-find #'executable-find '("cc" "gcc" "c99"))
                  ;; If no C compiler found, just use cc and let
@@ -3580,15 +3605,10 @@ treesit--install-language-grammar-1
          (lib-name (concat "libtree-sitter-" lang soext)))
     (unwind-protect
         (with-temp-buffer
-          (message "Cloning repository")
-          ;; git clone xxx --depth 1 --quiet [-b yyy] workdir
-          (if revision
-              (treesit--call-process-signal
-               "git" nil t nil "clone" url "--depth" "1" "--quiet"
-               "-b" revision workdir)
-            (treesit--call-process-signal
-             "git" nil t nil "clone" url "--depth" "1" "--quiet"
-             workdir))
+          (if url-is-dir
+              (when revision
+                (treesit--git-checkout-branch workdir revision))
+            (treesit--git-clone-repo url revision workdir))
           ;; We need to go into the source directory because some
           ;; header files use relative path (#include "../xxx").
           ;; cd "${sourcedir}"
@@ -3635,7 +3655,9 @@ treesit--install-language-grammar-1
             ;; Ignore errors, in case the old version is still used.
             (ignore-errors (delete-file old-fname)))
           (message "Library installed to %s/%s" out-dir lib-name))
-      (when (file-exists-p workdir)
+      ;; Remove workdir if it's not a repo owned by user and we
+      ;; managed to create it in the first place.
+      (when (and (not url-is-dir) (file-exists-p workdir))
         (delete-directory workdir t)))))
 
 ;;; Etc
-- 
2.43.0

bug#68579: [PATCH v2] Support a local repo as URL in treesit-language-source-alist

[Eli Zaretskii]

> From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> Cc: 68579@debbugs.gnu.org
> Date: Sat, 27 Jan 2024 21:53:15 +0300
> 
> Thanks! Patch is attached, everything is addressed except one question
> above. I also fixed "one space" to two spaces" in one place in the
> commit description.

Thanks, installed on the master branch, and closing the bug.

bug#68579: [PATCH v2] Support a local repo as URL in treesit-language-source-alist

[Eli Zaretskii]

Now _really_ closing the bug.