Update comments, clean up and fix code

7 files changed, 90 insertions, 215 deletions

diff --git a/mod/mod-ci-github-gh.cxx b/mod/mod-ci-github-gh.cxx
index dcea563..208adbd 100644
--- a/mod/mod-ci-github-gh.cxx
+++ b/mod/mod-ci-github-gh.cxx
@@ -7,8 +7,7 @@
 
 namespace brep
 {
-  // Return the GitHub check run status corresponding to a build_state. Throw
-  // invalid_argument if the build_state value was invalid.
+  // Return the GitHub check run status corresponding to a build_state.
   //
   string
   gh_to_status (build_state st)
@@ -187,8 +186,7 @@ namespace brep
   {
     p.next_expect (event::begin_object);
 
-    bool ni (false), nu (false), st (false), ma (false), ms (false),
-      bs (false), hd (false);
+    bool ni (false), nu (false), bs (false), hd (false);
 
     // Skip unknown/uninteresting members.
     //
@@ -201,14 +199,6 @@ namespace brep
 
       if      (c (ni, "node_id"))   node_id = p.next_expect_string ();
       else if (c (nu, "number"))    number = p.next_expect_number<unsigned int> ();
-      else if (c (st, "state"))     state = p.next_expect_string ();
-      else if (c (ma, "mergeable")) mergeable = p.next_expect_boolean_null<bool> ();
-      else if (c (ms, "merge_commit_sha"))
-      {
-        string* v (p.next_expect_string_null ());
-        if (v != nullptr)
-          merge_commit_sha = *v;
-      }
       else if (c (bs, "base"))
       {
         p.next_expect (event::begin_object);
@@ -274,9 +264,6 @@ namespace brep
 
     if (!ni) missing_member (p, "gh_pull_request", "node_id");
     if (!nu) missing_member (p, "gh_pull_request", "number");
-    if (!st) missing_member (p, "gh_pull_request", "state");
-    if (!ma) missing_member (p, "gh_pull_request", "mergeable");
-    if (!ms) missing_member (p, "gh_pull_request", "merge_commit_sha");
     if (!bs) missing_member (p, "gh_pull_request", "base");
     if (!hd) missing_member (p, "gh_pull_request", "head");
   }
@@ -286,13 +273,6 @@ namespace brep
   {
     os << "node_id: " << pr.node_id
        << ", number: " << pr.number
-       << ", state: " << pr.state
-       << ", mergeable: " << (pr.mergeable
-                              ? *pr.mergeable
-                                ? "true"
-                                : "false"
-                              : "null")
-       << ", merge_commit_sha:" << pr.merge_commit_sha
        << ", base: { "
        << "path: " << pr.base_path
        << ", ref: " << pr.base_ref
@@ -314,7 +294,7 @@ namespace brep
   {
     p.next_expect (event::begin_object);
 
-    bool ni (false), nm (false), fn (false), db (false), cu (false);
+    bool ni (false), nm (false), fn (false), cu (false);
 
     // Skip unknown/uninteresting members.
     //
@@ -328,7 +308,6 @@ namespace brep
       if      (c (ni, "node_id"))        node_id = p.next_expect_string ();
       else if (c (nm, "name"))           name = p.next_expect_string ();
       else if (c (fn, "full_name"))      path = p.next_expect_string ();
-      else if (c (db, "default_branch")) default_branch = p.next_expect_string ();
       else if (c (cu, "clone_url"))      clone_url = p.next_expect_string ();
       else p.next_expect_value_skip ();
     }
@@ -336,7 +315,6 @@ namespace brep
     if (!ni) missing_member (p, "gh_repository", "node_id");
     if (!nm) missing_member (p, "gh_repository", "name");
     if (!fn) missing_member (p, "gh_repository", "full_name");
-    if (!db) missing_member (p, "gh_repository", "default_branch");
     if (!cu) missing_member (p, "gh_repository", "clone_url");
   }
 
@@ -346,7 +324,6 @@ namespace brep
     os << "node_id: " << rep.node_id
        << ", name: " << rep.name
        << ", path: " << rep.path
-       << ", default_branch: " << rep.default_branch
        << ", clone_url: " << rep.clone_url;
 
     return os;
diff --git a/mod/mod-ci-github-gh.hxx b/mod/mod-ci-github-gh.hxx
index 281d765..f3bcfeb 100644
--- a/mod/mod-ci-github-gh.hxx
+++ b/mod/mod-ci-github-gh.hxx
@@ -73,17 +73,8 @@ namespace brep
     string node_id;
     unsigned int number;
 
-    string state; // "open" or "closed".
-
-    // If absent then the result of the test merge commit is not yet
-    // available. If true then `merge_commit_sha` contains the commit ID of
-    // the merge commit. If false then `merge_commit_sha` is either empty or
-    // no longer valid.
-    //
-    //   @@ TODO These appear to be unused.
-    //
-    optional<bool> mergeable;
-    string merge_commit_sha;
+    // @@ TMP The unused base/head members may be useful for trace output when
+    //    we receive the pull_request webhook.
 
     string base_path; // Repository path (<org>/<repo>) under github.com.
     string base_ref;  // @@ TODO Remove if remains unused.
@@ -127,7 +118,6 @@ namespace brep
     string node_id;
     string name;
     string path; // Repository path (<org>/<repo>) under github.com.
-    string default_branch;
     string clone_url;
 
     explicit
diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx
index 17096eb..1909e1f 100644
--- a/mod/mod-ci-github-gq.cxx
+++ b/mod/mod-ci-github-gq.cxx
@@ -241,7 +241,8 @@ namespace brep
       {
         vector<gh_check_run> check_runs; // Received check runs.
 
-        resp (json::parser& p): check_runs (gq_parse_response_check_runs (p)) {}
+        resp (json::parser& p)
+            : check_runs (gq_parse_response_check_runs (p)) {}
 
         resp () = default;
       } rs;
@@ -271,23 +272,23 @@ namespace brep
             // @@ Are we handling the case where the resulting state (built)
             //    differs from what we expect?
             //
-            if (rst != build_state::built && rst != st)
+            // @@@ Does built-to-built transition updates status?
+            //
+            if (rst != st && rst != build_state::built)
             {
               error << "unexpected check_run status: received '" << rcr.status
                     << "' but expected '" << gh_to_status (st) << '\'';
 
               return false; // Fail because something is clearly very wrong.
             }
-            else
-            {
-              check_run& cr (crs[i]);
 
-              if (!cr.node_id)
-                cr.node_id = move (rcr.node_id);
+            check_run& cr (crs[i]);
 
-              cr.state = rst;
-              cr.state_synced = true;
-            }
+            if (!cr.node_id)
+              cr.node_id = move (rcr.node_id);
+
+            cr.state = rst;
+            cr.state_synced = (rst == st);
           }
 
           return true;
diff --git a/mod/mod-ci-github-gq.hxx b/mod/mod-ci-github-gq.hxx
index 72283ee..9022fe3 100644
--- a/mod/mod-ci-github-gq.hxx
+++ b/mod/mod-ci-github-gq.hxx
@@ -20,12 +20,17 @@ namespace brep
   //
 
   // Create a new check run on GitHub for each build. Update `check_runs` with
-  // the new states and node IDs. Return false and issue diagnostics if the
-  // request failed.
+  // the new data (node id, state, and state_synced). Return false and issue
+  // diagnostics if the request failed.
   //
   // Note: no details_url yet since there will be no entry in the build result
   // search page until the task starts building.
   //
+  // Note that creating a check_run named `foo` will effectively replace any
+  // existing check_runs with that name. They will still exist on the GitHub
+  // servers but GitHub will only consider the latest one (for display in the
+  // UI or in determining the mergeability of a PR).
+  //
   bool
   gq_create_check_runs (const basic_mark& error,
                         vector<check_run>& check_runs,
@@ -35,8 +40,8 @@ namespace brep
                         build_state);
 
   // Create a new check run on GitHub for a build. Update `cr` with the new
-  // state and the node ID. Return false and issue diagnostics if the request
-  // failed.
+  // data (node id, state, and state_synced). Return false and issue
+  // diagnostics if the request failed.
   //
   // If the details_url is absent GitHub will use the app's homepage.
   //
@@ -64,8 +69,12 @@ namespace brep
   // Update a check run on GitHub.
   //
   // Send a GraphQL request that updates an existing check run. Update `cr`
-  // with the new state. Return false and issue diagnostics if the request
-  // failed.
+  // with the new data (state and state_synced). Return false and issue
+  // diagnostics if the request failed.
+  //
+  // Note that GitHub allows any state transitions except from built (but
+  // built to built is allowed). The latter case is signalled by setting the
+  // check_run state_synced member to false and the state member to built.
   //
   // If the details_url is absent GitHub will use the app's homepage.
   //
@@ -86,7 +95,7 @@ namespace brep
   // Fetch pre-check information for a pull request from GitHub. This
   // information is used to decide whether or not to CI the PR and is
   // comprised of the PR's head commit SHA, whether its head branch is behind
-  // its base branch, and its test merge commit SHA.
+  // its base branch, and its mergeability and test merge commit SHA.
   //
   // Return absent value if the merge commit is still being generated (which
   // means PR head branch behindness is not yet known either). See the
@@ -97,8 +106,12 @@ namespace brep
   // will be treated by the caller as still being generated).
   //
   // Note that the first request causes GitHub to start preparing the test
-  // merge commit. (For details see
-  // https://docs.github.com/rest/guides/getting-started-with-the-git-database-api#checking-mergeability-of-pull-requests.)
+  // merge commit.
+  //
+  // For details regarding the test merge commit and how to check/poll for PR
+  // mergeability see
+  // https://docs.github.com/en/rest/pulls/pulls?#get-a-pull-request and
+  // https://docs.github.com/en/rest/guides/using-the-rest-api-to-interact-with-your-git-database?#checking-mergeability-of-pull-requests
   //
   struct gq_pr_pre_check_info
   {
diff --git a/mod/mod-ci-github-service-data.cxx b/mod/mod-ci-github-service-data.cxx
index 2a538a5..be60205 100644
--- a/mod/mod-ci-github-service-data.cxx
+++ b/mod/mod-ci-github-service-data.cxx
@@ -116,8 +116,8 @@ namespace brep
                 string rid,
                 string rcu,
                 kind_type k,
-                bool rr,
                 bool pc,
+                bool rr,
                 string cs,
                 string rs)
       : kind (k), pre_check (pc), re_request (rr),
@@ -141,8 +141,8 @@ namespace brep
                 string rid,
                 string rcu,
                 kind_type k,
-                bool rr,
                 bool pc,
+                bool rr,
                 string cs,
                 string rs,
                 string pid,
diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx
index eb81ae4..0dd52ca 100644
--- a/mod/mod-ci-github-service-data.hxx
+++ b/mod/mod-ci-github-service-data.hxx
@@ -33,7 +33,7 @@ namespace brep
     build_state             state;
     bool                    state_synced;
 
-    optional<result_status> status; // Only present if state is built.
+    optional<result_status> status; // Only if state is built & synced.
 
     string
     state_string () const
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index 60a6bae..b2e0c41 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -585,120 +585,25 @@ namespace brep
     return true;
   }
 
