git-repo/99b5a17f2c951fe5979100c36e7e1dbb4c61b36c

{
  "comments": [
    {
      "unresolved": false,
      "key": {
        "uuid": "8f2a3c48_e8793eff",
        "filename": "/PATCHSET_LEVEL",
        "patchSetId": 12
      },
      "lineNbr": 0,
      "author": {
        "id": 4184636
      },
      "writtenOn": "2025-06-26T03:49:11Z",
      "side": 1,
      "message": "You are a highly experienced code reviewer specializing in Git patches. Your\ntask is to analyze the provided Git patch (`patch`) and provide comprehensive\nfeedback.  Focus on identifying potential bugs, inconsistencies, security\nvulnerabilities, and areas for improvement in code style and readability.\nYour response should be detailed and constructive, offering specific suggestions\nfor remediation where applicable. Prioritize clarity and conciseness in your\nfeedback.\n\n# Step by Step Instructions\n\n1.  Read the provided `patch` carefully.  Understand the changes it introduces to the codebase.\n\n2.  Analyze the `patch` for potential issues:\n    * **Functionality:** Does the code work as intended? Are there any bugs or unexpected behavior?\n    * **Security:** Are there any security vulnerabilities introduced by the patch?\n    * **Style:** Does the code adhere to the project\u0027s coding style guidelines? Is it readable and maintainable?\n    * **Consistency:** Are there any inconsistencies with existing code or design patterns?\n    * **Testing:** Does the patch include sufficient tests to cover the changes?\n\n3.  Formulate concise and constructive feedback for each identified issue.  Provide specific suggestions for remediation where possible.\n\n4.  Summarize your findings in a clear and organized manner.  Prioritize critical issues over minor ones.\n\n5.  Review the feedback written so far. Is the feedback comprehensive and sufficiently detailed?\nIf not, go back to step 2, focusing on any areas that require further analysis or clarification.\n If yes, proceed to step 6.\n\n6.  Output the complete review.\n\n\nPatch: \n\"\"\"\nFrom 99b5a17f2c951fe5979100c36e7e1dbb4c61b36c Mon Sep 17 00:00:00 2001\nFrom: Gavin Mak \u003cgavinmak@google.com\u003e\nDate: Tue, 17 Jun 2025 20:15:50 -0700\nSubject: [PATCH] sync: Share final error handling logic between sync modes\n\nDedupe error reporting logic for phased and interleaved sync modes by\nextracting it into _ReportErrors.\n\nError reporting will now distinguish between network and local failures\nand lists the specific repos that failed in each phase.\n\nBug: 421935613\nChange-Id: I4604a83943dbbd71d979158d7a1c4b8c243347d2\nReviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/484541\nTested-by: Gavin Mak \u003cgavinmak@google.com\u003e\nReviewed-by: Scott Lee \u003cddoman@google.com\u003e\nCommit-Queue: Gavin Mak \u003cgavinmak@google.com\u003e\n---\n\ndiff --git a/subcmds/sync.py b/subcmds/sync.py\nindex 3d4ab75..20d75dc 100644\n--- a/subcmds/sync.py\n+++ b/subcmds/sync.py\n@@ -2054,6 +2054,46 @@\n                     raise SyncFailFastError(aggregate_errors\u003derrors)\n         return err_update_projects, err_update_linkfiles\n \n+    def _ReportErrors(\n+        self,\n+        errors,\n+        err_network_sync\u003dFalse,\n+        failing_network_repos\u003dNone,\n+        err_checkout\u003dFalse,\n+        failing_checkout_repos\u003dNone,\n+        err_update_projects\u003dFalse,\n+        err_update_linkfiles\u003dFalse,\n+    ):\n+        \"\"\"Logs detailed error messages and raises a SyncError.\"\"\"\n+\n+        def print_and_log(err_msg):\n+            self.git_event_log.ErrorEvent(err_msg)\n+            logger.error(\"%s\", err_msg)\n+\n+        print_and_log(\"error: Unable to fully sync the tree\")\n+        if err_network_sync:\n+            print_and_log(\"error: Downloading network changes failed.\")\n+            if failing_network_repos:\n+                logger.error(\n+                    \"Failing repos (network):\\n%s\",\n+                    \"\\n\".join(sorted(failing_network_repos)),\n+                )\n+        if err_update_projects:\n+            print_and_log(\"error: Updating local project lists failed.\")\n+        if err_update_linkfiles:\n+            print_and_log(\"error: Updating copyfiles or linkfiles failed.\")\n+        if err_checkout:\n+            print_and_log(\"error: Checking out local projects failed.\")\n+            if failing_checkout_repos:\n+                logger.error(\n+                    \"Failing repos (checkout):\\n%s\",\n+                    \"\\n\".join(sorted(failing_checkout_repos)),\n+                )\n+        logger.error(\n+            \u0027Try re-running with \"-j1 --fail-fast\" to exit at the first error.\u0027\n+        )\n+        raise SyncError(aggregate_errors\u003derrors)\n+\n     def _SyncPhased(\n         self,\n         opt,\n@@ -2130,29 +2170,14 @@\n \n         # If we saw an error, exit with code 1 so that other scripts can check.\n         if err_event.is_set():\n-\n-            def print_and_log(err_msg):\n-                self.git_event_log.ErrorEvent(err_msg)\n-                logger.error(\"%s\", err_msg)\n-\n-            print_and_log(\"error: Unable to fully sync the tree\")\n-            if err_network_sync:\n-                print_and_log(\"error: Downloading network changes failed.\")\n-            if err_update_projects:\n-                print_and_log(\"error: Updating local project lists failed.\")\n-            if err_update_linkfiles:\n-                print_and_log(\"error: Updating copyfiles or linkfiles failed.\")\n-            if err_checkout:\n-                print_and_log(\"error: Checking out local projects failed.\")\n-                if err_results:\n-                    # Don\u0027t log repositories, as it may contain sensitive info.\n-                    logger.error(\"Failing repos:\\n%s\", \"\\n\".join(err_results))\n-            # Not useful to log.\n-            logger.error(\n-                \u0027Try re-running with \"-j1 --fail-fast\" to exit at the first \u0027\n-                \"error.\"\n+            self._ReportErrors(\n+                errors,\n+                err_network_sync\u003derr_network_sync,\n+                err_checkout\u003derr_checkout,\n+                failing_checkout_repos\u003derr_results,\n+                err_update_projects\u003derr_update_projects,\n+                err_update_linkfiles\u003derr_update_linkfiles,\n             )\n-            raise SyncError(aggregate_errors\u003derrors)\n \n     @classmethod\n     def _SyncOneProject(cls, opt, project_index, project) -\u003e _SyncResult:\n@@ -2375,8 +2400,16 @@\n                     err_event.set()\n                     if result.fetch_error:\n                         errors.append(result.fetch_error)\n+                        self._interleaved_err_network \u003d True\n+                        self._interleaved_err_network_results.append(\n+                            result.relpath\n+                        )\n                     if result.checkout_error:\n                         errors.append(result.checkout_error)\n+                        self._interleaved_err_checkout \u003d True\n+                        self._interleaved_err_checkout_results.append(\n+                            result.relpath\n+                        )\n \n             if not ret and opt.fail_fast:\n                 if pool:\n@@ -2407,6 +2440,12 @@\n         2.  Projects that share git objects are processed serially to prevent\n             race conditions.\n         \"\"\"\n+        # Temporary state for tracking errors in interleaved mode.\n+        self._interleaved_err_network \u003d False\n+        self._interleaved_err_network_results \u003d []\n+        self._interleaved_err_checkout \u003d False\n+        self._interleaved_err_checkout_results \u003d []\n+\n         err_event \u003d multiprocessing.Event()\n         synced_relpaths \u003d set()\n         project_list \u003d list(all_projects)\n@@ -2520,17 +2559,23 @@\n \n         pm.end()\n \n-        self._UpdateManifestLists(opt, err_event, errors)\n+        err_update_projects, err_update_linkfiles \u003d self._UpdateManifestLists(\n+            opt, err_event, errors\n+        )\n         if not self.outer_client.manifest.IsArchive:\n             self._GCProjects(project_list, opt, err_event)\n \n         self._PrintManifestNotices(opt)\n         if err_event.is_set():\n-            # TODO(b/421935613): Log errors better like SyncPhased.\n-            logger.error(\n-                \"error: Unable to fully sync the tree in interleaved mode.\"\n+            self._ReportErrors(\n+                errors,\n+                err_network_sync\u003dself._interleaved_err_network,\n+                failing_network_repos\u003dself._interleaved_err_network_results,\n+                err_checkout\u003dself._interleaved_err_checkout,\n+                failing_checkout_repos\u003dself._interleaved_err_checkout_results,\n+                err_update_projects\u003derr_update_projects,\n+                err_update_linkfiles\u003derr_update_linkfiles,\n             )\n-            raise SyncError(aggregate_errors\u003derrors)\n \n \n def _PostRepoUpgrade(manifest, quiet\u003dFalse):\n\n\"\"\"\nIMPORTANT NOTE: Start directly with the output, do not output any delimiters.\nTake a Deep Breath, read the instructions again, read the inputs again. Each instruction is crucial and must be executed with utmost care and attention to detail.\n\nReview:",
      "revId": "99b5a17f2c951fe5979100c36e7e1dbb4c61b36c",
      "serverId": "173816e5-2b9a-37c3-8a2e-48639d4f1153"
    }
  ]
}