Re: [PATCH v4 1/3] clone: do not include authentication data in guessed dir

Patrick Steinhardt [off-list ref] writes:

On Wed, Aug 05, 2015 at 12:41:27PM -0700, Junio C Hamano wrote:

quoted

Junio C Hamano [off-list ref] writes:

quoted

For completeness, here is what I think the end result (together with
Peff's series) of the test should look like.
...
Note that ssh://user:passw@rd@host:1234/ and user:passw@rd@host:/
tests fail for the same reason (finding @ should be greedy, I think).

And I think this should make it pass.  Just remember the last
occurrence of '@' by moving the 'start' every time we see an '@'
sign.

 builtin/clone.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index cae288f..5d86439 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c

@@ -160,13 +160,12 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 		start += 3;
 
 	/*
-	 * Skip authentication data.
+	 * Skip authentication data, if exists.
 	 */
-	ptr = start;
-	while (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')
-		ptr++;
-	if (*ptr == '@')
-		start = ptr + 1;
+	for (ptr = start; ptr < end && !is_dir_sep(*ptr); ptr++) {
+		if (*ptr == '@')
+			start = ptr + 1;
+	}
 
 	/*
 	 * Strip trailing spaces, slashes and /.git

I guess it makes sense to skip over @-signs greedily. Is there
anything I need to do here or will you squash those changes in?

Sorry but I won't have patience to split the "these need squashing
in" into multiple pieces and add them to appropriate commits in the
5 patch series (counting Peff's two fixes on top of which you built
your 3 patch series) correctly.  You as the original author are much
better equipped to do so than I am, so I'd appreciate if you can
reroll them as a 5-patch series.

Among the changes, the fix to builtin/clone.c needs to be squashed
into your "clone: do not include authentication data in guessed
dir", and the remainder of the patch to t5603 should go to Peff's
"clone: add tests for output directory", but with most of them
marked initially as failing.  And then as you fix them in your
patches, some of the "fail" mark should be dropped.

Thanks.

 builtin/clone.c          | 11 +++++------
 t/t5603-clone-dirname.sh | 14 +++++++++++++-
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index cae288f..5d86439 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c

@@ -160,13 +160,12 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 		start += 3;
 
 	/*
-	 * Skip authentication data.
+	 * Skip authentication data, if exists.
 	 */
-	ptr = start;
-	while (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')
-		ptr++;
-	if (*ptr == '@')
-		start = ptr + 1;
+	for (ptr = start; ptr < end && !is_dir_sep(*ptr); ptr++) {
+		if (*ptr == '@')
+			start = ptr + 1;
+	}
 
 	/*
 	 * Strip trailing spaces, slashes and /.git

diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh
index 27dbd6c..4897ea8 100755
--- a/t/t5603-clone-dirname.sh
+++ b/t/t5603-clone-dirname.sh

@@ -22,7 +22,8 @@ test_clone_dir () {
 	expect=success
 	bare=non-bare
 	clone_opts=
-	for i in "$@"; do
+	for i in "$@"
+	do
 		case "$i" in
 		fail)
 			expect=failure

@@ -61,12 +62,23 @@ test_clone_dir ssh://host/foo/ foo
 test_clone_dir ssh://host/foo.git/ foo
 test_clone_dir ssh://host/foo/.git/ foo
 
+test_clone_dir host:foo/ foo
+test_clone_dir host:foo.git/ foo
+test_clone_dir host:foo/.git/ foo
+
 # omitting the path should default to the hostname
 test_clone_dir ssh://host/ host
 test_clone_dir ssh://host:1234/ host
 test_clone_dir ssh://user@host/ host
+test_clone_dir host:/ host
+
+# auth materials should be redacted
 test_clone_dir ssh://user:password@host/ host
 test_clone_dir ssh://user:password@host:1234/ host
+test_clone_dir ssh://user:passw@rd@host:1234/ host
+test_clone_dir user@host:/ host
+test_clone_dir user:password@host:/ host
+test_clone_dir user:passw@rd@host:/ host
 
 # trailing port-like numbers should not be stripped for paths
 test_clone_dir ssh://user:password@host/test:1234 1234

-- 
2.5.0-423-ga363579

On Fri, Aug 07, 2015 at 01:45:54PM -0700, Junio C Hamano wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

On Wed, Aug 05, 2015 at 12:41:27PM -0700, Junio C Hamano wrote:

quoted

Junio C Hamano [off-list ref] writes:

quoted

For completeness, here is what I think the end result (together with
Peff's series) of the test should look like.
...
Note that ssh://user:passw@rd@host:1234/ and user:passw@rd@host:/
tests fail for the same reason (finding @ should be greedy, I think).

And I think this should make it pass.  Just remember the last
occurrence of '@' by moving the 'start' every time we see an '@'
sign.

 builtin/clone.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index cae288f..5d86439 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c

@@ -160,13 +160,12 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 		start += 3;
 
 	/*
-	 * Skip authentication data.
+	 * Skip authentication data, if exists.
 	 */
-	ptr = start;
-	while (ptr < end && !is_dir_sep(*ptr) && *ptr != '@')
-		ptr++;
-	if (*ptr == '@')
-		start = ptr + 1;
+	for (ptr = start; ptr < end && !is_dir_sep(*ptr); ptr++) {
+		if (*ptr == '@')
+			start = ptr + 1;
+	}
 
 	/*
 	 * Strip trailing spaces, slashes and /.git

I guess it makes sense to skip over @-signs greedily. Is there
anything I need to do here or will you squash those changes in?

Sorry but I won't have patience to split the "these need squashing
in" into multiple pieces and add them to appropriate commits in the
5 patch series (counting Peff's two fixes on top of which you built
your 3 patch series) correctly.  You as the original author are much
better equipped to do so than I am, so I'd appreciate if you can
reroll them as a 5-patch series.

Among the changes, the fix to builtin/clone.c needs to be squashed
into your "clone: do not include authentication data in guessed
dir", and the remainder of the patch to t5603 should go to Peff's
"clone: add tests for output directory", but with most of them
marked initially as failing.  And then as you fix them in your
patches, some of the "fail" mark should be dropped.

Thanks.

No problem, just wanted to make sure on how to proceed.