Re: [PATCH v4 1/3] clone: do not include authentication data in guessed dir
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 7, 2015, 20:45 UTC
- Message-ID
- <xmqqh9oayghp.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <20150806094732.GA746@pks-xps>
Patrick Steinhardt <ps@pks.im> writes:
Show 42 quoted lines
> On Wed, Aug 05, 2015 at 12:41:27PM -0700, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > 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