Principal is domainFormat agnostic.

Principal is domainFormat agnostic.

[myk, 2016-03-24 16:03:28]

The changes to Principal.java look great -- but while you have made tiny changes
to a couple of other test files, what is missing is changes to
PrincipalTest.java to demonstrate that principals composed with different domain
formats are (now) equal to each other (as well as having a zero value when
calling compareTo() on each other).  That's the only thing that's left to do.

https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/ada...
File src/com/google/enterprise/adaptor/Principal.java (right):

https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/Principal.java:69: &&
p.parse().plainName.equals(parse().plainName)
easier to read than creating new variables to hold the parsed entities.  Parsing
doesn't take too long; it's OK to do it twice.

https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/Principal.java:77: return Arrays.hashCode(new
Object[]{ isUser(), parse().domain, parse().plainName, namespace });
Line too long; please split after parse().domain,   (and indent the next line 4
additional spaces).

https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/Principal.java:102: // Only for users
This comment doesn't explain why you are checking domains only if both
principals are users.
Suggestion:  replace with something along the lines of:
// Skip the domain comparison for groups because ____
(fill in the blank with the justification).

https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/Principal.java:103: if (isUser() &&
other.isUser()) {
lines 97-98 imply that isUser() and other.isUser() have the same value -- you
could just check
if (isUser()) here.

https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/Principal.java:148: 
It's ok to keep this change (removal of leading blanks) in.

https://codereview.appspot.com/291280043/diff/1/test/com/google/enterprise/ad...
File test/com/google/enterprise/adaptor/AclTransformTest.java (right):

https://codereview.appspot.com/291280043/diff/1/test/com/google/enterprise/ad...
test/com/google/enterprise/adaptor/AclTransformTest.java:193: }
but for this file, it seems that the *only* change is whitespace.  So I would
suggest reverting this file (unless you really do have changes to make here).

[afedorenchik, 2016-03-24 18:55:17]

Hi,
I've done changes and all tests are good.
Please let me know if you have other comments.

https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/ada...
File src/com/google/enterprise/adaptor/Principal.java (right):

https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/Principal.java:69: &&
p.parse().plainName.equals(parse().plainName)
On 2016/03/24 16:03:28, myk wrote:
> easier to read than creating new variables to hold the parsed entities. 
Parsing
> doesn't take too long; it's OK to do it twice.

Acknowledged. Thanks.

https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/Principal.java:77: return Arrays.hashCode(new
Object[]{ isUser(), parse().domain, parse().plainName, namespace });
On 2016/03/24 16:03:28, myk wrote:
> Line too long; please split after parse().domain,   (and indent the next line
4
> additional spaces).

Done. Not sure about proper intend.

https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/Principal.java:102: // Only for users
On 2016/03/24 16:03:28, myk wrote:
> This comment doesn't explain why you are checking domains only if both
> principals are users.
> Suggestion:  replace with something along the lines of:
> // Skip the domain comparison for groups because ____
> (fill in the blank with the justification).

Done.

https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/Principal.java:103: if (isUser() &&
other.isUser()) {
On 2016/03/24 16:03:28, myk wrote:
> lines 97-98 imply that isUser() and other.isUser() have the same value -- you
> could just check
> if (isUser()) here.

Done.

[myk, 2016-03-24 21:08:12]

Looks good to me.   Thank you very much for your work!

https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/ada...
File src/com/google/enterprise/adaptor/Principal.java (right):

https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/Principal.java:77: return Arrays.hashCode(new
Object[]{ isUser(), parse().domain, parse().plainName, namespace });
On 2016/03/24 18:55:16, afedorenchik wrote:
> On 2016/03/24 16:03:28, myk wrote:
> > Line too long; please split after parse().domain,   (and indent the next
line
> 4
> > additional spaces).
> 
> Done. Not sure about proper intend.

4 was correct here.   In general, indent two for a new block:
if (condition) {
  thenAction();  // indented 2
} else {
  elseAction();  // indented 2
}

but indent 4 when the next line is split from the current line due to line
length limits.  If the second line isn't enough, the third line stays with the
second line's indent - it does not indent an additional 4.

https://codereview.appspot.com/291280043/diff/40001/test/com/google/enterpris...
File test/com/google/enterprise/adaptor/PrincipalTest.java (right):

https://codereview.appspot.com/291280043/diff/40001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/PrincipalTest.java:146:
assertFalse(p6.equals(g1));
Above new tests look good!  I'm surprised that Eclipse let you put in a line
with spaces only (lines 137, 141, 143).  It's OK with me if you leave those
spaces in (or if you decide to delete them).  If you do delete them (but make no
other change), feel free to submit.

[afedorenchik, 2016-03-24 21:56:24]

https://codereview.appspot.com/291280043/diff/40001/test/com/google/enterpris...
File test/com/google/enterprise/adaptor/PrincipalTest.java (right):

https://codereview.appspot.com/291280043/diff/40001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/PrincipalTest.java:146:
assertFalse(p6.equals(g1));
On 2016/03/24 21:08:12, myk wrote:
> Above new tests look good!  I'm surprised that Eclipse let you put in a line
> with spaces only (lines 137, 141, 143).  It's OK with me if you leave those
> spaces in (or if you decide to delete them).  If you do delete them (but make
no
> other change), feel free to submit.

Acknowledged. Not sure if it OK to leave as is. I can remove this spaces. Also
confused why Eclipse does not removed them on save.