Patchwork [8,of,8,v4] convert: return commit objects for revisions in the revmap

login
register
mail settings
Submitter David Soria Parra
Date Dec. 14, 2016, 9:46 a.m.
Message ID <eb4a6df3de20deb31e68.1481708804@devbig415.prn1.facebook.com>
Download mbox | patch
Permalink /patch/17898/
State Superseded
Headers show

Comments

David Soria Parra - Dec. 14, 2016, 9:46 a.m.
# HG changeset patch
# User David Soria Parra <davidsp@fb.com>
# Date 1481704712 28800
#      Wed Dec 14 00:38:32 2016 -0800
# Node ID eb4a6df3de20deb31e681aa7eba408dc352440c3
# Parent  26ec2474ab06fdaf1aca034c54309245ded267b0
convert: return commit objects for revisions in the revmap

Source revision data that exists in the revmap are ignored when pulling
data from Perforce as we consider them already imported. In case where
the `convertcmd.convert` algorithm requests a commit object for such
a revision we are creating it.  This is usually the case for parent of
the first imported revision.
Kostia Balytskyi - Dec. 14, 2016, 4:53 p.m.
On 12/14/16 9:46 AM, David Soria Parra wrote:
> # HG changeset patch

> # User David Soria Parra <davidsp@fb.com>

> # Date 1481704712 28800

> #      Wed Dec 14 00:38:32 2016 -0800

> # Node ID eb4a6df3de20deb31e681aa7eba408dc352440c3

> # Parent  26ec2474ab06fdaf1aca034c54309245ded267b0

> convert: return commit objects for revisions in the revmap

>

> Source revision data that exists in the revmap are ignored when pulling

> data from Perforce as we consider them already imported. In case where

> the `convertcmd.convert` algorithm requests a commit object for such

> a revision we are creating it.  This is usually the case for parent of

> the first imported revision.

>

> diff --git a/hgext/convert/p4.py b/hgext/convert/p4.py

> --- a/hgext/convert/p4.py

> +++ b/hgext/convert/p4.py

> @@ -321,6 +321,12 @@

>           return marshal.load(stdout)

>   

>       def getcommit(self, rev):

> +        if rev not in self.changeset and rev not in self.revmap:

> +            raise error.Abort(

> +                _("cannot find %s in the revmap or parsed changesets") % rev)

> +        if rev not in self.changeset:

I think this would be more clear if you used 'if rev in self.revmap'. 
But I think whoever queues it can change it in flight.
> +            d = self._fetch_revision(rev)

> +            return self._construct_commit(d, parents=None)

>           return self.changeset[rev]

>   

>       def gettags(self):

> _______________________________________________

> Mercurial-devel mailing list

> Mercurial-devel@mercurial-scm.org

> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Kostia Balytskyi - Dec. 14, 2016, 5 p.m.
Other than the nit below, this series LGTM.

On 12/14/16 4:53 PM, Kostia Balytskyi wrote:
>

> On 12/14/16 9:46 AM, David Soria Parra wrote:

>> # HG changeset patch

>> # User David Soria Parra <davidsp@fb.com>

>> # Date 1481704712 28800

>> #      Wed Dec 14 00:38:32 2016 -0800

>> # Node ID eb4a6df3de20deb31e681aa7eba408dc352440c3

>> # Parent  26ec2474ab06fdaf1aca034c54309245ded267b0

>> convert: return commit objects for revisions in the revmap

>>

>> Source revision data that exists in the revmap are ignored when pulling

>> data from Perforce as we consider them already imported. In case where

>> the `convertcmd.convert` algorithm requests a commit object for such

>> a revision we are creating it.  This is usually the case for parent of

>> the first imported revision.

>>

>> diff --git a/hgext/convert/p4.py b/hgext/convert/p4.py

>> --- a/hgext/convert/p4.py

>> +++ b/hgext/convert/p4.py

>> @@ -321,6 +321,12 @@

>>            return marshal.load(stdout)

>>    

>>        def getcommit(self, rev):

>> +        if rev not in self.changeset and rev not in self.revmap:

>> +            raise error.Abort(

>> +                _("cannot find %s in the revmap or parsed changesets") % rev)

>> +        if rev not in self.changeset:

> I think this would be more clear if you used 'if rev in self.revmap'.

> But I think whoever queues it can change it in flight.

>> +            d = self._fetch_revision(rev)

>> +            return self._construct_commit(d, parents=None)

>>            return self.changeset[rev]

>>    

>>        def gettags(self):

>> _______________________________________________

>> Mercurial-devel mailing list

>> Mercurial-devel@mercurial-scm.org

>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

> _______________________________________________

> Mercurial-devel mailing list

> Mercurial-devel@mercurial-scm.org

> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
David Soria Parra - Dec. 14, 2016, 5:55 p.m.
On Wed, Dec 14, 2016 at 04:53:24PM +0000, Kostia Balytskyi wrote:
> 
> 
> On 12/14/16 9:46 AM, David Soria Parra wrote:
> > # HG changeset patch
> > # User David Soria Parra <davidsp@fb.com>
> > # Date 1481704712 28800
> > #      Wed Dec 14 00:38:32 2016 -0800
> > # Node ID eb4a6df3de20deb31e681aa7eba408dc352440c3
> > # Parent  26ec2474ab06fdaf1aca034c54309245ded267b0
> > convert: return commit objects for revisions in the revmap
> >
> > Source revision data that exists in the revmap are ignored when pulling
> > data from Perforce as we consider them already imported. In case where
> > the `convertcmd.convert` algorithm requests a commit object for such
> > a revision we are creating it.  This is usually the case for parent of
> > the first imported revision.
> >
> > diff --git a/hgext/convert/p4.py b/hgext/convert/p4.py
> > --- a/hgext/convert/p4.py
> > +++ b/hgext/convert/p4.py
> > @@ -321,6 +321,12 @@
> >           return marshal.load(stdout)
> >   
> >       def getcommit(self, rev):
> > +        if rev not in self.changeset and rev not in self.revmap:
> > +            raise error.Abort(
> > +                _("cannot find %s in the revmap or parsed changesets") % rev)
> > +        if rev not in self.changeset:
> I think this would be more clear if you used 'if rev in self.revmap'. 
> But I think whoever queues it can change it in flight.
that would not be sufficient. We want to fallback to revmap but use
changeset if we have it.
	
So it would be:
	if rev in self.revmap and rev not in self.changeset:
or:
	if rev in self.changeset:
		return self.changeset[rev]
	elif rev in self.revmap:
		...
	else:
		...

I can sent a v5 later.

> > +            d = self._fetch_revision(rev)
> > +            return self._construct_commit(d, parents=None)
> >           return self.changeset[rev]
> >   
> >       def gettags(self):
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> 
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Kostia Balytskyi - Dec. 14, 2016, 6:23 p.m.
On 12/14/16 5:55 PM, David Soria Parra wrote:
> On Wed, Dec 14, 2016 at 04:53:24PM +0000, Kostia Balytskyi wrote:

>>

>> On 12/14/16 9:46 AM, David Soria Parra wrote:

>>> # HG changeset patch

>>> # User David Soria Parra <davidsp@fb.com>

>>> # Date 1481704712 28800

>>> #      Wed Dec 14 00:38:32 2016 -0800

>>> # Node ID eb4a6df3de20deb31e681aa7eba408dc352440c3

>>> # Parent  26ec2474ab06fdaf1aca034c54309245ded267b0

>>> convert: return commit objects for revisions in the revmap

>>>

>>> Source revision data that exists in the revmap are ignored when pulling

>>> data from Perforce as we consider them already imported. In case where

>>> the `convertcmd.convert` algorithm requests a commit object for such

>>> a revision we are creating it.  This is usually the case for parent of

>>> the first imported revision.

>>>

>>> diff --git a/hgext/convert/p4.py b/hgext/convert/p4.py

>>> --- a/hgext/convert/p4.py

>>> +++ b/hgext/convert/p4.py

>>> @@ -321,6 +321,12 @@

>>>            return marshal.load(stdout)

>>>    

>>>        def getcommit(self, rev):

>>> +        if rev not in self.changeset and rev not in self.revmap:

>>> +            raise error.Abort(

>>> +                _("cannot find %s in the revmap or parsed changesets") % rev)

>>> +        if rev not in self.changeset:

>> I think this would be more clear if you used 'if rev in self.revmap'.

>> But I think whoever queues it can change it in flight.

> that would not be sufficient. We want to fallback to revmap but use

> changeset if we have it.

> 	

> So it would be:

> 	if rev in self.revmap and rev not in self.changeset:

> or:

> 	if rev in self.changeset:

> 		return self.changeset[rev]

> 	elif rev in self.revmap:

> 		...

> 	else:

> 		...

>

> I can sent a v5 later.

Right, I missed the case when it's in both sets.
>

>>> +            d = self._fetch_revision(rev)

>>> +            return self._construct_commit(d, parents=None)

>>>            return self.changeset[rev]

>>>    

>>>        def gettags(self):

>>> _______________________________________________

>>> Mercurial-devel mailing list

>>> Mercurial-devel@mercurial-scm.org

>>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

>> _______________________________________________

>> Mercurial-devel mailing list

>> Mercurial-devel@mercurial-scm.org

>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

> _______________________________________________

> Mercurial-devel mailing list

> Mercurial-devel@mercurial-scm.org

> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/hgext/convert/p4.py b/hgext/convert/p4.py
--- a/hgext/convert/p4.py
+++ b/hgext/convert/p4.py
@@ -321,6 +321,12 @@ 
         return marshal.load(stdout)
 
     def getcommit(self, rev):
+        if rev not in self.changeset and rev not in self.revmap:
+            raise error.Abort(
+                _("cannot find %s in the revmap or parsed changesets") % rev)
+        if rev not in self.changeset:
+            d = self._fetch_revision(rev)
+            return self._construct_commit(d, parents=None)
         return self.changeset[rev]
 
     def gettags(self):