[PATCH] remote-hg: store converted URL

[PATCH] remote-hg: store converted URL

[Max Horn]

From: Felipe Contreras <felipe.contreras@gmail.com>

Mercurial might convert the URL to something more appropriate, like an
absolute path. Lets store that instead of the original URL, which won't
work from a different working directory if it's relative.

Suggested-by: Max Horn <max@quendi.de>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Max Horn <max@quendi.de>
---
For a discussion of the problem, see also
  http://article.gmane.org/gmane.comp.version-control.git/210250
While I am not quite happy with using "git config" to solve it, there
doesn't seem to be a better way right now.

 contrib/remote-helpers/git-remote-hg | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index c700600..7c74d8b 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -720,6 +720,14 @@ def do_export(parser):
     if peer:
         parser.repo.push(peer, force=False)
 
+def fix_path(alias, repo, orig_url):
+    repo_url = util.url(repo.url())
+    url = util.url(orig_url)
+    if str(url) == str(repo_url):
+        return
+    cmd = ['git', 'config', 'remote.%s.url' % alias, "hg::%s" % repo_url]
+    subprocess.call(cmd)
+
 def main(args):
     global prefix, dirname, branches, bmarks
     global marks, blob_marks, parsed_refs
@@ -766,6 +774,9 @@ def main(args):
     repo = get_repo(url, alias)
     prefix = 'refs/hg/%s' % alias
 
+    if not is_tmp:
+        fix_path(alias, peer or repo, url)
+
     if not os.path.exists(dirname):
         os.makedirs(dirname)
 
-- 
1.8.0.1.525.gaaf5ad5

Re: [PATCH] remote-hg: store converted URL

[Junio C Hamano]

Max Horn <max@quendi.de> writes:

> From: Felipe Contreras <felipe.contreras@gmail.com>
>
> Mercurial might convert the URL to something more appropriate, like an
> absolute path.

"What it is converted *TO*" is fairly clear with ", like an ...",
but from the first reading it was unclear to me "what it is
converted *FROM*" and "*WHEN* the conversion happens".  Do you mean
that the user gives "git clone" an URL "../hg-repo" via the command
line (e.g. the argument to "git clone" is spelled something like
"hg::../hg-repo"), and that "../hg-repo" is rewritten to something
else (an absolute path, e.g. "/srv/project/hg-repo")?

> Lets store that instead of the original URL, which won't
> work from a different working directory if it's relative.

What is lacking from this description is why it even needs to work
from a different working directory.  I am guessing that remote-hg
later creates a hidden Hg repository or something in a different
place and still tries to use the URL to interact with the upstream,
and that is what breaks, but with only the above description without
looking at your original report, people who will read the "git log"
output and find this change will not be able to tell why this was
needed, I am afraid.

Of course, the above guess of mine may even be wrong, but then that
is yet another reason that the log needs to explain the change
better.

> Suggested-by: Max Horn <max@quendi.de>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Max Horn <max@quendi.de>
> ---
> For a discussion of the problem, see also
>   http://article.gmane.org/gmane.comp.version-control.git/210250

I do not see any discussion; only your problem report.

Was this work done outside the list?  I just want to make sure this
patch is not something Felipe did not want to sign off for whatever
reason but you are passing it to the list as a patch signed off by
him.

Re: [PATCH] remote-hg: store converted URL

[Max Horn]

On 14.01.2013, at 19:14, Junio C Hamano wrote:

> Max Horn <max@quendi.de> writes:
> 
>> From: Felipe Contreras <felipe.contreras@gmail.com>
>> 
>> Mercurial might convert the URL to something more appropriate, like an
>> absolute path.
> 
> "What it is converted *TO*" is fairly clear with ", like an ...",
> but from the first reading it was unclear to me "what it is
> converted *FROM*" and "*WHEN* the conversion happens".  Do you mean
> that the user gives "git clone" an URL "../hg-repo" via the command
> line (e.g. the argument to "git clone" is spelled something like
> "hg::../hg-repo"), and that "../hg-repo" is rewritten to something
> else (an absolute path, e.g. "/srv/project/hg-repo")?

