Re: [PATCH] remote-hg: fix path when cloning with tilde expansion

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 9, 2013, 18:49 UTC
Message-ID: <7veha266nq.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1376068387-28510-1-git-send-email-apelisse@gmail.com>

Antoine Pelisse <apelisse@gmail.com> writes:

Show 15 quoted lines

> 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 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.

Show 11 quoted lines

> 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)

That looks like a good cleanup to me, too, but I may be missing some subtle points...

By the way, you earlier sent an updated 1/2; is this supposed to be 2/2 to conclude the two-patch series?

Show 21 quoted lines

> 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