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++`.

speck added inline comments.Dec 1 2021, 16:49

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}"; }

Cigaryno added a subscriber: Cigaryno.Oct 22 2022, 09:41

Cigaryno added a project: Arcanist (archived).Nov 5 2022, 08:38

arc lint

Harbormaster completed remote builds in B118: Diff 322.Feb 21 2023, 06:09

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.

valerio.bozzolan mentioned this in T15173: Running "arc patch D123" 4 times fails.Mar 19 2023, 13:11

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.

This revision now requires changes to proceed.Mar 19 2023, 14:15

valerio.bozzolan edited the summary of this revision. (Show Details)Mar 26 2023, 08:53