Page MenuHomePhorge

Allow an infinite number of arcpatch_DXXXX_X branches to be created
Needs RevisionPublic

Authored by ncollins on Nov 12 2021, 20:23.
Referenced Files
Unknown Object (File)
Sat, Jul 13, 02:53
Unknown Object (File)
Sat, Jul 13, 02:53
Unknown Object (File)
Sat, Jul 13, 02:44
Unknown Object (File)
Fri, Jul 12, 12:41
Unknown Object (File)
Fri, Jul 12, 12:40
Unknown Object (File)
Fri, Jul 12, 12:40
Unknown Object (File)
Fri, Jul 12, 06:42
Unknown Object (File)
Thu, Jul 11, 18:59

Details

Summary

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

Test Plan

WIP

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

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

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.

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