On Tue, May 13, 2014 at 10:30:26AM -0700, Junio C Hamano wrote: > Torsten Bögershausen <tbo...@web.de> writes: > > > I did some testing with Git 2.0-rc3 + 58aee0864adeeb5363f. > > The remote-helper tests for hg-git worked OK > > with both hg version 2.9 and 3.0 under both Mac OS and Linux. > > > > Should we consider 58aee086 to be included in Git 2.0 ? > > So the answer to your question is an unambiguous "no". > > It is a different matter if we want a change to allow us to operate > with both older and newer version of mercurial in a release of Git > in near future. The answer is a resounding "yes", even if the > solution may not be the exact 58aee0864 commit [*2*]. I think an > update should eventurally go to the maintenance tracks of even older > releases, perhaps maint-1.8.5 and upwards, after we merge it to > 'master' in preparation for the feature release that comes after Git > 2.0, which probably will be called 2.1 but do not quote me on that. > > Regarding the commit in question, I asked Felipe a question and > never got a straight answer. > > 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?
It should not be, as it is not used outside of hg 3.0. In fact, what
would be even better would be to test whether changegroup.getbundle is
available, and then set a function like `getbundl()` to run either
`changegroup.getbundl()` with the correct args, or `repo.getbundle()` as
a fallback.
changegroup is prefectly /okay/ to import unconditionally, though as you
say it will never be used.
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)
and then just calling::
cg = getbundle(repo, 'push', heads=list(p_revs), common=common)
The `__empty__` arg is there because repo.getbundle uses a different
number of arguments than the changegroup.getbundle function. It's a much
cleaner solution than assuming that that will stay in changegroup, and
not get moved back to repo in the future. Seems to be what you described
in the second bit, though. If you would like I can submit a patch once
I've finished running the tests on it, and possibly after having Felipe
run it through his tests to be sure thta the 2.[7-9] series works as
well, and maybe you would want to test it on other versions of
mercurial that you are running alongside git.
Just my 2 cents.
>
> I had a few more questions in mind that I didn't have a chance to
> ask, and the commit log message did not explain.
>
> Is the reason why this fix is needed is because "repo" stopped
> being a way to ask ".getbundle()" and in the new world order
> "changegroup" is the new way to ask ".getbundle()"?
>
> If the answer is "yes", then asking "are we with 3.0 or
> later---if so ask changegroup for ".getbundle()?", which is this
> patch does, may not be the right condition to trigger the new
> codepath. "Does repo know how to do .getbundle()? If not,
> import changegroup and ask that module to perform .getbundle()
> instead" may be a way to base the decision on a more directly
> relevant condition.
>
> Answers to these questions, if they came, were meant to convince
> myself that the patch 58aee0864 is the best solution to the problem,
> but because I only got "Of course it is not a mistake" [*1*], seeing
> it did not lead to a productive discussion, I gave up. So I haven't
> even managed to convince myself that that commit is the best
> solution to the problem.
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.
Thanks,
--
William Giokas | KaiSforza | http://kaictl.net/
GnuPG Key: 0x73CD09CF
Fingerprint: F73F 50EF BBE2 9846 8306 E6B8 6902 06D8 73CD 09CF
pgpC7ZLGG4hd9.pgp
Description: PGP signature
