patchremote-hg: fix path when cloning with tilde expansion

> The current code fixes the path to make it absolute when cloning, but
> doesn't consider tilde expansion, so that scenario fails throwing an
> exception because /home/myuser/~/my/repository doesn't exists:
>
>     $ git clone hg::~/my/repository && cd repository && git fetch
>
> Fix that by using python os.path.expanduser method.

> On Mon, Aug 5, 2013 at 3:12 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
>>     $ git clone hg::~/my/repository && cd repository && git fetch
>>
>> Fix that by using python os.path.expanduser method.
>
> Shouldn't that be the job of the shell? (s/~/$HOME/)

> The current code fixes the path to make it absolute when cloning, but
> doesn't consider tilde expansion, so that scenario fails throwing an
> exception because /home/myuser/~/my/repository doesn't exists:
>
>     $ git clone hg::~/my/repository && cd repository && git fetch
>
> Expand the tilde when checking if the path is absolute, so that we don't
> fix a path that doesn't need to be.
>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
> On Mon, Aug 5, 2013 at 10:30 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> Shouldn't that be the job of the shell? (s/~/$HOME/)
>
> I'm not sure what you mean here. Does it mean that I should stop cloning using "~" ?

> I also send this patch as I think it makes more sense to keep the
> ~ in the path, but just make sure we don't build invalid absolute
> path.
>
> By the way, I don't exactly understand why:
>
>     abs_url = urlparse.urljoin("%s/" % os.getcwd(), orig_url)
>
> is done right after instead of:
>
>     abs_url = os.path.abspath(orig_url)

> Cheers,
> Antoine
>
>  contrib/remote-helpers/git-remote-hg |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
> index 1897327..861c498 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -1135,7 +1135,7 @@ def do_option(parser):
>
>  def fix_path(alias, repo, orig_url):
>      url = urlparse.urlparse(orig_url, 'file')
> -    if url.scheme != 'file' or os.path.isabs(url.path):
> +    if url.scheme != 'file' or os.path.isabs(os.path.expanduser(url.path)):
>          return
>      abs_url = urlparse.urljoin("%s/" % os.getcwd(), orig_url)
>      cmd = ['git', 'config', 'remote.%s.url' % alias, "hg::%s" % abs_url]
> --
> 1.7.9.5

> Antoine Pelisse <apelisse@gmail.com> writes:
>> On Mon, Aug 5, 2013 at 10:30 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>>> Shouldn't that be the job of the shell? (s/~/$HOME/)
>>
>> I'm not sure what you mean here. Does it mean that I should stop cloning using "~" ?
>
> I think shells do not expand ~ when it appears in a string (e.g. hg::~/there);
> you could work it around with
>
>         git clone hg::$(echo ~/there)
>
> and I suspect that is what Felipe is alluding to.  A tool (like
> remote-hg bridge with this patch) that expands ~ in the middle of a
> string also may be surprising to some people, especially to those
> who know the shell does not.

> So when we run:
>
> git clone hg::~/my/repo
>
> Git will remove the "hg::" part, and Mercurial will expand tilde and
> clone $HOME/my/repo.

>> By the way, you earlier sent an updated 1/2; is this supposed to be
>> 2/2 to conclude the two-patch series?
>
> Those two patches don't interact with each other, but you can of
> course join them if it makes it easier for you (and I don't think one
> is going to have to go "faster" than the other anyway).

> Antoine Pelisse <apelisse@gmail.com> writes:
>
>> So when we run:
>>
>> git clone hg::~/my/repo
>>
>> Git will remove the "hg::" part, and Mercurial will expand tilde and
>> clone $HOME/my/repo.
>
> Now you confused me.  If the implementation were for us to remove
> the hg:: prefix and let Mercurial do whatever it wants to do with
> the rest, you are right that we will not have to do any expansion
> like your patch.  But you sent a patch to do so, so apparently it
> is not what happens.  So where does it go wrong?
>
> Puzzled...

>>> By the way, you earlier sent an updated 1/2; is this supposed to be
>>> 2/2 to conclude the two-patch series?
>>
>> Those two patches don't interact with each other, but you can of
>> course join them if it makes it easier for you (and I don't think one
>> is going to have to go "faster" than the other anyway).
>
> Hmph, so there is a different 2/2 that we haven't seen recently on
> the list (meaning you have three patches)?

