Allow reviewing individual commits #43

charignon · 2020-06-27T07:38:04Z

Same entry point as for PR.

charignon · 2020-06-27T07:43:48Z

Tested end to end on 0581a60

andrelkin · 2020-06-29T14:11:21Z

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 · 2020-06-29T15:28:56Z

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 · 2020-06-29T18:43:14Z

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
MariaDB/server@4432e80
with
MariaDB/server@4432e80#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 · 2020-06-29T20:06:31Z

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 · 2020-07-04T18:27:10Z

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 · 2020-08-18T15:16:32Z

@andrelkin could you please take another look?

Thanks

Same entry point as for PR.

andrelkin · 2020-08-24T13:28:31Z

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

andrelkin · 2020-08-24T14:32:02Z

Does not seem to work correctly in the plain commit case now.
E.g when MariaDB/server@9d37422 is requested
the retrieved diff is not from there.

I am investigating what is wrong later today.

andrelkin · 2020-08-28T13:00:38Z

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

andrelkin · 2020-08-28T16:48:39Z

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
MariaDB/server@2f85996
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 · 2020-09-01T19:17:06Z

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 · 2020-09-03T19:28:21Z

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 · 2020-09-05T13:34:34Z

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 · 2020-09-28T13:34:10Z

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 · 2020-09-28T18:38:36Z

github-review.el

+(defun github-review-get-commit-diff (commit-alist callback)
+  "Get the diff for a commit, given COMMIT-ALIST an alist recommitesenting a COMMIT.
+CALLBACK is called with the result"
+  (github-review-get-commit commit-alist t callback))


I think there should be just one getter-function github-review-get-object that
accepts object-id-alist from which it and its callees find out what
kind of PR or COMMIT the object is.
E.g github-review-get-commit is different from github-review-get-pr only that
it passes in alist sha-keyed value rather than num-keyed one to
github-review-format-commit-url.

So which one is passed needs checking and then to conduct a respecive specific branch of logics.

As another example here is a generalized function to save retrieved object:

+(defun github-review-save-diff (id-alist diff) + "Save a DIFF (string) to a temp file named after pr or commit sha specified by ID-ALIST." + (let* ((is-pr (> (string-to-number (github-review-a-get id-alist 'num)) 0))) + (find-file (format (if is-pr "%s/%s___%s___%s.diff" "%s/%s___%s___commit___%s.diff") + github-review-review-folder + (github-review-a-get id-alist 'owner) + (github-review-a-get id-alist 'repo) + (github-review-a-get id-alist (if is-pr 'num 'sha)))) + (buffer-read-only nil) + (erase-buffer) + (insert diff) + (save-buffer) + (when github-do-review-mode + (github-review-mode))))

If you won't have any objections I'm going to generalize all such "paired" functions.

charignon

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

andrelkin · 2020-10-05T16:41:42Z

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 · 2020-10-09T19:38:27Z

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 · 2020-10-12T10:14:56Z

Pr is done - #63. Thanks for your patience! I am eager to hear from you, dear @charignon ,or anybody to improve if anything.

charignon mentioned this pull request Jun 27, 2020

Accept non-PR "plain" commit for commenting on #42

Open

charignon force-pushed the lc--mvp-review-commit branch from 2f80101 to 20fdea2 Compare June 27, 2020 07:43

charignon changed the title ~~MVP of reviewing plain commits~~ Accept non-PR "plain" commit for commenting on Jun 27, 2020

charignon force-pushed the lc--mvp-review-commit branch from 7fc0a3e to 7dc28e8 Compare July 4, 2020 18:25

charignon changed the title ~~Accept non-PR "plain" commit for commenting on~~ Allow reviewing individual commits Jul 4, 2020

MVP of reviewing plain commits

509a21f

Same entry point as for PR.

charignon force-pushed the lc--mvp-review-commit branch from 3c0b1cc to 509a21f Compare August 21, 2020 21:43

Merge branch 'master' into lc--mvp-review-commit

04a956d

andrelkin reviewed Sep 28, 2020

View reviewed changes

charignon commented Oct 4, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow reviewing individual commits #43

Allow reviewing individual commits #43

charignon commented Jun 27, 2020

charignon commented Jun 27, 2020

andrelkin commented Jun 29, 2020

charignon commented Jun 29, 2020 •

edited

andrelkin commented Jun 29, 2020 •

edited

andrelkin commented Jun 29, 2020

charignon commented Jul 4, 2020

charignon commented Aug 18, 2020

andrelkin commented Aug 24, 2020

andrelkin commented Aug 24, 2020

andrelkin commented Aug 28, 2020 •

edited

andrelkin commented Aug 28, 2020 •

edited

andrelkin commented Sep 1, 2020

andrelkin commented Sep 3, 2020

andrelkin commented Sep 5, 2020 •

edited

andrelkin commented Sep 28, 2020 •

edited

andrelkin Sep 28, 2020 •

edited

charignon left a comment

andrelkin commented Oct 5, 2020

andrelkin commented Oct 9, 2020

andrelkin commented Oct 12, 2020

Allow reviewing individual commits #43

Are you sure you want to change the base?

Allow reviewing individual commits #43

Conversation

charignon commented Jun 27, 2020

charignon commented Jun 27, 2020

andrelkin commented Jun 29, 2020

charignon commented Jun 29, 2020 • edited

andrelkin commented Jun 29, 2020 • edited

andrelkin commented Jun 29, 2020

charignon commented Jul 4, 2020

charignon commented Aug 18, 2020

andrelkin commented Aug 24, 2020

andrelkin commented Aug 24, 2020

andrelkin commented Aug 28, 2020 • edited

andrelkin commented Aug 28, 2020 • edited

andrelkin commented Sep 1, 2020

andrelkin commented Sep 3, 2020

andrelkin commented Sep 5, 2020 • edited

andrelkin commented Sep 28, 2020 • edited

andrelkin Sep 28, 2020 • edited

Choose a reason for hiding this comment

charignon left a comment

Choose a reason for hiding this comment

andrelkin commented Oct 5, 2020

andrelkin commented Oct 9, 2020

andrelkin commented Oct 12, 2020

charignon commented Jun 29, 2020 •

edited

andrelkin commented Jun 29, 2020 •

edited

andrelkin commented Aug 28, 2020 •

edited

andrelkin commented Aug 28, 2020 •

edited

andrelkin commented Sep 5, 2020 •

edited

andrelkin commented Sep 28, 2020 •

edited

andrelkin Sep 28, 2020 •

edited