Yes, that was meant. 

> 
>> Lets store that instead of the original URL, which won't
>> work from a different working directory if it's relative.
> 
> What is lacking from this description is why it even needs to work
> from a different working directory.  I am guessing that remote-hg
> later creates a hidden Hg repository or something in a different
> place and still tries to use the URL to interact with the upstream,
> and that is what breaks, but with only the above description without
> looking at your original report, people who will read the "git log"
> output and find this change will not be able to tell why this was
> needed, I am afraid.
> 
> Of course, the above guess of mine may even be wrong, but then that
> is yet another reason that the log needs to explain the change
> better.

Fully agreed. How about this commit message:

-- >8 --
remote-hg: store converted URL of hg repo in git config

When remote-hg is invoked, read the remote repository URL from the git config,
give Mercurial a chance to expand it, and if changed, store it back into
the git config.

This fixes the following problem: Suppose you clone a local hg repository
using a relative path, e.g.
  git clone hg::hgrepo gitrepo
This stores "hg::hgrepo" in gitrepo/.git/config. However, no information
about the PWD is stored, making it impossible to correctly interpret the
relative path later on. Thus when latter attempting to, say, "git pull"
from inside gitrepo, remote-hg cannot resolve the relative path correctly,
and the user sees an unexpected error.

With this commit, the URL "hg::hgrepo" gets expanded (during cloning,
but also during any other remote operation) and the resulting absolute
URL (e.g. "hg::/abspath/hgrepo") is stored in gitrepo/.git/config.
Thus the git clone of hgrepo becomes usable.
-- >8 --

> 
>> Suggested-by: Max Horn <max@quendi.de>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> Signed-off-by: Max Horn <max@quendi.de>
>> ---
>> For a discussion of the problem, see also
>>  http://article.gmane.org/gmane.comp.version-control.git/210250
> 
> I do not see any discussion; only your problem report.

Aha, an english language issue on my side I guess: For me, a single person can "discuss" a problem (often, a research paper is said to be "discussing a problem"). Sorry for that. Anyway, the reason I gave that link was because it attempts explains the problem and one solution (which this patch ended up implementing), but also express that I feel a bit uncomfortable with this. Which I still do. Relying on the remote helper to invoke "git config" feels like a hack and I was wondering whether this is deemed an acceptable solution -- or whether one should instead extend the remote-helper protocol, allowing the remote helper to signal a rewritten remote URL (perhaps only directly after a clone?). As it is, the remote helper seems (?) to have no way to distinguish whether it is being called duri
 ng a clone or a pull; hence it has to "expand" and rewrite the URL every time it is called, just in case.


Anyway, as long as this particular command works somehow, I am fine:

  git clone hg::../relative/path/to/hg-repo  git-repo


> Was this work done outside the list?  I just want to make sure this
> patch is not something Felipe did not want to sign off for whatever
> reason but you are passing it to the list as a patch signed off by
> him.

The work was done by Felipe's and published in his github repository:
  https://github.com/felipec/git/commit/605bad5b52d2fcf3d8f5fd782a87d7c97d1b040a
See also the discussion (yeah, this time a real one ;-) leading to this:
  https://github.com/felipec/git/issues/2

