Thanks for the code review and archeology! I did some archeology on our end and indeed, it was removed on Sep 26, 2018 when we forked this diff workflow for exactly the reason in this diff. With the --no-verify the LFS objects were not being pushed to our staging repo, so our Jenkins jobs wouldn't have them and would fail. So I do think this the right fix :)

Hmm, I can't immediately find any upstream version of arc that has ever not had --no-verify: D13020 in 2015 had it, and it was version-checked but retained in D14033 (also in 2015) and seems untouched since then. If you're working with a fork, maybe blame/log to figure out why it was removed?

• oops wrong way

Nice find! I see there is already a --no-verify in this upstream version but we didn't have it in ours... maybe because of the lfs version? So maybe this diff is actually improving the life of lfs folks?

• go with --no-verify

Instead of GIT_LFS_SKIP_SMUDGE=1, try git push --no-verify -- <remote-uri> ...? That just skips the pre-push hook, which seems to work locally:

• Revert "only run fetch if git for-each-ref is empty"
• Revert "fix spelling"
• Revert "handle the case where you need to remove it before adding it"
• Revert "Use a named remote and branches for staging to help git-lfs"
• try the double push thing for lfs repos

As much as possible I'd like arc to have the property that interrupting/killing/aborting/striking-it-with-lightning at any point during execution leaves you in the least-surprising state we can leave you in, and "arc uninstalled LFS as a side effect" seems highly-surprising, so I'm hesitant to go down that path. Let me take a quick look and see if I can find some other way to trick LFS here -- maybe there's a clever --config flag or something like that.

Thanks Evan! I tried what you suggested but I think GIT_LFS_SKIP_SMUDGE=1 is only for pulling not pushing, since the first push in your suggestion is still uploading to LFS and their code doesn't seem to refer to that env var https://github.com/git-lfs/git-lfs/blob/5f969e6a3e48ecee48b6235adccc42a644e82101/commands/command_push.go, I can't really see any way to disable lfs other than git lfs uninstall; git push; git lfs install. Is that nuts?

That is, the sequence would look something like this, if the description above was muddy:

Part of this change -- pushing to a branch rather than a tag -- breaks a million things and can't come upstream without significant changes elsewhere.

• only run fetch if git for-each-ref is empty

• fix spelling

• handle the case where you need to remove it before adding it

try again with the remote removed from the failure before

try again now that I put in my ssh key

It's true that it has been 12 years, but I'm sure fwrite() will start returning an error code when it encounters a permanent, fatal EPIPE error condition soon.

The issue doesn't appear in the current code base with Firefox 73. That's not unexpected, as the UI has been tweaked several times between 2015 and now.

We also have two separate pieces of ngram extraction code:

For now, I'm going to change the ngram slicing to be character-oriented. This should never be worse than the current behavior, and moves us closer to effective normalization.

This appears to be the unicode normalization chart:

• Fix pattern

If you're affected by this and arc upgrade doesn't work, the easiest fix is to run git pull in arcanist/.

Does using --max-pack-size to reduce the maximum packfile size really let Git "checkpoint" after each packfile, so the process is effectively resumable?

PHI1655 identifies a specific case where enormous packfiles may create problems:

I have some code which runs and looks plausible (i.e., not covered in piles of callback garbage), at least:

See https://discourse.phabricator-community.org/t/data-truncated-when-pushing-into-repository/3586/ for what is likely to be a related issue.

The real root cause of this issue may have been a locale setting which uses comma as a decimal separator, see T7339.

@epriestley As perhaps an example of another use case, in my workplace we needed, amongst others, the following lists of Maniphest tasks:

1. assigned to the viewer, regardless of the author,
2. authored by the viewer, without self-assigned,
3. subscribed by the viewer, without (1) and (2).

(1) is obviously trivial (there's already a built-in query), but the other two seemed to require custom "NOT" filtering. Of course, (2) can be solved to an extent by using "Group by assigned", but it's not very convenient. Because I tend to self-assign most of the tasks that I create, the ones that I have assigned to other people quickly get visually overwhelmed by the large group of "Assigned to kerberizer" (not to mention that with my username this huge group tends to get in the middle of the list).

Here's an actual example of loadHardpoints($objects, $hardpoint):

Perhaps another approach that could be considered, doing the colorizing in the browser using something like highlight.js (https://highlightjs.org/). This, of course, puts the CPU burden on the end user instead of the server.

In the specific case of the Hardpoints, we currently often have code which loads objects but doesn't do anything with them. For example, most Query classes use didFilterResults() to fill things-that-sure-look-like-hardpoints, but few do anything with the results.

I'm going to mark this as resolved, since:

• Fix a unit test issue where a future was explicitly started before being added to an Iterator.

• Also remove uncalled "checkException()" method.

The core idea in D5104 + D5105 is that $future->resolve() and id(new FutureIterator(array($future)))->next() (like, roughly) execute meaningfully different code paths.

Somewhere in experimental or wilds, I introduced ArcanistConduitEngine. This has some weird fake future stuff going on, so this is probably now ripe.

Thanks, see D21029.

Couple of notes on the state of affairs here: