81a5a3dfa6
Patch Set 11: Code-Review+1
(1 comment)
Patch-set: 11
Reviewer: Gerrit User 4127492 <4127492@173816e5-2b9a-37c3-8a2e-48639d4f1153>
Label: Code-Review=+1, 060b0ff9a5f88d0f27ace78ce8d29a01e1be1b5e
Attention: {"person_ident":"Gerrit User 4127492 \u003c4127492@173816e5-2b9a-37c3-8a2e-48639d4f1153\u003e","operation":"REMOVE","reason":"\u003cGERRIT_ACCOUNT_4127492\u003e replied on the change"}
Attention: {"person_ident":"Gerrit User 1002666 \u003c1002666@173816e5-2b9a-37c3-8a2e-48639d4f1153\u003e","operation":"ADD","reason":"\u003cGERRIT_ACCOUNT_4127492\u003e replied on the change"}
75 lines
No EOL
3.7 KiB
Text
75 lines
No EOL
3.7 KiB
Text
{
|
||
"comments": [
|
||
{
|
||
"unresolved": true,
|
||
"key": {
|
||
"uuid": "39f7fbc6_e71028de",
|
||
"filename": "subcmds/sync.py",
|
||
"patchSetId": 4
|
||
},
|
||
"lineNbr": 1753,
|
||
"author": {
|
||
"id": 1002666
|
||
},
|
||
"writtenOn": "2025-06-03T19:39:45Z",
|
||
"side": 1,
|
||
"message": "I think you should be using the `RepoHook.FromSubcmd()` static method to populate most of the constructor args. Check the usage in `subcmds/upload.py`.",
|
||
"revId": "54227f79bcb8a6af52c6075066ac681c7b9e02f4",
|
||
"serverId": "173816e5-2b9a-37c3-8a2e-48639d4f1153"
|
||
},
|
||
{
|
||
"unresolved": false,
|
||
"key": {
|
||
"uuid": "eb8f03d1_0b40ea3c",
|
||
"filename": "subcmds/sync.py",
|
||
"patchSetId": 4
|
||
},
|
||
"lineNbr": 1753,
|
||
"author": {
|
||
"id": 4127492
|
||
},
|
||
"writtenOn": "2025-06-04T03:15:57Z",
|
||
"side": 1,
|
||
"message": "Thanks for the suggestion! Addressed in patchset 7.\n\n- Switched to `RepoHook.FromSubcmd()` for hook construction (same pattern as in `upload.py`)\n- Since `sync` doesn\u0027t have a native `opt` object, I introduced a minimal `DummyOpt` with the required attributes:\n - `bypass_hooks`\n - `allow_all_hooks`\n - `ignore_hooks`\n\nThis keeps the change self-contained without modifying `RepoHook` itself. Let me know if you\u0027d prefer an alternative (e.g., fallback handling inside `FromSubcmd()`).",
|
||
"parentUuid": "39f7fbc6_e71028de",
|
||
"revId": "54227f79bcb8a6af52c6075066ac681c7b9e02f4",
|
||
"serverId": "173816e5-2b9a-37c3-8a2e-48639d4f1153"
|
||
},
|
||
{
|
||
"unresolved": true,
|
||
"key": {
|
||
"uuid": "39848544_2c387cf8",
|
||
"filename": "subcmds/sync.py",
|
||
"patchSetId": 4
|
||
},
|
||
"lineNbr": 1753,
|
||
"author": {
|
||
"id": 1002666
|
||
},
|
||
"writtenOn": "2025-06-04T22:53:22Z",
|
||
"side": 1,
|
||
"message": "\u003e Thanks for the suggestion! Addressed in patchset 7.\n\u003e \n\u003e - Switched to `RepoHook.FromSubcmd()` for hook construction (same pattern as in `upload.py`)\n\u003e - Since `sync` doesn\u0027t have a native `opt` object, I introduced a minimal `DummyOpt` with the required attributes:\n\u003e - `bypass_hooks`\n\u003e - `allow_all_hooks`\n\u003e - `ignore_hooks`\n\nI think you need solve that by doing something like `RepoHook.AddOptionGroup(p, \u0027post-sync\u0027)` (probably at L568). That\u0027s what was done in upload.py when the hook code was [moved to RepoHook](https://gerrit.googlesource.com/git-repo/+/7f7acfe9fd93cfd4a697f2bc851d1b8182f6336e%5E%21/#F0).",
|
||
"parentUuid": "eb8f03d1_0b40ea3c",
|
||
"revId": "54227f79bcb8a6af52c6075066ac681c7b9e02f4",
|
||
"serverId": "173816e5-2b9a-37c3-8a2e-48639d4f1153"
|
||
},
|
||
{
|
||
"unresolved": false,
|
||
"key": {
|
||
"uuid": "ac5f2971_628712d1",
|
||
"filename": "subcmds/sync.py",
|
||
"patchSetId": 4
|
||
},
|
||
"lineNbr": 1753,
|
||
"author": {
|
||
"id": 4127492
|
||
},
|
||
"writtenOn": "2025-06-05T07:52:25Z",
|
||
"side": 1,
|
||
"message": "Thanks! In patchset 10, I added `RepoHook.AddOptionGroup(p, \u0027post-sync\u0027)` to `_Options()` in `sync.py`, consistent with how `upload.py` does it for `pre-upload`.\n\nThis allows `opt` to include the expected flags (`bypass_hooks`, `allow_all_hooks`, `ignore_hooks`) without needing to manually inject them.\n\nPreviously, I had assumed `opt` didn’t exist or was missing those attributes — I’ve corrected that understanding and aligned this patch with the standard hook integration pattern.\n\nLet me know if there’s anything else to adjust!",
|
||
"parentUuid": "39848544_2c387cf8",
|
||
"revId": "54227f79bcb8a6af52c6075066ac681c7b9e02f4",
|
||
"serverId": "173816e5-2b9a-37c3-8a2e-48639d4f1153"
|
||
}
|
||
]
|
||
} |