WIP
Make the Arcanist arc patch command to follow the "zero one infinity" rule.
Before this change, you cannot run the same command more than 4 times.
Closes T15173
Details
Details
- Reviewers
valerio.bozzolan - Group Reviewers
O1: Blessed Committers - Maniphest Tasks
- T15173: Running "arc patch D123" 4 times fails
WIP
Diff Detail
Diff Detail
- Repository
- rARC Arcanist
- Branch
- arcpatch-D25027
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 118 Build 118: arc lint + arc unit
Event Timeline
Comment Actions
Although there's merit to the "zero, one, infinity" rule, it might not be the best option here. If something goes wrong and $err happens to always be falsy, this will end up in an infinite loop instead of giving a clear error message. There is probably a reasonable finite value (that's greater than 4) which can be chosen as the limit to the number of attempts.
| src/workflow/ArcanistPatchWorkflow.php | ||
|---|---|---|
| 222 | Will this result in "{$base_name}_" rather than "{$base_name}" as the first proposed branch name? | |
| 235 | This can be just $proposed_suffix++. | |
| src/workflow/ArcanistPatchWorkflow.php | ||
|---|---|---|
| 221 | I think the linter would've requested there to be a space between while and open paren | |
| 222 | Yea I think this would result in the first non-collision attempt always being suffixed with underscore. The naive approach here would be if ($proposed_suffix == null) { $proposed_name = $base_name; } else { $proposed_name = "{$base_name}_{$proposed_suffix}"; } | |
Comment Actions
I also think that having the first branch called as arcpatch-D123_ (with trailing underscore) as default could be considered a breaking change by some users. I think this can and it should be avoided.
Comment Actions
I see that you marked this as "Work in progress." I don't know how to highlight this fact, so I'm marking this as "Request Changes" (also because some changes were requested). Sorry if this puts a red mark, but I like this proposal of infinite executions of arc patch.