Robin Rosenberg <robin.rosenberg@xxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html