Page MenuHomePhorge

Implements arc work for git workflow
Needs ReviewPublic

Authored by nib on Feb 13 2025, 05:07.

Details

Summary

Makes arc work [task] work with git. Creates git branch with task id and task name.

Closes T15993

Test Plan

Before the patch, try something like arc work T12345 (e.g. T15993 that exists here in Phorge) and see that a TODO exception is raised.

After the patch, try again, and you see that a new branch is created:

BRANCH  Checking out branch "T15993-support-for-arc-work-t12345-(workontask-workflow)".

Run git status to double-check if you are in the mentioned branch.

Run the command again, and you are still in the correct branch. So, the system recognizes that a branch already exist and use that again.

Diff Detail

Repository
rARC Arcanist
Branch
arcpatch-D25872_3
Lint
Lint Skipped
Unit
Test Failures
Build Status
Buildable 1784
Build 1784: arc lint + arc unit

Unit TestsFailed

TimeTest
213 msPhutilLibraryTestCase::testLibraryMap
EXCEPTION (CommandException): JSON command 'php -f /Users/nick/Code/PhpstormProjects/arcanist2/src/moduleutils/../../support/lib/extract-symbols.php -- --ugly -- /Users/nick/Code/PhpstormProjects/arcanist2/src/work/ArcanistWorkEngine.php' emitted text to stderr when none was expected: 0 COMMAND php -f /Users/nick/Code/PhpstormProjects/arcanist2/src/moduleutils/../../support/lib/extract-symbols.php -- --ugly -- /Users/nick/Code/PhpstormProjects/arcanist2/src/work/ArcanistWorkEngine.php
119 msPhutilLibraryTestCase::testEverythingImplemented
1 assertion(s) passed.
11 msPhutilLibraryTestCase::testMethodVisibility
1 assertion(s) passed.

Event Timeline

nib requested review of this revision.Feb 13 2025, 05:07

Thanks! I share a comment on the fly, maybe more comments soon after a test :)

src/work/ArcanistWorkEngine.php
242

Multi-byte support for emojis

aklapper added inline comments.
src/work/ArcanistWorkEngine.php
201

This should not be indented

aklapper requested changes to this revision.Feb 16 2025, 16:51

