From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Stefan Beller <sbeller@google.com>
Cc: gitster@pobox.com, git@vger.kernel.org, venv21@gmail.com,
dennis@kaarsemaker.net
Subject: Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL
Date: Wed, 12 Oct 2016 15:30:49 +0200 (CEST) [thread overview]
Message-ID: <alpine.DEB.2.20.1610121501390.3492@virtualbox> (raw)
In-Reply-To: <20161010175611.1058-1-sbeller@google.com>
Hi Stefan,
On Mon, 10 Oct 2016, Stefan Beller wrote:
> Before 63e95beb0 (2016-04-15, submodule: port resolve_relative_url from
> shell to C), it did not matter if the superprojects URL had a trailing
> slash or not. It was just chopped off as one of the first steps
> (The "remoteurl=${remoteurl%/}" near the beginning of
> resolve_relative_url(), which was removed in said commit).
>
> When porting this to the C version, an off-by-one error was introduced
> and we did not check the actual last character to be a slash, but the
> NULL delimiter.
It is a NUL delimiter, not a NULL delimiter.
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 444ec06..a7841a5 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -95,6 +95,8 @@ static int chop_last_dir(char **remoteurl, int is_relative)
> * 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
> @@ -113,8 +115,8 @@ static char *relative_url(const char *remote_url,
> struct strbuf sb = STRBUF_INIT;
> size_t len = strlen(remoteurl);
>
> - if (is_dir_sep(remoteurl[len]))
> - remoteurl[len] = '\0';
> + if (is_dir_sep(remoteurl[len-1]))
> + remoteurl[len-1] = '\0';
>
> if (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))
> is_relative = 0;
> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> index bf2deee..82b98f8 100755
> --- a/t/t0060-path-utils.sh
> +++ b/t/t0060-path-utils.sh
> @@ -319,6 +319,7 @@ test_submodule_relative_url "../" "foo/bar" "../submodule" "../foo/submodule"
> test_submodule_relative_url "../" "foo" "../submodule" "../submodule"
>
> test_submodule_relative_url "(null)" "../foo/bar" "../sub/a/b/c" "../foo/sub/a/b/c"
> +test_submodule_relative_url "(null)" "../foo/bar/" "../sub/a/b/c" "../foo/sub/a/b/c"
> test_submodule_relative_url "(null)" "../foo/bar" "../submodule" "../foo/submodule"
> test_submodule_relative_url "(null)" "../foo/submodule" "../submodule" "../foo/submodule"
> test_submodule_relative_url "(null)" "../foo" "../submodule" "../submodule"
I see that this already made it to `next`. I saw that because it breaks
the build of Git for Windows (this was not noticed earlier because other
compile failures prevented the tests from running), as now the test cases
173 and 177 of t0060 fail (*not* the newly introduced 163).
Here is the output with -v -x:
-- snip --
[...]
expecting success:
actual=$(git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' '../.') &&
test "$actual" = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/.'
+++ git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' ../.
++ actual=C:/git-sdk-64/usr/src/git/wip/t/.
++ test C:/git-sdk-64/usr/src/git/wip/t/. = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/.'
error: last command exited with $?=1
not ok 172 - test_submodule_relative_url: (null) /usr/src/git/wip/t/trash directory.t0060-path-utils/. ../. => C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/.
#
# actual=$(git submodule--helper
# resolve-relative-url-test '(null)'
# '/usr/src/git/wip/t/trash
# directory.t0060-path-utils/.' '../.') &&
# test "$actual" =
# 'C:/git-sdk-64/usr/src/git/wip/t/trash
# directory.t0060-path-utils/.'
#
[...]
expecting success:
actual=$(git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' '../submodule') &&
test "$actual" = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/submodule'
+++ git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' ../submodule
++ actual=C:/git-sdk-64/usr/src/git/wip/t/submodule
++ test C:/git-sdk-64/usr/src/git/wip/t/submodule = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/submodule'
error: last command exited with $?=1
not ok 176 - test_submodule_relative_url: (null) /usr/src/git/wip/t/trash directory.t0060-path-utils/. ../submodule => C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/submodule
#
# actual=$(git submodule--helper
# resolve-relative-url-test '(null)'
# '/usr/src/git/wip/t/trash
# directory.t0060-path-utils/.' '../submodule') &&
# test "$actual" =
# 'C:/git-sdk-64/usr/src/git/wip/t/trash
# directory.t0060-path-utils/submodule'
#
[...]
-- snap --
For comparison, this is how it succeeds in an Ubuntu VM:
-- snap --
expecting success:
actual=$(git submodule--helper resolve-relative-url-test '(null)' '/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.' '../.') &&
test "$actual" = '/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.'
+++ git submodule--helper resolve-relative-url-test '(null)' '/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.' ../.
++ actual='/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.'
++ test '/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.' = '/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.'
+ test_eval_ret_=0
+ want_trace
+ test t = t
+ test t = t
+ set +x
ok 173 - test_submodule_relative_url: (null) /home/virtualbox/git/wip/t/trash directory.t0060-path-utils/. ../. => /home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.
-- snap --
The reason that this fails on Windows is that the POSIX->Windows path
mangling of the MSYS2 shell strips the trailing . from "/some/directory/."
when converting it to "C:/git-sdk-64/some/directory", and for a good
reason: most Windows programs do not handle the trailing "." very well.
One very, very ugly workaround for this newly-introduced breakage would be
this:
-- snip --
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 82b98f8..abd82e9 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -328,11 +328,11 @@ 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" "./." "$(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)/." "../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/."
-- snap --
The reasons this is ugly: we specifically test for *Unixy* paths when we
use $PWD, as opposed to *Windowsy* paths when using $(pwd). We do this to
ensure a certain level of confidence that running things such as
git clone --recurse-submodules /z/project/.
work. And now that does not work anymore.
So where to go from here?
Ciao,
Dscho
next prev parent reply other threads:[~2016-10-12 14:23 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-10 17:56 [PATCH 1/2] submodule: ignore trailing slash on superproject URL Stefan Beller
2016-10-10 17:56 ` [PATCH 2/2] submodule: ignore trailing slash in relative url Stefan Beller
2016-10-10 19:58 ` [PATCH 1/2] submodule: ignore trailing slash on superproject URL Dennis Kaarsemaker
2016-10-12 13:30 ` Johannes Schindelin [this message]
2016-10-12 17:06 ` Stefan Beller
2016-10-13 11:11 ` Johannes Schindelin
2016-10-17 7:10 ` Junio C Hamano
2016-10-17 17:58 ` Stefan Beller
2016-10-17 18:28 ` Junio C Hamano
2016-10-17 18:58 ` Stefan Beller
2016-10-17 19:16 ` Junio C Hamano
2016-10-17 19:32 ` Johannes Sixt
2016-10-17 20:07 ` Junio C Hamano
2016-10-18 20:06 ` Johannes Sixt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.20.1610121501390.3492@virtualbox \
--to=johannes.schindelin@gmx.de \
--cc=dennis@kaarsemaker.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=sbeller@google.com \
--cc=venv21@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).