threads / patch / 20993

patchCircular references shouldn't be created

Subject: [PATCH JGIT] Circular references shouldn't be created

## tl;dr

5 messages between Sep 17, 2009 and Sep 18, 2009. Diffs are folded; open one to read it.

replies: 4people: 4as markdown or json

Sohn, Matthias· Sep 17, 2009, 19:23 UTC · lore
From: Matthias Sohn <matthias.sohn@sap.com>
Circular references shouldn't be created
Fix for bug: https://bugs.eclipse.org/bugs/show_bug.cgi?id=286743
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
---
 .../tst/org/spearce/jgit/lib/RefTest.java          |    9 +++++++++
 .../src/org/spearce/jgit/lib/RefDatabase.java      |    4 ++++
 2 files changed, 13 insertions(+), 0 deletions(-)
Show changes to 2 files +13 −0

org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefTest.java, org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefTest.java
index fabbe7e..ce6328b 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefTest.java
@@ -155,4 +155,13 @@ public void testOrigResolvedNamesSymRef() throws IOException {
 		assertEquals("refs/heads/master", ref.getName());
 		assertEquals("HEAD", ref.getOrigName());
 	}
+	
+	public void testIllegalCircularRef() throws IOException {
+		try {
+			db.writeSymref("HEAD", "HEAD");
+			fail("creation of circular reference should fail");
+		} catch (IllegalArgumentException expected) {
+			// attempt to create circular reference should fail
+		}
+	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
index 09cb9bb..483b1d0 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
@@ -174,6 +174,10 @@ RefRename newRename(String fromRef, String toRef) throws IOException {
 	 * @throws IOException
 	 */
 	void link(final String name, final String target) throws IOException {
+		if (name.equals(target))
+			throw new IllegalArgumentException(
+					"illegal circular reference : symref " + name
+							+ " cannot refer to " + target);
 		final byte[] content = Constants.encode("ref: " + target + "\n");
 		lockAndWriteFile(fileForRef(name), content);
 		synchronized (this) {

-- 
1.6.4.msysgit.0
Avery Pennarun· Sep 17, 2009, 21:40 UTC · re: Sohn, Matthias · lore

Re: [PATCH JGIT] Circular references shouldn't be created

On Thu, Sep 17, 2009 at 3:23 PM, Sohn, Matthias <matthias.sohn@sap.com> wrote:
Show 5 quoted lines
>        void link(final String name, final String target) throws IOException {
> +               if (name.equals(target))
> +                       throw new IllegalArgumentException(
> +                                       "illegal circular reference : symref " + name
> +                                                       + " cannot refer to " + target);
This isn't a very thorough fix.  It doesn't catch longer loops, like
    HEAD -> chicken -> HEAD
or
   a -> b -> c -> d -> a

Experimenting with original git.git's implementation, I see that this is allowed:

   git symbolic-ref refs/heads/boink refs/heads/boink
It succeeds and creates a file that looks like this:
   ref: refs/heads/boink
And "git show-ref refs/heads/boink" says: nothing (but returns an error code).
And "git log refs/heads/boink" says:
   warning: ignoring dangling symref refs/heads/boink.
   fatal: ambiguous argument 'refs/heads/boink': unknown revision or
path not in the working tree.
   Use '--' to separate paths from revisions

Clearly, in git.git, symref loops are caught at ref read time, not write time. This makes sense, since someone might foolishly twiddle the repository by hand and you don't want to get into an infinite loop in that case. Also, it's potentially useful to allow people to set invalid symrefs *temporarily*, as part of a multi step process.

Have fun,
Avery
Robin Rosenberg· Sep 17, 2009, 22:51 UTC · re: Avery Pennarun · lore

Re: [PATCH JGIT] Circular references shouldn't be created

torsdag 17 september 2009 23:40:12 skrev Avery Pennarun <apenwarr@gmail.com>:
Show 38 quoted lines
> On Thu, Sep 17, 2009 at 3:23 PM, Sohn, Matthias <matthias.sohn@sap.com> wrote:
> >        void link(final String name, final String target) throws IOException {
> > +               if (name.equals(target))
> > +                       throw new IllegalArgumentException(
> > +                                       "illegal circular reference : symref " + name
> > +                                                       + " cannot refer to " + target);
> 
> This isn't a very thorough fix.  It doesn't catch longer loops, like
> 
>     HEAD -> chicken -> HEAD
> 
> or
> 
>    a -> b -> c -> d -> a
> 
> Experimenting with original git.git's implementation, I see that this
> is allowed:
> 
>    git symbolic-ref refs/heads/boink refs/heads/boink
> 
> It succeeds and creates a file that looks like this:
> 
>    ref: refs/heads/boink
> 
> And "git show-ref refs/heads/boink" says: nothing (but returns an error code).
> 
> And "git log refs/heads/boink" says:
> 
>    warning: ignoring dangling symref refs/heads/boink.
>    fatal: ambiguous argument 'refs/heads/boink': unknown revision or
> path not in the working tree.
>    Use '--' to separate paths from revisions
> 
> Clearly, in git.git, symref loops are caught at ref read time, not
> write time.  This makes sense, since someone might foolishly twiddle
> the repository by hand and you don't want to get into an infinite loop
> in that case.  Also, it's potentially useful to allow people to set
> invalid symrefs *temporarily*, as part of a multi step process.
I had already written a patch much like this when I decided we need to do much better.

I think we should do this in the UI by not allowing the user to make a choice that would result in a loop and fixing the way the UI resolves choices. When creating a new branch we should analyze the selected ref and dereference it if it is a symbolic name like HEAD or if it is a tag, and perhaps show it like "HEAD (refs/heads/master)" in the the dialog.

Using unresolvable refs as the base for a new branch should be disallowed.
-- robin
Sohn, Matthias· Sep 18, 2009, 06:37 UTC · re: Robin Rosenberg · lore

RE: [PATCH JGIT] Circular references shouldn't be created

Robin Rosenberg <robin.rosenberg@dewire.com> wrote on Freitag, 18. September 2009 00:52
Show 45 quoted lines
>torsdag 17 september 2009 23:40:12 skrev Avery Pennarun
> <apenwarr@gmail.com>:
> > On Thu, Sep 17, 2009 at 3:23 PM, Sohn, Matthias
> <matthias.sohn@sap.com> wrote:
> > >        void link(final String name, final String target) throws
> IOException {
> > > +               if (name.equals(target))
> > > +                       throw new IllegalArgumentException(
> > > +                                       "illegal circular reference
> : symref " + name
> > > +                                                       + " cannot
> refer to " + target);
> >
> > This isn't a very thorough fix.  It doesn't catch longer loops, like
> >
> >     HEAD -> chicken -> HEAD
> >
> > or
> >
> >    a -> b -> c -> d -> a
> >
> > Experimenting with original git.git's implementation, I see that this
> > is allowed:
> >
> >    git symbolic-ref refs/heads/boink refs/heads/boink
> >
> > It succeeds and creates a file that looks like this:
> >
> >    ref: refs/heads/boink
> >
> > And "git show-ref refs/heads/boink" says: nothing (but returns an
> error code).
> >
> > And "git log refs/heads/boink" says:
> >
> >    warning: ignoring dangling symref refs/heads/boink.
> >    fatal: ambiguous argument 'refs/heads/boink': unknown revision or
> > path not in the working tree.
> >    Use '--' to separate paths from revisions
> >
> > Clearly, in git.git, symref loops are caught at ref read time, not
> > write time.  This makes sense, since someone might foolishly twiddle
> > the repository by hand and you don't want to get into an infinite loop
> > in that case.  Also, it's potentially useful to allow people to set
> > invalid symrefs *temporarily*, as part of a multi step process.

Looks like I was a bit short-sighted yesterday, I will try to cook a better solution.

Show 14 quoted lines
> 
> I had already written a patch much like this when I decided we need to
> do much better.
> 
> I think we should do this in the UI by not allowing the user to make a
> choice that would result in a loop and fixing the way the UI resolves
> choices. When creating a new branch we should analyze the selected
> ref and dereference it if it is a symbolic name like HEAD or if it is a
> tag,
> and perhaps show it like "HEAD (refs/heads/master)" in the the dialog.
> 
> Using unresolvable refs as the base for a new branch should be
> disallowed.
>

If we would do it in the EGit UI how about catching such cases in other applications using JGit ?

-- Matthias

Shawn O. Pearce· Sep 18, 2009, 21:20 UTC · re: Sohn, Matthias · lore

Re: [PATCH JGIT] Circular references shouldn't be created

"Sohn, Matthias" <matthias.sohn@sap.com> wrote:
Show 13 quoted lines
> Robin Rosenberg <robin.rosenberg@dewire.com> wrote on Freitag, 18. September 2009 00:52
> > I think we should do this in the UI by not allowing the user to make a
> > choice that would result in a loop and fixing the way the UI resolves
> > choices. When creating a new branch we should analyze the selected
> > ref and dereference it if it is a symbolic name like HEAD or if it is a
> > tag,
> > and perhaps show it like "HEAD (refs/heads/master)" in the the dialog.
> > 
> > Using unresolvable refs as the base for a new branch should be
> > disallowed.
> 
> If we would do it in the EGit UI how about catching such cases 
> in other applications using JGit ?

I agree with Matthias here, other applications using JGit will also want to be able to detect a ref loop at ref creation time, and also at ref reading time. We should put the test function into JGit and allow the UI to call that test function to determine if creating that symref right now would create a loop. EGit UI can then use that function to qualify the user's selection, and prevent the user from making a choice which would create a loop.

-- 
Shawn.

← back to recent threads