I'm not an important stakeholder, but I would like to share that in my installation https://gitpull.it I would like to have commits indexed as default as it happened as default and as it happens in GitHub and GitLab. So I'm now sincerely trying to understand how to restore the old behavior without keeping my own fork of Phorge if needed.

Thanks for the deep inspection! Tested and I agree

• Remove extra <tt> in bulk job dialog

In D25473#13673, @valerio.bozzolan wrote:

So this is the only blocking Test that is failing:

Visit /maniphest/, shift+click on at least 1 Task, click on Bulk Edit Selected, Continue, see the popup

If someone strongly feels that I should revert, please say so - thanks! :)

Valerio: Uhm, I'm sorry, I had not seen your comment here before I landed the patch (as I had checked my Differential page instead of my notifications).

Side note, we are introducing the possibility to share this kind of very creative and confusing URLs (that are safe from the point of view of XSS but) that could be an attracting point for lamers to generate confusing user messages inside Phorge itself, like:

Thanks for the additional debugging and finding steps to reproduce! In this case, I'd prefer not to log the issue and to only show a message to the user explaining what's the issue, including their requested bogus encoding.

Heh, very good catch! I can confirm, yes.

Thanks for landing!

This is something that may be not appreciated by some people.

Happy you fixed!

Thanks for the deeper investigation! I can confirm that with the last version and following my original steps to reproduce, no stuck daemon task about the mock looping due to an exception on http://phorge.localhost/daemon/ anymore, so I'm going to land this.

One of my favourite updates in my workplace was to store counts of feature usage per month in a table. Move forward 3 years, and whenever we need to update some code in a feature, we check for it in the table, and if it's not been used in 3 years we just scrap the feature.

Skip lint (javelinsymbols)

Great suggestions

• Implemented celerity_generate_unique_node_id()
• Implemented .collapsible-content > ul.remarkup-list CSS

Loved the comment, as otherwise I wouldn't have understood what was going on in the CSS. I've reworded it - hope that's OK.

JavaScript: ✅ I damn like your proposal and I think it is very OK, non-plus-ultra

I am quite happy with this now. Spent way too long on it, so I hope you like it too!

• Refactored the PHP code to build the collapsible button & div into buildCollapsibleIndex()
• Define the javelin call config first, then refer to it - saves defining the variables independently!
• Create an icon field in the button, allowing for open and close icons to be set
• Make both the icon and the text optional in the collapsible javascript - if you just want an icon, fine. Just text, also fine. Neither - sure!

So this is the only blocking Test that is failing:

We have just a minor issue but with a potentially working fix, and it's ready to land

I don't know. Let's see :)

The problem there is that by requiring collapsiblejx to have knowledge of the 'collapsible-toggler-label', we introduce tight coupling to it's caller. (see how adding 'jx' on to the end of collapsiblejx has made it uniquely identifiable?)
A better solution would be to have the collapsiblejx config contain 'btn_open_icon' and 'btn_close_icon' icons. I'll have a go...

avoid micro-optimizations that make this unreadable

• move duplicated button text into a variable
• improve formatting

• Renamed button/div IDs to remove spaces
• Renamed diffusion-collapsible-container to collapsiblejx
• Added margin-top: 10px; in the collapsible content - this overrides the higher up margin-top: 0px
• Moved webroot/rsrc/js/application/diffusion/collapsible-container.js to webroot/rsrc/js/core/behavior-collapsiblejx.js
• Renamed the JX from diffusion-collapsible-container to collapsiblejx - the single word collapsible was a bit too common if you 'git grep' for it.

As already noted we were affected by this margin-top: 0; that was making the TOC a bit too much attached to the button:

• sorry if I introduced an ID for just the label: now replace with a non-unique Javelin Sigil
• tried to fix the margin-top thing

I can also propose another small change to adopt a sigil for the button label, so we don't need an ID.

We need more help on this. Never approved anything similar, I don't have enough experience.

Tried to add a lovely icon but still keeping the label toggling and the whole button selectable

Renamed butt to btn as per PR. Much more common abbrieviation!

Minor, just suggestion

Happy to take your suggestions with a couple of changes

• Remove index from the config labels - we've made collapsible-container.js generic, and not just for an index.
• The reason for the if/else block was to enable long variable/attribute names so as to have self-documenting code. I've done away with it, but we do need a comment instead so as to show why we associate the 'push this to close' text with the open state and vice-versa.

I'm a bit confused (!) I thought there were no PHP changes on Rev 1414 and I approved it, but instead I now see that we have again PHP changes. It's unclear to me whenever the Diff I approved was "updated to reflect changes", or maybe I'm just totally crazy because of real life. Please share your opinion.

• Use dynamically generated platform path where possible

Hoping to be useful we can self-document this joining this group:

(This was already approved but I wait for another "Yeah land" since I changed a thing)

To fix this error:

Btw, there are some similar commands in src/applications/config/check/PhabricatorManualActivitySetupCheck.php. Should I change them as well? How can I "break Phorge" to see this page rendered?

I think there’s still a chunk to do here, including additional documentation during setup

• Actually the path is not needed here.

• Fix missing arguments to hsprintf; re-add csprintf to string using the %R conversion

• Improve prompts: use hsprintf instead of cprintf, add missing <tt> markers, simplify syntax

For future reference, I went digging for the difference between csprintf() and hsprintf(), and found the following:

This might be a duplicate of T15636

As an aside, what do you think of https://github.com/campbsb/example-phorge-php-project - should we put a selection of example projects on https://we.phorge.it, either as individual repos, or one repo containing a bunch of examples, or somewhere in the arcanist repo ?