> OK, I think I see why you are puzzled.
> ...
> But my issue is when I do that:
>
>     git clone hg::~/my_repo my_new_repo
>
> The clone works successfully by cloning $HOME/my_repo, but then, when
> we try to fix the repo path, we think that ~/my_repo is not an
> absolute path, so we make it absolute: /home/user/~/my_repo which is
> now off. So I'm not able to fetch that remote.

> OK, so clone works, but subsequent fetch from the cloned resoitory
> does not?  "git fetch hg::~/my_repo" will still work but the call to
> "git config" done near the place your patch touches does not store
> "hg::~/my_repo" because it thinks "~/my_repo" refers to
> "./~/my_repo" and tries to come up with an absolute path.  The patch
> tries to notice this case and return without rewriting, so that
> remote.*.url is kept as "hg::~/my_repo".
>
> Assuming that I am following your reasoning so far, I think I can
> agree with the patch (not that my agreement matters that much, as
> you seem to be a lot more familiar with this codepath).
>
> Thanks for explaining.

> Confusion everywhere :-)
>
> On Fri, Aug 9, 2013 at 10:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Antoine Pelisse <apelisse@gmail.com> writes:
>>
>>> So when we run:
>>>
>>> git clone hg::~/my/repo
>>>
>>> Git will remove the "hg::" part, and Mercurial will expand tilde and
>>> clone $HOME/my/repo.
>>
>> Now you confused me.  If the implementation were for us to remove
>> the hg:: prefix and let Mercurial do whatever it wants to do with
>> the rest, you are right that we will not have to do any expansion
>> like your patch.  But you sent a patch to do so, so apparently it
>> is not what happens.  So where does it go wrong?
>>
>> Puzzled...
>
> OK, I think I see why you are puzzled.
>
> Cloning works fine because we "fix the path" *after* the clone is done
> successfully, for the following reason:

>> OK, I think I see why you are puzzled.
>>
>> Cloning works fine because we "fix the path" *after* the clone is done
>> successfully, for the following reason:
>
> So if we didn't store a different path, it would work. So instead of
> expanding '~' ourselves, it would be better to don't expand anything,
> and leave it as it is, but how to detect that in fix_path()?

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> OK, I think I see why you are puzzled.
>>>
>>> Cloning works fine because we "fix the path" *after* the clone is done
>>> successfully, for the following reason:
>>
>> So if we didn't store a different path, it would work. So instead of
>> expanding '~' ourselves, it would be better to don't expand anything,
>> and leave it as it is, but how to detect that in fix_path()?
>
> I think that the patch relies on that os.path.expanduser(), if
> url.path is such a path that begins with "~" (or "~whom"), returns
> an absolute path.  When given an absolute path, or "~whom/path",
> fix_path returns without running 'git config' on remote.<alias>.url
> configuration.

> On Fri, Aug 9, 2013 at 5:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>>> OK, I think I see why you are puzzled.
>>>>
>>>> Cloning works fine because we "fix the path" *after* the clone is done
>>>> successfully, for the following reason:
>>>
>>> So if we didn't store a different path, it would work. So instead of
>>> expanding '~' ourselves, it would be better to don't expand anything,
>>> and leave it as it is, but how to detect that in fix_path()?
>>
>> I think that the patch relies on that os.path.expanduser(), if
>> url.path is such a path that begins with "~" (or "~whom"), returns
>> an absolute path.  When given an absolute path, or "~whom/path",
>> fix_path returns without running 'git config' on remote.<alias>.url
>> configuration.
>
> I think ~whom/path would run 'git config'.

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Fri, Aug 9, 2013 at 5:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>>> OK, I think I see why you are puzzled.
>>>>>
>>>>> Cloning works fine because we "fix the path" *after* the clone is done
>>>>> successfully, for the following reason:
>>>>
>>>> So if we didn't store a different path, it would work. So instead of
>>>> expanding '~' ourselves, it would be better to don't expand anything,
>>>> and leave it as it is, but how to detect that in fix_path()?
>>>
>>> I think that the patch relies on that os.path.expanduser(), if
>>> url.path is such a path that begins with "~" (or "~whom"), returns
>>> an absolute path.  When given an absolute path, or "~whom/path",
>>> fix_path returns without running 'git config' on remote.<alias>.url
>>> configuration.
>>
>> I think ~whom/path would run 'git config'.
>
> Hmph, do you mean the third example of this?
>
>         $ python
>         >>> import os
>         >>> os.path.expanduser("~/repo")
>         '/home/junio/repo'
>         >>> os.path.expanduser("~junio/repo")
>         '/home/junio/repo'
>         >>> os.path.expanduser("~felipe/repo")
>         '~felipe/repo'
>
> which will give "~felipe/repo" that is _not_ an absolute repository
> because no such user exists on this box?
>
> It is true that in that case fix_path() will not return early and
> will throw a bogus path at "git config", but if the "~whom" does not
> resolve to an existing home directory of a user, I am not sure what
> we can do better than what Antoine's patch does.

