[PATCH] client: grab the parent revision information from the server in hgclient.log()

Alastair Houghton alastair at alastairs-place.net
Tue Oct 30 06:54:42 CDT 2012


On 30 Oct 2012, at 09:57, Idan Kamara <idankk86 at gmail.com> wrote:

> On Mon, Oct 29, 2012 at 10:48 PM, Alastair Houghton <alastair at alastairs-place.net> wrote:
> >
> > # HG changeset patch
> > # User Alastair Houghton <alastair at coriolis-systems.com>
> > # Date 1351543544 0
> > # Node ID 0886df81888d4bf27d5a69f2a41820cf9fbff347
> > # Parent  86ff8611a8fad14d4c1dba6d75bc26caaa41e951
> > client: grab the parent revision information from the server in
> > hgclient.log()
> 
> It took me a while to realize that {parents} is trying to be smart
> and not always print the parents, which might make sense
> for display purposes, but I'm not sure if it's desirable here.

Well, hglib could do a test to see if rev was zero or not and then return either an empty list or the previous changeset revision---but fetching the node ID would incur a round-trip to the server and hence be quite expensive when fetching an entire log.

> Maybe we should be using {p1/2rev} and {p1/2node}?

It seems to me that the fact that Mercurial currently only supports two-way merge isn't something that should be encoded into the design of hglib.  Unless we're entirely certain that Mercurial will never support and never want to support n-way merge for n > 2, obviously.  Worth pointing out that Git *does* have support for n-way merge, and it's conceivable that there are cases where doing a single n-way merge is easier (though I think everyone accepts that in most cases an n-way merge ends up being performed as a series of 2-way merges).

> I'm not sure I like this special 'parent' tuple that is basically
> a thin version of revision.

That isn't exactly how I was looking at it---rather, I saw it as an improvement over a list of rev:node strings, which is what you'd get back from log() et al if I was being lazy :-)---but I can see why you might view it that way.  Also note that {parents} gives you *short* node hashes, not the full hash.

I rather saw the parentinfo tuple as just a component of the revisions tuple, but thought it nice to give the revision number and node names, so that rev.parent.rev and rev.parent.node would work in the same way that rev.rev and rev.node do now.

> Is there any reason we can't get rid of these tuples altogether
> and use changectx everywhere?

Doing that would, surely, break backwards compatibility, to a greater or lesser extent depending on how far you went in making changectx emulate the revision tuple?

If we were going to go down that route, I think I'd want client to implement a few more of the container methods (e.g. it could implement __len__, __iter__ and __reversed__, and it could also support slices for __getitem__), plus it'd be nice to be able to use with for log method options, e.g. so you could do something like

  with hglib.filters.user('foo'), hglib.filters.removed:
    revs = repo[-1::-1]

to get, in descending revision order, all of the revisions belonging to the user 'foo' including those where files were removed.

Some caching of changectx instances might be a good idea too, to avoid unnecessary data fetches.

However, this is turning into a wish list :-)

BTW, looking at it again, I wonder why it is that changectx.parents() wasn't written like this:

  def _parents(self):
      """return contexts for each parent changeset"""
      par = self._repo.parents(rev=self)
      if not par:
          return [changectx(self._repo, -1)]
      return [changectx(self._repo, cset) for cset in par]

since, as it's currently written, that would have avoided a call to log() for each revision.  Maybe that would be better than my change too, since mine still calls log() every time.

Kind regards,

Alastair.

--
http://alastairs-place.net






More information about the Mercurial-devel mailing list