{"thread":{"id":"31186","subject":"Did we break receive-pack recently?","startedAt":"2012-08-05T01:55:23Z","lastAt":"2012-08-07T22:55:02Z","messageCount":15,"participants":["Junio C Hamano","Brandon Casey","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"196480","messageId":"7vk3xe6r1w.fsf@alter.siamese.dyndns.org","threadId":"31186","inReplyTo":null,"subject":"Did we break receive-pack recently?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-05T01:55:23Z","receivedAt":"2012-08-05T01:55:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I just saw this:\n\n $ git push ko\n ko: Counting objects: 332, done.\n Delta compression using up to 4 threads.\n Compressing objects: 100% (110/110), done.\n Writing objects: 100% (130/130), 32.27 KiB, done.\n Total 130 (delta 106), reused 21 (delta 20)\n Auto packing the repository for optimum performance.\n fatal: protocol error: bad line length character: Remo\n error: error in sideband demultiplexer\n To ra.kernel.org:/pub/scm/git/git.git\n ...\n\nWhat is unusual with this push is that it happened to trigger the\nauto-gc on the receiving end and the message \"Auto packing the\nrepository...\" came back to the pusher just fine, but somebody\nnearby seem to have tried to say \"Remo\"(te---probably) without\nproperly using the sideband.\n\nDoes this ring a bell to anybody?\n"},{"id":"196524","messageId":"CA+sFfMcA2qUcigPg_ijWJmiTKYY9V4f4f6XQp8xT76wLaUXSxA@mail.gmail.com","threadId":"31186","inReplyTo":"7vk3xe6r1w.fsf@alter.siamese.dyndns.org","subject":"Re: Did we break receive-pack recently?","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-08-06T01:37:51Z","receivedAt":"2012-08-06T01:37:51Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Sat, Aug 4, 2012 at 6:55 PM, Junio C Hamano wrote:\n> I just saw this:\n>\n> $ git push ko\n> ko: Counting objects: 332, done.\n> Delta compression using up to 4 threads.\n> Compressing objects: 100% (110/110), done.\n> Writing objects: 100% (130/130), 32.27 KiB, done.\n> Total 130 (delta 106), reused 21 (delta 20)\n> Auto packing the repository for optimum performance.\n> fatal: protocol error: bad line length character: Remo\n> error: error in sideband demultiplexer\n> To ra.kernel.org:/pub/scm/git/git.git\n> ...\n>\n> What is unusual with this push is that it happened to trigger the\n> auto-gc on the receiving end and the message \"Auto packing the\n> repository...\" came back to the pusher just fine, but somebody\n> nearby seem to have tried to say \"Remo\"(te---probably) without\n> properly using the sideband.\n\nOr perhaps \"Remo\" is short for \"Removing...\".\n\nPerhaps this is the source:\n\n $ grep Remo builtin/prune.c\n printf(\"Removing stale temporary file %s\\n\", fullpath);\n\n-Brandon\n"},{"id":"196534","messageId":"CA+sFfMdXc+usFRnCNVoke91_X2qWZARTvPHO=B7Ukxr-j7JB2g@mail.gmail.com","threadId":"31186","inReplyTo":"CA+sFfMcA2qUcigPg_ijWJmiTKYY9V4f4f6XQp8xT76wLaUXSxA@mail.gmail.com","subject":"Re: Did we break receive-pack recently?","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-08-06T03:32:29Z","receivedAt":"2012-08-06T03:32:29Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Sun, Aug 5, 2012 at 6:37 PM, Brandon Casey wrote:\n> On Sat, Aug 4, 2012 at 6:55 PM, Junio C Hamano wrote:\n>> I just saw this:\n>>\n>> $ git push ko\n>> ko: Counting objects: 332, done.\n>> Delta compression using up to 4 threads.\n>> Compressing objects: 100% (110/110), done.\n>> Writing objects: 100% (130/130), 32.27 KiB, done.\n>> Total 130 (delta 106), reused 21 (delta 20)\n>> Auto packing the repository for optimum performance.\n>> fatal: protocol error: bad line length character: Remo\n>> error: error in sideband demultiplexer\n>> To ra.kernel.org:/pub/scm/git/git.git\n>> ...\n>>\n>> What is unusual with this push is that it happened to trigger the\n>> auto-gc on the receiving end and the message \"Auto packing the\n>> repository...\" came back to the pusher just fine, but somebody\n>> nearby seem to have tried to say \"Remo\"(te---probably) without\n>> properly using the sideband.\n>\n> Or perhaps \"Remo\" is short for \"Removing...\".\n>\n> Perhaps this is the source:\n>\n> $ grep Remo builtin/prune.c\n> printf(\"Removing stale temporary file %s\\n\", fullpath);\n\nVerified...\n\ntest_path=`pwd` &&\ngit init test_repo1 &&\n( cd test_repo1 &&\n echo D >file.txt &&\n git add . &&\n git commit -m 'Commit something that hashes to 17...' &&\n echo MN >file.txt &&\n git commit -a -m 'Commit something else that hashes to 17...'\n) &&\ngit init --bare test_repo2.git &&\n( cd test_repo2.git &&\n git config gc.auto 1 &&\n touch -d '2012-07-01' objects/tmp_test\n) &&\n( cd test_repo1 &&\n git push \"file://$test_path/test_repo2.git\" HEAD:refs/heads/master\n)\n\nIt seems to have been broken since we added 'gc --auto' to receive\npack in 2009 (or maybe since I added that printf to prune.c in 2008\ndepending on how you look at it :b ). Apparently it's something not\nvery likely to be triggered. Probably because most servers running\ngit daemon frequently run 'git gc'. Wonder what k.org's policy is?\n\nI think the original thinking behind writing to stdout\nindiscriminately was that removing a temporary object was something\nthat was considered unusual, so the user should always be informed.\nThis is unlike removing a stale object, which is something that is\nexpected during normal usage. If a stale temporary object is removed,\nthen it means that some piece of git which should have removed it,\nfailed to do so.\n\nWe could write the message to stderr instead, but I think the full\npath to a temporary stale object is not appropriate to communicate to\nremote users over the wire.\n\nSo, I think it's best just to protect it with 'if (show_only ||\nverbose)' like the other informational messages.\n\nPatch forthcoming, hopefully with a test. Doesn't look like we have\nanything testing the auto-gc spawned from receive-pack. I'll see if I\ncan come up with something.\n\n-Brandon\n"},{"id":"196589","messageId":"1344315709-15897-1-git-send-email-drafnel@gmail.com","threadId":"31186","inReplyTo":"CA+sFfMdXc+usFRnCNVoke91_X2qWZARTvPHO=B7Ukxr-j7JB2g@mail.gmail.com","subject":"[PATCH 1/2] t/t5400: demonstrate breakage caused by informational message from prune","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-08-07T05:01:48Z","receivedAt":"2012-08-07T05:01:48Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"When receive-pack triggers 'git gc --auto' and 'git prune' is called to\nremove a stale temporary object, 'git prune' prints an informational\nmessage to stdout about the file that it will remove. Since this message\nis written to stdout, it is sent back over the transport channel to the git\nclient which tries to interpret it as part of the pack protocol and then\npromptly terminates with a complaint about a protocol error.\n\nIntroduce a test which exercises the auto-gc functionality of receive-pack\nand demonstrates this breakage.\n\nSigned-off-by: Brandon Casey \n---\n t/t5400-send-pack.sh | 35 +++++++++++++++++++++++++++++++++++\n 1 file changed, 35 insertions(+)\n\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex 0eace37..04a8791 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -145,6 +145,41 @@ test_expect_success 'push --all excludes remote-tracking hierarchy' '\n \t)\n '\n \n+test_expect_failure 'receive-pack runs auto-gc in remote repo' '\n+\trm -rf parent child &&\n+\tgit init parent &&\n+\t(\n+\t # Setup a repo with 2 packs\n+\t cd parent &&\n+\t echo \"Some text\" >file.txt &&\n+\t git add . &&\n+\t git commit -m \"Initial commit\" &&\n+\t git repack -adl &&\n+\t echo \"Some more text\" >>file.txt &&\n+\t git commit -a -m \"Second commit\" &&\n+\t git repack\n+\t) &&\n+\tcp -a parent child &&\n+\t(\n+\t # Set the child to auto-pack if more than one pack exists\n+\t cd child &&\n+\t git config gc.autopacklimit 1 &&\n+\t git branch test_auto_gc &&\n+\t # And create a file that follows the temporary object naming\n+\t # convention for the auto-gc to remove\n+\t : >.git/objects/tmp_test_object &&\n+\t test-chmtime =-1209601 .git/objects/tmp_test_object\n+\t) &&\n+\t(\n+\t cd parent &&\n+\t echo \"Even more text\" >>file.txt &&\n+\t git commit -a -m \"Third commit\" &&\n+\t git send-pack ../child HEAD:refs/heads/test_auto_gc >output 2>&1 &&\n+\t grep \"Auto packing the repository for optimum performance.\" output\n+\t) &&\n+\ttest ! -e child/.git/objects/tmp_test_object\n+'\n+\n rewound_push_setup() {\n \trm -rf parent child &&\n \tmkdir parent &&\n-- \n1.7.12.rc1.17.g9a7365c\n"},{"id":"196590","messageId":"1344315709-15897-2-git-send-email-drafnel@gmail.com","threadId":"31186","inReplyTo":"1344315709-15897-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 2/2] prune.c: only print informational message in show_only or verbose mode","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-08-07T05:01:49Z","receivedAt":"2012-08-07T05:01:49Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"This informational message can cause a problem if 'git prune' is spawned\nfrom an auto-gc during receive-pack. In this case, the informational\nmessage will be sent back over the wire to the git client and the client\nwill try to interpret it as part of the pack protocol and will produce an\nerror.\n\nSo let's refrain from producing this message unless show_only or verbose\nis enabled.\n\nThis fixes the test in t5400.\n\nSigned-off-by: Brandon Casey \n---\n builtin/prune.c | 3 ++-\n t/t5400-send-pack.sh | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex b99b635..6cb9944 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -25,7 +25,8 @@ static int prune_tmp_object(const char *path, const char *filename)\n \t\treturn error(\"Could not stat '%s'\", fullpath);\n \tif (st.st_mtime > expire)\n \t\treturn 0;\n-\tprintf(\"Removing stale temporary file %s\\n\", fullpath);\n+\tif (show_only || verbose)\n+\t\tprintf(\"Removing stale temporary file %s\\n\", fullpath);\n \tif (!show_only)\n \t\tunlink_or_warn(fullpath);\n \treturn 0;\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex 04a8791..250c720 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -145,7 +145,7 @@ test_expect_success 'push --all excludes remote-tracking hierarchy' '\n \t)\n '\n \n-test_expect_failure 'receive-pack runs auto-gc in remote repo' '\n+test_expect_success 'receive-pack runs auto-gc in remote repo' '\n \trm -rf parent child &&\n \tgit init parent &&\n \t(\n-- \n1.7.12.rc1.17.g9a7365c\n"},{"id":"196591","messageId":"7vtxwfw9rp.fsf@alter.siamese.dyndns.org","threadId":"31186","inReplyTo":"1344315709-15897-2-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-07T05:28:42Z","receivedAt":"2012-08-07T05:28:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n\nThis patch may fix the immediate symptom, but I think the right fix\nis to correct the way \"gc\" is invoked by receive-pack, so that\nnothing \"gc\" writes to its standard output can leak to the standard\noutput of the receive-pack. After all, receive-pack is the one that\nknows that its output is a structured protocol communication channel\nand should not be contaminated by random crufts. Receive-pack is\nthe one that is responsible to avoid this kind of problem in the\nfirst place.\n\nOnce that fix is done, any future changes to \"gc\" or its subprograms\nwon't be able to cause the same breakage again. Which automatically\nmakes your patch unnecessary.\n\nSomething along this line, perhaps.\n\nNote that this chooses to expose what comes out of the standard\noutput of the subprocess to the standard error to be shown to the\nuser sitting on the other end. This is in line with what we do to\nall of our hooks (Cf. cd83c74 (Redirect update hook stdout to\nstderr., 2006-12-30)).\n\nIf we instead want to discard the standard output, we would need to\neither extend run_command_v_opt(), or set up our own child_process\nand spawn the subprocess using the underlying run_command() API\nourselves.\n\n builtin/receive-pack.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex ee7751a..19bdc66 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -979,7 +979,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\t\tconst char *argv_gc_auto[] = {\n \t\t\t\t\"gc\", \"--auto\", \"--quiet\", NULL,\n \t\t\t};\n-\t\t\trun_command_v_opt(argv_gc_auto, RUN_GIT_CMD);\n+\t\t\tint opt = RUN_GIT_CMD | RUN_COMMAND_STDOUT_TO_STDERR;\n+\t\t\trun_command_v_opt(argv_gc_auto, opt);\n \t\t}\n \t\tif (auto_update_server_info)\n \t\t\tupdate_server_info(0);\n \n"},{"id":"196592","messageId":"20120807053225.GA1541@sigill.intra.peff.net","threadId":"31186","inReplyTo":"1344315709-15897-2-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-07T05:32:25Z","receivedAt":"2012-08-07T05:32:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 06, 2012 at 10:01:49PM -0700, Brandon Casey wrote:\n\n> This informational message can cause a problem if 'git prune' is spawned\n> from an auto-gc during receive-pack. In this case, the informational\n> message will be sent back over the wire to the git client and the client\n> will try to interpret it as part of the pack protocol and will produce an\n> error.\n> \n> So let's refrain from producing this message unless show_only or verbose\n> is enabled.\n\nThis seems like a band-aid. The real problem is that auto-gc can\ninterfere with the pack protocol, which it should not be allowed to do,\nno matter what it produces.\n\nWe could fix that root cause with this patch (on top of your 1/2):\n\n-- >8 --\nSubject: [PATCH] receive-pack: redirect auto-gc stdout to stderr\n\nIn some cases, git-gc may produce informational messages to\nstdout, rather than stderr. This is bad for receive-pack,\nbecause its stdout (and therefore that of its child) is\nconnected to a git client and speaking pack protocol.\nInstead, let's redirect these messages to stderr to avoid\ninterference and let the client see them.\n\nSigned-off-by: Jeff King \n---\nWe already do the same thing for all of the hooks we run. With this\nchange, all sub-processes have their stdout redirected (either to a\npipe, or to stderr) except git-unpack-objects.\n\nLooking at unpack-objects, it should not write anything to stdout under\nnormal circumstances. However, if it is fed more bytes than the\npack data claims (e.g., extra entries beyond what the header claims), it\nwill send them to stdout. I've never heard of that happening, but\nprobably it should go to /dev/null, and/or flag an error.\n\n builtin/receive-pack.c | 3 ++-\n t/t5400-send-pack.sh | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 0afb8b2..e0b9f2e 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -977,7 +977,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\t\tconst char *argv_gc_auto[] = {\n \t\t\t\t\"gc\", \"--auto\", \"--quiet\", NULL,\n \t\t\t};\n-\t\t\trun_command_v_opt(argv_gc_auto, RUN_GIT_CMD);\n+\t\t\trun_command_v_opt(argv_gc_auto,\n+\t\t\t\t\t RUN_GIT_CMD | RUN_COMMAND_STDOUT_TO_STDERR);\n \t\t}\n \t\tif (auto_update_server_info)\n \t\t\tupdate_server_info(0);\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex 04a8791..250c720 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -145,7 +145,7 @@ test_expect_success 'push --all excludes remote-tracking hierarchy' '\n \t)\n '\n \n-test_expect_failure 'receive-pack runs auto-gc in remote repo' '\n+test_expect_success 'receive-pack runs auto-gc in remote repo' '\n \trm -rf parent child &&\n \tgit init parent &&\n \t(\n-- \n1.7.12.rc1.12.g6d3a2d7\n"},{"id":"196593","messageId":"7vpq73w9i8.fsf@alter.siamese.dyndns.org","threadId":"31186","inReplyTo":"7vtxwfw9rp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-07T05:34:23Z","receivedAt":"2012-08-07T05:34:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Note that this chooses to expose what comes out of the standard\n> output of the subprocess to the standard error to be shown to the\n> user sitting on the other end. This is in line with what we do to\n> all of our hooks (Cf. cd83c74 (Redirect update hook stdout to\n> stderr., 2006-12-30)).\n\nOk, now a tested patch, on top of your 1/2\n\n-- >8 --\nSubject: [PATCH] receive-pack: do not leak output from auto-gc to standard output\n\nThe standard output channel of receive-pack is a structured protocol\nchannel, and subprocesses must never be allowed to leak anything\ninto it by writing to their standard output.\n\nUse RUN_COMMAND_STDOUT_TO_STDERR option to run_command_v_opt() just\nlike we do when running hooks to prevent output from \"gc\" leaking to\nthe standard output.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/receive-pack.c | 3 ++-\n t/t5400-send-pack.sh | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 0afb8b2..3f05d97 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -977,7 +977,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\t\tconst char *argv_gc_auto[] = {\n \t\t\t\t\"gc\", \"--auto\", \"--quiet\", NULL,\n \t\t\t};\n-\t\t\trun_command_v_opt(argv_gc_auto, RUN_GIT_CMD);\n+\t\t\tint opt = RUN_GIT_CMD | RUN_COMMAND_STDOUT_TO_STDERR;\n+\t\t\trun_command_v_opt(argv_gc_auto, opt);\n \t\t}\n \t\tif (auto_update_server_info)\n \t\t\tupdate_server_info(0);\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex 04a8791..250c720 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -145,7 +145,7 @@ test_expect_success 'push --all excludes remote-tracking hierarchy' '\n \t)\n '\n \n-test_expect_failure 'receive-pack runs auto-gc in remote repo' '\n+test_expect_success 'receive-pack runs auto-gc in remote repo' '\n \trm -rf parent child &&\n \tgit init parent &&\n \t(\n-- \n1.7.12.rc1.93.g8914ab8\n"},{"id":"196594","messageId":"CA+sFfMdVhTwAFLUgrO-mLBh8apG-5X1OJKCN9xgq3-N+1RBrvg@mail.gmail.com","threadId":"31186","inReplyTo":"7vpq73w9i8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-08-07T05:44:07Z","receivedAt":"2012-08-07T05:44:07Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Mon, Aug 6, 2012 at 10:34 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n> Ok, now a tested patch, on top of your 1/2\n\n\nOn Mon, Aug 6, 2012 at 10:32 PM, Jeff King wrote:\n>\n> This seems like a band-aid. The real problem is that auto-gc can\n> interfere with the pack protocol, which it should not be allowed to do,\n> no matter what it produces.\n>\n> We could fix that root cause with this patch (on top of your 1/2):\n\nAnyone else? :)\n\nAh, I wasn't aware of that feature of run_command. Both look obviously correct.\n\nAnd the comment I made yesterday about leaking the full path to the\nremote end can be disregarded, since prune will report the path\nrelative to the repository base.\n\nThanks,\n-Brandon\n"},{"id":"196596","messageId":"20120807060311.GB13222@sigill.intra.peff.net","threadId":"31186","inReplyTo":"CA+sFfMdVhTwAFLUgrO-mLBh8apG-5X1OJKCN9xgq3-N+1RBrvg@mail.gmail.com","subject":"Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-07T06:03:11Z","receivedAt":"2012-08-07T06:03:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 06, 2012 at 10:44:07PM -0700, Brandon Casey wrote:\n\n> On Mon, Aug 6, 2012 at 10:32 PM, Jeff King wrote:\n> >\n> > This seems like a band-aid. The real problem is that auto-gc can\n> > interfere with the pack protocol, which it should not be allowed to do,\n> > no matter what it produces.\n> >\n> > We could fix that root cause with this patch (on top of your 1/2):\n> \n> Anyone else? :)\n\nSorry to gang up on you. :)\n\nI still think your 2/2 is worth doing independently, though. It is silly\nthat git-prune will not mention pruned objects without \"-v\", but will\nmention temporary files. They should be in the same category.\n\n-Peff\n"},{"id":"196598","messageId":"CA+sFfMc28N2eKNa=GiKHvxOeN3u=-ruFQqTBz7cbCGX-G=TTgA@mail.gmail.com","threadId":"31186","inReplyTo":"20120807060311.GB13222@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-08-07T06:33:36Z","receivedAt":"2012-08-07T06:33:36Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Mon, Aug 6, 2012 at 11:03 PM, Jeff King wrote:\n> On Mon, Aug 06, 2012 at 10:44:07PM -0700, Brandon Casey wrote:\n>> Anyone else? :)\n>\n> Sorry to gang up on you. :)\n\nHeh. :b\n\n> I still think your 2/2 is worth doing independently, though. It is silly\n> that git-prune will not mention pruned objects without \"-v\", but will\n> mention temporary files. They should be in the same category.\n\nAs I mentioned in an earlier message, I think the original thinking\nwas that removing a temporary object should be an unusual occurrence\nthat indicates a failure of some sort, so you want to inform the user\nwho may want to investigate (of course the file's gone, so what's to\ninvestigate). Removing a stale object file on the other hand is just\npart of the normal operation. That is why the former is always\nprinted out and the latter only when -v is used.\n\nThat was the original thinking, but I don't think it matters very\nmuch. Printing both using the same conditions seems valid. My commit\nmessage should be scrapped and replaced with something like your\nparagraph though..\n\n-Brandon\n"},{"id":"196606","messageId":"7vhasewvy6.fsf@alter.siamese.dyndns.org","threadId":"31186","inReplyTo":"CA+sFfMc28N2eKNa=GiKHvxOeN3u=-ruFQqTBz7cbCGX-G=TTgA@mail.gmail.com","subject":"Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-07T15:41:53Z","receivedAt":"2012-08-07T15:41:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> On Mon, Aug 6, 2012 at 11:03 PM, Jeff King wrote:\n>> On Mon, Aug 06, 2012 at 10:44:07PM -0700, Brandon Casey wrote:\n>>> Anyone else? :)\n>>\n>> Sorry to gang up on you. :)\n>\n> Heh. :b\n>\n>> I still think your 2/2 is worth doing independently, though. It is silly\n>> that git-prune will not mention pruned objects without \"-v\", but will\n>> mention temporary files. They should be in the same category.\n>\n> As I mentioned in an earlier message, I think the original thinking\n> was that removing a temporary object should be an unusual occurrence\n> that indicates a failure of some sort, so you want to inform the user\n> who may want to investigate (of course the file's gone, so what's to\n> investigate). Removing a stale object file on the other hand is just\n> part of the normal operation. That is why the former is always\n> printed out and the latter only when -v is used.\n\nThat matches my understanding, modulo \"may want to investigate\" is\nprobably more like \"may want to be reminded of an earlier repack\nthat was aborted\".\n\n> That was the original thinking, but I don't think it matters very\n> much. Printing both using the same conditions seems valid.\n\nYeah, I agree that it does not make much difference either way and\nboth ways of thinking feel equally valid.\n"},{"id":"196626","messageId":"7vlihqv0ks.fsf@alter.siamese.dyndns.org","threadId":"31186","inReplyTo":"20120807060311.GB13222@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-07T21:44:51Z","receivedAt":"2012-08-07T21:44:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I still think your 2/2 is worth doing independently, though. It is silly\n> that git-prune will not mention pruned objects without \"-v\", but will\n> mention temporary files. They should be in the same category.\n\nOk, so I'll queue it as a separate topic with a different\njustification.\n\n-- >8 --\nFrom: Brandon Casey \nDate: Mon, 6 Aug 2012 22:01:49 -0700\nSubject: [PATCH] prune.c: only print informational message in show_only or verbose mode\n\n\"git prune\" reports removal of loose object files that are no longer\nnecessary only under the \"-v\" option, but unconditionally reports\nremoval of temporary files that are no longer needed.\n\nThe original thinking was that presence of a leftover temporary file\nshould be an unusual occurrence that may indicate an earlier failure\nof some sort, and the user may want to be reminded of it. Removing\nan unnecessary loose object file, on the other hand, is just part of\nthe normal operation. That is why the former is always printed out\nand the latter only when -v is used.\n\nBut neither report is particularly useful. Hide both of these\nbehind the \"-v\" option for consistency.\n\nSigned-off-by: Brandon Casey \nSigned-off-by: Junio C Hamano \n---\n builtin/prune.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex b99b635..6cb9944 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -25,7 +25,8 @@ static int prune_tmp_object(const char *path, const char *filename)\n \t\treturn error(\"Could not stat '%s'\", fullpath);\n \tif (st.st_mtime > expire)\n \t\treturn 0;\n-\tprintf(\"Removing stale temporary file %s\\n\", fullpath);\n+\tif (show_only || verbose)\n+\t\tprintf(\"Removing stale temporary file %s\\n\", fullpath);\n \tif (!show_only)\n \t\tunlink_or_warn(fullpath);\n \treturn 0;\n-- \n1.7.12.rc2.53.g9ec2ef6\n"},{"id":"196628","messageId":"20120807215946.GB22974@sigill.intra.peff.net","threadId":"31186","inReplyTo":"7vlihqv0ks.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-07T21:59:46Z","receivedAt":"2012-08-07T21:59:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 07, 2012 at 02:44:51PM -0700, Junio C Hamano wrote:\n\n> Ok, so I'll queue it as a separate topic with a different\n> justification.\n> \n> -- >8 --\n> From: Brandon Casey \n> Date: Mon, 6 Aug 2012 22:01:49 -0700\n> Subject: [PATCH] prune.c: only print informational message in show_only or verbose mode\n> \n> \"git prune\" reports removal of loose object files that are no longer\n> necessary only under the \"-v\" option, but unconditionally reports\n> removal of temporary files that are no longer needed.\n> \n> The original thinking was that presence of a leftover temporary file\n\ns/presence/the &/\n\n> should be an unusual occurrence that may indicate an earlier failure\n> of some sort, and the user may want to be reminded of it. Removing\n> an unnecessary loose object file, on the other hand, is just part of\n> the normal operation. That is why the former is always printed out\n> and the latter only when -v is used.\n> \n> But neither report is particularly useful. Hide both of these\n> behind the \"-v\" option for consistency.\n> \n> Signed-off-by: Brandon Casey \n> Signed-off-by: Junio C Hamano \n> ---\n\nLooks fine to me. I think tmpfile removal is also not that interesting\nin general. A stale file can happen any time the user aborts an\noperation via ^C. But I think your justification is sufficient as-is\n(and this topic is not worth spending too much more time on).\n\n-Peff\n"},{"id":"196631","messageId":"CA+sFfMe+NsPxz555iZ6X0f8Kca8Vu2+2gFWm628O0XYFaHOzXQ@mail.gmail.com","threadId":"31186","inReplyTo":"7vlihqv0ks.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] prune.c: only print informational message in show_only or verbose mode","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-08-07T22:55:02Z","receivedAt":"2012-08-07T22:55:02Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Tue, Aug 7, 2012 at 2:44 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> I still think your 2/2 is worth doing independently, though. It is silly\n>> that git-prune will not mention pruned objects without \"-v\", but will\n>> mention temporary files. They should be in the same category.\n>\n> Ok, so I'll queue it as a separate topic with a different\n> justification.\n\nLooks fine to me. Thanks.\n\n-Brandon\n"}]}