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.

I currently intend to move away from staging areas by default (see T13426). Although I can imagine a future mode where staging areas support "push to branch" vs "push to tag" vs "push to ref" configuration, this world would be on the far side of T13426. There are major implications around this decision (tag vs branch vs ref) in terms of client behavior and third-party build system behavior, particularly when the staging area is a "real" repository.

Since "saved states" can plausibly implement their own LFS logic, they will likely moot this.

(Earlier, see also https://github.com/git-lfs/git-lfs/issues/1069 and https://github.com/git-lfs/git-lfs/pull/1085, although neither of these help much.)

I think a possible simpler fix here (at least, in terms of disruption to existing behavior) is:

• Detect the repository is LFS.
• Push to the base tag normally, but with GIT_LFS_SKIP_SMUDGE=1.
• Push the base commit to the diff tag with GIT_LFS_SKIP_SMUDGE=1.
• Fast-forward push the tip commit to the diff tag without modifying LFS behavior.

Then, LFS should only be invoked on the third push, and the hook should have sensible old and new ref hashes, I think? If you can get a prototype of this approach working, I'm likely comfortable bringing it upstream, at least as a stopgap.

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

$ GIT_LFS_SKIP_SMUDGE=1 git push -- <remote-uri> master:refs/tags/phabricator/base/123 master:refs/tags/phabricator/diff/123
$ git push -- <remote-uri> HEAD:refs/tags/phabricator/diff/123

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?

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.

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:

$ git push -- ssh://epriestley@local.phacility.com/source/spellbook.git HEAD:refs/tags/lfs/$RANDOM
# Request received by "local.phacility.net", forwarding to cluster host "local.phacility.net".
# Acquiring write lock for repository "spellbook"...
# Acquired write lock immediately.
# Acquiring read lock for repository "spellbook" on device "local.phacility.net"...
# Acquired read lock immediately.
# Device "local.phacility.net" is already a cluster leader and does not need to be synchronized.
# Ready to receive on cluster host "local.phacility.net".
Uploading LFS objects: 100% (1/1), 1.0 MB | 0 B/s, done.
Total 0 (delta 0), reused 0 (delta 0)
# Released cluster write lock.
To ssh://local.phacility.com/source/spellbook.git
 * [new tag]         HEAD -> lfs/25761

$ git push --no-verify -- ssh://epriestley@local.phacility.com/source/spellbook.git HEAD:refs/tags/lfs/$RANDOM
# Request received by "local.phacility.net", forwarding to cluster host "local.phacility.net".
# Acquiring write lock for repository "spellbook"...
# Acquired write lock immediately.
# Acquiring read lock for repository "spellbook" on device "local.phacility.net"...
# Acquired read lock immediately.
# Device "local.phacility.net" is already a cluster leader and does not need to be synchronized.
# Ready to receive on cluster host "local.phacility.net".
Total 0 (delta 0), reused 0 (delta 0)
# Released cluster write lock.
To ssh://local.phacility.com/source/spellbook.git
 * [new tag]         HEAD -> lfs/4639

This is more broad than a specific LFS-disabling switch and "breaks" other pre-push hooks, but I'm not aware of any kind of pre-push hook in the wild which should be invoked for staging areas, so I'm comfortable upstreaming this change and dealing with the fallout when it happens if some install is doing something particularly creative.

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?

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?

I think this behavior is probably desirable and likely a net improvement for LFS users even if you're the only ones experiencing the original issue (excessive LFS transfer), but if we're fixing a bug with your secret fork I am VERY GRUMPY.

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 :)

I'll address the code review today.

For my own internal reference, PHI1234 has related discussion.

(Also, hello after all this time! Hope you're all doing well over at phacility.)

Recently I have noticed this:

Instead, this please:

tyvm

If you believe you've found a bug in Phabricator, please report it on Discourse, including complete reproduction instructions.