Re: Error using git-remote-hg

From: William Giokas <1007380@gmail.com>
Date: May 13, 2014, 19:39 UTC
Message-ID: <20140513193911.GG9051@wst420>
In-Reply-To: <53726e0355875_4aa4b312f892@nysa.notmuch>

On Tue, May 13, 2014 at 02:09:55PM -0500, Felipe Contreras wrote:

Show 12 quoted lines

> William Giokas wrote:
> > On Tue, May 13, 2014 at 10:30:26AM -0700, Junio C Hamano wrote:
> 
> > >     Why do we "import changegroup" unconditionally, even though it
> > >     is only used in the new codepath meant only for version 3.0 or
> > >     higher, not inside the "if" block that decides if we need that
> > >     module? 
> 
> > changegroup is prefectly /okay/ to import unconditionally, though as you
> > say it will never be used.
> 
> As you say, it's perfectly OK.

But wrong. Yes, it works, but it's not how it should be done when we have a code review such as this. It should simply not be done and makes no sense to do with an 'if <check ver>; else' kind of thing later in the application.

Show 16 quoted lines

> 
> > We can also be even more explicit with what we import by doing something
> > like::
> > 
> >   try:
> >       from mercurial.changegroup import getbundle
> > 
> >   except ImportError:
> >       def getbundle(__empty__, **kwargs):
> >           return repo.getbundle(**kwargs)
> 
> We could try that, but that would assume we want to maintain getbundle()
> for the long run, and I personally don't want to do that. I would rather
> contact the Mercurial developers about ways in which the push() method
> can be improved so we don't need to have our own version. Hopefully
> after it's improved we wouldn't have to call getbundle().

Assuming that mercurial <3.0 will not change, then this should never need to change. Changes in 'getbundle' upstream would require changes either way.

Show 5 quoted lines

> Moreover, eventually there will be a Mercurial 4.0, even 5.0, and at
> some point we would want to remove the hacks for older versions. When we
> do so we would want the import to remain unconditionally, and remove the
> 'check_version(3, 0)' which is already helping to explain what the code
> is for without the need of comments.

The same exact thing can be done with this. In fact, it would probably allow us to have better future-proofing with regards to new versions of mercurial, there would just be different try:except statements at the beginning.

Show 8 quoted lines

> 
> > I was really sad to see that, and didn't have time to really look at it
> > because of work and other projects, but I hope this presents a better
> > solution than the current patch.
> 
> Either way Junio doesn't maintain this code, I do. And it's not
> maintained in git.git, git's maintained out-of-tree (thanks to Junio's
> decisions).

I still see it in git.git, and I will contribute this upstream for as long as it is in the tree. If you want to use the patch that I sent to this list, feel free.

> So please post your suggestions and patches to git-fc@googlegroups.com,
> and use the latest code at https://github.com/felipec/git-remote-hg.

Thanks,

-- 
William Giokas | KaiSforza | http://kaictl.net/
GnuPG Key: 0x73CD09CF
Fingerprint: F73F 50EF BBE2 9846 8306  E6B8 6902 06D8 73CD 09CF