From 0af5944cac5e09ee99f242d029c89f924e4dd294 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Mon, 8 Feb 2010 19:10:50 -0800 Subject: [PATCH 01/14] Refactor SideBandOutputStream to be buffered Instead of relying on our callers to wrap us up inside of a BufferedOutputStream and using the proper block sizing, do the buffering directly inside of SideBandOutputStream. This ensures we don't get large write-throughs from BufferedOutputStream that might overflow the configured packet size. The constructor of SideBandOutputStream is also beefed up to check its arguments and ensure they are within acceptable ranges for the current side-band protocol. Change-Id: Ic14567327d03c9e972f9734b8228178bc448867d Signed-off-by: Shawn O. Pearce --- .../jgit/transport/PacketLineOutTest.java | 22 +-- .../transport/SideBandOutputStreamTest.java | 127 ++++++++++++++---- .../eclipse/jgit/transport/PacketLineOut.java | 14 +- .../jgit/transport/SideBandOutputStream.java | 98 +++++++++++--- .../transport/SideBandProgressMonitor.java | 12 +- .../eclipse/jgit/transport/UploadPack.java | 13 +- 6 files changed, 198 insertions(+), 88 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PacketLineOutTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PacketLineOutTest.java index 6eb98ac12..c66d4d52a 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PacketLineOutTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PacketLineOutTest.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009, Google Inc. + * Copyright (C) 2009-2010, Google Inc. * and other copyright owners as documented in the project's IP log. * * This program and the accompanying materials are made available @@ -119,8 +119,7 @@ public void testWritePacket2() throws IOException { } public void testWritePacket3() throws IOException { - final int buflen = SideBandOutputStream.MAX_BUF - - SideBandOutputStream.HDR_SIZE; + final int buflen = SideBandOutputStream.MAX_BUF - 5; final byte[] buf = new byte[buflen]; for (int i = 0; i < buf.length; i++) { buf[i] = (byte) i; @@ -137,23 +136,6 @@ public void testWritePacket3() throws IOException { } } - // writeChannelPacket - - public void testWriteChannelPacket1() throws IOException { - out.writeChannelPacket(1, new byte[] { 'a' }, 0, 1); - assertBuffer("0006\001a"); - } - - public void testWriteChannelPacket2() throws IOException { - out.writeChannelPacket(2, new byte[] { 'b' }, 0, 1); - assertBuffer("0006\002b"); - } - - public void testWriteChannelPacket3() throws IOException { - out.writeChannelPacket(3, new byte[] { 'c' }, 0, 1); - assertBuffer("0006\003c"); - } - // flush public void testFlush() throws IOException { diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/SideBandOutputStreamTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/SideBandOutputStreamTest.java index 3c79f138c..61c894e41 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/SideBandOutputStreamTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/SideBandOutputStreamTest.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009, Google Inc. + * Copyright (C) 2009-2010, Google Inc. * and other copyright owners as documented in the project's IP log. * * This program and the accompanying materials are made available @@ -43,6 +43,13 @@ package org.eclipse.jgit.transport; +import static org.eclipse.jgit.transport.SideBandOutputStream.CH_DATA; +import static org.eclipse.jgit.transport.SideBandOutputStream.CH_ERROR; +import static org.eclipse.jgit.transport.SideBandOutputStream.CH_PROGRESS; +import static org.eclipse.jgit.transport.SideBandOutputStream.HDR_SIZE; +import static org.eclipse.jgit.transport.SideBandOutputStream.MAX_BUF; +import static org.eclipse.jgit.transport.SideBandOutputStream.SMALL_BUF; + import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.OutputStream; @@ -58,62 +65,90 @@ public class SideBandOutputStreamTest extends TestCase { private ByteArrayOutputStream rawOut; - private PacketLineOut pckOut; - protected void setUp() throws Exception { super.setUp(); rawOut = new ByteArrayOutputStream(); - pckOut = new PacketLineOut(rawOut); } public void testWrite_CH_DATA() throws IOException { final SideBandOutputStream out; - out = new SideBandOutputStream(SideBandOutputStream.CH_DATA, pckOut); + out = new SideBandOutputStream(CH_DATA, SMALL_BUF, rawOut); out.write(new byte[] { 'a', 'b', 'c' }); + out.flush(); assertBuffer("0008\001abc"); } public void testWrite_CH_PROGRESS() throws IOException { final SideBandOutputStream out; - out = new SideBandOutputStream(SideBandOutputStream.CH_PROGRESS, pckOut); + out = new SideBandOutputStream(CH_PROGRESS, SMALL_BUF, rawOut); out.write(new byte[] { 'a', 'b', 'c' }); + out.flush(); assertBuffer("0008\002abc"); } public void testWrite_CH_ERROR() throws IOException { final SideBandOutputStream out; - out = new SideBandOutputStream(SideBandOutputStream.CH_ERROR, pckOut); + out = new SideBandOutputStream(CH_ERROR, SMALL_BUF, rawOut); out.write(new byte[] { 'a', 'b', 'c' }); + out.flush(); assertBuffer("0008\003abc"); } public void testWrite_Small() throws IOException { final SideBandOutputStream out; - out = new SideBandOutputStream(SideBandOutputStream.CH_DATA, pckOut); + out = new SideBandOutputStream(CH_DATA, SMALL_BUF, rawOut); out.write('a'); out.write('b'); out.write('c'); + out.flush(); + assertBuffer("0008\001abc"); + } + + public void testWrite_SmallBlocks1() throws IOException { + final SideBandOutputStream out; + out = new SideBandOutputStream(CH_DATA, 6, rawOut); + out.write('a'); + out.write('b'); + out.write('c'); + out.flush(); assertBuffer("0006\001a0006\001b0006\001c"); } + public void testWrite_SmallBlocks2() throws IOException { + final SideBandOutputStream out; + out = new SideBandOutputStream(CH_DATA, 6, rawOut); + out.write(new byte[] { 'a', 'b', 'c' }); + out.flush(); + assertBuffer("0006\001a0006\001b0006\001c"); + } + + public void testWrite_SmallBlocks3() throws IOException { + final SideBandOutputStream out; + out = new SideBandOutputStream(CH_DATA, 7, rawOut); + out.write('a'); + out.write(new byte[] { 'b', 'c' }); + out.flush(); + assertBuffer("0007\001ab0006\001c"); + } + public void testWrite_Large() throws IOException { - final int buflen = SideBandOutputStream.MAX_BUF - - SideBandOutputStream.HDR_SIZE; + final int buflen = MAX_BUF - HDR_SIZE; final byte[] buf = new byte[buflen]; for (int i = 0; i < buf.length; i++) { buf[i] = (byte) i; } final SideBandOutputStream out; - out = new SideBandOutputStream(SideBandOutputStream.CH_DATA, pckOut); + out = new SideBandOutputStream(CH_DATA, MAX_BUF, rawOut); out.write(buf); + out.flush(); final byte[] act = rawOut.toByteArray(); - final String explen = Integer.toString(buf.length + 5, 16); - assertEquals(5 + buf.length, act.length); + final String explen = Integer.toString(buf.length + HDR_SIZE, 16); + assertEquals(HDR_SIZE + buf.length, act.length); assertEquals(new String(act, 0, 4, "UTF-8"), explen); assertEquals(1, act[4]); - for (int i = 0, j = 5; i < buf.length; i++, j++) { + for (int i = 0, j = HDR_SIZE; i < buf.length; i++, j++) { assertEquals(buf[i], act[j]); } } @@ -132,17 +167,63 @@ public void flush() throws IOException { } }; - new SideBandOutputStream(SideBandOutputStream.CH_DATA, - new PacketLineOut(mockout)).flush(); - assertEquals(0, flushCnt[0]); - - new SideBandOutputStream(SideBandOutputStream.CH_ERROR, - new PacketLineOut(mockout)).flush(); + new SideBandOutputStream(CH_DATA, SMALL_BUF, mockout).flush(); assertEquals(1, flushCnt[0]); + } - new SideBandOutputStream(SideBandOutputStream.CH_PROGRESS, - new PacketLineOut(mockout)).flush(); - assertEquals(2, flushCnt[0]); + public void testConstructor_RejectsBadChannel() { + try { + new SideBandOutputStream(-1, MAX_BUF, rawOut); + fail("Accepted -1 channel number"); + } catch (IllegalArgumentException e) { + assertEquals("channel -1 must be in range [0, 255]", e.getMessage()); + } + + try { + new SideBandOutputStream(0, MAX_BUF, rawOut); + fail("Accepted 0 channel number"); + } catch (IllegalArgumentException e) { + assertEquals("channel 0 must be in range [0, 255]", e.getMessage()); + } + + try { + new SideBandOutputStream(256, MAX_BUF, rawOut); + fail("Accepted 256 channel number"); + } catch (IllegalArgumentException e) { + assertEquals("channel 256 must be in range [0, 255]", e + .getMessage()); + } + } + + public void testConstructor_RejectsBadBufferSize() { + try { + new SideBandOutputStream(CH_DATA, -1, rawOut); + fail("Accepted -1 for buffer size"); + } catch (IllegalArgumentException e) { + assertEquals("packet size -1 must be >= 5", e.getMessage()); + } + + try { + new SideBandOutputStream(CH_DATA, 0, rawOut); + fail("Accepted 0 for buffer size"); + } catch (IllegalArgumentException e) { + assertEquals("packet size 0 must be >= 5", e.getMessage()); + } + + try { + new SideBandOutputStream(CH_DATA, 1, rawOut); + fail("Accepted 1 for buffer size"); + } catch (IllegalArgumentException e) { + assertEquals("packet size 1 must be >= 5", e.getMessage()); + } + + try { + new SideBandOutputStream(CH_DATA, Integer.MAX_VALUE, rawOut); + fail("Accepted " + Integer.MAX_VALUE + " for buffer size"); + } catch (IllegalArgumentException e) { + assertEquals("packet size " + Integer.MAX_VALUE + + " must be <= 65520", e.getMessage()); + } } private void assertBuffer(final String exp) throws IOException { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineOut.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineOut.java index 81dd4f6a1..51506b20a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineOut.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineOut.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008-2009, Google Inc. + * Copyright (C) 2008-2010, Google Inc. * Copyright (C) 2008-2009, Robin Rosenberg * Copyright (C) 2008, Shawn O. Pearce * and other copyright owners as documented in the project's IP log. @@ -105,14 +105,6 @@ public void writePacket(final byte[] packet) throws IOException { out.write(packet); } - void writeChannelPacket(final int channel, final byte[] buf, int off, - int len) throws IOException { - formatLength(len + 5); - lenbuffer[4] = (byte) channel; - out.write(lenbuffer, 0, 5); - out.write(buf, off, len); - } - /** * Write a packet end marker, sometimes referred to as a flush command. *

@@ -149,6 +141,10 @@ public void flush() throws IOException { '7', '8', '9', 'a', 'b', 'c', 'd', 'e', 'f' }; private void formatLength(int w) { + formatLength(lenbuffer, w); + } + + static void formatLength(byte[] lenbuffer, int w) { int o = 3; while (o >= 0 && w != 0) { lenbuffer[o--] = hexchar[w & 0xf]; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandOutputStream.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandOutputStream.java index 5e50fd89b..6e0a52627 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandOutputStream.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandOutputStream.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008, Google Inc. + * Copyright (C) 2008-2010, Google Inc. * and other copyright owners as documented in the project's IP log. * * This program and the accompanying materials are made available @@ -47,11 +47,10 @@ import java.io.OutputStream; /** - * Multiplexes data and progress messages + * Multiplexes data and progress messages. *

- * To correctly use this class you must wrap it in a BufferedOutputStream with a - * buffer size no larger than either {@link #SMALL_BUF} or {@link #MAX_BUF}, - * minus {@link #HDR_SIZE}. + * This stream is buffered at packet sizes, so the caller doesn't need to wrap + * it in yet another buffered stream. */ class SideBandOutputStream extends OutputStream { static final int CH_DATA = SideBandInputStream.CH_DATA; @@ -66,34 +65,93 @@ class SideBandOutputStream extends OutputStream { static final int HDR_SIZE = 5; - private final int channel; + private final OutputStream out; - private final PacketLineOut pckOut; + private final byte[] buffer; - private byte[] singleByteBuffer; + /** + * Number of bytes in {@link #buffer} that are valid data. + *

+ * Initialized to {@link #HDR_SIZE} if there is no application data in the + * buffer, as the packet header always appears at the start of the buffer. + */ + private int cnt; - SideBandOutputStream(final int chan, final PacketLineOut out) { - channel = chan; - pckOut = out; + /** + * Create a new stream to write side band packets. + * + * @param chan + * channel number to prefix all packets with, so the remote side + * can demultiplex the stream and get back the original data. + * Must be in the range [0, 255]. + * @param sz + * maximum size of a data packet within the stream. The remote + * side needs to agree to the packet size to prevent buffer + * overflows. Must be in the range [HDR_SIZE + 1, MAX_BUF). + * @param os + * stream that the packets are written onto. This stream should + * be attached to a SideBandInputStream on the remote side. + */ + SideBandOutputStream(final int chan, final int sz, final OutputStream os) { + if (chan <= 0 || chan > 255) + throw new IllegalArgumentException("channel " + chan + + " must be in range [0, 255]"); + if (sz <= HDR_SIZE) + throw new IllegalArgumentException("packet size " + sz + + " must be >= " + HDR_SIZE); + else if (MAX_BUF < sz) + throw new IllegalArgumentException("packet size " + sz + + " must be <= " + MAX_BUF); + + out = os; + buffer = new byte[sz]; + buffer[4] = (byte) chan; + cnt = HDR_SIZE; } @Override public void flush() throws IOException { - if (channel != CH_DATA) - pckOut.flush(); + if (HDR_SIZE < cnt) + writeBuffer(); + out.flush(); } @Override - public void write(final byte[] b, final int off, final int len) - throws IOException { - pckOut.writeChannelPacket(channel, b, off, len); + public void write(final byte[] b, int off, int len) throws IOException { + while (0 < len) { + int capacity = buffer.length - cnt; + if (cnt == HDR_SIZE && capacity < len) { + // Our block to write is bigger than the packet size, + // stream it out as-is to avoid unnecessary copies. + PacketLineOut.formatLength(buffer, buffer.length); + out.write(buffer, 0, HDR_SIZE); + out.write(b, off, capacity); + off += capacity; + len -= capacity; + + } else { + if (capacity == 0) + writeBuffer(); + + int n = Math.min(len, capacity); + System.arraycopy(b, off, buffer, cnt, n); + cnt += n; + off += n; + len -= n; + } + } } @Override public void write(final int b) throws IOException { - if (singleByteBuffer == null) - singleByteBuffer = new byte[1]; - singleByteBuffer[0] = (byte) b; - write(singleByteBuffer); + if (cnt == buffer.length) + writeBuffer(); + buffer[cnt++] = (byte) b; + } + + private void writeBuffer() throws IOException { + PacketLineOut.formatLength(buffer, cnt); + out.write(buffer, 0, cnt); + cnt = HDR_SIZE; } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandProgressMonitor.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandProgressMonitor.java index 89d338c89..efce7b1da 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandProgressMonitor.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandProgressMonitor.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008, Google Inc. + * Copyright (C) 2008-2010, Google Inc. * and other copyright owners as documented in the project's IP log. * * This program and the accompanying materials are made available @@ -43,7 +43,7 @@ package org.eclipse.jgit.transport; -import java.io.BufferedOutputStream; +import java.io.OutputStream; import java.io.OutputStreamWriter; import java.io.PrintWriter; @@ -66,12 +66,8 @@ class SideBandProgressMonitor implements ProgressMonitor { private int totalWork; - SideBandProgressMonitor(final PacketLineOut pckOut) { - final int bufsz = SideBandOutputStream.SMALL_BUF - - SideBandOutputStream.HDR_SIZE; - out = new PrintWriter(new OutputStreamWriter(new BufferedOutputStream( - new SideBandOutputStream(SideBandOutputStream.CH_PROGRESS, - pckOut), bufsz), Constants.CHARSET)); + SideBandProgressMonitor(final OutputStream os) { + out = new PrintWriter(new OutputStreamWriter(os, Constants.CHARSET)); } public void start(final int totalTasks) { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index b76b22b77..39c4243ba 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -43,9 +43,6 @@ package org.eclipse.jgit.transport; -import static org.eclipse.jgit.transport.BasePackFetchConnection.MultiAck; - -import java.io.BufferedOutputStream; import java.io.EOFException; import java.io.IOException; import java.io.InputStream; @@ -70,6 +67,7 @@ import org.eclipse.jgit.revwalk.RevObject; import org.eclipse.jgit.revwalk.RevTag; import org.eclipse.jgit.revwalk.RevWalk; +import org.eclipse.jgit.transport.BasePackFetchConnection.MultiAck; import org.eclipse.jgit.transport.RefAdvertiser.PacketLineOutRefAdvertiser; import org.eclipse.jgit.util.io.InterruptTimer; import org.eclipse.jgit.util.io.TimeoutInputStream; @@ -556,13 +554,12 @@ private void sendPack() throws IOException { int bufsz = SideBandOutputStream.SMALL_BUF; if (options.contains(OPTION_SIDE_BAND_64K)) bufsz = SideBandOutputStream.MAX_BUF; - bufsz -= SideBandOutputStream.HDR_SIZE; - - packOut = new BufferedOutputStream(new SideBandOutputStream( - SideBandOutputStream.CH_DATA, pckOut), bufsz); + packOut = new SideBandOutputStream(SideBandOutputStream.CH_DATA, + bufsz, rawOut); if (progress) - pm = new SideBandProgressMonitor(pckOut); + pm = new SideBandProgressMonitor(new SideBandOutputStream( + SideBandOutputStream.CH_PROGRESS, bufsz, rawOut)); } final PackWriter pw; From f2dc9f0bfe0bd521a9a2cf446b5210d8c85b583a Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 9 Feb 2010 09:14:00 -0800 Subject: [PATCH 02/14] Refactor SideBandInputStream construction Typically we refer to the raw InputStream (the stream without the pkt-line headers on it) as rawIn, and the pkt-line header variant as pckIn. Refactor our fields to reflect that. To ensure these are actually the same underlying InputStream, we now create our own PacketLineIn wrapper around the supplied raw InputStream. Its a very low-cost object since it has only the 4 byte length buffer. Instead of hardcoding the header length as 5, use the constant from SideBandOutputStream. This makes it a bit more clear what we are consuming, exactly here. Change-Id: Iebd05538042913536b88c3ddc3adc3a86a841cc5 Signed-off-by: Shawn O. Pearce --- .../transport/BasePackFetchConnection.java | 7 ++++- .../eclipse/jgit/transport/PacketLineIn.java | 5 ---- .../jgit/transport/SideBandInputStream.java | 28 +++++++++---------- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java index 84e55b61e..dc9c7948b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java @@ -46,6 +46,7 @@ package org.eclipse.jgit.transport; import java.io.IOException; +import java.io.InputStream; import java.util.Collection; import java.util.Collections; import java.util.Date; @@ -609,7 +610,11 @@ private void markCommon(final RevObject obj, final AckNackResult anr) private void receivePack(final ProgressMonitor monitor) throws IOException { final IndexPack ip; - ip = IndexPack.create(local, sideband ? pckIn.sideband(monitor) : in); + InputStream input = in; + if (sideband) + input = new SideBandInputStream(input, monitor); + + ip = IndexPack.create(local, input); ip.setFixThin(thinPack); ip.setObjectChecking(transport.isCheckFetchedObjects()); ip.index(monitor); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java index db6abef1a..1022eb2ee 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java @@ -51,7 +51,6 @@ import org.eclipse.jgit.errors.PackProtocolException; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.MutableObjectId; -import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.util.IO; import org.eclipse.jgit.util.RawParseUtils; @@ -80,10 +79,6 @@ static enum AckNackResult { lenbuffer = new byte[4]; } - InputStream sideband(final ProgressMonitor pm) { - return new SideBandInputStream(this, in, pm); - } - AckNackResult readACK(final MutableObjectId returnedId) throws IOException { final String line = readString(); if (line.length() == 0) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java index 40a6808d2..7b5422644 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java @@ -44,6 +44,8 @@ package org.eclipse.jgit.transport; +import static org.eclipse.jgit.transport.SideBandOutputStream.HDR_SIZE; + import java.io.IOException; import java.io.InputStream; import java.util.regex.Matcher; @@ -69,7 +71,7 @@ * Channel 3 results in an exception being thrown, as the remote side has issued * an unrecoverable error. * - * @see PacketLineIn#sideband(ProgressMonitor) + * @see SideBandOutputStream */ class SideBandInputStream extends InputStream { static final int CH_DATA = 1; @@ -84,9 +86,9 @@ class SideBandInputStream extends InputStream { private static Pattern P_BOUNDED = Pattern.compile( "^([\\w ]+):.*\\((\\d+)/(\\d+)\\).*", Pattern.DOTALL); - private final PacketLineIn pckIn; + private final InputStream rawIn; - private final InputStream in; + private final PacketLineIn pckIn; private final ProgressMonitor monitor; @@ -102,11 +104,10 @@ class SideBandInputStream extends InputStream { private int available; - SideBandInputStream(final PacketLineIn aPckIn, final InputStream aIn, - final ProgressMonitor aProgress) { - pckIn = aPckIn; - in = aIn; - monitor = aProgress; + SideBandInputStream(final InputStream in, final ProgressMonitor progress) { + rawIn = in; + pckIn = new PacketLineIn(rawIn); + monitor = progress; currentTask = ""; } @@ -116,7 +117,7 @@ public int read() throws IOException { if (eof) return -1; available--; - return in.read(); + return rawIn.read(); } @Override @@ -126,7 +127,7 @@ public int read(final byte[] b, int off, int len) throws IOException { needDataPacket(); if (eof) break; - final int n = in.read(b, off, Math.min(len, available)); + final int n = rawIn.read(b, off, Math.min(len, available)); if (n < 0) break; r += n; @@ -147,8 +148,8 @@ private void needDataPacket() throws IOException { return; } - channel = in.read(); - available -= 5; // length header plus channel indicator + channel = rawIn.read(); + available -= HDR_SIZE; // length header plus channel indicator if (available == 0) continue; @@ -157,7 +158,6 @@ private void needDataPacket() throws IOException { return; case CH_PROGRESS: progress(readString(available)); - continue; case CH_ERROR: eof = true; @@ -229,7 +229,7 @@ private boolean doProgressLine(final String msg) { private String readString(final int len) throws IOException { final byte[] raw = new byte[len]; - IO.readFully(in, raw, 0, len); + IO.readFully(rawIn, raw, 0, len); return RawParseUtils.decode(Constants.CHARSET, raw, 0, len); } } From b7e8cefc9216ee852499cb15935f879df1e1e3b7 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 9 Feb 2010 09:44:24 -0800 Subject: [PATCH 03/14] Decode side-band channel number as unsigned integer This field is unsigned in the protocol, so treat it as such when we report the channel number in errors. Change-Id: I20a52809c7a756e9f66b3557a4300ae1e11f6d25 Signed-off-by: Shawn O. Pearce --- .../src/org/eclipse/jgit/transport/SideBandInputStream.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java index 7b5422644..5f3b34e6f 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java @@ -148,7 +148,7 @@ private void needDataPacket() throws IOException { return; } - channel = rawIn.read(); + channel = rawIn.read() & 0xff; available -= HDR_SIZE; // length header plus channel indicator if (available == 0) continue; From 3a9295b8942639b8ac9f161a84b20fe15896e6b6 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 9 Feb 2010 10:39:15 -0800 Subject: [PATCH 04/14] Prefix remote progress tasks with "remote: " When we pull task messages off the remote peer via sideband #2 prefix them with the string "remote: " to make it clear to the user these are coming from the other system, and not from their local client. Change-Id: I02c5e67c6be67e30e40d3bc4be314d6640feb519 Signed-off-by: Shawn O. Pearce --- .../eclipse/jgit/transport/SideBandInputStream.java | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java index 5f3b34e6f..dadc0638b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java @@ -74,6 +74,8 @@ * @see SideBandOutputStream */ class SideBandInputStream extends InputStream { + private static final String PFX_REMOTE = "remote: "; + static final int CH_DATA = 1; static final int CH_PROGRESS = 2; @@ -161,7 +163,7 @@ private void needDataPacket() throws IOException { continue; case CH_ERROR: eof = true; - throw new TransportException("remote: " + readString(available)); + throw new TransportException(PFX_REMOTE + readString(available)); default: throw new PackProtocolException("Invalid channel " + channel); } @@ -201,8 +203,7 @@ private boolean doProgressLine(final String msg) { if (!currentTask.equals(taskname)) { currentTask = taskname; lastCnt = 0; - final int tot = Integer.parseInt(matcher.group(3)); - monitor.beginTask(currentTask, tot); + beginTask(Integer.parseInt(matcher.group(3))); } final int cnt = Integer.parseInt(matcher.group(2)); monitor.update(cnt - lastCnt); @@ -216,7 +217,7 @@ private boolean doProgressLine(final String msg) { if (!currentTask.equals(taskname)) { currentTask = taskname; lastCnt = 0; - monitor.beginTask(currentTask, ProgressMonitor.UNKNOWN); + beginTask(ProgressMonitor.UNKNOWN); } final int cnt = Integer.parseInt(matcher.group(2)); monitor.update(cnt - lastCnt); @@ -227,6 +228,10 @@ private boolean doProgressLine(final String msg) { return false; } + private void beginTask(final int totalWorkUnits) { + monitor.beginTask(PFX_REMOTE + currentTask, totalWorkUnits); + } + private String readString(final int len) throws IOException { final byte[] raw = new byte[len]; IO.readFully(rawIn, raw, 0, len); From 4c44810df406115c811890336eaa131a8413149b Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 9 Feb 2010 10:46:01 -0800 Subject: [PATCH 05/14] Use more restrictive patterns for sideband progress scraping To avoid scraping a non-progress message as though it were a progress item for the progress monitor, use a more restrictive pattern to watch the remote side's messages. These two regexps should match any message produced by C Git since 42e18fbf5f94 ("more compact progress display", Oct 2007), and which first appeared in Git 1.5.4. Change-Id: I57e34cf59d42c1dbcbd1a83dd6f499ce5e39d15d Signed-off-by: Shawn O. Pearce --- .../org/eclipse/jgit/transport/SideBandInputStream.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java index dadc0638b..0abbe7e09 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java @@ -82,11 +82,11 @@ class SideBandInputStream extends InputStream { static final int CH_ERROR = 3; - private static Pattern P_UNBOUNDED = Pattern.compile( - "^([\\w ]+): (\\d+)( |, done)?.*", Pattern.DOTALL); + private static Pattern P_UNBOUNDED = Pattern + .compile("^([\\w ]+): +(\\d+)(?:, done\\.)? *$"); - private static Pattern P_BOUNDED = Pattern.compile( - "^([\\w ]+):.*\\((\\d+)/(\\d+)\\).*", Pattern.DOTALL); + private static Pattern P_BOUNDED = Pattern + .compile("^([\\w ]+): +\\d+% +\\( *(\\d+)/ *(\\d+)\\)(?:, done\\.)? *$"); private final InputStream rawIn; From d33f939e8eb47795c1ec8e89a605303770e95f34 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Mon, 8 Feb 2010 19:18:51 -0800 Subject: [PATCH 06/14] ReceivePack: Enable side-band-64k capability for status reports We now advertise the side-band-64k capability inside of ReceivePack, allowing hooks to echo status messages down the side band channel instead of over the optional stderr stream. This change permits hooks running inside of an http:// based push invocation to still message the end-user with more detailed errors than the small per-command string in the status report. Change-Id: I64f251ef2d13ab3fd0e1a319a4683725455e5244 Signed-off-by: Shawn O. Pearce --- .../transport/BasePackPushConnection.java | 2 + .../eclipse/jgit/transport/ReceivePack.java | 82 ++++++++++++------- 2 files changed, 54 insertions(+), 30 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackPushConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackPushConnection.java index 2603ca287..6e8b05da2 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackPushConnection.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackPushConnection.java @@ -86,6 +86,8 @@ class BasePackPushConnection extends BasePackConnection implements static final String CAPABILITY_OFS_DELTA = "ofs-delta"; + static final String CAPABILITY_SIDE_BAND_64K = "side-band-64k"; + private final boolean thinPack; private boolean capableDeleteRefs; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java index 4d63ee68e..c5bbdac5a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java @@ -43,13 +43,20 @@ package org.eclipse.jgit.transport; -import java.io.BufferedWriter; +import static org.eclipse.jgit.transport.BasePackPushConnection.CAPABILITY_DELETE_REFS; +import static org.eclipse.jgit.transport.BasePackPushConnection.CAPABILITY_OFS_DELTA; +import static org.eclipse.jgit.transport.BasePackPushConnection.CAPABILITY_REPORT_STATUS; +import static org.eclipse.jgit.transport.BasePackPushConnection.CAPABILITY_SIDE_BAND_64K; +import static org.eclipse.jgit.transport.SideBandOutputStream.CH_DATA; +import static org.eclipse.jgit.transport.SideBandOutputStream.CH_PROGRESS; +import static org.eclipse.jgit.transport.SideBandOutputStream.MAX_BUF; + import java.io.EOFException; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.io.OutputStreamWriter; -import java.io.PrintWriter; +import java.io.Writer; import java.util.ArrayList; import java.util.Collections; import java.util.HashSet; @@ -84,12 +91,6 @@ * Implements the server side of a push connection, receiving objects. */ public class ReceivePack { - static final String CAPABILITY_REPORT_STATUS = BasePackPushConnection.CAPABILITY_REPORT_STATUS; - - static final String CAPABILITY_DELETE_REFS = BasePackPushConnection.CAPABILITY_DELETE_REFS; - - static final String CAPABILITY_OFS_DELTA = BasePackPushConnection.CAPABILITY_OFS_DELTA; - /** Database we write the stored objects into. */ private final Repository db; @@ -151,7 +152,7 @@ public class ReceivePack { private PacketLineOut pckOut; - private PrintWriter msgs; + private Writer msgs; /** The refs we advertised as existing at the start of the connection. */ private Map refs; @@ -165,9 +166,12 @@ public class ReceivePack { /** An exception caught while unpacking and fsck'ing the objects. */ private Throwable unpackError; - /** if {@link #enabledCapablities} has {@link #CAPABILITY_REPORT_STATUS} */ + /** If {@link BasePackPushConnection#CAPABILITY_REPORT_STATUS} is enabled. */ private boolean reportStatus; + /** If {@link BasePackPushConnection#CAPABILITY_SIDE_BAND_64K} is enabled. */ + private boolean sideBand; + /** Lock around the received pack file, while updating refs. */ private PackLock packLock; @@ -440,7 +444,12 @@ public List getAllCommands() { * string must not end with an LF, and must not contain an LF. */ public void sendError(final String what) { - sendMessage("error", what); + try { + if (msgs != null) + msgs.write("error: " + what + "\n"); + } catch (IOException e) { + // Ignore write failures. + } } /** @@ -454,12 +463,12 @@ public void sendError(final String what) { * string must not end with an LF, and must not contain an LF. */ public void sendMessage(final String what) { - sendMessage("remote", what); - } - - private void sendMessage(final String type, final String what) { - if (msgs != null) - msgs.println(type + ": " + what); + try { + if (msgs != null) + msgs.write(what + "\n"); + } catch (IOException e) { + // Ignore write failures. + } } /** @@ -499,16 +508,8 @@ public void receive(final InputStream input, final OutputStream output, pckIn = new PacketLineIn(rawIn); pckOut = new PacketLineOut(rawOut); - if (messages != null) { - msgs = new PrintWriter(new BufferedWriter( - new OutputStreamWriter(messages, Constants.CHARSET), - 8192)) { - @Override - public void println() { - print('\n'); - } - }; - } + if (messages != null) + msgs = new OutputStreamWriter(messages, Constants.CHARSET); enabledCapablities = new HashSet(); commands = new ArrayList(); @@ -516,8 +517,19 @@ public void println() { service(); } finally { try { - if (msgs != null) { + if (pckOut != null) + pckOut.flush(); + if (msgs != null) msgs.flush(); + + if (sideBand) { + // If we are using side band, we need to send a final + // flush-pkt to tell the remote peer the side band is + // complete and it should stop decoding. We need to + // use the original output stream as rawOut is now the + // side band data channel. + // + new PacketLineOut(output).end(); } } finally { unlockPack(); @@ -581,10 +593,9 @@ void sendString(final String s) throws IOException { } else if (msgs != null) { sendStatusReport(false, new Reporter() { void sendString(final String s) throws IOException { - msgs.println(s); + msgs.write(s + "\n"); } }); - msgs.flush(); } postReceive.onPostReceive(this, filterCommands(Result.OK)); @@ -609,6 +620,7 @@ private void unlockPack() { public void sendAdvertisedRefs(final RefAdvertiser adv) throws IOException { final RevFlag advertised = walk.newFlag("ADVERTISED"); adv.init(walk, advertised); + adv.advertiseCapability(CAPABILITY_SIDE_BAND_64K); adv.advertiseCapability(CAPABILITY_DELETE_REFS); adv.advertiseCapability(CAPABILITY_REPORT_STATUS); if (allowOfsDelta) @@ -667,6 +679,16 @@ private void recvCommands() throws IOException { private void enableCapabilities() { reportStatus = enabledCapablities.contains(CAPABILITY_REPORT_STATUS); + + sideBand = enabledCapablities.contains(CAPABILITY_SIDE_BAND_64K); + if (sideBand) { + OutputStream out = rawOut; + + rawOut = new SideBandOutputStream(CH_DATA, MAX_BUF, out); + pckOut = new PacketLineOut(rawOut); + msgs = new OutputStreamWriter(new SideBandOutputStream(CH_PROGRESS, + MAX_BUF, out), Constants.CHARSET); + } } private boolean needPack() { From 673b3984bdc540da99e1c390cf31f455896a1cda Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 10 Feb 2010 11:49:27 -0800 Subject: [PATCH 07/14] Capture non-progress side band #2 messages and put in result Any messages received on side band #2 that aren't scraped as a progress message into our ProgressMonitor are now forwarded to a buffer which is later included into the OperationResult object. Application callers can use this buffer to present the additional messages from the remote peer after the push or fetch operation has concluded. The smart push connections using the native send-pack/receive-pack protocol now request side-band-64k capability if it is available and forward any messages received through that channel onto this message buffer. This makes hook messages available over smart HTTP, or even over SSH. The SSH transport was modified to redirect the remote command's stderr stream into the message buffer, interleaved with any data received over side band #2. Due to buffering between these two different channels in the SSH channel mux itself the order of any writes between the two cannot be ensured, but it tries to stay close. The local fork transport was also modified to redirect the local receive-pack's stderr into the message buffer, rather than going to the invoking JVM's System.err. This gives applications a chance to log the local error messages, rather than needing to redirect their JVM's stderr before startup. To keep things simple, the application has to wait for the entire operation to complete before it can see the messages. This may be a downside if the user is trying to debug a remote hook that is blocking indefinitely, the user would need to abort the connection before they can inspect the message buffer in any sort of UI built on top of JGit. Change-Id: Ibc215f4569e63071da5b7e5c6674ce924ae39e11 Signed-off-by: Shawn O. Pearce --- .../jgit/http/test/HookMessageTest.java | 174 ++++++++++++++++++ .../jgit/pgm/AbstractFetchCommand.java | 30 ++- .../src/org/eclipse/jgit/pgm/Push.java | 1 + .../jgit/transport/BaseConnection.java | 37 +++- .../transport/BasePackFetchConnection.java | 2 +- .../transport/BasePackPushConnection.java | 30 ++- .../eclipse/jgit/transport/Connection.java | 20 ++ .../eclipse/jgit/transport/FetchProcess.java | 7 +- .../jgit/transport/OperationResult.java | 29 +++ .../eclipse/jgit/transport/PushProcess.java | 25 ++- .../jgit/transport/SideBandInputStream.java | 28 +-- .../jgit/transport/TransportGitSsh.java | 139 +++----------- .../jgit/transport/TransportLocal.java | 79 ++++---- .../eclipse/jgit/util/io/MessageWriter.java | 115 ++++++++++++ .../jgit/util/io/StreamCopyThread.java | 128 +++++++++++++ 15 files changed, 657 insertions(+), 187 deletions(-) create mode 100644 org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/HookMessageTest.java create mode 100644 org.eclipse.jgit/src/org/eclipse/jgit/util/io/MessageWriter.java create mode 100644 org.eclipse.jgit/src/org/eclipse/jgit/util/io/StreamCopyThread.java diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/HookMessageTest.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/HookMessageTest.java new file mode 100644 index 000000000..224ea05c1 --- /dev/null +++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/HookMessageTest.java @@ -0,0 +1,174 @@ +/* + * Copyright (C) 2010, Google Inc. + * and other copyright owners as documented in the project's IP log. + * + * This program and the accompanying materials are made available + * under the terms of the Eclipse Distribution License v1.0 which + * accompanies this distribution, is reproduced below, and is + * available at http://www.eclipse.org/org/documents/edl-v10.php + * + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * + * - Neither the name of the Eclipse Foundation, Inc. nor the + * names of its contributors may be used to endorse or promote + * products derived from this software without specific prior + * written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +package org.eclipse.jgit.http.test; + +import java.util.Collection; +import java.util.Collections; +import java.util.List; + +import javax.servlet.http.HttpServletRequest; + +import org.eclipse.jetty.servlet.ServletContextHandler; +import org.eclipse.jetty.servlet.ServletHolder; +import org.eclipse.jgit.errors.RepositoryNotFoundException; +import org.eclipse.jgit.http.server.GitServlet; +import org.eclipse.jgit.http.server.resolver.DefaultReceivePackFactory; +import org.eclipse.jgit.http.server.resolver.RepositoryResolver; +import org.eclipse.jgit.http.server.resolver.ServiceNotAuthorizedException; +import org.eclipse.jgit.http.server.resolver.ServiceNotEnabledException; +import org.eclipse.jgit.http.test.util.AccessEvent; +import org.eclipse.jgit.http.test.util.HttpTestCase; +import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.lib.Constants; +import org.eclipse.jgit.lib.NullProgressMonitor; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.lib.RepositoryConfig; +import org.eclipse.jgit.revwalk.RevBlob; +import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.transport.PreReceiveHook; +import org.eclipse.jgit.transport.PushResult; +import org.eclipse.jgit.transport.ReceiveCommand; +import org.eclipse.jgit.transport.ReceivePack; +import org.eclipse.jgit.transport.RemoteRefUpdate; +import org.eclipse.jgit.transport.Transport; +import org.eclipse.jgit.transport.URIish; + +public class HookMessageTest extends HttpTestCase { + private Repository remoteRepository; + + private URIish remoteURI; + + protected void setUp() throws Exception { + super.setUp(); + + final TestRepository src = createTestRepository(); + final String srcName = src.getRepository().getDirectory().getName(); + + ServletContextHandler app = server.addContext("/git"); + GitServlet gs = new GitServlet(); + gs.setRepositoryResolver(new RepositoryResolver() { + public Repository open(HttpServletRequest req, String name) + throws RepositoryNotFoundException, + ServiceNotEnabledException { + if (!name.equals(srcName)) + throw new RepositoryNotFoundException(name); + + final Repository db = src.getRepository(); + db.incrementOpen(); + return db; + } + }); + gs.setReceivePackFactory(new DefaultReceivePackFactory() { + public ReceivePack create(HttpServletRequest req, Repository db) + throws ServiceNotEnabledException, + ServiceNotAuthorizedException { + ReceivePack recv = super.create(req, db); + recv.setPreReceiveHook(new PreReceiveHook() { + public void onPreReceive(ReceivePack rp, + Collection commands) { + rp.sendMessage("message line 1"); + rp.sendError("no soup for you!"); + rp.sendMessage("come back next year!"); + } + }); + return recv; + } + + }); + app.addServlet(new ServletHolder(gs), "/*"); + + server.setUp(); + + remoteRepository = src.getRepository(); + remoteURI = toURIish(app, srcName); + + RepositoryConfig cfg = remoteRepository.getConfig(); + cfg.setBoolean("http", null, "receivepack", true); + cfg.save(); + } + + public void testPush_CreateBranch() throws Exception { + final TestRepository src = createTestRepository(); + final RevBlob Q_txt = src.blob("new text"); + final RevCommit Q = src.commit().add("Q", Q_txt).create(); + final Repository db = src.getRepository(); + final String dstName = Constants.R_HEADS + "new.branch"; + Transport t; + PushResult result; + + t = Transport.open(db, remoteURI); + try { + final String srcExpr = Q.name(); + final boolean forceUpdate = false; + final String localName = null; + final ObjectId oldId = null; + + RemoteRefUpdate update = new RemoteRefUpdate(src.getRepository(), + srcExpr, dstName, forceUpdate, localName, oldId); + result = t.push(NullProgressMonitor.INSTANCE, Collections + .singleton(update)); + } finally { + t.close(); + } + + assertTrue(remoteRepository.hasObject(Q_txt)); + assertNotNull("has " + dstName, remoteRepository.getRef(dstName)); + assertEquals(Q, remoteRepository.getRef(dstName).getObjectId()); + fsck(remoteRepository, Q); + + List requests = getRequests(); + assertEquals(2, requests.size()); + + AccessEvent service = requests.get(1); + assertEquals("POST", service.getMethod()); + assertEquals(join(remoteURI, "git-receive-pack"), service.getPath()); + assertEquals(200, service.getStatus()); + + assertEquals("message line 1\n" // + + "error: no soup for you!\n" // + + "come back next year!\n", // + result.getMessages()); + } +} diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/AbstractFetchCommand.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/AbstractFetchCommand.java index f5e3c504c..1e0356750 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/AbstractFetchCommand.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/AbstractFetchCommand.java @@ -1,6 +1,6 @@ /* * Copyright (C) 2008, Charles O'Farrell - * Copyright (C) 2008-2009, Google Inc. + * Copyright (C) 2008-2010, Google Inc. * Copyright (C) 2008, Marek Zawirski * Copyright (C) 2008, Shawn O. Pearce * and other copyright owners as documented in the project's IP log. @@ -79,6 +79,34 @@ protected void showFetchResult(final Transport tn, final FetchResult r) { out.format(" %c %-17s %-10s -> %s", type, longType, src, dst); out.println(); } + + showRemoteMessages(r.getMessages()); + } + + static void showRemoteMessages(String pkt) { + while (0 < pkt.length()) { + final int lf = pkt.indexOf('\n'); + final int cr = pkt.indexOf('\r'); + final int s; + if (0 <= lf && 0 <= cr) + s = Math.min(lf, cr); + else if (0 <= lf) + s = lf; + else if (0 <= cr) + s = cr; + else { + System.err.println("remote: " + pkt); + break; + } + + if (pkt.charAt(s) == '\r') + System.err.print("remote: " + pkt.substring(0, s) + "\r"); + else + System.err.println("remote: " + pkt.substring(0, s)); + + pkt = pkt.substring(s + 1); + } + System.err.flush(); } private String longTypeOf(final TrackingRefUpdate u) { diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Push.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Push.java index 6248ec299..2c0254563 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Push.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Push.java @@ -162,6 +162,7 @@ private void printPushResult(final URIish uri, printRefUpdateResult(uri, result, rru); } + AbstractFetchCommand.showRemoteMessages(result.getMessages()); if (everythingUpToDate) out.println("Everything up-to-date"); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseConnection.java index 14be1700c..1339b8691 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseConnection.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseConnection.java @@ -1,4 +1,5 @@ /* + * Copyright (C) 2010, Google Inc. * Copyright (C) 2008, Marek Zawirski * Copyright (C) 2008, Robin Rosenberg * Copyright (C) 2008, Shawn O. Pearce @@ -45,6 +46,8 @@ package org.eclipse.jgit.transport; +import java.io.StringWriter; +import java.io.Writer; import java.util.Collection; import java.util.Collections; import java.util.Map; @@ -58,12 +61,13 @@ * @see BasePackConnection * @see BaseFetchConnection */ -abstract class BaseConnection implements Connection { - +public abstract class BaseConnection implements Connection { private Map advertisedRefs = Collections.emptyMap(); private boolean startedOperation; + private Writer messageWriter; + public Map getRefsMap() { return advertisedRefs; } @@ -76,6 +80,10 @@ public final Ref getRef(final String name) { return advertisedRefs.get(name); } + public String getMessages() { + return messageWriter != null ? messageWriter.toString() : ""; + } + public abstract void close(); /** @@ -106,4 +114,29 @@ protected void markStartedOperation() throws TransportException { "Only one operation call per connection is supported."); startedOperation = true; } + + /** + * Get the writer that buffers messages from the remote side. + * + * @return writer to store messages from the remote. + */ + protected Writer getMessageWriter() { + if (messageWriter == null) + setMessageWriter(new StringWriter()); + return messageWriter; + } + + /** + * Set the writer that buffers messages from the remote side. + * + * @param writer + * the writer that messages will be delivered to. The writer's + * {@code toString()} method should be overridden to return the + * complete contents. + */ + protected void setMessageWriter(Writer writer) { + if (messageWriter != null) + throw new IllegalStateException("Writer already initialized"); + messageWriter = writer; + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java index dc9c7948b..7b90ec199 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java @@ -612,7 +612,7 @@ private void receivePack(final ProgressMonitor monitor) throws IOException { InputStream input = in; if (sideband) - input = new SideBandInputStream(input, monitor); + input = new SideBandInputStream(input, monitor, getMessageWriter()); ip = IndexPack.create(local, input); ip.setFixThin(thinPack); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackPushConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackPushConnection.java index 6e8b05da2..ba1170747 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackPushConnection.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackPushConnection.java @@ -94,6 +94,8 @@ class BasePackPushConnection extends BasePackConnection implements private boolean capableReport; + private boolean capableSideBand; + private boolean capableOfsDelta; private boolean sentCommand; @@ -145,8 +147,21 @@ protected void doPush(final ProgressMonitor monitor, writeCommands(refUpdates.values(), monitor); if (writePack) writePack(refUpdates, monitor); - if (sentCommand && capableReport) - readStatusReport(refUpdates); + if (sentCommand) { + if (capableReport) + readStatusReport(refUpdates); + if (capableSideBand) { + // Ensure the data channel is at EOF, so we know we have + // read all side-band data from all channels and have a + // complete copy of the messages (if any) buffered from + // the other data channels. + // + int b = in.read(); + if (0 <= b) + throw new TransportException(uri, "expected EOF;" + + " received '" + (char) b + "' instead"); + } + } } catch (TransportException e) { throw e; } catch (Exception e) { @@ -158,7 +173,7 @@ protected void doPush(final ProgressMonitor monitor, private void writeCommands(final Collection refUpdates, final ProgressMonitor monitor) throws IOException { - final String capabilities = enableCapabilities(); + final String capabilities = enableCapabilities(monitor); for (final RemoteRefUpdate rru : refUpdates) { if (!capableDeleteRefs && rru.isDelete()) { rru.setStatus(Status.REJECTED_NODELETE); @@ -191,11 +206,18 @@ private void writeCommands(final Collection refUpdates, outNeedsEnd = false; } - private String enableCapabilities() { + private String enableCapabilities(final ProgressMonitor monitor) { final StringBuilder line = new StringBuilder(); capableReport = wantCapability(line, CAPABILITY_REPORT_STATUS); capableDeleteRefs = wantCapability(line, CAPABILITY_DELETE_REFS); capableOfsDelta = wantCapability(line, CAPABILITY_OFS_DELTA); + + capableSideBand = wantCapability(line, CAPABILITY_SIDE_BAND_64K); + if (capableSideBand) { + in = new SideBandInputStream(in, monitor, getMessageWriter()); + pckIn = new PacketLineIn(in); + } + if (line.length() > 0) line.setCharAt(0, '\0'); return line.toString(); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/Connection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/Connection.java index 3f0c3d8e5..e386c26c1 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/Connection.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/Connection.java @@ -1,4 +1,5 @@ /* + * Copyright (C) 2010, Google Inc. * Copyright (C) 2008, Marek Zawirski * Copyright (C) 2008, Shawn O. Pearce * and other copyright owners as documented in the project's IP log. @@ -104,7 +105,26 @@ public interface Connection { * must close that network socket, disconnecting the two peers. If the * remote repository is actually local (same system) this method must close * any open file handles used to read the "remote" repository. + *

+ * If additional messages were produced by the remote peer, these should + * still be retained in the connection instance for {@link #getMessages()}. */ public void close(); + /** + * Get the additional messages, if any, returned by the remote process. + *

+ * These messages are most likely informational or error messages, sent by + * the remote peer, to help the end-user correct any problems that may have + * prevented the operation from completing successfully. Application UIs + * should try to show these in an appropriate context. + *

+ * The message buffer is available after {@link #close()} has been called. + * Prior to closing the connection, the message buffer may be empty. + * + * @return the messages returned by the remote, most likely terminated by a + * newline (LF) character. The empty string is returned if the + * remote produced no additional messages. + */ + public String getMessages(); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java index 65a5b1769..b86f86d2f 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java @@ -146,7 +146,7 @@ else if (tagopt == TagOpt.FETCH_TAGS) // Connection was used for object transfer. If we // do another fetch we must open a new connection. // - closeConnection(); + closeConnection(result); } else { includedTags = false; } @@ -170,7 +170,7 @@ else if (tagopt == TagOpt.FETCH_TAGS) } } } finally { - closeConnection(); + closeConnection(result); } final RevWalk walk = new RevWalk(transport.local); @@ -210,9 +210,10 @@ private void fetchObjects(final ProgressMonitor monitor) "peer did not supply a complete object graph"); } - private void closeConnection() { + private void closeConnection(final FetchResult result) { if (conn != null) { conn.close(); + result.addMessages(conn.getMessages()); conn = null; } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/OperationResult.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/OperationResult.java index e93f7f760..115cfbcc8 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/OperationResult.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/OperationResult.java @@ -1,4 +1,5 @@ /* + * Copyright (C) 2010, Google Inc. * Copyright (C) 2008, Marek Zawirski * Copyright (C) 2007-2009, Robin Rosenberg * Copyright (C) 2008, Shawn O. Pearce @@ -65,6 +66,8 @@ public abstract class OperationResult { final SortedMap updates = new TreeMap(); + StringBuilder messageBuffer; + /** * Get the URI this result came from. *

@@ -136,4 +139,30 @@ void setAdvertisedRefs(final URIish u, final Map ar) { void add(final TrackingRefUpdate u) { updates.put(u.getLocalName(), u); } + + /** + * Get the additional messages, if any, returned by the remote process. + *

+ * These messages are most likely informational or error messages, sent by + * the remote peer, to help the end-user correct any problems that may have + * prevented the operation from completing successfully. Application UIs + * should try to show these in an appropriate context. + * + * @return the messages returned by the remote, most likely terminated by a + * newline (LF) character. The empty string is returned if the + * remote produced no additional messages. + */ + public String getMessages() { + return messageBuffer != null ? messageBuffer.toString() : ""; + } + + void addMessages(final String msg) { + if (msg != null && msg.length() > 0) { + if (messageBuffer == null) + messageBuffer = new StringBuilder(); + messageBuffer.append(msg); + if (!msg.endsWith("\n")) + messageBuffer.append('\n'); + } + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushProcess.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushProcess.java index 17e1dfc77..03b783427 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushProcess.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushProcess.java @@ -122,8 +122,12 @@ class PushProcess { PushResult execute(final ProgressMonitor monitor) throws NotSupportedException, TransportException { monitor.beginTask(PROGRESS_OPENING_CONNECTION, ProgressMonitor.UNKNOWN); + + final PushResult res = new PushResult(); connection = transport.openPush(); try { + res.setAdvertisedRefs(transport.getURI(), connection.getRefsMap()); + res.setRemoteUpdates(toPush); monitor.endTask(); final Map preprocessed = prepareRemoteUpdates(); @@ -133,10 +137,16 @@ else if (!preprocessed.isEmpty()) connection.push(monitor, preprocessed); } finally { connection.close(); + res.addMessages(connection.getMessages()); } if (!transport.isDryRun()) updateTrackingRefs(); - return prepareOperationResult(); + for (final RemoteRefUpdate rru : toPush.values()) { + final TrackingRefUpdate tru = rru.getTrackingRefUpdate(); + if (tru != null) + res.add(tru); + } + return res; } private Map prepareRemoteUpdates() @@ -226,17 +236,4 @@ private void updateTrackingRefs() { } } } - - private PushResult prepareOperationResult() { - final PushResult result = new PushResult(); - result.setAdvertisedRefs(transport.getURI(), connection.getRefsMap()); - result.setRemoteUpdates(toPush); - - for (final RemoteRefUpdate rru : toPush.values()) { - final TrackingRefUpdate tru = rru.getTrackingRefUpdate(); - if (tru != null) - result.add(tru); - } - return result; - } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java index 0abbe7e09..796cb745a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SideBandInputStream.java @@ -48,6 +48,7 @@ import java.io.IOException; import java.io.InputStream; +import java.io.Writer; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -83,10 +84,10 @@ class SideBandInputStream extends InputStream { static final int CH_ERROR = 3; private static Pattern P_UNBOUNDED = Pattern - .compile("^([\\w ]+): +(\\d+)(?:, done\\.)? *$"); + .compile("^([\\w ]+): +(\\d+)(?:, done\\.)? *[\r\n]$"); private static Pattern P_BOUNDED = Pattern - .compile("^([\\w ]+): +\\d+% +\\( *(\\d+)/ *(\\d+)\\)(?:, done\\.)? *$"); + .compile("^([\\w ]+): +\\d+% +\\( *(\\d+)/ *(\\d+)\\)(?:, done\\.)? *[\r\n]$"); private final InputStream rawIn; @@ -94,6 +95,8 @@ class SideBandInputStream extends InputStream { private final ProgressMonitor monitor; + private final Writer messages; + private String progressBuffer = ""; private String currentTask; @@ -106,10 +109,12 @@ class SideBandInputStream extends InputStream { private int available; - SideBandInputStream(final InputStream in, final ProgressMonitor progress) { + SideBandInputStream(final InputStream in, final ProgressMonitor progress, + final Writer messageStream) { rawIn = in; pckIn = new PacketLineIn(rawIn); monitor = progress; + messages = messageStream; currentTask = ""; } @@ -170,7 +175,7 @@ private void needDataPacket() throws IOException { } } - private void progress(String pkt) { + private void progress(String pkt) throws IOException { pkt = progressBuffer + pkt; for (;;) { final int lf = pkt.indexOf('\n'); @@ -185,16 +190,13 @@ else if (0 <= cr) else break; - final String msg = pkt.substring(0, s); - if (doProgressLine(msg)) - pkt = pkt.substring(s + 1); - else - break; + doProgressLine(pkt.substring(0, s + 1)); + pkt = pkt.substring(s + 1); } progressBuffer = pkt; } - private boolean doProgressLine(final String msg) { + private void doProgressLine(final String msg) throws IOException { Matcher matcher; matcher = P_BOUNDED.matcher(msg); @@ -208,7 +210,7 @@ private boolean doProgressLine(final String msg) { final int cnt = Integer.parseInt(matcher.group(2)); monitor.update(cnt - lastCnt); lastCnt = cnt; - return true; + return; } matcher = P_UNBOUNDED.matcher(msg); @@ -222,10 +224,10 @@ private boolean doProgressLine(final String msg) { final int cnt = Integer.parseInt(matcher.group(2)); monitor.update(cnt - lastCnt); lastCnt = cnt; - return true; + return; } - return false; + messages.write(msg); } private void beginTask(final int totalWorkUnits) { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportGitSsh.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportGitSsh.java index 5ee7887f6..c69304243 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportGitSsh.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportGitSsh.java @@ -1,5 +1,4 @@ /* - * Copyright (C) 2008-2010, Google Inc. * Copyright (C) 2008, Marek Zawirski * Copyright (C) 2008, Robin Rosenberg * Copyright (C) 2008, Shawn O. Pearce @@ -47,8 +46,6 @@ package org.eclipse.jgit.transport; import java.io.IOException; -import java.io.InputStream; -import java.io.InterruptedIOException; import java.io.OutputStream; import java.io.PipedInputStream; import java.io.PipedOutputStream; @@ -57,6 +54,8 @@ import org.eclipse.jgit.errors.TransportException; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.util.QuotedString; +import org.eclipse.jgit.util.io.MessageWriter; +import org.eclipse.jgit.util.io.StreamCopyThread; import com.jcraft.jsch.ChannelExec; import com.jcraft.jsch.JSchException; @@ -88,8 +87,6 @@ static boolean canHandle(final URIish uri) { return false; } - OutputStream errStream; - TransportGitSsh(final Repository local, final URIish uri) { super(local, uri); } @@ -145,15 +142,15 @@ private String commandFor(final String exe) { return cmd.toString(); } - ChannelExec exec(final String exe) throws TransportException { + ChannelExec exec(final String exe, final OutputStream err) + throws TransportException { initSession(); final int tms = getTimeout() > 0 ? getTimeout() * 1000 : 0; try { final ChannelExec channel = (ChannelExec) sock.openChannel("exec"); channel.setCommand(commandFor(exe)); - errStream = createErrorStream(); - channel.setErrStream(errStream, true); + channel.setErrStream(err); channel.connect(tms); return channel; } catch (JSchException je) { @@ -161,9 +158,9 @@ ChannelExec exec(final String exe) throws TransportException { } } - void checkExecFailure(int status, String exe) throws TransportException { + void checkExecFailure(int status, String exe, String why) + throws TransportException { if (status == 127) { - String why = errStream.toString(); IOException cause = null; if (why != null && why.length() > 0) cause = new IOException(why); @@ -172,41 +169,8 @@ void checkExecFailure(int status, String exe) throws TransportException { } } - /** - * @return the error stream for the channel, the stream is used to detect - * specific error reasons for exceptions. - */ - private static OutputStream createErrorStream() { - return new OutputStream() { - private StringBuilder all = new StringBuilder(); - - private StringBuilder sb = new StringBuilder(); - - public String toString() { - String r = all.toString(); - while (r.endsWith("\n")) - r = r.substring(0, r.length() - 1); - return r; - } - - @Override - public void write(final int b) throws IOException { - if (b == '\r') { - return; - } - - sb.append((char) b); - - if (b == '\n') { - all.append(sb); - sb.setLength(0); - } - } - }; - } - - NoRemoteRepositoryException cleanNotFound(NoRemoteRepositoryException nf) { - String why = errStream.toString(); + NoRemoteRepositoryException cleanNotFound(NoRemoteRepositoryException nf, + String why) { if (why == null || why.length() == 0) return nf; @@ -235,7 +199,7 @@ private OutputStream outputStream(ChannelExec channel) throws IOException { if (getTimeout() <= 0) return out; final PipedInputStream pipeIn = new PipedInputStream(); - final CopyThread copyThread = new CopyThread(pipeIn, out); + final StreamCopyThread copyThread = new StreamCopyThread(pipeIn, out); final PipedOutputStream pipeOut = new PipedOutputStream(pipeIn) { @Override public void flush() throws IOException { @@ -257,65 +221,6 @@ public void close() throws IOException { return pipeOut; } - private static class CopyThread extends Thread { - private final InputStream src; - - private final OutputStream dst; - - private volatile boolean doFlush; - - CopyThread(final InputStream i, final OutputStream o) { - setName(Thread.currentThread().getName() + "-Output"); - src = i; - dst = o; - } - - void flush() { - if (!doFlush) { - doFlush = true; - interrupt(); - } - } - - @Override - public void run() { - try { - final byte[] buf = new byte[1024]; - for (;;) { - try { - if (doFlush) { - doFlush = false; - dst.flush(); - } - - final int n; - try { - n = src.read(buf); - } catch (InterruptedIOException wakey) { - continue; - } - if (n < 0) - break; - dst.write(buf, 0, n); - } catch (IOException e) { - break; - } - } - } finally { - try { - src.close(); - } catch (IOException e) { - // Ignore IO errors on close - } - try { - dst.close(); - } catch (IOException e) { - // Ignore IO errors on close - } - } - } - } - class SshFetchConnection extends BasePackFetchConnection { private ChannelExec channel; @@ -324,12 +229,14 @@ class SshFetchConnection extends BasePackFetchConnection { SshFetchConnection() throws TransportException { super(TransportGitSsh.this); try { - channel = exec(getOptionUploadPack()); + final MessageWriter msg = new MessageWriter(); + setMessageWriter(msg); + channel = exec(getOptionUploadPack(), msg.getRawStream()); if (channel.isConnected()) init(channel.getInputStream(), outputStream(channel)); else - throw new TransportException(uri, errStream.toString()); + throw new TransportException(uri, getMessages()); } catch (TransportException err) { close(); @@ -343,9 +250,9 @@ class SshFetchConnection extends BasePackFetchConnection { try { readAdvertisedRefs(); } catch (NoRemoteRepositoryException notFound) { - close(); - checkExecFailure(exitStatus, getOptionUploadPack()); - throw cleanNotFound(notFound); + final String msgs = getMessages(); + checkExecFailure(exitStatus, getOptionUploadPack(), msgs); + throw cleanNotFound(notFound, msgs); } } @@ -373,12 +280,14 @@ class SshPushConnection extends BasePackPushConnection { SshPushConnection() throws TransportException { super(TransportGitSsh.this); try { - channel = exec(getOptionReceivePack()); + final MessageWriter msg = new MessageWriter(); + setMessageWriter(msg); + channel = exec(getOptionReceivePack(), msg.getRawStream()); if (channel.isConnected()) init(channel.getInputStream(), outputStream(channel)); else - throw new TransportException(uri, errStream.toString()); + throw new TransportException(uri, getMessages()); } catch (TransportException err) { close(); @@ -392,9 +301,9 @@ class SshPushConnection extends BasePackPushConnection { try { readAdvertisedRefs(); } catch (NoRemoteRepositoryException notFound) { - close(); - checkExecFailure(exitStatus, getOptionReceivePack()); - throw cleanNotFound(notFound); + final String msgs = getMessages(); + checkExecFailure(exitStatus, getOptionReceivePack(), msgs); + throw cleanNotFound(notFound, msgs); } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportLocal.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportLocal.java index a99a9b413..a9bdcd809 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportLocal.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportLocal.java @@ -1,6 +1,6 @@ /* * Copyright (C) 2007, Dave Watson - * Copyright (C) 2008-2009, Google Inc. + * Copyright (C) 2008-2010, Google Inc. * Copyright (C) 2008, Marek Zawirski * Copyright (C) 2008, Robin Rosenberg * Copyright (C) 2008, Shawn O. Pearce @@ -59,6 +59,8 @@ import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.util.FS; +import org.eclipse.jgit.util.io.MessageWriter; +import org.eclipse.jgit.util.io.StreamCopyThread; /** * Transport to access a local directory as though it were a remote peer. @@ -129,11 +131,10 @@ public void close() { // Resources must be established per-connection. } - protected Process startProcessWithErrStream(final String cmd) + protected Process spawn(final String cmd) throws TransportException { try { final String[] args; - final Process proc; if (cmd.startsWith("git-")) { args = new String[] { "git", cmd.substring(4), PWD }; @@ -148,9 +149,7 @@ protected Process startProcessWithErrStream(final String cmd) } } - proc = Runtime.getRuntime().exec(args, null, remoteGitDir); - new StreamRewritingThread(cmd, proc.getErrorStream()).start(); - return proc; + return Runtime.getRuntime().exec(args, null, remoteGitDir); } catch (IOException err) { throw new TransportException(uri, err.getMessage(), err); } @@ -246,9 +245,20 @@ public void close() { class ForkLocalFetchConnection extends BasePackFetchConnection { private Process uploadPack; + private Thread errorReaderThread; + ForkLocalFetchConnection() throws TransportException { super(TransportLocal.this); - uploadPack = startProcessWithErrStream(getOptionUploadPack()); + + final MessageWriter msg = new MessageWriter(); + setMessageWriter(msg); + + uploadPack = spawn(getOptionUploadPack()); + + final InputStream upErr = uploadPack.getErrorStream(); + errorReaderThread = new StreamCopyThread(upErr, msg.getRawStream()); + errorReaderThread.start(); + final InputStream upIn = uploadPack.getInputStream(); final OutputStream upOut = uploadPack.getOutputStream(); init(upIn, upOut); @@ -268,6 +278,16 @@ public void close() { uploadPack = null; } } + + if (errorReaderThread != null) { + try { + errorReaderThread.join(); + } catch (InterruptedException e) { + // Stop waiting and return anyway. + } finally { + errorReaderThread = null; + } + } } } @@ -351,9 +371,20 @@ public void close() { class ForkLocalPushConnection extends BasePackPushConnection { private Process receivePack; + private Thread errorReaderThread; + ForkLocalPushConnection() throws TransportException { super(TransportLocal.this); - receivePack = startProcessWithErrStream(getOptionReceivePack()); + + final MessageWriter msg = new MessageWriter(); + setMessageWriter(msg); + + receivePack = spawn(getOptionReceivePack()); + + final InputStream rpErr = receivePack.getErrorStream(); + errorReaderThread = new StreamCopyThread(rpErr, msg.getRawStream()); + errorReaderThread.start(); + final InputStream rpIn = receivePack.getInputStream(); final OutputStream rpOut = receivePack.getOutputStream(); init(rpIn, rpOut); @@ -373,34 +404,14 @@ public void close() { receivePack = null; } } - } - } - static class StreamRewritingThread extends Thread { - private final InputStream in; - - StreamRewritingThread(final String cmd, final InputStream in) { - super("JGit " + cmd + " Errors"); - this.in = in; - } - - public void run() { - final byte[] tmp = new byte[512]; - try { - for (;;) { - final int n = in.read(tmp); - if (n < 0) - break; - System.err.write(tmp, 0, n); - System.err.flush(); - } - } catch (IOException err) { - // Ignore errors reading errors. - } finally { + if (errorReaderThread != null) { try { - in.close(); - } catch (IOException err2) { - // Ignore errors closing the pipe. + errorReaderThread.join(); + } catch (InterruptedException e) { + // Stop waiting and return anyway. + } finally { + errorReaderThread = null; } } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/io/MessageWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/io/MessageWriter.java new file mode 100644 index 000000000..22c3ce94e --- /dev/null +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/io/MessageWriter.java @@ -0,0 +1,115 @@ +/* + * Copyright (C) 2009-2010, Google Inc. + * and other copyright owners as documented in the project's IP log. + * + * This program and the accompanying materials are made available + * under the terms of the Eclipse Distribution License v1.0 which + * accompanies this distribution, is reproduced below, and is + * available at http://www.eclipse.org/org/documents/edl-v10.php + * + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * + * - Neither the name of the Eclipse Foundation, Inc. nor the + * names of its contributors may be used to endorse or promote + * products derived from this software without specific prior + * written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +package org.eclipse.jgit.util.io; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.OutputStream; +import java.io.OutputStreamWriter; +import java.io.Writer; + +import org.eclipse.jgit.lib.Constants; +import org.eclipse.jgit.transport.BaseConnection; +import org.eclipse.jgit.util.RawParseUtils; + +/** + * Combines messages from an OutputStream (hopefully in UTF-8) and a Writer. + *

+ * This class is primarily meant for {@link BaseConnection} in contexts where a + * standard error stream from a command execution, as well as messages from a + * side-band channel, need to be combined together into a buffer to represent + * the complete set of messages from a remote repository. + *

+ * Writes made to the writer are re-encoded as UTF-8 and interleaved into the + * buffer that {@link #getRawStream()} also writes to. + *

+ * {@link #toString()} returns all written data, after converting it to a String + * under the assumption of UTF-8 encoding. + *

+ * Internally {@link RawParseUtils#decode(byte[])} is used by {@code toString()} + * tries to work out a reasonably correct character set for the raw data. + */ +public class MessageWriter extends Writer { + private final ByteArrayOutputStream buf; + + private final OutputStreamWriter enc; + + /** Create an empty writer. */ + public MessageWriter() { + buf = new ByteArrayOutputStream(); + enc = new OutputStreamWriter(getRawStream(), Constants.CHARSET); + } + + @Override + public void write(char[] cbuf, int off, int len) throws IOException { + synchronized (buf) { + enc.write(cbuf, off, len); + enc.flush(); + } + } + + /** + * @return the underlying byte stream that character writes to this writer + * drop into. Writes to this stream should should be in UTF-8. + */ + public OutputStream getRawStream() { + return buf; + } + + @Override + public void close() throws IOException { + // Do nothing, we are buffered with no resources. + } + + @Override + public void flush() throws IOException { + // Do nothing, we are buffered with no resources. + } + + /** @return string version of all buffered data. */ + @Override + public String toString() { + return RawParseUtils.decode(buf.toByteArray()); + } +} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/io/StreamCopyThread.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/io/StreamCopyThread.java new file mode 100644 index 000000000..a2b954017 --- /dev/null +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/io/StreamCopyThread.java @@ -0,0 +1,128 @@ +/* + * Copyright (C) 2009-2010, Google Inc. + * and other copyright owners as documented in the project's IP log. + * + * This program and the accompanying materials are made available + * under the terms of the Eclipse Distribution License v1.0 which + * accompanies this distribution, is reproduced below, and is + * available at http://www.eclipse.org/org/documents/edl-v10.php + * + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * + * - Neither the name of the Eclipse Foundation, Inc. nor the + * names of its contributors may be used to endorse or promote + * products derived from this software without specific prior + * written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +package org.eclipse.jgit.util.io; + +import java.io.IOException; +import java.io.InputStream; +import java.io.InterruptedIOException; +import java.io.OutputStream; + +/** Thread to copy from an input stream to an output stream. */ +public class StreamCopyThread extends Thread { + private static final int BUFFER_SIZE = 1024; + + private final InputStream src; + + private final OutputStream dst; + + private volatile boolean doFlush; + + /** + * Create a thread to copy data from an input stream to an output stream. + * + * @param i + * stream to copy from. The thread terminates when this stream + * reaches EOF. The thread closes this stream before it exits. + * @param o + * stream to copy into. The destination stream is automatically + * closed when the thread terminates. + */ + public StreamCopyThread(final InputStream i, final OutputStream o) { + setName(Thread.currentThread().getName() + "-StreamCopy"); + src = i; + dst = o; + } + + /** + * Request the thread to flush the output stream as soon as possible. + *

+ * This is an asynchronous request to the thread. The actual flush will + * happen at some future point in time, when the thread wakes up to process + * the request. + */ + public void flush() { + if (!doFlush) { + doFlush = true; + interrupt(); + } + } + + @Override + public void run() { + try { + final byte[] buf = new byte[BUFFER_SIZE]; + for (;;) { + try { + if (doFlush) { + doFlush = false; + dst.flush(); + } + + final int n; + try { + n = src.read(buf); + } catch (InterruptedIOException wakey) { + continue; + } + if (n < 0) + break; + dst.write(buf, 0, n); + } catch (IOException e) { + break; + } + } + } finally { + try { + src.close(); + } catch (IOException e) { + // Ignore IO errors on close + } + try { + dst.close(); + } catch (IOException e) { + // Ignore IO errors on close + } + } + } +} From 243b0d64a68990b10a3e6e0d5612cd5bb25626f8 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 10 Feb 2010 19:54:07 -0800 Subject: [PATCH 08/14] Wait for EOF on stderr before finishing SSH channel JSch will allow us to close the connection and then just drop any late messages coming over the stderr stream for the command. This makes it easy to lose final output on a command, like from Gerrit Code Review's post receive hook. Instead spawn a background thread to copy data from JSch's pipe into our own buffer, and wait for that thread to receive EOF on the pipe before we declare the connection closed. This way we don't have a race condition between the stderr data arriving and JSch just tearing down the channel. Change-Id: Ica1ba40ed2b4b6efb7d5e4ea240efc0a56fb71f6 Signed-off-by: Shawn O. Pearce --- .../jgit/transport/TransportGitSsh.java | 58 ++++++++++++++----- 1 file changed, 45 insertions(+), 13 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportGitSsh.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportGitSsh.java index c69304243..d4d4f5412 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportGitSsh.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportGitSsh.java @@ -1,4 +1,5 @@ /* + * Copyright (C) 2008-2010, Google Inc. * Copyright (C) 2008, Marek Zawirski * Copyright (C) 2008, Robin Rosenberg * Copyright (C) 2008, Shawn O. Pearce @@ -46,6 +47,7 @@ package org.eclipse.jgit.transport; import java.io.IOException; +import java.io.InputStream; import java.io.OutputStream; import java.io.PipedInputStream; import java.io.PipedOutputStream; @@ -142,15 +144,13 @@ private String commandFor(final String exe) { return cmd.toString(); } - ChannelExec exec(final String exe, final OutputStream err) - throws TransportException { + ChannelExec exec(final String exe) throws TransportException { initSession(); final int tms = getTimeout() > 0 ? getTimeout() * 1000 : 0; try { final ChannelExec channel = (ChannelExec) sock.openChannel("exec"); channel.setCommand(commandFor(exe)); - channel.setErrStream(err); channel.connect(tms); return channel; } catch (JSchException je) { @@ -224,6 +224,8 @@ public void close() throws IOException { class SshFetchConnection extends BasePackFetchConnection { private ChannelExec channel; + private Thread errorThread; + private int exitStatus; SshFetchConnection() throws TransportException { @@ -231,12 +233,16 @@ class SshFetchConnection extends BasePackFetchConnection { try { final MessageWriter msg = new MessageWriter(); setMessageWriter(msg); - channel = exec(getOptionUploadPack(), msg.getRawStream()); - if (channel.isConnected()) - init(channel.getInputStream(), outputStream(channel)); - else - throw new TransportException(uri, getMessages()); + channel = exec(getOptionUploadPack()); + if (!channel.isConnected()) + throw new TransportException(uri, "connection failed"); + + final InputStream upErr = channel.getErrStream(); + errorThread = new StreamCopyThread(upErr, msg.getRawStream()); + errorThread.start(); + + init(channel.getInputStream(), outputStream(channel)); } catch (TransportException err) { close(); @@ -258,6 +264,16 @@ class SshFetchConnection extends BasePackFetchConnection { @Override public void close() { + if (errorThread != null) { + try { + errorThread.join(); + } catch (InterruptedException e) { + // Stop waiting and return anyway. + } finally { + errorThread = null; + } + } + super.close(); if (channel != null) { @@ -275,6 +291,8 @@ public void close() { class SshPushConnection extends BasePackPushConnection { private ChannelExec channel; + private Thread errorThread; + private int exitStatus; SshPushConnection() throws TransportException { @@ -282,12 +300,16 @@ class SshPushConnection extends BasePackPushConnection { try { final MessageWriter msg = new MessageWriter(); setMessageWriter(msg); - channel = exec(getOptionReceivePack(), msg.getRawStream()); - if (channel.isConnected()) - init(channel.getInputStream(), outputStream(channel)); - else - throw new TransportException(uri, getMessages()); + channel = exec(getOptionReceivePack()); + if (!channel.isConnected()) + throw new TransportException(uri, "connection failed"); + + final InputStream rpErr = channel.getErrStream(); + errorThread = new StreamCopyThread(rpErr, msg.getRawStream()); + errorThread.start(); + + init(channel.getInputStream(), outputStream(channel)); } catch (TransportException err) { close(); @@ -309,6 +331,16 @@ class SshPushConnection extends BasePackPushConnection { @Override public void close() { + if (errorThread != null) { + try { + errorThread.join(); + } catch (InterruptedException e) { + // Stop waiting and return anyway. + } finally { + errorThread = null; + } + } + super.close(); if (channel != null) { From 1f4a30b80d734d28baeb48fb45013716f0afb7a4 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Thu, 11 Feb 2010 11:43:29 -0800 Subject: [PATCH 09/14] Catch and report "ERR message" during remote advertisements GitHub broke the native git protocol a while ago by interjecting an "ERR message" line into the upload-pack or receive-pack advertisement list. This didn't match the expected pattern, so it caused existing C Git clients to abort with a protocol exception. These days, C Git clients actually look for this message and abort with a more graceful notice to the end-user. JGit should do the same, including setting up a custom exception type that makes it easier for higher-level UIs to identify a message from the remote site and present it to the user. Change-Id: I51ab62a382cfaf1082210e8bfaa69506fd0d9786 Signed-off-by: Shawn O. Pearce --- .../errors/RemoteRepositoryException.java | 70 +++++++++++++++++++ .../jgit/transport/BasePackConnection.java | 20 ++++++ 2 files changed, 90 insertions(+) create mode 100644 org.eclipse.jgit/src/org/eclipse/jgit/errors/RemoteRepositoryException.java diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/errors/RemoteRepositoryException.java b/org.eclipse.jgit/src/org/eclipse/jgit/errors/RemoteRepositoryException.java new file mode 100644 index 000000000..ab094c946 --- /dev/null +++ b/org.eclipse.jgit/src/org/eclipse/jgit/errors/RemoteRepositoryException.java @@ -0,0 +1,70 @@ +/* + * Copyright (C) 2010, Google Inc. + * and other copyright owners as documented in the project's IP log. + * + * This program and the accompanying materials are made available + * under the terms of the Eclipse Distribution License v1.0 which + * accompanies this distribution, is reproduced below, and is + * available at http://www.eclipse.org/org/documents/edl-v10.php + * + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * + * - Neither the name of the Eclipse Foundation, Inc. nor the + * names of its contributors may be used to endorse or promote + * products derived from this software without specific prior + * written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +package org.eclipse.jgit.errors; + +import org.eclipse.jgit.transport.URIish; + +/** + * Contains a message from the remote repository indicating a problem. + *

+ * Some remote repositories may send customized error messages describing why + * they cannot be accessed. These messages are wrapped up in this exception and + * thrown to the caller of the transport operation. + */ +public class RemoteRepositoryException extends TransportException { + private static final long serialVersionUID = 1L; + + /** + * Constructs a RemoteRepositoryException for a message. + * + * @param uri + * URI used for transport + * @param message + * message, exactly as supplied by the remote repository. May + * contain LFs (newlines) if the remote formatted it that way. + */ + public RemoteRepositoryException(URIish uri, String message) { + super(uri, message); + } +} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackConnection.java index 74c27a7cd..0411c61fa 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackConnection.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackConnection.java @@ -59,6 +59,7 @@ import org.eclipse.jgit.errors.NoRemoteRepositoryException; import org.eclipse.jgit.errors.PackProtocolException; +import org.eclipse.jgit.errors.RemoteRepositoryException; import org.eclipse.jgit.errors.TransportException; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectIdRef; @@ -149,6 +150,19 @@ protected final void init(InputStream myIn, OutputStream myOut) { outNeedsEnd = true; } + /** + * Reads the advertised references through the initialized stream. + *

+ * Subclass implementations may call this method only after setting up the + * input and output streams with {@link #init(InputStream, OutputStream)}. + *

+ * If any errors occur, this connection is automatically closed by invoking + * {@link #close()} and the exception is wrapped (if necessary) and thrown + * as a {@link TransportException}. + * + * @throws TransportException + * the reference list could not be scanned. + */ protected void readAdvertisedRefs() throws TransportException { try { readAdvertisedRefsImpl(); @@ -179,6 +193,12 @@ private void readAdvertisedRefsImpl() throws IOException { if (line == PacketLineIn.END) break; + if (line.startsWith("ERR ")) { + // This is a customized remote service error. + // Users should be informed about it. + throw new RemoteRepositoryException(uri, line.substring(4)); + } + if (avail.isEmpty()) { final int nul = line.indexOf('\0'); if (nul >= 0) { From d8c3e98d73a4fb7409580a7d9f3395ac506f7e5d Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Thu, 11 Feb 2010 10:58:22 -0800 Subject: [PATCH 10/14] Use "ERR message" for early ReceivePack problems If the application wants to, it can use sendError(String) to send one or more error messages to clients before the advertisements are sent. These will cause a C Git client to break out of the advertisement parsing loop, display "remote error: message\n", and terminate. Servers can optionally use this to send a detailed error to a client explaining why it cannot use the ReceivePack service on a repository. Over smart HTTP these errors are sent in a 200 OK response, and are in the payload, allowing the Git client to give the end-user the custom message rather than the generic error "403 Forbidden". Change-Id: I03f4345183765d21002118617174c77f71427b5a Signed-off-by: Shawn O. Pearce --- .../jgit/http/test/AdvertiseErrorTest.java | 151 ++++++++++++++++++ .../eclipse/jgit/transport/ReceivePack.java | 39 ++++- 2 files changed, 182 insertions(+), 8 deletions(-) create mode 100644 org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/AdvertiseErrorTest.java diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/AdvertiseErrorTest.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/AdvertiseErrorTest.java new file mode 100644 index 000000000..47d7806a1 --- /dev/null +++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/AdvertiseErrorTest.java @@ -0,0 +1,151 @@ +/* + * Copyright (C) 2010, Google Inc. + * and other copyright owners as documented in the project's IP log. + * + * This program and the accompanying materials are made available + * under the terms of the Eclipse Distribution License v1.0 which + * accompanies this distribution, is reproduced below, and is + * available at http://www.eclipse.org/org/documents/edl-v10.php + * + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * + * - Neither the name of the Eclipse Foundation, Inc. nor the + * names of its contributors may be used to endorse or promote + * products derived from this software without specific prior + * written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +package org.eclipse.jgit.http.test; + +import java.util.Collections; + +import javax.servlet.http.HttpServletRequest; + +import org.eclipse.jetty.servlet.ServletContextHandler; +import org.eclipse.jetty.servlet.ServletHolder; +import org.eclipse.jgit.errors.RemoteRepositoryException; +import org.eclipse.jgit.errors.RepositoryNotFoundException; +import org.eclipse.jgit.http.server.GitServlet; +import org.eclipse.jgit.http.server.resolver.DefaultReceivePackFactory; +import org.eclipse.jgit.http.server.resolver.RepositoryResolver; +import org.eclipse.jgit.http.server.resolver.ServiceNotAuthorizedException; +import org.eclipse.jgit.http.server.resolver.ServiceNotEnabledException; +import org.eclipse.jgit.http.test.util.HttpTestCase; +import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.lib.Constants; +import org.eclipse.jgit.lib.NullProgressMonitor; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.lib.RepositoryConfig; +import org.eclipse.jgit.revwalk.RevBlob; +import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.transport.ReceivePack; +import org.eclipse.jgit.transport.RemoteRefUpdate; +import org.eclipse.jgit.transport.Transport; +import org.eclipse.jgit.transport.URIish; + +public class AdvertiseErrorTest extends HttpTestCase { + private Repository remoteRepository; + + private URIish remoteURI; + + protected void setUp() throws Exception { + super.setUp(); + + final TestRepository src = createTestRepository(); + final String srcName = src.getRepository().getDirectory().getName(); + + ServletContextHandler app = server.addContext("/git"); + GitServlet gs = new GitServlet(); + gs.setRepositoryResolver(new RepositoryResolver() { + public Repository open(HttpServletRequest req, String name) + throws RepositoryNotFoundException, + ServiceNotEnabledException { + if (!name.equals(srcName)) + throw new RepositoryNotFoundException(name); + + final Repository db = src.getRepository(); + db.incrementOpen(); + return db; + } + }); + gs.setReceivePackFactory(new DefaultReceivePackFactory() { + public ReceivePack create(HttpServletRequest req, Repository db) + throws ServiceNotEnabledException, + ServiceNotAuthorizedException { + ReceivePack rp = super.create(req, db); + rp.sendError("message line 1"); + rp.sendError("no soup for you!"); + rp.sendError("come back next year!"); + return rp; + } + + }); + app.addServlet(new ServletHolder(gs), "/*"); + + server.setUp(); + + remoteRepository = src.getRepository(); + remoteURI = toURIish(app, srcName); + + RepositoryConfig cfg = remoteRepository.getConfig(); + cfg.setBoolean("http", null, "receivepack", true); + cfg.save(); + } + + public void testPush_CreateBranch() throws Exception { + final TestRepository src = createTestRepository(); + final RevBlob Q_txt = src.blob("new text"); + final RevCommit Q = src.commit().add("Q", Q_txt).create(); + final Repository db = src.getRepository(); + final String dstName = Constants.R_HEADS + "new.branch"; + final Transport t = Transport.open(db, remoteURI); + try { + final String srcExpr = Q.name(); + final boolean forceUpdate = false; + final String localName = null; + final ObjectId oldId = null; + + RemoteRefUpdate update = new RemoteRefUpdate(src.getRepository(), + srcExpr, dstName, forceUpdate, localName, oldId); + try { + t.push(NullProgressMonitor.INSTANCE, Collections + .singleton(update)); + fail("push completed without throwing exception"); + } catch (RemoteRepositoryException error) { + assertEquals(remoteURI + ": message line 1\n" // + + "no soup for you!\n" // + + "come back next year!", // + error.getMessage()); + } + } finally { + t.close(); + } + } +} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java index c5bbdac5a..d79965870 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java @@ -163,6 +163,9 @@ public class ReceivePack { /** Commands to execute, as received by the client. */ private List commands; + /** Error to display instead of advertising the references. */ + private StringBuilder advertiseError; + /** An exception caught while unpacking and fsck'ing the objects. */ private Throwable unpackError; @@ -428,10 +431,17 @@ public List getAllCommands() { } /** - * Send an error message to the client, if it supports receiving them. + * Send an error message to the client. *

- * If the client doesn't support receiving messages, the message will be - * discarded, with no other indication to the caller or to the client. + * If any error messages are sent before the references are advertised to + * the client, the errors will be sent instead of the advertisement and the + * receive operation will be aborted. All clients should receive and display + * such early stage errors. + *

+ * If the reference advertisements have already been sent, messages are sent + * in a side channel. If the client doesn't support receiving messages, the + * message will be discarded, with no other indication to the caller or to + * the client. *

* {@link PreReceiveHook}s should always try to use * {@link ReceiveCommand#setResult(Result, String)} with a result status of @@ -444,11 +454,17 @@ public List getAllCommands() { * string must not end with an LF, and must not contain an LF. */ public void sendError(final String what) { - try { - if (msgs != null) - msgs.write("error: " + what + "\n"); - } catch (IOException e) { - // Ignore write failures. + if (refs == null) { + if (advertiseError == null) + advertiseError = new StringBuilder(); + advertiseError.append(what).append('\n'); + } else { + try { + if (msgs != null) + msgs.write("error: " + what + "\n"); + } catch (IOException e) { + // Ignore write failures. + } } } @@ -558,6 +574,8 @@ private void service() throws IOException { sendAdvertisedRefs(new PacketLineOutRefAdvertiser(pckOut)); else refs = refFilter.filter(db.getAllRefs()); + if (advertiseError != null) + return; recvCommands(); if (!commands.isEmpty()) { enableCapabilities(); @@ -618,6 +636,11 @@ private void unlockPack() { * the formatter failed to write an advertisement. */ public void sendAdvertisedRefs(final RefAdvertiser adv) throws IOException { + if (advertiseError != null) { + adv.writeOne("ERR " + advertiseError); + return; + } + final RevFlag advertised = walk.newFlag("ADVERTISED"); adv.init(walk, advertised); adv.advertiseCapability(CAPABILITY_SIDE_BAND_64K); From 882d03f70e089bfad2fe2e84f4f9cd69e805e332 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Thu, 11 Feb 2010 18:02:22 -0800 Subject: [PATCH 11/14] Fix smart HTTP client buffer alignment This proved to be a pretty difficult to find bug. If we read exactly the number of response bytes from the UnionInputStream and didn't try to read beyond that length, the last connection's InputStream is still inside of the UnionInputStream, and UnionInputStream.isEmpty() returns false. But there is no data present, so the next read request to our UnionInputStream returns EOF at a point where the HTTP client code should have started a new request in order to get more data. Instead of wrapping the UnionInputStream, push an dummy stream onto the end of it which when invoked always starts the next request and then returns EOF. The UnionInputStream will automatically pop that dummy stream out, and then read the next request's stream. This way we never get into the state where we don't think we need to run another request in order to satisfy the current read request, but we really do. The bug was hidden for so long because BasePackConnection.init() was always wrapping the InputStream into a BufferedInputStream with an 8 KiB buffer. This made the odds of us reading from the UnionInputStream the exact number of available bytes quite low, as the BufferedInputStream would always try to read a full buffer size. Change-Id: I02b5ec3ef6853688687d91de000a5fbe2354915d Signed-off-by: Shawn O. Pearce --- .../eclipse/jgit/transport/TransportHttp.java | 47 ++++++------------- 1 file changed, 15 insertions(+), 32 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java index 8de16c13d..f49828bf2 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java @@ -632,9 +632,9 @@ class Service { private final String responseType; - private final UnionInputStream httpIn; + private final HttpExecuteStream execute; - final HttpInputStream in; + final UnionInputStream in; final HttpOutputStream out; @@ -645,8 +645,8 @@ class Service { this.requestType = "application/x-" + serviceName + "-request"; this.responseType = "application/x-" + serviceName + "-result"; - this.httpIn = new UnionInputStream(); - this.in = new HttpInputStream(httpIn); + this.execute = new HttpExecuteStream(); + this.in = new UnionInputStream(execute); this.out = new HttpOutputStream(); } @@ -712,7 +712,8 @@ void execute() throws IOException { throw wrongContentType(responseType, contentType); } - httpIn.add(openInputStream(conn)); + in.add(openInputStream(conn)); + in.add(execute); conn = null; } @@ -729,43 +730,25 @@ protected OutputStream overflow() throws IOException { } } - class HttpInputStream extends InputStream { - private final UnionInputStream src; - - HttpInputStream(UnionInputStream httpIn) { - this.src = httpIn; - } - - private InputStream self() throws IOException { - if (src.isEmpty()) { - // If we have no InputStreams available it means we must - // have written data previously to the service, but have - // not yet finished the HTTP request in order to get the - // response from the service. Ensure we get it now. - // - execute(); - } - return src; - } - + class HttpExecuteStream extends InputStream { public int available() throws IOException { - return self().available(); + execute(); + return 0; } public int read() throws IOException { - return self().read(); + execute(); + return -1; } public int read(byte[] b, int off, int len) throws IOException { - return self().read(b, off, len); + execute(); + return -1; } public long skip(long n) throws IOException { - return self().skip(n); - } - - public void close() throws IOException { - src.close(); + execute(); + return 0; } } } From 2156aa894cefbabd322fc405138c306bb4e939cd Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Thu, 11 Feb 2010 18:10:45 -0800 Subject: [PATCH 12/14] Reduce multi-level buffered streams in transport code Some transports actually provide stream buffering on their own, without needing to be wrapped up inside of a BufferedInputStream in order to smooth out system calls to read or write. A great example of this is the JSch SSH client, or the Apache MINA SSHD server. Both use custom buffering to packetize the streams into the encrypted SSH channel, and wrapping them up inside of a BufferedInputStream or BufferedOutputStream is relatively pointless. Our SideBandOutputStream implementation also provides some fairly large buffering, equal to one complete side-band packet on the main data channel. Wrapping that inside of a BufferedOutputStream just to smooth out small writes from PackWriter causes extra data copies, and provides no advantage. We can save some memory and some CPU cycles by letting PackWriter dump directly into the SideBandOutputStream's internal buffer array. Instead we push the buffering streams down to be as close to the network socket (or operating system pipe) as possible. This allows us to smooth out the smaller reads/writes from pkt-line messages during advertisement and negotation, but avoid copying altogether when the stream switches to larger writes over a side band channel. Change-Id: I2f6f16caee64783c77d3dd1b2a41b3cc0c64c159 Signed-off-by: Shawn O. Pearce --- .../eclipse/jgit/junit/TestRepository.java | 8 ++++--- .../jgit/lib/ConcurrentRepackTest.java | 10 ++++---- .../src/org/eclipse/jgit/lib/PackWriter.java | 12 +++------- .../jgit/transport/BasePackConnection.java | 24 ++++++++++++------- .../transport/BasePackPushConnection.java | 1 + .../eclipse/jgit/transport/BundleWriter.java | 12 ++++------ .../jgit/transport/TransportGitAnon.java | 20 ++++++++++++++-- .../jgit/transport/TransportLocal.java | 18 ++++++++++---- .../eclipse/jgit/transport/UploadPack.java | 7 ++---- .../jgit/transport/WalkPushConnection.java | 3 +++ 10 files changed, 71 insertions(+), 44 deletions(-) diff --git a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java index e738276bd..59504aa78 100644 --- a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java +++ b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java @@ -43,9 +43,11 @@ package org.eclipse.jgit.junit; +import java.io.BufferedOutputStream; import java.io.File; import java.io.FileOutputStream; import java.io.IOException; +import java.io.OutputStream; import java.security.MessageDigest; import java.util.ArrayList; import java.util.Collections; @@ -570,10 +572,10 @@ public void packAndPrune() throws Exception { pw.preparePack(all, Collections. emptySet()); final ObjectId name = pw.computeName(); - FileOutputStream out; + OutputStream out; final File pack = nameFor(odb, name, ".pack"); - out = new FileOutputStream(pack); + out = new BufferedOutputStream(new FileOutputStream(pack)); try { pw.writePack(out); } finally { @@ -582,7 +584,7 @@ public void packAndPrune() throws Exception { pack.setReadOnly(); final File idx = nameFor(odb, name, ".idx"); - out = new FileOutputStream(idx); + out = new BufferedOutputStream(new FileOutputStream(idx)); try { pw.writeIndex(out); } finally { diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConcurrentRepackTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConcurrentRepackTest.java index 9e83aa0e1..69430ed33 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConcurrentRepackTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConcurrentRepackTest.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009, Google Inc. + * Copyright (C) 2009-2010, Google Inc. * Copyright (C) 2009, Robin Rosenberg * and other copyright owners as documented in the project's IP log. * @@ -44,9 +44,11 @@ package org.eclipse.jgit.lib; +import java.io.BufferedOutputStream; import java.io.File; import java.io.FileOutputStream; import java.io.IOException; +import java.io.OutputStream; import java.util.Arrays; import org.eclipse.jgit.errors.IncorrectObjectTypeException; @@ -203,16 +205,16 @@ private File[] pack(final Repository src, final RevObject... list) private static void write(final File[] files, final PackWriter pw) throws IOException { final long begin = files[0].getParentFile().lastModified(); - FileOutputStream out; + OutputStream out; - out = new FileOutputStream(files[0]); + out = new BufferedOutputStream(new FileOutputStream(files[0])); try { pw.writePack(out); } finally { out.close(); } - out = new FileOutputStream(files[1]); + out = new BufferedOutputStream(new FileOutputStream(files[1])); try { pw.writeIndex(out); } finally { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/PackWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/PackWriter.java index 6162deab7..b30e5f7c2 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/PackWriter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/PackWriter.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008-2009, Google Inc. + * Copyright (C) 2008-2010, Google Inc. * Copyright (C) 2008, Marek Zawirski * and other copyright owners as documented in the project's IP log. * @@ -44,7 +44,6 @@ package org.eclipse.jgit.lib; -import java.io.BufferedOutputStream; import java.io.IOException; import java.io.OutputStream; import java.security.MessageDigest; @@ -97,7 +96,6 @@ * undefined behavior. *

*/ - public class PackWriter { /** * Title of {@link ProgressMonitor} task used during counting objects to @@ -578,9 +576,8 @@ private List sortByName() { *

* * @param packStream - * output stream of pack data. If the stream is not buffered it - * will be buffered by the writer. Caller is responsible for - * closing the stream. + * output stream of pack data. The stream should be buffered by + * the caller. The caller is responsible for closing the stream. * @throws IOException * an error occurred reading a local object's data to include in * the pack, or writing compressed object data to the output @@ -590,8 +587,6 @@ public void writePack(OutputStream packStream) throws IOException { if (reuseDeltas || reuseObjects) searchForReuse(); - if (!(packStream instanceof BufferedOutputStream)) - packStream = new BufferedOutputStream(packStream); out = new PackOutputStream(packStream); writeMonitor.beginTask(WRITING_OBJECTS_PROGRESS, getObjectsNumber()); @@ -599,7 +594,6 @@ public void writePack(OutputStream packStream) throws IOException { writeObjects(); writeChecksum(); - out.flush(); windowCursor.release(); writeMonitor.endTask(); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackConnection.java index 0411c61fa..a2c572c60 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackConnection.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackConnection.java @@ -1,5 +1,4 @@ /* - * Copyright (C) 2009, Constantine Plotnikov * Copyright (C) 2008-2010, Google Inc. * Copyright (C) 2008, Marek Zawirski * Copyright (C) 2008, Robin Rosenberg @@ -47,8 +46,6 @@ package org.eclipse.jgit.transport; -import java.io.BufferedInputStream; -import java.io.BufferedOutputStream; import java.io.EOFException; import java.io.IOException; import java.io.InputStream; @@ -97,10 +94,10 @@ abstract class BasePackConnection extends BaseConnection { /** Timer to manage {@link #timeoutIn} and {@link #timeoutOut}. */ private InterruptTimer myTimer; - /** Buffered input stream reading from the remote. */ + /** Input stream reading from the remote. */ protected InputStream in; - /** Buffered output stream sending to the remote. */ + /** Output stream sending to the remote. */ protected OutputStream out; /** Packet line decoder around {@link #in}. */ @@ -127,6 +124,17 @@ abstract class BasePackConnection extends BaseConnection { uri = transport.uri; } + /** + * Configure this connection with the directional pipes. + * + * @param myIn + * input stream to receive data from the peer. Caller must ensure + * the input is buffered, otherwise read performance may suffer. + * @param myOut + * output stream to transmit data to the peer. Caller must ensure + * the output is buffered, otherwise write performance may + * suffer. + */ protected final void init(InputStream myIn, OutputStream myOut) { final int timeout = transport.getTimeout(); if (timeout > 0) { @@ -140,10 +148,8 @@ protected final void init(InputStream myIn, OutputStream myOut) { myOut = timeoutOut; } - in = myIn instanceof BufferedInputStream ? myIn - : new BufferedInputStream(myIn, IndexPack.BUFFER_SIZE); - out = myOut instanceof BufferedOutputStream ? myOut - : new BufferedOutputStream(myOut); + in = myIn; + out = myOut; pckIn = new PacketLineIn(in); pckOut = new PacketLineOut(out); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackPushConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackPushConnection.java index ba1170747..e10cefd3a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackPushConnection.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackPushConnection.java @@ -244,6 +244,7 @@ private void writePack(final Map refUpdates, writer.preparePack(newObjects, remoteObjects); final long start = System.currentTimeMillis(); writer.writePack(out); + out.flush(); packTransferTime = System.currentTimeMillis() - start; } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BundleWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BundleWriter.java index db1312ca3..7b0a5eec4 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BundleWriter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BundleWriter.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008-2009, Google Inc. + * Copyright (C) 2008-2010, Google Inc. * and other copyright owners as documented in the project's IP log. * * This program and the accompanying materials are made available @@ -43,7 +43,6 @@ package org.eclipse.jgit.transport; -import java.io.BufferedOutputStream; import java.io.IOException; import java.io.OutputStream; import java.io.OutputStreamWriter; @@ -155,18 +154,15 @@ public void assume(final RevCommit c) { * This method can only be called once per BundleWriter instance. * * @param os - * the stream the bundle is written to. If the stream is not - * buffered it will be buffered by the writer. Caller is - * responsible for closing the stream. + * the stream the bundle is written to. The stream should be + * buffered by the caller. The caller is responsible for closing + * the stream. * @throws IOException * an error occurred reading a local object's data to include in * the bundle, or writing compressed object data to the output * stream. */ public void writeBundle(OutputStream os) throws IOException { - if (!(os instanceof BufferedOutputStream)) - os = new BufferedOutputStream(os); - final HashSet inc = new HashSet(); final HashSet exc = new HashSet(); inc.addAll(include.values()); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportGitAnon.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportGitAnon.java index a127ff50a..8a0b4357c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportGitAnon.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportGitAnon.java @@ -45,7 +45,11 @@ package org.eclipse.jgit.transport; +import java.io.BufferedInputStream; +import java.io.BufferedOutputStream; import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; import java.net.ConnectException; import java.net.InetAddress; import java.net.InetSocketAddress; @@ -136,7 +140,13 @@ class TcpFetchConnection extends BasePackFetchConnection { super(TransportGitAnon.this); sock = openConnection(); try { - init(sock.getInputStream(), sock.getOutputStream()); + InputStream sIn = sock.getInputStream(); + OutputStream sOut = sock.getOutputStream(); + + sIn = new BufferedInputStream(sIn); + sOut = new BufferedOutputStream(sOut); + + init(sIn, sOut); service("git-upload-pack", pckOut); } catch (IOException err) { close(); @@ -169,7 +179,13 @@ class TcpPushConnection extends BasePackPushConnection { super(TransportGitAnon.this); sock = openConnection(); try { - init(sock.getInputStream(), sock.getOutputStream()); + InputStream sIn = sock.getInputStream(); + OutputStream sOut = sock.getOutputStream(); + + sIn = new BufferedInputStream(sIn); + sOut = new BufferedOutputStream(sOut); + + init(sIn, sOut); service("git-receive-pack", pckOut); } catch (IOException err) { close(); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportLocal.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportLocal.java index a9bdcd809..b9b9dbd00 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportLocal.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportLocal.java @@ -47,6 +47,8 @@ package org.eclipse.jgit.transport; +import java.io.BufferedInputStream; +import java.io.BufferedOutputStream; import java.io.File; import java.io.IOException; import java.io.InputStream; @@ -259,8 +261,12 @@ class ForkLocalFetchConnection extends BasePackFetchConnection { errorReaderThread = new StreamCopyThread(upErr, msg.getRawStream()); errorReaderThread.start(); - final InputStream upIn = uploadPack.getInputStream(); - final OutputStream upOut = uploadPack.getOutputStream(); + InputStream upIn = uploadPack.getInputStream(); + OutputStream upOut = uploadPack.getOutputStream(); + + upIn = new BufferedInputStream(upIn); + upOut = new BufferedOutputStream(upOut); + init(upIn, upOut); readAdvertisedRefs(); } @@ -385,8 +391,12 @@ class ForkLocalPushConnection extends BasePackPushConnection { errorReaderThread = new StreamCopyThread(rpErr, msg.getRawStream()); errorReaderThread.start(); - final InputStream rpIn = receivePack.getInputStream(); - final OutputStream rpOut = receivePack.getOutputStream(); + InputStream rpIn = receivePack.getInputStream(); + OutputStream rpOut = receivePack.getOutputStream(); + + rpIn = new BufferedInputStream(rpIn); + rpOut = new BufferedOutputStream(rpOut); + init(rpIn, rpOut); readAdvertisedRefs(); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index 39c4243ba..3d5abd34b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -583,12 +583,9 @@ private void sendPack() throws IOException { } } pw.writePack(packOut); + packOut.flush(); - if (sideband) { - packOut.flush(); + if (sideband) pckOut.end(); - } else { - rawOut.flush(); - } } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/WalkPushConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/WalkPushConnection.java index 88b7ca438..f977915bb 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/WalkPushConnection.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/WalkPushConnection.java @@ -45,6 +45,7 @@ import static org.eclipse.jgit.transport.WalkRemoteObjectDatabase.ROOT_DIR; +import java.io.BufferedOutputStream; import java.io.IOException; import java.io.OutputStream; import java.util.ArrayList; @@ -251,6 +252,7 @@ private void sendpack(final List updates, final String wt = "Put " + base.substring(0, 12); OutputStream os = dest.writeFile(pathPack, monitor, wt + "..pack"); try { + os = new BufferedOutputStream(os); pw.writePack(os); } finally { os.close(); @@ -258,6 +260,7 @@ private void sendpack(final List updates, os = dest.writeFile(pathIdx, monitor, wt + "..idx"); try { + os = new BufferedOutputStream(os); pw.writeIndex(os); } finally { os.close(); From c0f093899f8c027fb4e0f4418ac19b0f8ad6be16 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Thu, 11 Feb 2010 18:53:22 -0800 Subject: [PATCH 13/14] http.server: Use TemporaryBuffer and compress some responses The HTTP server side code now uses the same approach that the smart HTTP client code uses when preparing a request body. The payload is streamed into a TemporaryBuffer of limited size. If the entire data fits, its compressed with gzip if the user agent supports that, and a Content-Length header is used to transmit the fixed length body to the peer. If however the data overflows the limited memory segment, its streamed uncompressed to the peer. One might initially think that larger contents which overflow the buffer should also be compressed, rather than sent raw, since they were deemed "large". But usually these larger contents are actually a pack file which has been already heavily compressed by Git specific routines. Trying to deflate that with gzip is probably going to take up more space, not less, so the compression overhead isn't worthwhile. This buffer and compress optimization helps repositories with a large number of references, as their text based advertisements compress well. For example jgit's own native repository currently requires 32,628 bytes for its full advertisement of 489 references. Most repositories have fewer references, and thus could compress their entire response in one buffer. Change-Id: I790609c9f763339e0a1db9172aa570e29af96f42 Signed-off-by: Shawn O. Pearce --- .../jgit/http/server/InfoRefsServlet.java | 14 +- .../jgit/http/server/ReceivePackServlet.java | 22 +-- .../jgit/http/server/ServletUtils.java | 2 +- .../jgit/http/server/SmartOutputStream.java | 130 ++++++++++++++++++ .../http/server/SmartServiceInfoRefs.java | 9 +- .../jgit/http/server/UploadPackServlet.java | 5 +- .../http/test/SmartClientSmartServerTest.java | 4 +- 7 files changed, 150 insertions(+), 36 deletions(-) create mode 100644 org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/SmartOutputStream.java diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/InfoRefsServlet.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/InfoRefsServlet.java index b766196fd..f667ce95a 100644 --- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/InfoRefsServlet.java +++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/InfoRefsServlet.java @@ -44,9 +44,9 @@ package org.eclipse.jgit.http.server; import static org.eclipse.jgit.http.server.ServletUtils.getRepository; -import static org.eclipse.jgit.http.server.ServletUtils.send; import java.io.IOException; +import java.io.OutputStreamWriter; import java.util.Map; import javax.servlet.http.HttpServlet; @@ -69,20 +69,18 @@ public void doGet(final HttpServletRequest req, final HttpServletResponse rsp) throws IOException { // Assume a dumb client and send back the dumb client // version of the info/refs file. - final byte[] raw = dumbHttp(req); rsp.setContentType(HttpSupport.TEXT_PLAIN); rsp.setCharacterEncoding(Constants.CHARACTER_ENCODING); - send(raw, req, rsp); - } - private byte[] dumbHttp(final HttpServletRequest req) throws IOException { final Repository db = getRepository(req); final RevWalk walk = new RevWalk(db); final RevFlag ADVERTISED = walk.newFlag("ADVERTISED"); - final StringBuilder out = new StringBuilder(); + + final OutputStreamWriter out = new OutputStreamWriter( + new SmartOutputStream(req, rsp), Constants.CHARSET); final RefAdvertiser adv = new RefAdvertiser() { @Override - protected void writeOne(final CharSequence line) { + protected void writeOne(final CharSequence line) throws IOException { // Whoever decided that info/refs should use a different // delimiter than the native git:// protocol shouldn't // be allowed to design this sort of stuff. :-( @@ -100,6 +98,6 @@ protected void end() { Map refs = db.getAllRefs(); refs.remove(Constants.HEAD); adv.send(refs); - return out.toString().getBytes(Constants.CHARACTER_ENCODING); + out.close(); } } diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ReceivePackServlet.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ReceivePackServlet.java index 1a426af96..ba8b8ab66 100644 --- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ReceivePackServlet.java +++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ReceivePackServlet.java @@ -50,9 +50,7 @@ import static org.eclipse.jgit.http.server.ServletUtils.getInputStream; import static org.eclipse.jgit.http.server.ServletUtils.getRepository; -import java.io.ByteArrayOutputStream; import java.io.IOException; -import java.io.OutputStream; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; @@ -104,11 +102,14 @@ public void doPost(final HttpServletRequest req, } final Repository db = getRepository(req); - final ByteArrayOutputStream out = new ByteArrayOutputStream(); try { final ReceivePack rp = receivePackFactory.create(req, db); rp.setBiDirectionalPipe(false); + rsp.setContentType(RSP_TYPE); + + final SmartOutputStream out = new SmartOutputStream(req, rsp); rp.receive(getInputStream(req), out, null); + out.close(); } catch (ServiceNotAuthorizedException e) { rsp.sendError(SC_UNAUTHORIZED); @@ -123,20 +124,5 @@ public void doPost(final HttpServletRequest req, rsp.sendError(SC_INTERNAL_SERVER_ERROR); return; } - - reply(rsp, out.toByteArray()); - } - - private void reply(final HttpServletResponse rsp, final byte[] result) - throws IOException { - rsp.setContentType(RSP_TYPE); - rsp.setContentLength(result.length); - final OutputStream os = rsp.getOutputStream(); - try { - os.write(result); - os.flush(); - } finally { - os.close(); - } } } diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ServletUtils.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ServletUtils.java index d6b039246..7514d7c7e 100644 --- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ServletUtils.java +++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ServletUtils.java @@ -189,7 +189,7 @@ private static byte[] sendInit(byte[] content, return content; } - private static boolean acceptsGzipEncoding(final HttpServletRequest req) { + static boolean acceptsGzipEncoding(final HttpServletRequest req) { final String accepts = req.getHeader(HDR_ACCEPT_ENCODING); return accepts != null && 0 <= accepts.indexOf(ENCODING_GZIP); } diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/SmartOutputStream.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/SmartOutputStream.java new file mode 100644 index 000000000..00cb67ca9 --- /dev/null +++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/SmartOutputStream.java @@ -0,0 +1,130 @@ +/* + * Copyright (C) 2010, Google Inc. + * and other copyright owners as documented in the project's IP log. + * + * This program and the accompanying materials are made available + * under the terms of the Eclipse Distribution License v1.0 which + * accompanies this distribution, is reproduced below, and is + * available at http://www.eclipse.org/org/documents/edl-v10.php + * + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * + * - Neither the name of the Eclipse Foundation, Inc. nor the + * names of its contributors may be used to endorse or promote + * products derived from this software without specific prior + * written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +package org.eclipse.jgit.http.server; + +import static org.eclipse.jgit.http.server.ServletUtils.acceptsGzipEncoding; +import static org.eclipse.jgit.util.HttpSupport.ENCODING_GZIP; +import static org.eclipse.jgit.util.HttpSupport.HDR_CONTENT_ENCODING; + +import java.io.IOException; +import java.io.OutputStream; +import java.util.zip.GZIPOutputStream; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.eclipse.jgit.util.TemporaryBuffer; + +/** + * Buffers a response, trying to gzip it if the user agent supports that. + *

+ * If the response overflows the buffer, gzip is skipped and the response is + * streamed to the client as its produced, most likely using HTTP/1.1 chunked + * encoding. This is useful for servlets that produce mixed-mode content, where + * smaller payloads are primarily pure text that compresses well, while much + * larger payloads are heavily compressed binary data. {@link UploadPackServlet} + * is one such servlet. + */ +class SmartOutputStream extends TemporaryBuffer { + private static final int LIMIT = 32 * 1024; + + private final HttpServletRequest req; + + private final HttpServletResponse rsp; + + private boolean startedOutput; + + SmartOutputStream(final HttpServletRequest req, + final HttpServletResponse rsp) { + super(LIMIT); + this.req = req; + this.rsp = rsp; + } + + @Override + protected OutputStream overflow() throws IOException { + startedOutput = true; + return rsp.getOutputStream(); + } + + public void close() throws IOException { + super.close(); + + if (!startedOutput) { + // If output hasn't started yet, the entire thing fit into our + // buffer. Try to use a proper Content-Length header, and also + // deflate the response with gzip if it will be smaller. + TemporaryBuffer out = this; + + if (256 < out.length() && acceptsGzipEncoding(req)) { + TemporaryBuffer gzbuf = new TemporaryBuffer.Heap(LIMIT); + try { + GZIPOutputStream gzip = new GZIPOutputStream(gzbuf); + out.writeTo(gzip, null); + gzip.close(); + if (gzbuf.length() < out.length()) { + out = gzbuf; + rsp.setHeader(HDR_CONTENT_ENCODING, ENCODING_GZIP); + } + } catch (IOException err) { + // Most likely caused by overflowing the buffer, meaning + // its larger if it were compressed. Discard compressed + // copy and use the original. + } + } + + // The Content-Length cannot overflow when cast to an int, our + // hardcoded LIMIT constant above assures us we wouldn't store + // more than 2 GiB of content in memory. + rsp.setContentLength((int) out.length()); + final OutputStream os = rsp.getOutputStream(); + try { + out.writeTo(os, null); + os.flush(); + } finally { + os.close(); + } + } + } +} diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/SmartServiceInfoRefs.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/SmartServiceInfoRefs.java index 96716d070..94e2e7f6f 100644 --- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/SmartServiceInfoRefs.java +++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/SmartServiceInfoRefs.java @@ -46,9 +46,7 @@ import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN; import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED; import static org.eclipse.jgit.http.server.ServletUtils.getRepository; -import static org.eclipse.jgit.http.server.ServletUtils.send; -import java.io.ByteArrayOutputStream; import java.io.IOException; import javax.servlet.Filter; @@ -90,13 +88,14 @@ public void doFilter(ServletRequest request, ServletResponse response, final HttpServletResponse rsp = (HttpServletResponse) response; try { final Repository db = getRepository(req); - final ByteArrayOutputStream buf = new ByteArrayOutputStream(); + rsp.setContentType("application/x-" + svc + "-advertisement"); + + final SmartOutputStream buf = new SmartOutputStream(req, rsp); final PacketLineOut out = new PacketLineOut(buf); out.writeString("# service=" + svc + "\n"); out.end(); advertise(req, db, new PacketLineOutRefAdvertiser(out)); - rsp.setContentType("application/x-" + svc + "-advertisement"); - send(buf.toByteArray(), req, rsp); + buf.close(); } catch (ServiceNotAuthorizedException e) { rsp.sendError(SC_UNAUTHORIZED); diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java index dc874fa5f..8de5f0678 100644 --- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java +++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java @@ -106,7 +106,10 @@ public void doPost(final HttpServletRequest req, final UploadPack up = uploadPackFactory.create(req, db); up.setBiDirectionalPipe(false); rsp.setContentType(RSP_TYPE); - up.upload(getInputStream(req), rsp.getOutputStream(), null); + + final SmartOutputStream out = new SmartOutputStream(req, rsp); + up.upload(getInputStream(req), out, null); + out.close(); } catch (ServiceNotAuthorizedException e) { rsp.reset(); diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java index fd78bb451..f7b3bdb20 100644 --- a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java +++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2010, Google Inc. + * Copyright (C) 2010, Google Inc. * and other copyright owners as documented in the project's IP log. * * This program and the accompanying materials are made available @@ -253,8 +253,6 @@ public void testInitialClone_Small() throws Exception { assertEquals(200, service.getStatus()); assertEquals("application/x-git-upload-pack-result", service .getResponseHeader(HDR_CONTENT_TYPE)); - assertNull("no compression (never compressed)", service - .getResponseHeader(HDR_CONTENT_ENCODING)); } public void testFetchUpdateExisting() throws Exception { From 89cdc3b713c214a8f7142ef0d0df714027ad9876 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Fri, 12 Feb 2010 07:00:32 -0800 Subject: [PATCH 14/14] Reuse the line buffer between strings in PacketLineIn When reading pkt-lines off an InputStream we are quite likely to consume a whole group of fairly short lines in rapid succession, such as in the have exchange that occurs in the fetch-pack/upload-pack protocol. Rather than allocating a throwaway buffer for each line's raw byte sequence, reuse a buffer that is equal to the small side-band packet size, which is 1000 bytes. Text based pkt-lines are required to be less than this size because many widely deployed versions of C Git use a statically allocated array of this length. Change-Id: Ia5c8e95b85020f7f80b6d269dda5059b092d274d Signed-off-by: Shawn O. Pearce --- .../eclipse/jgit/transport/PacketLineIn.java | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java index 1022eb2ee..170e4ddbe 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008-2009, Google Inc. + * Copyright (C) 2008-2010, Google Inc. * Copyright (C) 2008-2009, Robin Rosenberg * Copyright (C) 2008, Shawn O. Pearce * and other copyright owners as documented in the project's IP log. @@ -72,11 +72,11 @@ static enum AckNackResult { private final InputStream in; - private final byte[] lenbuffer; + private final byte[] lineBuffer; PacketLineIn(final InputStream i) { in = i; - lenbuffer = new byte[4]; + lineBuffer = new byte[SideBandOutputStream.SMALL_BUF]; } AckNackResult readACK(final MutableObjectId returnedId) throws IOException { @@ -124,22 +124,27 @@ String readStringRaw() throws IOException { len -= 4; // length header (4 bytes) - final byte[] raw = new byte[len]; + byte[] raw; + if (len <= lineBuffer.length) + raw = lineBuffer; + else + raw = new byte[len]; + IO.readFully(in, raw, 0, len); return RawParseUtils.decode(Constants.CHARSET, raw, 0, len); } int readLength() throws IOException { - IO.readFully(in, lenbuffer, 0, 4); + IO.readFully(in, lineBuffer, 0, 4); try { - final int len = RawParseUtils.parseHexInt16(lenbuffer, 0); + final int len = RawParseUtils.parseHexInt16(lineBuffer, 0); if (len != 0 && len < 4) throw new ArrayIndexOutOfBoundsException(); return len; } catch (ArrayIndexOutOfBoundsException err) { throw new IOException("Invalid packet line header: " - + (char) lenbuffer[0] + (char) lenbuffer[1] - + (char) lenbuffer[2] + (char) lenbuffer[3]); + + (char) lineBuffer[0] + (char) lineBuffer[1] + + (char) lineBuffer[2] + (char) lineBuffer[3]); } } }