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

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!

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.

2. 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

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.

In D25872#23808, @valerio.bozzolan wrote:

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.

2. 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)

In D25872#23862, @valerio.bozzolan wrote:

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.

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)

In D25872#27178, @valerio.bozzolan wrote:

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

In D25872#27180, @valerio.bozzolan wrote:

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.

In D25872#27180, @valerio.bozzolan wrote:

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)