This patch ought to work fine, but looking at the code my guess would be that we intended to allocate the ISVNClient once and use it throughout and only dispose it at the end. I think the dispose stuff was added later to all the users of ISVNClient and was probably done carelessly here. I cannot think of any reason to keep creating new clients in this function. Do you want to rework it so that the client is constructed once, used as needed and then disposed at the end of the process?

I would not be shocked if there are a couple of other similar problems somewhere in the code. There are not too many that work the way commit needed to though.

I wasn't quite sure about the requirements here, so I went the safe way. I thought that we might be required to use different ISVNClients per SVNTeamProvider, as we request the clients via the resource's repository, which will usually be different for each team provider. Do you know the semantics here?

If it is actually safe to use an ISVNClient for different team providers, I will adjust accordingly.

Getting the client from the Repository is just legacy code that pre-dates the 1.0 release. That code should probably be removed and callers updated to use SVNClientManager directly. There is nothing special about ISVNClient that makes it specific to any repository. So yes it is safe to use it for multiple projects.

I think that pre-1.0 the Repository contained the credentials, but we stopped supporting this a long, long time ago and rely on SVN's credential storage exclusively. Anyway, this is probably why the early design had this method to get a client.

Okay. This makes the fix even nicer, as there's already an unused svn client retrieved outside the loop

This looks good in general. I need to go back and take a closer look at the original change where this was added. Looking at the comments now, it looks like we might have introduced a new SVNClient here for console logging purposes. I cannot immediately think of why without spending a bit more time to refresh my memory on how these classes work, but maybe when you get it from the Repository class it sets up the client with the notification handler and the way the existing svn client in this class is setup it does not. This all seems fixable regardless, I just want to make sure I can recall why this code was written in the first place.

@selsemore feel free to jump in on this too.

You're right, the repository location does actually set up a notification listener. So I'll have to re-add the notification listener to the existing client. Will do in the coming week.

    public ISVNClientAdapter getSVNClient() throws SVNException {
    	ISVNClientAdapter svnClient =
    		SVNProviderPlugin.getPlugin().getSVNClient();
    
    	svnClient.addNotifyListener(NotificationListener.getInstance());
    
    	return svnClient;
    }

Thinking about this more, I wonder if your original patch is therefore not the simplest and best solution here? I do not think the overhead of constructing a new ISVNClient is too great that the optimization to reuse the existing one is worth complicating the code too much.

We can also have the best of both worlds: lazily initialize the client as before but then only dispose it after the last iteration. So simply move the try-catch-finally up one level so that it contains the loop instead of the other way round. The diff looks somewhat intriguing, but it's really mostly whitespace changes.

Thanks