On Tue, May 13, 2014 at 02:09:55PM -0500, Felipe Contreras wrote: > 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. > > > 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. > 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. > > > 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@xxxxxxxxxxxxxxxx, > 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
Attachment:
pgpY9J0GQCs0K.pgp
Description: PGP signature