The function name including a negative, “nonempty”, throws me off…

In D25535#15480, @avivey wrote:

Maybe this is wild, but: should we consider removing the compiled xhpast.exe file from the repository, and host is externally?

Ty. I’m still confused how “limit” is required but doesn’t throw an error when not specified. Can address that later if needed.

😆

This might regress the case where file is “0”

I don't know what an "hardpoint" is.

❤️

ty

Great, I appreciate your thoroughness here!

In D25341#14044, @matmarex wrote:

I had no idea that I am supposed to "land" these changes. Surely it should be the responsibility of the maintainers to decide into which release a change will go and when?

Anyway, I have been waiting for several months for someone to stop ignoring me and accept my patch, until @aklapper explained it to me… This seems like an awful workflow.

seems good

I take it the api used by Phorge hasn’t updated or changed in the update?

I didn’t find changes in our fork regarding this. We should either change these parameters to be optional in their definition or throw exceptions when they aren’t specified. Changing them to be optional seems the better behavior.

Not having a title seems j like a bug elsewhere

Thank you!

Ah I see below it assumes string. Thanks!

Is it possible tasks isn’t a string in this case, where non_empty_scalar might be more of a match?

seems good, ty!

Oh I see you did confirm it’s fixed upstream. Should we just update the version we’re using?

Since this is a third-party dependency can we check if this was fixed upstream, and that an update to phpmailer would fix this?

Swear I’ve come across this before… it might be fixed in our forked branch so I’ll take a look. Does $limit also need same treatment?

In D25671#18360, @speck wrote:

Agree the current non-serious language needs updated.

Agree the current non-serious language needs updated. Maybe something like,

Ah, yes thank you for the video. I should have clarified but was asking for screenies of the result after this change. The changes look reasonable but I’m not a regular css user

screenies?

Should the conduit handler also check this and throw an exception? Doing so will send back a more informative error to the caller. I did some of this in https://secure.phabricator.com/D21872

Thank you for testing. I think this looks good. In the one case that the old var is used it’s rendered as a string in pht so string assumption seems okay here.

One by one

thank you!

yarp

I don’t believe this style of doc annotation is used elsewhere. What’s the purpose of this?

Seems good

Are there mechanisms to re-gen the doc after we land changes? If not that might be something we should be able to do with harnormaster, and we can bring the docs back after something like that is set up to ensure they’re always updated

The change looks reasonable though I’m not familiar at this level of the database schema design

Ah yep, GET with body will get flagged in a number of cases.

nice, tyty

I prefer to invert the logic and return early, to avoid having the entire function contents inside a nested level - up to you but this looks good.

yep!

I’m not very familiar with this area of code but this sounds appropriate

Is it possible to check if anything is relying on this not working with negative numbers?

whoops

A mermaid-js one could be written based on the old graphviz one:

I haven’t run this specific issue but I have seen other oddities when using my installed arc within the arc or phorge repos. I’m unable to get it to work unless I use the arc that exists along side that phorge repo. That might be similar here.

Hahaha I’m just now seeing this. Thank you for putting together the presentation and giving the talk. I enjoy seeing your excitement!

I configure my browsers to increase the default font size because my eyes can’t handle the small fonts. Most pages tend to size appropriately but there are some oddities, including Phorge. I’ll grab some screen caps when I’m back on workstation.

Both of these failures are edge cases so it does seem likely we wouldn't have found them as quickly. I do still wonder how many Windows users we have.

The ArcanistBundleTestCase failures are due to diff binary not being present on Windows systems. The tests could be rewritten to utilize git diff instead but would take some effort.

Fix some lints about line length

Fix use of ExecFuture to pass a scalar string as first argument instead of variable.

I managed to compile xhpast on windows using llvm installed via Scoop. I also have Visual Studio C++ 19 installed which may be required (I saw some suspicious output that indicated that some include files were being pulled from that install somehow).

I wonder how many Windows users we have. The xhpast linter does not work, and the unit test failures listed here are pre-existing and occur without these changes.

Almost seems a little too easy

I’m not sure about Permalink being a top-level button. Maybe under the View Options menu?

I believe it was originally made for Phacility to accept donations and then process payments for paid support, neither of which we want at Phorge, and I doubt is used elsewhere. Removing seems fine to me.

Is it assumed that using the site will automatically license the user’s content under these, or should there be a line for that?

Thanks!

@bekay while in this area of code would you have any pointers on these other issues I’d like to fix at some point?

1. File browse widget doesn’t work unless logged in, for publicly accessible repos
2. The search results pop up width is often too narrow to show the full results path, and truncates the end of the path instead of beginning.

Looks good! Thank you so much for this!

Thanks - I did mean as an inline comment in the code but either is good.

Is the input to parseDiff guaranteed to be UTF-8 encoded? I don’t have the code on-hand to look up but it’s possible encoding is handled elsewhere — or arc might have a hard requirement for all input to be UTF-8. All mercurial commands run by arcanist are done with a flag to enforce UTF-8.

Hmm I’ve used mercurial and arcanist/Phab for years at my company and don’t believe we’ve ever run into this. Any idea what’s causing the presence of the BOM? We’ll apply a change to handle the UTF-8 bom but I am curious what may have caused it to show up. Is your hgrc configured in some way for this or maybe an environment variable?