Fix duplicate file detection logic - Issue 660

[andrew-bowley]

The error is occurring in method removeDuplicatesFromJar at line 830 of class ApkMojo in package com.simpligility.maven.plugins.android.phase09package:

duplicateZos.putNextEntry( entry );

The error is genuine in that the ZipEntry "entry" is a duplicate. The previous line failed to correctly detect a duplicate because the logic is reversed:

!duplicatesAdded.add( entry.getName() )

API: HashSet.add() returns true if this set did not already contain the specified element.

This change corrects indicated statement at line 828 by removing the exclamation mark in front of "duplicatessAdded".

1 file changed, 1 insertion(+), 1 deletion(-)

[william-ferguson-au]

@andrew-bowley I think you are correct in that this code block should only be entered if (duplicatesAdded(.add( entry.getName() )

But I can't see where duplicatesAdded is ever modified, so it always be an emptySet.
@kedzie can you shed some light as this was code you added in version:6a1a483.

[andrew-bowley]

Hi William

The duplicatesAdded HashSet is modified when the add operation returns true.
Combining a test with an action in one method is efficient but the intention
gets obscured.
The alternative is to do a contains() test and then only proceed with the
add() if the result is false.

@@ -825,7 +825,7 @@ private File removeDuplicatesFromJar( File in,
List duplicates,

                 //if not handled by transformer, add (once) to

duplicates jar

                 if ( !resourceTransformed )

                 {

•                    if ( !duplicatesAdded.contains( entry.getName() )
  

                       {
                           duplicatesAdded.add( entry.getName() ) )

Regards

Andrew

---

From: William Ferguson [mailto:[email protected]]
Sent: Tuesday, 28 July 2015 4:40 PM
To: simpligility/android-maven-plugin
Cc: Andrew Bowley
Subject: Re: [android-maven-plugin] Fix duplicate file detection logic -
Issue 660 (#661)

In
src/main/java/com/simpligility/maven/plugins/android/phase09package/ApkMojo.
java
<#661 (comment)
5617931> :

@@ -825,7 +825,7 @@ private File removeDuplicatesFromJar( File in,
List duplicates,

                 //if not handled by transformer, add (once) to

duplicates jar

                 if ( !resourceTransformed )

                 {

•                    if ( !duplicatesAdded.add( entry.getName() ) )
  

•                    if ( duplicatesAdded.add( entry.getName() ) )
  

@andrew-bowley https://github.com/andrew-bowley I think you are correct
in that this code block should only be entered if (duplicatesAdded(.add(
entry.getName() )

But I can't see where duplicatesAdded is ever modified, so it always be an
emptySet.
@kedzie https://github.com/kedzie can you shed some light as this was
code you added in version:6a1a483.

Reply to this email directly or view
<https://github.com/simpligility/android-maven-plugin/pull/661/files#r356179
31> it on GitHub.
<https://github.com/notifications/beacon/AIVw2I2GKi36lx8t379vuUDu8eHW1PArks5
ohxsxgaJpZM4Fg3R3.gif>

[william-ferguson-au]

Of course.
Can I suggest you change it to make the meaning clearer.
The efficiency gain it gives is of low value here.

[WonderCsabo]

I just checked the commit list, and i found it really misleading. I think commits like these should be squashed (next time). IMHO.

[william-ferguson-au]

Fair point

[mosabua]

True. We can either make it a requirement for pull requests to only have a squashed commit in it or we can squash upon merging. The problem with squashing upon merging of course is that the author changes to whoever does the merge which is kind of misleading. Wdyt @simpligility/android-maven-plugins-core-committers ..require squash in pull request?

[WonderCsabo]

Maybe my previous comment was a little bit misunderstood. I do not think just one squashed commit per PRs is a good thing. Commits should be created logically, in numbers as much as needed, like outlined in this guide.

In this case, a squash was needed because the follow-up commits were just fixes to the first one, and did not add new changes (like fixing code style etc). Moreover, the commit messages were also the same.

Also, it is always better to ask to author to rebase the branch. Since the author knows the code and the commits the best, furthermore the commit history is really clear, and GH also shows the PR merged correctly.

[william-ferguson-au]

I agree with Csaba.
I should have asked the submitter to rebase and squash those commits
because the 2nd two were logically fixes to the first.
ᐧ

On Tue, Sep 1, 2015 at 7:42 AM, Csaba Kozák [email protected]
wrote:

Maybe my previous comment was a little bit misunderstood. I do not think
just one squashed commit per PRs is a good thing. Commits should be created
logically, in numbers as much as needed, like outlined in this
https://github.com/agis-/git-style-guide/blob/master/README.md#commits
guide.

In this case, a squash was needed because the follow-up commits were just
fixes to the first one, and did not add new changes (like fixing code style
etc). Moreover, the commit messages were also the same.

Also, it is always better to ask to author to rebase the branch. Since the
author know the best the code and the commits, moreover the commit history
is really clear, and GH also shows the PR merged correctly.

—
Reply to this email directly or view it on GitHub
#661 (comment)
.

[Shusshu]

+1 @WonderCsabo