Page MenuHomePhabricator

infinitepush: handle lfs correctly

Authored by stash on Jul 10 2017, 1:42 PM.


Group Reviewers
Restricted Project
rFBHGX557b7fa4bea5: infinitepush: handle lfs correctly

Make sure that infinitepush work fine with lfs. To achieve it we try to use
changegroup3 if it's available and also run prepush hook before infinitepush.

Test Plan

Run arc unit

Diff Detail

rFBHGX Facebook Mercurial Extensions
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

stash created this revision.Jul 10 2017, 1:42 PM
stash added a reviewer: dsp.Jul 11 2017, 10:48 AM
dsp added inline comments.Jul 11 2017, 11:10 AM

I feel this code is pretty specific and not forward compatible if there is ever a cgversion = '04'.

I would recommend using cgversion = changegroup.safeversion(repo) and overwrite changegroup.supportedoutgoingversions(repo) and discarding '01' in there. That way we can ensure we don't do 01, but everything later.

We should also always try to handle lfs and leave it to the LFS extension to correctly set the supported version, e.g.:

def extsetup(...):
    def infinitepushsupported(orig, repo):
         versions = orig(repo)
         return versions

    extensions.wrapfunction(changegroup, 'supportedoutgoingversions', infinitepushsupported)

def getscratchbranchpart(...):
     cgversion = changegroup.safeversion(repo)
     _handlelfs(repo, outgoing.missing)

@quark what do you think?

quark requested changes to this revision.Jul 11 2017, 2:42 PM

Back to your queue for addressing dsop's comment. Glad to see #testcases being used again!


Maybe make it a public API like uploadblobsfromrevs(repo, revs)? It's a bit strange to keep "Move implementation to a separate function" in docstring, maybe just say:

"""upload lfs blobs introduced by revs

Note: also used by other extensions. avoid renaming.

This could use the new uploadblobsfromrevs API.


I agree. Infinitepush only needs to remove cg01 support (better to check if 01 exists before dropping it). LFS drops cg02 support independently. And uploading LFS blobs does not depend on changegroup. So calling existing API to get the cg version is better.

This revision now requires changes to proceed.Jul 11 2017, 2:42 PM
stash added a comment.Jul 12 2017, 8:18 AM

#testcases rock, thank you @quark

stash edited edge metadata.Jul 12 2017, 9:06 AM
stash updated this revision to Diff 84.

addressed comments

dsp accepted this revision.Jul 12 2017, 10:13 AM

@stash thank you for taking care of this!



Let's move this into the try block, to make it more clear it only gets executed when no exception happens.

stash updated this revision to Diff 85.Jul 12 2017, 12:46 PM

addressed comments

stash added a comment.Jul 13 2017, 3:40 AM

I've already landed this revision. How can I close it? Should I just abandon the revision

durham added a subscriber: durham.Jul 13 2017, 3:55 PM

I just pushed the latest commits to bitbucket. I think we have to do that to get this phabricator to pick them up and close revisions.

This revision was automatically updated to reflect the committed changes.