Page MenuHomePhabricator

Use a named remote and branches for staging to help git-lfs
ClosedPublic

Authored by ptarjan on Mar 19 2020, 6:29 PM.

Details

Summary

I'm working on a fairly large monorepo using phabricator and we are
running into a git-lfs issue. Every single arc diff we have to wait
many seconds while all our lfs files are walked before the push goes through.

I chatted with the git-lfs folks in
https://github.com/git-lfs/git-lfs/issues/4076 and they suggested this workflow
change. I tested it on our repo and it does indeed seem to fix the issue. Three
things were change:

  1. The remote url is named
  2. Use branches instead of tags
  3. Do a git fetch

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

Test Plan

Before

$ arc diff
Linting...
 LINT OKAY  No lint problems.
Running unit tests...
 UNIT OKAY  No unit test failures.
 PUSH STAGING  Pushing changes to staging area...
Uploading LFS objects: 100% (8211/8211), 1.0 GB | 0 B/s, done.
Enumerating objects: 31, done.
Counting objects: 100% (31/31), done.
Delta compression using up to 12 threads
Compressing objects: 100% (21/21), done.
Writing objects: 100% (24/24), 1.77 KiB | 8.00 KiB/s, done.
Total 24 (delta 19), reused 0 (delta 0)
remote: Resolving deltas: 100% (19/19), completed with 6 local objects.
To github.com:robinhoodmarkets/web-staging.git
 * [new tag]             2581578b59b6341c1023377fd8741814e89fcd0c -> phabricator/base/427095
 * [new tag]             7d9be50fbca590f15742d8cc20edd4f082dfbb3b -> phabricator/diff/427095
Updated an existing Differential revision:
        Revision URI: https://phabricator.robinhood.com/D135442

Included changes:
  M       __shared/tools/arcanist-js/src/workflow/RHArcanistDiffWorkflow.php

After

$ arc diff
Linting...
 LINT OKAY  No lint problems.
Running unit tests...
 UNIT OKAY  No unit test failures.
 PUSH STAGING  Pushing changes to staging area...
Enumerating objects: 15, done.
Counting objects: 100% (15/15), done.
Delta compression using up to 12 threads
Compressing objects: 100% (7/7), done.
Writing objects: 100% (8/8), 638 bytes | 2.00 KiB/s, done.
Total 8 (delta 6), reused 0 (delta 0)
remote: Resolving deltas: 100% (6/6), completed with 6 local objects.
To github.com:robinhoodmarkets/web-staging.git
 * [new branch]          2581578b59b6341c1023377fd8741814e89fcd0c -> phabricator/base/427105
 * [new branch]          274cc3a25eb7856291c1d4c078b28b76af8a43b2 -> phabricator/diff/427105
Updated an existing Differential revision:
        Revision URI: https://phabricator.robinhood.com/D135442

Included changes:
  M       __shared/tools/arcanist-js/src/workflow/RHArcanistDiffWorkflow.php

(notice the lack of Uploading LFS objects)

Diff Detail

Repository
rARC Arcanist
Branch
master
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 23940
Build 32948: arc lint + arc unit

Event Timeline

ptarjan created this revision.Mar 19 2020, 6:29 PM
ptarjan requested review of this revision.Mar 19 2020, 6:29 PM
ptarjan updated this revision to Diff 50118.Mar 19 2020, 6:33 PM

try again now that I put in my ssh key

ptarjan updated this revision to Diff 50119.EditedMar 19 2020, 6:33 PM

try again with the remote removed from the failure before

ptarjan updated this revision to Diff 50120.Mar 19 2020, 6:37 PM
  • handle the case where you need to remove it before adding it
ptarjan updated this revision to Diff 50121.Mar 19 2020, 6:41 PM
  • fix spelling
ptarjan updated this revision to Diff 50122.Mar 19 2020, 9:59 PM
  • only run fetch if git for-each-ref is empty

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.

ptarjan updated this revision to Diff 50125.Mar 20 2020, 12:42 AM
  • 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
epriestley added a comment.EditedMar 20 2020, 12:48 AM

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:

Push Without --no-verify, Transfers LFS Objects
$ 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
Push With --no-verify, No LFS Transfer
$ 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.

ptarjan updated this revision to Diff 50126.Mar 20 2020, 12:59 AM
  • go with --no-verify

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?

epriestley added inline comments.Mar 20 2020, 1:00 AM
src/workflow/ArcanistDiffWorkflow.php
2904

Slightly more conventional if written as:

list($stdout) = execx(
  'git ls-files -- %s',
  ':(attr:filter=lfs)');

This is effectively the same, but:

  1. Throws if git ls-files fails, rather than continuing silently, which is generally desirable for consistency with how everything else works.
  2. Invokes escaping logic, which is probably always "add single quotes" but may, e.g., differ on Windows.

I've also added -- as a prayer on the altar of T13491, but this appears to be pointless because Git always treats these patterns as patterns, even when a file with the same name exists on disk:

epriestley@orbital ~/scratch/spellbook $ git ls-files ':(attr:filter=lfs)'
1M.blob
epriestley@orbital ~/scratch/spellbook $ git ls-files ':(attr:filter=lfs)' --
1M.blob
epriestley@orbital ~/scratch/spellbook $ git ls-files -- ':(attr:filter=lfs)'
1M.blob
epriestley@orbital ~/scratch/spellbook $ ls -alh ':(attr:filter=lfs)'
-rw-r--r--  1 epriestley  staff     0B Mar 19 17:54 :(attr:filter=lfs)

Some day far in the future we might figure out how to specify inputs to computer programs without ambiguity.

ptarjan updated this revision to Diff 50127.Mar 20 2020, 1:01 AM
  • oops wrong way

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.

src/workflow/ArcanistDiffWorkflow.php
2981

Exactly as written, this list will still include --no-verify (and thus not push LFS objects), I think?

epriestley added inline comments.Mar 20 2020, 1:10 AM
src/workflow/ArcanistDiffWorkflow.php
2905

You should test the no-LFS-files case to make sure, but this probably (?) needs to be !empty(trim($stdout)).

As written, in a non-LFS repository:

  • $stdout will be a single newline.
  • empty("\n") is false.
  • !empty("\n") is true.
  • LFS will always be detected, even in a non-LFS repository.
ptarjan added a comment.EditedMar 20 2020, 3:56 PM

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.

epriestley added inline comments.Mar 20 2020, 4:25 PM
src/workflow/ArcanistDiffWorkflow.php
2905

After sleeping on it, I realize my suggested replacement isn't quite right either: it will result in the wrong behavior if the repository has one or more LFS-controlled files which have only tabs and spaces in their names, and no other LFS-controlled files with other characters in their names. This isn't very likely or common, but is a valid repository state.

I think a completely correct test is:

  • Run git ls-files -z ... to get "\0" terminators.
  • Test for $is_lfs = (strpos($stdout, "\0") !== false);
ptarjan updated this revision to Diff 50128.EditedMar 20 2020, 6:44 PM
ptarjan marked 2 inline comments as done.
  • code review
epriestley accepted this revision.Mar 20 2020, 6:47 PM

Thanks! I added you to Blessed Committers, so you should be able to land this. See that project description for help if you run into issues, or you can likely use "Land Revision" from the web UI here, or I can land it for you if all of that is more trouble than it's worth.

This revision is now accepted and ready to land.Mar 20 2020, 6:47 PM
ptarjan marked 2 inline comments as done.Mar 20 2020, 6:48 PM

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