[JGIT PATCH 0/4] Branch deletion testing and bug fixes

[JGIT PATCH 0/4] Branch deletion testing and bug fixes

[Charles O'Farrell]

Added a few tests for the RefUpdate delete method, which in turn
exposed two minor bugs. Previously you couldn't delete a ref
pointing to the current branch. Fixing that would also then allow
the current branch to be deleted too.

*** BLURB HERE ***

Charles O'Farrell (4):
  Ensured that RefUpdate cannot delete current branch
  Fixed bug where RefUpdate didn't delete identical HEAD branch
  Added test for RefUpdate branch deletion
  Added extra javadoc for delete to significant RefUpdate results

 .../tst/org/spearce/jgit/lib/RefUpdateTest.java    |  106 ++++++++++++++++++++
 .../src/org/spearce/jgit/lib/RefUpdate.java        |   30 ++++--
 2 files changed, 127 insertions(+), 9 deletions(-)
 create mode 100644 org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java

[JGIT PATCH 1/4] Ensured that RefUpdate cannot delete current branch

[Charles O'Farrell]

If attempted it will return a REJECTED_CURRENT_BRANCH Result.

Signed-off-by: Charles O'Farrell <charleso@charleso.org>
---
 .../src/org/spearce/jgit/lib/RefUpdate.java        |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
index ca77b75..aa2cecb 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
@@ -110,6 +110,13 @@
 		REJECTED,
 
 		/**
+		 * Rejected because trying to delete the current branch.
+		 * <p>
+		 * Has no meaning for update.
+		 */
+		REJECTED_CURRENT_BRANCH,
+
+		/**
 		 * The ref was probably not updated because of I/O error.
 		 * <p>
 		 * Unexpected I/O error occurred when writing new ref. Such error may
@@ -323,6 +330,9 @@ public Result update(final RevWalk walk) throws IOException {
 	 * @throws IOException
 	 */
 	public Result delete() throws IOException {
+		if (name.substring(Constants.R_HEADS.length()).equals(
+				db.getRepository().getBranch()))
+			return Result.REJECTED_CURRENT_BRANCH;
 		try {
 			return updateImpl(new RevWalk(db.getRepository()),
 					new DeleteStore());
-- 
1.6.0.49.gea35

[JGIT PATCH 2/4] Fixed bug where RefUpdate didn't delete identical HEAD branch

[Charles O'Farrell]

Signed-off-by: Charles O'Farrell <charleso@charleso.org>
---
 .../src/org/spearce/jgit/lib/RefUpdate.java        |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
index aa2cecb..34a784b 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
@@ -359,7 +359,7 @@ private Result updateImpl(final RevWalk walk, final Store store)
 			newObj = safeParse(walk, newValue);
 			oldObj = safeParse(walk, oldValue);
 			if (newObj == oldObj)
-				return Result.NO_CHANGE;
+				return store.store(lock, Result.NO_CHANGE);
 
 			if (newObj instanceof RevCommit && oldObj instanceof RevCommit) {
 				if (walk.isMergedInto((RevCommit) oldObj, (RevCommit) newObj))
@@ -390,6 +390,8 @@ private static RevObject safeParse(final RevWalk rw, final AnyObjectId id)
 
 	private Result updateStore(final LockFile lock, final Result status)
 			throws IOException {
+		if (status == Result.NO_CHANGE)
+			return status;
 		lock.setNeedStatInformation(true);
 		lock.write(newValue);
 		String msg = getRefLogMessage();
-- 
1.6.0.49.gea35

[JGIT PATCH 3/4] Added test for RefUpdate branch deletion

[Charles O'Farrell]

Signed-off-by: Charles O'Farrell <charleso@charleso.org>
---
 .../tst/org/spearce/jgit/lib/RefUpdateTest.java    |  106 ++++++++++++++++++++
 1 files changed, 106 insertions(+), 0 deletions(-)
 create mode 100644 org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java
new file mode 100644
index 0000000..62a60c3
--- /dev/null
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java
@@ -0,0 +1,106 @@
+/*
+ * Copyright (C) 2008, Charles O'Farrell <charleso@charleso.org>
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Git Development Community nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+package org.spearce.jgit.lib;
+
+import java.io.File;
+import java.io.IOException;
+
+import org.spearce.jgit.lib.RefUpdate.Result;
+
+public class RefUpdateTest extends RepositoryTestCase {
+
+	private RefUpdate updateRef(final String name) throws IOException {
+		final RefUpdate ref = db.updateRef(name);
+		ref.setNewObjectId(db.resolve(Constants.HEAD));
+		return ref;
+	}
+
+	private void delete(final RefUpdate ref, final Result expected)
+			throws IOException {
+		delete(ref, expected, true, true);
+	}
+
+	private void delete(final RefUpdate ref, final Result expected,
+			final boolean exists, final boolean removed) throws IOException {
+		assertEquals(exists, db.getAllRefs().containsKey(ref.getName()));
+		assertEquals(expected, ref.delete());
+		assertEquals(!removed, db.getAllRefs().containsKey(ref.getName()));
+	}
+
+	public void testLooseDelete() throws IOException {
+		final String newRef = "refs/heads/abc";
+		RefUpdate ref = updateRef(newRef);
+		ref.update(); // create loose ref
+		ref = updateRef(newRef); // refresh
+		delete(ref, Result.NO_CHANGE);
+	}
+
+	public void testDeleteHead() throws IOException {
+		final RefUpdate ref = updateRef(Constants.HEAD);
+		delete(ref, Result.NOT_ATTEMPTED, true, false);
+	}
+
+	public void testLogDeleted() throws IOException {
+		final File log = new File(db.getDirectory(), Constants.LOGS
+				+ "/refs/heads/a");
+		log.getParentFile().mkdirs();
+		log.createNewFile();
+		assertTrue(log.exists());
+		final RefUpdate ref = updateRef("refs/heads/a");
+		delete(ref, Result.FAST_FORWARD);
+		assertFalse(log.exists());
+	}
+
+	public void testDeleteNotFound() throws IOException {
+		final RefUpdate ref = updateRef("refs/heads/xyz");
+		delete(ref, Result.NEW, false, true);
+	}
+
+	public void testDeleteFastForward() throws IOException {
+		final RefUpdate ref = updateRef("refs/heads/a");
+		delete(ref, Result.FAST_FORWARD);
+	}
+
+	public void testDeleteForce() throws IOException {
+		final RefUpdate ref = db.updateRef("refs/heads/b");
+		ref.setNewObjectId(db.resolve("refs/heads/a"));
+		delete(ref, Result.REJECTED, true, false);
+		ref.setForceUpdate(true);
+		delete(ref, Result.FORCED);
+	}
+}
-- 
1.6.0.49.gea35

[JGIT PATCH 4/4] Added extra javadoc for delete to significant RefUpdate results

[Charles O'Farrell]

Signed-off-by: Charles O'Farrell <charleso@charleso.org>
---
 .../src/org/spearce/jgit/lib/RefUpdate.java        |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
index 34a784b..525bb62 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
@@ -52,11 +52,11 @@
 public class RefUpdate {
 	/** Status of an update request. */
 	public static enum Result {
-		/** The ref update has not been attempted by the caller. */
+		/** The ref update/delete has not been attempted by the caller. */
 		NOT_ATTEMPTED,
 
 		/**
-		 * The ref could not be locked for update.
+		 * The ref could not be locked for update/delete.
 		 * <p>
 		 * This is generally a transient failure and is usually caused by
 		 * another process trying to access the ref at the same time as this
@@ -69,12 +69,12 @@
 		 * Same value already stored.
 		 * <p>
 		 * Both the old value and the new value are identical. No change was
-		 * necessary.
+		 * necessary for an update. For delete the branch is removed.
 		 */
 		NO_CHANGE,
 
 		/**
-		 * The ref was created locally.
+		 * The ref was created locally for an update, but ignored for delete.
 		 * <p>
 		 * The ref did not exist when the update started, but it was created
 		 * successfully with the new value.
@@ -82,7 +82,7 @@
 		NEW,
 
 		/**
-		 * The ref had to be forcefully updated.
+		 * The ref had to be forcefully updated/deleted.
 		 * <p>
 		 * The ref already existed but its old value was not fully merged into
 		 * the new value. The configuration permitted a forced update to take
@@ -92,7 +92,7 @@
 		FORCED,
 
 		/**
-		 * The ref was updated in a fast-forward way.
+		 * The ref was updated/deleted in a fast-forward way.
 		 * <p>
 		 * The tracking ref already existed and its old value was fully merged
 		 * into the new value. No history was made unreachable.
@@ -104,7 +104,7 @@
 		 * <p>
 		 * The tracking ref already existed but its old value was not fully
 		 * merged into the new value. The configuration did not allow a forced
-		 * update to take place, so ref still contains the old value. No
+		 * update/delete to take place, so ref still contains the old value. No
 		 * previous history was lost.
 		 */
 		REJECTED,
@@ -117,7 +117,7 @@
 		REJECTED_CURRENT_BRANCH,
 
 		/**
-		 * The ref was probably not updated because of I/O error.
+		 * The ref was probably not updated/deleted because of I/O error.
 		 * <p>
 		 * Unexpected I/O error occurred when writing new ref. Such error may
 		 * result in uncertain state, but most probably ref was not updated.
-- 
1.6.0.49.gea35

Re: [JGIT PATCH 1/4] Ensured that RefUpdate cannot delete current branch

[Shawn O. Pearce]

Charles O'Farrell <charleso@charleso.org> wrote:
> If attempted it will return a REJECTED_CURRENT_BRANCH Result.
 
> @@ -323,6 +330,9 @@ public Result update(final RevWalk walk) throws IOException {
>  	 * @throws IOException
>  	 */
>  	public Result delete() throws IOException {
> +		if (name.substring(Constants.R_HEADS.length()).equals(
> +				db.getRepository().getBranch()))
> +			return Result.REJECTED_CURRENT_BRANCH;
>  		try {
>  			return updateImpl(new RevWalk(db.getRepository()),
>  					new DeleteStore());

I'm squashing this into the patch, as I think its a safer (and
faster) way to evaluate what the current branch is.  We have a
cache in RefDatabase showing the current value of HEAD.  We also
don't look at .git/head-name, which happens during a bisection.

But I also don't think we want to mess around with this test if
we are dealing with refs/remotes or refs/tags.  Really it is only
refs/heads/ that should typically appear in HEAD, so we only need
to guard against that case.  If the user knows enough to make HEAD
point at something else, maybe they will also know enough to not
delete the damn thing out from under themselves.  If we did fix it
to test for all refs, its just a matter of removing the first if.

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
index aa2cecb..77dada0 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
@@ -330,9 +330,12 @@ public Result update(final RevWalk walk) throws IOException {
 	 * @throws IOException
 	 */
 	public Result delete() throws IOException {
-		if (name.substring(Constants.R_HEADS.length()).equals(
-				db.getRepository().getBranch()))
-			return Result.REJECTED_CURRENT_BRANCH;
+		if (name.startsWith(Constants.R_HEADS)) {
+			final Ref head = db.readRef(Constants.HEAD);
+			if (head != null && name.equals(head.getName()))
+				return Result.REJECTED_CURRENT_BRANCH;
+		}
+
 		try {
 			return updateImpl(new RevWalk(db.getRepository()),
 					new DeleteStore());

-- 
Shawn.

Re: [JGIT PATCH 3/4] Added test for RefUpdate branch deletion

[Shawn O. Pearce]

Charles O'Farrell <charleso@charleso.org> wrote:
> +public class RefUpdateTest extends RepositoryTestCase {
> +
> +	public void testDeleteHead() throws IOException {
> +		final RefUpdate ref = updateRef(Constants.HEAD);
> +		delete(ref, Result.NOT_ATTEMPTED, true, false);
> +	}
> +}

So I see you didn't run the unit tests before emailing.  ;-)

I squashed this into the patch I am replying to:

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java
index 62a60c3..1ade2ef 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java
@@ -72,7 +72,7 @@ public void testLooseDelete() throws IOException {
 
 	public void testDeleteHead() throws IOException {
 		final RefUpdate ref = updateRef(Constants.HEAD);
-		delete(ref, Result.NOT_ATTEMPTED, true, false);
+		delete(ref, Result.REJECTED_CURRENT_BRANCH, true, false);
 	}
 
 	public void testLogDeleted() throws IOException {

Otherwise the series looks good.  Compiles.  Passes tests.
Tests look correct.  Thanks!

-- 
Shawn.

Re: [JGIT PATCH 3/4] Added test for RefUpdate branch deletion

[Robin Rosenberg]

lördagen den 23 augusti 2008 00.45.46 skrev Charles O'Farrell:

Having unit tests is lovely and we appreciate that very much. We need more of this. 
Unfortunately it breaks. Is the fix the obvious one?

-- robin

> +	public void testDeleteHead() throws IOException {
> +		final RefUpdate ref = updateRef(Constants.HEAD);
> +		delete(ref, Result.NOT_ATTEMPTED, true, false);

junit.framework.AssertionFailedError: expected:<NOT_ATTEMPTED> but was:<REJECTED_CURRENT_BRANCH>
	at junit.framework.Assert.fail(Assert.java:47)
	at junit.framework.Assert.failNotEquals(Assert.java:280)
	at junit.framework.Assert.assertEquals(Assert.java:64)
	at junit.framework.Assert.assertEquals(Assert.java:71)
	at org.spearce.jgit.lib.RefUpdateTest.delete(RefUpdateTest.java:61)
	at org.spearce.jgit.lib.RefUpdateTest.testDeleteHead(RefUpdateTest.java:75)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:623)
	at junit.framework.TestCase.runTest(TestCase.java:164)
	at junit.framework.TestCase.runBare(TestCase.java:130)
	at junit.framework.TestResult$1.protect(TestResult.java:106)
	at junit.framework.TestResult.runProtected(TestResult.java:124)
	at junit.framework.TestResult.run(TestResult.java:109)
	at junit.framework.TestCase.run(TestCase.java:120)
	at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:460)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:673)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:386)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:196)

Re: [JGIT PATCH 3/4] Added test for RefUpdate branch deletion

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> lördagen den 23 augusti 2008 00.45.46 skrev Charles O'Farrell:
> 
> Having unit tests is lovely and we appreciate that very much. We need more of this. 
> Unfortunately it breaks. Is the fix the obvious one?
> 
> > +	public void testDeleteHead() throws IOException {
> > +		final RefUpdate ref = updateRef(Constants.HEAD);
> > +		delete(ref, Result.NOT_ATTEMPTED, true, false);
> 
> junit.framework.AssertionFailedError: expected:<NOT_ATTEMPTED> but was:<REJECTED_CURRENT_BRANCH>

Yes.  See my reply.  Its already on repo.or.cz with the necessary
squashed patch.

-- 
Shawn.