Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
On Sat, Aug 10, 2013 at 12:18 AM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@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... We would be doing better than os.path.expanduser(), because if Mercurial somehow decided to treat ~ differently, our code would still work just fine. If we do os.path.expanduser(), then we are not outsourcing anything, we are taking ownership and fixing the path by ourselves and dealing with all the consequences. 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. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
Felipe Contreras felipe.contre...@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?
It outsources the determination of is this a special notation to
name a home directory? logic to .isabs(.expanduser()) instead of
doing a .beginswith('~') ourselves.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
On Sat, Aug 10, 2013 at 1:50 AM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@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. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
On Sat, Aug 10, 2013 at 9:07 AM, Felipe Contreras felipe.contre...@gmail.com wrote: On Sat, Aug 10, 2013 at 1:50 AM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@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. OK Cool, Thank you both, -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
Antoine Pelisse apeli...@gmail.com writes: 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 apeli...@gmail.com --- On Mon, Aug 5, 2013 at 10:30 PM, Felipe Contreras felipe.contre...@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. 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? 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
On Fri, Aug 9, 2013 at 8:49 PM, Junio C Hamano gits...@pobox.com wrote: Antoine Pelisse apeli...@gmail.com writes: On Mon, Aug 5, 2013 at 10:30 PM, Felipe Contreras felipe.contre...@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. It looks like mercurial will expand the tilde (it it starts with it): hg init \~ will create a $HOME/.hg. (while git init \~ will create ./~). 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. So what should we do ? I think we should stick as close as possible to Hg behavior: That is consider that a path starting with tilde is absolute, and not try to fix it by building /home/user/~/repo/path. Of course if we could not depend on I think Hg works like that, it would be better if we could resolve that by asking Mercurial. I will dig into it. 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). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
Antoine Pelisse apeli...@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)? Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
Confusion everywhere :-) On Fri, Aug 9, 2013 at 10:53 PM, Junio C Hamano gits...@pobox.com wrote: Antoine Pelisse apeli...@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: If you run: git clone hg::./my_repo my_new_repo The remote path will be hg::./my_repo, so we have to fix this path (otherwise you won't be able to run git fetch from inside my_new_repo). It's currently done by checking if ./my_repo is an absolute path or not, and try to make it absolute if required. 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. What the current patch does, is to expand the tilde before checking if the path is absolute. So that fixes the bug, but that indeed can be confusing to another user that would expect hg::~/my_repo/ to *not be* hg::$HOME/my_repo (because he knows the expansion should not happen in that case). 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)? I have 2 patch (1 from me, 1 from Felipe): One with the tilde expansion, the other one with shared_path initialization (which now conflicts with the resend from Felipe) I will try to provide a better versioning of the patches next time. Sorry for the confusion, Thanks, -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
Antoine Pelisse apeli...@gmail.com writes: 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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
On Fri, Aug 9, 2013 at 11:45 PM, Junio C Hamano gits...@pobox.com wrote: 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. Thanks -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
On Fri, Aug 9, 2013 at 4:19 PM, Antoine Pelisse apeli...@gmail.com wrote: Confusion everywhere :-) On Fri, Aug 9, 2013 at 10:53 PM, Junio C Hamano gits...@pobox.com wrote: Antoine Pelisse apeli...@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: 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
Felipe Contreras felipe.contre...@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. Presumably this git config is to fix what is already there, and in the case where the path is already absolute (e.g. /home/ap/hgrepo as opposed to ~ap/hgrepo) the resulting repository has a correct value for the variable set already without the need to fix it (that is why the original code just returns from the function), so doing the same for ~whom case with this patch should leave the setting, which presumably is hg::~ap/hgrepo? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
On Fri, Aug 9, 2013 at 5:15 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
Felipe Contreras felipe.contre...@gmail.com writes: On Fri, Aug 9, 2013 at 5:15 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
On Fri, Aug 9, 2013 at 6:39 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: On Fri, Aug 9, 2013 at 5:15 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@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. I was thinking something like this: if url.scheme != 'file' or os.path.isabs(url.path) or url.path[0] == '~': return -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
Felipe Contreras felipe.contre...@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... -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
On Mon, Aug 5, 2013 at 3:12 PM, Antoine Pelisse apeli...@gmail.com wrote: 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. Shouldn't that be the job of the shell? (s/~/$HOME/) -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix path when cloning with tilde expansion
On Mon, Aug 5, 2013 at 10:30 PM, Felipe Contreras felipe.contre...@gmail.com wrote: On Mon, Aug 5, 2013 at 3:12 PM, Antoine Pelisse apeli...@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/) I guess it is, as long as it looks like a path: $ echo ~ /home/myuser $ echo hg::~ hg::~ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