I took his sign-off from there and interpreted it as saying that Felipe was OK with this being pushed to git.git. But perhaps this is not what I should have done? In that case I am very sorry :-(. It's just that I feel this patch is quite useful and important for daily use (which is why I suggested it in the first place ;-), so I was/am quite eager to see it in.

Cheers,
Max


PS: recently, yet another tool has (re)emerged for using hg repos from inside git:
  https://github.com/buchuki/gitifyhg
This is partially based on Felipe's work, but has several bug fixes atop that. It is also seems to be a priority for its author, so it os more actively developed... anyway, that's now, what, "solution" #5 or #6? I really hope the dust on this will settle soon and we'll have just one (or maybe two) tools doing a decent job, instead of attention splitting over so many different ones...

Re: [PATCH] remote-hg: store converted URL

[Junio C Hamano]

Max Horn <max@quendi.de> writes:

> On 14.01.2013, at 19:14, Junio C Hamano wrote:
>
>> What is lacking from this description is why it even needs to work
>> from a different working directory....

In your rewrite below, this is still lacking, I think.

> Fully agreed. How about this commit message:
>
> -- >8 --
> remote-hg: store converted URL of hg repo in git config
>
> When remote-hg is invoked, read the remote repository URL from the git config,
> give Mercurial a chance to expand it, and if changed, store it back into
> the git config.
>
> This fixes the following problem: Suppose you clone a local hg repository
> using a relative path, e.g.
>   git clone hg::hgrepo gitrepo
> This stores "hg::hgrepo" in gitrepo/.git/config. However, no information
> about the PWD is stored, making it impossible to correctly interpret the
> relative path later on.

Here, you say "correctly interpret relative path later on", but it
is not made clear why it is even necessary.  And that makes ...

> Thus when latter attempting to, say, "git pull"
> from inside gitrepo, remote-hg cannot resolve the relative path correctly,
> and the user sees an unexpected error.

... "cannot resolve the relative path correctly" above sound like a
bug in remote-hg.  Something like:

    Cloning a local hg repository using a relative path, e.g.

      git clone hg::hgrepo gitrepo

    stores "hg::hgrepo" in gitrepo/.git/config as its URL.  When
    remote-hg is invoked by "git fetch", it chdirs to X (which is
    different from the "gitrepo" directory) and uses the URL (which
    is not correct, as it is a relative path but the cwd is
    different when it is used) to interact with the original
    "hgrepo", which will fail.

is needed, but you didn't explain what that X is.  Perhaps it is a
temporary directory.  Perhaps it is a hidden Hg repository somewhere
in gitrepo/.git directory.  Or something else.

With that explained ...

> With this commit, the URL "hg::hgrepo" gets expanded (during cloning,
> but also during any other remote operation) and the resulting absolute
> URL (e.g. "hg::/abspath/hgrepo") is stored in gitrepo/.git/config.
> Thus the git clone of hgrepo becomes usable.

... the description of the fix start making sense, but not without.

>> Was this work done outside the list?  I just want to make sure this
>> patch is not something Felipe did not want to sign off for whatever
>> reason but you are passing it to the list as a patch signed off by
>> him.
>
> The work was done by Felipe's and published in his github repository:
>   https://github.com/felipec/git/commit/605bad5b52d2fcf3d8f5fd782a87d7c97d1b040a
> See also the discussion (yeah, this time a real one ;-) leading to this:
>   https://github.com/felipec/git/issues/2
>
> I took his sign-off from there and interpreted it as saying that
> Felipe was OK with this being pushed to git.git. But perhaps this
> is not what I should have done?

You did nothing wrong, other than not having given the necessary
context to understand how the change flowed here and it is kosher.

Which you now have, so it is OK.

Thanks.

Re: [PATCH] remote-hg: store converted URL

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Max Horn <max@quendi.de> writes:
> ...
>> See also the discussion (yeah, this time a real one ;-) leading to this:
>>   https://github.com/felipec/git/issues/2
>> ...

If I understand correctly, the $backend::$opaqueToken is a contract
between the remote-helper and the remote-$backend that says "When
user wants to interact with the same (foreign) repository, we agreed
to let her use 'origin' nickname.  The remote-helper looks up this
opaque token that corresponds to 'origin' and gives it to the
remote-$backend, and whatever is in the opaque token should be
sufficient for the remote-$backend to figure out how to proceed from
there".

But in this hg::../over/there case, it seems that string is not
sufficient for remote-hg to do so and the contract is broken.

When "git clone $backend::$opaqueToken repo" is run in /dir/ecto/ry,
and then subsequent "git fetch origin" will be run in (a
subdirectory of) /dir/ecto/ry/repo, but anything relative to
/dir/ecto/ry will not work once you go inside /dir/ecto/ry/repo.
The "create a new repository here" argument could even be an
absolute path to a totally different place, so if the
remote-$backend wants to use $opaqueToken as anything relative to
the $(cwd) when "git clone" was invoked, that original location
needs to be available somehow.

Would a new helper protocol message be necessary, so that the
backend can rewrite the $opaqueToken at "clone" time and tell the
helper what to store as URL instead of the original?  I do not think
that is much different from remote-$backend updating the value of the
remote.origin.URL using "git config".

An alternative approach may be for somebody (either the "git clone"
or the remote-$backend) to store a "base directory" when "git clone"
was invoked in remote.origin.dirAtCloneTime variable, so that the
next time remote-$backend runs, it can read that directory and
interpret the $opaqueToken as a relative path to that directory if
it wants to.  That way, nobody needs to rewrite $opaqueToken.

How do other remote helpers solve this, I have to wonder, though.
By not allowing relative paths to a directory?

Re: [PATCH] remote-hg: store converted URL

[Max Horn]

On 15.01.2013, at 17:05, Junio C Hamano wrote:

> Max Horn <max@quendi.de> writes:
> 
>> On 14.01.2013, at 19:14, Junio C Hamano wrote:
>> 
>>> What is lacking from this description is why it even needs to work
>>> from a different working directory....
> 
> In your rewrite below, this is still lacking, I think.

Hm, I thought I made it clear: It has to change because relative paths only make sense when you know the reference point they are relative with.

Typically. This is the pwd. But when I 

  git clone repo newrepo
  cd newrepo

I just changed the PWD. The clone command was given the relative path "repo". If git were to use that, it would suddenly refer to a directory inside newrepo, not next to it. Bang. Hence, git expands the relative path to an absolute one in the above example.

But git cannot do that for URLs in the form HELPER::PATH, because such a string is necessarily opaque to git.

<snip>

>> Thus when latter attempting to, say, "git pull"
>> from inside gitrepo, remote-hg cannot resolve the relative path correctly,
>> and the user sees an unexpected error.
> 
> ... "cannot resolve the relative path correctly" above sound like a
> bug in remote-hg.  Something like:
> 
>    Cloning a local hg repository using a relative path, e.g.
> 
>      git clone hg::hgrepo gitrepo
> 
>    stores "hg::hgrepo" in gitrepo/.git/config as its URL.  When
>    remote-hg is invoked by "git fetch", it chdirs to X (which is
>    different from the "gitrepo" directory) and uses the URL (which
>    is not correct, as it is a relative path but the cwd is
>    different when it is used) to interact with the original
>    "hgrepo", which will fail.
> 
> is needed, but you didn't explain what that X is.  Perhaps it is a
> temporary directory.  Perhaps it is a hidden Hg repository somewhere
> in gitrepo/.git directory.  Or something else.

None of the above. Nor does the remote helper chdir anywhere. It is the user who has done the chdir: Away from the location he invoked "git clone" at, and into the new repository directory that previously did not even exist.



Cheers,
Max

Re: [PATCH] remote-hg: store converted URL

[Max Horn]

On 15.01.2013, at 17:51, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Max Horn <max@quendi.de> writes:
>> ...
>>> See also the discussion (yeah, this time a real one ;-) leading to this:
>>>  https://github.com/felipec/git/issues/2
>>> ...
> 
> If I understand correctly, the $backend::$opaqueToken is a contract
> between the remote-helper and the remote-$backend

Just to clarify: What is the "remote-helper" ? So far, I thought of this being a contract between git (or some part of git), and remote-$backend (i.e. remote-hg in this case). 


> that says "When
> user wants to interact with the same (foreign) repository, we agreed
> to let her use 'origin' nickname.  The remote-helper looks up this
> opaque token that corresponds to 'origin' and gives it to the
> remote-$backend, and whatever is in the opaque token should be
> sufficient for the remote-$backend to figure out how to proceed from
> there".

Supposing that I interpret "remote-helper" correctly, that sounds about right to me.

> 
> But in this hg::../over/there case, it seems that string is not
> sufficient for remote-hg to do so and the contract is broken.

One could put it that way.

> 
> When "git clone $backend::$opaqueToken repo" is run in /dir/ecto/ry,
> and then subsequent "git fetch origin" will be run in (a
> subdirectory of) /dir/ecto/ry/repo, but anything relative to
> /dir/ecto/ry will not work once you go inside /dir/ecto/ry/repo.
> The "create a new repository here" argument could even be an
> absolute path to a totally different place, so if the
> remote-$backend wants to use $opaqueToken as anything relative to
> the $(cwd) when "git clone" was invoked, that original location
> needs to be available somehow.

That would be one option. Another is to do what the proposed patch does, and what git itself does: Change the relative path into an absolute one. This requires remote-$backend to be able to modify the opaque token supplied by the user.

Yet another would be to be more strict in remote-$backend as to which opaque tokens to accept: When it contains a relative path, simply always refuse to work, even if the PWD happens to be set the right way. Of course this would be quite undesirable from a user's perspective.


> 
> Would a new helper protocol message be necessary, so that the
> backend can rewrite the $opaqueToken at "clone" time and tell the
> helper what to store as URL instead of the original?  I do not think
> that is much different from remote-$backend updating the value of the
> remote.origin.URL using "git config".
> 
> An alternative approach may be for somebody (either the "git clone"
> or the remote-$backend) to store a "base directory" when "git clone"
> was invoked in remote.origin.dirAtCloneTime variable, so that the
> next time remote-$backend runs, it can read that directory and
> interpret the $opaqueToken as a relative path to that directory if
> it wants to.  That way, nobody needs to rewrite $opaqueToken.

As I said above, this would be an option. However, I would prefer rewriting the $opaqueToken, as that would be closer to what git does for "native" tokens passed to "git clone" Specifically, I am talking about get_repo_path() in builtin/clone.c which is called by cmd_clone. What this does is in my eyes essentially the equivalent of what the patch discussed here is doing.

Anyway, at the end of the day, I mainly care about relative paths working, somehow :-). But I think it would be important to make the issue easy to resolve for all remote-$backend authors, as many of them are affected (see below).

