[JGit] Push to new Amazon S3 does not work? ("funny refname")

[JGit] Push to new Amazon S3 does not work? ("funny refname")

[Daniel Cheng]

Pushing to new Amazon S3 repository does not work.
It say "funny refname" without pushing anything:

<<<<<<<<<
$ jgit push s3 master
To amazon-s3://0NQ4APQ8R7S6HQ65TWR2@egitsdiz/1.git
 ! [remote rejected] master -> master (funny refname)
$ s3cmd la
         DIR   s3://egitsdiz/1.git/
$
>>>>>>>>>

Any idea what's happening here?


The code is in WalkPushConnection.java line 137:
<<<<<<<<<
134    final List<RemoteRefUpdate> updates = new ArrayList<RemoteRefUpdate>();
135    for (final RemoteRefUpdate u : refUpdates.values()) {
136        final String n = u.getRemoteName();
137        if (!n.startsWith("refs/") || !Repository.isValidRefName(n)) {
138            u.setStatus(Status.REJECTED_OTHER_REASON);
139            u.setMessage("funny refname");
140            continue;
141        }
>>>>>>>>>

u.getRemoteName() gives "master" here.
Removing  n.startsWith("refs/") would generate a bad `packed-refs`
file in later code.
I tried to fix this, but failed to do so without breaking GitSsh transports

Re: [JGit] Push to new Amazon S3 does not work? ("funny refname")

[Robin Rosenberg]

lördag 07 mars 2009 17:05:02 skrev Daniel Cheng <j16sdiz+freenet@gmail.com>:
> Pushing to new Amazon S3 repository does not work.
> It say "funny refname" without pushing anything:
> 
> <<<<<<<<<
> $ jgit push s3 master
> To amazon-s3://0NQ4APQ8R7S6HQ65TWR2@egitsdiz/1.git
>  ! [remote rejected] master -> master (funny refname)
> $ s3cmd la
>          DIR   s3://egitsdiz/1.git/
> $
> >>>>>>>>>
> 
> Any idea what's happening here?
> 
> 
> The code is in WalkPushConnection.java line 137:
> <<<<<<<<<
> 134    final List<RemoteRefUpdate> updates = new ArrayList<RemoteRefUpdate>();
> 135    for (final RemoteRefUpdate u : refUpdates.values()) {
> 136        final String n = u.getRemoteName();
> 137        if (!n.startsWith("refs/") || !Repository.isValidRefName(n)) {
> 138            u.setStatus(Status.REJECTED_OTHER_REASON);
> 139            u.setMessage("funny refname");
> 140            continue;
> 141        }
> >>>>>>>>>
> 
> u.getRemoteName() gives "master" here.
> Removing  n.startsWith("refs/") would generate a bad `packed-refs`
> file in later code.
> I tried to fix this, but failed to do so without breaking GitSsh transports

This is not specific to s3. It seems jgit wants a fully qualified ref for the remote
side, so refs/heads/master will work for the other protocols, and I guess s3 too.

- robin

Re: [JGit] Push to new Amazon S3 does not work? ("funny refname")

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> lördag 07 mars 2009 17:05:02 skrev Daniel Cheng <j16sdiz+freenet@gmail.com>:
> > Pushing to new Amazon S3 repository does not work.
> > It say "funny refname" without pushing anything:
> > 
> > <<<<<<<<<
> > $ jgit push s3 master
> > To amazon-s3://0NQ4APQ8R7S6HQ65TWR2@egitsdiz/1.git
> >  ! [remote rejected] master -> master (funny refname)
> > $ s3cmd la
> >          DIR   s3://egitsdiz/1.git/
> > $
> > >>>>>>>>>
> 
> This is not specific to s3. It seems jgit wants a fully qualified ref for the remote
> side, so refs/heads/master will work for the other protocols, and I guess s3 too.

Correct.

The "jgit push" command line client lacks the DWIMery of "git push".

Specifically, from a pure API usage perspective, "jgit push"
is responsible for expanding the user input of "master" into the
"refs/heads/master:refs/heads/master" refspec that the lower level
PushProcess class wants.

Here it failed to do that, and the lower-level transport (rightly)
rejected the invalid ref name.

  Side note:

  That API definition that says the client should do the DWIMery
  of ref expansion also makes it nearly impossible to implement
  "push matching" or "randomsha1:master" refspec, as the client
  doesn't have the network connection open and doesn't have the
  advertised ref information early enough.

The reason we punted on this and didn't do this particular
expansion DWIMery in "jgit push" is we lack a good way to resolve
"master" into "refs/heads/master", or "v1.0" into "refs/tags/v1.0".
Repository does not expose the ref lookup algorithm, only resolve(),
which converts "master" into a SHA-1 ObjectId.

If someone exposed this portion of the resolve logic in the
Repository class, I think it would be a fairly simple change
in Push to support this DIWMery.

But until then, you need to say:

  jgit push s3 refs/heads/master:refs/heads/master

or maybe this DWIMery might work:

  jgit push s3 refs/heads/master

Its been a while since I passed args.  I usually have
remote.$name.push in place for things that I push to.

-- 
Shawn.

[EGIT PATCH] Evaluate short refnames into full names during push

[Robin Rosenberg]

With this we can use short names like master instead of refs/heads/master
when pushing. This is slightly more convenient. Pushing a delete still
requires the long format.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../src/org/spearce/jgit/lib/Repository.java       |   11 ++++++++++
 .../src/org/spearce/jgit/transport/Transport.java  |   21 +++++++++++++++++--
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
index 30bd4a3..3ab51b1 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
@@ -928,6 +928,17 @@ public String getBranch() throws IOException {
 	}
 	
 	/**
+	 * Get a ref by name.
+	 * 
+	 * @param name
+	 * @return the Ref with the given name, or null if it does not exist
+	 * @throws IOException
+	 */
+	public Ref getRef(final String name) throws IOException {
+		return refs.readRef(name);
+	}
+
+	/**
 	 * @return all known refs (heads, tags, remotes).
 	 */
 	public Map<String, Ref> getAllRefs() {
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 3aec5ca..64745a8 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
@@ -51,6 +51,7 @@
 
 import org.spearce.jgit.errors.NotSupportedException;
 import org.spearce.jgit.errors.TransportException;
+import org.spearce.jgit.lib.Constants;
 import org.spearce.jgit.lib.NullProgressMonitor;
 import org.spearce.jgit.lib.ProgressMonitor;
 import org.spearce.jgit.lib.Ref;
@@ -243,10 +244,24 @@ else if (TransportLocal.canHandle(remote))
 		final Collection<RefSpec> procRefs = expandPushWildcardsFor(db, specs);
 
 		for (final RefSpec spec : procRefs) {
-			final String srcRef = spec.getSource();
+			String srcRef = spec.getSource();
+			final Ref src = db.getRef(srcRef);
+			if (src != null)
+				srcRef = src.getName();
+			String remoteName = spec.getDestination();
 			// null destination (no-colon in ref-spec) is a special case
-			final String remoteName = (spec.getDestination() == null ? spec
-					.getSource() : spec.getDestination());
+			if (remoteName == null) {
+				remoteName = srcRef; 
+			} else {
+				if (!remoteName.startsWith(Constants.R_REFS)) {
+					// null source is another special case (delete)
+					if (srcRef != null) {
+						// assume the same type of ref at the destination
+						String srcPrefix = srcRef.substring(0, srcRef.indexOf('/', Constants.R_REFS.length()));
+						remoteName = srcPrefix + "/" + remoteName;
+					}
+				}
+			}
 			final boolean forceUpdate = spec.isForceUpdate();
 			final String localName = findTrackingRefName(remoteName, fetchSpecs);
 
-- 
1.6.1.285.g35d8b

Re: [EGIT PATCH] Evaluate short refnames into full names during push

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> @@ -243,10 +244,24 @@ else if (TransportLocal.canHandle(remote))
>  		final Collection<RefSpec> procRefs = expandPushWildcardsFor(db, specs);
>  
>  		for (final RefSpec spec : procRefs) {
> -			final String srcRef = spec.getSource();
> +			String srcRef = spec.getSource();
> +			final Ref src = db.getRef(srcRef);
> +			if (src != null)
> +				srcRef = src.getName();
> +			String remoteName = spec.getDestination();
>  			// null destination (no-colon in ref-spec) is a special case
> -			final String remoteName = (spec.getDestination() == null ? spec
> -					.getSource() : spec.getDestination());

Oh, right.  I forgot about the fact that Marek put the code here, as
then "push = master" in a config file works...

OK.  I'll apply.

-- 
Shawn.

[EGIT PATCH] Prevent an exception if the user tries to push a non-existing ref.

[Robin Rosenberg]

Instead of a StringIndexOutOfBoundsException we now get an error telling
us that the ref could not be resolved.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../src/org/spearce/jgit/transport/Transport.java  |    2 +-
 1 files changed, 1 insertions(+), 1 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 a0a2575..8a25213 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
@@ -255,7 +255,7 @@ else if (TransportLocal.canHandle(remote))
 			} else {
 				if (!remoteName.startsWith(Constants.R_REFS)) {
 					// null source is another special case (delete)
-					if (srcRef != null) {
+					if (src != null) {
 						// assume the same type of ref at the destination
 						String srcPrefix = srcRef.substring(0, srcRef.indexOf('/', Constants.R_REFS.length()));
 						remoteName = srcPrefix + "/" + remoteName;
-- 
1.6.1.285.g35d8b

Re: [EGIT PATCH] Prevent an exception if the user tries to push a non-existing ref.

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> Instead of a StringIndexOutOfBoundsException we now get an error telling
> us that the ref could not be resolved.

*sigh*

> 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 a0a2575..8a25213 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
> @@ -255,7 +255,7 @@ else if (TransportLocal.canHandle(remote))
>  			} else {
>  				if (!remoteName.startsWith(Constants.R_REFS)) {
>  					// null source is another special case (delete)
> -					if (srcRef != null) {
> +					if (src != null) {
>  						// assume the same type of ref at the destination
>  						String srcPrefix = srcRef.substring(0, srcRef.indexOf('/', Constants.R_REFS.length()));
>  						remoteName = srcPrefix + "/" + remoteName;

After reading that code again, I'm tempted to apply this instead.
Its a much larger patch, but I think the result is a lot easier
to follow.

--8<--
Fix DWIMery for push to handle non-existant source refs

Instead of a StringIndexOutOfBoundsException we now get an error
telling us that the ref could not be resolved.

Found-by: Robin Rosenberg <robin.rosenberg@dewire.com>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/transport/Transport.java  |   45 ++++++++++---------
 1 files changed, 24 insertions(+), 21 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 a0a2575..1068f50 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
@@ -244,29 +244,32 @@ else if (TransportLocal.canHandle(remote))
 		final Collection<RefSpec> procRefs = expandPushWildcardsFor(db, specs);
 
 		for (final RefSpec spec : procRefs) {
-			String srcRef = spec.getSource();
-			final Ref src = db.getRef(srcRef);
-			if (src != null)
-				srcRef = src.getName();
-			String remoteName = spec.getDestination();
-			// null destination (no-colon in ref-spec) is a special case
-			if (remoteName == null) {
-				remoteName = srcRef;
-			} else {
-				if (!remoteName.startsWith(Constants.R_REFS)) {
-					// null source is another special case (delete)
-					if (srcRef != null) {
-						// assume the same type of ref at the destination
-						String srcPrefix = srcRef.substring(0, srcRef.indexOf('/', Constants.R_REFS.length()));
-						remoteName = srcPrefix + "/" + remoteName;
-					}
-				}
+			String srcSpec = spec.getSource();
+			final Ref srcRef = db.getRef(srcSpec);
+			if (srcRef != null)
+				srcSpec = srcRef.getName();
+
+			String destSpec = spec.getDestination();
+			if (destSpec == null) {
+				// No destination (no-colon in ref-spec), DWIMery assumes src
+				//
+				destSpec = srcSpec;
 			}
-			final boolean forceUpdate = spec.isForceUpdate();
-			final String localName = findTrackingRefName(remoteName, fetchSpecs);
 
-			final RemoteRefUpdate rru = new RemoteRefUpdate(db, srcRef,
-					remoteName, forceUpdate, localName, null);
+			if (srcRef != null && !destSpec.startsWith(Constants.R_REFS)) {
+				// Assume the same kind of ref at the destination, e.g.
+				// "refs/heads/foo:master", DWIMery assumes master is also
+				// under "refs/heads/".
+				//
+				final String n = srcRef.getName();
+				final int kindEnd = n.indexOf('/', Constants.R_REFS.length());
+				destSpec = n.substring(0, kindEnd + 1) + destSpec;
+			}
+
+			final boolean forceUpdate = spec.isForceUpdate();
+			final String localName = findTrackingRefName(destSpec, fetchSpecs);
+			final RemoteRefUpdate rru = new RemoteRefUpdate(db, srcSpec,
+					destSpec, forceUpdate, localName, null);
 			result.add(rru);
 		}
 		return result;
-- 
1.6.2.185.g8b635


-- 
Shawn.

Re: [EGIT PATCH] Prevent an exception if the user tries to push a non-existing ref.

[Robin Rosenberg]

måndag 09 mars 2009 16:50:49 skrev "Shawn O. Pearce" <spearce@spearce.org>:
> After reading that code again, I'm tempted to apply this instead.
> Its a much larger patch, but I think the result is a lot easier
> to follow.

I wouldn't say "a lot", but a little perhaps.

-- robin