Setting Request Changes per the small review comments so far (as I am looking at https://we.phorge.it/differential/query/all/ and it helps to see correct statuses)

This revision now requires changes to proceed.Feb 16 2025, 16:51

Hi @nib - still thanks and you are welcome in updating this patch using something like this:

arc patch D25872
(edits)
arc diff

If you can't, we can eventually help. Thanks!

  • Fixes indents, adds emoji support, improves REGEX

I've tested this and I'm impressed. You probably have changed my life.

Since the original workflow was completely borked I supposed this is the minimal viable product. But note that running the command twice causes this:

STDERR
fatal: a branch named 'T15187-blabla' already exists
  1. Probably in this specific case it should just try to checkout there without creating the branch (?). I also don't think it should behave like arc patch that creates something different.
  1. Also, what should happen if the title changes a bit and if I run it again? Maybe it should detect tasks starting with Tsomething and use that?

What do you think about? Note that I'm still impressed and happy as-is lol

nib marked 2 inline comments as done.

Fixes error when invoking arc work 2nd time on task id

  • Handles existing branches
  • Fixes indents, adds emoji support, improves REGEX
  • Fix indents, adds emoji support, improve REGEX
  • Detect existing branch for task

Hopefully the latest change will work for you. Now it checks existing git branches matching "T12345*" and if any exist, it will git checkout the first one, else it will create it from the task name.
So that should allow you to change the task title, and to use arc work T12345 as many times as you want without error.

I've tested this and I'm impressed. You probably have changed my life.

Since the original workflow was completely borked I supposed this is the minimal viable product. But note that running the command twice causes this:

STDERR
fatal: a branch named 'T15187-blabla' already exists
  1. Probably in this specific case it should just try to checkout there without creating the branch (?). I also don't think it should behave like arc patch that creates something different.
  1. Also, what should happen if the title changes a bit and if I run it again? Maybe it should detect tasks starting with Tsomething and use that?

What do you think about? Note that I'm still impressed and happy as-is lol

@aklapper Would it make sense to move the branch-naming lines into a function in class ArcanistGitWorkEngine, since it the branch-naming is git-specific? E.g.

public function buildBranchName($task_ref) {
  $task_name = $task_ref->getName();
  // regex, preg_replace, trim, etc
  return $branch_name;
}
In D25872#23861, @nib wrote:

@aklapper Would it make sense to move the branch-naming lines into a function in class ArcanistGitWorkEngine, since it the branch-naming is git-specific? E.g.

public function buildBranchName($task_ref) {
  $task_name = $task_ref->getName();
  // regex, preg_replace, trim, etc
  return $branch_name;
}

Probably yes so it's easier with Subversion and Mercurial in the future (?)

Probably something with "for task" in the name like buildBranchNameForTask($task_ref)

src/work/ArcanistWorkEngine.php
201

And we can remove this newline <3 ihih

Handle arc work for new and existing git branches

  • Handle arc work for new and existing git branches
In D25872#23861, @nib wrote:

@aklapper Would it make sense to move the branch-naming lines into a function in class ArcanistGitWorkEngine, since it the branch-naming is git-specific? E.g.

public function buildBranchName($task_ref) {
  $task_name = $task_ref->getName();
  // regex, preg_replace, trim, etc
  return $branch_name;
}

Probably yes so it's easier with Subversion and Mercurial in the future (?)

Probably something with "for task" in the name like buildBranchNameForTask($task_ref)

I put all the changes into ArcanistWorkEngine.php for this diff. We're actually already using a version where the git-specific logic is moved into ArcanistGitWorkEngine.php.

I can make those changes as part of this diff if you prefer, or create a new diff specifically for moving the branch-naming logic. Let me know which approach you'd prefer.

FYI, I encountered some unit test errors when submitting this diff, but they appear to be related to PHP 8.4.4 deprecation warnings in Arcanist's codebase rather than issues with my changes.

  • arc lint
  • arc unit
  • fix a couple of id($api) and just use $api
valerio.bozzolan edited the test plan for this revision. (Show Details)

Tested for T15100 and T15993 and I'm a bit surprised to obtain, this very long branch with commas and square brackets (and just the closing bracket? uhm)

T15100-feature-request]-option-to-measure-wip-limits-based-on-card-count-instead-of-points,-to-more-closely-adhere-to-kanban-standards

and this with round brackets (that is not Bash friendly for the git autocomplete):

T15993-support-for-arc-work-t12345-(workontask-workflow)

Probably here we should normalize a bit more these branch names. To do it, I see that Phorge already has a "normalize" method that does great things, but unfortunately it's not available in Arcanist (we cannot call it, or we create a circular dependency)...

https://we.phorge.it/source/phorge/browse/master/src/infrastructure/util/PhabricatorSlug.php

What do you think about? Should we import a very minimal "ArcanistSlug" class? And now/then having PhabricatorSlug extending that? (to give 100% backward compatibility but uniform code)

I'm a bit unsure. Thanks for opinions.

P.S. Uh, I noticed that arc work also has a --start parameter! I documented a bit the expected behavior in the task T15993.

So that parameter is currently not supported for tasks, maybe relevant for the patch author:

$ arc work --start stable T15993
 NEW BRANCH  Creating new branch "T15993-support-for-arc-work-t12345-(workontask-workflow)" from "master".
 BRANCH  Checking out branch "T15993-support-for-arc-work-t12345-(workontask-workflow)".

So you see that from "master" that means we are not supporting --start, I guess.

If we cannot add support to that, we can add another cute TODO-exception for this corner case I guess (and create another task I guess)

Tested for T15100 and T15993 and I'm a bit surprised to obtain, this very long branch with commas and square brackets (and just the closing bracket? uhm)