> 
> How do other remote helpers solve this, I have to wonder, though.
> By not allowing relative paths to a directory?

So far, all I look at do not deal with this at all. Any attempts to deal with it should be pretty easy to recognize: The remote-$backend would have to store something into the git config, or else, verify the opaque token and refuse to work with it under certain conditions (e.g. when it contains a relative path). But they don't. E.g. git-remote-testgit has the exact same problem. Doing

   git init repo && cd repo && echo a > a && git add a && git ci -m a a
   git clone testgit::repo clone
   cd clone

results in a .git/config file containing

[remote "origin"]
	url = testgit::repo
	fetch = +refs/heads/*:refs/remotes/origin/*

Trying to do a "git push" from within the clone then gives this:

fatal: 'repo/.git' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
Traceback (most recent call last):
  File "/usr/local/libexec/git-core/git-remote-testgit", line 272, in <module>
    sys.exit(main(sys.argv))
  File "/usr/local/libexec/git-core/git-remote-testgit", line 261, in main
    repo = get_repo(alias, url)
  File "/usr/local/libexec/git-core/git-remote-testgit", line 39, in get_repo
    repo.get_revs()
  File "/usr/local/lib/python2.7/site-packages/git_remote_helpers/git/repo.py", line 59, in get_revs
    check_call(args, stdout=ofile)
  File "/usr/local/lib/python2.7/site-packages/git_remote_helpers/util.py", line 174, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git', 'ls-remote', 'repo/.git']' returned non-zero exit status 128




Cheers,
Max

Re: [PATCH] remote-hg: store converted URL

[Junio C Hamano]

Max Horn <max@quendi.de> writes:

> So far, all I look at do not deal with this at all. Any attempts
> to deal with it should be pretty easy to recognize: The
> remote-$backend would have to store something into the git config,
> or else, verify the opaque token and refuse to work with it under
> certain conditions (e.g. when it contains a relative path). But
> they don't. E.g. git-remote-testgit has the exact same problem.

Thanks for confirming what I suspected.  I think the way Felipe's
patch makes remote-hg take responsibility of how $opaqueToken should
look like for future invocations is the simplest and makes the most
sense.  We could try to go fancier and end up over-engineering, but
I'd rather have a simple fix in the tree first.

Thanks.