Re: Did we break receive-pack recently?

On Sun, Aug 5, 2012 at 6:37 PM, Brandon Casey [off-list ref] wrote:

On Sat, Aug 4, 2012 at 6:55 PM, Junio C Hamano [off-list ref] wrote:

quoted

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