[egit PATCH] avoiding output in the error log

[egit PATCH] avoiding output in the error log

[robert_no.spam_m]

Hi,this is a patch for egit. The purpose of the patch
is to avoid a lot of output in the error log.

>From 6f7be5d1a41d356a9b558ac81722e959095ff04d Mon Sep 17 00:00:00 2001
From: robert <robert_no.spam_m@yahoo.fr>
Date: Mon, 25 Aug 2008 20:42:39 +0200
Subject: [PATCH] Checks added to avoid a lot of entries in the log

* IResource.exists() must be checked before calling setProperty on it.
* getActiveDecorator() was returning null in some case.
---
 .../internal/decorators/GitResourceDecorator.java  |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitResourceDecorator.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitResourceDecorator.java
index 5857eaf..7c10ec6 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitResourceDecorator.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitResourceDecorator.java
@@ -143,7 +143,9 @@ if (resources.size() > 0)
 					try {
 						m.accept(new IResourceVisitor() {
 							public boolean visit(IResource resource) throws CoreException {
-								getActiveDecorator().clearDecorationState(resource);
+								GitResourceDecorator decorator = getActiveDecorator();
+								if (decorator != null)
+									decorator.clearDecorationState(resource);
 								return true;
 							}
 						},
@@ -197,8 +199,10 @@ synchronized (resources) {
 	} // End ResCL
 
 	void clearDecorationState(IResource r) throws CoreException {
-		r.setSessionProperty(GITFOLDERDIRTYSTATEPROPERTY, null);
-		fireLabelProviderChanged(new LabelProviderChangedEvent(this, r));
+		if (r.exists()) {
+			r.setSessionProperty(GITFOLDERDIRTYSTATEPROPERTY, null);
+			fireLabelProviderChanged(new LabelProviderChangedEvent(this, r));
+		}
 	}
 
 	static ResCL myrescl = new ResCL();
-- 
1.6.0

Re: [egit PATCH] avoiding output in the error log

[Robin Rosenberg]

onsdagen den 27 augusti 2008 11.34.33 skrev robert_no.spam_m@yahoo.fr:
> Hi,this is a patch for egit. The purpose of the patch
> is to avoid a lot of output in the error log.

Welcome Robert!

> 
> From 6f7be5d1a41d356a9b558ac81722e959095ff04d Mon Sep 17 00:00:00 2001
> From: robert <robert_no.spam_m@yahoo.fr>
> Date: Mon, 25 Aug 2008 20:42:39 +0200
> Subject: [PATCH] Checks added to avoid a lot of entries in the log
> 
> * IResource.exists() must be checked before calling setProperty on it.
> * getActiveDecorator() was returning null in some case.
> ---
>  .../internal/decorators/GitResourceDecorator.java  |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitResourceDecorator.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitResourceDecorator.java
> index 5857eaf..7c10ec6 100644
> --- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitResourceDecorator.java
> +++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/decorators/GitResourceDecorator.java
> @@ -143,7 +143,9 @@ if (resources.size() > 0)
>  					try {
>  						m.accept(new IResourceVisitor() {
>  							public boolean visit(IResource resource) throws CoreException {
> -								getActiveDecorator().clearDecorationState(resource);
> +								GitResourceDecorator decorator = getActiveDecorator();
> +								if (decorator != null)
> +									decorator.clearDecorationState(resource);

When doest his happen?

>  								return true;
>  							}
>  						},
> @@ -197,8 +199,10 @@ synchronized (resources) {
>  	} // End ResCL
>  
>  	void clearDecorationState(IResource r) throws CoreException {
> -		r.setSessionProperty(GITFOLDERDIRTYSTATEPROPERTY, null);
> -		fireLabelProviderChanged(new LabelProviderChangedEvent(this, r));
> +		if (r.exists()) {
> +			r.setSessionProperty(GITFOLDERDIRTYSTATEPROPERTY, null);
> +			fireLabelProviderChanged(new LabelProviderChangedEvent(this, r));
> +		}

Shawn, you removed the test from the code in 4a230ea1. Perhaps you could care to
comment on this patch to restore the test, though slightly different.

		if (r.isAccessible())
			r.setSessionProperty(GITFOLDERDIRTYSTATEPROPERTY, null);

-- robin

Re: [egit PATCH] avoiding output in the error log

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> > @@ -197,8 +199,10 @@ synchronized (resources) {
> >  	} // End ResCL
> >  
> >  	void clearDecorationState(IResource r) throws CoreException {
> > -		r.setSessionProperty(GITFOLDERDIRTYSTATEPROPERTY, null);
> > -		fireLabelProviderChanged(new LabelProviderChangedEvent(this, r));
> > +		if (r.exists()) {
> > +			r.setSessionProperty(GITFOLDERDIRTYSTATEPROPERTY, null);
> > +			fireLabelProviderChanged(new LabelProviderChangedEvent(this, r));
> > +		}
> 
> Shawn, you removed the test from the code in 4a230ea1. Perhaps you could care to
> comment on this patch to restore the test, though slightly different.
> 
> 		if (r.isAccessible())
> 			r.setSessionProperty(GITFOLDERDIRTYSTATEPROPERTY, null);

This may be a patch queue reordering bug.

If you look at 4a230ea1 I moved the isAccessible test higher up,
such that we shouldn't enter into clearDecorationState unless the
resource is accessible.  But there may currently be two code paths
that enter this method, and a patch I did not yet submit had removed
the other code path.

So in the short/medium term I agree with your patch, basically undo
the second hunk of 4a230ea1.  But keep the middle hunk, it was a
real bug I ran into.

-- 
Shawn.

Re: [egit PATCH] avoiding output in the error log

[Robin Rosenberg]

onsdagen den 27 augusti 2008 22.33.43 skrev Shawn O. Pearce:
> Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> > > @@ -197,8 +199,10 @@ synchronized (resources) {
> > >  	} // End ResCL
> > >  
> > >  	void clearDecorationState(IResource r) throws CoreException {
> > > -		r.setSessionProperty(GITFOLDERDIRTYSTATEPROPERTY, null);
> > > -		fireLabelProviderChanged(new LabelProviderChangedEvent(this, r));
> > > +		if (r.exists()) {
> > > +			r.setSessionProperty(GITFOLDERDIRTYSTATEPROPERTY, null);
> > > +			fireLabelProviderChanged(new LabelProviderChangedEvent(this, r));
> > > +		}
> > 
> > Shawn, you removed the test from the code in 4a230ea1. Perhaps you could care to
> > comment on this patch to restore the test, though slightly different.
> > 
> > 		if (r.isAccessible())
> > 			r.setSessionProperty(GITFOLDERDIRTYSTATEPROPERTY, null);
> 
> This may be a patch queue reordering bug.
> 
> If you look at 4a230ea1 I moved the isAccessible test higher up,
> such that we shouldn't enter into clearDecorationState unless the
> resource is accessible.  But there may currently be two code paths
> that enter this method, and a patch I did not yet submit had removed
> the other code path.

Good. So then the question is whether we should use isAccessible() or exists()? Normally
this makes no difference, but for a Project (at least) there is a difference. I project that exits
is not accessible when closed. 

-- robin

Re: [egit PATCH] avoiding output in the error log

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> > > 
> > > Shawn, you removed the test from the code in 4a230ea1. Perhaps you could care to
> > > comment on this patch to restore the test, though slightly different.
> > > 
> > > 		if (r.isAccessible())
> > > 			r.setSessionProperty(GITFOLDERDIRTYSTATEPROPERTY, null);
> 
> Good. So then the question is whether we should use isAccessible() or exists()? Normally
> this makes no difference, but for a Project (at least) there is a difference. I project that exits
> is not accessible when closed. 

It _has_ to be isAccessible().  You cannot get or set a session
property on a closed project.

-- 
Shawn.