-  // High-level description of pull request (PR) handling
+  // Miscellaneous pull request facts
   //
-  // @@ TODO Update these comments.
+  // - Although some of the GitHub documentation makes it sound like they
+  //   expect check runs to be added to both the PR head commit and the merge
+  //   commit, the PR UI does not react to the merge commit's check runs
+  //   consistently. It actually seems to be quite broken. The only thing it
+  //   does seem to do reliably is blocking the PR merge if the merge commit's
+  //   check runs are not successful (i.e, overriding the PR head commit's
+  //   check runs). But the UI looks quite messed up generally in this state.
   //
-  // - Some GitHub pull request terminology:
+  // - When new commits are added to a PR base branch, pull_request.base.sha
+  //   does not change, but the test merge commit will be updated to include
+  //   the new commits to the base branch.
   //
-  //   - Fork and pull model: Pull requests are created in a forked
-  //     repository. Thus the head and base repositories are different.
+  // - When new commits are added to a PR head branch, pull_request.head.sha
+  //   gets updated with the head commit's SHA and check_suite.pull_requests[]
+  //   will contain all PRs with this branch as head.
   //
-  //   - Shared repository model: The pull request head and base branches are
-  //     in the same repository. For example, from a feature branch onto
-  //     master.
-  //
-  // - CI the merge commit but add check runs to the pull request head commit
-  //
-  //   Most of the major CI integrations build the merge commit instead of the
-  //   PR head commit.
-  //
-  //   Adding the check runs to the PR head commit is recommended by the
-  //   following blog posts by a GitHub employee who is one of the best
-  //   sources on these topics:
-  //   https://www.kenmuse.com/blog/shared-commits-and-github-checks/ and
-  //   https://www.kenmuse.com/blog/creating-github-checks/.
-  //
-  //   Do not add any check runs to the merge commit because:
-  //
-  //   - The PR head commit is the only commit that can be relied upon to
-  //     exist throughout the PR's lifetime. The merge commit, on the other
-  //     hand, can change during the PR process. When that happens the PR will
-  //     look for check runs on the new merge commit, effectively discarding
-  //     the ones we had before.
-  //
-  //   - Although some of the GitHub documentation makes it sound like they
-  //     expect check runs to be added to both the PR head commit and the
-  //     merge commit, the PR UI does not react to the merge commit's check
-  //     runs consistently. It actually seems to be quite broken.
-  //
-  //     The only thing it seems to do reliably is blocking the PR merge if
-  //     the merge commit's check runs are not successful (i.e, overriding the
-  //     PR head commit's check runs). But the UI looks quite messed up
-  //     generally in this state.
-  //
-  //   Note that, in the case of a PR from a forked repository (the so-called
-  //   "fork and pull" model), GitHub will copy the PR head commit from the
-  //   head repository (the forked one) into the base repository. So the check
-  //   runs must always be added to the base repository, whether the PR is
-  //   from within the same repository or from a forked repository. The merge
-  //   and head commits will be at refs/pull/<PR-number>/{merge,head}.
-  //
-  // - @@ TODO Shared repo model problems
-  //
-  //      The root of all of these problems is that, in the shared repository
-  //      model, for every PR we also receive a check_suite for the commit to
-  //      the head branch. The situation is exacerbated by the fact that the
-  //      PR and CS can arrive in any order.
-  //
-  //       - There will be two CIs running concurrently, building different
-  //         commits: the head commit (check_suite) vs merge commit
-  //         (pull_request).
-  //
-  //       - The CS and PR check_runs will all be added to the same commit
-  //         SHA: pull_request.head.sha, check_suite.head_sha (see above for
-  //         the reasons we don't put the check_runs on the merge
-  //         commit).
-  //
-  //       - Recall that creating a check_run named `A` will effectively
-  //         replace any existing check_runs with that name. They will still
-  //         exist on the GitHub servers but GitHub will only consider the
-  //         latest one (for display in the UI or in determining the
-  //         mergeability of a PR).
-  //
-  //       - Some CS CRs may finish after the corresponding PR CRs, thus
-  //         potentially inverting the true status of a PR (e.g., allow the
-  //         merge of a PR with a bad merge commit).
-  //
-  //   Thus we need a way to prevent any CS check_runs from being updated
-  //   after having received a PR.
-  //
-  //   Problem 1: Create PR from feature branch
-  //
-  //     - Receive check_suite for commit to feature branch
-  //     - Receive pull_request
-  //
-  //     Solution: When receive a PR, fetch all check_suites with that head
-  //     SHA (curl REPO/commits/SHA/check-suites) and cancel their CI
-  //     jobs.
-  //
-  //     Thus there will be no more CS check_run updates. Note however that in
-  //     most cases the PR will be received long enough after the CS for the
-  //     latter's check_runs to all have completed already.
-  //
-  //     Note that there will not be a merge CR on the head yet so the PR will
-  //     never be green.
-  //
-  //   Problem 2: New commits are added to PR head branch
-  //
-  //     Note: The check_suite and pull_request can arrive in any order.
-  //
-  //     - check_suite(requested, PR_head)
-  //
-  //       Note: check_suite.pull_requests[] will contain all PRs with this
-  //       branch as head.
-  //
-  //       Note: check_suite.pull_requests[i].head.sha will be the new,
-  //       updated PR head sha.
-  //
-  //     - pull_request(synchronize)
-  //
-  //     Solution: Ignore all check_suites with non-empty pull_requests[].
-  //
-  // - New commits are added to PR base branch
-  //
-  //   Note: In this case pull_request.base.sha does not change, but the merge
-  //   commit will be updated to include the new commits to the base branch.
+  // Remaining TODOs
   //
   // - @@ TODO? PR base branch changed (to a different branch)
   //
