add small PHPDoc related documentation to clarify further reviews

Thanks for this patch (again)

Tested also this one, thanks

Tested locally, verified that the domain is never a PhutilURI. No implosions.

Thanks for this patch

Thanks again

Premising I 100% agree in the removal of version suffix and I would +1 just that.

This discussion (and disagreement) is exactly the reason why we need a CLA ASAP.
While consulting with a relevant legal counsel.

Just some probably interesting notes :D

Thank you speck, very interesting.

Hi @aklapper. Are you affected by this as well?

@ton yep thanks, really! that will be handled here:

Feel free to reopen to share any doubt about this method

What are the reasons for doing commit message rendering on the server side?

In an ideal world the solution should be as simple as having a template somewhere in {{repo_root}}/.arcanist/commit_template.j2 that arcanist injects values in and renders.

I was similarly looking at lua for the same reason as it's used by neovim 😆 . There are a few libraries for lua in rust but the one that seems recently active is mlua, which doesn't implement lua execution in rust but binds to lua binary/library. If we go the route of binding to another library/binary then our build/package would have to include the lua library file too (though it may be possible to embed it?). Something like Rhai (or there's also Rune and a few others), are implemented fully in Rust so there's less to worry about during the build/package process. I wasn't sold one way or the other but wanted to check out Lua along with some of the others to see how well they turn out.

arc patch problem is likely fixed. But I encountered similar problem here
https://we.phorge.it/T15351#8287

This seems to have fixed the problem. But let me try a bit more Diffs to be extra sure

I see what you are saying. I quite like Lua and how neovim uses it to achieve this extensibility...

I’m fully aware of the friction that comes up trying to get developers using arcanist, the difficulties with PHP, the complexity in not understanding what’s going on, etc. and also familiar with the benefits of a compiled binary solution.

@speck the whole point of an alternative phab client implementation is remove friction that comes with arcanist. Namely:

Hah this has been something spinning in the back of my mind for a while. One thing that will be difficult to support in something like Rust (not sure about Go) is that the PHP arcanist allows for very easily extending by dropping in your own PHP files to the extensions folder.

We can probably come up with a way to allow an extension to customize the "render template" implementation without forking (e.g., just add a hook in differential.getcommitmessage), but it would have to be very carful about the whole form-thing.
Maybe just adding an "for landing" flag in differential.getcommitmessage (or adding a different method) would solve this problem...

In T15364#8281, @ton wrote:

The template is actually generated server side, not in arcanist, so it just a matter of getting the BSD Phorge server to have the right opinions.
Some of the sections can be configured already

Please point me in the right place in the code where I can find the template

In upstream phabricator I worked with Evan to update the Mercurial API in a way that all mercurial commands/futures are executed through a common path and allow making modifications to only those executions. For this we should look at something similar so that we're only passing $HOME to git commands and not to others.

The template is actually generated server side, not in arcanist, so it just a matter of getting the BSD Phorge server to have the right opinions.
Some of the sections can be configured already

re @avivey : arc land --hold sort of works but look at the ergonomics:

From https://drewdevault.com/2018/10/05/Dont-sign-a-CLA.html

Many people, myself included, contribute to open source projects under the assumption that my contributions will help serve a project which continues to be open source in perpetuity, and a CLA provides a means for the project maintainers to circumvent that.

Assumption here is key. The entire purpose of a CLA is to remove this assumption and clarify how contributions are managed. This post and disposition is based entirely around the organization receiving contributions will behave in an adversarial manner in the future. If the organization doesn't earn your trust then why trust them with your contributions?

Set default empty strings for $content_type and $user_agent, as requested by valerio.bozzolan

I tested this patch locally with some fuzzy tests. Obviously without issues since this is an easy-peasy case.

Alternatively - there's also arc land --hold, which does almost everything except for the git push, which would allow the user to update the commit message. Have anyone tried that?

thanks @ton - these look very actionable:

By the way it happened again here :D

In T15249#8232, @ton wrote:

@avivey today I tried arc patch to download a bunch of Diffs.

Some diffs check out OK. Lots of Diffs fail to be fetched, I get a very useless error message:

Exception
strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated
(Run with `--trace` for a full exception trace.)

Hi @ton can I ask you if, manually patching Arcanist with the indicated diff in the Task description, fixes for you?

more details about arc land - https://we.phorge.it/T15364

In T15121#8063, @avivey wrote:

Any contribution to Phorge is implicitly covered by Apache 2.0, which allows re-licensing by anyone.
The CLA is only making this explicit.

Thanks @avivey !