{"thread":{"id":"34960","subject":"git clone silently aborts if stdout gets a broken pipe","startedAt":"2013-09-18T16:52:13Z","lastAt":"2013-09-19T15:48:47Z","messageCount":11,"participants":["Peter Kjellerstedt","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"227844","messageId":"A612847CFE53224C91B23E3A5B48BAC798CD91DB0B@xmail3.se.axis.com","threadId":"34960","inReplyTo":null,"subject":"git clone silently aborts if stdout gets a broken pipe","fromName":"Peter Kjellerstedt","fromEmail":"peter.kjellerstedt@axis.com","sentAt":"2013-09-18T16:52:13Z","receivedAt":"2013-09-18T16:52:13Z","isPatch":false,"sender":{"key":"peter.kjellerstedt@axis.com","avatar":"https://gravatar.com/avatar/6d5a0182283c8eccd7b134a54dbfd5f30038f3ad4d38b96f424884b614a61ca2?d=mp&s=160"},"body":"One of our Perl scripts that does a git clone suddenly \nstarted to fail when I upgraded to git 1.8.4 from 1.8.3.1.\n\nThe failing Perl code used a construct like this:\n\n\tGit::command_oneline('clone', $url, $path);\n\nThere is no error raised, but the directory specified by \n$path is not created. If I look at the process using strace \nI can see the clone taking place, but then it seems to get \na broken pipe since the code above only cares about the \nfirst line from stdout (and with the addition of \"Checking \nconnectivity...\" git clone now outputs two lines to stdout).\n\nIf I change the code to:\n\n\tmy @foo = Git::command('clone', $url, $path);\n\nit works as expected.\n\nI have attached a simple Perl script that shows the problem.\nRun it as \"clone_test.pl <git url>\". With git 1.8.4 it will \nfail for the first two test cases, whereas with older git \nversions it succeeds for all four test cases.\n\nI hope this is enough information for someone to look into \nthis regression.\n\nBest regards,\n//Peter\n\n"},{"id":"227853","messageId":"20130918184551.GC18821@sigill.intra.peff.net","threadId":"34960","inReplyTo":"A612847CFE53224C91B23E3A5B48BAC798CD91DB0B@xmail3.se.axis.com","subject":"Re: git clone silently aborts if stdout gets a broken pipe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-18T18:45:51Z","receivedAt":"2013-09-18T18:45:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 18, 2013 at 06:52:13PM +0200, Peter Kjellerstedt wrote:\n\n> The failing Perl code used a construct like this:\n> \n> \tGit::command_oneline('clone', $url, $path);\n> \n> There is no error raised, but the directory specified by \n> $path is not created. If I look at the process using strace \n> I can see the clone taking place, but then it seems to get \n> a broken pipe since the code above only cares about the \n> first line from stdout (and with the addition of \"Checking \n> connectivity...\" git clone now outputs two lines to stdout).\n\nI think your perl script is somewhat questionable, as it is making\nassumptions about the output of git-clone, and you would do better to\naccept arbitrary-sized output (or better yet, leave stdout pointing to\nthe user, so they can see the output, which is meant for them).\n\nThat being said, the new messages should almost certainly go to stderr.\n\n-- >8 --\nSubject: [PATCH] clone: write \"checking connectivity\" to stderr\n\nIn commit 0781aa4 (clone: let the user know when\ncheck_everything_connected is run, 2013-05-03), we started\ngiving the user a progress report during clone. However,\nsince the actual work happens in a sub-process, we do not\nuse the usual progress code that counts the objects, but\nrather just print a message ourselves.\n\nThis message goes to stdout via printf, which is unlike\nother progress messages (both the eye candy within clone,\nand the \"checking connectivity\" progress in other commands).\nLet's send it to stderr for consistency.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clone.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex ca3eb68..3c91844 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -551,12 +551,12 @@ static void update_remote_refs(const struct ref *refs,\n \n \tif (check_connectivity) {\n \t\tif (0 <= option_verbosity)\n-\t\t\tprintf(_(\"Checking connectivity... \"));\n+\t\t\tfprintf(stderr, _(\"Checking connectivity... \"));\n \t\tif (check_everything_connected_with_transport(iterate_ref_map,\n \t\t\t\t\t\t\t      0, &rm, transport))\n \t\t\tdie(_(\"remote did not send all necessary objects\"));\n \t\tif (0 <= option_verbosity)\n-\t\t\tprintf(_(\"done\\n\"));\n+\t\t\tfprintf(stderr, _(\"done\\n\"));\n \t}\n \n \tif (refs) {\n-- \n1.8.4.rc4.16.g228394f\n"},{"id":"227855","messageId":"20130918190437.GD18821@sigill.intra.peff.net","threadId":"34960","inReplyTo":"20130918184551.GC18821@sigill.intra.peff.net","subject":"Re: git clone silently aborts if stdout gets a broken pipe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-18T19:04:37Z","receivedAt":"2013-09-18T19:04:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 18, 2013 at 02:45:51PM -0400, Jeff King wrote:\n\n> That being said, the new messages should almost certainly go to stderr.\n> \n> -- >8 --\n> Subject: [PATCH] clone: write \"checking connectivity\" to stderr\n> \n> In commit 0781aa4 (clone: let the user know when\n> check_everything_connected is run, 2013-05-03), we started\n> giving the user a progress report during clone. However,\n> since the actual work happens in a sub-process, we do not\n> use the usual progress code that counts the objects, but\n> rather just print a message ourselves.\n> \n> This message goes to stdout via printf, which is unlike\n> other progress messages (both the eye candy within clone,\n> and the \"checking connectivity\" progress in other commands).\n> Let's send it to stderr for consistency.\n\nHrm, this actually breaks t5701, which expects \"clone 2>err\" to print\nnothing to stderr.\n\nWhat should happen here? The message is emulating the usual progress\nmessages, which are silent when stderr is redirected. So we could\nactually use isatty() in the usual way to suppress them. On the other\nhand, the point of that suppression is that the regular progress code\nproduces long output that is not meant to be seen sequentially (i.e., it\nis overwritten in the terminal with \"\\r\"). But this message does not do\nso. So we can just tweak t5701 to be more careful about what it is\nlooking for.\n\nAlso, we should arguably give the \"Cloning into...\" message the same\ntreatment. We have printed that to stdout for a very long time, so there\nis a slim chance that somebody actually tries to parse it. But I think\nthey are wrong to do so; we already changed it once (in 28ba96a), and\nthese days it is internationalized, anyway.\n\nIn May of 2012 I posted this patch, but it got overlooked, and I forgot\nabout it but carried it in my tree since then. Maybe we should apply it\nnow (it fixes t5701; the \"checking connectivity\" patch can come on top,\nor even just be squashed in).\n\n-- >8 --\nSubject: [PATCH] clone: send diagnostic messages to stderr\n\nPutting messages like \"Cloning into..\" and \"done\" on stdout\nis un-Unix and uselessly clutters the stdout channel. Send\nthem to stderr.\n\nWe have to tweak two tests to accommodate this:\n\n  1. t5601 checks for doubled output due to forking, and\n     doesn't actually care where the output goes; adjust it\n     to check stderr.\n\n  2. t5702 is trying to test whether progress output was\n     sent to stderr, but naively does so by checking\n     whether stderr produced any output. Instead, have it\n     look for \"%\", a token found in progress output but not\n     elsewhere (and which lets us avoid hard-coding the\n     progress text in the test).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clone.c          | 6 +++---\n t/t5601-clone.sh         | 2 +-\n t/t5702-clone-options.sh | 4 ++--\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 3c91844..8723a3a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -379,7 +379,7 @@ static void clone_local(const char *src_repo, const char *dest_repo)\n \t}\n \n \tif (0 <= option_verbosity)\n-\t\tprintf(_(\"done.\\n\"));\n+\t\tfprintf(stderr, _(\"done.\\n\"));\n }\n \n static const char *junk_work_tree;\n@@ -849,9 +849,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \tif (0 <= option_verbosity) {\n \t\tif (option_bare)\n-\t\t\tprintf(_(\"Cloning into bare repository '%s'...\\n\"), dir);\n+\t\t\tfprintf(stderr, _(\"Cloning into bare repository '%s'...\\n\"), dir);\n \t\telse\n-\t\t\tprintf(_(\"Cloning into '%s'...\\n\"), dir);\n+\t\t\tfprintf(stderr, _(\"Cloning into '%s'...\\n\"), dir);\n \t}\n \tinit_db(option_template, INIT_DB_QUIET);\n \twrite_config(&option_config);\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 0629149..b3b11e6 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -36,7 +36,7 @@ test_expect_success C_LOCALE_OUTPUT 'output from clone' '\n \n test_expect_success C_LOCALE_OUTPUT 'output from clone' '\n \trm -fr dst &&\n-\tgit clone -n \"file://$(pwd)/src\" dst >output &&\n+\tgit clone -n \"file://$(pwd)/src\" dst >output 2>&1 &&\n \ttest $(grep Clon output | wc -l) = 1\n '\n \ndiff --git a/t/t5702-clone-options.sh b/t/t5702-clone-options.sh\nindex 85cadfa..67e170e 100755\n--- a/t/t5702-clone-options.sh\n+++ b/t/t5702-clone-options.sh\n@@ -22,14 +22,14 @@ test_expect_success 'redirected clone -v' '\n test_expect_success 'redirected clone' '\n \n \tgit clone \"file://$(pwd)/parent\" clone-redirected >out 2>err &&\n-\ttest_must_be_empty err\n+\t! grep % err\n \n '\n test_expect_success 'redirected clone -v' '\n \n \tgit clone --progress \"file://$(pwd)/parent\" clone-redirected-progress \\\n \t\t>out 2>err &&\n-\ttest -s err\n+\tgrep % err\n \n '\n \n-- \n1.8.4.rc4.16.g228394f\n"},{"id":"227858","messageId":"xmqqmwnaudtg.fsf@gitster.dls.corp.google.com","threadId":"34960","inReplyTo":"20130918190437.GD18821@sigill.intra.peff.net","subject":"Re: git clone silently aborts if stdout gets a broken pipe","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-18T19:31:23Z","receivedAt":"2013-09-18T19:31:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Sep 18, 2013 at 02:45:51PM -0400, Jeff King wrote:\n>\n>> That being said, the new messages should almost certainly go to stderr.\n>> \n>> -- >8 --\n>> Subject: [PATCH] clone: write \"checking connectivity\" to stderr\n>> \n>> In commit 0781aa4 (clone: let the user know when\n>> check_everything_connected is run, 2013-05-03), we started\n>> giving the user a progress report during clone. However,\n>> since the actual work happens in a sub-process, we do not\n>> use the usual progress code that counts the objects, but\n>> rather just print a message ourselves.\n>> \n>> This message goes to stdout via printf, which is unlike\n>> other progress messages (both the eye candy within clone,\n>> and the \"checking connectivity\" progress in other commands).\n>> Let's send it to stderr for consistency.\n>\n> Hrm, this actually breaks t5701, which expects \"clone 2>err\" to print\n> nothing to stderr.\n\nHmm, where in t5701?  Ah, you meant t5702 and possibly t5601.\n\n> What should happen here? The message is emulating the usual progress\n> messages, which are silent when stderr is redirected. So we could\n> actually use isatty() in the usual way to suppress them. On the other\n> hand, the point of that suppression is that the regular progress code\n> produces long output that is not meant to be seen sequentially (i.e., it\n> is overwritten in the terminal with \"\\r\"). But this message does not do\n> so. So we can just tweak t5701 to be more careful about what it is\n> looking for.\n\nI actually think \"it is long and not meant to be seen sequentially\"\nis a bad classifier; these new messages are also progress report in\nthat it reports \"we are now in this phase\".  So if I were to vote, I\nwould say we should apply the same progress-silencing criteria,\npreferrably by not checking isatty() again, but by recording the\ndecision we have already made when squelching the progress during\nthe transfer in order to make sure they stay consistent.\n\n> Also, we should arguably give the \"Cloning into...\" message the same\n> treatment. We have printed that to stdout for a very long time, so there\n> is a slim chance that somebody actually tries to parse it. But I think\n> they are wrong to do so; we already changed it once (in 28ba96a), and\n> these days it is internationalized, anyway.\n\nGood thinking.  Please make it so ;-)\n"},{"id":"227860","messageId":"20130918200152.GA17074@sigill.intra.peff.net","threadId":"34960","inReplyTo":"xmqqmwnaudtg.fsf@gitster.dls.corp.google.com","subject":"Re: git clone silently aborts if stdout gets a broken pipe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-18T20:01:52Z","receivedAt":"2013-09-18T20:01:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 18, 2013 at 12:31:23PM -0700, Junio C Hamano wrote:\n\n> > Hrm, this actually breaks t5701, which expects \"clone 2>err\" to print\n> > nothing to stderr.\n> \n> Hmm, where in t5701?  Ah, you meant t5702 and possibly t5601.\n\nYes, sorry, I meant t5702.\n\n> I actually think \"it is long and not meant to be seen sequentially\"\n> is a bad classifier; these new messages are also progress report in\n> that it reports \"we are now in this phase\".  So if I were to vote, I\n> would say we should apply the same progress-silencing criteria,\n> preferrably by not checking isatty() again, but by recording the\n> decision we have already made when squelching the progress during\n> the transfer in order to make sure they stay consistent.\n\nUnfortunately that decision is made in the transport code, not by clone\nitself. We can cheat and peek at \"transport->progress\" after\ninitializing the transport. That would require some refactoring, though;\nwe print \"Cloning into\" before setting up the transport. And we do not\neven tell the transport about our progress options if we are doing a\nlocal clone.\n\nIf we wanted to _just_ suppress \"Checking connectivity\" (and not\n\"Cloning into...\"), that's a bit easier. And I could see an argument\nthat the former is the only one that falls into the \"progress report\"\ncategory.\n\n> > Also, we should arguably give the \"Cloning into...\" message the same\n> > treatment. We have printed that to stdout for a very long time, so there\n> > is a slim chance that somebody actually tries to parse it. But I think\n> > they are wrong to do so; we already changed it once (in 28ba96a), and\n> > these days it is internationalized, anyway.\n> \n> Good thinking.  Please make it so ;-)\n\nOK. I've squashed the \"use stderr\" patches into one, and added a patch\non top to correctly check the progress flag.\n\n  [1/2]: clone: send diagnostic messages to stderr\n  [2/2]: clone: treat \"checking connectivity\" like other progress\n\n-Peff\n"},{"id":"227861","messageId":"20130918200513.GA731@sigill.intra.peff.net","threadId":"34960","inReplyTo":"20130918200152.GA17074@sigill.intra.peff.net","subject":"[PATCH 1/2] clone: send diagnostic messages to stderr","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-18T20:05:13Z","receivedAt":"2013-09-18T20:05:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Putting messages like \"Cloning into..\" and \"done\" on stdout\nis un-Unix and uselessly clutters the stdout channel. Send\nthem to stderr.\n\nWe have to tweak two tests to accommodate this:\n\n  1. t5601 checks for doubled output due to forking, and\n     doesn't actually care where the output goes; adjust it\n     to check stderr.\n\n  2. t5702 is trying to test whether progress output was\n     sent to stderr, but naively does so by checking\n     whether stderr produced any output. Instead, have it\n     look for \"%\", a token found in progress output but not\n     elsewhere (and which lets us avoid hard-coding the\n     progress text in the test).\n\nThis should not regress any scripts that try to parse the\ncurrent output, as the output is already internationalized\nand therefore unstable.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clone.c          | 10 +++++-----\n t/t5601-clone.sh         |  2 +-\n t/t5702-clone-options.sh |  9 +++++----\n 3 files changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex ca3eb68..8723a3a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -379,7 +379,7 @@ static void clone_local(const char *src_repo, const char *dest_repo)\n \t}\n \n \tif (0 <= option_verbosity)\n-\t\tprintf(_(\"done.\\n\"));\n+\t\tfprintf(stderr, _(\"done.\\n\"));\n }\n \n static const char *junk_work_tree;\n@@ -551,12 +551,12 @@ static void update_remote_refs(const struct ref *refs,\n \n \tif (check_connectivity) {\n \t\tif (0 <= option_verbosity)\n-\t\t\tprintf(_(\"Checking connectivity... \"));\n+\t\t\tfprintf(stderr, _(\"Checking connectivity... \"));\n \t\tif (check_everything_connected_with_transport(iterate_ref_map,\n \t\t\t\t\t\t\t      0, &rm, transport))\n \t\t\tdie(_(\"remote did not send all necessary objects\"));\n \t\tif (0 <= option_verbosity)\n-\t\t\tprintf(_(\"done\\n\"));\n+\t\t\tfprintf(stderr, _(\"done\\n\"));\n \t}\n \n \tif (refs) {\n@@ -849,9 +849,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \tif (0 <= option_verbosity) {\n \t\tif (option_bare)\n-\t\t\tprintf(_(\"Cloning into bare repository '%s'...\\n\"), dir);\n+\t\t\tfprintf(stderr, _(\"Cloning into bare repository '%s'...\\n\"), dir);\n \t\telse\n-\t\t\tprintf(_(\"Cloning into '%s'...\\n\"), dir);\n+\t\t\tfprintf(stderr, _(\"Cloning into '%s'...\\n\"), dir);\n \t}\n \tinit_db(option_template, INIT_DB_QUIET);\n \twrite_config(&option_config);\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 0629149..b3b11e6 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -36,7 +36,7 @@ test_expect_success C_LOCALE_OUTPUT 'output from clone' '\n \n test_expect_success C_LOCALE_OUTPUT 'output from clone' '\n \trm -fr dst &&\n-\tgit clone -n \"file://$(pwd)/src\" dst >output &&\n+\tgit clone -n \"file://$(pwd)/src\" dst >output 2>&1 &&\n \ttest $(grep Clon output | wc -l) = 1\n '\n \ndiff --git a/t/t5702-clone-options.sh b/t/t5702-clone-options.sh\nindex 85cadfa..d3dbdfe 100755\n--- a/t/t5702-clone-options.sh\n+++ b/t/t5702-clone-options.sh\n@@ -19,17 +19,18 @@ test_expect_success 'redirected clone -v' '\n \n '\n \n-test_expect_success 'redirected clone' '\n+test_expect_success 'redirected clone does not show progress' '\n \n \tgit clone \"file://$(pwd)/parent\" clone-redirected >out 2>err &&\n-\ttest_must_be_empty err\n+\t! grep % err\n \n '\n-test_expect_success 'redirected clone -v' '\n+\n+test_expect_success 'redirected clone -v does show progress' '\n \n \tgit clone --progress \"file://$(pwd)/parent\" clone-redirected-progress \\\n \t\t>out 2>err &&\n-\ttest -s err\n+\tgrep % err\n \n '\n \n-- \n1.8.4.rc4.16.g228394f\n"},{"id":"227862","messageId":"20130918200650.GB731@sigill.intra.peff.net","threadId":"34960","inReplyTo":"20130918200152.GA17074@sigill.intra.peff.net","subject":"[PATCH 2/2] clone: treat \"checking connectivity\" like other progress","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-18T20:06:50Z","receivedAt":"2013-09-18T20:06:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When stderr does not point to a tty, we typically suppress\n\"we are now in this phase\" progress reporting (e.g., we ask\nthe server not to send us \"counting objects\" and the like).\n\nThe new \"checking connectivity\" message is in the same vein,\nand should be suppressed. Since clone relies on the\ntransport code to make the decision, we can simply sneak a\npeek at the \"progress\" field of the transport struct. That\nproperly takes into account both the verbosity and progress\noptions we were given, as well as the result of isatty().\n\nNote that we do not set up that progress flag for a local\nclone, as we do not fetch using the transport at all. That's\nacceptable here, though, because we also do not perform a\nconnectivity check in that case.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThough the last paragraph explains why this is OK, it feels a bit\nfragile. I wonder if we should hoist the call to transport_set_verbosity\noutside the \"!is_local\" conditional. I do not think it would hurt\nanything.\n\n builtin/clone.c          | 4 ++--\n t/t5702-clone-options.sh | 3 ++-\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 8723a3a..7c62298 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -550,12 +550,12 @@ static void update_remote_refs(const struct ref *refs,\n \tconst struct ref *rm = mapped_refs;\n \n \tif (check_connectivity) {\n-\t\tif (0 <= option_verbosity)\n+\t\tif (transport->progress)\n \t\t\tfprintf(stderr, _(\"Checking connectivity... \"));\n \t\tif (check_everything_connected_with_transport(iterate_ref_map,\n \t\t\t\t\t\t\t      0, &rm, transport))\n \t\t\tdie(_(\"remote did not send all necessary objects\"));\n-\t\tif (0 <= option_verbosity)\n+\t\tif (transport->progress)\n \t\t\tfprintf(stderr, _(\"done\\n\"));\n \t}\n \ndiff --git a/t/t5702-clone-options.sh b/t/t5702-clone-options.sh\nindex d3dbdfe..9e24ec8 100755\n--- a/t/t5702-clone-options.sh\n+++ b/t/t5702-clone-options.sh\n@@ -22,7 +22,8 @@ test_expect_success 'redirected clone does not show progress' '\n test_expect_success 'redirected clone does not show progress' '\n \n \tgit clone \"file://$(pwd)/parent\" clone-redirected >out 2>err &&\n-\t! grep % err\n+\t! grep % err &&\n+\ttest_i18ngrep ! \"Checking connectivity\" err\n \n '\n \n-- \n1.8.4.rc4.16.g228394f\n"},{"id":"227863","messageId":"20130918203513.GA24928@sigill.intra.peff.net","threadId":"34960","inReplyTo":"20130918200650.GB731@sigill.intra.peff.net","subject":"[PATCH 3/2] clone: always set transport options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-18T20:35:13Z","receivedAt":"2013-09-18T20:35:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 18, 2013 at 04:06:50PM -0400, Jeff King wrote:\n\n> Note that we do not set up that progress flag for a local\n> clone, as we do not fetch using the transport at all. That's\n> acceptable here, though, because we also do not perform a\n> connectivity check in that case.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Though the last paragraph explains why this is OK, it feels a bit\n> fragile. I wonder if we should hoist the call to transport_set_verbosity\n> outside the \"!is_local\" conditional. I do not think it would hurt\n> anything.\n\nActually, I think the option-setting in clone is a little bit broken.\nMostly it is just making fragile assumptions that happen to be true\n(e.g., that fetching the ref list will never care about the progress\nflag), but there are some options that should be respected in both\ncases.\n\nI think we should do this on top.\n\n-- >8 --\nSubject: [PATCH] clone: always set transport options\n\nA clone will always create a transport struct, whether we\nare cloning locally or using an actual protocol. In the\nlocal case, we only use the transport to get the list of\nrefs, and then transfer the objects out-of-band.\n\nHowever, there are many options that we do not bother\nsetting up in the local case. For the most part, these are\nnoops, because they only affect the object-fetching stage\n(e.g., the --depth option).  However, some options do have a\nvisible impact. For example, giving the path to upload-pack\nvia \"-u\" does not currently work for a local clone, even\nthough we need upload-pack to get the ref list.\n\nWe can just drop the conditional entirely and set these\noptions for both local and non-local clones. Rather than\nkeep track of which options impact the object versus the ref\nfetching stage, we can simply let the noops be noops (and\nthe cost of setting the options in the first place is not\nhigh).\n\nThe one exception is that we also check that the transport\nprovides both a \"get_refs_list\" and a \"fetch\" method. We\nwill now be checking the former for both cases (which is\ngood, since a transport that cannot fetch refs would not\nwork for a local clone), and we tweak the conditional to\ncheck for a \"fetch\" only when we are non-local.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe diff is rather unreadable, but using \"show -b\" reveals the actual\nchanges.\n\n builtin/clone.c        | 30 ++++++++++++++----------------\n t/t5701-clone-local.sh |  4 ++++\n 2 files changed, 18 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 7c62298..7ac677d 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -884,27 +884,25 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tremote = remote_get(option_origin);\n \ttransport = transport_get(remote, remote->url[0]);\n \n-\tif (!is_local) {\n-\t\tif (!transport->get_refs_list || !transport->fetch)\n-\t\t\tdie(_(\"Don't know how to clone %s\"), transport->url);\n+\tif (!transport->get_refs_list || (!is_local && !transport->fetch))\n+\t\tdie(_(\"Don't know how to clone %s\"), transport->url);\n \n-\t\ttransport_set_option(transport, TRANS_OPT_KEEP, \"yes\");\n+\ttransport_set_option(transport, TRANS_OPT_KEEP, \"yes\");\n \n-\t\tif (option_depth)\n-\t\t\ttransport_set_option(transport, TRANS_OPT_DEPTH,\n-\t\t\t\t\t     option_depth);\n-\t\tif (option_single_branch)\n-\t\t\ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, \"1\");\n+\tif (option_depth)\n+\t\ttransport_set_option(transport, TRANS_OPT_DEPTH,\n+\t\t\t\t     option_depth);\n+\tif (option_single_branch)\n+\t\ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, \"1\");\n \n-\t\ttransport_set_verbosity(transport, option_verbosity, option_progress);\n+\ttransport_set_verbosity(transport, option_verbosity, option_progress);\n \n-\t\tif (option_upload_pack)\n-\t\t\ttransport_set_option(transport, TRANS_OPT_UPLOADPACK,\n-\t\t\t\t\t     option_upload_pack);\n+\tif (option_upload_pack)\n+\t\ttransport_set_option(transport, TRANS_OPT_UPLOADPACK,\n+\t\t\t\t     option_upload_pack);\n \n-\t\tif (transport->smart_options && !option_depth)\n-\t\t\ttransport->smart_options->check_self_contained_and_connected = 1;\n-\t}\n+\tif (transport->smart_options && !option_depth)\n+\t\ttransport->smart_options->check_self_contained_and_connected = 1;\n \n \trefs = transport_get_remote_refs(transport);\n \ndiff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh\nindex 7ff6e0e..c490368 100755\n--- a/t/t5701-clone-local.sh\n+++ b/t/t5701-clone-local.sh\n@@ -134,4 +134,8 @@ test_expect_success 'cloning a local path with --no-local does not hardlink' '\n \t! repo_is_hardlinked force-nonlocal\n '\n \n+test_expect_success 'cloning locally respects \"-u\" for fetching refs' '\n+\ttest_must_fail git clone --bare -u false a should_not_work.git\n+'\n+\n test_done\n-- \n1.8.4.rc4.16.g228394f\n"},{"id":"227874","messageId":"A612847CFE53224C91B23E3A5B48BAC798CD91DBA7@xmail3.se.axis.com","threadId":"34960","inReplyTo":"20130918184551.GC18821@sigill.intra.peff.net","subject":"RE: git clone silently aborts if stdout gets a broken pipe","fromName":"Peter Kjellerstedt","fromEmail":"peter.kjellerstedt@axis.com","sentAt":"2013-09-19T07:54:38Z","receivedAt":"2013-09-19T07:54:38Z","isPatch":false,"sender":{"key":"peter.kjellerstedt@axis.com","avatar":"https://gravatar.com/avatar/6d5a0182283c8eccd7b134a54dbfd5f30038f3ad4d38b96f424884b614a61ca2?d=mp&s=160"},"body":"> -----Original Message-----\n> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On\n> Behalf Of Jeff King\n> Sent: den 18 september 2013 20:46\n> To: Peter Kjellerstedt\n> Cc: Junio C Hamano; Nguyen Thai Ngoc Duy; git@vger.kernel.org\n> Subject: Re: git clone silently aborts if stdout gets a broken pipe\n> \n> On Wed, Sep 18, 2013 at 06:52:13PM +0200, Peter Kjellerstedt wrote:\n> \n> > The failing Perl code used a construct like this:\n> >\n> > \tGit::command_oneline('clone', $url, $path);\n> >\n> > There is no error raised, but the directory specified by\n> > $path is not created. If I look at the process using strace\n> > I can see the clone taking place, but then it seems to get\n> > a broken pipe since the code above only cares about the\n> > first line from stdout (and with the addition of \"Checking\n> > connectivity...\" git clone now outputs two lines to stdout).\n> \n> I think your perl script is somewhat questionable, as it is making\n> assumptions about the output of git-clone, and you would do better to\n> accept arbitrary-sized output \n\nWell, the whole idea of using Git::command_oneline() is that we \nare only interested in the first line of output, similar to using \n\"| head -1\". If we had wanted all of the output we would have used \nGit::command() instead. Since the Git Perl module is released as a \npart of Git, I would expect it to work as documented regardless of \nwhich Git command is used with Git::command_oneline().\n\nIn the case of git clone the output to stdout is pretty small so \nretrieving all of it would of course not be much overhead, but for \nsome other commands retrieving all output when only the first line \nis wanted (or maybe not even that one) seems unnecessary.\n\nHowever, what surprised me most was that git clone failed silently \nwhen it got a broken pipe. I cannot really see the reason for \naborting due to stdout getting a broken pipe in the first place. \nBut if it is, I would at least have expected an error which our \nscript would have caught and aborted with an appropriate error \nmessage. Now it instead failed later when it actually tried to \naccess the files in the repository it thought it had cloned...\n\n> (or better yet, leave stdout pointing to\n> the user, so they can see the output, which is meant for them).\n\nWell, in this specific case it is a script being run as a cron job \nso anything sent to stdout would cause an unnecessary mail.\n\n> That being said, the new messages should almost certainly go to stderr.\n\nI can but agree.\n\n> -- >8 --\n> Subject: [PATCH] clone: write \"checking connectivity\" to stderr\n> \n> In commit 0781aa4 (clone: let the user know when\n> check_everything_connected is run, 2013-05-03), we started\n> giving the user a progress report during clone. However,\n> since the actual work happens in a sub-process, we do not\n> use the usual progress code that counts the objects, but\n> rather just print a message ourselves.\n> \n> This message goes to stdout via printf, which is unlike\n> other progress messages (both the eye candy within clone,\n> and the \"checking connectivity\" progress in other commands).\n> Let's send it to stderr for consistency.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/clone.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index ca3eb68..3c91844 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -551,12 +551,12 @@ static void update_remote_refs(const struct ref *refs,\n> \n>  \tif (check_connectivity) {\n>  \t\tif (0 <= option_verbosity)\n> -\t\t\tprintf(_(\"Checking connectivity... \"));\n> +\t\t\tfprintf(stderr, _(\"Checking connectivity... \"));\n>  \t\tif (check_everything_connected_with_transport(iterate_ref_map,\n>  \t\t\t\t\t\t\t      0, &rm, transport))\n>  \t\t\tdie(_(\"remote did not send all necessary objects\"));\n>  \t\tif (0 <= option_verbosity)\n> -\t\t\tprintf(_(\"done\\n\"));\n> +\t\t\tfprintf(stderr, _(\"done\\n\"));\n>  \t}\n> \n>  \tif (refs) {\n> --\n> 1.8.4.rc4.16.g228394f\n> \n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n\nThanks for taking the time to provide a solution to the problem.\n\n//Peter\n\n"},{"id":"227876","messageId":"20130919083530.GA12597@sigill.intra.peff.net","threadId":"34960","inReplyTo":"A612847CFE53224C91B23E3A5B48BAC798CD91DBA7@xmail3.se.axis.com","subject":"Re: git clone silently aborts if stdout gets a broken pipe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-19T08:35:30Z","receivedAt":"2013-09-19T08:35:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 19, 2013 at 09:54:38AM +0200, Peter Kjellerstedt wrote:\n\n> > I think your perl script is somewhat questionable, as it is making\n> > assumptions about the output of git-clone, and you would do better to\n> > accept arbitrary-sized output \n> \n> Well, the whole idea of using Git::command_oneline() is that we \n> are only interested in the first line of output, similar to using \n> \"| head -1\". If we had wanted all of the output we would have used \n> Git::command() instead. Since the Git Perl module is released as a \n> part of Git, I would expect it to work as documented regardless of \n> which Git command is used with Git::command_oneline().\n\nI think command_oneline is exactly like \"| head -1\" in this case. Doing\n\"git clone | head -1\" would also fail, and should not be used. In\ngeneral, you do not want to put a limiting pipe on a command with side\neffects beyond output. The design of unix pipes and SIGPIPE is such that\nyou can do \"generate_output | head\", and \"generate_output\" will get\nSIGPIPE and die after realizing that its writer no longer cares about\nthe output. But if your command is doing something besides output, that\nassumption doesn't hold.\n\nArguably, \"git clone\" should be taking the initiative to ignore SIGPIPE\nitself.  Its primary function is not output, but doing the clone. If\noutput fails, we would want to continue the clone, not die.\n\nBy the way, did you actually want to capture the stdout of git-clone, or\nwere you just trying to suppress it? Because the eventual patch I posted\nsends it to stderr, under the assumption that what used to go to stdout\nshould not be captured and parsed (because it is localized and subject\nto change).\n\n> However, what surprised me most was that git clone failed silently \n> when it got a broken pipe.\n\nIt's not \"git clone\" that is doing this, I think, but rather the design\nof command_oneline. If I do:\n\n  (sleep 1; git clone ...; echo >&2 exit=$?) | false\n\nthen I see:\n\n  exit=141\n\nThat is, clone dies from SIGPIPE trying to write \"Cloning into...\". But\ncommand_oneline is specifically designed to ignore SIGPIPE death,\nbecause you would want something like:\n\n  command_oneline(\"git\", \"rev-list\", \"$A..$B\");\n\nto give you the first line, and then you do not care if the rest of the\nrev-list dies due to SIGPIPE (it is a good thing, because by closing the\npipe you are telling it that its output is not needed). It may be that\nthe documentation for command_oneline can be improved to mention this\nsubtlety.\n\n-Peff\n"},{"id":"227887","messageId":"A612847CFE53224C91B23E3A5B48BAC798CDF1DC31@xmail3.se.axis.com","threadId":"34960","inReplyTo":"20130919083530.GA12597@sigill.intra.peff.net","subject":"RE: git clone silently aborts if stdout gets a broken pipe","fromName":"Peter Kjellerstedt","fromEmail":"peter.kjellerstedt@axis.com","sentAt":"2013-09-19T15:48:47Z","receivedAt":"2013-09-19T15:48:47Z","isPatch":false,"sender":{"key":"peter.kjellerstedt@axis.com","avatar":"https://gravatar.com/avatar/6d5a0182283c8eccd7b134a54dbfd5f30038f3ad4d38b96f424884b614a61ca2?d=mp&s=160"},"body":"> -----Original Message-----\n> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On\n> Behalf Of Jeff King\n> Sent: den 19 september 2013 10:36\n> To: Peter Kjellerstedt\n> Cc: Junio C Hamano; Nguyen Thai Ngoc Duy; git@vger.kernel.org\n> Subject: Re: git clone silently aborts if stdout gets a broken pipe\n> \n> On Thu, Sep 19, 2013 at 09:54:38AM +0200, Peter Kjellerstedt wrote:\n> \n> > > I think your perl script is somewhat questionable, as it is making\n> > > assumptions about the output of git-clone, and you would do better\n> to\n> > > accept arbitrary-sized output\n> >\n> > Well, the whole idea of using Git::command_oneline() is that we\n> > are only interested in the first line of output, similar to using\n> > \"| head -1\". If we had wanted all of the output we would have used\n> > Git::command() instead. Since the Git Perl module is released as a\n> > part of Git, I would expect it to work as documented regardless of\n> > which Git command is used with Git::command_oneline().\n> \n> I think command_oneline is exactly like \"| head -1\" in this case. Doing\n> \"git clone | head -1\" would also fail, and should not be used. In\n> general, you do not want to put a limiting pipe on a command with side\n> effects beyond output. The design of unix pipes and SIGPIPE is such that\n> you can do \"generate_output | head\", and \"generate_output\" will get\n> SIGPIPE and die after realizing that its writer no longer cares about\n> the output. But if your command is doing something besides output, that\n> assumption doesn't hold.\n\nA very valid point.\n\n> Arguably, \"git clone\" should be taking the initiative to ignore SIGPIPE\n> itself.  Its primary function is not output, but doing the clone. If\n> output fails, we would want to continue the clone, not die.\n> \n> By the way, did you actually want to capture the stdout of git-clone, or\n> were you just trying to suppress it? Because the eventual patch I posted\n> sends it to stderr, under the assumption that what used to go to stdout\n> should not be captured and parsed (because it is localized and subject\n> to change).\n\nNo, we were not really interested in the output to stdout (which is \nwhy the return value from Git::command_oneline() was ignored).\n\n> > However, what surprised me most was that git clone failed silently\n> > when it got a broken pipe.\n> \n> It's not \"git clone\" that is doing this, I think, but rather the design\n> of command_oneline. If I do:\n> \n>   (sleep 1; git clone ...; echo >&2 exit=$?) | false\n> \n> then I see:\n> \n>   exit=141\n> \n> That is, clone dies from SIGPIPE trying to write \"Cloning into...\". But\n> command_oneline is specifically designed to ignore SIGPIPE death,\n> because you would want something like:\n> \n>   command_oneline(\"git\", \"rev-list\", \"$A..$B\");\n> \n> to give you the first line, and then you do not care if the rest of the\n> rev-list dies due to SIGPIPE (it is a good thing, because by closing the\n> pipe you are telling it that its output is not needed). It may be that\n> the documentation for command_oneline can be improved to mention this\n> subtlety.\n\nOk, all of it makes sense now. Thank you for the explanation.\nI have corrected our script so it now works correctly with \ngit 1.8.4 as well.\n\n> -Peff\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n\n//Peter\n\n"}]}