@@ -769,6 +674,12 @@ namespace brep
 
     // Note that PR rebuilds (re-requested) are handled by check_suite().
     //
+    // Note that, in the case of a remote PR, GitHub will copy the PR head
+    // commit from the head (forked) repository into the base repository. So
+    // the check runs must always be added to the base repository, whether the
+    // PR is local or remote. The head commit refs are located at
+    // refs/pull/<PR-number>/head.
+    //
     service_data sd (warning_success,
                      move (iat->token),
                      iat->expires_at,
@@ -898,9 +809,12 @@ namespace brep
     }
     else
     {
-      // Create the CI tenant.
+      // Create the CI tenant by reusing the pre-check service data.
+      //
+      sd.pre_check = false;
 
-      // Update service data's check_sha if this is a remote PR.
+      // Set the service data's check_sha if this is a remote PR. The test
+      // merge commit refs are located at refs/pull/<PR-number>/merge.
       //
       if (sd.kind == service_data::remote)
         sd.check_sha = *pc->merge_commit_sha;
@@ -1098,7 +1012,7 @@ namespace brep
     {
       string ru; // Repository URL.
 
-      // CI the merge test commit for remote PRs and the head commit for
+      // CI the test merge commit for remote PRs and the head commit for
       // everything else (branch push or local PRs).
       //
       if (sd.kind == service_data::remote)
@@ -1822,34 +1736,8 @@ namespace brep
 
       if (cr.state_synced)
       {
-        // Check run was created/updated successfully to built.
-        //
-        // @@ TMP Feels like this should also be done inside
-        //    gq_{create,update}_check_run() -- where cr.state is set if the
-        //    create/update succeeds -- but I think we didn't want to pass a
-        //    result_status into a gq_ function because converting to a GitHub
-        //    conclusion/title/summary is reasonably complicated.
-        //
-        //    @@@ We need to redo that code:
-        //
-        //    - Pass the vector of check runs with new state (and status) set.
-        //    - Update synchronized flag inside those functions.
-        //    - Update the state to built if it's already built on GitHub --
-        //      but then what do we set the status to?
-        //
-        //      @@ TMP This scenario can only arise for updates to building.
-        //         For creations a new CR will always be created so the
-        //         returned state will always be what we asked for; and we
-        //         never update to queued.
-        //
-        //         As for updates to building, if GH has already been updated
-        //         to built then the build_built() lambda will soon save the
-        //         built state and valid status so nothing more should need to
-        //         be done. In fact, whenever updating to building we do stop
-        //         immediately if it's already built on GH.
-        //
-        //    - Maybe signal in return value (optional<bool>?) that there is
-        //      a discrepancy.
+        // Check run was created/updated successfully to built (with
+        // status we specified).
         //
         cr.status = b.status;
 
@@ -1919,24 +1807,30 @@ namespace brep
       if (iat)
         sd.installation_access = *iat;
 
-      if (check_run* scr = sd.find_check_run (cr.build_id))
+      // Only update the check_run state in service data if it matches the
+      // state (specifically, status) on GitHub.
+      //
+      if (cr.state_synced)
       {
-        // This will most commonly generate a duplicate warning (see above).
-        // We could save the old state and only warn if it differs but let's
-        // not complicate things for now.
-        //
-#if 0
-        if (scr->state != build_state::building)
+        if (check_run* scr = sd.find_check_run (cr.build_id))
         {
-          warn << "check run " << cr.build_id << ": out of order built "
-               << "notification service data update; existing state: "
-               << scr->state_string ();
-        }
+          // This will most commonly generate a duplicate warning (see above).
+          // We could save the old state and only warn if it differs but let's
+          // not complicate things for now.
+          //
+#if 0
+          if (scr->state != build_state::building)
+          {
+            warn << "check run " << cr.build_id << ": out of order built "
+                 << "notification service data update; existing state: "
+                 << scr->state_string ();
+          }
 #endif
-        *scr = cr;
+          *scr = cr;
+        }
+        else
+          sd.check_runs.push_back (cr);
       }
-      else
-        sd.check_runs.push_back (cr);
 
       return sd.json ();
     };