This is an archive of the discontinued Mercurial Phabricator instance.

narrow: add server logic to send cg while widening without ellipsis
ClosedPublic

Authored by pulkit on Aug 27 2018, 8:09 AM.

Details

Summary

Before this patch, if you try to widen a narrow clone without ellipsis enabled,
it will be broken and the exchange.pull() done by tracked command to widen the
clone will be no-op because no custom logic exists for this and server sees that
we have all csets and it says no changes found.

The widening with ellipsis send KILL for existing changegroups and send new
changegroups because of the change in ellipsis hash, but we can prevent that in
non-ellipsis cases.

This patch adds server side logic to send the changegroups for the changesets
which are on the client again with filelogs and manifests for the new includes.
This is a very starting implementation and we send changegroups and manifests
too while we can prevent them.

Following things can definitely be improved in the logic this patch adds:

  1. Send just the filelogs and treemanifests
  2. Send the filelogs only for the additions in the include

I tried 1) here but the code is coupled tightly and the way I was able to do
that was hacking into the changegroup generation code in a very dirty way, like
adding conditionals and preventing the yield.

This patch also adds a 'widen' kwarg to prevent other commands except widening
to go through that codepath.

The test changes demonstrate that the new implementation is correct and fixes
things.

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

pulkit created this revision.Aug 27 2018, 8:09 AM
martinvonz added inline comments.
hgext/narrow/narrowbundle2.py
77

replace by assert (if you agree with my suggestions further down)?

84–85

This is true when using ellipses as well (and the client probably has some of the filelogs too). I had imagined we'd introduce a new widen wire protocol command. That command would probably look at the known argument and would probably work the same for ellipsis or not (ellipsis-ness would be unchanged by the command). Then we will stop passing oldincludepats, oldexcludepats, and known to getbundle.

Perhaps getbundlechangegrouppart_nonellipsis can be renamed to getbundlechangegrouppart_widen already and then gradually evolve into a handler for the new command?

319

Can we not infer widen from oldincludepats, oldincludepats != includepats, excludepats?

pulkit added inline comments.Aug 29 2018, 1:13 PM
hgext/narrow/narrowbundle2.py
319

I tried that but test-narrow-acl.t fails. Also, this is the error trying to prevent which I added the widen kwarg to make sure clone operations does not go through that.

But, clones with narrowhgacl set will go through this code path.

I looked into fixing this traceback in past but was unable to fix it. Any pointer will be appreciated.

--- /storage/pulkit/hg-committed/tests/test-narrow-acl.t
+++ /storage/pulkit/hg-committed/tests/test-narrow-acl.t.err
@@ -22,21 +22,98 @@
   adding changesets
   adding manifests
   adding file changes
-  added 3 changesets with 2 changes to 2 files
-  new changesets * (glob)
-  updating to branch default
-  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  added 0 changesets with 0 changes to 0 files
+  transaction abort!
+  rollback completed
+  Traceback (most recent call last):
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/scmutil.py", line 164, in callcatch
+      return func()
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/dispatch.py", line 350, in _runcatchfunc
+      return _dispatch(req)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/dispatch.py", line 987, in _dispatch
+      cmdpats, cmdoptions)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/dispatch.py", line 733, in runcommand
+      ret = _runcommand(ui, options, cmd, d)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/dispatch.py", line 995, in _runcommand
+      return cmdfunc()
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/dispatch.py", line 984, in <lambda>
+      d = lambda: util.checksignature(func)(ui, *args, **strcmdopt)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/util.py", line 1532, in check
+      return func(*args, **kwargs)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/util.py", line 1532, in check
+      return func(*args, **kwargs)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/hgext/narrow/narrowcommands.py", line 137, in clonenarrowcmd
+      return orig(ui, repo, *args, **pycompat.strkwargs(opts))
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/util.py", line 1532, in check
+      return func(*args, **kwargs)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/commands.py", line 1475, in clone
+      shareopts=opts.get('shareopts'))
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/hg.py", line 710, in clone
+      streamclonerequested=stream)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/hgext/narrow/narrowcommands.py", line 132, in pullnarrow
+      return orig(repo, *args, **kwargs)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/exchange.py", line 1494, in pull
+      _fullpullbundle2(repo, pullop)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/exchange.py", line 1434, in _fullpullbundle2
+      _pullbundle2(pullop)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/exchange.py", line 1677, in _pullbundle2
+      bundle2.processbundle(pullop.repo, bundle, op=op)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/bundle2.py", line 460, in processbundle
+      processparts(repo, op, unbundler)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/bundle2.py", line 467, in processparts
+      _processpart(op, part)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/bundle2.py", line 534, in _processpart
+      handler(op, part)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/bundle2.py", line 2138, in handlephases
+      phases.updatephases(op.repo.unfiltered(), op.gettransaction, headsbyphase)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/phases.py", line 603, in updatephases
+      heads = [c.node() for c in repo.set(revset, headsbyphase[phase])]
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/localrepo.py", line 916, in set
+      for r in self.revs(expr, *args):
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/localrepo.py", line 905, in revs
+      return m(self)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/revset.py", line 2273, in mfunc
+      return getset(repo, subset, tree, order)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/revset.py", line 102, in getset
+      return methods[x[0]](repo, subset, *x[1:], order=order)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/revset.py", line 194, in differenceset
+      return getset(repo, subset, x, order) - getset(repo, subset, y, anyorder)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/revset.py", line 102, in getset
+      return methods[x[0]](repo, subset, *x[1:], order=order)
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/revset.py", line 122, in stringset
+      x = scmutil.intrev(scmutil.revsymbol(repo, x))
+    File "/place/vartmp/hgtests.Hyf1ig/install/lib/python/mercurial/scmutil.py", line 618, in revsymbol
+      raise error.RepoLookupError(_("unknown revision '%s'") % symbol)
+  RepoLookupError: unknown revision 'e74ca1f005ce671b1e03fd521eac2efb63d49b87'
+  abort: unknown revision 'e74ca1f005ce671b1e03fd521eac2efb63d49b87'!
+  [255]
martinvonz accepted this revision.Aug 29 2018, 7:35 PM
martinvonz added inline comments.
hgext/narrow/narrowbundle2.py
319

Yeah, that does seem harder than I had hoped :( I don't see much harm in adding the widen argument to the protocol, so let's just do it as you have done it here for now. It's probably pretty easy to create a separate wire protocol *command* for it later.

This revision is now accepted and ready to land.Aug 29 2018, 7:35 PM
This revision was automatically updated to reflect the committed changes.
pulkit added inline comments.Aug 30 2018, 8:45 AM
hgext/narrow/narrowbundle2.py
319

Yep. You can except improvements on this from me in upcoming weeks and I will try to make this a separate wire protocol command.