William Giokas wrote:
> 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.
That's exactly how it should be done. Maybe this visualization helps
from mercurial import hg, ui, bookmarks, ... # for hg >= 3.0
from mercurial import node, error, extensions, ... # for hg >= 3.0
from mercurial import changegroup # for hg >= 3.0
if check_version(3, 0):
cg = changegroup.getbundle(repo, 'push', ... # for hg >= 3.0
else:
cg = repo.getbundle('push', heads=list(p_revs) # for hg < 3.0
Eventually the code would become:
from mercurial import hg, ui, bookmarks, ... # for hg >= 3.0
from mercurial import node, error, extensions, ... # for hg >= 3.0
from mercurial import changegroup # for hg >= 3.0
cg = changegroup.getbundle(repo, 'push', ... # for hg >= 3.0
The fact that at some point 'import changegroup' was not needed is
irrelevant.
Primarily we want to support the current version of Mercurial.
Secondarily we want to support older versions. That's why we add the
repo.getbundle() code (ass an addendum to the core part).
> > > 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.
But it should. Otherwise the code will keep having more and more hacks
and it will become less and less maintainable.
That's why we don't have code for hg 1.0; because it would require too
many hacks, and nobody cared anyway.
Just like nobody cares about hg 1.0, eventually nobody will care about
hg 2.0.
> Changes in 'getbundle' upstream would require changes either way.
I doubt we will see any more changes in getbundle, at least not until
4.0, and hopefully by then we wouldn't be using it anyway. I am willing
to bet we won't see those changes.
> > 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.
No, your code doesn't show that this is for versiosn <= 3.0,
'check_version(3, 0)' does.
Plus, when we remove this code my version makes it straight forward:
- if check_version(3, 0):
- cg = changegroup.getbundle(repo, 'push', ...
- else:
- cg = repo.getbundle('push', heads=list(p_revs), ...
+ cg = changegroup.getbundle(repo, 'push', ...
Not so with your code:
-
-try:
- from mercurial.changegroup import getbundle
-
-except ImportError:
- def getbundle(__empty__, **kwargs):
- return repo.getbundle(**kwargs)
+from mercurial import getbundle
import re
import sys
@@ -1036,7 +1030,7 @@ def push_unsafe(repo, remote, ...
if not checkheads(repo, remote, p_revs):
return None
- cg = getbundle(repo, 'push', heads=list(p_revs), ...
+ cg = changegroup.getbundle(repo, 'push', ...
> > > 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.
And what happens when it's eventually removed?
> If you want to use the patch that I sent to this list, feel free.
I don't believe this is the way to go, so I won't.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html