[PATCH 3/3] http-push: update tests

[PATCH 3/3] http-push: update tests

[Ray Chuan]

note: the test needs to chmod the test_repo.git folder so that
apache/mod_dav can create .DAV folders in it for locking.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 src/git-1.6.1/t/t5540-http-push.sh |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/git-1.6.1/t/t5540-http-push.sh
b/src/git-1.6.1/t/t5540-http-push.sh
index da95886..730b9cb 100755
--- a/src/git-1.6.1/t/t5540-http-push.sh
+++ b/src/git-1.6.1/t/t5540-http-push.sh
@@ -51,17 +51,16 @@ test_expect_success 'clone remote repository' '
 	git clone $HTTPD_URL/test_repo.git test_repo_clone
 '

-test_expect_failure 'push to remote repository' '
+test_expect_success 'push to remote repository' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	: >path2 &&
 	git add path2 &&
 	test_tick &&
 	git commit -m path2 &&
-	git push &&
-	[ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]
+	git push origin master
 '

-test_expect_failure 'create and delete remote branch' '
+test_expect_success 'create and delete remote branch' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	git checkout -b dev &&
 	: >path3 &&
-- 
1.6.0.4

Re: [PATCH 3/3] http-push: update tests

[Johannes Schindelin]

Hi,

On Sat, 17 Jan 2009, Ray Chuan wrote:

> note: the test needs to chmod the test_repo.git folder so that
> apache/mod_dav can create .DAV folders in it for locking.

Is this supposed to explain ...

> @@ -51,17 +51,16 @@ test_expect_success 'clone remote repository' '
>  	git clone $HTTPD_URL/test_repo.git test_repo_clone
>  '
> 
> -test_expect_failure 'push to remote repository' '
> +test_expect_success 'push to remote repository' '
>  	cd "$ROOT_PATH"/test_repo_clone &&
>  	: >path2 &&
>  	git add path2 &&
>  	test_tick &&
>  	git commit -m path2 &&
> -	git push &&
> -	[ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]
> +	git push origin master
>  '

... this removal?  I do not think this is a good change, as it removes 
a test that is actually pretty important.

BTW I do not understand at all what you mean by "we need to chmod".  Does 
the Apache instance not run with the current user's permissions?

Ciao,
Dscho

Re: [PATCH 3/3] http-push: update tests

[Ray Chuan]

Hi,

On Sat, Jan 17, 2009 at 1:23 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sat, 17 Jan 2009, Ray Chuan wrote:
>
>> note: the test needs to chmod the test_repo.git folder so that
>> apache/mod_dav can create .DAV folders in it for locking.
>
> Is this supposed to explain ...
>
>> @@ -51,17 +51,16 @@ test_expect_success 'clone remote repository' '
>>       git clone $HTTPD_URL/test_repo.git test_repo_clone
>>  '
>>
>> -test_expect_failure 'push to remote repository' '
>> +test_expect_success 'push to remote repository' '
>>       cd "$ROOT_PATH"/test_repo_clone &&
>>       : >path2 &&
>>       git add path2 &&
>>       test_tick &&
>>       git commit -m path2 &&
>> -     git push &&
>> -     [ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]
>> +     git push origin master
>>  '
>
> ... this removal?  I do not think this is a good change, as it removes
> a test that is actually pretty important.

i'm sorry for the poor commit message, what i wanted to do was to
change the tests to expect success rather than failure. no tests were
removed; only their expected outcomes were modified. currently, the
pushes fail, so the test 'fails as expected'; now the pushes succeed,
so they shouldn't be expecting failed pushes (or anything else).

> BTW I do not understand at all what you mean by "we need to chmod".  Does
> the Apache instance not run with the current user's permissions?

i wrote 'we need to chmod', cos i'm not sure what the permissions are
required. sorry, i didn't notice that apache was running with the
current user's permissions. but i added the note just in case, for
users who don't run apache with the current user, so they modify the
test appropriately (ie adding a chmod).

> Ciao,
> Dscho
>



-- 
Cheers,
Ray Chuan

Re: [PATCH 3/3] http-push: update tests

[Junio C Hamano]

"Ray Chuan" <rctay89@gmail.com> writes:

>>> -test_expect_failure 'push to remote repository' '
>>> +test_expect_success 'push to remote repository' '
>>>       cd "$ROOT_PATH"/test_repo_clone &&
>>>       : >path2 &&
>>>       git add path2 &&
>>>       test_tick &&
>>>       git commit -m path2 &&
>>> -     git push &&
>>> -     [ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]
>>> +     git push origin master
>>>  '
>>
>> ... this removal?  I do not think this is a good change, as it removes
>> a test that is actually pretty important.
>
> i'm sorry for the poor commit message, what i wanted to do was to
> change the tests to expect success rather than failure. no tests were
> removed; only their expected outcomes were modified. currently, the
> pushes fail, so the test 'fails as expected'; now the pushes succeed,
> so they shouldn't be expecting failed pushes (or anything else).

