From 209af95853c7d281779d422146953b9dcb9b6ce6 Mon Sep 17 00:00:00 2001 From: Thomas Hisch Date: Sun, 31 Jul 2022 16:27:20 +0200 Subject: [PATCH] Remove gerrit-rest--escape-project and add new sync-v2 function gerrit-rest--escape-project can be removed in favor of the url-build-query-string function, which escapes all fields in the query string. gerrit-rest--escape-project was also used to determine the unique-changeid. Since the branch name might also contain chars that need to be escaped, url-hexify-string is used on the whole unique-changeid string instead of gerrit-rest--escape-project. This change also fises some docstring issues reported by emacs-29. Change-Id: I9c72023a54c9d4444d9b9dbc7903aa7df2e60067 --- gerrit-rest.el | 248 +++++++++++++++++++++++++++++-------------------- gerrit.el | 29 +++--- 2 files changed, 161 insertions(+), 116 deletions(-) diff --git a/gerrit-rest.el b/gerrit-rest.el index 4b3a09a..c4f981b 100644 --- a/gerrit-rest.el +++ b/gerrit-rest.el @@ -37,7 +37,8 @@ (require 'cl-lib) (require 'cl-extra) ;; for cl-prettyprint -(defvar gerrit-host) +(defvar gerrit-host) ;; defined in gerrit.el +(declare-function gerrit--get-protocol "gerrit.el") (defvar gerrit-patch-buffer) (defvar url-http-end-of-headers) @@ -75,6 +76,49 @@ servers it needs to be set to an empty string." (json-false nil)) (json-read-from-string ,str)))) +(cl-defun gerrit-rest-sync-v2 (method endpoint + &key + params + data) + "Perform an API request to the ENDPOINT using METHOD. +Optional arg PARAMS may be provided to specify parmeters for the request url. +The optional arg DATA may be used as inputs for POST/PUT requests." + (let ((url-request-method method) + (url-request-extra-headers + `(("Content-Type" . "application/json") + ("Authorization" . ,(concat "Basic " (gerrit-rest-authentication))))) + (url-request-data data) + (target (concat (gerrit--get-protocol) + gerrit-host + gerrit-rest-endpoint-prefix + endpoint + (when params + (concat "?" (url-build-query-string params)))))) + + (with-current-buffer (url-retrieve-synchronously target t) + (if (string-equal method "POST") + ;; TODO read output and check if it was successful?? + t + (gerrit-rest--read-json + (progn + (goto-char url-http-end-of-headers) + ;; if there is an error in search-forward-regexp, write + ;; the buffer contents to a *gerrit-rest-status* buffer + (if-let ((pos (search-forward-regexp "^)]}'$" nil t))) + (buffer-substring pos (point-max)) + ;; ")]}'" was not found in the REST response + (let ((buffer (get-buffer-create "*gerrit-rest-status*")) + (contents (buffer-substring (point-min) (point-max)))) + (with-current-buffer buffer + (goto-char (point-max)) + (insert ?\n) + (insert (format "%s: %s (%s)" url-request-method target url-request-extra-headers)) + (insert ?\n) + (insert contents) + (error (concat "error with gerrit request (take a look at the " + "*gerrit-rest-status* buffer for more information"))))))))))) + +;; TODO deprecate gerrit-rest-sync (defun gerrit-rest-sync (method data &optional path) "Interact with the API using method METHOD and data DATA. Optional arg PATH may be provided to specify another location further @@ -109,14 +153,10 @@ down the URL structure to send the request." (error (concat "error with gerrit request (take a look at the " "*gerrit-rest-status* buffer for more information"))))))))))) -(defun gerrit-rest--escape-project (project) - "Escape project name PROJECT for usage in REST API requests." - (url-hexify-string project)) - (defun gerrit-rest-get-server-version () "Return the gerrit server version." (interactive) - (let ((versionstr (gerrit-rest-sync "GET" nil "/config/server/version"))) + (let ((versionstr (gerrit-rest-sync-v2 "GET" "/config/server/version"))) (when (called-interactively-p 'interactive) (message versionstr)) versionstr)) @@ -124,7 +164,7 @@ down the URL structure to send the request." (defun gerrit-rest-get-server-info () "Return the gerrit server info." (interactive) - (let ((serverinfo (gerrit-rest-sync "GET" nil "/config/server/info"))) + (let ((serverinfo (gerrit-rest-sync-v2 "GET" "/config/server/info"))) (when (called-interactively-p 'interactive) (switch-to-buffer (get-buffer-create "*gerrit-server-info*")) (erase-buffer) @@ -137,28 +177,25 @@ down the URL structure to send the request." (defun gerrit-rest-get-change-info (changenr) "Return information about an open change with CHANGENR." (interactive "sEnter a changenr name: ") - (let* ((req (concat "/changes/" - changenr - "?o=DOWNLOAD_COMMANDS&" - "o=CURRENT_REVISION&" - "o=CURRENT_COMMIT&" - "o=DETAILED_LABELS&" - "o=DETAILED_ACCOUNTS"))) - (gerrit-rest-sync "GET" nil req))) + (gerrit-rest-sync-v2 "GET" (format "/changes/%s/" changenr) + :params '(("o" "DOWNLOAD_COMMANDS") + ("o" "CURRENT_REVISION") + ("o" "CURRENT_COMMIT") + ("o" "DETAILED_LABELS") + ("o" "DETAILED_ACCOUNTS")))) (defun gerrit-rest-get-topic-info (topicname) "Return information about an open topic with TOPICNAME." ;; TODO create new buffer and insert stuff there ;; TODO query open topics (interactive "sEnter a topic name: ") - (let* ((fmtstr (concat "/changes/?q=is:open+topic:%s&" - "o=DOWNLOAD_COMMANDS&" - "o=CURRENT_REVISION&" - "o=CURRENT_COMMIT&" - "o=DETAILED_LABELS&" - "o=DETAILED_ACCOUNTS")) - (req (format fmtstr topicname))) - (gerrit-rest-sync "GET" nil req))) + (gerrit-rest-sync-v2 "GET" "/changes/" + :params `(("q" (concat "is:open+topic:" ,topicname)) + ("o" "DOWNLOAD_COMMANDS") + ("o" "CURRENT_REVISION") + ("o" "CURRENT_COMMIT") + ("o" "DETAILED_LABELS") + ("o" "DETAILED_ACCOUNTS")))) (defun gerrit-rest--get-gerrit-accounts () "Return an alist of all active gerrit users." @@ -168,7 +205,10 @@ down the URL structure to send the request." (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 "/accounts/?q=is:active&o=DETAILS&S=0")) + (gerrit-rest-sync-v2 "GET" "/accounts/" + :params '(("q" "is:active") + ("o" "DETAILS") + ("S" 0)))) (error '()))) (defun gerrit-rest-open-reviews-for-project (project) @@ -176,14 +216,13 @@ down the URL structure to send the request." (interactive "sEnter gerrit project: ") ;; see https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#list-changes (let* ((limit-entries 25) - (req (format (concat "/changes/?q=is:open+project:%s&" - "o=CURRENT_REVISION&" - "o=CURRENT_COMMIT&" - "o=DETAILED_LABELS&" - (format "n=%d&" limit-entries) - "o=DETAILED_ACCOUNTS") - (funcall #'gerrit-rest--escape-project project))) - (resp (gerrit-rest-sync "GET" nil req))) + (resp (gerrit-rest-sync-v2 "GET" "/changes/" + :params `(("q" (concat "is:open+project:" ,project)) + ("o" "CURRENT_REVISION") + ("o" "CURRENT_COMMIT") + ("o" "DETAILED_LABELS") + ("o" "DETAILED_ACCOUNTS") + ("n" ,limit-entries))))) ;; (setq open-reviews-response resp) ;; for debugging only (use M-x ielm) resp)) @@ -194,137 +233,139 @@ down the URL structure to send the request." "Set the assignee to ASSIGNEE of a change with nr CHANGENR." (interactive "sEnter a changenr: \nsEnter assignee: ") ;; TODO error handling? - (gerrit-rest-sync "PUT" - (encode-coding-string (json-encode - `((assignee . ,assignee))) 'utf-8) - (format "/changes/%s/assignee" changenr))) + (gerrit-rest-sync-v2 "PUT" + (format "/changes/%s/assignee" changenr) + :data (encode-coding-string (json-encode + `((assignee . ,assignee))) 'utf-8))) (defun gerrit-rest-change-add-reviewer (changenr reviewer) "Add REVIEWER to a change with nr CHANGENR." (interactive "sEnter a changenr: \nsEnter reviewer: ") ;; Do we want to verify the return entity? - (gerrit-rest-sync "POST" - (encode-coding-string (json-encode - `((reviewer . ,reviewer))) 'utf-8) - (format "/changes/%s/reviewers/%s" changenr reviewer))) + (gerrit-rest-sync-v2 "POST" + (format "/changes/%s/reviewers/%s" changenr reviewer) + :data (encode-coding-string (json-encode + `((reviewer . ,reviewer))) 'utf-8))) (defun gerrit-rest-change-delete-reviewer (changenr reviewer) "Delete REVIEWER from a change with nr CHANGENR." (interactive "sEnter a changenr: \nsEnter reviewer: ") - (gerrit-rest-sync "POST" - (encode-coding-string (json-encode - '((notify . "None"))) 'utf-8) - (format "/changes/%s/reviewers/%s/delete" changenr reviewer))) + (gerrit-rest-sync-v2 "POST" + (format "/changes/%s/reviewers/%s/delete" changenr reviewer) + :data (encode-coding-string (json-encode + '((notify . "None"))) 'utf-8))) (defun gerrit-rest-change-set-topic (changenr topic) "Set the topic to TOPIC of a change CHANGENR." (interactive "sEnter a changenr: \nsEnter topic: ") - (gerrit-rest-sync "PUT" - (encode-coding-string (json-encode - `((topic . ,topic))) 'utf-8) - (format "/changes/%s/topic" changenr))) + (gerrit-rest-sync-v2 "PUT" + (format "/changes/%s/topic" changenr) + :data (encode-coding-string (json-encode + `((topic . ,topic))) 'utf-8))) (defun gerrit-rest-change-delete-topic (changenr) "Delete the topic of a change CHANGENR." (interactive "sEnter a changenr: ") - (gerrit-rest-sync "DELETE" - nil - (format "/changes/%s/topic" changenr))) + (gerrit-rest-sync-v2 "DELETE" + (format "/changes/%s/topic" changenr))) (defun gerrit-rest-change-get-messages (changenr) ;; note that filenames are returned as symbols - (gerrit-rest-sync "GET" nil (format "/changes/%s/messages" changenr))) + (gerrit-rest-sync-v2 "GET" (format "/changes/%s/messages" changenr))) (defun gerrit-rest-change-get-comments (changenr) ;; note that filenames are returned as symbols - (gerrit-rest-sync "GET" nil (format "/changes/%s/comments" changenr))) + (gerrit-rest-sync-v2 "GET" (format "/changes/%s/comments" changenr))) (defun gerrit-rest-change-set-cr-vote (changenr vote message) "Set a Code-Review vote VOTE of a change CHANGENR. A comment MESSAGE can be provided." (interactive "sEnter a changenr: \nsEnter vote [-2, -1, 0, +1, +2]: \nsEnter message: ") - (gerrit-rest-sync "POST" - (encode-coding-string (json-encode - `((message . ,message) - (labels . - ((Code-Review . ,vote))))) 'utf-8) - (format "/changes/%s/revisions/current/review" changenr))) + (gerrit-rest-sync-v2 "POST" + (format "/changes/%s/revisions/current/review" changenr) + :data (encode-coding-string (json-encode + `((message . ,message) + (labels . + ((Code-Review . ,vote))))) 'utf-8))) (defun gerrit-rest-change-delete-cr-vote (changenr username) "Delete a Code-Review vote VOTE from a change CHANGENR from the user USERNAME." (interactive "sEnter a changenr: \nsEnter a username: ") - (gerrit-rest-sync "POST" - (encode-coding-string (json-encode - '((notify . "None"))) 'utf-8) - (format "/changes/%s/reviewers/%s/votes/Code-Review/delete" changenr username))) + (gerrit-rest-sync-v2 "POST" + (format "/changes/%s/reviewers/%s/votes/Code-Review/delete" + changenr username) + :data (encode-coding-string (json-encode + '((notify . "None"))) 'utf-8))) (defun gerrit-rest-change-set-verified-vote (changenr vote message) "Verify a change CHANGENR by voting with VOTE. A comment MESSAGE can be provided." (interactive "sEnter a changenr: \nsEnter vote [-1, 0, +1]: \nsEnter message: ") - (gerrit-rest-sync "POST" - (encode-coding-string (json-encode - `((message . ,message) - (labels . - ((Verified . ,vote))))) 'utf-8) - (format "/changes/%s/revisions/current/review" changenr))) + (gerrit-rest-sync-v2 "POST" + (format "/changes/%s/revisions/current/review" changenr) + :data (encode-coding-string (json-encode + `((message . ,message) + (labels . + ((Verified . ,vote))))) 'utf-8))) (defun gerrit-rest-change-delete-verified-vote (changenr username) "Delete a Verified vote VOTE from a change CHANGENR from te user USERNAME." (interactive "sEnter a changenr: \nsEnter a username: ") - (gerrit-rest-sync "POST" - (encode-coding-string (json-encode - '((notify . "None"))) 'utf-8) - (format "/changes/%s/reviewers/%s/votes/Verified/delete" changenr username))) + (gerrit-rest-sync-v2 "POST" + (format "/changes/%s/reviewers/%s/votes/Verified/delete" changenr username) + :data (encode-coding-string (json-encode + '((notify . "None"))) 'utf-8))) (defun gerrit-rest-change-set-Work-in-Progress (changenr) "Set the state of the change CHANGENR to Work-in-Progress." (interactive "sEnter a changenr: ") - (gerrit-rest-sync "POST" - (encode-coding-string (json-encode - `((message . ,"Set using gerrit.el"))) 'utf-8) - (format "/changes/%s/wip" changenr))) + (gerrit-rest-sync-v2 "POST" + (format "/changes/%s/wip" changenr) + :data (encode-coding-string + (json-encode + `((message . ,"Set using gerrit.el"))) 'utf-8))) (defun gerrit-rest-change-set-Ready-for-Review (changenr) "Set the state of the change CHANGENR to Reday-for-Review." (interactive "sEnter a changenr: ") - (gerrit-rest-sync "POST" - (encode-coding-string (json-encode - `((message . ,"Set using gerrit.el"))) 'utf-8) - (format "/changes/%s/ready" changenr))) + (gerrit-rest-sync-v2 "POST" + (format "/changes/%s/ready" changenr) + :data (encode-coding-string + (json-encode + `((message . ,"Set using gerrit.el"))) 'utf-8))) (defun gerrit-rest-change-add-comment (changenr comment) "Add a comment message COMMENT to latest version of change CHANGENR." (interactive "sEnter changenr: \nsEnter comment: ") - (gerrit-rest-sync "POST" - (encode-coding-string (json-encode - `((message . ,comment))) 'utf-8) - (format "/changes/%s/revisions/current/review" changenr))) + (gerrit-rest-sync-v2 "POST" + (format "/changes/%s/revisions/current/review" changenr) + :data (encode-coding-string + (json-encode + `((message . ,comment))) 'utf-8))) (defun gerrit-rest-change-get-labels (changenr) "Return the current labels dictionary of a change CHANGENR." (interactive "sEnter changenr: ") (let* ((req (format "/changes/%s/revisions/current/review" changenr)) - (resp (gerrit-rest-sync "GET" nil req))) + (resp (gerrit-rest-sync-v2 "GET" req))) (assoc 'labels (cdr resp)))) (defun gerrit-rest-change-query (expression) "Return information about changes that match EXPRESSION." (interactive "sEnter a search expression: ") - (gerrit-rest-sync "GET" nil - (concat (format "/changes/?q=%s&" expression) - "o=CURRENT_REVISION&" - "o=CURRENT_COMMIT&" - "o=LABELS" - ;; "o=DETAILED_LABELS" - ;; "o=DETAILED_ACCOUNTS")) - ))) + (gerrit-rest-sync-v2 "GET" "/changes/" + :params `(("q" ,expression) + ("o" "CURRENT_REVISION") + ("o" "CURRENT_COMMIT") + ("o" "LABELS") + ;; "o=DETAILED_LABELS" + ;; "o=DETAILED_ACCOUNTS" + ))) (defun gerrit-rest-change-get (changenr) "Return information about change with CHANGENR." (interactive "sEnter changenr: ") - (gerrit-rest-sync "GET" nil - (format "/changes/%s" changenr))) + (gerrit-rest-sync-v2 "GET" (format "/changes/%s" changenr))) (defun gerrit-rest-change-patch (changenr) "Download latest patch of change with CHANGENR and open it in new buffer. @@ -337,7 +378,9 @@ to CHANGENR is not locally cloned." (url-request-extra-headers `(("Authorization" . ,(concat "Basic " (gerrit-rest-authentication))))) (url-request-data nil) - (target (concat (gerrit--get-protocol) gerrit-host gerrit-rest-endpoint-prefix + (target (concat (gerrit--get-protocol) + gerrit-host + gerrit-rest-endpoint-prefix (format "/changes/%s/revisions/current/patch" changenr)))) (message "Opening patch of %s" changenr) (setq gerrit-patch-buffer (get-buffer-create "*gerrit-patch*")) @@ -364,11 +407,12 @@ to CHANGENR is not locally cloned." ;; topic commands (defun gerrit-rest--change-info-to-unique-changeid (change-info) - (concat (gerrit-rest--escape-project (alist-get 'project change-info)) - "~" - (alist-get 'branch change-info) - "~" - (alist-get 'change_id change-info))) + (url-hexify-string + (concat (alist-get 'project change-info) + "~" + (alist-get 'branch change-info) + "~" + (alist-get 'change_id change-info)))) (defun gerrit-rest-topic-set-assignee (topic assignee) "Set the ASSIGNEE of all changes of a TOPIC." diff --git a/gerrit.el b/gerrit.el index 28619f2..47d0c27 100644 --- a/gerrit.el +++ b/gerrit.el @@ -160,15 +160,15 @@ the user to select a change from a list of changes. Currently supported columns are: - 'number (the change number) - 'owner (the change owner) - 'branch (the branch of the change) - 'subject (the subject of the commit msg) - 'project (the project name)") + \='number (the change number) + \='owner (the change owner) + \='branch (the branch of the change) + \='subject (the subject of the commit msg) + \='project (the project name)") ;; TODO introduce a function? (defcustom gerrit-project-to-local-workspace-alist nil - "This alist can be used for specifying the 'known' gerrit projects. + "This alist can be used for specifying the \='known\=' gerrit projects. The alist is needed for determining the workspace directory for certain gerrit projects. @@ -189,7 +189,7 @@ Each element is a list comprising ((PROJECT BRANCH) WORKSPACE) ..." "Filter string used for querying gerrit changes. If you are interested only in the changes for certain projects, -you can use 'is:open (project:A OR project:B OR project:C)'" +you can use \='is:open (project:A OR project:B OR project:C)\='" :group 'gerrit :type 'string) @@ -219,7 +219,7 @@ you can use 'is:open (project:A OR project:B OR project:C)'" (defun gerrit-download--get-refspec (change-metadata) "Return the refspec of a gerrit change from CHANGE-METADATA. -This refspec is a string of the form 'refs/changes/xx/xx/x'." +This refspec is a string of the form \='refs/changes/xx/xx/x\='." ;; this is important for determining the refspec needed for ;; git-fetch ;; change-ref is e.g. "refs/changes/16/35216/2" @@ -616,7 +616,7 @@ The returned string is not prefixed with the remote." (cadr (s-split "/" upstream-branch))))) (defun gerrit-get-current-project () - "Return the gerrit project name, e.g., 'software/jobdeck'." + "Return the gerrit project name, e.g.,\='software/jobdeck\='." (interactive) (let* ((remote-url (car (magit-config-get-from-cached-list @@ -656,11 +656,12 @@ A string like the following is returned: myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940" (let ((branch (substring-no-properties (gerrit-get-upstream-branch))) (project (substring-no-properties (gerrit-get-current-project)))) - (concat (gerrit-rest--escape-project project) - "~" - branch - "~" - (gerrit-get-changeid-from-current-commit)))) + (url-hexify-string (concat + project + "~" + branch + "~" + (gerrit-get-changeid-from-current-commit)))))