threads / patch / 15174

patch, 4 partsBranch deletion testing and bug fixes

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

## tl;dr

9 messages between Aug 22, 2008 and Aug 22, 2008. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Charles O'Farrell· Aug 22, 2008, 22:45 UTC · lore

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
Charles O'Farrell· Aug 22, 2008, 22:45 UTC · re: Charles O'Farrell · lore

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

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(-)
Show changes to org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java +10 −0
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
Charles O'Farrell· Aug 22, 2008, 22:45 UTC · re: Charles O'Farrell · lore

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

Signed-off-by: Charles O'Farrell <charleso@charleso.org>
---
 .../src/org/spearce/jgit/lib/RefUpdate.java        |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Show changes to org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java +3 −1
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
Charles O'Farrell· Aug 22, 2008, 22:45 UTC · re: Charles O'Farrell · lore

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

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
Show changes to org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java +106 −0
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
Charles O'Farrell· Aug 22, 2008, 22:45 UTC · re: Charles O'Farrell · lore

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

Signed-off-by: Charles O'Farrell <charleso@charleso.org>
---
 .../src/org/spearce/jgit/lib/RefUpdate.java        |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)
Show changes to org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java +8 −8
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
Shawn O. Pearce· Aug 22, 2008, 23:26 UTC · re: Charles O'Farrell · lore

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

Charles O'Farrell <charleso@charleso.org> wrote:
Show 7 quoted lines
> +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:
Show changes to org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java +1 −1
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.
Robin Rosenberg· Aug 22, 2008, 23:28 UTC · re: Charles O'Farrell · lore

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

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)
Shawn O. Pearce· Aug 22, 2008, 23:31 UTC · re: Robin Rosenberg · lore

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

Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
Show 10 quoted lines
> 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.
Shawn O. Pearce· Aug 22, 2008, 23:22 UTC · re: Charles O'Farrell · lore

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

Charles O'Farrell <charleso@charleso.org> wrote:
> If attempted it will return a REJECTED_CURRENT_BRANCH Result.
Show 10 quoted lines
> @@ -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.

Show changes to org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java +6 −3
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.

← back to recent threads