The original seems to want the push to succeed, and also it wants the file
refs/heads/master to be present after the push (presumably because there
should be that ref when the push succeeds).  If you fixed "push" that used
to fail to succeed, that is great, and s/failure/success/ is a good thing.

But you are removing something else without explanation.  Why do you need
to remove the part of the test that checks if refs/heads/master is present?
Is it looking for a file in a wrong place?

Re: [PATCH 3/3] http-push: update tests

[Johannes Schindelin]

Hi,

On Sat, 17 Jan 2009, Junio C Hamano wrote:

> The original seems to want the push to succeed, and also it wants the 
> file refs/heads/master to be present after the push (presumably because 
> there should be that ref when the push succeeds).  If you fixed "push" 
> that used to fail to succeed, that is great, and s/failure/success/ is a 
> good thing.
> 
> But you are removing something else without explanation.  Why do you 
> need to remove the part of the test that checks if refs/heads/master is 
> present? Is it looking for a file in a wrong place?

As I mentioned with two other patches, the push does not succeed, and that 
is the reason for the "failure" in test_expect_failure.

It does not succeed for two reasons:

- due to an off-by-path_len bug, xmalloc() tries to allocate ~4GB of 
  memory, which is a bit much, so http-push die()s with an OOM.

- even with that fix, the push fails because it cannot find any common 
  refs.  It cannot find them because it does not download info/refs as it 
  is supposed to do, but it looks through refs/, missing the fact that the 
  refs are packed (which it cannot handle).

Ciao,
Dscho

Re: [PATCH 3/3] http-push: update tests

[Ray Chuan]

Hi,

On Sat, Jan 17, 2009 at 6:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "Ray Chuan" <rctay89@gmail.com> writes:
>
>>>> -test_expect_failure 'push to remote repository' '
>>>> +test_expect_success 'push to remote repository' '
>>>>       cd "$ROOT_PATH"/test_repo_clone &&
>>>>       : >path2 &&
>>>>       git add path2 &&
>>>>       test_tick &&
>>>>       git commit -m path2 &&
>>>> -     git push &&
>>>> -     [ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]

i modified the push arguments as there was no remote ref/branch
specified. With a fixed "git push", that line says:

  No refs in common and none specified; doing nothing.

i'd like to take this chance to inquire, what does the -f, plus square
brackets, really mean? i assumed it was to force push to go ahead even
if "a remote ref that is not an ancestor of the local ref used to
overwrite it" check fails.

-- 
Cheers,
Ray Chuan

Re: [PATCH 3/3] http-push: update tests

[Jakub Narebski]

"Ray Chuan" <rctay89@gmail.com> writes:

>>>>> -     git push &&
>>>>> -     [ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]
> 
> i modified the push arguments as there was no remote ref/branch
> specified. With a fixed "git push", that line says:
> 
>   No refs in common and none specified; doing nothing.
> 
> i'd like to take this chance to inquire, what does the -f, plus square
> brackets, really mean? i assumed it was to force push to go ahead even
> if "a remote ref that is not an ancestor of the local ref used to
> overwrite it" check fails.

Errr...

  git push &&
  [ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]

means, do "git push", and if it succeeds test (which should really be
written as 'test <cond>' and not '[ <cond> ]' I think) if the file
does exists and is regular file ('help test' or 'man test').

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 3/3] http-push: update tests

[Ray Chuan]

change tests to expect success.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 t/t5540-http-push.sh |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index da95886..4acc087 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -51,17 +51,17 @@ test_expect_success 'clone remote repository' '
 	git clone $HTTPD_URL/test_repo.git test_repo_clone
 '

-test_expect_failure 'push to remote repository' '
+test_expect_success 'push to remote repository' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	: >path2 &&
 	git add path2 &&
 	test_tick &&
 	git commit -m path2 &&
-	git push &&
+	git push origin master &&
 	[ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]
 '

-test_expect_failure 'create and delete remote branch' '
+test_expect_success 'create and delete remote branch' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	git checkout -b dev &&
 	: >path3 &&
-- 
1.5.6.3

Re: [PATCH 3/3] http-push: update tests

[Jakub Narebski]

Ray Chuan wrote:

> Subject: [PATCH 3/3] http-push: update tests
>
> change tests to expect success.

Minor nit: I would use here

Subject: [PATCH 3/3] http-push: change tests to expect success

-- 
Jakub Narebski
Poland

Re: [PATCH 3/3] http-push: update tests

[Johannes Schindelin]

Hi,

On Sat, 17 Jan 2009, Ray Chuan wrote:

> -test_expect_failure 'push to remote repository' '
> +test_expect_success 'push to remote repository' '
>  	cd "$ROOT_PATH"/test_repo_clone &&
>  	: >path2 &&
>  	git add path2 &&
>  	test_tick &&
>  	git commit -m path2 &&
> -	git push &&
> +	git push origin master &&
>  	[ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]
>  '

But it _should_ succeed without <remote> <branch>.  It should find that 
there is a master locally and one remotely, and push that.

Ciao,
Dscho