This is an archive of the discontinued Mercurial Phabricator instance.

narrow: when widening, don't include manifests the client already has
ClosedPublic

Authored by martinvonz on Oct 5 2018, 4:49 PM.

Details

Summary

When widening, we already don't include the changelog (since
f1844a10ee19) and files that the client already has (since
c73c7653dfb9). However, we still include all manifests needed for the
new narrowspec. When using flat manifests, that means we resend all
the manifests even though the client necessarily has all of them. For
tree manifests, we unnecessarily resend the root manifests and any
subdirectory manifests that the client already has.

This patch makes it so we no longer resend manifests that the client
already has. It does so by passing an extra matcher to the changegroup
packer and it uses that for filtering out directories matching the old
matcher's visitdir(). For consistency between directories and files,
it also makes the filtering of files look at both old and new matcher
rather than passing in a diff matcher as we did before.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Oct 5 2018, 4:49 PM
pulkit added a subscriber: pulkit.Oct 5 2018, 6:28 PM

@martinvonz Thanks a lot for putting time and effort in trying alternate approach. Sadly this does not work. I didn't tested this on our repo yet, but I tested this on test-narrow-widen-no-ellipsis.t. You can also do that, by removing the globs from this line https://www.mercurial-scm.org/repo/hg-committed/file/daff528e00d7/tests/test-narrow-widen-no-ellipsis.t#l116 and seeing the payload before and after the patch. With this patch they don't change, so we are sending the same amount of data before and after this patch. I have mentioned numbers in D4887 description.

hgext/narrow/narrowwirepeer.py
86

I swapped diffmatch and oldmatch here while testing because that's how widen_bundle() accepts them

pulkit added inline comments.Oct 5 2018, 6:43 PM
mercurial/changegroup.py
1059–1068

Also, I think this is not the right place to case this because not yielding these deltas will prevent the lookupfn being called and will result in tmfnodes being not updated correctly.

martinvonz updated this revision to Diff 12129.Oct 14 2018, 8:20 AM

This patch is now waiting for me to test it on our internal repo and make sure it works on multi-level dirs and big repos. Since this a server side fix, it will take some time for me to test this internally and I will try to do it in next couple of days. Also this is something which we will definitely want in the upcoming release.

martinvonz retitled this revision from RFC: narrow: don't include manifests the client already has to narrow: don't include manifests the client already has.Oct 16 2018, 3:02 PM
martinvonz updated this revision to Diff 12175.
martinvonz marked an inline comment as done.Oct 16 2018, 4:17 PM
martinvonz planned changes to this revision.
martinvonz added inline comments.
mercurial/changegroup.py
1059–1068

The name "_oldfilematcher" is a little ironic since it's not used for file (only directories). The reason that we don't consult it for files is that we get passed a differenecematcher when widening and the subtraction of files the client already has is done by that.

If we're making changegroup.generate() aware of widening like this patch does, perhaps we should make the caller not pass a differencematcher and we should instead check _oldfilematcher for files too. That would be more consistent and easier to understand. I guess there's not much reason not to do that, so I'll work on that (I'll update this patch).

1059–1068

Fixed. I had not realized that deltas is a generator. Thanks for point that out.

martinvonz marked an inline comment as done.Oct 16 2018, 5:08 PM
martinvonz updated this revision to Diff 12184.

I have tested this and it works well with our internal repo too. Thanks!

We already have some tests and none fails, do you want to add more tests or I can queue this?

In D4895#76912, @pulkit wrote:

I have tested this and it works well with our internal repo too. Thanks!
We already have some tests and none fails, do you want to add more tests or I can queue this?

I should at least update the commit message. I'll try to do that ~now.

martinvonz edited the summary of this revision. (Show Details)Oct 17 2018, 12:52 PM
martinvonz retitled this revision from narrow: don't include manifests the client already has to narrow: when widening, don't include manifests the client already has.
martinvonz updated this revision to Diff 12217.
In D4895#76912, @pulkit wrote:

I have tested this and it works well with our internal repo too. Thanks!
We already have some tests and none fails, do you want to add more tests or I can queue this?

I should at least update the commit message. I'll try to do that ~now.

Done. I also added an extra test.

pulkit accepted this revision.Oct 18 2018, 6:52 AM

Many thanks for investing time and efforts into this. \o/

mercurial/changegroup.py
1065

maybe, we can prevent walking the deltas in case of trees because if the root does not match, the rest of tree won't match. I will try to test this and send a patch if that works.

This revision was automatically updated to reflect the committed changes.
pulkit added inline comments.Oct 18 2018, 7:55 AM
hgext/narrow/narrowwirepeer.py
15

removed this in flight to make pyflakes happy.

yuja added a subscriber: yuja.Oct 18 2018, 9:39 AM

narrowwirepeer.py:15

hg,
match as matchmod,
narrowspec,

removed this in flight to make pyflakes happy.

So did I. :)