>> Hmph, do you mean the third example of this?
>>
>>         $ python
>>         >>> import os
>>         >>> os.path.expanduser("~/repo")
>>         '/home/junio/repo'
>>         >>> os.path.expanduser("~junio/repo")
>>         '/home/junio/repo'
>>         >>> os.path.expanduser("~felipe/repo")
>>         '~felipe/repo'
>>
>> which will give "~felipe/repo" that is _not_ an absolute repository
>> because no such user exists on this box?
>>
>> It is true that in that case fix_path() will not return early and
>> will throw a bogus path at "git config", but if the "~whom" does not
>> resolve to an existing home directory of a user, I am not sure what
>> we can do better than what Antoine's patch does.
>
> I was thinking something like this:
>
> if url.scheme != 'file' or os.path.isabs(url.path) or url.path[0] == '~':
>   return

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> Hmph, do you mean the third example of this?
>>>
>>>         $ python
>>>         >>> import os
>>>         >>> os.path.expanduser("~/repo")
>>>         '/home/junio/repo'
>>>         >>> os.path.expanduser("~junio/repo")
>>>         '/home/junio/repo'
>>>         >>> os.path.expanduser("~felipe/repo")
>>>         '~felipe/repo'
>>>
>>> which will give "~felipe/repo" that is _not_ an absolute repository
>>> because no such user exists on this box?
>>>
>>> It is true that in that case fix_path() will not return early and
>>> will throw a bogus path at "git config", but if the "~whom" does not
>>> resolve to an existing home directory of a user, I am not sure what
>>> we can do better than what Antoine's patch does.
>>
>> I was thinking something like this:
>>
>> if url.scheme != 'file' or os.path.isabs(url.path) or url.path[0] == '~':
>>   return
>
> That did cross my mind.
>
> I know ~/ and ~who/ are expanded on UNIXy systems, and I read in
> Python documentation that Python on Windows treats ~/ and ~who/ the
> same way as on UNIXy systems, so the "begins with ~" test would work
> on both systems.  But it is probably a better design to outsource
> that knowledge to os.path.expanduser(), with the emphasis on "os."
> part of that function.  That way, we do not even have to care about
> such potential platform specifics, which is a big plus.  The only
> possible difference that approach makes is the above example of
> naming a non-existent ~user, but that will not work anyway, so...

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> If I clone ~/git, and then change my username, and move my home
>> directory, doing a 'git fetch' in ~/git wouldn't work anymore, because
>> we have expanded the path and fixed it to my old home, if instead we
>> simply return without fixing, it would still work just fine.
>
> Antoine's patch runs expanduser() only to see if the given one gets
> modified to absolute path, and makes fix_path() return without
> calling the extra 'git config', so it is my understanding that the
> above describes exactly what the patch does.  Am I reading the patch
> incorrectly?

> On Sat, Aug 10, 2013 at 1:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> If I clone ~/git, and then change my username, and move my home
>>> directory, doing a 'git fetch' in ~/git wouldn't work anymore, because
>>> we have expanded the path and fixed it to my old home, if instead we
>>> simply return without fixing, it would still work just fine.
>>
>> Antoine's patch runs expanduser() only to see if the given one gets
>> modified to absolute path, and makes fix_path() return without
>> calling the extra 'git config', so it is my understanding that the
>> above describes exactly what the patch does.  Am I reading the patch
>> incorrectly?
>
> Antoine's *second* patch, which I missed, does that, yeah. That should
> work fine.