Re: [PATCHv3] submodule--helper: normalize funny urls

[Stefan Beller]

On Tue, Oct 18, 2016 at 7:05 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>> I am not sure.  Certainly we would want to make sure that the normal
>>> case (i.e. no funny trailing junk) to work correctly, but we do want
>>> to protect the fix from future breakage as well, no?
>>
>> Exactly. So not intermediate "root" that we clone from, but adapting the
>> relative URLs. Maybe half the broken tests can switch to 'root' and the 
>> others
>> go with the current behavior of cloning . to super.
>>>
>>> Perhaps we can do a preliminary step to update tests to primarily
>>> check the cases that do not involve URL with trailing "/." by either
>>> doing that double clone, or more preferrably, clone from "$(pwd)"
>>> and adjust the incorrect submodule reference that have been relying
>>> on the buggy behaviour.  With that "root" trick, the test would pass
>>> with or without the fix under discussion, right?
>>
>> I assume so, (not tested).
>
> OK.  Thanks for sticking with it.

Ok, the root trick works fine without the fix, however we preferrably
want to fix it
without double cloning, then the fix becomes a bit harder to follow:

instead of

git clone . super

we do

git clone "$(pwd)" super &&
git -C super config --unset remote.origin.url &&

instead, such that the relative urls work the same way.
(The super becomes its own authoritative source of truth with no upstream.
In that case the url is relative to the superproject, e.g.

git -C super submodule add ../submodule

will then resolve the relative path from super/../submodule to be just
as before, where
we bugily used $(git config remote.origin.url =
...super/.)/../submodule, which due to the
but resolved to the submodule as well (as /. was counted as a directory).

Re: [PATCHv3] submodule--helper: normalize funny urls

[Junio C Hamano]

Stefan Beller  writes:

> My thought was to fix it nevertheless, such that the url recorded as
> remote.origin.url is always the first case (no l or /. at the end).
>
> If we were to add this fix to clone, then it may be easier to debug
> submodule url schemes for users as the submodule url would then
> be a concatenation of remote.origin.url and the relative part.
>
> That seems easier to understand than ${remote.origin.url%%/.} +
> relative path, maybe? (Because then the user doesn't need to guess
> or remember historical behavior that is wrong on how this)

Are you declaring that trailing / or /. will now be illegal?  If you
are declaring that, then I agree that new codepaths no longer have
to worry about "strip / or /. before concatenating" and will
simplify things for them.  But otherwise, such a "fix" also would
have an effect of hiding bugs from codepaths.  They still need to be
prepared to see any of the three variants and cope with them
correctly, right?

Re: [PATCHv3] submodule--helper: normalize funny urls

[Stefan Beller]

On Thu, Oct 20, 2016 at 12:26 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Do we actually want to fix git-clone as well?
>
> If I understand correctly, the point of this fix is to make it not
> to matter whether the original URL the end user gives or recorded as
> the remote by "git clone" in the repository is any one of:
>
> $any_leading_part/path/to/dir
> $any_leading_part/path/to/dir/
> $any_leading_part/path/to/dir/.
>
> So I do not think there is anything to "fix", as long as "git clone"

Yes "git clone" works with any of the three above.

My thought was to fix it nevertheless, such that the url recorded as
remote.origin.url is always the first case (no l or /. at the end).

If we were to add this fix to clone, then it may be easier to debug
submodule url schemes for users as the submodule url would then
be a concatenation of remote.origin.url and the relative part.

That seems easier to understand than ${remote.origin.url%%/.} +
relative path, maybe? (Because then the user doesn't need to guess
or remember historical behavior that is wrong on how this)

> that is given any one of the above three records any one of the
> above three as the result.  It _may_ be desirable if the result is
> identical what was given as input, but I do not offhand think that
> is required.
>
>> I tried and then I see breakage in 5603-clone-dirname
>> as ssh://host seems to be an invalid url; it has to end with a slash?
>
> That is a separate issue, isn't it?  We shouldn't be touching the
> leading ":///" part, I would think.

I agree, So I'll first fix the submodule parts only.

>
> For example, a URL "../another" relative to "ssh://host/path" may be
> "ssh://host/another", but shouldn't it be an error to take
> "../../outside" relative to "ssh://host/path"?

That is correct. I'll stop looking at clone code.

Re: [PATCHv3] submodule--helper: normalize funny urls

[Junio C Hamano]

Stefan Beller  writes:

> Do we actually want to fix git-clone as well?

If I understand correctly, the point of this fix is to make it not
to matter whether the original URL the end user gives or recorded as
the remote by "git clone" in the repository is any one of:

$any_leading_part/path/to/dir
$any_leading_part/path/to/dir/
$any_leading_part/path/to/dir/.

So I do not think there is anything to "fix", as long as "git clone"
that is given any one of the above three records any one of the
above three as the result.  It _may_ be desirable if the result is
identical what was given as input, but I do not offhand think that
is required.

> I tried and then I see breakage in 5603-clone-dirname
> as ssh://host seems to be an invalid url; it has to end with a slash?

That is a separate issue, isn't it?  We shouldn't be touching the
leading ":///" part, I would think.  

For example, a URL "../another" relative to "ssh://host/path" may be
"ssh://host/another", but shouldn't it be an error to take
"../../outside" relative to "ssh://host/path"?

Re: [PATCHv3] submodule--helper: normalize funny urls

[Stefan Beller]

On Tue, Oct 18, 2016 at 7:05 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>> I am not sure.  Certainly we would want to make sure that the normal
>>> case (i.e. no funny trailing junk) to work correctly, but we do want
>>> to protect the fix from future breakage as well, no?
>>
>> Exactly. So not intermediate "root" that we clone from, but adapting the
>> relative URLs. Maybe half the broken tests can switch to 'root' and the 
>> others
>> go with the current behavior of cloning . to super.
>>>
>>> Perhaps we can do a preliminary step to update tests to primarily
>>> check the cases that do not involve URL with trailing "/." by either
>>> doing that double clone, or more preferrably, clone from "$(pwd)"
>>> and adjust the incorrect submodule reference that have been relying
>>> on the buggy behaviour.  With that "root" trick, the test would pass
>>> with or without the fix under discussion, right?
>>
>> I assume so, (not tested).
>
> OK.  Thanks for sticking with it.

Do we actually want to fix git-clone as well?
I tried and then I see breakage in 5603-clone-dirname
as ssh://host seems to be an invalid url; it has to end with a slash?

If we were to fix clone as well, then we'd still have a lot of possible stale
data (ending in /.) out there, so maybe we want to not fix clone for
now and only
fix it when computing the submodule url.

So I'll first fix up the test suite to not rely on buggy behavior and
then send this patch
with no change in tests? That sounds strange to me as it hides away the cause.

Re: [PATCHv3] submodule--helper: normalize funny urls

[Junio C Hamano]

Stefan Beller  writes:

>> I am not sure.  Certainly we would want to make sure that the normal
>> case (i.e. no funny trailing junk) to work correctly, but we do want
>> to protect the fix from future breakage as well, no?
>
> Exactly. So not intermediate "root" that we clone from, but adapting the
> relative URLs. Maybe half the broken tests can switch to 'root' and the others
> go with the current behavior of cloning . to super.
>>
>> Perhaps we can do a preliminary step to update tests to primarily
>> check the cases that do not involve URL with trailing "/." by either
>> doing that double clone, or more preferrably, clone from "$(pwd)"
>> and adjust the incorrect submodule reference that have been relying
>> on the buggy behaviour.  With that "root" trick, the test would pass
>> with or without the fix under discussion, right?
>
> I assume so, (not tested).

OK.  Thanks for sticking with it.

Re: [PATCHv3] submodule--helper: normalize funny urls

[Stefan Beller]

On Tue, Oct 18, 2016 at 5:56 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> The underlying issue is two fold:
>>
>> * in t3600 we'd need
>> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
>> index d046d98..545d32f 100755
>> --- a/t/t3600-rm.sh
>> +++ b/t/t3600-rm.sh
>> @@ -616,7 +616,7 @@ test_expect_success 'setup subsubmodule' '
>> git submodule update &&
>> (cd submod &&
>> git update-index --add --cacheinfo 16 $(git
>> rev-parse HEAD) subsubmod &&
>> -   git config -f .gitmodules submodule.sub.url ../. &&
>> +   git config -f .gitmodules submodule.sub.url ./. &&
>> git config -f .gitmodules submodule.sub.path subsubmod &&
>> git submodule init &&
>> git add .gitmodules &&
>>
>> because the sub-submodule URL is actually the same as the submodule
>> (because we'd test lazily)
>
> This fix sounds entirely sane.  The "../." you touched was depending
> on the buggy behaviour; it is exactly the case of "there were two
> wrongs that were covering each other; after one of them gets fixed,
> the other one's brokenness is exposed", right?
>
>> However in t7403, we have a construct like:
>>
>> git clone . super
>>
>> which then results in
>>
>> git -C super remote -v
>> ./git/t/trash directory.t7403-submodule-sync/. (fetch)
>
> That sounds expected.  We do not have to add trailing "/.", but the
> system ought to work with or without it correctly and the same way.
>
>> However instead of fixing the levels of nesting, the fix is as easy as:
>> diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
>> index 0726799..525d32b 100755
>> --- a/t/t7403-submodule-sync.sh
>> +++ b/t/t7403-submodule-sync.sh
>> @@ -15,7 +15,9 @@ test_expect_success setup '
>> git add file &&
>> test_tick &&
>> git commit -m upstream &&
>> -   git clone . super &&
>> +   # avoid cloning a repository with a url ending in /.
>> +   git clone . root &&
>> +   git clone root super &&
>> git clone super submodule &&
>> (
>> cd submodule &&
>>
>> Same goes for t740{6,7} as well as t7506.
>
> Isn't the issue the same as that "3600-rm" one?  I know you said
> twofold upfront, but I am not sure I agree.

I took a couple of hours trying to get the same fix applied to the t7* tests,
but that doesn't seem to be as easy. I'll try again.

>
> The "super" repository refers to its submodules with "../submodule"
> in the test code we have, even though the submodule referred to
> lives inside $TRASH, and by fixing the "trailing /. and trailing
> /root are treated the same way" bug, its reference created in the
> test ends up referring to one level above, perhaps in t/submodule,
> instead of the intended place t/trash/submodule.  By using "root",
> you are making their wrong references point at the right place.

Correct.

>
> Admittedly, the updated test above tests something different from
> what it originally wanted to test, which feels somewhat distasteful
> but I do not think it is wrong.

I think it is. I was just showing how to quick fix the issue, and how wide the
impact was. More like assessing the situation of what is broken with
that patch, rather than proposing a way to go for fixing.

>
>> I think this change to the test suite is not warranted, because
>> we want to have the current behavior as-is ...
>
> I am not sure.  Certainly we would want to make sure that the normal
> case (i.e. no funny trailing junk) to work correctly, but we do want
> to protect the fix from future breakage as well, no?

Exactly. So not intermediate "root" that we clone from, but adapting the
relative URLs. Maybe half the broken tests can switch to 'root' and the others
go with the current behavior of cloning . to super.

>
> Perhaps we can do a preliminary step to update tests to primarily
> check the cases that do not involve URL with trailing "/." by either
> doing that double clone, or more preferrably, clone from "$(pwd)"
> and adjust the incorrect submodule reference that have been relying
> on the buggy behaviour.  With that "root" trick, the test would pass
> with or without the fix under discussion, right?

I assume so, (not tested).

>
> Then do the fix under discussion and introduce a test that clones
> from "." refers to submodules with relative path and makes sure that
> trailing "/." is interpreted correctly.

Re: [PATCHv3] submodule--helper: normalize funny urls

[Junio C Hamano]

Stefan Beller  writes:

> The underlying issue is two fold:
>
> * in t3600 we'd need
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index d046d98..545d32f 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -616,7 +616,7 @@ test_expect_success 'setup subsubmodule' '
> git submodule update &&
> (cd submod &&
> git update-index --add --cacheinfo 16 $(git
> rev-parse HEAD) subsubmod &&
> -   git config -f .gitmodules submodule.sub.url ../. &&
> +   git config -f .gitmodules submodule.sub.url ./. &&
> git config -f .gitmodules submodule.sub.path subsubmod &&
> git submodule init &&
> git add .gitmodules &&
>
> because the sub-submodule URL is actually the same as the submodule
> (because we'd test lazily)

This fix sounds entirely sane.  The "../." you touched was depending
on the buggy behaviour; it is exactly the case of "there were two
wrongs that were covering each other; after one of them gets fixed,
the other one's brokenness is exposed", right?

> However in t7403, we have a construct like:
>
> git clone . super
>
> which then results in
>
> git -C super remote -v
> ./git/t/trash directory.t7403-submodule-sync/. (fetch)

That sounds expected.  We do not have to add trailing "/.", but the
system ought to work with or without it correctly and the same way.

> However instead of fixing the levels of nesting, the fix is as easy as:
> diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
> index 0726799..525d32b 100755
> --- a/t/t7403-submodule-sync.sh
> +++ b/t/t7403-submodule-sync.sh
> @@ -15,7 +15,9 @@ test_expect_success setup '
> git add file &&
> test_tick &&
> git commit -m upstream &&
> -   git clone . super &&
> +   # avoid cloning a repository with a url ending in /.
> +   git clone . root &&
> +   git clone root super &&
> git clone super submodule &&
> (
> cd submodule &&
>
> Same goes for t740{6,7} as well as t7506.

Isn't the issue the same as that "3600-rm" one?  I know you said
twofold upfront, but I am not sure I agree.

The "super" repository refers to its submodules with "../submodule"
in the test code we have, even though the submodule referred to
lives inside $TRASH, and by fixing the "trailing /. and trailing
/root are treated the same way" bug, its reference created in the
test ends up referring to one level above, perhaps in t/submodule,
instead of the intended place t/trash/submodule.  By using "root",
you are making their wrong references point at the right place.

Admittedly, the updated test above tests something different from
what it originally wanted to test, which feels somewhat distasteful
but I do not think it is wrong.

> I think this change to the test suite is not warranted, because
> we want to have the current behavior as-is ...

I am not sure.  Certainly we would want to make sure that the normal
case (i.e. no funny trailing junk) to work correctly, but we do want
to protect the fix from future breakage as well, no?

Perhaps we can do a preliminary step to update tests to primarily
check the cases that do not involve URL with trailing "/." by either
doing that double clone, or more preferrably, clone from "$(pwd)"
and adjust the incorrect submodule reference that have been relying
on the buggy behaviour.  With that "root" trick, the test would pass
with or without the fix under discussion, right?

Then do the fix under discussion and introduce a test that clones
from "." refers to submodules with relative path and makes sure that
trailing "/." is interpreted correctly.

Re: [PATCHv3] submodule--helper: normalize funny urls

[Stefan Beller]

On Tue, Oct 18, 2016 at 2:19 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Stefan Beller  writes:
>>
>>> The remote URL for the submodule can be specified relative
>>> ...
>>> v3:
>>>  * fixed the coding style.
>>
>> Ah, thanks.  I had a squash queued on top but will replace with this
>> one.
>
> Heh, I guess I shouldn't have responded before seeing what this
> breaks.  Applied on top of sb/submodule-ignore-trailing-slash, these
> seem to break.

Ugh. (I should have tested more than just t0060).

The underlying issue is two fold:

* in t3600 we'd need
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index d046d98..545d32f 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -616,7 +616,7 @@ test_expect_success 'setup subsubmodule' '
git submodule update &&
(cd submod &&
git update-index --add --cacheinfo 16 $(git
rev-parse HEAD) subsubmod &&
-   git config -f .gitmodules submodule.sub.url ../. &&
+   git config -f .gitmodules submodule.sub.url ./. &&
git config -f .gitmodules submodule.sub.path subsubmod &&
git submodule init &&
git add .gitmodules &&

because the sub-submodule URL is actually the same as the submodule
(because we'd test lazily)

This looks ok from a bug fixers perspective.

However in t7403, we have a construct like:

git clone . super

which then results in

git -C super remote -v
./git/t/trash directory.t7403-submodule-sync/. (fetch)

And the commit message of this patch claimed we'd never use
the /. syntax ourselves. (We could argue the stupid users in the test
suite are doing it wrong, because in practice nobody would use clone
to create a nested repository? Not sure I agree.)

However instead of fixing the levels of nesting, the fix is as easy as:
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 0726799..525d32b 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -15,7 +15,9 @@ test_expect_success setup '
git add file &&
test_tick &&
git commit -m upstream &&
-   git clone . super &&
+   # avoid cloning a repository with a url ending in /.
+   git clone . root &&
+   git clone root super &&
git clone super submodule &&
(
cd submodule &&

Same goes for t740{6,7} as well as t7506.

I think this change to the test suite is not warranted, because
we want to have the current behavior as-is as it seems like a nice
hack:

* Maybe we'd want to think about checking for the URL in git clone
  normalize the URL before configuring remote.origin.URL

* an often observed work flow for submodule tests seems:

mkdir sub1 &&
git -C sub1 init  &&
...

git clone . super &&
git -C super submodule add ../sub1
... # the ../sub1 looks intuitively correct
# because from the current directory which is
# super the relative path is ../sub1
#
# However in reality this ought to be a relative URL,
# and as super sits in the same directory as sub1
# ./sub1 would be "correct" according to the documentation
# However as the super remote URL ends with /.
# we had a bug that we needed to add one layer of unnesting
# and that is how ../sub1 worked.


Not sure about this patch any more.

Stefan

Re: [PATCHv3] submodule--helper: normalize funny urls

[Junio C Hamano]

Junio C Hamano  writes:

> Stefan Beller  writes:
>
>> The remote URL for the submodule can be specified relative
>> ...
>> v3:
>>  * fixed the coding style.
>
> Ah, thanks.  I had a squash queued on top but will replace with this
> one.

Heh, I guess I shouldn't have responded before seeing what this
breaks.  Applied on top of sb/submodule-ignore-trailing-slash, these
seem to break.

t/trash directory.t3600-rm
t/trash directory.t7403-submodule-sync
t/trash directory.t7406-submodule-update
t/trash directory.t7407-submodule-foreach
t/trash directory.t7506-status-submodule

Some may be showing broken assumptions of the downstream, two wrongs
compensating each other and correcting one exposing breakage of the
other.  I didn't look at them deeply.

Re: [PATCHv3] submodule--helper: normalize funny urls

[Junio C Hamano]

Stefan Beller  writes:

> The remote URL for the submodule can be specified relative
> ...
> v3:
>  * fixed the coding style.

Ah, thanks.  I had a squash queued on top but will replace with this
one.

[PATCHv3] 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 "/" when resolving a
relative URL "../" off of it.  Stripping "/." at
the end does *not* take you one level up, even though
stripping "/" 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.

Helped-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
---
v3:
 * fixed the coding style.

v2:
 * reworded the commit message, taken from Junio, but added more explanation
   why we want to introduce this patch. 
 * added the length check
 * use an infinite loop with break instead of a variable
   to determine the ending condition.
   
 builtin/submodule--helper.c | 48 +
 t/t0060-path-utils.sh   | 11 +++
 2 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 260f46f..4f11082 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  ../../../../chttp: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   ../../../../chttp: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