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

[Idan Kamara]

On Tue, Oct 30, 2012 at 1:54 PM, Alastair Houghton <
alastair at alastairs-place.net> wrote:
>
> 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.

That won't work in general, consider a<-b<-c and `hg log -r 'a or c'`.

>
> > 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).

Regardless if that happens (very unlikely imo), we could always fetch the
other parents by changing the template.

>
> > 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.

Yeah that's what I thought but it's a bit funny. You could reuse revision
with None's as the missing fields but I like that even less.

>
> > 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?

BC is certainly something we should keep in mind, but we're in v0.2...

>
> 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.

Not sure about slices and other fancy things but we can add len, iter
and reversed right now.

>
> 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]

Looks good, send a patch to fix that?

>
> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20121030/ae8ab0e1/attachment.html>