)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":30073,"name":"Brendan Shephard","email":"bshephar@bne-home.net","username":"bshephar"},"change_message_id":"c12764b27ced09a809cb88352eab3cb05f0b5b24","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"487fb1b4_c0a20833","updated":"2026-05-08 22:34:00.000000000","message":"Nice catch man!","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"33303b0838ed48d83dc5a2879be36d5eda96fe63","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"e57edea9_4edb7ccf","updated":"2026-05-08 23:28:35.000000000","message":"recheck\n\nI\u0027m pretty sure the tempauth post failure is unrelated; I didn\u0027t see any logs/output when I tried to click through on it","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"}],"swift/common/middleware/s3api/s3request.py":[{"author":{"_account_id":30073,"name":"Brendan Shephard","email":"bshephar@bne-home.net","username":"bshephar"},"change_message_id":"c12764b27ced09a809cb88352eab3cb05f0b5b24","unresolved":true,"context_lines":[{"line_number":445,"context_line":"            self._to_read -\u003d len(buf)"},{"line_number":446,"context_line":"            if readline and buf[-1:] \u003d\u003d b\u0027\\n\u0027:"},{"line_number":447,"context_line":"                break"},{"line_number":448,"context_line":"            if not buf and not self._completed_payload:"},{"line_number":449,"context_line":"                raise S3InputIncomplete"},{"line_number":450,"context_line":"        return b\u0027\u0027.join(bufs)"},{"line_number":451,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"95cdeba7_08f1adf0","line":448,"updated":"2026-05-08 22:34:00.000000000","message":"Wouldn\u0027t you want to do this earlier? Does all of the checking on L436-L445 provide any value if we\u0027re just going to raise here? Couldn\u0027t we move this new addition to L435 before buf.append() and raise a bit earlier after we\u0027ve finished collecting `buf`?\n\nMaybe like:\n```\n  if readline:\n    buf \u003d self._chunk_reader.readline(size - bytes_read)\n  else:\n    buf \u003d self._chunk_reader.read(size - bytes_read)\n  if not buf and self._chunk_reader.chunk_size !\u003d 0:\n      raise S3InputIncomplete\n  bufs.append(buf)\n  if self._chunk_reader.to_read \u003d\u003d 0:\n```","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"},{"author":{"_account_id":30073,"name":"Brendan Shephard","email":"bshephar@bne-home.net","username":"bshephar"},"change_message_id":"4f505152ab954b602771aef92fdb1d6260cf356d","unresolved":false,"context_lines":[{"line_number":445,"context_line":"            self._to_read -\u003d len(buf)"},{"line_number":446,"context_line":"            if readline and buf[-1:] \u003d\u003d b\u0027\\n\u0027:"},{"line_number":447,"context_line":"                break"},{"line_number":448,"context_line":"            if not buf and not self._completed_payload:"},{"line_number":449,"context_line":"                raise S3InputIncomplete"},{"line_number":450,"context_line":"        return b\u0027\u0027.join(bufs)"},{"line_number":451,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"1d2f00be_3b1e096a","line":448,"in_reply_to":"20a9aca2_7bbe8cfe","updated":"2026-05-08 23:56:32.000000000","message":"Yeah, apologies I unintentionally changed the if statement. I only included it for illustrative purposes to demonstrate the positioning I was proposing. The main point of my comment was to inquire about the usefulness of the preceding code. If we had no use for it, then it would be unnecessary CPU cycles. But I think you have answered this when you said the value is in keeping the control logic and retaining the primary exit mechanism.\n\nThanks for the clarification.","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"33303b0838ed48d83dc5a2879be36d5eda96fe63","unresolved":true,"context_lines":[{"line_number":445,"context_line":"            self._to_read -\u003d len(buf)"},{"line_number":446,"context_line":"            if readline and buf[-1:] \u003d\u003d b\u0027\\n\u0027:"},{"line_number":447,"context_line":"                break"},{"line_number":448,"context_line":"            if not buf and not self._completed_payload:"},{"line_number":449,"context_line":"                raise S3InputIncomplete"},{"line_number":450,"context_line":"        return b\u0027\u0027.join(bufs)"},{"line_number":451,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"20a9aca2_7bbe8cfe","line":448,"in_reply_to":"95cdeba7_08f1adf0","updated":"2026-05-08 23:28:35.000000000","message":"personally I find `and not self._complete_playload` rolls of the tongue a little better than `self._chunk_reader.chunk_size !\u003d 0` (\"not !\u003d 0\"?  \"is \u003d\u003d 0\"?  \"\u003c\u003d 0\"? or... not?)\n\nI also like the symmetry of the the condition that terminates the loop early (with an error) being at the END of a `while` loop that is predicated on the *same* condition `while not self._completed_payload` signaling clearly that `if [not] buf *and* completed_playload\" we\u0027re going to exit anyway.\n\n\u003e Wouldn\u0027t you want to do this earlier?\n\nno I like it where I put it; other implementations could also make the new tests pass tho - and that\u0027s probably the main thing that matters.  CRITICAL.  BUG.  FIX.  The rest is just non-material personal preference; beauty is in the eye and all that:\n\nhttps://github.com/NVIDIA/swift/blob/master/REVIEW_GUIDELINES.rst#maintainable-code-is-obvious\n\n\u003e Does all of the checking on L436-L445 provide any value\n\nI think the value is it keeps the existing logic for `_completed_playload` un-disturbed, which I think is a good thing *if* we\u0027re trying to keep `self._completed_playload` as the loop\u0027s *primary* exit/control interface.\n\n\u003e Couldn\u0027t we move ... raise a bit earlier\n\ncorrect, other diffs could also fix the included tests.  this one is mine.  ;)\n\nI think your logic is sound, but i\u0027m not sure I understand your reasoning.  It feels less like \"I think this is more readable/maintainable/obvious because XYZ\" and more like \"just confirming there\u0027s nothing special about the placement; the main thing is we need to detect when \"empty buff is bad\" (i.e. anytime the playload is not complete) and raise an error, correct?\" - in which case: \"correct\"\n\nThis was the most obvious way I could find to spell: \"it\u0027s a bug if this loop does not exit when when payload complete OR if buff is empty and NOT payload complete\"\n\nI could have also gone with `if not (buf or complete)` - i wrote it the way I did on purpose; not because it makes the computer happier (the computer doesn\u0027t care about readability/maintainability) but because it makes ME happiest (*I* care).\n\nAs far as \"but exit sooner is more *efficient*\" - this is already a hot loop and this is a special error condition; I don\u0027t care about `len(0)` running right before I raise an error.  I care about reading/understanding/maintaining the correct behavior of this parser for another 10 years w/o forgetting to raise an error on empty buff and payload not complete.  Using `_complete_payload` *leverages* an abstraction instead of breaking it.   Doing the check before continuing the loop re-enforces the special error condition and the guarantee of safe termination by *emphasizing* the termination rather than burying it w/ other logic.  That\u0027s my thinking at least...","commit_id":"24acbb55eedf68db06bdc418a901b08057538613"}]}