Re: [EGIT PATCH 08/10] Add ref rename support to JGit

["Shawn O. Pearce" <spearce@spearce.org>]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> Now refs can be renamed. The intent is that should be safe. Only the named
> refs and associated logs are updated. Any symbolic refs referring to the renames
> branches are unaffected, except HEAD, which is updated if the branch it refers
> to is being renamed.
...
> +//	public void testRenameBranchCannotLockAFileHEADisToLockHead() throws IOException {
> +//		an example following the rename failure pattern, but this one is ok!
> +//		tryRenameWhenLocked("HEAD", "refs/heads/b", "refs/heads/new/name",
> +//				"refs/heads/new/name");
> +//	}

Commented out test case?  Eh?

> +//	public void testRenameBranchCannotLockAFileHEADisOtherLockHead() throws IOException {
> +//		an example following the rename failure pattern, but this one is ok!
> +//		tryRenameWhenLocked("HEAD", "refs/heads/b", "refs/heads/new/name",
> +//				"refs/heads/a");
> +//	}

Ditto.

> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
...
> +	RefRename newRename(String fromRef, String toRef) throws IOException {
> +		refreshPackedRefs();
> +		Ref f = readRefBasic(fromRef, 0);
> +		Ref t = readRefBasic(toRef, 0);
> +		if (t != null)
> +			throw new IOException("Ref rename target exists: " + t.getName());

NAK, I'd prefer to have the RefRename return successfully,
but when we try to execute it, it has a result that fails with
RefUpdate.REJECTED.

> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefLogWriter.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefLogWriter.java
...
> +	static void renameTo(final Repository db, final RefUpdate from,
> +			final RefUpdate to) throws IOException {
> +		final File logdir = new File(db.getDirectory(), Constants.LOGS);
> +		final File reflogFrom = new File(logdir, from.getName());
> +		if (!reflogFrom.exists())
> +			return;
> +		final File reflogTo = new File(logdir, to.getName());
> +		final File reflogToDir = reflogTo.getParentFile();
> +		File tmp = new File(logdir,"tmp-renamed-log");
> +		tmp.delete(); // if any exists

That's not thread safe.  Most of the rest of JGit is actually
thread safe.  I think we should be here too.  We should use
a temporary file name inside logdir, one that isn't a legal
ref name at all.  Maybe:

  File tmp = File.createTempFile("tmp..renamed", "-log", logdir);
  if (!reflogFrom.renameTo(tmp)) {
    tmp.delete();
    if (!reflogFrom.renameTo(tmp))
      throw new IOException....
  }

> +		if (!reflogToDir.exists() && !reflogToDir.mkdirs()) {
> +			throw new IOException("Cannot create directory " + reflogToDir);
> +		}
> +		if (!tmp.renameTo(reflogTo)) {
> +			throw new IOException("Cannot rename (" + tmp + ")" + reflogFrom
> +					+ " to " + reflogTo);
> +		}

If we fail here, should we attempt to put the tmp log file back
into the old name?  Or leave it orphaned under the tmp file?

> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefRename.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefRename.java
...
> +	public Result rename() throws IOException {
> +		Ref oldRef = oldFromDelete.db.readRef(Constants.HEAD);
> +		boolean renameHEADtoo = oldRef != null
> +				&& oldRef.getName().equals(oldFromDelete.getName());
> +		try {
> +			Repository db = oldFromDelete.getRepository();

Style nit: Hoist to top of method, so oldRef init can use it.

> +			RefLogWriter.renameTo(db, oldFromDelete,
> +					newToUpdate);

Style nit: This fits on one line.

> +			newToUpdate.setRefLogMessage(null, false);
> +			RefUpdate tmpUpdateRef = oldFromDelete.db.getRepository().updateRef(
> +					"RENAMED-REF");

Like the log, this isn't thread safe.

> +			if (renameHEADtoo) {
> +				try {
> +					oldFromDelete.db.link(Constants.HEAD, "RENAMED-REF");
> +				} catch (IOException e) {
> +					RefLogWriter.renameTo(db,
> +							newToUpdate, oldFromDelete);
> +					return renameResult = Result.LOCK_FAILURE;
> +				}
> +			}
> +			tmpUpdateRef.setNewObjectId(oldFromDelete.getOldObjectId());

We should also do:

  oldFromDelete.setExpectedOldObjectId(oldFromDelete.getOldObjectId())

so that if the old ref was modified between the time we read it
and the time we delete it, we fail on the delete, and don't whack
a ref that was modified by another thread.

> +			RefLogWriter.append(this, "jgit branch: renamed "

I think this should just be "renamed: $old to $new".

> +					+ db.shortenRefName(oldFromDelete.getName()) + " to "
> +					+ db.shortenRefName(newToUpdate.getName()));
> +			return renameResult = Result.RENAMED;
> +		} catch (RuntimeException e) {
> +			throw e;

What's the point of this catch block?

> +		} catch (IOException e) {
> +			System.err.println(e);

Please don't print to System.err from such a low level method.

> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
...
> +		/**
> +		 * The ref was renamed from another name
> +		 * <p>
> +		 */
> +		RENAMED

Is a new state really necessary?  Can't we use NEW on success?
After all, the dst is now created.

>  	}
>  
>  	/** Repository the ref is stored in. */
> -	private final RefDatabase db;
> +	final RefDatabase db;

I wonder if this shouldn't just be a public accessor method; we expose
getRepository() on other application visible objects like RevWalk.
  
>  	/** Location of the loose file holding the value of this ref. */
> -	private final File looseFile;
> +	final File looseFile;

I don't see this used outside of the class, is it still necessary?

>  	/** Result of the update operation. */
> -	private Result result = Result.NOT_ATTEMPTED;
> +	Result result = Result.NOT_ATTEMPTED;

Ditto.
  
> +	static void deleteEmptyDir(File dir, int depth) {
> +		for (; depth > 0 && dir != null; depth--) {
> +			if (dir.exists() && !dir.delete())

Why bother with a stat() call, just to do an rmdir()?

> -	private static int count(final String s, final char c) {
> +	static int count(final String s, final char c) {

Since all callers pass '/', maybe we should just change this to
use '/' as a constant in the loop and remove the char c parameter?
I think that would fix the nasty line wrapping in RefLogWriter too.

-- 
Shawn.