If you want to upload a change/commit that is already up2date, gerrit returns the following message in the git push call

 ! [remote rejected]   HEAD -> refs/for/main (no new changes)
error: failed to push some refs to 'gerrit.example.org:software/demo'

This is currently only visible in the magit-process buffer. It would be nice if this message were also output in the echo area.

If git-review is called with unstaged changes, I get the following output:

Errors running git rebase -p -i remotes/origin/version10
error: cannot rebase: You have unstaged changes.
error: Please commit or stash them.
It is likely that your change has a merge conflict. You may resolve it
in the working tree now as described above and then run 'git review'
again, or if you do not want to resolve it yet (note that the change
can not merge until the conflict is resolved) you may run 'git rebase
--abort' then 'git review -R' to upload the change without rebasing.

gerrit.el should temporarily stash changes before calling git-review.

I'll try to fix this issue in git-review.

What is still missing in gerrit.el is support for reviewing gerrit changes from within emacs. The reason why this hasn't been done yet, is that it was not clear how an emacs UI for this could look like. I've just stumbled upon an emacs pkg that adds support for reviews on github, which is https://github.com/charignon/github-review. The UI that is used there could also be used for gerrit.el.
If you are aware of similar (emacs) projects, please let me know.

Sometimes it happens that I'm not sure if the checked-out branch is in sync with gerrit, i.e., that all not-yet-submitted changes are on gerrit and are up2date.

It would be nice if e.g. magit-status showed in the Unmerged into section the following info for each commit

• was the change uploaded to gerrit
• has the change on gerrit already been submitted
• is the local version or the version on the server is newer

• gerrit-upload could skip the git push call if the version on the server is the same as the local version of the current branch/commit.

A new function would be nice that:

• finds the workspace of the project on the host (we need to introduce a mapping from (projectname + branch) to the directory of the workspace on the host)
• switches the working directory to this workspace and downloads the change
• opens a overview of the change in which
  ** modified files can easily be opened in a new buffer
  ** the displayed diff hunks can easily be edited inline (requires a fix for magit/magit#2938)

Functions which can be used for implementing this feature:

• (magit-show-commit "HEAD")
• magit-diff-visit-worktree-file
• (gerrit-download)

The idea is to write a new minor mode that can be used for displaying all kinds of tables with headers.

I think it makes sense to check if there are uncommitted changes in the workspace at the beginning of gerrit-download and if yes, ask the user if he/she wants to stash/unstash(=autostash) them.

If you upload a change that is already on gerrit it should be possible to somehow display the local changes (if there are any) between the local HEAD and the latest patchset of the change on gerrit.

If your local commit/change is in sync with the latest PS of the change on gerrit, gerrit-upload should output an error/warning.

To be performed steps:

• find the change with the changeid of the current change on the gerrit server and fetch (only fetch!) it
• if HEAD and FETCH_HEAD are identical, output a warning, otherwise
• diff HEAD against the FETCH_HEAD and display it in new buffer

The google gerrit server (gerrit.googlesource.com) requires authentification using cookies, which is currently not supported by gerrit.el. Unfortunately the underlying url.el AFAIK does not have support for this, but curl does:

curl --cookie ~/.gitcookies https://gerrit-review.googlesource.com/a/config/server/info 

This new function should have two optional parameters: change-id and patchset.

A command that displays a dashboard similar to the one shown in the web interface would be useful.
Using tabulated-list-mode seems to be quite easy to do this, but it does not have support for the sections ("Assigned Reviews", "Work in Progress", ...)

It happened to me already several times that I wanted to upload a commit (or commits) to gerrit in a repo that doesn't have the gerrit commit-msg hook, which is responsible for adding the ChangeId line to the commit-msg.

In a recent commit I've added support for checking if the commit contains a change-id line before the gerrit-upload transient is shown. This avoids that you have to re-enter the gerrit-upload transient settings (assignee, topic, ..) after realizing that gerrit didn't accept the new commit due to a missing change-id line.

It would be nice if we could instead add a hook to git-commit-mode that checks if the upstream is a repo on a gerrit server downloads the commit-msg hook if it does not yet exist for the current repo.

assignees no longer exit in recent gerrit release, instead attention sets can now be used (
https://gerrit-review.googlesource.com/Documentation/user-attention-set.html)

• Add support for displaying reviewers in the dashboard
• Add support for displaying the attention set in the dashboard
• Add support for changing the attention set in the dashboard

Hi Thomas,

just wanted to drop by and say hello and check if you have had any time to try out the egerrit package. I wanted to let you know that I have moved the development to sourcehut, so you will find the package here https://git.sr.ht/~niklaseklund/egerrit.

Have you given it any more thoughts to possibilities for collaboration? :). I have focused on some other development recently but picked up the package the other week and made some more improvements to it. So then I came to think about your package :)

