D2067: changegroup: do not delta lfs revisions

indygreg (Gregory Szorc) phabricator at mercurial-scm.org
Wed Feb 7 17:33:52 EST 2018


indygreg requested changes to this revision.
indygreg added a comment.
This revision now requires changes to proceed.


  This looks mostly good. I would like a change to address a future footgun though.
  
  I would also appreciate someone familiar with censor and narrow to weigh in on the implications of disabling delta generation for revisions that have the censor and ellipsis flags set. I'm pretty sure narrow will cope since it reimplements changegroup generation. Not sure how censor will react. (But I know we already have random code for detecting censored nodes during changegroup generation.)

INLINE COMMENTS

> revlog.py:719
> +        # disable delta if either rev uses non-default flag (ex. LFS)
> +        if self.flags(baserev) or self.flags(rev):
> +            return False

This logic assumes that revision flags will only ever be used to influence the presence of content. That is true today because our flags are for `REVIDX_ISCENSORED`, `REVIDX_ELLIPSIS`, and `REVIDX_EXTSTORED`. But if we ever introduced a new revision flag for e.g. compression strategy, then testing for non-empty revision flags would be wrong.

I think we want to capture the bitmask of revision flags influencing delta generation explicitly. Or we want a big comment by the revision flags constants at the top of the file telling people to audit `candelta()` when adding new revision flags.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2067

To: quark, indygreg, #hg-reviewers
Cc: mercurial-devel


More information about the Mercurial-devel mailing list