* [PATCH JGIT] Circular references shouldn't be created
@ 2009-09-17 19:23 Sohn, Matthias
2009-09-17 21:40 ` Avery Pennarun
0 siblings, 1 reply; 5+ messages in thread
From: Sohn, Matthias @ 2009-09-17 19:23 UTC (permalink / raw)
To: Shawn O. Pearce, Robin Rosenberg; +Cc: git@vger•kernel.org
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(-)
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH JGIT] Circular references shouldn't be created
2009-09-17 19:23 [PATCH JGIT] Circular references shouldn't be created Sohn, Matthias
@ 2009-09-17 21:40 ` Avery Pennarun
2009-09-17 22:51 ` Robin Rosenberg
0 siblings, 1 reply; 5+ messages in thread
From: Avery Pennarun @ 2009-09-17 21:40 UTC (permalink / raw)
To: Sohn, Matthias; +Cc: Shawn O. Pearce, Robin Rosenberg, git@vger•kernel.org
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.
Have fun,
Avery
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH JGIT] Circular references shouldn't be created
2009-09-17 21:40 ` Avery Pennarun
@ 2009-09-17 22:51 ` Robin Rosenberg
2009-09-18 6:37 ` Sohn, Matthias
0 siblings, 1 reply; 5+ messages in thread
From: Robin Rosenberg @ 2009-09-17 22:51 UTC (permalink / raw)
To: Avery Pennarun; +Cc: Sohn, Matthias, Shawn O. Pearce, git@vger•kernel.org
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.
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
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH JGIT] Circular references shouldn't be created
2009-09-17 22:51 ` Robin Rosenberg
@ 2009-09-18 6:37 ` Sohn, Matthias
2009-09-18 21:20 ` Shawn O. Pearce
0 siblings, 1 reply; 5+ messages in thread
From: Sohn, Matthias @ 2009-09-18 6:37 UTC (permalink / raw)
To: Robin Rosenberg, Avery Pennarun; +Cc: Shawn O. Pearce, git@vger•kernel.org
Robin Rosenberg <robin.rosenberg@dewire•com> wrote on Freitag, 18. September 2009 00:52
>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.
>
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH JGIT] Circular references shouldn't be created
2009-09-18 6:37 ` Sohn, Matthias
@ 2009-09-18 21:20 ` Shawn O. Pearce
0 siblings, 0 replies; 5+ messages in thread
From: Shawn O. Pearce @ 2009-09-18 21:20 UTC (permalink / raw)
To: Sohn, Matthias; +Cc: Robin Rosenberg, Avery Pennarun, git@vger•kernel.org
"Sohn, Matthias" <matthias.sohn@sap•com> wrote:
> 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-09-18 21:20 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-09-17 19:23 [PATCH JGIT] Circular references shouldn't be created Sohn, Matthias
2009-09-17 21:40 ` Avery Pennarun
2009-09-17 22:51 ` Robin Rosenberg
2009-09-18 6:37 ` Sohn, Matthias
2009-09-18 21:20 ` Shawn O. Pearce
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox