{"thread":{"id":"17220","subject":"[PATCH 3/3] http-push: update tests","startedAt":"2009-01-17T02:59:29Z","lastAt":"2009-01-17T21:24:03Z","messageCount":10,"participants":["Ray Chuan","Johannes Schindelin","Junio C Hamano","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"100792","messageId":"be6fef0d0901161859qbea135bwe89e48caaa69a77c@mail.gmail.com","threadId":"17220","inReplyTo":null,"subject":"[PATCH 3/3] http-push: update tests","fromName":"Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-01-17T02:59:29Z","receivedAt":"2009-01-17T02:59:29Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"note: the test needs to chmod the test_repo.git folder so that\napache/mod_dav can create .DAV folders in it for locking.\n\nSigned-off-by: Tay Ray Chuan \n---\n src/git-1.6.1/t/t5540-http-push.sh | 7 +++----\n 1 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/src/git-1.6.1/t/t5540-http-push.sh\nb/src/git-1.6.1/t/t5540-http-push.sh\nindex da95886..730b9cb 100755\n--- a/src/git-1.6.1/t/t5540-http-push.sh\n+++ b/src/git-1.6.1/t/t5540-http-push.sh\n@@ -51,17 +51,16 @@ test_expect_success 'clone remote repository' '\n \tgit clone $HTTPD_URL/test_repo.git test_repo_clone\n '\n\n-test_expect_failure 'push to remote repository' '\n+test_expect_success 'push to remote repository' '\n \tcd \"$ROOT_PATH\"/test_repo_clone &&\n \t: >path2 &&\n \tgit add path2 &&\n \ttest_tick &&\n \tgit commit -m path2 &&\n-\tgit push &&\n-\t[ -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master\" ]\n+\tgit push origin master\n '\n\n-test_expect_failure 'create and delete remote branch' '\n+test_expect_success 'create and delete remote branch' '\n \tcd \"$ROOT_PATH\"/test_repo_clone &&\n \tgit checkout -b dev &&\n \t: >path3 &&\n-- \n1.6.0.4\n"},{"id":"100803","messageId":"alpine.DEB.1.00.0901170621440.3586@pacific.mpi-cbg.de","threadId":"17220","inReplyTo":"be6fef0d0901161859qbea135bwe89e48caaa69a77c@mail.gmail.com","subject":"Re: [PATCH 3/3] http-push: update tests","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-17T05:23:34Z","receivedAt":"2009-01-17T05:23:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 17 Jan 2009, Ray Chuan wrote:\n\n> note: the test needs to chmod the test_repo.git folder so that\n> apache/mod_dav can create .DAV folders in it for locking.\n\nIs this supposed to explain ...\n\n> @@ -51,17 +51,16 @@ test_expect_success 'clone remote repository' '\n> \tgit clone $HTTPD_URL/test_repo.git test_repo_clone\n> '\n> \n> -test_expect_failure 'push to remote repository' '\n> +test_expect_success 'push to remote repository' '\n> \tcd \"$ROOT_PATH\"/test_repo_clone &&\n> \t: >path2 &&\n> \tgit add path2 &&\n> \ttest_tick &&\n> \tgit commit -m path2 &&\n> -\tgit push &&\n> -\t[ -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master\" ]\n> +\tgit push origin master\n> '\n\n... this removal? I do not think this is a good change, as it removes \na test that is actually pretty important.\n\nBTW I do not understand at all what you mean by \"we need to chmod\". Does \nthe Apache instance not run with the current user's permissions?\n\nCiao,\nDscho\n"},{"id":"100813","messageId":"be6fef0d0901170040r7e11806et87cc5dc3c6f13a2a@mail.gmail.com","threadId":"17220","inReplyTo":"alpine.DEB.1.00.0901170621440.3586@pacific.mpi-cbg.de","subject":"Re: [PATCH 3/3] http-push: update tests","fromName":"Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-01-17T08:40:38Z","receivedAt":"2009-01-17T08:40:38Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sat, Jan 17, 2009 at 1:23 PM, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Sat, 17 Jan 2009, Ray Chuan wrote:\n>\n>> note: the test needs to chmod the test_repo.git folder so that\n>> apache/mod_dav can create .DAV folders in it for locking.\n>\n> Is this supposed to explain ...\n>\n>> @@ -51,17 +51,16 @@ test_expect_success 'clone remote repository' '\n>> git clone $HTTPD_URL/test_repo.git test_repo_clone\n>> '\n>>\n>> -test_expect_failure 'push to remote repository' '\n>> +test_expect_success 'push to remote repository' '\n>> cd \"$ROOT_PATH\"/test_repo_clone &&\n>> : >path2 &&\n>> git add path2 &&\n>> test_tick &&\n>> git commit -m path2 &&\n>> - git push &&\n>> - [ -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master\" ]\n>> + git push origin master\n>> '\n>\n> ... this removal? I do not think this is a good change, as it removes\n> a test that is actually pretty important.\n\ni'm sorry for the poor commit message, what i wanted to do was to\nchange the tests to expect success rather than failure. no tests were\nremoved; only their expected outcomes were modified. currently, the\npushes fail, so the test 'fails as expected'; now the pushes succeed,\nso they shouldn't be expecting failed pushes (or anything else).\n\n> BTW I do not understand at all what you mean by \"we need to chmod\". Does\n> the Apache instance not run with the current user's permissions?\n\ni wrote 'we need to chmod', cos i'm not sure what the permissions are\nrequired. sorry, i didn't notice that apache was running with the\ncurrent user's permissions. but i added the note just in case, for\nusers who don't run apache with the current user, so they modify the\ntest appropriately (ie adding a chmod).\n\n> Ciao,\n> Dscho\n>\n\n\n\n-- \nCheers,\nRay Chuan\n"},{"id":"100869","messageId":"7vsknh7og5.fsf@gitster.siamese.dyndns.org","threadId":"17220","inReplyTo":"be6fef0d0901170040r7e11806et87cc5dc3c6f13a2a@mail.gmail.com","subject":"Re: [PATCH 3/3] http-push: update tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-17T18:54:50Z","receivedAt":"2009-01-17T18:54:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ray Chuan\" writes:\n\n>>> -test_expect_failure 'push to remote repository' '\n>>> +test_expect_success 'push to remote repository' '\n>>> cd \"$ROOT_PATH\"/test_repo_clone &&\n>>> : >path2 &&\n>>> git add path2 &&\n>>> test_tick &&\n>>> git commit -m path2 &&\n>>> - git push &&\n>>> - [ -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master\" ]\n>>> + git push origin master\n>>> '\n>>\n>> ... this removal? I do not think this is a good change, as it removes\n>> a test that is actually pretty important.\n>\n> i'm sorry for the poor commit message, what i wanted to do was to\n> change the tests to expect success rather than failure. no tests were\n> removed; only their expected outcomes were modified. currently, the\n> pushes fail, so the test 'fails as expected'; now the pushes succeed,\n> so they shouldn't be expecting failed pushes (or anything else).\n\nThe original seems to want the push to succeed, and also it wants the file\nrefs/heads/master to be present after the push (presumably because there\nshould be that ref when the push succeeds). If you fixed \"push\" that used\nto fail to succeed, that is great, and s/failure/success/ is a good thing.\n\nBut you are removing something else without explanation. Why do you need\nto remove the part of the test that checks if refs/heads/master is present?\nIs it looking for a file in a wrong place?\n"},{"id":"100874","messageId":"alpine.DEB.1.00.0901172035000.3586@pacific.mpi-cbg.de","threadId":"17220","inReplyTo":"7vsknh7og5.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/3] http-push: update tests","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-17T19:37:38Z","receivedAt":"2009-01-17T19:37:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 17 Jan 2009, Junio C Hamano wrote:\n\n> The original seems to want the push to succeed, and also it wants the \n> file refs/heads/master to be present after the push (presumably because \n> there should be that ref when the push succeeds). If you fixed \"push\" \n> that used to fail to succeed, that is great, and s/failure/success/ is a \n> good thing.\n> \n> But you are removing something else without explanation. Why do you \n> need to remove the part of the test that checks if refs/heads/master is \n> present? Is it looking for a file in a wrong place?\n\nAs I mentioned with two other patches, the push does not succeed, and that \nis the reason for the \"failure\" in test_expect_failure.\n\nIt does not succeed for two reasons:\n\n- due to an off-by-path_len bug, xmalloc() tries to allocate ~4GB of \n memory, which is a bit much, so http-push die()s with an OOM.\n\n- even with that fix, the push fails because it cannot find any common \n refs. It cannot find them because it does not download info/refs as it \n is supposed to do, but it looks through refs/, missing the fact that the \n refs are packed (which it cannot handle).\n\nCiao,\nDscho\n"},{"id":"100875","messageId":"be6fef0d0901171155p26e14aa1t90c0d7b8ec7925f3@mail.gmail.com","threadId":"17220","inReplyTo":"7vsknh7og5.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/3] http-push: update tests","fromName":"Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-01-17T19:55:09Z","receivedAt":"2009-01-17T19:55:09Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sat, Jan 17, 2009 at 6:54 PM, Junio C Hamano wrote:\n> \"Ray Chuan\" writes:\n>\n>>>> -test_expect_failure 'push to remote repository' '\n>>>> +test_expect_success 'push to remote repository' '\n>>>> cd \"$ROOT_PATH\"/test_repo_clone &&\n>>>> : >path2 &&\n>>>> git add path2 &&\n>>>> test_tick &&\n>>>> git commit -m path2 &&\n>>>> - git push &&\n>>>> - [ -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master\" ]\n\ni modified the push arguments as there was no remote ref/branch\nspecified. With a fixed \"git push\", that line says:\n\n No refs in common and none specified; doing nothing.\n\ni'd like to take this chance to inquire, what does the -f, plus square\nbrackets, really mean? i assumed it was to force push to go ahead even\nif \"a remote ref that is not an ancestor of the local ref used to\noverwrite it\" check fails.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"100881","messageId":"m3zlhpy981.fsf@localhost.localdomain","threadId":"17220","inReplyTo":"be6fef0d0901171155p26e14aa1t90c0d7b8ec7925f3@mail.gmail.com","subject":"Re: [PATCH 3/3] http-push: update tests","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-01-17T20:21:37Z","receivedAt":"2009-01-17T20:21:37Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"Ray Chuan\" writes:\n\n>>>>> - git push &&\n>>>>> - [ -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master\" ]\n> \n> i modified the push arguments as there was no remote ref/branch\n> specified. With a fixed \"git push\", that line says:\n> \n> No refs in common and none specified; doing nothing.\n> \n> i'd like to take this chance to inquire, what does the -f, plus square\n> brackets, really mean? i assumed it was to force push to go ahead even\n> if \"a remote ref that is not an ancestor of the local ref used to\n> overwrite it\" check fails.\n\nErrr...\n\n git push &&\n [ -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master\" ]\n\nmeans, do \"git push\", and if it succeeds test (which should really be\nwritten as 'test ' and not '[ ]' I think) if the file\ndoes exists and is regular file ('help test' or 'man test').\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"100882","messageId":"be6fef0d0901171300t44b60aedm801f6f18d88b654b@mail.gmail.com","threadId":"17220","inReplyTo":"m3zlhpy981.fsf@localhost.localdomain","subject":"Re: [PATCH 3/3] http-push: update tests","fromName":"Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-01-17T21:00:16Z","receivedAt":"2009-01-17T21:00:16Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"change tests to expect success.\n\nSigned-off-by: Tay Ray Chuan \n---\n t/t5540-http-push.sh | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh\nindex da95886..4acc087 100755\n--- a/t/t5540-http-push.sh\n+++ b/t/t5540-http-push.sh\n@@ -51,17 +51,17 @@ test_expect_success 'clone remote repository' '\n \tgit clone $HTTPD_URL/test_repo.git test_repo_clone\n '\n\n-test_expect_failure 'push to remote repository' '\n+test_expect_success 'push to remote repository' '\n \tcd \"$ROOT_PATH\"/test_repo_clone &&\n \t: >path2 &&\n \tgit add path2 &&\n \ttest_tick &&\n \tgit commit -m path2 &&\n-\tgit push &&\n+\tgit push origin master &&\n \t[ -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master\" ]\n '\n\n-test_expect_failure 'create and delete remote branch' '\n+test_expect_success 'create and delete remote branch' '\n \tcd \"$ROOT_PATH\"/test_repo_clone &&\n \tgit checkout -b dev &&\n \t: >path3 &&\n-- \n1.5.6.3\n"},{"id":"100883","messageId":"200901172215.37109.jnareb@gmail.com","threadId":"17220","inReplyTo":"be6fef0d0901171300t44b60aedm801f6f18d88b654b@mail.gmail.com","subject":"Re: [PATCH 3/3] http-push: update tests","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-01-17T21:15:35Z","receivedAt":"2009-01-17T21:15:35Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Ray Chuan wrote:\n\n> Subject: [PATCH 3/3] http-push: update tests\n>\n> change tests to expect success.\n\nMinor nit: I would use here\n\nSubject: [PATCH 3/3] http-push: change tests to expect success\n\n-- \nJakub Narebski\nPoland\n"},{"id":"100885","messageId":"alpine.DEB.1.00.0901172223250.3586@pacific.mpi-cbg.de","threadId":"17220","inReplyTo":"be6fef0d0901171300t44b60aedm801f6f18d88b654b@mail.gmail.com","subject":"Re: [PATCH 3/3] http-push: update tests","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-17T21:24:03Z","receivedAt":"2009-01-17T21:24:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 17 Jan 2009, Ray Chuan wrote:\n\n> -test_expect_failure 'push to remote repository' '\n> +test_expect_success 'push to remote repository' '\n> \tcd \"$ROOT_PATH\"/test_repo_clone &&\n> \t: >path2 &&\n> \tgit add path2 &&\n> \ttest_tick &&\n> \tgit commit -m path2 &&\n> -\tgit push &&\n> +\tgit push origin master &&\n> \t[ -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master\" ]\n> '\n\nBut it _should_ succeed without . It should find that \nthere is a master locally and one remotely, and push that.\n\nCiao,\nDscho\n"}]}