only the first page of gerrit account results get fetched

[rmmanseau]

by default the gerrit API implements pagination. the instance of gerrit returns a maximum of 500 users per page, even tho there are over 2000 users. this plugin will only fetch the first page.

fixed in my config with this brittle hack that assumes the page length will be 500. works for now : )

 ;; override function in gerrit-rest.el
 (defun gerrit-rest--get-gerrit-accounts ()
   "Return an alist of all active gerrit users."
   (interactive)
   (condition-case nil
       (let ((accounts (list))
             (batch (list))
             (n 0)
             (stop nil))
         (while (not stop)
           (progn
             (setq batch
                   (mapcar (lambda (account-info) (cons (cdr (assoc '_account_id account-info))
                                                        (cdr (assoc 'username account-info))))
                           ;; see https://gerrit-review.googlesource.com/Documentation/rest-api-accounts.html
                           ;; and https://gerrit-review.googlesource.com/Documentation/user-search-accounts.html#_search_operators
                           (gerrit-rest-sync "GET" nil (concat "/accounts/?q=is:active&o=DETAILS&S="
                                                               (number-to-string n)))))
             (if (not (eq (length batch) 500))
                 (setq stop t))
             (setq n (+ n (length batch)))
             (setq accounts (append accounts batch))
             ))
         accounts)
     (error '())))

[twmr]

This is a duplicate of #8, which contains an improved version of gerrit-rest--get-gerrit-accounts. Unfortunately calling this is quite slow. I guess it makes sense that we allow the user of gerrit.el to write the accounts to a file and use the file instead of performing the query after every emacs restart.

[rmmanseau]

thanks for the link, your code is a lot more robust 😅

caching in a file seems like a good idea as long as theres an intuitive way to overwrite the file with a re-fetch in the case that new users have been added.

the call is slow, but i think thats a significantly better outcome that opening the gerrit dashboard and seeing that the owner field is mysteriously missing for a lot of changes.

[twmr]

caching in a file seems like a good idea as long as theres an intuitive way to overwrite the file with a re-fetch in the case that new users have been added.

Yes, this is an important point! I was thinking about a new function that refreshes the cache and updates the file. We could output a warning when the file is older than a configurable amount of days and also ask the user to perform the update now or later (similar to the way this is done in e.g. oh-my-zsh).

the call is slow, but i think thats a significantly better outcome that opening the gerrit dashboard and seeing that the owner field is mysteriously missing for a lot of changes.

I agree. A warning in the *Warnings* buffer for this case would at least be nice.

[twmr]

@rmmanseau I've tried fetching more than ten pages with the code in #8 and I get a

Cannot go beyond page 10 of results error I go beyond the 10th page on review.gerrithub.io.

Do you have the same issue with your gerrit server?

[rmmanseau]

It appears that the 10 page limit error happens in the review.gerrithub.io UI as well:

I definitely don't run into page limits like that on the gerrit server at my work, so I dont think its a gerrit thing. my guess is that this is specific to review.gerrithub.io, maybe to avoid too much traffic from ppl testing scripts

[twmr]

my guess is that this is specific to review.gerrithub.io, maybe to avoid too much traffic from ppl testing scripts

I also thought about that, but this check is actually part of the upstream gerrit code: https://gerrit-googlesource.g.globit.com/gerrit/+/166d58ff5d872ffa3ba028a7d3aeffcf5dc9afea/java/com/google/gerrit/index/query/QueryProcessor.java#241

Here the check was introduced https://gerrit-googlesource.g.globit.com/gerrit/+/f56d365d04f

[rmmanseau]

looks like its up to the server to disable that check or not, no way for the plugin to work around it.

I think a sane thing for the plugin to do would be fetch as many pages as it can & if it can't fetch all of them print a warning to *Warnings*. then carry on with whatever users it managed to grab.

[rmmanseau]

is there anything blocking this? the implementation looks fine to me.

i've been overriding gerrit-rest--get-gerrit-accounts in my config to get account fetching to work, but i recently upgraded this package to get the --no-verify option in the transient and didn't notice the previous changes to gerrit-rest.el that happened here: https://github.com/thisch/gerrit.el/commit/a6451dc02bf630be371965fd03722ba4b30e4ede

i spent about an hour trying to figure out why gerrit.el was broken before finding this.

updating the overridden version as so fixed it

 (defun gerrit-rest--get-gerrit-accounts ()
   "Return an alist of all active gerrit users."
   (interactive)
   (condition-case nil
       (let ((continue t)
             (start-idx 0)
             (accounts '()))
         (while continue
           (let ((response
                  ;; see https://gerrit-review.googlesource.com/Documentation/rest-api-accounts.html
                  ;; and https://gerrit-review.googlesource.com/Documentation/user-search-accounts.html#_search_operators
                  (gerrit-rest-sync "GET" nil (format "/accounts/?q=is:active&o=DETAILS&S=%s" start-idx))))
             (setq accounts (append
                             accounts
                             (mapcar (lambda (account-info)
                                       (cons (alist-get '_account_id account-info)
                                             account-info))
                                     response)
                             ))
             (setq start-idx (+ start-idx (length response)))
             ;; (message "start: %s" start-idx)
             (setq continue (alist-get '_more_accounts (car (last response))))))
         accounts)
     (error '())))

[twmr]

is there anything blocking this?

No, actually not.

i spent about an hour trying to figure out why gerrit.el was broken before finding this.

I'm sry to hear that.

Should not be fixed in master.