)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":12898,"name":"Tony Breeds","email":"tony@bakeyournoodle.com","username":"tonyb"},"change_message_id":"d9fad2ede0f2a31aa3dc9938ba35c29a69c19f8f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"e8778c64_69f70802","updated":"2024-02-28 01:01:16.000000000","message":"Looks good to me.  I think the checks are okay as is, and certainly bug-compatible with what went before","commit_id":"3351782144fcc6b0526f62ef41e726d72fba8f12"},{"author":{"_account_id":12898,"name":"Tony Breeds","email":"tony@bakeyournoodle.com","username":"tonyb"},"change_message_id":"a76f129028db306ea49a9d70f417b1b2ffef6f64","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"1bb2db77_9726d728","updated":"2024-03-05 00:24:16.000000000","message":"Looks good to me.\n\nNot +A\u0027ing in case there are other changes we want to include in the release","commit_id":"4fddad6558837ba89721db0dd13c9304088b0e59"}],"git_review/cmd.py":[{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"f4ae8b339cd72ea18af8f6492a251e21806e3b86","unresolved":true,"context_lines":[{"line_number":429,"context_line":"            \"git\", \"submodule\", \"foreach\","},{"line_number":430,"context_line":"            \u0027cp -p %s \"$(git rev-parse --git-dir)/hooks/\"\u0027 % target_file)"},{"line_number":431,"context_line":""},{"line_number":432,"context_line":"    if not os.access(target_file, os.X_OK):"},{"line_number":433,"context_line":"        mark_executable(target_file)"},{"line_number":434,"context_line":""},{"line_number":435,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1671932e_b3005724","line":432,"updated":"2024-02-27 21:33:23.000000000","message":"The `X_OK` check only appears to be True if `u+x` is set. If we set `g+x` or `o+x` without `u+x` then we could potentially chmod the file every time we run git-review?\n\nWe\u0027re already statting the file below and checking masks I wonder if we should improve this check too. Something like:\n\n```\nold_mask \u003d os.path.stat.S_IMODE(os.stat(target_file).st_mode)\nif old_mask \u0026 (S_IXUSR | S_IXGRP | S_IXOTH) \u003d\u003d 0:\n    # insert old mark_executable code there using already retrieved mask.\n```","commit_id":"3351782144fcc6b0526f62ef41e726d72fba8f12"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"607fdbe3cae60afaa8554606094a45a855e773e9","unresolved":false,"context_lines":[{"line_number":429,"context_line":"            \"git\", \"submodule\", \"foreach\","},{"line_number":430,"context_line":"            \u0027cp -p %s \"$(git rev-parse --git-dir)/hooks/\"\u0027 % target_file)"},{"line_number":431,"context_line":""},{"line_number":432,"context_line":"    if not os.access(target_file, os.X_OK):"},{"line_number":433,"context_line":"        mark_executable(target_file)"},{"line_number":434,"context_line":""},{"line_number":435,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"421ac963_772417f9","line":432,"in_reply_to":"165f63aa_3be5fc0b","updated":"2024-02-28 21:44:34.000000000","message":"Always setting user perms +rwx avoids problems with this check here. I think this is fine now due to the update in mark_executable.","commit_id":"3351782144fcc6b0526f62ef41e726d72fba8f12"},{"author":{"_account_id":12898,"name":"Tony Breeds","email":"tony@bakeyournoodle.com","username":"tonyb"},"change_message_id":"d9fad2ede0f2a31aa3dc9938ba35c29a69c19f8f","unresolved":true,"context_lines":[{"line_number":429,"context_line":"            \"git\", \"submodule\", \"foreach\","},{"line_number":430,"context_line":"            \u0027cp -p %s \"$(git rev-parse --git-dir)/hooks/\"\u0027 % target_file)"},{"line_number":431,"context_line":""},{"line_number":432,"context_line":"    if not os.access(target_file, os.X_OK):"},{"line_number":433,"context_line":"        mark_executable(target_file)"},{"line_number":434,"context_line":""},{"line_number":435,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"165f63aa_3be5fc0b","line":432,"in_reply_to":"1671932e_b3005724","updated":"2024-02-28 01:01:16.000000000","message":"Wow, that\u0027s kinda funky.  If we\u0027re going to later that check then we\u0027d also need to do it at https://review.opendev.org/c/opendev/git-review/+/910268/2/git_review/cmd.py#1800","commit_id":"3351782144fcc6b0526f62ef41e726d72fba8f12"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"e98be70cb5252bc7d94853df7b8ff5197787f667","unresolved":false,"context_lines":[{"line_number":429,"context_line":"            \"git\", \"submodule\", \"foreach\","},{"line_number":430,"context_line":"            \u0027cp -p %s \"$(git rev-parse --git-dir)/hooks/\"\u0027 % target_file)"},{"line_number":431,"context_line":""},{"line_number":432,"context_line":"    if not os.access(target_file, os.X_OK):"},{"line_number":433,"context_line":"        mark_executable(target_file)"},{"line_number":434,"context_line":""},{"line_number":435,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"d920e14b_71aef1fc","line":432,"in_reply_to":"421ac963_772417f9","updated":"2024-02-29 16:59:11.000000000","message":"Tony, interesting that this is double-guarded like that! Also interesting that the outer one doesn\u0027t look at `UPDATE`, unlike L385...\n\nI wonder if it would be better to always call `set_hooks_commit_msg` -- doesn\u0027t it already perform those checks internally?","commit_id":"3351782144fcc6b0526f62ef41e726d72fba8f12"},{"author":{"_account_id":5263,"name":"Jeremy Stanley","display_name":"fungi","email":"fungi@yuggoth.org","username":"fungi","status":"missing, presumed fed"},"change_message_id":"4cb0fe72d6c162db30823d969042c502a1d49ec2","unresolved":false,"context_lines":[{"line_number":430,"context_line":"            \u0027cp -p %s \"$(git rev-parse --git-dir)/hooks/\"\u0027 % target_file)"},{"line_number":431,"context_line":""},{"line_number":432,"context_line":"    if not os.access(target_file, os.X_OK):"},{"line_number":433,"context_line":"        mark_executable(target_file)"},{"line_number":434,"context_line":""},{"line_number":435,"context_line":""},{"line_number":436,"context_line":"def mark_executable(target_file):"}],"source_content_type":"text/x-python","patch_set":2,"id":"417a2147_625f2e39","line":433,"updated":"2024-02-27 21:50:25.000000000","message":"Good point, it does look like this is reached outside the check for whether or not the file exists, presumably to fix up permissions in case the user has accidentally changed them previously.","commit_id":"3351782144fcc6b0526f62ef41e726d72fba8f12"},{"author":{"_account_id":5263,"name":"Jeremy Stanley","display_name":"fungi","email":"fungi@yuggoth.org","username":"fungi","status":"missing, presumed fed"},"change_message_id":"4cb0fe72d6c162db30823d969042c502a1d49ec2","unresolved":false,"context_lines":[{"line_number":435,"context_line":""},{"line_number":436,"context_line":"def mark_executable(target_file):"},{"line_number":437,"context_line":"    # add executable permission for everyone who has read access"},{"line_number":438,"context_line":"    mask \u003d os.path.stat.S_IMODE(os.stat(target_file).st_mode)"},{"line_number":439,"context_line":"    if mask | os.path.stat.S_IRUSR \u003d\u003d mask:"},{"line_number":440,"context_line":"        mask \u003d mask | os.path.stat.S_IXUSR"},{"line_number":441,"context_line":"    if mask | os.path.stat.S_IRGRP \u003d\u003d mask:"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a13b6c9_8e21ad16","line":438,"updated":"2024-02-27 21:50:25.000000000","message":"As corvus pointed out in #opendev, os.path.stat seems to just be a compatibility alias for stat, so we can shorten all this.","commit_id":"3351782144fcc6b0526f62ef41e726d72fba8f12"},{"author":{"_account_id":5263,"name":"Jeremy Stanley","display_name":"fungi","email":"fungi@yuggoth.org","username":"fungi","status":"missing, presumed fed"},"change_message_id":"4cb0fe72d6c162db30823d969042c502a1d49ec2","unresolved":false,"context_lines":[{"line_number":442,"context_line":"        mask \u003d mask | os.path.stat.S_IXGRP"},{"line_number":443,"context_line":"    if mask | os.path.stat.S_IROTH \u003d\u003d mask:"},{"line_number":444,"context_line":"        mask \u003d mask | os.path.stat.S_IXOTH"},{"line_number":445,"context_line":"    os.chmod(target_file, mask)"},{"line_number":446,"context_line":""},{"line_number":447,"context_line":""},{"line_number":448,"context_line":"def test_remote_url(remote_url):"}],"source_content_type":"text/x-python","patch_set":2,"id":"05ab6d84_d5eabc53","line":445,"updated":"2024-02-27 21:50:25.000000000","message":"I thought about that but didn\u0027t for consistency and because it seemed possible some system might use e.g. group perms but no owner perms, though yes that seems rather unlikely/pathological. I can redo the initial check to just unconditionally add them instead.","commit_id":"3351782144fcc6b0526f62ef41e726d72fba8f12"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"8d3780d3b2bd229ad1a66e230883f551394e31b0","unresolved":true,"context_lines":[{"line_number":442,"context_line":"        mask \u003d mask | os.path.stat.S_IXGRP"},{"line_number":443,"context_line":"    if mask | os.path.stat.S_IROTH \u003d\u003d mask:"},{"line_number":444,"context_line":"        mask \u003d mask | os.path.stat.S_IXOTH"},{"line_number":445,"context_line":"    os.chmod(target_file, mask)"},{"line_number":446,"context_line":""},{"line_number":447,"context_line":""},{"line_number":448,"context_line":"def test_remote_url(remote_url):"}],"source_content_type":"text/x-python","patch_set":2,"id":"7f5b4f4a_8fe2f955","line":445,"updated":"2024-02-27 21:34:35.000000000","message":"We should OR in S_IRUSR and S_IXUSR if there are no read or write bits set as well.","commit_id":"3351782144fcc6b0526f62ef41e726d72fba8f12"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"53eaeef30c4ad708c16839279af10495cc046f7d","unresolved":false,"context_lines":[{"line_number":442,"context_line":"        mask \u003d mask | os.path.stat.S_IXGRP"},{"line_number":443,"context_line":"    if mask | os.path.stat.S_IROTH \u003d\u003d mask:"},{"line_number":444,"context_line":"        mask \u003d mask | os.path.stat.S_IXOTH"},{"line_number":445,"context_line":"    os.chmod(target_file, mask)"},{"line_number":446,"context_line":""},{"line_number":447,"context_line":""},{"line_number":448,"context_line":"def test_remote_url(remote_url):"}],"source_content_type":"text/x-python","patch_set":2,"id":"943a43a1_9d02fa36","line":445,"in_reply_to":"05ab6d84_d5eabc53","updated":"2024-02-28 00:26:06.000000000","message":"My main concern was that we would always end up running these stat checks and never modify things in a way to stop that from happening given weird umask or initial other related file perm conditions.\n\nFor example if the file is set with no user perms we would fail the `if not os.access(target_file, os.X_OK)` check. Then depending on the state of group and other perms may not set any executable bits at all. I think we can address that by modifying the os.access check to check individual executable bits then update the mark_executable() function code to check if any +x is added. If not then set u+rx as a conservative fallback. This is probably overkill and the vast majority of users would never notice. So I\u0027m happy to just leave that out if we think it isn\u0027t necessary.","commit_id":"3351782144fcc6b0526f62ef41e726d72fba8f12"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"607fdbe3cae60afaa8554606094a45a855e773e9","unresolved":false,"context_lines":[{"line_number":442,"context_line":"        mask \u003d mask | os.path.stat.S_IXGRP"},{"line_number":443,"context_line":"    if mask | os.path.stat.S_IROTH \u003d\u003d mask:"},{"line_number":444,"context_line":"        mask \u003d mask | os.path.stat.S_IXOTH"},{"line_number":445,"context_line":"    os.chmod(target_file, mask)"},{"line_number":446,"context_line":""},{"line_number":447,"context_line":""},{"line_number":448,"context_line":"def test_remote_url(remote_url):"}],"source_content_type":"text/x-python","patch_set":2,"id":"b1b5fea8_e9fac820","line":445,"in_reply_to":"7f5b4f4a_8fe2f955","updated":"2024-02-28 21:44:34.000000000","message":"Done","commit_id":"3351782144fcc6b0526f62ef41e726d72fba8f12"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"607fdbe3cae60afaa8554606094a45a855e773e9","unresolved":false,"context_lines":[{"line_number":442,"context_line":"        mask \u003d mask | os.path.stat.S_IXGRP"},{"line_number":443,"context_line":"    if mask | os.path.stat.S_IROTH \u003d\u003d mask:"},{"line_number":444,"context_line":"        mask \u003d mask | os.path.stat.S_IXOTH"},{"line_number":445,"context_line":"    os.chmod(target_file, mask)"},{"line_number":446,"context_line":""},{"line_number":447,"context_line":""},{"line_number":448,"context_line":"def test_remote_url(remote_url):"}],"source_content_type":"text/x-python","patch_set":2,"id":"f0107c7b_3733ee73","line":445,"in_reply_to":"943a43a1_9d02fa36","updated":"2024-02-28 21:44:34.000000000","message":"The latest patchset seems to address my concerns by always giving the owner rwx perms which should prevent the check on line 433 from always running mark_executable each time git-review runs.","commit_id":"3351782144fcc6b0526f62ef41e726d72fba8f12"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"e98be70cb5252bc7d94853df7b8ff5197787f667","unresolved":true,"context_lines":[{"line_number":438,"context_line":"    mask \u003d stat.S_IMODE(os.stat(target_file).st_mode)"},{"line_number":439,"context_line":"    # make sure the owner always has read+write+exec perms"},{"line_number":440,"context_line":"    mask |\u003d stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR"},{"line_number":441,"context_line":"    # add executable permission for everyone else who has read access"},{"line_number":442,"context_line":"    if mask | stat.S_IRGRP \u003d\u003d mask:"},{"line_number":443,"context_line":"        mask |\u003d stat.S_IXGRP"},{"line_number":444,"context_line":"    if mask | stat.S_IROTH \u003d\u003d mask:"}],"source_content_type":"text/x-python","patch_set":4,"id":"ee26a341_0943a940","line":441,"updated":"2024-02-29 16:59:11.000000000","message":"It\u0027s a little curious (to me) that we would only open up these perms _sometimes_ -- i.e., when the owner can\u0027t execute. I think I get the reasoning for the `access` check (don\u0027t call `chmod` when nothing needs to be done), but I think you could get something similar by dropping the `access` check and always doing\n```\norig_mask \u003d stat.S_IMODE(os.stat(target_file).st_mode)\n# make sure the owner always has read+write+exec perms\nmask \u003d orig_mask | stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR\n# add executable permission for everyone else who has read access\nif mask \u0026 stat.S_IRGRP:\n    mask |\u003d stat.S_IXGRP\nif mask \u0026 stat.S_IROTH:\n    mask |\u003d stat.S_IXOTH\n# only update if we made changes\nif mask !\u003d orig_mask:\n    os.chmod(target_file, mask)\n```\nI feel like it would have a more consistent result (plus it saves a `stat` call).","commit_id":"0d8d82903bc8cbc8f13217dc05e012fa9d8d4503"},{"author":{"_account_id":5263,"name":"Jeremy Stanley","display_name":"fungi","email":"fungi@yuggoth.org","username":"fungi","status":"missing, presumed fed"},"change_message_id":"6f4f0e2c4e8d19a96d083e1f595be6f403ef663c","unresolved":false,"context_lines":[{"line_number":438,"context_line":"    mask \u003d stat.S_IMODE(os.stat(target_file).st_mode)"},{"line_number":439,"context_line":"    # make sure the owner always has read+write+exec perms"},{"line_number":440,"context_line":"    mask |\u003d stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR"},{"line_number":441,"context_line":"    # add executable permission for everyone else who has read access"},{"line_number":442,"context_line":"    if mask | stat.S_IRGRP \u003d\u003d mask:"},{"line_number":443,"context_line":"        mask |\u003d stat.S_IXGRP"},{"line_number":444,"context_line":"    if mask | stat.S_IROTH \u003d\u003d mask:"}],"source_content_type":"text/x-python","patch_set":4,"id":"37003a3f_49a514f1","line":441,"updated":"2024-02-29 18:06:45.000000000","message":"Thanks, I agree only calling chmod() if there\u0027s a change to be made is the way to go. The \"when to do it\" is indeed a little fuzzy. The primary goal here is to make sure *initial* permissions are correct when the hook script is first installed (the HTTP method otherwise ends up creating a file with no execute permissions). Historically, git-review has also tried to \"fix\" permissions again later at runtime which, while I can see the appeal, I\u0027m not sure I entirely agree with. On the one hand, if a user intentionally set the hook non-executable it would be nice to avoid silently undoing that, but on the other hand this tool is mostly aimed at making sure users don\u0027t need to know the details of git hook permissions and if they accidentally screw it up then we just fix it for them.","commit_id":"0d8d82903bc8cbc8f13217dc05e012fa9d8d4503"}],"git_review/tests/test_git_review.py":[{"author":{"_account_id":5263,"name":"Jeremy Stanley","display_name":"fungi","email":"fungi@yuggoth.org","username":"fungi","status":"missing, presumed fed"},"change_message_id":"6f4f0e2c4e8d19a96d083e1f595be6f403ef663c","unresolved":false,"context_lines":[{"line_number":105,"context_line":"        self._run_git_review("},{"line_number":106,"context_line":"            \u0027-s\u0027, chdir\u003dos.path.join(self.test_dir, \u0027subdirectory\u0027))"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    def test_install_remote_hook(self):"},{"line_number":109,"context_line":"        \"\"\"Test whether git-review -s correctly creates the commit-msg hook"},{"line_number":110,"context_line":"        from the Gerrit server with appropriate permissions and content."},{"line_number":111,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"f02085d2_fe6901b5","line":108,"updated":"2024-02-29 18:06:45.000000000","message":"Right, to be clear, all hook installations are remote until the follow up change, I just tried to be precise with the test name so that it doesn\u0027t need to be changed later for differentiation.","commit_id":"0d8d82903bc8cbc8f13217dc05e012fa9d8d4503"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"607fdbe3cae60afaa8554606094a45a855e773e9","unresolved":true,"context_lines":[{"line_number":105,"context_line":"        self._run_git_review("},{"line_number":106,"context_line":"            \u0027-s\u0027, chdir\u003dos.path.join(self.test_dir, \u0027subdirectory\u0027))"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    def test_install_remote_hook(self):"},{"line_number":109,"context_line":"        \"\"\"Test whether git-review -s correctly creates the commit-msg hook"},{"line_number":110,"context_line":"        from the Gerrit server with appropriate permissions and content."},{"line_number":111,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"a4974be9_015e0381","line":108,"range":{"start_line":108,"start_character":21,"end_line":108,"end_character":27},"updated":"2024-02-28 21:44:34.000000000","message":"This test isn\u0027t testing the remote hook installation but the local vendored version installation due to the lack of the --remote-hook flag?\n\nShould we have a test that test the --remote-hook flag?","commit_id":"0d8d82903bc8cbc8f13217dc05e012fa9d8d4503"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"7620a9e635d2ad4ef1cf9dbd6a92e521bf64ebc4","unresolved":false,"context_lines":[{"line_number":105,"context_line":"        self._run_git_review("},{"line_number":106,"context_line":"            \u0027-s\u0027, chdir\u003dos.path.join(self.test_dir, \u0027subdirectory\u0027))"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    def test_install_remote_hook(self):"},{"line_number":109,"context_line":"        \"\"\"Test whether git-review -s correctly creates the commit-msg hook"},{"line_number":110,"context_line":"        from the Gerrit server with appropriate permissions and content."},{"line_number":111,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"d88f6356_073874ad","line":108,"range":{"start_line":108,"start_character":21,"end_line":108,"end_character":27},"in_reply_to":"a4974be9_015e0381","updated":"2024-02-28 21:47:59.000000000","message":"The followup change addresses this concern.","commit_id":"0d8d82903bc8cbc8f13217dc05e012fa9d8d4503"}]}