Adding indirect branch / revision filtering

Most instances seem to not support direct branch / revision filtering
in Changes API requests, that is, requests such as

<host>/api/v2/changes?revision=<revision-id>

timeout or return an error code.

In such cases, we issue a more general API request, and filter the
response by revision:

<host>/api/v2/changes?limit=<limit>&order=-changeid

We add a new custom var `buildbot-api-changes-direct-filter' to
control the relevant behaviour.

The mariadb instance seems to be the only instance that work better
with direct filtering, whereas python and buildbot seems to work with
both, and the remaining instances only supporting indirect filtering.

We also add a few more custom vars for limiting in API requests, and
fix some regressions introduced in the previous commit, about setting
the host.

3 files changed, 111 insertions, 70 deletions

diff --git a/buildbot-client.el b/buildbot-client.el
index 1aaf55f..8d32c01 100644
--- a/buildbot-client.el
+++ b/buildbot-client.el
@@ -37,6 +37,45 @@ Can be generated with `buildbot-get-all-builders'.")
   :group 'buildbot
   :type 'string)
 
+(defcustom buildbot-builder-build-limit 100
+  "The limit in a request to the Build API filtered by builder.
+
+If set to nil, use server default, i.e. do not set limit in
+relevant Builds API requests."
+  :group 'buildbot
+  :type '(choice (const :tag "Server default" nil) natnum))
+
+(defcustom buildbot-branch-changes-limit 10
+  "The limit in a request to the Changes API filtered by branch.
+
+Only relevant when `buildbot-api-changes-direct-filter' is
+non-nil. If set to nil, use server default, i.e. do not set limit
+in relevant Changes API requests."
+  :group 'buildbot
+  :type '(choice (const :tag "Server default" nil) natnum))
+
+(defcustom buildbot-api-changes-limit 300
+  "The limit in a request to the Changes API.
+
+Only relevant when `buildbot-api-changes-direct-filter' is nil.
+If set to nil, use server default, i.e. do not set limit in
+Changes API requests.'"
+  :group 'buildbot
+  :type '(choice (const :tag "Server default" nil) natnum))
+
+(defcustom buildbot-api-changes-direct-filter nil
+  "Method to filter revision or bracnh in the Changes API requests.
+
+If non-nil, filter directly in API request, otherwise call the
+API with a limit, and filter from the response.
+
+Direct filtering is more accurate, but may have extremely high
+latency or may be unsupported in some hosts. Most (all?) hosts
+support indirect filtering, but depending on the limit, it may
+miss some changes associated to the required revision or branch."
+  :group 'buildbot
+  :type 'boolean)
+
 (defun buildbot-api-change (attr)
   "Call the Changes API with ATTR."
   (buildbot-url-fetch-json
@@ -91,12 +130,12 @@ Can be generated with `buildbot-get-all-builders'.")
   (buildbot-url-fetch-raw
    (format "%s/api/v2/logs/%d/raw" buildbot-host logid)))
 
-(defun buildbot-get-recent-builds-by-builder (builder-id limit)
+(defun buildbot-get-recent-builds-by-builder (builder-id)
   "Get LIMIT number of recent builds by the builder with BUILDER-ID."
   (alist-get 'builds
              (buildbot-api-builders-builds
               builder-id
-              `((limit . ,limit)
+              `((limit . ,buildbot-builder-build-limit)
                 (order . "-number")
                 (property . "revision")))))
 
@@ -133,8 +172,17 @@ Can be generated with `buildbot-get-all-builders'.")
 (defun buildbot-get-changes-by-revision (revision)
   "Get the changes from a REVISION."
   (let ((changes
-         (alist-get 'changes
-                    (buildbot-api-change `((revision . ,revision))))))
+         (if buildbot-api-changes-direct-filter
+             (alist-get
+              'changes
+              (buildbot-api-change `((revision . ,revision))))
+           (seq-filter
+            (lambda (change)
+              (equal revision (alist-get 'revision change)))
+            (alist-get
+             'changes
+             (buildbot-api-change `((limit . ,buildbot-api-changes-limit)
+                                    (order . "-changeid"))))))))
     (mapcar
      (lambda (change)
        (if (assq 'builds change)
@@ -153,12 +201,20 @@ Can be generated with `buildbot-get-all-builders'.")
   "Get the steps of a build with BUILDID."
   (alist-get 'steps (buildbot-api-step buildid)))
 
-(defun buildbot-get-changes-by-branch (branch-name limit)
+(defun buildbot-get-changes-by-branch (branch-name)
   "Get LIMIT number of changes of a branch with BRANCH-NAME."
-  (alist-get 'changes
-             (buildbot-api-change
-              (cons `(branch . ,branch-name)
-                    (when limit `((limit . ,limit)))))))
+  (if buildbot-api-changes-direct-filter
+      (alist-get
+       'changes
+       (buildbot-api-change `((branch . ,branch-name)
+                              (limit . ,buildbot-branch-changes-limit))))
+    (seq-filter
+     (lambda (change)
+       (equal branch-name (alist-get 'branch change)))
+     (alist-get
+      'changes
+      (buildbot-api-change `((limit . ,buildbot-api-changes-limit)
+                             (order . "-changeid")))))))
 
 (provide 'buildbot-client)
 ;;; buildbot-client.el ends here
diff --git a/buildbot-utils.el b/buildbot-utils.el
index f28ab75..538844e 100644
--- a/buildbot-utils.el
+++ b/buildbot-utils.el
@@ -97,10 +97,12 @@ With non-nil WITH-HEADER, include the header in the result."
 
 (defun buildbot-format-attr (attr)
   "Format an alist ATTR into a url query string."
-  (string-join (mapcar (lambda (pair)
-                         (format "%s=%s" (car pair) (cdr pair)))
-                       attr)
-               "&"))
+  (string-join
+   (mapcar
+    (lambda (pair)
+      (format "%s=%s" (car pair) (cdr pair)))
+    (seq-filter #'cdr attr))
+   "&"))
 
 (defun buildbot-format-epoch-time (epoch)
   "Format an EPOCH."
diff --git a/buildbot-view.el b/buildbot-view.el
index 4898353..0cc9b26 100644
--- a/buildbot-view.el
+++ b/buildbot-view.el
@@ -32,10 +32,10 @@
 
 (defvar buildbot-view-header-regex "^\\[.*\\]$"
   "The header regex in a Buildbot buffer.")
-(defvar buildbot-view-branch-change-limit 10)
-(defvar buildbot-view-builder-build-limit 50)
-;; 'revision, 'build, 'step, or 'log
-(defvar-local buildbot-view-type nil)
+(defvar-local buildbot-view-type nil
+  "The type of the Buildbot view.
+
+One of `revision', `build', `step', or `log'.")
 (defvar-local buildbot-view-data nil)
 
 (defvar buildbot-view-mode-map
@@ -45,7 +45,7 @@
     (define-key kmap "f" #'buildbot-view-next-header-same-thing)
     (define-key kmap (kbd "M-p") #'buildbot-view-previous-header)
     (define-key kmap "p" #'buildbot-view-previous-failed-header)
-    (define-key kmap (kbd "b") #'buildbot-view-previous-header-same-thing)
+    (define-key kmap "b" #'buildbot-view-previous-header-same-thing)
     (define-key kmap "g" #'buildbot-view-reload)
     (define-key kmap (kbd "<return>") #'buildbot-view-open-thing-at-point)
     kmap)
@@ -339,12 +339,15 @@ Finally, call `buildbot-get-all-builders' to get the builders."
       (buildbot-builders-same-host host)
       (let ((buildbot-host host)) (buildbot-get-all-builders))))
 
-(defun buildbot-view-open (type data &optional force)
+(defun buildbot-view-open (type data &optional force host)
   "Open a view of TYPE using DATA.
 
-With a non-nil FORCE, reload the view buffer if exists."
-  (let ((buffer-name (buildbot-view-buffer-name type data))
-        (host buildbot-host))
+With a non-nil FORCE, reload the view buffer if exists.
+
+With a non-nil HOST, set the `buildbot-host' of the view buffer,
+otherwise pass the value from the current buffer."
+  (unless host (setq host (or buildbot-host buildbot-default-host)))
+  (let ((buffer-name (buildbot-view-buffer-name type data)))
     (when (or force (not (get-buffer buffer-name)))
       (with-current-buffer (get-buffer-create buffer-name)
         (buildbot-view-mode)
@@ -362,26 +365,6 @@ With a non-nil FORCE, reload the view buffer if exists."
   (interactive)
   (buildbot-view-update))
 
-(defun buildbot-open-with-host (open-fun)
-  "Cal OPEN-FUN after reading a specified host."
-  (let ((buildbot-host (read-string "Buildbot host: ")))
-    (funcall open-fun)))
-
-(defun buildbot-revision-open-internal (revision)
-  "Open a revision view of REVISION id."
-  (buildbot-view-open 'revision `((revision . ,revision))))
-
-(defun buildbot-branch-open-internal (branch)
-  "Open a branch view of BRANCH name."
-  (buildbot-view-open 'branch `((branch . ,branch))))
-
-(defun buildbot-builder-open-internal (builder-name)
-  "Open a builder view of BUILDER-NAME."
-  (buildbot-view-open
-   'builder
-   (list (cons 'builder
-               (buildbot-builder-by-name builder-name)))))
-
 ;;;###autoload
 (defun buildbot-revision-open (&optional read-host)
   "Open a revision view.
@@ -389,12 +372,12 @@ With a non-nil FORCE, reload the view buffer if exists."
 With a nonnil prefix arg READ-HOST, will prompt for the host
 first."
   (interactive "P")
-  (let ((thunk (lambda ()
-                 (buildbot-revision-open-internal
-                  (read-string "Revision (e.g. commit hash): ")))))
-    (if read-host
-        (buildbot-open-with-host thunk)
-      (funcall thunk))))
+  (let ((host (when read-host (read-string "Buildbot host: "))))
+    (buildbot-view-open
+     'revision
+     `((revision . ,(read-string "Revision (e.g. commit hash): ")))
+     nil
+     host)))
 
 ;;;###autoload
 (defun buildbot-branch-open (&optional read-host)
@@ -403,12 +386,12 @@ first."
 With a nonnil prefix arg READ-HOST, will prompt for the host
 first."
   (interactive "P")
-  (let ((thunk (lambda ()
-                 (buildbot-branch-open-internal
-                  (read-string "Branch: ")))))
-    (if read-host
-        (buildbot-open-with-host thunk)
-      (funcall thunk))))
+  (let ((host (when read-host (read-string "Buildbot host: "))))
+    (buildbot-view-open
+     'branch
+     `((branch . ,(read-string "Branch: ")))
+     nil
+     host)))
 
 ;;;###autoload
 (defun buildbot-builder-open (read-host)
@@ -417,18 +400,20 @@ first."
 With a nonnil prefix arg READ-HOST, will prompt for the host
 first."
   (interactive "P")
-  (let ((thunk (lambda ()
-                 (let ((buildbot-builders
-                        (buildbot-get-builders-smart)))
-                   (buildbot-builder-open-internal
-                    (completing-read
-                     "Builder name: "
-                     (mapcar
-                      (lambda (builder) (alist-get 'name builder))
-                      buildbot-builders)))))))
-    (if read-host
-        (buildbot-open-with-host thunk)
-      (funcall thunk))))
+  (let* ((host (when read-host (read-string "Buildbot host: ")))
+         (buildbot-builders
+          (buildbot-get-builders-smart host)))
+    (buildbot-view-open
+     'builder
+     `((builder .
+                ,(buildbot-builder-by-name
+                  (completing-read
+                   "Builder name: "
+                   (mapcar
+                    (lambda (builder) (alist-get 'name builder))
+                    buildbot-builders)))))
+     nil
+     host)))
 
 (defun buildbot-view-update ()
   "Refresh a view."
@@ -441,8 +426,7 @@ first."
        (insert (buildbot-branch-format
                 (alist-get 'branch buildbot-view-data)
                 (buildbot-get-changes-by-branch
-                 (alist-get 'branch buildbot-view-data)
-                 buildbot-view-branch-change-limit))))
+                 (alist-get 'branch buildbot-view-data)))))
       ('revision
        (let ((revision-and-changes-info
               (buildbot-get-revision-and-changes-info
@@ -455,8 +439,7 @@ first."
        (let* ((builder (alist-get 'builder buildbot-view-data))
               (builds
                (buildbot-get-recent-builds-by-builder
-                (alist-get 'builderid builder)
-                buildbot-view-builder-build-limit)))
+                (alist-get 'builderid builder))))
          (insert (buildbot-builder-format builder builds))))
       ('build
        (let ((revision (alist-get 'revision buildbot-view-data)))