On 20 Feb 2025, at 21:29, Philip Kaludercic <philipk@posteo.net> wrote:
[…]
Until then, I have taken a look at the code and left a few comments and
suggestions in the following diff. None of them are blocking to add the
package:
Many thanks for the review, it’s highly appreciated! I’ll merge a patch shortly; some answers to comments below.
diff --git a/project-jj.el b/project-jj.el
index 9431a1d..869e88d 100644
--- a/project-jj.el
+++ b/project-jj.el
@@ -33,13 +33,13 @@
(lambda (dir)
(let ((default-directory dir))
(mapcar
- #'expand-file-name
- (process-lines "jj" "file" "list"))))
+ #'expand-file-name ;can you use the template language to expand the path names
+ (process-lines "jj" "file" "list")))) ;why not pass the `dirs' as arguments to the process?
(or dirs
(list (project-root project)))))
Done, but we need to absolutify the resulting filenames ("jj file list" prints relative names); I’m using `file-name-concat’ instead of `expand-file-name’ in the hope that it’s cheaper.
diff --git a/vc-jj.el b/vc-jj.el
index c0c92e9..d39a126 100644
--- a/vc-jj.el
+++ b/vc-jj.el
@@ -51,7 +55,7 @@
(defcustom vc-jj-log-template "builtin_log_compact"
"The template to use for `vc-print-log'."
- :type '(radio (const "builtin_log_oneline")
+ :type '(radio (const "builtin_log_oneline") ;any specific reason for choosing `radio'?
We want a choice between some constant strings and a free-form string:
There’s some predefined templates that the user might want to choose instead of writing a fully custom template as a freeform string, which is also possible. The names of these templates are somewhat hidden in jj's help text, so we make them constant and clickable via the radio widget.
@@ -73,7 +80,7 @@
(with-temp-buffer
(and (= 0 (call-process "jj" nil t nil "diff" "--summary" "--" file))
(not (= (point-min) (point-max)))
- (progn (goto-char (point-min)) (looking-at "A ")))))
+ (progn (goto-char (point-min)) (looking-at "A "))))) ;perhaps `looking-at-p', unless you want to use the match data?
Done throughout the file
(defun vc-jj--file-conflicted (file)
(with-temp-buffer
@@ -81,6 +88,9 @@
(not (= (point-min) (point-max)))
(progn (goto-char (point-min)) (looking-at file)))))
+;; can you explain your logic here? It seems a bit hacky, and I feel
+;; that we could improve it a bit.
+
;;;###autoload (defun vc-jj-registered (file)
;;;###autoload "Return non-nil if FILE is registered with jj."
;;;###autoload (if (and (vc-find-root file ".jj") ; Short cut.
@@ -100,19 +110,18 @@
I didn’t yet look at your and Stefan’s exchange later, will cross-check and fix accordingly when I get there :)
@@ -169,36 +182,37 @@ self.conflict(), \"\\n\",
self.divergent(), \"\\n\",
self.hidden(), \"\\n\"
)"))
- (status (concat
- (when (string= conflict "true") "(conflict)")
- (when (string= divergent "true") "(divergent)")
- (when (string= hidden "true") "(hidden)")))
+ (status (concat ;can these all occur?
Maybe not all of the statuses can occur at the same time, but the repo can be in these statuses according to the template language. In the test suite I only check for the conflicted status so far, which displays as expected. Hmm, glancing at
https://jj-vcs.github.io/jj/latest/templates/#commit-type I should also include (immutable); noted down.
+ (concat
+ (propertize (format "% -11s: " key) 'face 'vc-dir-header)
+ ;; there is no header value emphasis face, so we
+ ;; use vc-dir-status-up-to-date for the prefix.
+ (when prefix (propertize prefix 'face 'vc-dir-status-up-to-date)) ;`propertize' works on a copy of the string, this has no effect!
It should be fine: the resulting propertized copy is what ends up in the displayed string, and I do see the prefix colorized in the expected way. I changed `when’ to `and’ though :)
+ (propertize value 'face 'vc-dir-header-value))))
+ (string-join (seq-remove ;you don't need this, "nil stands for the empty string."
+ #'null
If we don’t do this we get extra empty lines, see (string-join ‘(nil nil nil) “\n”)
@@ -206,6 +220,7 @@ self.hidden(), \"\\n\"
(defun vc-jj-mode-line-string (file)
"Return a mode line string and tooltip for FILE."
(pcase-let* ((long-rev (vc-jj-working-revision file))
+ ;; generally speaking, how robust is this? are you sure the output will always look right and what happens if that isn't the case?
It’s quite robust, since the template specifies the exact format of the output (two lines and what they must contain). We have a lot of control over the output of jj via its template language for jj subcommands that take a -T parameter; unfortunately, not all of them do.
@@ -302,8 +325,9 @@ self.hidden(), \"\\n\"
".gitignore"))))
(expand-file-name
".gitignore"
- (if (string-prefix-p (file-name-as-directory root)
- (file-name-as-directory ignore))
+ (if (locate-dominating-file
+ (lambda (dir) (file-equal-p dir root))
+ ignore)
ignore
root))))
This one I don’t understand; according to its docstring, locate-dominating-file takes a function only for its second argument. I’ll try to simplify the logic anyway; left a TODO.