Also note that we have a similar PhabricatorDataNotAttachedException in PhabricatorRepositoryCommit (via getRepository()) in downstream https://phabricator.wikimedia.org/T360714. It's without reproduction steps but sounds a bit similar.

In D25118#14541, @speck wrote:

I forgot I had this requesting changes. Any idea of the performance hit in markup rendering now that PhutilURI is constructed for every link? I think its constructor does a fair amount.

@avivey to do now:

The repo is set to "host" right now, but it can also be set to "observe from" or "mirror to" another repo.

Sorry, I'm not explaining it very well. For the database engine to use index (col1, col2, col3), col1 must be specified somewhere in the WHERE clause, but it's precise location is irrelevant - the database engine will work it out. As to why we need both indexes, there's a good explanation on https://use-the-index-luke.com/sql/where-clause/the-equals-operator/concatenated-keys

Interesting, and a little troubling if I understand it correctly; It means that the query ... WHERE dst = $a and type = $b and src = $c will be a full-table-read, but the equivalent query src = $c and type = $b and dst = $a will use the index to resolve quickly?

minor optimization and unit tests are still happy

@avivey - the order of the keys is irrelevant when it comes to determining uniqueness, but extremely important in other respects. In order for an index to be used when selecting rows, the database must be provided with values for the index fields, in the order specified. Ie you always need to specify a value for the first field, and can get away without successive ones.

Are the keys actually different? Looks like they have the same fields in different order. Does this makes a difference in the implementation?

@valerio.bozzolan agreed - I'm not proposing to remove the (dst, type, src) index, just the uniqueness constraint on that index. Sorry for not making that clear.

But in any case, it may have sense to keep a generic index on (dst, type, src) (or maybe just dst, type) in that specific order, since the UNIQUE KEY was probably a "micro-optimization" to achieve such index for queries operating on dst, or also dst and type, but all without src. This is just an opinion from an alien. Better to wait for more answers.

Your email address(es) set by you for your user account in the Phorge Project

That is private data. Nobody can access it.

I'm pretty sure getValueForQuery() should return some value?

I have a problem with this statement:

Would anyone give this another review? Might make sense to apply the one-liner in D25527 to have a simple test case on the Maniphest Advanced Search page

On the other hand, as the variable is called $html_details let's go for phutil_implode_html() to be super-safe? ping @roberto.urbani

CVE-2017-5223, CVE-2018-19296 and CVE-2020-36326:

CVE-2021-34551:
This one requires passing user-provided input as a filename to the "setLanguage" method; We don't call that method.

First pass, these one do not apply to us (and some of them do not apply to anyone at all):

(I've put a note in Dependencies for now)

add inline documentation because we can

In D25546#15665, @valerio.bozzolan wrote:

I also don't get if in this part we have access to any thing related to Maniphest, ManiphestEditEngine or this kind of stuff, to just call $something->getCommentPanelPlaceholder() and have the business logic there in the specific application itself.

Let's split the problems:

Updated Pygments with pip to 2.17.2 und cut the time in half. So yeah, that's already an improvement.

I guess we could also try to cache the individual rendered code-blocks.

I assume we should change strpos("\0", $value) to strpos($value, chr(0))

(and I made sure this new function isn't exported to the arc lib namespace)

extract to function

Well, good to know that it is not something in the phorge codebase. Our server has pygmentize 2.14.0 - but the server itself is really not the best, so that could be an explanation. Maybe we could make some remarks about performance and keeping the versions fresh on the diviner page...

There is still that minor inline comment here, maybe interesting

Yeah, it's probably not impossible to so safely - especially since people has been working on it for over a decade - but it's hard to do right. I know Wikipedia allows user-uploaded SVGs in some way, so maybe this is a solved problem.

I fixed arc test-traits in R12 (45f900a587).

@avivey I have no idea if they are actually any good but PHP based SVG sanitizers like svg-sanitizer exist. I also noticed this Rust based library made by cloudflare svg-hush.

lol

Update: I've installed pygments (2.15.1), and it took about 3 seconds to render. It takes about 14 seconds here (with pygments 2.3.1).

mm, dumping this file in my dev env renders pretty much immediately; that's a good sign that it's the code blocks, because (1) pygments is known to be slow and (2) I don't have it installed.

Ah, I see - that makes sense. Thank you for the detailed response.

I'm quite sure the problem is limited on repeated code blocks.

"slow remarkup" often boils down to 1-2 inefficient regexp in a rule somewhere.

Can reproduce

In T15670#16064, @valerio.bozzolan wrote:

A root problem is that highlighted line number(s) should be a # fragment really, to do not multiply pages exponentially.

Apparently PhutilSortVector.php is somehow recent, 2016, without any comment about that strpos("\0", $value) that is probably not correct

I also don't understand strpos("\0", $value) since the documentation says that the second argument is the needle

In D25051#15888, @valerio.bozzolan wrote:

I'm a little worried about this migration; Do we have garbage collector for this? it would be safer to let it run.

You are concerned for performance reasons because it doesn't limit the results. Right?