Re: [EGIT PATCH 2/6] 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.
...
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefLogWriter.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefLogWriter.java
> index a077051..2efa12d 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefLogWriter.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefLogWriter.java
> @@ -58,6 +58,24 @@ static void append(final RefUpdate u, final String msg) throws IOException {
>  		appendOneRecord(oldId, newId, ident, msg, db, u.getName());
>  	}
>  
> +	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 refdirTo = reflogTo.getParentFile();
> +		if (!refdirTo.exists() && !refdirTo.mkdirs()) {
> +			throw new IOException("Cannot create directory " + refdirTo);
> +		}
> +		if (!reflogFrom.renameTo(reflogTo)) {

What happens if from = "refs/heads/a" and to = "refs/heads/a/b" ?

> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefRename.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefRename.java
> new file mode 100644
> index 0000000..4ea9cfa
> --- /dev/null
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefRename.java
> @@ -0,0 +1,101 @@
> +package org.spearce.jgit.lib;

Copyright header?

> +
> +import java.io.File;
> +import java.io.IOException;
> +import java.util.ArrayList;
> +import java.util.List;
> +
> +import org.spearce.jgit.lib.RefUpdate.DeleteStore;
> +import org.spearce.jgit.lib.RefUpdate.Result;
> +import org.spearce.jgit.lib.RefUpdate.UpdateStore;
> +
> +/**
> + * A RefUpdate combination for renaming a ref
> + */
> +public class RefRename {
> +	private RefUpdate newToUpdate;
> +
> +	private RefUpdate oldFromDelete;
> +
> +	private Result renameResult;
> +
> +	RefRename(final RefUpdate toUpdate, final RefUpdate fromUpdate) {
> +		newToUpdate = toUpdate;
> +		oldFromDelete = fromUpdate;
> +	}
> +
> +	/**
> +	 * @return result of rename operation
> +	 */
> +	public Result getResult() {
> +		return renameResult;
> +	}
> +
> +	/**
> +	 * @return the result of the new ref update
> +	 * @throws IOException
> +	 */
> +	public Result rename() throws IOException {
> +		List<LockFile> lockFiles = new ArrayList<LockFile>();
> +		LockFile lockFileFrom = new LockFile(oldFromDelete.looseFile);
> +		LockFile lockFileTo = new LockFile(newToUpdate.looseFile);
> +		LockFile lockFileHEAD = new LockFile(new File(oldFromDelete.db
> +				.getRepository().getDirectory(), Constants.HEAD));
> +		lockFiles.add(lockFileTo);
> +		lockFiles.add(lockFileFrom);
> +		lockFiles.add(lockFileHEAD);
> +		try {
> +			for (LockFile l : lockFiles) {
> +				if (!l.lock()) {
> +					unlock(lockFiles);
> +					return Result.LOCK_FAILURE;
> +				}
> +			}
> +		} catch (RuntimeException e) {
> +			unlock(lockFiles);
> +			throw e;
> +		} catch (IOException e) {
> +			unlock(lockFiles);
> +			throw e;
> +		}
> +		boolean renameHEADtoo = oldFromDelete.db.readRef(Constants.HEAD)
> +				.getName().equals(oldFromDelete.getName());
> +		try {
> +			UpdateStore toStore = newToUpdate.newUpdateStore();
> +			RefLogWriter.renameTo(oldFromDelete.getRepository(), oldFromDelete,
> +					newToUpdate);
> +			newToUpdate.setNewObjectId(oldFromDelete.getOldObjectId());
> +			newToUpdate.setExpectedOldObjectId(oldFromDelete.getOldObjectId());

The dst ref can't expect its current value to be the src ref's
current value, the dst ref shouldn't exist, right?  So I'm not sure
how your code is working with this particular assignment occurring
in it.  Shouldn't the update store be failing over the compare on
expected old id?

I think we always want

  newToUpdate.setExpectedOldObjectId(ObjectId.zeroId())

as the JavaDoc for that method says zeroId should be used if the
ref isn't supposed to exist, which should be the case for the
destination side of a rename.

Ooooh.  I see.  Here UpdateStore is bypassing the CAS check, as
it runs inside of the RefUpdate.update() method, before the Store
instance is constructed.

Post taking the LockFile successfully we need to validate that

  ObjectId.zeroId().equals(db.idOf(newToUpdate.getName()))

is still true, otherwise it means someone else created the
destination ref and we're about to overwrite it.

> +			newToUpdate.setRefLogMessage("jgit branch: renamed "
> +					+ oldFromDelete.getName() + " to "
> +					+ oldFromDelete.getName(), false);

Typo, you meant newToUpdate here in the 2nd case.

-- 
Shawn.