<div dir="ltr">On Tue, Oct 30, 2012 at 1:54 PM, Alastair Houghton <<a href="mailto:alastair@alastairs-place.net">alastair@alastairs-place.net</a>> wrote:<br>><br>> On 30 Oct 2012, at 09:57, Idan Kamara <<a href="mailto:idankk86@gmail.com">idankk86@gmail.com</a>> wrote:<br>
><br>> > On Mon, Oct 29, 2012 at 10:48 PM, Alastair Houghton<br>> > <<a href="mailto:alastair@alastairs-place.net">alastair@alastairs-place.net</a>> wrote:<br>> > ><br>> > > # HG changeset patch<br>
> > > # User Alastair Houghton <<a href="mailto:alastair@coriolis-systems.com">alastair@coriolis-systems.com</a>><br>> > > # Date 1351543544 0<br>> > > # Node ID 0886df81888d4bf27d5a69f2a41820cf9fbff347<br>
> > > # Parent 86ff8611a8fad14d4c1dba6d75bc26caaa41e951<br>> > > client: grab the parent revision information from the server in<br>> > > hgclient.log()<br>> ><br>> > It took me a while to realize that {parents} is trying to be smart<br>
> > and not always print the parents, which might make sense<br>> > for display purposes, but I'm not sure if it's desirable here.<br>><br>> Well, hglib could do a test to see if rev was zero or not and then return<br>
> either an empty list or the previous changeset revision---but fetching the<br>> node ID would incur a round-trip to the server and hence be quite expensive<br>> when fetching an entire log.<div><br></div><div>That won't work in general, consider a<-b<-c and `hg log -r 'a or c'`.</div>
<div><br>><br>> > Maybe we should be using {p1/2rev} and {p1/2node}?<br>><br>> It seems to me that the fact that Mercurial currently only supports<br>> two-way merge isn't something that should be encoded into the design of<br>
> hglib. Unless we're entirely certain that Mercurial will never support and<br>> never want to support n-way merge for n > 2, obviously. Worth pointing out<br>> that Git *does* have support for n-way merge, and it's conceivable that<br>
> there are cases where doing a single n-way merge is easier (though I think<br>> everyone accepts that in most cases an n-way merge ends up being performed<br>> as a series of 2-way merges).</div><div><br></div>
<div>Regardless if that happens (very unlikely imo), we could always fetch the</div><div>other parents by changing the template.</div><div><br>><br>> > I'm not sure I like this special 'parent' tuple that is basically<br>
> > a thin version of revision.<br>><br>> That isn't exactly how I was looking at it---rather, I saw it as an<br>> improvement over a list of rev:node strings, which is what you'd get back<br>> from log() et al if I was being lazy :-)---but I can see why you might view<br>
> it that way. Also note that {parents} gives you *short* node hashes, not<br>> the full hash.<br>><br>> I rather saw the parentinfo tuple as just a component of the revisions<br>> tuple, but thought it nice to give the revision number and node names, so<br>
> that rev.parent.rev and rev.parent.node would work in the same way that<br>> rev.rev and rev.node do now.</div><div><br></div><div>Yeah that's what I thought but it's a bit funny. You could reuse revision</div>
<div>with None's as the missing fields but I like that even less.</div><div><br>><br>> > Is there any reason we can't get rid of these tuples altogether<br>> > and use changectx everywhere?<br>><br>
> Doing that would, surely, break backwards compatibility, to a greater or<br>> lesser extent depending on how far you went in making changectx emulate the<br>> revision tuple?</div><div><br></div><div>BC is certainly something we should keep in mind, but we're in v0.2...</div>
<div><br>><br>> If we were going to go down that route, I think I'd want client to<br>> implement a few more of the container methods (e.g. it could implement<br>> __len__, __iter__ and __reversed__, and it could also support slices for<br>
> __getitem__), plus it'd be nice to be able to use with for log method<br>> options, e.g. so you could do something like<br>><br>> with hglib.filters.user('foo'), hglib.filters.removed:<br>> revs = repo[-1::-1]<br>
><br>> to get, in descending revision order, all of the revisions belonging to<br>> the user 'foo' including those where files were removed.</div><div><br></div><div>Not sure about slices and other fancy things but we can add len, iter</div>
<div>and reversed right now.</div><div><br>><br>> Some caching of changectx instances might be a good idea too, to avoid<br>> unnecessary data fetches.<br>><br>> However, this is turning into a wish list :-)<br>
><br>> BTW, looking at it again, I wonder why it is that changectx.parents()<br>> wasn't written like this:<br>><br>> def _parents(self):<br>> """return contexts for each parent changeset"""<br>
> par = self._repo.parents(rev=self)<br>> if not par:<br>> return [changectx(self._repo, -1)]<br>> return [changectx(self._repo, cset) for cset in par]</div><div><br></div><div>Looks good, send a patch to fix that?</div>
<div><br>><br>> since, as it's currently written, that would have avoided a call to log()<br>> for each revision. Maybe that would be better than my change too, since<br>> mine still calls log() every time.<br>
</div></div>