threads / patch / 14367

patch, 4 partsAvoid deadlock while fetching from local repository

Subject: [JGIT PATCH 1/4] Avoid deadlock while fetching from local repository

## tl;dr

5 messages between Jul 9, 2008 and Jul 9, 2008. Diffs are folded; open one to read it.

replies: 4people: 1as markdown or json

Shawn O. Pearce· Jul 9, 2008, 04:15 UTC · lore

[JGIT PATCH 0/4] Misc. push transport fixes

Few minor bugs related to our push implementation. A deadlock is fixed in the local fetch case and pack generation to include annotated tags is also fixed.

We also now do essentially "git init" on the remote repository if we are creating it over a dumb transport during the first push request made to it.

Shawn O. Pearce (4):
  Avoid deadlock while fetching from local repository
  Fix pushing of annotated tags to actually include the tag object
  Automatically initialize a new dumb repository during push
  Use a singleton for the NullProgressMonitor implementation
 .../org/spearce/jgit/lib/NullProgressMonitor.java  |    3 +
 .../src/org/spearce/jgit/revwalk/ObjectWalk.java   |   13 ++++-
 .../spearce/jgit/transport/BasePackConnection.java |    7 ++-
 .../jgit/transport/BasePackFetchConnection.java    |    1 +
 .../jgit/transport/BasePackPushConnection.java     |    1 +
 .../spearce/jgit/transport/WalkPushConnection.java |   54 ++++++++++++++++++++
 .../src/org/spearce/jgit/util/TemporaryBuffer.java |    2 +-
 7 files changed, 77 insertions(+), 4 deletions(-)
Shawn O. Pearce· Jul 9, 2008, 04:15 UTC · re: Shawn O. Pearce · lore

We cannot send a packet line end command to git-upload-pack after we have sent our want list and obtained a pack back from it. Once the want list has ended git-upload-pack wants no further data sent to it, and attempting to write more may cause us to deadlock as the pipe won't accept the data.

Not sending the packet line end if we don't send a want list is a (minor) protocol error. To avoid these protocol errors (which may display on stderr from git-upload-pack) we still send the end during close if we have not yet sent a want list.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../spearce/jgit/transport/BasePackConnection.java |    7 ++++++-
 .../jgit/transport/BasePackFetchConnection.java    |    1 +
 .../jgit/transport/BasePackPushConnection.java     |    1 +
 3 files changed, 8 insertions(+), 1 deletions(-)
Show changes to 3 files +8 −1

org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java, org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java, org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java

diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
index a878f01..7dc4620 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
@@ -83,6 +83,9 @@ abstract class BasePackConnection extends BaseConnection {
 	/** Packet line encoder around {@link #out}. */
 	protected PacketLineOut pckOut;
 
+	/** Send {@link PacketLineOut#end()} before closing {@link #out}? */
+	protected boolean outNeedsEnd;
+
 	/** Capability tokens advertised by the remote side. */
 	private final Set<String> remoteCapablities = new HashSet<String>();
 
@@ -99,6 +102,7 @@ abstract class BasePackConnection extends BaseConnection {
 
 		pckIn = new PacketLineIn(in);
 		pckOut = new PacketLineOut(out);
+		outNeedsEnd = true;
 	}
 
 	protected void readAdvertisedRefs() throws TransportException {
@@ -195,7 +199,8 @@ abstract class BasePackConnection extends BaseConnection {
 	public void close() {
 		if (out != null) {
 			try {
-				pckOut.end();
+				if (outNeedsEnd)
+					pckOut.end();
 				out.close();
 			} catch (IOException err) {
 				// Ignore any close errors.
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
index 04a91bf..12b36f2 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
@@ -253,6 +253,7 @@ abstract class BasePackFetchConnection extends BasePackConnection implements
 			pckOut.writeString(line.toString());
 		}
 		pckOut.end();
+		outNeedsEnd = false;
 		return !first;
 	}
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java
index 784a578..6d95eaf 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java
@@ -148,6 +148,7 @@ class BasePackPushConnection extends BasePackConnection implements
 		if (monitor.isCancelled())
 			throw new TransportException(uri, "push cancelled");
 		pckOut.end();
+		outNeedsEnd = false;
 	}
 
 	private String enableCapabilties() {
-- 
1.5.6.74.g8a5e
Shawn O. Pearce· Jul 9, 2008, 04:15 UTC · re: Shawn O. Pearce · lore

[JGIT PATCH 2/4] Fix pushing of annotated tags to actually include the tag object

When we pushed an annotated tag to a remote repository jgit was failing to include the annotated tag object itself as ObjectWalk nextObject() failed to pop the object out of the internal pending queue and return it for inclusion in the generated pack file.

We need to use two different flags for SEEN and IN_PENDING as a blob or tree object can be SEEN during a TreeWalk, yet may also have been inserted into the pending object list. SEEN means we have output the object to the caller, IN_PENDING means we have placed it into the pending object list. Together these prevent duplicates in the queue and duplicate return values.

Noticed by Robin while trying to push an annotated tag:
  $ git tag -m foo X
  $ git rev-parse X
  49aa0e93621...
  $ jgit push somerepo refs/tags/X
  error: unpack should have generated 49aa0e93621...
  To somerepo.git
   ! [remote rejected] X -> X (bad pack)
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/revwalk/ObjectWalk.java   |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)
Show changes to org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java +11 −2
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java
index 6a5b857..1ba21eb 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java
@@ -66,6 +66,15 @@ import org.spearce.jgit.treewalk.TreeWalk;
  * commits that are returned first.
  */
 public class ObjectWalk extends RevWalk {
+	/**
+	 * Indicates a non-RevCommit is in {@link #pendingObjects}.
+	 * <p>
+	 * We can safely reuse {@link RevWalk#REWRITE} here for the same value as it
+	 * is only set on RevCommit and {@link #pendingObjects} never has RevCommit
+	 * instances inserted into it.
+	 */
+	private static final int IN_PENDING = RevWalk.REWRITE;
+
 	private final TreeWalk treeWalk;
 
 	private BlockObjQueue pendingObjects;
@@ -361,8 +370,8 @@ public class ObjectWalk extends RevWalk {
 	}
 
 	private void addObject(final RevObject o) {
-		if ((o.flags & SEEN) == 0) {
-			o.flags |= SEEN;
+		if ((o.flags & IN_PENDING) == 0) {
+			o.flags |= IN_PENDING;
 			pendingObjects.add(o);
 		}
 	}
-- 
1.5.6.74.g8a5e
Shawn O. Pearce· Jul 9, 2008, 04:15 UTC · re: Shawn O. Pearce · lore

[JGIT PATCH 3/4] Automatically initialize a new dumb repository during push

If we are pushing into a repository which has no refs and no packs we can reasonably assume the repository is brand new. In such cases we must ensure there is a valid HEAD and config file in the repository, otherwise Git clients reading the repository may not be able to recognize its contents correctly.

By default we try to create HEAD from refs/heads/master, if that is among the branches being pushed by the user. This mirrors what git-init would have initialized HEAD to be. However if master was not among the branches pushed then another branch is selected as a reasonable default.

We initialize .git/config to a very bare configuration, listing only the repository version. We avoid setting up core.filemode as we cannot genrally detect if the remote side understands executable mode bits.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../spearce/jgit/transport/WalkPushConnection.java |   54 ++++++++++++++++++++
 1 files changed, 54 insertions(+), 0 deletions(-)
Show changes to org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java +54 −0
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java
index e11b85a..bb5a653 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java
@@ -48,6 +48,7 @@ import java.util.TreeMap;
 
 import org.spearce.jgit.errors.TransportException;
 import org.spearce.jgit.lib.AnyObjectId;
+import org.spearce.jgit.lib.Constants;
 import org.spearce.jgit.lib.ObjectId;
 import org.spearce.jgit.lib.PackWriter;
 import org.spearce.jgit.lib.ProgressMonitor;
@@ -151,6 +152,12 @@ class WalkPushConnection extends BaseConnection implements PushConnection {
 		for (final RemoteRefUpdate u : updates)
 			updateCommand(u);
 
+		// Is this a new repository? If so we should create additional
+		// metadata files so it is properly initialized during the push.
+		//
+		if (!updates.isEmpty() && isNewRepository())
+			createNewRepository(updates);
+
 		if (!packedRefUpdates.isEmpty()) {
 			try {
 				dest.writePackedRefs(newRefs.values());
@@ -307,4 +314,51 @@ class WalkPushConnection extends BaseConnection implements PushConnection {
 			u.setMessage(e.getMessage());
 		}
 	}
+
+	private boolean isNewRepository() {
+		return getRefsMap().isEmpty() && packNames != null
+				&& packNames.isEmpty();
+	}
+
+	private void createNewRepository(final List<RemoteRefUpdate> updates)
+			throws TransportException {
+		try {
+			final String ref = "ref: " + pickHEAD(updates) + "\n";
+			final byte[] bytes = ref.getBytes(Constants.CHARACTER_ENCODING);
+			dest.writeFile("../HEAD", bytes);
+		} catch (IOException e) {
+			throw new TransportException(uri, "cannot create HEAD", e);
+		}
+
+		try {
+			final String config = "[core]\n"
+					+ "\trepositoryformatversion = 0\n";
+			final byte[] bytes = config.getBytes(Constants.CHARACTER_ENCODING);
+			dest.writeFile("../config", bytes);
+		} catch (IOException e) {
+			throw new TransportException(uri, "cannot create config", e);
+		}
+	}
+
+	private static String pickHEAD(final List<RemoteRefUpdate> updates) {
+		// Try to use master if the user is pushing that, it is the
+		// default branch and is likely what they want to remain as
+		// the default on the new remote.
+		//
+		for (final RemoteRefUpdate u : updates) {
+			final String n = u.getRemoteName();
+			if (n.equals(Constants.HEADS_PREFIX + "/" + Constants.MASTER))
+				return n;
+		}
+
+		// Pick any branch, under the assumption the user pushed only
+		// one to the remote side.
+		//
+		for (final RemoteRefUpdate u : updates) {
+			final String n = u.getRemoteName();
+			if (n.startsWith(Constants.HEADS_PREFIX + "/"))
+				return n;
+		}
+		return updates.get(0).getRemoteName();
+	}
 }
-- 
1.5.6.74.g8a5e
Shawn O. Pearce· Jul 9, 2008, 04:15 UTC · re: Shawn O. Pearce · lore

[JGIT PATCH 4/4] Use a singleton for the NullProgressMonitor implementation

No reason to create a new instance every time we need to shove a null monitor into a variable because someone passed us null.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../org/spearce/jgit/lib/NullProgressMonitor.java  |    3 +++
 .../src/org/spearce/jgit/util/TemporaryBuffer.java |    2 +-
 2 files changed, 4 insertions(+), 1 deletions(-)
Show changes to 2 files +4 −1

org.spearce.jgit/src/org/spearce/jgit/lib/NullProgressMonitor.java, org.spearce.jgit/src/org/spearce/jgit/util/TemporaryBuffer.java

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/NullProgressMonitor.java b/org.spearce.jgit/src/org/spearce/jgit/lib/NullProgressMonitor.java
index de75b90..191dc00 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/NullProgressMonitor.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/NullProgressMonitor.java
@@ -42,6 +42,9 @@ package org.spearce.jgit.lib;
  * A NullProgressMonitor does not report progress anywhere.
  */
 public class NullProgressMonitor implements ProgressMonitor {
+	/** Immutable instance of a null progress monitor. */
+	public static final NullProgressMonitor INSTANCE = new NullProgressMonitor();
+
 	public void start(int totalTasks) {
 		// Do not report.
 	}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/util/TemporaryBuffer.java b/org.spearce.jgit/src/org/spearce/jgit/util/TemporaryBuffer.java
index 20db580..d597c38 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/util/TemporaryBuffer.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/util/TemporaryBuffer.java
@@ -200,7 +200,7 @@ public class TemporaryBuffer extends OutputStream {
 	public void writeTo(final OutputStream os, ProgressMonitor pm)
 			throws IOException {
 		if (pm == null)
-			pm = new NullProgressMonitor();
+			pm = NullProgressMonitor.INSTANCE;
 		if (blocks != null) {
 			// Everything is in core so we can stream directly to the output.
 			//
-- 
1.5.6.74.g8a5e

← back to recent threads