T15100-feature-request]-option-to-measure-wip-limits-based-on-card-count-instead-of-points,-to-more-closely-adhere-to-kanban-standards

and this with round brackets (that is not Bash friendly for the git autocomplete):

T15993-support-for-arc-work-t12345-(workontask-workflow)

Probably here we should normalize a bit more these branch names. To do it, I see that Phorge already has a "normalize" method that does great things, but unfortunately it's not available in Arcanist (we cannot call it, or we create a circular dependency)...

https://we.phorge.it/source/phorge/browse/master/src/infrastructure/util/PhabricatorSlug.php

What do you think about? Should we import a very minimal "ArcanistSlug" class? And now/then having PhabricatorSlug extending that? (to give 100% backward compatibility but uniform code)

I'm a bit unsure. Thanks for opinions.

Thanks for the feedback.
You're right, the branch naming needs to produce shorter branch names and filter out more weird characters.
I think the branch name just needs to make it clear to us humans what the task is about. So, after the T12345-, just a few dash-separated words from the beginning of the task name, minus "weird" characters, is what I'd like to see.

But I didn't know if that might trample on other people's use cases. Maybe some people *want* emojis and other weird stuff in their branch names?
So that's why I used the most permissive branch-naming rules allowed by git/github.
(git rules: https://git-scm.com/docs/git-check-ref-format)
(Those rules allow "]" but not "[", hence the weird result you saw in your test.)

Also, I agree the max length allowed is too long. Perhaps reduce from 244 bytes to 64?

PhabricatorSlug::normalize() seems to replace dots with "dot", and adds a "/" to the end, so I don't think it's good for cleaning branch names.

I think the easiest 2 options would be:

1.) Simplify regex to limit branch names to: 0-9a-z and "-" (and perhaps a few other characters)

2.) Keep the existing pattern I created to ensure git/Github compatibility forever and ever. Then add another preg_replace() to eliminate some other weird characters, and truncate length.

Current regex:
$pattern = '#(^[./]+|//|/\.+|\.{2,}|@{|[/.]+$|^@$|[~^:\x00-\x20\x7F?*\[\\\\])#u'; ('\\\\' represents '\\' in the regex, because of PHP string escaping)
playground: https://regex101.com/r/OyzRPj/1

P.S. Uh, I noticed that arc work also has a --start parameter! I documented a bit the expected behavior in the task T15993.

So that parameter is currently not supported for tasks, maybe relevant for the patch author:

$ arc work --start stable T15993
 NEW BRANCH  Creating new branch "T15993-support-for-arc-work-t12345-(workontask-workflow)" from "master".
 BRANCH  Checking out branch "T15993-support-for-arc-work-t12345-(workontask-workflow)".

So you see that from "master" that means we are not supporting --start, I guess.

If we cannot add support to that, we can add another cute TODO-exception for this corner case I guess (and create another task I guess)

I just took a look and it seems doable. There's a getStartArgument() method I can use. I'll look at it more tomorrow.

P.S. Uh, I noticed that arc work also has a --start parameter! I documented a bit the expected behavior in the task T15993.

So that parameter is currently not supported for tasks, maybe relevant for the patch author:

$ arc work --start stable T15993
 NEW BRANCH  Creating new branch "T15993-support-for-arc-work-t12345-(workontask-workflow)" from "master".
 BRANCH  Checking out branch "T15993-support-for-arc-work-t12345-(workontask-workflow)".

So you see that from "master" that means we are not supporting --start, I guess.

If we cannot add support to that, we can add another cute TODO-exception for this corner case I guess (and create another task I guess)

I just tested the sequence in your edit to T15993 and made it work with this:

$start = $this->getStartArgument()
      ?? id($api)->getBranchName();

It uses the --start argument if it exists, otherwise it uses the current branch.

Seems nice! Feel free to update.

P.S. As already said you can avoid id($api)->getBranchName() since it's OK to use just $api->getBranchName().

(The id() trick is only useful when you are creating a new object from a class, e.g. id(new Class())->something(), since creating objects on-the-fly may create some syntax problems in old PHP versions)