Page MenuHomePhabricator

When pushing changes to staging, also push the base commit
ClosedPublic

Authored by epriestley on Mar 7 2016, 2:27 PM.

Details

Summary

Fixes T10509. Pushing changes to staging can be inefficient. What happens, roughly, is:

  • Master is at commit "W" -- "W" is the most recent published commit in the main repository.
  • The local working copy has one change on top of that, "X", so its history is commits "A, B, C, D, E, F, ..., U, V, W, X".
  • The remote has some other previous changes that I or other users have made, maybe like "A, B, C, ..., S, T, U, Y" and "A, B, C, ..., T, U, V, Z", from previous pushes to staging areas.
  • "X", "Y" and "Z" will never actually make it to master, because they'll be squash-merged/amended by arc land.

So the local says "I want to push 'X'", and the remote says "I know about 'Y' and 'Z', are those in the history of 'W'? You only need to send me new stuff if they are".

But they aren't, so the local says "nope, so here's the whole history for you". This is slow and sends a ton of data that the remote already has over the network.

In theory, Git could use a slightly different algorithm to tell the local about more commits, but this is hard, rarely useful, and not the kind of thing I'd be excited about changing if I was the Git upstream.

Instead, when pushing "X", also push "W", to trick Git into telling future clients about it.

Now, the remote should say "I know about 'W', 'Y' and 'Z'", and the local will say "oh, great, 'W' is in history, here's just the changes since then".

Also, fail arc diff if the push to staging fails, and tell users to use --skip-staging. This code has been in production for a while and doesn't seem to have any issues, and a failed push to staging prevents builds, lands, etc.

Test Plan
  • Ran arc diff, saw two changes push.
  • Ran arc diff --base arc:empty, saw only one change push.
  • Ran arc diff with an intentionally broken staging area, saw an error.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to When pushing changes to staging, also push the base commit.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
chad edited edge metadata.
This revision is now accepted and ready to land.Mar 7 2016, 2:47 PM

I'm not actually completely sure that this does what it's supposed to because I'm having trouble tricking git into transferring too much data locally -- that is, I can't really reproduce the original issue.

I think it probably works because (a) users in T8238 have hit this issue and (b) cronning a push to master seems to fix it. It's possible that pushing to a tag doesn't work while pushing to a branch does, but git is always outsmarting me locally when I try to trick it into doing too much work.

This costs us pretty much nothing to try, so we can see if it fixes the problem and dig in a bit more if it doesn't.

This revision was automatically updated to reflect the committed changes.