[PATCH 0/3] Fix submodule url issues

[PATCH 0/3] Fix submodule url issues

[Stefan Beller]

previous patch: http://public-inbox.org/git/20161018210623.32696-1-sbeller@google.com/

First fix our test suite in patch 2,
then patch 3 is a resend of the previous patch to normalize the configured
remote.

Thanks,
Stefan

Stefan Beller (3):
  t7506: fix diff order arguments in test_cmp
  submodule tests: replace cloning from . by "$(pwd)"
  submodule--helper: normalize funny urls

 builtin/submodule--helper.c  | 48 +++++++++++++++++++++++++++++++++-----------
 t/t0060-path-utils.sh        | 11 ++++++----
 t/t3600-rm.sh                |  1 +
 t/t7064-wtstatus-pv2.sh      |  9 ++++++---
 t/t7403-submodule-sync.sh    |  3 ++-
 t/t7406-submodule-update.sh  |  6 ++++--
 t/t7407-submodule-foreach.sh |  3 ++-
 t/t7506-status-submodule.sh  |  7 ++++---
 8 files changed, 62 insertions(+), 26 deletions(-)

-- 
2.10.1.507.g2a9098a

[PATCH 1/3] t7506: fix diff order arguments in test_cmp

[Stefan Beller]

Fix the argument order for test_cmp. When given the expected
result first the diff shows the actual output with '+' and the
expectation with '-', which is the convention for our tests.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7506-status-submodule.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index d31b34d..74cb6b8 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -260,7 +260,7 @@ test_expect_success 'diff with merge conflict in .gitmodules' '
 		cd super &&
 		git diff >../diff_actual 2>&1
 	) &&
-	test_cmp diff_actual diff_expect
+	test_cmp diff_expect diff_actual
 '
 
 test_expect_success 'diff --submodule with merge conflict in .gitmodules' '
@@ -268,7 +268,7 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '
 		cd super &&
 		git diff --submodule >../diff_submodule_actual 2>&1
 	) &&
-	test_cmp diff_submodule_actual diff_submodule_expect
+	test_cmp diff_submodule_expect diff_submodule_actual
 '
 
 test_done
-- 
2.10.1.507.g2a9098a

[PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"

[Stefan Beller]

When adding a submodule via "git submodule add <relative url>",
the relative url applies to the superprojects remote. When the
superproject was cloned via "git clone . super", the remote url
is ending with '/.'.

The logic to construct the relative urls is not smart enough to
detect that the ending /. is referring to the directory itself
but rather treats it like any other relative path, i.e.

    path/to/dir/. + ../relative/path/to/submodule

would result in

    path/to/dir/relative/path/to/submodule

and not omit the "dir" as you may expect.

As in a later patch we'll normalize the remote url before the
computation of relative urls takes place, we need to first get our
test suite in a shape with normalized urls only, which is why we should
refrain from cloning from '.'

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7064-wtstatus-pv2.sh      | 9 ++++++---
 t/t7403-submodule-sync.sh    | 3 ++-
 t/t7406-submodule-update.sh  | 6 ++++--
 t/t7407-submodule-foreach.sh | 3 ++-
 t/t7506-status-submodule.sh  | 3 ++-
 5 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index 3012a4d..95514bb 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -330,7 +330,8 @@ test_expect_success 'verify UU (edit-edit) conflict' '
 test_expect_success 'verify upstream fields in branch header' '
 	git checkout master &&
 	test_when_finished "rm -rf sub_repo" &&
-	git clone . sub_repo &&
+	git clone "$(pwd)" sub_repo &&
+	git -C sub_repo config --unset remote.origin.url &&
 	(
 		## Confirm local master tracks remote master.
 		cd sub_repo &&
@@ -392,8 +393,10 @@ test_expect_success 'verify upstream fields in branch header' '
 
 test_expect_success 'create and add submodule, submodule appears clean (A. S...)' '
 	git checkout master &&
-	git clone . sub_repo &&
-	git clone . super_repo &&
+	git clone "$(pwd)" sub_repo &&
+	git -C sub_repo config --unset remote.origin.url &&
+	git clone "$(pwd)" super_repo &&
+	git -C super_repo config --unset remote.origin.url &&
 	(	cd super_repo &&
 		git submodule add ../sub_repo sub1 &&
 
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 0726799..6d85391 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -15,7 +15,8 @@ test_expect_success setup '
 	git add file &&
 	test_tick &&
 	git commit -m upstream &&
-	git clone . super &&
+	git clone "$(pwd)" super &&
+	git -C super config --unset remote.origin.url &&
 	git clone super submodule &&
 	(
 		cd submodule &&
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 64f322c..9430c2a 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -26,7 +26,8 @@ test_expect_success 'setup a submodule tree' '
 	git add file &&
 	test_tick &&
 	git commit -m upstream &&
-	git clone . super &&
+	git clone "$(pwd)" super &&
+	git -C super config --unset remote.origin.url &&
 	git clone super submodule &&
 	git clone super rebasing &&
 	git clone super merging &&
@@ -64,7 +65,8 @@ test_expect_success 'setup a submodule tree' '
 	 test_tick &&
 	 git commit -m "none"
 	) &&
-	git clone . recursivesuper &&
+	git clone "$(pwd)" recursivesuper &&
+	git -C recursivesuper config --unset remote.origin.url &&
 	( cd recursivesuper
 	 git submodule add ../super super
 	)
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6ba5daf..257e817 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -17,7 +17,8 @@ test_expect_success 'setup a submodule tree' '
 	git add file &&
 	test_tick &&
 	git commit -m upstream &&
-	git clone . super &&
+	git clone "$(pwd)" super &&
+	git -C super config --unset remote.origin.url &&
 	git clone super submodule &&
 	(
 		cd super &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 74cb6b8..62a99bc 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -197,7 +197,8 @@ A  sub1
 EOF
 
 test_expect_success 'status with merge conflict in .gitmodules' '
-	git clone . super &&
+	git clone "$(pwd)" super &&
+	git -C super config --unset remote.origin.url &&
 	test_create_repo_with_commit sub1 &&
 	test_tick &&
 	test_create_repo_with_commit sub2 &&
-- 
2.10.1.507.g2a9098a

[PATCH 3/3] submodule--helper: normalize funny urls

[Stefan Beller]

The remote URL for the submodule can be specified relative
to the URL of the superproject in .gitmodules.  A top-level
git://site.xz/toplevel.git can specify in its .gitmodules

        [submodule "sub"]
                url = ../submodule.git
                path = sub

to say that git://site.xz/submodule.git is where the
submodule bound at its "sub/" is found.

However, when the toplevel is cloned like this:

        git clone git://site.xz/toplevel.git/. top

i.e. the toplevel specifies its URL with trailing "/.", the
code set the URL to git://site.xz/toplevel.git/submodule.git
for the submodule, which is nonsense.  This was because the
logic failed to treat trailing "/." any differently from
trailing "/<anything-without-slash>" when resolving a
relative URL "../<something>" off of it.  Stripping "/." at
the end does *not* take you one level up, even though
stripping "/<anything-without-slash>" does!

Some users may rely on this by always cloning with '/.' and having
an additional '../' in the relative path for the submodule, and this
patch breaks them. So why introduce this patch?

The fix in c12922024 (submodule: ignore trailing slash on superproject
URL, 2016-10-10) and prior discussion revealed, that Git and Git
for Windows treat URLs differently, as currently Git for Windows
strips off a trailing dot from paths when calling a Git binary
unlike when running a shell. Which means Git for Windows is already
doing the right thing for the case mentioned above, but it would fail
our current tests, that test for the broken behavior and it would
confuse users working across platforms. So we'd rather fix it
in Git to ignore any of these trailing no ops in the path properly.

We never produce the URLs with a trailing '/.' in Git ourselves,
they come to us, because the user used it as the URL for cloning
a superproject. Normalize these paths.

The test 3600 needs a slight adaption as well, and was not covered by
the previous patch, because the setup of the submodule in this test
is different than the "clone . super" pattern that was fixed in the
previous patch.

The url configured for the submodule path submod is "./.", which
gets normalized with this patch, such that the nested submodule
'subsubmod' doesn't resolve its path properly via '../'.
In later tests we do not need the url any more as testing of
removal of the config including url happens in early tests, so the
easieast fix for that test is to just make the submodule its own
authoritative source of truth by removing the remote url.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 48 +++++++++++++++++++++++++++++++++------------
 t/t0060-path-utils.sh       | 11 +++++++----
 t/t3600-rm.sh               |  1 +
 3 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5..21e2e2a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -76,6 +76,29 @@ static int chop_last_dir(char **remoteurl, int is_relative)
 	return 0;
 }
 
+static void strip_url_ending(char *url, size_t *len_)
+{
+	size_t len = len_ ? *len_ : strlen(url);
+
+	for (;;) {
+		if (len > 1 && is_dir_sep(url[len - 2]) && url[len - 1] == '.') {
+			url[len-2] = '\0';
+			len -= 2;
+			continue;
+		}
+		if (len > 0 && is_dir_sep(url[len-1])) {
+			url[len-1] = '\0';
+			len--;
+			continue;
+		}
+
+		break;
+	}
+
+	if (len_)
+		*len_ = len;
+}
+
 /*
  * The `url` argument is the URL that navigates to the submodule origin
  * repo. When relative, this URL is relative to the superproject origin
@@ -93,14 +116,16 @@ static int chop_last_dir(char **remoteurl, int is_relative)
  * the superproject working tree otherwise.
  *
  * NEEDSWORK: This works incorrectly on the domain and protocol part.
- * remote_url      url              outcome          expectation
- * http://a.com/b  ../c             http://a.com/c   as is
- * http://a.com/b/ ../c             http://a.com/c   same as previous line, but
- *                                                   ignore trailing slash in url
- * http://a.com/b  ../../c          http://c         error out
- * http://a.com/b  ../../../c       http:/c          error out
- * http://a.com/b  ../../../../c    http:c           error out
- * http://a.com/b  ../../../../../c    .:c           error out
+ * remote_url       url              outcome          expectation
+ * http://a.com/b   ../c             http://a.com/c   as is
+ * http://a.com/b/  ../c             http://a.com/c   same as previous line, but
+ *                                                    ignore trailing '/' in url
+ * http://a.com/b/. ../c             http://a.com/c   same as previous line, but
+ *                                                    ignore trailing '/.' in url
+ * http://a.com/b   ../../c          http://c         error out
+ * http://a.com/b   ../../../c       http:/c          error out
+ * http://a.com/b   ../../../../c    http:c           error out
+ * http://a.com/b   ../../../../../c    .:c           error out
  * NEEDSWORK: Given how chop_last_dir() works, this function is broken
  * when a local part has a colon in its path component, too.
  */
@@ -115,8 +140,7 @@ static char *relative_url(const char *remote_url,
 	struct strbuf sb = STRBUF_INIT;
 	size_t len = strlen(remoteurl);
 
-	if (is_dir_sep(remoteurl[len-1]))
-		remoteurl[len-1] = '\0';
+	strip_url_ending(remoteurl, &len);
 
 	if (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))
 		is_relative = 0;
@@ -149,10 +173,10 @@ static char *relative_url(const char *remote_url,
 	}
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url);
-	if (ends_with(url, "/"))
-		strbuf_setlen(&sb, sb.len - 1);
 	free(remoteurl);
 
+	strip_url_ending(sb.buf, &sb.len);
+
 	if (starts_with_dot_slash(sb.buf))
 		out = xstrdup(sb.buf + 2);
 	else
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 25b48e5..e154e5f 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -329,14 +329,17 @@ test_submodule_relative_url "(null)" "./foo" "../submodule" "submodule"
 test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" "//somewhere else/subrepo"
 test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
 test_submodule_relative_url "(null)" "$PWD/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
-test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
-test_submodule_relative_url "(null)" "$PWD" "./." "$(pwd)/."
+test_submodule_relative_url "(null)" "$PWD/sub/." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD/sub/./." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD/sub/.////././/./." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD" "./." "$(pwd)"
 test_submodule_relative_url "(null)" "$PWD/addtest" "../repo" "$(pwd)/repo"
 test_submodule_relative_url "(null)" "$PWD" "./å äö" "$(pwd)/å äö"
-test_submodule_relative_url "(null)" "$PWD/." "../submodule" "$(pwd)/submodule"
+test_submodule_relative_url "(null)" "$PWD/sub" "../submodule" "$(pwd)/submodule"
+test_submodule_relative_url "(null)" "$PWD/sub/." "../submodule" "$(pwd)/submodule"
 test_submodule_relative_url "(null)" "$PWD/submodule" "../submodule" "$(pwd)/submodule"
 test_submodule_relative_url "(null)" "$PWD/home2/../remote" "../bundle1" "$(pwd)/home2/../bundle1"
-test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$(pwd)/submodule_update_repo/."
+test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$(pwd)/submodule_update_repo"
 test_submodule_relative_url "(null)" "file:///tmp/repo" "../subrepo" "file:///tmp/subrepo"
 test_submodule_relative_url "(null)" "foo/bar" "../submodule" "foo/submodule"
 test_submodule_relative_url "(null)" "foo" "../submodule" "submodule"
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index d046d98..7839ccd 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -615,6 +615,7 @@ test_expect_success 'setup subsubmodule' '
 	git reset --hard &&
 	git submodule update &&
 	(cd submod &&
+		git config --unset remote.origin.url &&
 		git update-index --add --cacheinfo 160000 $(git rev-parse HEAD) subsubmod &&
 		git config -f .gitmodules submodule.sub.url ../. &&
 		git config -f .gitmodules submodule.sub.path subsubmod &&
-- 
2.10.1.507.g2a9098a

Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"

[Johannes Sixt]

Am 22.10.2016 um 01:59 schrieb Stefan Beller:
> When adding a submodule via "git submodule add <relative url>",
> the relative url applies to the superprojects remote. When the
> superproject was cloned via "git clone . super", the remote url
> is ending with '/.'.
>
> The logic to construct the relative urls is not smart enough to
> detect that the ending /. is referring to the directory itself
> but rather treats it like any other relative path, i.e.
>
>     path/to/dir/. + ../relative/path/to/submodule
>
> would result in
>
>     path/to/dir/relative/path/to/submodule
>
> and not omit the "dir" as you may expect.
>
> As in a later patch we'll normalize the remote url before the
> computation of relative urls takes place, we need to first get our
> test suite in a shape with normalized urls only, which is why we should
> refrain from cloning from '.'

But you are removing a valid use case from the tests. Aren't you 
sweeping something under the rug with this patch?

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/t7064-wtstatus-pv2.sh      | 9 ++++++---
>  t/t7403-submodule-sync.sh    | 3 ++-
>  t/t7406-submodule-update.sh  | 6 ++++--
>  t/t7407-submodule-foreach.sh | 3 ++-
>  t/t7506-status-submodule.sh  | 3 ++-
>  5 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
> index 3012a4d..95514bb 100755
> --- a/t/t7064-wtstatus-pv2.sh
> +++ b/t/t7064-wtstatus-pv2.sh
> @@ -330,7 +330,8 @@ test_expect_success 'verify UU (edit-edit) conflict' '
>  test_expect_success 'verify upstream fields in branch header' '
>  	git checkout master &&
>  	test_when_finished "rm -rf sub_repo" &&
> -	git clone . sub_repo &&
> +	git clone "$(pwd)" sub_repo &&
> +	git -C sub_repo config --unset remote.origin.url &&

Why is it necessary to remove this configuration? Is it because when it 
is present, the submodule does not construct its own reference? And if 
so, should it not be sufficient to only remove the configuration 
(without changing 'git clone' command), but move this patch after the 
patch that fixes the /. treatment?

>  	(
>  		## Confirm local master tracks remote master.
>  		cd sub_repo &&
...

Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> The logic to construct the relative urls is not smart enough to
>> detect that the ending /. is referring to the directory itself
>> but rather treats it like any other relative path, i.e.
>>
>>     path/to/dir/. + ../relative/path/to/submodule
>>
>> would result in
>>
>>     path/to/dir/relative/path/to/submodule
>>
>> and not omit the "dir" as you may expect.
>>
>> As in a later patch we'll normalize the remote url before the
>> computation of relative urls takes place, we need to first get our
>> test suite in a shape with normalized urls only, which is why we should
>> refrain from cloning from '.'
>
> But you are removing a valid use case from the tests. Aren't you
> sweeping something under the rug with this patch?

I share the same reaction.

If the primary problem being solved is that the combination of a
relative URL ../sub and the base URL for the superproject which is
set to /path/to/dir/. (due to "clone .") were incorrectly resolved
as /path/to/dir/sub (because the buggy relative path logic did not
know that removing "/." at the end does not take you to one level
up), and a topic that fixes the bug would make that relative URL
../sub to be resolved as /path/to/sub, of course.  Otherwise, the
topic did not fix the bug.  

Now if a test that wanted to have a clone of the superproject by
"clone .", which results in the base URL of /path/to/dir/., actually
wants to refer in its .gitmodules to /path/to/dir/sub (which after
all was where the submodule the test created with or without the
bugfix), I would think the right adjustment for the test that used
to rely on the buggy behaviour would be to stop using ../sub and
instead use ./sub as the relative URL, no?  After all, the bug forced
the original test writer to write ../sub but the submodule in this
case actually is at ./sub relative to its superproject, and that is
what the original test writer would have written if the bug weren't
there in the first place, no?

Another thing I do not quite understand is why this step comes
before the fix.  If the "clone ." is adjusted to avoid triggering
the buggy behaviour, i.e. making the base URL to /path/to/dir
(instead of /path/to/dir/.), wouldn't the relative URL ../sub that
was written to work around the bug that hasn't been fixed yet in
this step need to be adjusted anyway?  It would end up referring to
/path/to/sub and not /path/to/dir/sub until the fix is put in place.

Is the removal of remote.origin.url a wrong workaround for that
breakage, I wonder...  I do not understand that change at all, and I
do not think it was explained in the log message.

If we really wanted to update the test before fixing the bug, I
would understand if this step changed ../sub (or whatever relative
URL that has extra ../ only because the base URL has extra /. at the
end to compensate for the buggy resolution) to ./sub in the tests
and marked them to expect failure, and then a later step that fixes
the bug turns them to expect success without make any other change.

Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"

[Stefan Beller]

On Sat, Oct 22, 2016 at 12:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>>> The logic to construct the relative urls is not smart enough to
>>> detect that the ending /. is referring to the directory itself
>>> but rather treats it like any other relative path, i.e.
>>>
>>>     path/to/dir/. + ../relative/path/to/submodule
>>>
>>> would result in
>>>
>>>     path/to/dir/relative/path/to/submodule
>>>
>>> and not omit the "dir" as you may expect.
>>>
>>> As in a later patch we'll normalize the remote url before the
>>> computation of relative urls takes place, we need to first get our
>>> test suite in a shape with normalized urls only, which is why we should
>>> refrain from cloning from '.'
>>
>> But you are removing a valid use case from the tests. Aren't you
>> sweeping something under the rug with this patch?
>
> I share the same reaction.

Oh I see. I agree.
I reverted the lines that replace . by "$(pwd)" and it still works, but it
still works because the code path regarding local paths was not broken
(both . as well as "$(pwd)" are working without the fix)

>
> If the primary problem being solved is that the combination of a
> relative URL ../sub and the base URL for the superproject which is
> set to /path/to/dir/. (due to "clone .") were incorrectly resolved
> as /path/to/dir/sub (because the buggy relative path logic did not
> know that removing "/." at the end does not take you to one level
> up), and a topic that fixes the bug would make that relative URL
> ../sub to be resolved as /path/to/sub, of course.  Otherwise, the
> topic did not fix the bug.
>
> Now if a test that wanted to have a clone of the superproject by
> "clone .", which results in the base URL of /path/to/dir/., actually
> wants to refer in its .gitmodules to /path/to/dir/sub (which after
> all was where the submodule the test created with or without the
> bugfix), I would think the right adjustment for the test that used
> to rely on the buggy behaviour would be to stop using ../sub and
> instead use ./sub as the relative URL, no?  After all, the bug forced
> the original test writer to write ../sub but the submodule in this
> case actually is at ./sub relative to its superproject, and that is
> what the original test writer would have written if the bug weren't
> there in the first place, no?

True.

I have looked into it again, and by now I think the bug is a feature,
actually.

Consider this:

    git clone . super
    git -C super submodule add ../submodule
    # we thought the previous line is buggy
    git clone super super-clone

Now in the super-clone the ../submodule is the correct
relative url, because the url where we cloned from doesn't
end in /.

If we were to fix the bug with the /. ending, then we would need the
following:

    git clone . super
    git -C super submodule add ./submodule
    # correct relative URL above!

    git clone super super-clone
    cd superclone && sed s|url =./|url = ../|
    # make sure we fix the relative URLs to account for a different remote.

And this doesn't seem right to me as it burdens the user of the super-clone.

>
> Another thing I do not quite understand is why this step comes
> before the fix.  If the "clone ." is adjusted to avoid triggering
> the buggy behaviour, i.e. making the base URL to /path/to/dir
> (instead of /path/to/dir/.), wouldn't the relative URL ../sub that
> was written to work around the bug that hasn't been fixed yet in
> this step need to be adjusted anyway?  It would end up referring to
> /path/to/sub and not /path/to/dir/sub until the fix is put in place.
>
> Is the removal of remote.origin.url a wrong workaround for that
> breakage, I wonder...  I do not understand that change at all, and I
> do not think it was explained in the log message.

I think it is wrong, because it is sidestepping the actual issue.
Continuing from above:

    git clone super-clone super-clone2
    # this works again, as the remote change doesn't matter.

    mkdir test && git -C test clone ../ .
    # submodule urls need to be "undone again to work:
    cd test && sed s|url =../|url = ./|

So I think keeping the /. around as it currently is, is a pretty
useful bug.

>
> If we really wanted to update the test before fixing the bug, I
> would understand if this step changed ../sub (or whatever relative
> URL that has extra ../ only because the base URL has extra /. at the
> end to compensate for the buggy resolution) to ./sub in the tests
> and marked them to expect failure, and then a later step that fixes
> the bug turns them to expect success without make any other change.

I'll think about this further, but currently I am inclined to declare
it a nonbug
and as it eases testing a lot. Also if someone wants to clone a repository
with broken relative urls, they can work around that by e.g.

    git clone <url>/.

to fix one level of indentation, though it is not documented and seems
to be brittle.

Thanks,
Stefan

Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"

[Johannes Sixt]

Am 22.10.2016 um 22:46 schrieb Stefan Beller:
> I have looked into it again, and by now I think the bug is a feature,
> actually.
>
> Consider this:
>
>     git clone . super
>     git -C super submodule add ../submodule
>     # we thought the previous line is buggy
>     git clone super super-clone

At this point, we *should* have this if there were no bugs (at least 
that is my assumption):

   /tmp
   !
   + submodule     <- submodule's remote repo
   !
   + foo           <- we are here (.), super's remote repo
     !
     + super       <- remote.origin.url=/tmp/foo/.
       !
       + submodule <- remote.origin.url=/tmp/foo/./../submodule
                      submodule.submodule.url=../submodule

When I test this, 'git submodule add' fails:

foo@master> git -C super submodule add ../submodule
fatal: repository '/tmp/foo/submodule' does not exist
fatal: clone of '/tmp/foo/submodule' into submodule path 
'/tmp/foo/super/submodule' failed

> Now in the super-clone the ../submodule is the correct
> relative url, because the url where we cloned from doesn't
> end in /.

I do not understand why this would be relevant. The question is not how 
the submodule's remote URL ends, but how the submodule's remote URL is 
constructed from the super-project's URL and the relative path specified 
for 'git submodule add'.

Whether ../submodule or ./submodule is the correct relative URL depends 
on where the origin of the submodule is located relative to the origin 
of the super-project. In the above example, it is ../submodule. However, 
the error message tells us that git looked in /tmp/foo/submodule, which 
looks like the /. bug!

I do not understand where you see a feature here. What am I missing?

-- Hannes

Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 22.10.2016 um 22:46 schrieb Stefan Beller:
>> I have looked into it again, and by now I think the bug is a feature,
>> actually.
>>
>> Consider this:
>>
>>     git clone . super
>>     git -C super submodule add ../submodule
>>     # we thought the previous line is buggy
>>     git clone super super-clone
>
> At this point, we *should* have this if there were no bugs (at least
> that is my assumption):
>
>   /tmp
>   !
>   + submodule     <- submodule's remote repo
>   !
>   + foo           <- we are here (.), super's remote repo
>     !
>     + super       <- remote.origin.url=/tmp/foo/.
>       !
>       + submodule <- remote.origin.url=/tmp/foo/./../submodule
>                      submodule.submodule.url=../submodule
>
> When I test this, 'git submodule add' fails:
>
> foo@master> git -C super submodule add ../submodule
> fatal: repository '/tmp/foo/submodule' does not exist
> fatal: clone of '/tmp/foo/submodule' into submodule path
> '/tmp/foo/super/submodule' failed
>
>> Now in the super-clone the ../submodule is the correct
>> relative url, because the url where we cloned from doesn't
>> end in /.
>
> I do not understand why this would be relevant. The question is not
> how the submodule's remote URL ends, but how the submodule's remote
> URL is constructed from the super-project's URL and the relative path
> specified for 'git submodule add'.

FWIW, that matches my understanding.

> Whether ../submodule or ./submodule is the correct relative URL
> depends on where the origin of the submodule is located relative to
> the origin of the super-project. In the above example, it is
> ../submodule. However, the error message tells us that git looked in
> /tmp/foo/submodule, which looks like the /. bug!
>
> I do not understand where you see a feature here. What am I missing?
>
> -- Hannes

Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"

[Stefan Beller]

On Sun, Oct 23, 2016 at 3:14 AM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 22.10.2016 um 22:46 schrieb Stefan Beller:
>>
>> I have looked into it again, and by now I think the bug is a feature,
>> actually.
>>
>> Consider this:
>>
>>     git clone . super
>>     git -C super submodule add ../submodule
>>     # we thought the previous line is buggy
>>     git clone super super-clone
>
>
> At this point, we *should* have this if there were no bugs (at least that is
> my assumption):
>
>   /tmp
>   !
>   + submodule     <- submodule's remote repo
>   !
>   + foo           <- we are here (.), super's remote repo
>     !
>     + super       <- remote.origin.url=/tmp/foo/.
>       !
>       + submodule <- remote.origin.url=/tmp/foo/./../submodule
>                      submodule.submodule.url=../submodule
>
> When I test this, 'git submodule add' fails:

Yes this setup currently fails because the  /tmp/foo/./../submodule
is computed to be /tmp/foo/submodule.

In the test suite the directory "foo" is usually called
"trash\ directory.tXXXX-description" and the remote of
the submodule is created inside of it, such that:

/tmp
   !
   + foo            <- we are here (.), super's remote repo
     !
     + submodule    <- submodule's remote repo
     !
     + super        <- remote.origin.url=/tmp/foo/.
       !
       + submodule  <- remote.origin.url=/tmp/foo/./../submodule
                       submodule.submodule.url=../submodule
                       current result=/tmp/foo/submodule

works.

>
> foo@master> git -C super submodule add ../submodule
> fatal: repository '/tmp/foo/submodule' does not exist
> fatal: clone of '/tmp/foo/submodule' into submodule path
> '/tmp/foo/super/submodule' failed
>
>> Now in the super-clone the ../submodule is the correct
>> relative url, because the url where we cloned from doesn't
>> end in /.
>
>
> I do not understand why this would be relevant. The question is not how the
> submodule's remote URL ends, but how the submodule's remote URL is
> constructed from the super-project's URL and the relative path specified for
> 'git submodule add'.

I was not precise here. If you have the working setup as above, you can clone
super to super-clone and it keeps working, because of the current "bug".

The difference between "super" that is cloned from . and "super-clone" that
is cloned from "super" is only the remote url, and currently
/tmp/foo/super
/tmp/foo/.

+ relative path ../submodule resolve to the same submodule remote.

>
> Whether ../submodule or ./submodule is the correct relative URL depends on
> where the origin of the submodule is located relative to the origin of the
> super-project. In the above example, it is ../submodule. However, the error
> message tells us that git looked in /tmp/foo/submodule, which looks like the
> /. bug!

Correct.

At the time I was trying to fix the urls in the test suite with the
bug fix and I then
realized that this bugfix introduces a lot of pain to our test suite,
because we do
these secondary clones quite a few times. The pattern usually is:
* setup super (cloned from .)
* clone super to super-clone
* trash super-clone for testing purposes and delete it.

>
> I do not understand where you see a feature here. What am I missing?

The ease of use in our own testing suite is the feature I was talking about.

When we would fix the bug, we would not just need to fix
s|../submodule|./submodule| when setting up super, but we would need to
fix it every time again in reverse when creating these short lived
"super-clones"
that get tested on and deleted.

So maybe the correct fix for the testing suite is a double clone, i.e. instead
of


    # prepare some stuff
    git clone . super

we rather do:

    # mkdir upstream && prepare stuff in upstream
    git clone upstream super

However that way we would end up not exercising the
code path to be fixed with the actual bug fix i.e. we'd never clone from /.
because it is silly conceptually. We could add a new test for that though
that only tests that cloning from . constructs the correct URL.
However that is already tested in t0060 resolving the relative URLs
via the git submodule helper.

Thanks for bearing this discussion,
I feel like I am missing the obvious point here,

Stefan

Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"

[Johannes Sixt]

Am 24.10.2016 um 21:10 schrieb Stefan Beller:
> The ease of use in our own testing suite is the feature I was talking about.

Ok, thanks for the clarification. Next steps would then be, IMO, to fix 
the tests to match the future world order and mark tests that fail due 
to the bug with test_expect_failure, and then switch them back to 
test_expect_success in the patch with the bug fix.

-- Hannes