Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

Fixed the --excludes option #17

Closed
wants to merge 1 commit into from
Closed

Fixed the --excludes option #17

wants to merge 1 commit into from

Conversation

Benaan
Copy link

@Benaan Benaan commented May 9, 2014

The CLI client produced the following error when doing a push command which uses the --excludes option. The --includes option doesn't cause this error.

java.lang.UnsupportedOperationException
at java.util.AbstractList.add(AbstractList.java:148)
at java.util.AbstractList.add(AbstractList.java:108)
at org.zanata.client.commands.push.AbstractPushStrategy.addExcludesForLocaleFilenames(AbstractPushStrategy.java:112)
at org.zanata.client.commands.push.AbstractPushStrategy.getSrcFiles(AbstractPushStrategy.java:98)
at org.zanata.client.commands.push.PropertiesStrategy.findDocNames(PropertiesStrategy.java:80)
at org.zanata.client.commands.push.PushCommand.pushCurrentModule(PushCommand.java:280)
at org.zanata.client.commands.push.PushCommand.run(PushCommand.java:203)
at org.zanata.client.commands.ConfigurableCommand.runWithActions(ConfigurableCommand.java:94)
at org.zanata.client.commands.ArgsUtil.runCommand(ArgsUtil.java:48)
at org.zanata.client.ZanataClient.processArgs(ZanataClient.java:168)
at org.zanata.client.ZanataClient.main(ZanataClient.java:93)

@zanata-jenkins
Copy link

Can one of the admins verify this patch?

@buildhive
Copy link

Zanata » zanata-client #174 SUCCESS
This pull request looks good
(what's this?)

@@ -175,7 +175,7 @@ void setIncludes(String includes) {
+ "--excludes=\"Pattern1,Pattern2,Pattern3\"")
public
void setExcludes(String excludes) {
this.excludes = StringUtil.split(excludes, ",");
this.excludes.addAll(StringUtil.split(excludes, ","));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the main cause/fix of the issue. Field should be set to new value in Setter method. We should check what is the exact exception in AbstractPushStrategy.java:112.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @aeng here in that a setter should not be adding elements to the existing list.
Perhaps the right change should be to wrap the split call around a new concrete list:

this.excludes = new ArrayList( StringUtil.split(excludes, ",") )

The problem seems to be that the excludes list is being set to an unmodifiable list, that later down the code is being reused and modified.
@Benaan Could you provide a test case or at least describe your environment (OS, Java version, etc.) and the exact command line you were running when you saw this?

@Benaan
Copy link
Author

Benaan commented May 12, 2014

The problem is indeed that setExcludes creates an unmodifiable list. I've tested your solution and it also fixes the error.

My environment is:
Debian 7.4 (wheezy)
Oracle Java 7u55

I've tested with both the latest master version and the 3.3.0 release. Both produce the error.

The command I've used to test this:

zanata-cli -B -e push --excludes **/log4j.properties -s . -t .

@seanf
Copy link
Contributor

seanf commented May 19, 2014

@Benaan would you mind checking out #20? This problem is really about the code not being clear about what's mutable and what's not, so I have used ImmutableSet/List where possible, and adapted the logic to suit.

@seanf
Copy link
Contributor

seanf commented May 21, 2014

Replaced by #20 and #22.

Thanks @Benaan.

@seanf seanf closed this May 21, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants