[JGIT PATCH 1/2] Cleanup Transport.applyConfig to use setter methods more consistently

[JGIT PATCH 1/2] Cleanup Transport.applyConfig to use setter methods more consistently

[Shawn O. Pearce]

This just struck me as odd that we sometimes used a setter method,
and sometimes used direct assignment to the field.  Now we use the
setter method if it is available.  The fetch and push refspecs do
not have setters, so must still be done by direct field assignment.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/transport/Transport.java  |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java b/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
index 1068f50..c36ccdd 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
@@ -568,9 +568,9 @@ public void setRemoveDeletedRefs(final boolean remove) {
 	 */
 	public void applyConfig(final RemoteConfig cfg) {
 		setOptionUploadPack(cfg.getUploadPack());
-		fetch = cfg.getFetchRefSpecs();
+		setOptionReceivePack(cfg.getReceivePack());
 		setTagOpt(cfg.getTagOpt());
-		optionReceivePack = cfg.getReceivePack();
+		fetch = cfg.getFetchRefSpecs();
 		push = cfg.getPushRefSpecs();
 	}
 
-- 
1.6.3.2.416.g04d0

[JGIT PATCH 2/2] Add support for remote.name.pushurl

[Shawn O. Pearce]

In C Git commit 203462347fce Michael J Gruber added support for a
new URL key within a remote block, permitting a different URL to
be used for push than for fetch.  In the commit message he cites
an example where fetch runs over git://, but push uses ssh://,
as the git:// protocol has lower connection setup overheads.

This change complicates the Transport API as now we must know
in advance when the Transport.open() call is made what type of
operation the caller wants to perform, so we know which config
key to honor when constructing the Transport objects.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/pgm/Push.java             |    3 +-
 .../org/spearce/jgit/transport/RemoteConfig.java   |   47 ++++++
 .../src/org/spearce/jgit/transport/Transport.java  |  150 ++++++++++++++++++--
 3 files changed, 190 insertions(+), 10 deletions(-)

diff --git a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java
index 19d31a1..9364e4a 100644
--- a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java
+++ b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java
@@ -102,7 +102,8 @@ protected void run() throws Exception {
 				refSpecs.add(spec.setForceUpdate(true));
 		}
 
-		final List<Transport> transports = Transport.openAll(db, remote);
+		final List<Transport> transports;
+		transports = Transport.openAll(db, remote, Transport.Operation.PUSH);
 		for (final Transport transport : transports) {
 			transport.setPushThin(thin);
 			if (receivePack != null)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java b/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java
index 519a8a5..f7ed2d7 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java
@@ -58,6 +58,8 @@
 
 	private static final String KEY_URL = "url";
 
+	private static final String KEY_PUSHURL = "pushurl";
+
 	private static final String KEY_FETCH = "fetch";
 
 	private static final String KEY_PUSH = "push";
@@ -108,6 +110,8 @@
 
 	private List<URIish> uris;
 
+	private List<URIish> pushURIs;
+
 	private List<RefSpec> fetch;
 
 	private List<RefSpec> push;
@@ -147,6 +151,11 @@ public RemoteConfig(final RepositoryConfig rc, final String remoteName)
 		for (final String s : vlst)
 			uris.add(new URIish(s));
 
+		vlst = rc.getStringList(SECTION, name, KEY_PUSHURL);
+		pushURIs = new ArrayList<URIish>(vlst.length);
+		for (final String s : vlst)
+			pushURIs.add(new URIish(s));
+
 		vlst = rc.getStringList(SECTION, name, KEY_FETCH);
 		fetch = new ArrayList<RefSpec>(vlst.length);
 		for (final String s : vlst)
@@ -187,6 +196,11 @@ public void update(final RepositoryConfig rc) {
 		rc.setStringList(SECTION, getName(), KEY_URL, vlst);
 
 		vlst.clear();
+		for (final URIish u : getPushURIs())
+			vlst.add(u.toPrivateString());
+		rc.setStringList(SECTION, getName(), KEY_PUSHURL, vlst);
+
+		vlst.clear();
 		for (final RefSpec u : getFetchRefSpecs())
 			vlst.add(u.toString());
 		rc.setStringList(SECTION, getName(), KEY_FETCH, vlst);
@@ -265,6 +279,39 @@ public boolean removeURI(final URIish toRemove) {
 	}
 
 	/**
+	 * Get all configured push-only URIs under this remote.
+	 * 
+	 * @return the set of URIs known to this remote.
+	 */
+	public List<URIish> getPushURIs() {
+		return Collections.unmodifiableList(pushURIs);
+	}
+
+	/**
+	 * Add a new push-only URI to the end of the list of URIs.
+	 * 
+	 * @param toAdd
+	 *            the new URI to add to this remote.
+	 * @return true if the URI was added; false if it already exists.
+	 */
+	public boolean addPushURI(final URIish toAdd) {
+		if (pushURIs.contains(toAdd))
+			return false;
+		return pushURIs.add(toAdd);
+	}
+
+	/**
+	 * Remove a push-only URI from the list of URIs.
+	 * 
+	 * @param toRemove
+	 *            the URI to remove from this remote.
+	 * @return true if the URI was added; false if it already exists.
+	 */
+	public boolean removePushURI(final URIish toRemove) {
+		return pushURIs.remove(toRemove);
+	}
+
+	/**
 	 * Remembered specifications for fetching from a repository.
 	 * 
 	 * @return set of specs used by default when fetching.
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java b/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
index c36ccdd..ac4807f 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
@@ -72,8 +72,18 @@
  * Callers must ensure a transport is accessed by only one thread at a time.
  */
 public abstract class Transport {
+	/** Type of operation a Transport is being opened for. */
+	public enum Operation {
+		/** Transport is to fetch objects locally. */
+		FETCH,
+		/** Transport is to push objects remotely. */
+		PUSH;
+	}
+
 	/**
 	 * Open a new transport instance to connect two repositories.
+	 * <p>
+	 * This method assumes {@link Operation#FETCH}.
 	 * 
 	 * @param local
 	 *            existing local repository.
@@ -90,15 +100,41 @@
 	 */
 	public static Transport open(final Repository local, final String remote)
 			throws NotSupportedException, URISyntaxException {
+		return open(local, remote, Operation.FETCH);
+	}
+
+	/**
+	 * Open a new transport instance to connect two repositories.
+	 * 
+	 * @param local
+	 *            existing local repository.
+	 * @param remote
+	 *            location of the remote repository - may be URI or remote
+	 *            configuration name.
+	 * @param op
+	 *            planned use of the returned Transport; the URI may differ
+	 *            based on the type of connection desired.
+	 * @return the new transport instance. Never null. In case of multiple URIs
+	 *         in remote configuration, only the first is chosen.
+	 * @throws URISyntaxException
+	 *             the location is not a remote defined in the configuration
+	 *             file and is not a well-formed URL.
+	 * @throws NotSupportedException
+	 *             the protocol specified is not supported.
+	 */
+	public static Transport open(final Repository local, final String remote,
+			final Operation op) throws NotSupportedException,
+			URISyntaxException {
 		final RemoteConfig cfg = new RemoteConfig(local.getConfig(), remote);
-		final List<URIish> uris = cfg.getURIs();
-		if (uris.size() == 0)
+		if (doesNotExist(cfg))
 			return open(local, new URIish(remote));
-		return open(local, cfg);
+		return open(local, cfg, op);
 	}
 
 	/**
 	 * Open new transport instances to connect two repositories.
+	 * <p>
+	 * This method assumes {@link Operation#FETCH}.
 	 *
 	 * @param local
 	 *            existing local repository.
@@ -116,18 +152,44 @@ public static Transport open(final Repository local, final String remote)
 	public static List<Transport> openAll(final Repository local,
 			final String remote) throws NotSupportedException,
 			URISyntaxException {
+		return openAll(local, remote, Operation.FETCH);
+	}
+
+	/**
+	 * Open new transport instances to connect two repositories.
+	 *
+	 * @param local
+	 *            existing local repository.
+	 * @param remote
+	 *            location of the remote repository - may be URI or remote
+	 *            configuration name.
+	 * @param op
+	 *            planned use of the returned Transport; the URI may differ
+	 *            based on the type of connection desired.
+	 * @return the list of new transport instances for every URI in remote
+	 *         configuration.
+	 * @throws URISyntaxException
+	 *             the location is not a remote defined in the configuration
+	 *             file and is not a well-formed URL.
+	 * @throws NotSupportedException
+	 *             the protocol specified is not supported.
+	 */
+	public static List<Transport> openAll(final Repository local,
+			final String remote, final Operation op)
+			throws NotSupportedException, URISyntaxException {
 		final RemoteConfig cfg = new RemoteConfig(local.getConfig(), remote);
-		final List<URIish> uris = cfg.getURIs();
-		if (uris.size() == 0) {
+		if (doesNotExist(cfg)) {
 			final ArrayList<Transport> transports = new ArrayList<Transport>(1);
 			transports.add(open(local, new URIish(remote)));
 			return transports;
 		}
-		return openAll(local, cfg);
+		return openAll(local, cfg, op);
 	}
 
 	/**
 	 * Open a new transport instance to connect two repositories.
+	 * <p>
+	 * This method assumes {@link Operation#FETCH}.
 	 * 
 	 * @param local
 	 *            existing local repository.
@@ -144,17 +206,45 @@ public static Transport open(final Repository local, final String remote)
 	 */
 	public static Transport open(final Repository local, final RemoteConfig cfg)
 			throws NotSupportedException {
-		if (cfg.getURIs().isEmpty())
+		return open(local, cfg, Operation.FETCH);
+	}
+
+	/**
+	 * Open a new transport instance to connect two repositories.
+	 * 
+	 * @param local
+	 *            existing local repository.
+	 * @param cfg
+	 *            configuration describing how to connect to the remote
+	 *            repository.
+	 * @param op
+	 *            planned use of the returned Transport; the URI may differ
+	 *            based on the type of connection desired.
+	 * @return the new transport instance. Never null. In case of multiple URIs
+	 *         in remote configuration, only the first is chosen.
+	 * @throws NotSupportedException
+	 *             the protocol specified is not supported.
+	 * @throws IllegalArgumentException
+	 *             if provided remote configuration doesn't have any URI
+	 *             associated.
+	 */
+	public static Transport open(final Repository local,
+			final RemoteConfig cfg, final Operation op)
+			throws NotSupportedException {
+		final List<URIish> uris = getURIs(cfg, op);
+		if (uris.isEmpty())
 			throw new IllegalArgumentException(
 					"Remote config \""
 					+ cfg.getName() + "\" has no URIs associated");
-		final Transport tn = open(local, cfg.getURIs().get(0));
+		final Transport tn = open(local, uris.get(0));
 		tn.applyConfig(cfg);
 		return tn;
 	}
 
 	/**
 	 * Open new transport instances to connect two repositories.
+	 * <p>
+	 * This method assumes {@link Operation#FETCH}.
 	 *
 	 * @param local
 	 *            existing local repository.
@@ -168,7 +258,29 @@ public static Transport open(final Repository local, final RemoteConfig cfg)
 	 */
 	public static List<Transport> openAll(final Repository local,
 			final RemoteConfig cfg) throws NotSupportedException {
-		final List<URIish> uris = cfg.getURIs();
+		return openAll(local, cfg, Operation.FETCH);
+	}
+
+	/**
+	 * Open new transport instances to connect two repositories.
+	 *
+	 * @param local
+	 *            existing local repository.
+	 * @param cfg
+	 *            configuration describing how to connect to the remote
+	 *            repository.
+	 * @param op
+	 *            planned use of the returned Transport; the URI may differ
+	 *            based on the type of connection desired.
+	 * @return the list of new transport instances for every URI in remote
+	 *         configuration.
+	 * @throws NotSupportedException
+	 *             the protocol specified is not supported.
+	 */
+	public static List<Transport> openAll(final Repository local,
+			final RemoteConfig cfg, final Operation op)
+			throws NotSupportedException {
+		final List<URIish> uris = getURIs(cfg, op);
 		final List<Transport> transports = new ArrayList<Transport>(uris.size());
 		for (final URIish uri : uris) {
 			final Transport tn = open(local, uri);
@@ -178,6 +290,26 @@ public static Transport open(final Repository local, final RemoteConfig cfg)
 		return transports;
 	}
 
+	private static List<URIish> getURIs(final RemoteConfig cfg,
+			final Operation op) {
+		switch (op) {
+		case FETCH:
+			return cfg.getURIs();
+		case PUSH: {
+			List<URIish> uris = cfg.getPushURIs();
+			if (uris.isEmpty())
+				uris = cfg.getURIs();
+			return uris;
+		}
+		default:
+			throw new IllegalArgumentException(op.toString());
+		}
+	}
+
+	private static boolean doesNotExist(final RemoteConfig cfg) {
+		return cfg.getURIs().isEmpty() && cfg.getPushURIs().isEmpty();
+	}
+
 	/**
 	 * Open a new transport instance to connect two repositories.
 	 * 
-- 
1.6.3.2.416.g04d0

Re: [JGIT PATCH 2/2] Add support for remote.name.pushurl

[Robin Rosenberg]

söndag 21 juni 2009 03:21:56 skrev "Shawn O. Pearce" <spearce@spearce.org>:
> In C Git commit 203462347fce Michael J Gruber added support for a
> new URL key within a remote block, permitting a different URL to
> be used for push than for fetch.  In the commit message he cites
> an example where fetch runs over git://, but push uses ssh://,
> as the git:// protocol has lower connection setup overheads.
> 
> This change complicates the Transport API as now we must know
> in advance when the Transport.open() call is made what type of
> operation the caller wants to perform, so we know which config
> key to honor when constructing the Transport objects.
> 
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> ---
>  .../src/org/spearce/jgit/pgm/Push.java             |    3 +-
>  .../org/spearce/jgit/transport/RemoteConfig.java   |   47 ++++++
>  .../src/org/spearce/jgit/transport/Transport.java  |  150 ++++++++++++++++++--
>  3 files changed, 190 insertions(+), 10 deletions(-)
> 
> diff --git a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java
> index 19d31a1..9364e4a 100644
> --- a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java
> +++ b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java
> @@ -102,7 +102,8 @@ protected void run() throws Exception {
>  				refSpecs.add(spec.setForceUpdate(true));
>  		}
>  
> -		final List<Transport> transports = Transport.openAll(db, remote);
> +		final List<Transport> transports;
> +		transports = Transport.openAll(db, remote, Transport.Operation.PUSH);

Nit-pick. We usually initialize in one statement. I'll squash that for you.

Seems we need to consider the Eclipse UI, since that has only of only one UI. Can we hold on to
that or at least establish a related issue in the bug tracker for Egit.

-- robin

Re: [JGIT PATCH 2/2] Add support for remote.name.pushurl

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> s?ndag 21 juni 2009 03:21:56 skrev "Shawn O. Pearce" <spearce@spearce.org>:
> > diff --git a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java
> > index 19d31a1..9364e4a 100644
> > --- a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java
> > +++ b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java
> > @@ -102,7 +102,8 @@ protected void run() throws Exception {
> >  				refSpecs.add(spec.setForceUpdate(true));
> >  		}
> >  
> > -		final List<Transport> transports = Transport.openAll(db, remote);
> > +		final List<Transport> transports;
> > +		transports = Transport.openAll(db, remote, Transport.Operation.PUSH);
> 
> Nit-pick. We usually initialize in one statement. I'll squash that for you.

Actually I did that because otherwise we'd run over 80 columns on
that line.  If you let Eclipse format the code for you, it line
wraps onto a 2nd line anyway.  In such cases I personally just find
this format more readable.  But whatever.
 
-- 
Shawn.