/Niklas

Since it doesn't seem that #2 will soon be fixed in gerrit and in git-review and since not everyone is using git-review, we should remove the usage of git-review from gerrit.el and use REST calls instead.

The user-interface of gerrit-download and gerrit-upload should be kept and only slightly adapted.

DONE which features of git-review do we want to implement in gerrit.el?
DONE how do we determine the target branch for a to-be-uploaded gerrit-change? git-review reads .gitreview file, which may contain a branch name that is used for gerrit changes.
DONE who sets-up the pre-commit hooks that ensure that a "Change-ID" line is part of the commit message?

gerrit-download

This implementation could be used instead of the current gerrit-download function. It uses the REST API for querying the open changes in the current project and uses magit for downloading the change.

(defun gerrit-format-change (change)
  (concat
   (propertize (number-to-string (alist-get '_number change)) 'face 'magit-hash)
   " "
   (propertize (alist-get 'branch change) 'face 'magit-branch-remote)
   " "
   (propertize (alist-get 'subject change) 'face 'magit-section-highlight)
   ))

(defun gerrit--get-refspec(change-metadata)
  ;; this is important for determining the refspec needed for
  ;; git-fetch
  ;; change-ref is e.g. "refs/changes/16/35216/2"
  (let* ((revisions (alist-get 'revisions change-metadata))
         (revision (alist-get 'current_revision change-metadata)))
    (gerrit--alist-get-recursive (intern revision) 'ref revisions)))

(defun gerrit--download-change (change-metadata)
  (let* ((change-nr (alist-get '_number change-metadata))
         (change-branch (alist-get 'branch change-metadata))
         (change-topic (or (alist-get 'topic change-metadata)
                           (number-as-string change-nr)))
         ;; TODO ensure that gerrit--acounts-alist is initialized
         (change-owner (alist-get (gerrit--alist-get-recursive
                                   'owner '_account_id change-metadata)
                                  gerrit--accounts-alist))
         (local-branch (format "reviews/%s/%s" change-owner change-topic)))

    ;; TODO log messages?

    ;; this runs async, which is not what I want, because then I can't
    ;; run other git commands afterwards
    ;; (magit-fetch-refspec (gerrit-get-remote) (gerrit--get-refspec change-metadata) nil)
    (magit-call-git "fetch" (gerrit-get-remote) (gerrit--get-refspec change-metadata))
    ;; TODO handle errors of magit-branch-and-checkout:
    ;; check if a local branch with this name already exists
    ;; if yes, determine the remote and remote-branch of this local-branch
    ;;         and reuse it only if they are equal with the current-remote
    ;;         (gerrit-get-remote)
    ;;              and `change-branch`
    (magit-branch-and-checkout local-branch "FETCH_HEAD")))


(defun gerrit-download-new ()
  "Download change from the gerrit server."
  (interactive)
  (let* ((open-changes
          (seq-map #'gerrit-format-change (gerrit-rest-change-query
                                    (concat "status:open project:"
                                            (gerrit-get-current-project)))))
         (selected-line (completing-read
                         "Download Change: " open-changes nil nil))
         (changenr (car (s-split " " (s-trim selected-line))))

         ;; the return value of `gerrit-rest-change-query` contains the
         ;; current revision, but not the one of `gerrit-rest-change-get`.
         (change-metadata (car (gerrit-rest-change-query changenr))))

    (gerrit--download-change change-metadata)))

https://github.com/thisch/gerrit.el/blob/4de561d1295d4c86ca9b159ab0c746bedc2d0380/gerrit.el#L433-L434

the above line in the condition of gerrit-upload:--action causes the function to error out if there are unknown transient args rather than just ignoring them. I tried adding an argument to the upload transient that would enable an env var before calling the function and then disable it afterwards, but it ended up not working because the function would error out at the above lines when it got around to parsing the new argument I injected into the transient. See my snippets of code here

;;
;; add advice that enables skipping the pre push hook by setting an env var before
;; calling the upload function and then unsetting it after
;;
(defadvice gerrit-upload:--action (around advice-gerrit-upload:--action activate)
  (interactive)
  (if (called-interactively-p 'any)
      (progn
        (cl-loop for arg in (transient-args 'gerrit-upload-transient) do
                 (cond ((string= "skip-checks" arg)
                        (setenv "GIT_SKIP_PRE_PUSH_HOOK" "1"))))
        (call-interactively (ad-get-orig-definition 'gerrit-upload:--action))
        (setenv "GIT_SKIP_PRE_PUSH_HOOK" nil))
    ad-do-it))

(transient-insert-suffix 'gerrit-upload-transient (list 0 -1)
  '("s" "Skip checks" "skip-checks"))

I ended up overriding gerrit-upload:--action and deleting the t condition but it would be nice if that change were upstreamed into this repo so that I could implement this only using advice.

  1 integrationtests/ git … checkout review/thomas_hisch/58720
error: Your local changes to the following files would be overwritten by checkout:
	integrationtests/test_data.py
	pytest.ini
Please commit your changes or stash them before you switch branches.
Aborting

  0 integrationtests/ git … reset --hard FETCH_HEAD
HEAD is now at f778b6a WIP add testcase

We have to check the return value of the git checkout command before we start the git reset command.

The exit code is returned by magit-call-git, but currently we are ignoring the return value. The respective code that causes a problem looks like

(progn
(magit-call-git "foobar1")
(magit-call-git "foobar2")) ;; foobar2 is run even though foobar1 failed

The GET /accounts/ endpoint returns a max number of 500 accounts. If the gerrit server has more than 500 accounts registered, then the client needs to call the /accounts endpoints multiple times.

See
https://gerrit-review.googlesource.com/Documentation/rest-api-accounts.html

gerrit.el is currently limited to 500 accounts, but the following patch should fix that. The problem is that querying a couple of thousand accounts is very slow.

(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)
                                           (alist-get 'username 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 '())))

For tests the review.gerrithub.org gerrit instance can be used, which has between 10000 and 20000 accounts.

For comparison here is a simple shell script that uses curl to download the accounts.
TODO output some numbers

#!/bin/bash
for i in `seq 0 500 10000`; do
  echo $i;
  curl --silent -o out/$i.out -n https://review.gerrithub.io/a/accounts/?q=is:active\&o=DETAILS\&S=$i
done

steps to reproduce:

1. open gerrit dashboard
2. click on one of the repo buttons, ie abc ( opens buffer called gerrit:project:abc)
3. click on one of the owner buttons, ie xyz (open buffer called gerrit:owner:xyz)
4. click on a button pointing to repo abc

expected behavior:
gerrit:project:abc buffer opens and the contents reflect the abc repo

actual behavior:
gerrit:project:abc buffer opens but the contents never update, they still reflect the contest of gerrit:owner:xyz

the only way to get the proper contents of gerrit:project:abc to show is to delete the buffer that already exists before clicking on the button again.

its not a huge issue but its kind of inconvenient to have to be aware of which gerrit dashboards are open. itd be nice to be able to click the buttons and have them work as expected regardless of which buffers are currently open.

thanks for the project btw! upload & such work perfectly and have been very helpful : )

git-review does not support setting the assignee via a command line option. The reason for this is that gerrit (server) doesn't provide a push_option for setting assignees.

See

• https://storyboard.openstack.org/#!/story/2004945
• https://bugs.chromium.org/p/gerrit/issues/detail?id=6154&q=assignee&colspec=ID%20Type%20Stars%20Status%20Priority%20Owner%20Summary

As long as git-review doesn't have a means to set the assignee, we have to parse the change number(s) from the output of git-review and set the assignee via REST API calls.

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 '())))