{"thread":{"id":"20993","subject":"[PATCH JGIT] Circular references shouldn't be created","startedAt":"2009-09-17T19:23:13Z","lastAt":"2009-09-18T21:20:17Z","messageCount":5,"participants":["Sohn, Matthias","Avery Pennarun","Robin Rosenberg","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"123466","messageId":"C89280B882467443A695734861B942B28759DB95@DEWDFECCR09.wdf.sap.corp","threadId":"20993","inReplyTo":null,"subject":"[PATCH JGIT] Circular references shouldn't be created","fromName":"Sohn, Matthias","fromEmail":"matthias.sohn@sap.com","sentAt":"2009-09-17T19:23:13Z","receivedAt":"2009-09-17T19:23:13Z","isPatch":true,"sender":{"key":"matthias.sohn@sap.com","avatar":"https://gravatar.com/avatar/88bbb2733bcb977ec2d2cc1916ba8a70d6d41432c146bb8dab4f7f802261194e?d=mp&s=160"},"body":"From: Matthias Sohn \nCircular references shouldn't be created\n\nFix for bug: https://bugs.eclipse.org/bugs/show_bug.cgi?id=286743\n\nSigned-off-by: Matthias Sohn \n---\n .../tst/org/spearce/jgit/lib/RefTest.java | 9 +++++++++\n .../src/org/spearce/jgit/lib/RefDatabase.java | 4 ++++\n 2 files changed, 13 insertions(+), 0 deletions(-)\n\ndiff --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\nindex fabbe7e..ce6328b 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefTest.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefTest.java\n@@ -155,4 +155,13 @@ public void testOrigResolvedNamesSymRef() throws IOException {\n \t\tassertEquals(\"refs/heads/master\", ref.getName());\n \t\tassertEquals(\"HEAD\", ref.getOrigName());\n \t}\n+\t\n+\tpublic void testIllegalCircularRef() throws IOException {\n+\t\ttry {\n+\t\t\tdb.writeSymref(\"HEAD\", \"HEAD\");\n+\t\t\tfail(\"creation of circular reference should fail\");\n+\t\t} catch (IllegalArgumentException expected) {\n+\t\t\t// attempt to create circular reference should fail\n+\t\t}\n+\t}\n }\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java\nindex 09cb9bb..483b1d0 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java\n@@ -174,6 +174,10 @@ RefRename newRename(String fromRef, String toRef) throws IOException {\n \t * @throws IOException\n \t */\n \tvoid link(final String name, final String target) throws IOException {\n+\t\tif (name.equals(target))\n+\t\t\tthrow new IllegalArgumentException(\n+\t\t\t\t\t\"illegal circular reference : symref \" + name\n+\t\t\t\t\t\t\t+ \" cannot refer to \" + target);\n \t\tfinal byte[] content = Constants.encode(\"ref: \" + target + \"\\n\");\n \t\tlockAndWriteFile(fileForRef(name), content);\n \t\tsynchronized (this) {\n-- \n1.6.4.msysgit.0\n\n"},{"id":"123467","messageId":"32541b130909171440w1a6d2394t4acc6a2f791c143@mail.gmail.com","threadId":"20993","inReplyTo":"C89280B882467443A695734861B942B28759DB95@DEWDFECCR09.wdf.sap.corp","subject":"Re: [PATCH JGIT] Circular references shouldn't be created","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2009-09-17T21:40:12Z","receivedAt":"2009-09-17T21:40:12Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"On Thu, Sep 17, 2009 at 3:23 PM, Sohn, Matthias wrote:\n>        void link(final String name, final String target) throws IOException {\n> +               if (name.equals(target))\n> +                       throw new IllegalArgumentException(\n> +                                       \"illegal circular reference : symref \" + name\n> +                                                       + \" cannot refer to \" + target);\n\nThis isn't a very thorough fix. It doesn't catch longer loops, like\n\n HEAD -> chicken -> HEAD\n\nor\n\n a -> b -> c -> d -> a\n\nExperimenting with original git.git's implementation, I see that this\nis allowed:\n\n git symbolic-ref refs/heads/boink refs/heads/boink\n\nIt succeeds and creates a file that looks like this:\n\n ref: refs/heads/boink\n\nAnd \"git show-ref refs/heads/boink\" says: nothing (but returns an error code).\n\nAnd \"git log refs/heads/boink\" says:\n\n warning: ignoring dangling symref refs/heads/boink.\n fatal: ambiguous argument 'refs/heads/boink': unknown revision or\npath not in the working tree.\n Use '--' to separate paths from revisions\n\nClearly, in git.git, symref loops are caught at ref read time, not\nwrite time. This makes sense, since someone might foolishly twiddle\nthe repository by hand and you don't want to get into an infinite loop\nin that case. Also, it's potentially useful to allow people to set\ninvalid symrefs *temporarily*, as part of a multi step process.\n\nHave fun,\n\nAvery\n"},{"id":"123472","messageId":"200909180051.47794.robin.rosenberg@dewire.com","threadId":"20993","inReplyTo":"32541b130909171440w1a6d2394t4acc6a2f791c143@mail.gmail.com","subject":"Re: [PATCH JGIT] Circular references shouldn't be created","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2009-09-17T22:51:47Z","receivedAt":"2009-09-17T22:51:47Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"torsdag 17 september 2009 23:40:12 skrev Avery Pennarun :\n> On Thu, Sep 17, 2009 at 3:23 PM, Sohn, Matthias wrote:\n> > void link(final String name, final String target) throws IOException {\n> > + if (name.equals(target))\n> > + throw new IllegalArgumentException(\n> > + \"illegal circular reference : symref \" + name\n> > + + \" cannot refer to \" + target);\n> \n> This isn't a very thorough fix. It doesn't catch longer loops, like\n> \n> HEAD -> chicken -> HEAD\n> \n> or\n> \n> a -> b -> c -> d -> a\n> \n> Experimenting with original git.git's implementation, I see that this\n> is allowed:\n> \n> git symbolic-ref refs/heads/boink refs/heads/boink\n> \n> It succeeds and creates a file that looks like this:\n> \n> ref: refs/heads/boink\n> \n> And \"git show-ref refs/heads/boink\" says: nothing (but returns an error code).\n> \n> And \"git log refs/heads/boink\" says:\n> \n> warning: ignoring dangling symref refs/heads/boink.\n> fatal: ambiguous argument 'refs/heads/boink': unknown revision or\n> path not in the working tree.\n> Use '--' to separate paths from revisions\n> \n> Clearly, in git.git, symref loops are caught at ref read time, not\n> write time. This makes sense, since someone might foolishly twiddle\n> the repository by hand and you don't want to get into an infinite loop\n> in that case. Also, it's potentially useful to allow people to set\n> invalid symrefs *temporarily*, as part of a multi step process.\n\nI had already written a patch much like this when I decided we need to do much better.\n\nI think we should do this in the UI by not allowing the user to make a\nchoice that would result in a loop and fixing the way the UI resolves\nchoices. When creating a new branch we should analyze the selected \nref and dereference it if it is a symbolic name like HEAD or if it is a tag, \nand perhaps show it like \"HEAD (refs/heads/master)\" in the the dialog.\n\nUsing unresolvable refs as the base for a new branch should be disallowed.\n\n-- robin\n"},{"id":"123480","messageId":"C89280B882467443A695734861B942B28759DEAA@DEWDFECCR09.wdf.sap.corp","threadId":"20993","inReplyTo":"200909180051.47794.robin.rosenberg@dewire.com","subject":"RE: [PATCH JGIT] Circular references shouldn't be created","fromName":"Sohn, Matthias","fromEmail":"matthias.sohn@sap.com","sentAt":"2009-09-18T06:37:56Z","receivedAt":"2009-09-18T06:37:56Z","isPatch":true,"sender":{"key":"matthias.sohn@sap.com","avatar":"https://gravatar.com/avatar/88bbb2733bcb977ec2d2cc1916ba8a70d6d41432c146bb8dab4f7f802261194e?d=mp&s=160"},"body":"Robin Rosenberg wrote on Freitag, 18. September 2009 00:52\n>torsdag 17 september 2009 23:40:12 skrev Avery Pennarun\n> :\n> > On Thu, Sep 17, 2009 at 3:23 PM, Sohn, Matthias\n> wrote:\n> > > void link(final String name, final String target) throws\n> IOException {\n> > > + if (name.equals(target))\n> > > + throw new IllegalArgumentException(\n> > > + \"illegal circular reference\n> : symref \" + name\n> > > + + \" cannot\n> refer to \" + target);\n> >\n> > This isn't a very thorough fix. It doesn't catch longer loops, like\n> >\n> > HEAD -> chicken -> HEAD\n> >\n> > or\n> >\n> > a -> b -> c -> d -> a\n> >\n> > Experimenting with original git.git's implementation, I see that this\n> > is allowed:\n> >\n> > git symbolic-ref refs/heads/boink refs/heads/boink\n> >\n> > It succeeds and creates a file that looks like this:\n> >\n> > ref: refs/heads/boink\n> >\n> > And \"git show-ref refs/heads/boink\" says: nothing (but returns an\n> error code).\n> >\n> > And \"git log refs/heads/boink\" says:\n> >\n> > warning: ignoring dangling symref refs/heads/boink.\n> > fatal: ambiguous argument 'refs/heads/boink': unknown revision or\n> > path not in the working tree.\n> > Use '--' to separate paths from revisions\n> >\n> > Clearly, in git.git, symref loops are caught at ref read time, not\n> > write time. This makes sense, since someone might foolishly twiddle\n> > the repository by hand and you don't want to get into an infinite loop\n> > in that case. Also, it's potentially useful to allow people to set\n> > invalid symrefs *temporarily*, as part of a multi step process.\n\nLooks like I was a bit short-sighted yesterday, I will try to cook a better\nsolution.\n\n> \n> I had already written a patch much like this when I decided we need to\n> do much better.\n> \n> I think we should do this in the UI by not allowing the user to make a\n> choice that would result in a loop and fixing the way the UI resolves\n> choices. When creating a new branch we should analyze the selected\n> ref and dereference it if it is a symbolic name like HEAD or if it is a\n> tag,\n> and perhaps show it like \"HEAD (refs/heads/master)\" in the the dialog.\n> \n> Using unresolvable refs as the base for a new branch should be\n> disallowed.\n> \n\nIf we would do it in the EGit UI how about catching such cases \nin other applications using JGit ?\n\n--\nMatthias\n"},{"id":"123506","messageId":"20090918212017.GI14660@spearce.org","threadId":"20993","inReplyTo":"C89280B882467443A695734861B942B28759DEAA@DEWDFECCR09.wdf.sap.corp","subject":"Re: [PATCH JGIT] Circular references shouldn't be created","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-09-18T21:20:17Z","receivedAt":"2009-09-18T21:20:17Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Sohn, Matthias\" wrote:\n> Robin Rosenberg wrote on Freitag, 18. September 2009 00:52\n> > I think we should do this in the UI by not allowing the user to make a\n> > choice that would result in a loop and fixing the way the UI resolves\n> > choices. When creating a new branch we should analyze the selected\n> > ref and dereference it if it is a symbolic name like HEAD or if it is a\n> > tag,\n> > and perhaps show it like \"HEAD (refs/heads/master)\" in the the dialog.\n> > \n> > Using unresolvable refs as the base for a new branch should be\n> > disallowed.\n> \n> If we would do it in the EGit UI how about catching such cases \n> in other applications using JGit ?\n\nI agree with Matthias here, other applications using JGit will\nalso want to be able to detect a ref loop at ref creation time,\nand also at ref reading time. We should put the test function into\nJGit and allow the UI to call that test function to determine if\ncreating that symref right now would create a loop. EGit UI can\nthen use that function to qualify the user's selection, and prevent\nthe user from making a choice which would create a loop.\n\n-- \nShawn.\n"}]}