)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"04b4192e14f2b8fe539faaf613c9815af21474ba","unresolved":false,"context_lines":[{"line_number":17,"context_line":"For the branch cache we need to distinguish three different cases:"},{"line_number":18,"context_line":"1. branche cache miss (no fetch yet)"},{"line_number":19,"context_line":"2. branch cache hit (including empty list of branches)"},{"line_number":20,"context_line":"3. fetch error"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Instead of storing an empty list in case of a fetch error we will store"},{"line_number":23,"context_line":"a sentinel value of `None` in those cases. However, with this change we"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"960abe10_9f7bb321","line":20,"updated":"2022-07-01 16:16:20.000000000","message":"-1: I think somewhere -- maybe in the class docs? -- we should document what the possible states are.  I think there\u0027s good info in this commit message that we should make sure is in the code.","commit_id":"70579fe081b6c7fe293ed1542d7e493c9640fce4"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"2c387ba1a5d819fce5afe09962a8ab999cc7dd06","unresolved":false,"context_lines":[{"line_number":17,"context_line":"For the branch cache we need to distinguish three different cases:"},{"line_number":18,"context_line":"1. branche cache miss (no fetch yet)"},{"line_number":19,"context_line":"2. branch cache hit (including empty list of branches)"},{"line_number":20,"context_line":"3. fetch error"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Instead of storing an empty list in case of a fetch error we will store"},{"line_number":23,"context_line":"a sentinel value of `None` in those cases. However, with this change we"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"d994071d_2872556e","line":20,"updated":"2022-07-01 16:17:00.000000000","message":"Er, I didn\u0027t really mean for this to be a -1.  More of a strong suggestion.","commit_id":"70579fe081b6c7fe293ed1542d7e493c9640fce4"},{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"b1a9605630f3b7bd6d52edf7b060bc181507c007","unresolved":false,"context_lines":[{"line_number":17,"context_line":"For the branch cache we need to distinguish three different cases:"},{"line_number":18,"context_line":"1. branche cache miss (no fetch yet)"},{"line_number":19,"context_line":"2. branch cache hit (including empty list of branches)"},{"line_number":20,"context_line":"3. fetch error"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Instead of storing an empty list in case of a fetch error we will store"},{"line_number":23,"context_line":"a sentinel value of `None` in those cases. However, with this change we"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"9429f1f8_fa3b392e","line":20,"in_reply_to":"d994071d_2872556e","updated":"2022-07-04 09:48:34.000000000","message":"Done","commit_id":"70579fe081b6c7fe293ed1542d7e493c9640fce4"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"306c9f81b52813c8c6771b4f249fed0456c5c97d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"6beece8d_09675437","updated":"2022-07-01 09:50:32.000000000","message":"recheck","commit_id":"70579fe081b6c7fe293ed1542d7e493c9640fce4"}],"zuul/connection/__init__.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"b0acfe2e4e0c0fa6aab4338141ce2c32b93bbca7","unresolved":false,"context_lines":[{"line_number":249,"context_line":"        if self._branch_cache:"},{"line_number":250,"context_line":"            try:"},{"line_number":251,"context_line":"                branches \u003d self._branch_cache.getProjectBranches("},{"line_number":252,"context_line":"                    project.name, exclude_unprotected, min_ltime)"},{"line_number":253,"context_line":"            except LookupError:"},{"line_number":254,"context_line":"                if self.read_only:"},{"line_number":255,"context_line":"                    # A scheduler hasn\u0027t attempted to fetch them yet"}],"source_content_type":"text/x-python","patch_set":3,"id":"a1571947_a7965859","line":252,"updated":"2022-07-01 15:30:26.000000000","message":"Not a minus one, but I think a reasonable thing to consider: I think this is the only place we actually use LookupError.  We could simplify a bunch of calling sites by adding in a \"want_exceptions\" argument, false by default, true here.","commit_id":"70579fe081b6c7fe293ed1542d7e493c9640fce4"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"bf33f96e1baf6e7ece491d1ed58e4e7d4fd47151","unresolved":false,"context_lines":[{"line_number":249,"context_line":"        if self._branch_cache:"},{"line_number":250,"context_line":"            try:"},{"line_number":251,"context_line":"                branches \u003d self._branch_cache.getProjectBranches("},{"line_number":252,"context_line":"                    project.name, exclude_unprotected, min_ltime)"},{"line_number":253,"context_line":"            except LookupError:"},{"line_number":254,"context_line":"                if self.read_only:"},{"line_number":255,"context_line":"                    # A scheduler hasn\u0027t attempted to fetch them yet"}],"source_content_type":"text/x-python","patch_set":3,"id":"cd2dc466_b0da7e8d","line":252,"updated":"2022-07-06 13:18:49.000000000","message":"This works for me.","commit_id":"70579fe081b6c7fe293ed1542d7e493c9640fce4"},{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"b1a9605630f3b7bd6d52edf7b060bc181507c007","unresolved":false,"context_lines":[{"line_number":249,"context_line":"        if self._branch_cache:"},{"line_number":250,"context_line":"            try:"},{"line_number":251,"context_line":"                branches \u003d self._branch_cache.getProjectBranches("},{"line_number":252,"context_line":"                    project.name, exclude_unprotected, min_ltime)"},{"line_number":253,"context_line":"            except LookupError:"},{"line_number":254,"context_line":"                if self.read_only:"},{"line_number":255,"context_line":"                    # A scheduler hasn\u0027t attempted to fetch them yet"}],"source_content_type":"text/x-python","patch_set":3,"id":"e4cab70e_f51b7b1a","line":252,"in_reply_to":"a1571947_a7965859","updated":"2022-07-04 09:48:34.000000000","message":"I think that makes sense. I solved it a bit differently by introducing a `default` argument to be returned in case of a cache miss. This way I can keep raising the LookupError by default.\n\nI felt that was the least surprising behavior. Not raising by default conflates the cache miss and error case, so I wanted to make that a bit more explicit.\n\nIf you have a strong opinion on the default behavior I\u0027m open to changing that the way you suggested.","commit_id":"70579fe081b6c7fe293ed1542d7e493c9640fce4"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"f18f65d6679980449b0d7df8711eb910cc851d89","unresolved":true,"context_lines":[{"line_number":313,"context_line":"            # cache, so we never clear it."},{"line_number":314,"context_line":""},{"line_number":315,"context_line":"            branches \u003d self._branch_cache.getProjectBranches("},{"line_number":316,"context_line":"                project_name, True, default\u003dNone)"},{"line_number":317,"context_line":""},{"line_number":318,"context_line":"            if not branches:"},{"line_number":319,"context_line":"                branches \u003d []"}],"source_content_type":"text/x-python","patch_set":4,"id":"1999592f_b541a8de","line":316,"range":{"start_line":316,"start_character":36,"end_line":316,"end_character":48},"updated":"2022-07-06 16:07:18.000000000","message":"Nit: You should be able to set the default to [] here and then drop the block on lines 318-319.","commit_id":"c98f14025aa4f72334eab437401ceb2a960e6915"},{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"1f2a6f3bc207f0a698c6480e1f5c1f5b22278659","unresolved":true,"context_lines":[{"line_number":313,"context_line":"            # cache, so we never clear it."},{"line_number":314,"context_line":""},{"line_number":315,"context_line":"            branches \u003d self._branch_cache.getProjectBranches("},{"line_number":316,"context_line":"                project_name, True, default\u003dNone)"},{"line_number":317,"context_line":""},{"line_number":318,"context_line":"            if not branches:"},{"line_number":319,"context_line":"                branches \u003d []"}],"source_content_type":"text/x-python","patch_set":4,"id":"85cdecf4_8e84c92b","line":316,"range":{"start_line":316,"start_character":36,"end_line":316,"end_character":48},"in_reply_to":"1999592f_b541a8de","updated":"2022-07-06 16:10:19.000000000","message":"I think that wouldn\u0027t work as branches can still be None in the error case. The default is only returned when there is a cache miss.","commit_id":"c98f14025aa4f72334eab437401ceb2a960e6915"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"fa0d4ca4d8dba36a11fc55b86fde1648d41d81f5","unresolved":true,"context_lines":[{"line_number":313,"context_line":"            # cache, so we never clear it."},{"line_number":314,"context_line":""},{"line_number":315,"context_line":"            branches \u003d self._branch_cache.getProjectBranches("},{"line_number":316,"context_line":"                project_name, True, default\u003dNone)"},{"line_number":317,"context_line":""},{"line_number":318,"context_line":"            if not branches:"},{"line_number":319,"context_line":"                branches \u003d []"}],"source_content_type":"text/x-python","patch_set":4,"id":"b1fae849_96285539","line":316,"range":{"start_line":316,"start_character":36,"end_line":316,"end_character":48},"in_reply_to":"85cdecf4_8e84c92b","updated":"2022-07-06 16:12:10.000000000","message":"Oh right. None is the default and the error value. Only the default would change by setting it to [] but the error value could be None.","commit_id":"c98f14025aa4f72334eab437401ceb2a960e6915"}],"zuul/driver/github/githubconnection.py":[{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"beddba20f22217b1f64de0b8b0bad8aa44e404fd","unresolved":true,"context_lines":[{"line_number":630,"context_line":"        # Get all protected branches"},{"line_number":631,"context_line":"        new_protected_branches \u003d set("},{"line_number":632,"context_line":"            self.connection._branch_cache.getProjectBranches("},{"line_number":633,"context_line":"                project_name, True))"},{"line_number":634,"context_line":""},{"line_number":635,"context_line":"        newly_protected \u003d new_protected_branches - old_protected_branches"},{"line_number":636,"context_line":"        newly_unprotected \u003d old_protected_branches - new_protected_branches"}],"source_content_type":"text/x-python","patch_set":2,"id":"106d8fcf_cbdf26f1","line":633,"updated":"2022-06-30 23:27:01.000000000","message":"Don\u0027t we need to catch the LookupError here too? Or does raising an error properly finish the processing necessary here? It seems that the newly_protected and newly_unprotected values would be wrong if old_proected_branches is an empty set due to the LookupError as well. Maybe we should raise early if we just want to stop processing.\n\nSeperately do we need to handle the case where we attempt set(None) which is a TypeError: \u0027NoneType\u0027 object is not iterable?","commit_id":"e8b5a1c7dc6ebcc917e63658d74d6173426676ce"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"09d7832d82861d4b696437754c3aa22cbb1a8e53","unresolved":false,"context_lines":[{"line_number":630,"context_line":"        # Get all protected branches"},{"line_number":631,"context_line":"        new_protected_branches \u003d set("},{"line_number":632,"context_line":"            self.connection._branch_cache.getProjectBranches("},{"line_number":633,"context_line":"                project_name, True))"},{"line_number":634,"context_line":""},{"line_number":635,"context_line":"        newly_protected \u003d new_protected_branches - old_protected_branches"},{"line_number":636,"context_line":"        newly_unprotected \u003d old_protected_branches - new_protected_branches"}],"source_content_type":"text/x-python","patch_set":2,"id":"f0e153cd_fb7ca40b","line":633,"updated":"2022-07-01 15:30:22.000000000","message":"I think the key thing to note about updateProjectBranches is that it doesn\u0027t want to perform more queries than necessary.  In particular, we don\u0027t want it to look up unprotected branches if there are no tenants that use unprotected branches.  That\u0027s why it checks to see if there is a cache entry for each of the two forks and only runs that query if there is.\n\nIf one of the forks is null, it doesn\u0027t have the information needed to know whether that query is necessary or not (the exclude_unprotected argument to getProjectBranches supplies that information in that case, so it knows whether it should perform a query).","commit_id":"e8b5a1c7dc6ebcc917e63658d74d6173426676ce"},{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"3213540c2e96cc69583cd7abe3a6c03c8fd8d39a","unresolved":true,"context_lines":[{"line_number":630,"context_line":"        # Get all protected branches"},{"line_number":631,"context_line":"        new_protected_branches \u003d set("},{"line_number":632,"context_line":"            self.connection._branch_cache.getProjectBranches("},{"line_number":633,"context_line":"                project_name, True))"},{"line_number":634,"context_line":""},{"line_number":635,"context_line":"        newly_protected \u003d new_protected_branches - old_protected_branches"},{"line_number":636,"context_line":"        newly_unprotected \u003d old_protected_branches - new_protected_branches"}],"source_content_type":"text/x-python","patch_set":2,"id":"6f3f2405_75da17c2","line":633,"in_reply_to":"106d8fcf_cbdf26f1","updated":"2022-07-01 07:36:55.000000000","message":"Yep, we need to deal with the set(None) case.\n\nI think it makes sense to restructure this method a bit and abort the event processing early. When the branch cache is empty (LookupError) or there was an error (None) it doesn\u0027t make sense to continue as `updateProjectBranches()` only updates when there are existing branches.\n\nIt also looks like we don\u0027t have to deal with None/LookupError for the \"new protected branches\" case, as we know that the cache is not empty and a fetch error in `updateProjectBranches()` would raise an exception and abort the event processing.\n\nWhat I\u0027m not sure is if the logic behind `updateProjectBranches()` is correct. I\u0027d expect it to try and correct the branch cache when there was a fetch error or even populate the branch cache if empty. But I think I might be missing something about the life cycle of the branch cache.","commit_id":"e8b5a1c7dc6ebcc917e63658d74d6173426676ce"}],"zuul/zk/branch_cache.py":[{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"beddba20f22217b1f64de0b8b0bad8aa44e404fd","unresolved":true,"context_lines":[{"line_number":142,"context_line":"                raise LookupError(f\"No branches for project {project_name}\")"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"        if exclude_unprotected:"},{"line_number":145,"context_line":"            if protected_branches is not None:"},{"line_number":146,"context_line":"                return protected_branches"},{"line_number":147,"context_line":"        else:"},{"line_number":148,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"48dcf08f_911f4a27","line":145,"updated":"2022-06-30 23:27:01.000000000","message":"I think we can/should simplify the code here. If protected_branches is None we\u0027ll return None at the end of the function. Instead we should be able to simplify. Either return early here omitting the is not None check or something like:\n\n  if not exclude_unprotected:\n      # AKA include unprotected\n      try:\n          remainder_branches \u003d self.cache.remainder[project_name]\n      except KeyError:\n          raise LookupError(f\"No branches for project {project_name}\")\n      if remainder_branches is not None:\n          return (protected_branches or []) + remainder_branches\n  return None\n  \nThen we only process remainder branches if necessary and fall through and return none otherwise. A bit easier to follow when None is returned and when we raise the exception.","commit_id":"e8b5a1c7dc6ebcc917e63658d74d6173426676ce"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"a5faafabdbf96d0571ec9692cf66b15b34f99e75","unresolved":false,"context_lines":[{"line_number":142,"context_line":"                raise LookupError(f\"No branches for project {project_name}\")"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"        if exclude_unprotected:"},{"line_number":145,"context_line":"            if protected_branches is not None:"},{"line_number":146,"context_line":"                return protected_branches"},{"line_number":147,"context_line":"        else:"},{"line_number":148,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"4e7d94a3_c3315204","line":145,"updated":"2022-07-01 00:12:25.000000000","message":"I\u0027m having trouble following that logic.  You say \"if protected_branches is None we\u0027ll return None\", but if remainder_branches in not None, then we can return something other than None even if protected branches is None.  Am I missing something?\n\n(Basically: run through this code with: exclude_unprotected\u003dTrue; protected_branches\u003dNone; remainder_branches\u003d[\u0027master\u0027];)","commit_id":"e8b5a1c7dc6ebcc917e63658d74d6173426676ce"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"55e70eab49b46271a5ce61d30b1bed51b712c7b2","unresolved":false,"context_lines":[{"line_number":142,"context_line":"                raise LookupError(f\"No branches for project {project_name}\")"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"        if exclude_unprotected:"},{"line_number":145,"context_line":"            if protected_branches is not None:"},{"line_number":146,"context_line":"                return protected_branches"},{"line_number":147,"context_line":"        else:"},{"line_number":148,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"06af7692_774873a9","line":145,"updated":"2022-07-01 00:47:35.000000000","message":"That update looks equivalent to me.","commit_id":"e8b5a1c7dc6ebcc917e63658d74d6173426676ce"},{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"3213540c2e96cc69583cd7abe3a6c03c8fd8d39a","unresolved":false,"context_lines":[{"line_number":142,"context_line":"                raise LookupError(f\"No branches for project {project_name}\")"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"        if exclude_unprotected:"},{"line_number":145,"context_line":"            if protected_branches is not None:"},{"line_number":146,"context_line":"                return protected_branches"},{"line_number":147,"context_line":"        else:"},{"line_number":148,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"06f41ac7_740c38a7","line":145,"in_reply_to":"06af7692_774873a9","updated":"2022-07-01 07:36:55.000000000","message":"Yep, makes sense.","commit_id":"e8b5a1c7dc6ebcc917e63658d74d6173426676ce"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"fd04424a25a49e5a13a95ae873e5e06c35d6ce7a","unresolved":false,"context_lines":[{"line_number":142,"context_line":"                raise LookupError(f\"No branches for project {project_name}\")"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"        if exclude_unprotected:"},{"line_number":145,"context_line":"            if protected_branches is not None:"},{"line_number":146,"context_line":"                return protected_branches"},{"line_number":147,"context_line":"        else:"},{"line_number":148,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"9246521e_8970b039","line":145,"in_reply_to":"4e7d94a3_c3315204","updated":"2022-07-01 00:25:36.000000000","message":"The remaining branches handling is in an else block to the if exclude_unprotected: condition. That means the block here is exclusive to processing remaining branches.\n\nif exclude_unprotected \u003d\u003d True then we skip over the else block when protected_branches is None and return None at the end of the method. I think we can simplify that by just checking if we need to handle remaining branches else return None.","commit_id":"e8b5a1c7dc6ebcc917e63658d74d6173426676ce"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"7605a5715ce458d015918e07b296962203274a64","unresolved":false,"context_lines":[{"line_number":142,"context_line":"                raise LookupError(f\"No branches for project {project_name}\")"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"        if exclude_unprotected:"},{"line_number":145,"context_line":"            if protected_branches is not None:"},{"line_number":146,"context_line":"                return protected_branches"},{"line_number":147,"context_line":"        else:"},{"line_number":148,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"deb25ccb_cb757e61","line":145,"in_reply_to":"9246521e_8970b039","updated":"2022-07-01 00:29:31.000000000","message":"Oh wait I think I see what you might have been trying to get at. Remaining branches isn\u0027t the problem it is returning the protected_branches properly. My example should be:\n\n  if not exclude_unprotected:\n      # AKA include unprotected\n      try:\n          remainder_branches \u003d self.cache.remainder[project_name]\n      except KeyError:\n          raise LookupError(f\"No branches for project {project_name}\")\n      if remainder_branches is not None:\n          return (protected_branches or []) + remainder_branches\n  return protected_branches","commit_id":"e8b5a1c7dc6ebcc917e63658d74d6173426676ce"}]}