Allow reviewing individual commits

[charignon]

Same entry point as for PR.

[charignon]

Tested end to end on https://github.com/charignon/github-review/commit/0581a6079ebc1404c873aef133ee66d8bb7217c8

[andrelkin]

At testing one observation if found. The commit review start function does not pick up

+  "Start review given commit URL given COMMIT-ALIST."
+  (deferred:$
+    (deferred:parallel
+      ;; Get the diff
+      (lambda () (github-review-get-commit-deferred commit-alist t))
+      ;; And the PR object
+      (lambda () (github-review-get-commit-deferred commit-alist nil)))
-----> 	(when github-review-fetch-top-level-and-review-comments)
             (lambda () (github-review-get-commit-comments-deferred pr-alist))
+    (deferred:nextc it```
comments. I am suggesting where it is supposed to be called based on the PR version. In my local patch I have it defined as

```+ (defun github-review-get-commit-comments (pr-alist callback)
+   "Get the top level comments on a PR.
+ PR-ALIST is an alist representing a PR
+ CALLBACK will be called back when done"
+   (ghub-get (github-review-format-pr-url 'get-commit-comments pr-alist)
+             nil
+             :auth 'github-review
+             :host (github-review-api-host pr-alist)
+             :errorback (lambda (&rest _) (message "Error talking to GitHub"))
+             :callback callback))
+ 
+ (defun github-review-get-commit-comments-deferred (pr-alist)
+   "Get the top level comments on a PR.
+ PR-ALIST is an alist representing a PR
+ CALLBACK will be called back when done
+ return a deferred object"
+   (let ((d (deferred:new #'identity)))
+     (github-review-get-commit-comments pr-alist (apply-partially (lambda (d v &rest _)  (deferred:callback-post d v)) d)) d))

[charignon]

What do you mean by does not pick up? When you call github-review-start and give a link to a commit like https://github.com/charignon/github-review/commit/0581a6079ebc1404c873aef133ee66d8bb7217c8 does it not work? What do your commits link look like? I suspect that GitHub has multiple format to refer to commits in the context of a PR vs out of context.

[andrelkin]

What do you mean by does not pick up? When you call github-review-start and give a link to a commit like https://github.com/charignon/github-review/commit/0581a6079ebc1404c873aef133ee66d8bb7217c8 does it not work?

Yes. E.g I commented today on a commit https://github.com/mariadb/server/commit/4432e80a722fd70aa704a65d10237e8450387943 with https://github.com/mariadb/server/commit/4432e80a722fd70aa704a65d10237e8450387943#r40237028

And that comment follows the patch itself. It does not show up via M-x github-review-start. Neither do comments within the patch.

What do your commits link look like? I suspect that GitHub has multiple format to refer to commits in the context of a PR vs out of context.

What I receive is the description and the diff.

I pointed to (when github-review-fetch-top-level-and-review-comments)... block that does exist only in the PR review branch, must be it to blame, no?

[andrelkin]

Here is a patch that I sketched to fix the comments delivery, briefly tested. [peek_comments.diff.txt](https://github.com/charignon/github-review/files/4847763/peek_comments.diff.txt)

[charignon]

Your patch looks good, I added it and I will rewrite the author's name to match your name and email. I also added support for urls referring to commit in PRs. @andrelkin feel free to test it again. I am planning to refactor, test and merge this in the coming week.

[charignon]

@andrelkin could you please take another look?

Thanks

[andrelkin]

Salute, @charignon ! I somewhat tested patches of Jul 4th. Now turning to test lc--mvp-review-commit branch. Thanks for the invite :-)!

[andrelkin]

Does not seem to work correctly in the plain commit case now. E.g when https://github.com/MariaDB/server/commit/9d3742212e1d54a6ceaae87a45ec3428a59d1bd7 is requested the retrieved diff is not from there.

I am investigating what is wrong later today.

[andrelkin]

My testing env was not clean. This branch actually works for me. Thanks!

[andrelkin]

Actually, I can't make my comments, formatted as specified and sent via github-review-comment, to land to the commit page. Investigating.. The page in question is https://github.com/MariaDB/server/commit/2f859962b032cc75176e05df8d704eec413cdb17 My comment block (one line) is as the following one:

~ MDEV-23534: SIGSEGV in sf_malloc_usable_size/my_free on SET GLOBAL REPLICATE_DO_TABLE
~ 
~ Backporting fixes for:
~ 
~ MDEV-22317: SIGSEGV in my_free/delete_dynamic in optimized builds (ARIA)
~ MDEV-22059: MSAN report at replicate_ignore_table_grant
# Could you please refer to the orginal patches with git commit id:s.
diff --git a/mysql-test/suite/rpl/r/rpl_filter_tables_dynamic.result b/mysql-test/suite/rpl/r/rpl_filter_tables_dynamic.result
index 5a746c88458c..9709e24fbdec 100644
--- a/mysql-test/suite/rpl/r/rpl_filter_tables_dynamic.result
+++ b/mysql-test/suite/rpl/r/rpl_filter_tables_dynamic.result
@@ -5,6 +5,8 @@ ERROR HY000: This operation cannot be performed as you have a running slave '';
 SET @@GLOBAL.replicate_ignore_table="test.t4,test.t5,test.t6";
 ERROR HY000: This operation cannot be performed as you have a running slave ''; run STOP SLAVE '' first
 include/stop_slave.inc
+SET @@GLOBAL.replicate_do_table="";
+SET @@GLOBAL.replicate_ignore_table="";
 SET @@GLOBAL.replicate_do_table="test.t1,test.t2,test.t3";
 SET @@GLOBAL.replicate_ignore_table="test.t4,test.t5,test.t6";
 include/start_slave.inc

# Could you please refer to the orginal patches with git commit id:s. was also tried without the space after '#', still with the same outcome.

[andrelkin]

Actually, I can't make my comments, formatted as specified and sent via github-review-comment, to land to the commit page. Investigating..

I think I recognized the issue in github-review-submit-review-commit. (comments (github-review-a-get parsed-review 'comments)) evaluates comments to empty list even if parsed-review (top-level comments) is not nil. The function finally attempts to call in a per-(empty!)-comments-item loop github-review-post-review-commit, so does nothing.

[andrelkin]

So the problem of posting comments on commit is caused by generated by github-review-parse-review-lines empty alist keyed by comments. A hack like the following one

@@ -518,7 +519,9 @@ This function infers the PR name based on the current filename"
          (parsed-review (github-review-parsed-review-from-current-buffer)))
     (message "Submitting review, this may take a while ...")
     (let* ((sha (github-review-a-get commit-alist 'sha))
-           (comments (github-review-a-get parsed-review 'comments)))
+           (comments               ;; a bug in github-review-parse-review-lines which does not create the comments-keyed alist
+	                           ;;(github-review-a-get parsed-review 'comments))) 
+	    (list parsed-review))) ;; weird hack!!!
       (cl-loop for x in comments collect (github-review-post-review-commit
                                           commit-alist
                                           x (lambda (&rest _)

makes the posting of a comment in the header of commit ( not inside the diff) to succeed. I'll check closer github-review-parse-review-lines later and try to fix it.

[andrelkin]

The posting issue is resolved after I fully understood it. The following hunk must be the final (or close to that) solution sustaining manual tests:

@@ -518,11 +519,16 @@ This function infers the PR name based on the current filename"
          (parsed-review (github-review-parsed-review-from-current-buffer)))
     (message "Submitting review, this may take a while ...")
     (let* ((sha (github-review-a-get commit-alist 'sha))
+	   (parsed-body (github-review-a-get parsed-review 'body))
            (comments (github-review-a-get parsed-review 'comments)))
+      (when parsed-body
+	(github-review-post-review-commit commit-alist `((body . ,parsed-body))
+					   (lambda (&rest _)
+                                              (message "Done submitting commit top-level comments"))))
       (cl-loop for x in comments collect (github-review-post-review-commit
                                           commit-alist
                                           x (lambda (&rest _)
-                                              (message "Done submitting review")))))))
+                                              (message "Done submitting commit diff comments")))))))

[andrelkin]

Even though a reported above issue fixed I am still working on reviewing, testing and some improving. E.g I noticed it's enough to have just one github-review-save-diff which is able to guess with is the being saved document actual type is; in my emacs configuration diff-files are hooked to be read-only, so I refined the function once more; it must be very helpful for a lot of various processing for the diff buffer to know its diff's commit-id (sha) as well as the parent's commit-id. I am also arranging a hook to exploit (for my need), exemplify usage).

I think the best if I'll proceed with a PR on my own that bases on this lc-mvp... branch's tip (btw, does MVP stand for Model-View-Present pattern?).

[andrelkin]

Could you send out a PR with that and we can discuss there? Thanks!

I am cleaning it up. Need few days to complete that (considering the size of my primary load :-)).

[andrelkin]

Could you send out a PR with that and we can discuss there? Thanks!

I am cleaning it up. Need few days to complete that (considering the size of my primary load :-)).

My forked branch is mostly done. I just need to rebase its commits into 2 or 3 for easier reviewing. Must be finishing this weekend.

[andrelkin]

Pr is done - https://github.com/charignon/github-review/pull/63. Thanks for your patience! I am eager to hear from you, dear @charignon ,or anybody to improve if anything.