Re: Did we break receive-pack recently?

From: Brandon Casey <drafnel@gmail.com>
Date: Aug 6, 2012, 03:32 UTC
Message-ID: <CA+sFfMdXc+usFRnCNVoke91_X2qWZARTvPHO=B7Ukxr-j7JB2g@mail.gmail.com>
In-Reply-To: <CA+sFfMcA2qUcigPg_ijWJmiTKYY9V4f4f6XQp8xT76wLaUXSxA@mail.gmail.com>

On Sun, Aug 5, 2012 at 6:37 PM, Brandon Casey <drafnel@gmail.com> wrote:

Show 27 quoted lines

> On Sat, Aug 4, 2012 at 6:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> I just saw this:
>>
>>     $ git push ko
>>     ko: Counting objects: 332, done.
>>     Delta compression using up to 4 threads.
>>     Compressing objects: 100% (110/110), done.
>>     Writing objects: 100% (130/130), 32.27 KiB, done.
>>     Total 130 (delta 106), reused 21 (delta 20)
>>     Auto packing the repository for optimum performance.
>>     fatal: protocol error: bad line length character: Remo
>>     error: error in sideband demultiplexer
>>     To ra.kernel.org:/pub/scm/git/git.git
>>     ...
>>
>> What is unusual with this push is that it happened to trigger the
>> auto-gc on the receiving end and the message "Auto packing the
>> repository..." came back to the pusher just fine, but somebody
>> nearby seem to have tried to say "Remo"(te---probably) without
>> properly using the sideband.
>
> Or perhaps "Remo" is short for "Removing...".
>
> Perhaps this is the source:
>
>    $ grep Remo builtin/prune.c
>    printf("Removing stale temporary file %s\n", fullpath);

Verified...

test_path=`pwd` &&
git init test_repo1 &&
( cd test_repo1 &&
  echo D >file.txt &&
  git add . &&
  git commit -m 'Commit something that hashes to 17...' &&
  echo MN >file.txt &&
  git commit -a -m 'Commit something else that hashes to 17...'
) &&
git init --bare test_repo2.git &&
( cd test_repo2.git &&
  git config gc.auto 1 &&
  touch -d '2012-07-01' objects/tmp_test
) &&
( cd test_repo1 &&
  git push "file://$test_path/test_repo2.git" HEAD:refs/heads/master
)

It seems to have been broken since we added 'gc --auto' to receive pack in 2009 (or maybe since I added that printf to prune.c in 2008 depending on how you look at it :b ). Apparently it's something not very likely to be triggered. Probably because most servers running git daemon frequently run 'git gc'. Wonder what k.org's policy is?

I think the original thinking behind writing to stdout indiscriminately was that removing a temporary object was something that was considered unusual, so the user should always be informed. This is unlike removing a stale object, which is something that is expected during normal usage. If a stale temporary object is removed, then it means that some piece of git which should have removed it, failed to do so.

We could write the message to stderr instead, but I think the full path to a temporary stale object is not appropriate to communicate to remote users over the wire.

So, I think it's best just to protect it with 'if (show_only || verbose)' like the other informational messages.

Patch forthcoming, hopefully with a test. Doesn't look like we have anything testing the auto-gc spawned from receive-pack. I'll see if I can come up with something.

-Brandon