* git commit/push can fail silently when clone omits ".git" @ 2012-11-04 19:50 Jeffrey S. Haemer 2012-11-08 18:56 ` Jeff King 0 siblings, 1 reply; 7+ messages in thread From: Jeffrey S. Haemer @ 2012-11-04 19:50 UTC (permalink / raw) To: Git Issues [-- Attachment #1: Type: text/plain, Size: 1213 bytes --] Ladies and Gentlemen, I'm running git 1.7.9.5 on Ubuntu 12.04.1 LTS I got bitten by what follows. Yes, it's an edge case. Yes I now understand why it does what it does. Yes the right answer is "Don't do that, Jeff." :-) Still, it took me a little time to figure out what I'd done wrong because the failure is silent, so I thought I'd document it. Perhaps there's even some way to issue an error message for cases like this. The attached test script shows the issue in detail, but here's the basic failure: $ ls hello.git $ git clone hello # *Mistake!* Succeeds, but should have cloned "hello.git" or into something else. $ cd hello; touch foo; git add foo; git commit -am"add a new file" $ git status # says I'm a rev ahead of the origin $ git push # nothing pushed $ git status # says everything's okay At this point hello/foo still exists, there's nothing to commit, git diff origin/master reports nothing, yet foo was never pushed to hello.git. HTH! -- Jeffrey Haemer <jeffrey.haemer@gmail.com> 720-837-8908 [cell], http://seejeffrun.blogspot.com [blog], http://www.youtube.com/user/goyishekop [vlog] פרייהייט? דאס איז יאַנג דינען וואָרט. [-- Attachment #2: clone-from-suffixless-gitrepo-issue.sh --] [-- Type: application/x-sh, Size: 2193 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: git commit/push can fail silently when clone omits ".git" 2012-11-04 19:50 git commit/push can fail silently when clone omits ".git" Jeffrey S. Haemer @ 2012-11-08 18:56 ` Jeff King 2012-11-09 18:42 ` Heiko Voigt 0 siblings, 1 reply; 7+ messages in thread From: Jeff King @ 2012-11-08 18:56 UTC (permalink / raw) To: Jeffrey S. Haemer; +Cc: Jens Lehmann, Heiko Voigt, Git Issues On Sun, Nov 04, 2012 at 12:50:58PM -0700, Jeffrey S. Haemer wrote: > I got bitten by what follows. Yes, it's an edge case. Yes I now understand > why it does what it does. Yes the right answer is "Don't do that, Jeff." :-) > > Still, it took me a little time to figure out what I'd done wrong because > the failure is silent, so I thought I'd document it. Perhaps there's even > some way to issue an error message for cases like this. > > The attached test script shows the issue in detail, but here's the basic > failure: > > $ ls > hello.git > $ git clone hello # *Mistake!* Succeeds, but should have cloned "hello.git" > or into something else. It does clone hello.git into "hello", but it sets remote.origin.url in the cloned repository to "/path/to/hello". I.e., to itself, rather than the correct hello.git. The reason is that "clone" sets the config from the repo name you gave it, not the path it finds on disk. The name you gave was not ambiguous at the time of clone, but it became so during the clone. I am tempted to say that we should set the config to the path we found on disk, not what the user gave us. That includes the ugly "/.git" for non-bare repos, but we should be able to safely strip that off without adding any ambiguity (i.e, it is only "foo" versus "foo.git" that is ambiguous). Unfortunately, the patch below which does that seems to make t7407 very unhappy. It looks like the submodule test uses "git clone ." and "git-submodule add" expects the "/." to still be at the end of the configured URL when processing relative submodule paths. I'm not sure if that is important, or an unnecessary brittleness in the submodule code. Jens, Heiko? --- diff --git a/builtin/clone.c b/builtin/clone.c index 0d663e3..687d5c0 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -713,8 +713,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix) repo_name = argv[0]; path = get_repo_path(repo_name, &is_bundle); - if (path) - repo = xstrdup(absolute_path(repo_name)); + if (path) { + if (!suffixcmp(path, "/.git")) + repo = xstrndup(path, strlen(path) - 5); + else + repo = xstrdup(path); + } else if (!strchr(repo_name, ':')) die(_("repository '%s' does not exist"), repo_name); else diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 67869b4..0eeeb2d 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -280,4 +280,20 @@ test_expect_success 'clone checking out a tag' ' test_cmp fetch.expected fetch.actual ' +test_expect_success 'clone does not create ambiguous config' ' + git init --bare ambiguous.git && + git clone ambiguous && + ( + cd ambiguous && + test_commit one && + git push --all + ) && + echo one >expect && + ( + cd ambiguous.git && + git log -1 --format=%s + ) >actual && + test_cmp expect actual +' + test_done ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: git commit/push can fail silently when clone omits ".git" 2012-11-08 18:56 ` Jeff King @ 2012-11-09 18:42 ` Heiko Voigt 2012-11-13 8:32 ` [PATCH 0/3] fix cloning superprojects from "." Heiko Voigt 0 siblings, 1 reply; 7+ messages in thread From: Heiko Voigt @ 2012-11-09 18:42 UTC (permalink / raw) To: Jeff King; +Cc: Jeffrey S. Haemer, Jens Lehmann, Git Issues Hi, On Thu, Nov 08, 2012 at 01:56:43PM -0500, Jeff King wrote: > Unfortunately, the patch below which does that seems to make t7407 very > unhappy. It looks like the submodule test uses "git clone ." and > "git-submodule add" expects the "/." to still be at the end of the > configured URL when processing relative submodule paths. I'm not sure if > that is important, or an unnecessary brittleness in the submodule code. > > Jens, Heiko? After some analysis it seems to me that the test deviates from the expected behavior. For relative urls we have documented that if we have a remote in the superproject a relative submodule path is relative to that remotes url. In the test super has been cloned from ".". So the tests root directory should be the directory the submodule path is relative to. That would be ./submodule (since submodule is also in the root directory) and not ../submodule. Before your patch a "/." was added to the origin of super and "/." is currently counted as a path component. So we have another corner case here: When your superproject was cloned from "." the urls you currently have to specify with submodule add are wrong (one ".." to much). Since this is a change in behaviour I would like to further think about the implications this brings if we fix this. Not sure how many people clone from ".". The correct behavior (as documented) is the one you introduce with your patch. If we decide to fix this we should also correct the path calculation in git-submodule.sh. Cheers Heiko ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 0/3] fix cloning superprojects from "." 2012-11-09 18:42 ` Heiko Voigt @ 2012-11-13 8:32 ` Heiko Voigt 2012-11-13 8:34 ` [PATCH 1/3] Fix relative submodule setup of submodule tests Heiko Voigt ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Heiko Voigt @ 2012-11-13 8:32 UTC (permalink / raw) To: Jeff King; +Cc: Jeffrey S. Haemer, Jens Lehmann, Git Issues Hi, On Fri, Nov 09, 2012 at 07:42:26PM +0100, Heiko Voigt wrote: > Since this is a change in behaviour I would like to further think about > the implications this brings if we fix this. Not sure how many people > clone from ".". The correct behavior (as documented) is the one you > introduce with your patch. If we decide to fix this we should also correct > the path calculation in git-submodule.sh. Ok I think this corner case is not that commonly used since most people work with remote remotes which you can not cd into to clone from ".". Here is a patch series to clean this handling up. Cheers Heiko Heiko Voigt (3): Fix relative submodule setup of submodule tests ensure that relative submodule url needs ./ or ../ fix corner case for relative submodule path calculation git-submodule.sh | 22 +++++++++++++++++ t/t7400-submodule-basic.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++ t/t7403-submodule-sync.sh | 2 ++ t/t7406-submodule-update.sh | 2 ++ t/t7407-submodule-foreach.sh | 2 ++ t/t7506-status-submodule.sh | 2 ++ 6 files changed, 86 insertions(+) -- 1.8.0.3.gaed4666 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/3] Fix relative submodule setup of submodule tests 2012-11-13 8:32 ` [PATCH 0/3] fix cloning superprojects from "." Heiko Voigt @ 2012-11-13 8:34 ` Heiko Voigt 2012-11-13 8:35 ` [PATCH 2/3] ensure that relative submodule url needs ./ or ../ Heiko Voigt 2012-11-13 8:35 ` [PATCH 3/3] fix corner case for relative submodule path calculation Heiko Voigt 2 siblings, 0 replies; 7+ messages in thread From: Heiko Voigt @ 2012-11-13 8:34 UTC (permalink / raw) To: Jeff King; +Cc: Jeffrey S. Haemer, Jens Lehmann, Git Issues If a remote is configured in a superproject relative submodule urls should be relative to that remote. Since we have a bug in relative path calculation for superproject paths that contain a "/." using ../submodule was accepted here. We are going to fix this behavior so we first need to correct these tests. Later tests expect the submodules origin to be in a directory underneath the tests root. Lets remove the origin from super (which points directly at the tests root directory) to keep these tests expectations. Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net> --- t/t7403-submodule-sync.sh | 2 ++ t/t7406-submodule-update.sh | 2 ++ t/t7407-submodule-foreach.sh | 2 ++ t/t7506-status-submodule.sh | 2 ++ 4 files changed, 8 insertions(+) diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh index 524d5c1..b310a58 100755 --- a/t/t7403-submodule-sync.sh +++ b/t/t7403-submodule-sync.sh @@ -18,6 +18,8 @@ test_expect_success setup ' git clone . super && git clone super submodule && (cd super && + # relative submodule urls relate to this folder not the remotes + git remote rm origin && git submodule add ../submodule submodule && test_tick && git commit -m "submodule" diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index 1542653..f3628c9 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -32,6 +32,8 @@ test_expect_success 'setup a submodule tree' ' git clone super merging && git clone super none && (cd super && + # relative submodule urls relate to this folder not the remotes + git remote rm origin && git submodule add ../submodule submodule && test_tick && git commit -m "submodule" && diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh index 9b69fe2..99956a6 100755 --- a/t/t7407-submodule-foreach.sh +++ b/t/t7407-submodule-foreach.sh @@ -21,6 +21,8 @@ test_expect_success 'setup a submodule tree' ' git clone super submodule && ( cd super && + # relative submodule urls relate to this folder not the remotes + git remote rm origin && git submodule add ../submodule sub1 && git submodule add ../submodule sub2 && git submodule add ../submodule sub3 && diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh index d31b34d..9021b1a 100755 --- a/t/t7506-status-submodule.sh +++ b/t/t7506-status-submodule.sh @@ -203,6 +203,8 @@ test_expect_success 'status with merge conflict in .gitmodules' ' test_create_repo_with_commit sub2 && ( cd super && + # relative submodule urls relate to this folder not the remotes + git remote rm origin && prev=$(git rev-parse HEAD) && git checkout -b add_sub1 && git submodule add ../sub1 && -- 1.8.0.3.gaed4666 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] ensure that relative submodule url needs ./ or ../ 2012-11-13 8:32 ` [PATCH 0/3] fix cloning superprojects from "." Heiko Voigt 2012-11-13 8:34 ` [PATCH 1/3] Fix relative submodule setup of submodule tests Heiko Voigt @ 2012-11-13 8:35 ` Heiko Voigt 2012-11-13 8:35 ` [PATCH 3/3] fix corner case for relative submodule path calculation Heiko Voigt 2 siblings, 0 replies; 7+ messages in thread From: Heiko Voigt @ 2012-11-13 8:35 UTC (permalink / raw) To: Jeff King; +Cc: Jeffrey S. Haemer, Jens Lehmann, Git Issues Even though a relative path can be without them the documentation explicitely talks about them. Lets ensure that behavior with a test. Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net> --- t/t7400-submodule-basic.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 5397037..3c2afa6 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -506,6 +506,18 @@ test_expect_success 'set up for relative path tests' ' ) ' +test_expect_success 'subrepo is NOT considered a relative path"' ' + ( + cd reltest && + cp pristine-.git-config .git/config && + cp pristine-.gitmodules .gitmodules && + git config -f .gitmodules submodule.sub.url "subrepo" && + git config remote.origin.url "$submodurl" && + git submodule init && + test "$(git config submodule.sub.url)" = subrepo + ) +' + test_expect_success '../subrepo works with URL - ssh://hostname/repo' ' ( cd reltest && -- 1.8.0.3.gaed4666 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] fix corner case for relative submodule path calculation 2012-11-13 8:32 ` [PATCH 0/3] fix cloning superprojects from "." Heiko Voigt 2012-11-13 8:34 ` [PATCH 1/3] Fix relative submodule setup of submodule tests Heiko Voigt 2012-11-13 8:35 ` [PATCH 2/3] ensure that relative submodule url needs ./ or ../ Heiko Voigt @ 2012-11-13 8:35 ` Heiko Voigt 2 siblings, 0 replies; 7+ messages in thread From: Heiko Voigt @ 2012-11-13 8:35 UTC (permalink / raw) To: Jeff King; +Cc: Jeffrey S. Haemer, Jens Lehmann, Git Issues A trailing /. for the superprojects origin is treated as a full path component. This is wrong. Lets add a test and fix this. Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net> --- git-submodule.sh | 22 ++++++++++++++++++++++ t/t7400-submodule-basic.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) diff --git a/git-submodule.sh b/git-submodule.sh index ab6b110..9f61a9c 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -69,6 +69,28 @@ resolve_relative_url () ;; esac + # strip one dot path components + tempurl="$remoteurl" + remoteurl= + sep= + while test -n "$tempurl" + do + case "$tempurl" in + */.) + tempurl="${tempurl%/.}" + ;; + ?*/*) + remoteurl="${tempurl##*/}$sep$remoteurl" + tempurl="${tempurl%/*}" + sep=/ + ;; + *) + remoteurl="$tempurl$sep$remoteurl" + tempurl= + ;; + esac + done + while test -n "$url" do case "$url" in diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 3c2afa6..1b4cc00 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -518,6 +518,50 @@ test_expect_success 'subrepo is NOT considered a relative path"' ' ) ' +test_expect_success '../subrepo works with absolute local path - "$submodurl/repo/."' ' + ( + cd reltest && + cp pristine-.git-config .git/config && + cp pristine-.gitmodules .gitmodules && + git config remote.origin.url "$submodurl/repo/." && + git submodule init && + test "$(git config submodule.sub.url)" = "$submodurl/subrepo" + ) +' + +test_expect_success '../subrepo works with absolute local path - "$submodurl/repo/./"' ' + ( + cd reltest && + cp pristine-.git-config .git/config && + cp pristine-.gitmodules .gitmodules && + git config remote.origin.url "$submodurl/repo/./" && + git submodule init && + test "$(git config submodule.sub.url)" = "$submodurl/subrepo" + ) +' + +test_expect_success '../subrepo works with absolute local path - "$submodurl/./repo/."' ' + ( + cd reltest && + cp pristine-.git-config .git/config && + cp pristine-.gitmodules .gitmodules && + git config remote.origin.url "$submodurl/./repo/." && + git submodule init && + test "$(git config submodule.sub.url)" = "$submodurl/subrepo" + ) +' + +test_expect_success '../subrepo works with absolute local path - "$submodurl/././repo/."' ' + ( + cd reltest && + cp pristine-.git-config .git/config && + cp pristine-.gitmodules .gitmodules && + git config remote.origin.url "$submodurl/././repo/." && + git submodule init && + test "$(git config submodule.sub.url)" = "$submodurl/subrepo" + ) +' + test_expect_success '../subrepo works with URL - ssh://hostname/repo' ' ( cd reltest && -- 1.8.0.3.gaed4666 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-11-13 8:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-11-04 19:50 git commit/push can fail silently when clone omits ".git" Jeffrey S. Haemer 2012-11-08 18:56 ` Jeff King 2012-11-09 18:42 ` Heiko Voigt 2012-11-13 8:32 ` [PATCH 0/3] fix cloning superprojects from "." Heiko Voigt 2012-11-13 8:34 ` [PATCH 1/3] Fix relative submodule setup of submodule tests Heiko Voigt 2012-11-13 8:35 ` [PATCH 2/3] ensure that relative submodule url needs ./ or ../ Heiko Voigt 2012-11-13 8:35 ` [PATCH 3/3] fix corner case for relative submodule path calculation Heiko Voigt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).