[MERGE] push handles file-ids containing quotes correctly

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

John Arbash Meinel wrote:
> Aaron Bentley wrote:
> 
>>>I'm just fixing a broken function, not rewriting it the way I think it
>>>should have been written in the first place.
> 
> 
> Why not fix it the right way... Either way you are taking wrong behavior
> and making it right.

Because I assumed (wrongly, it seems) that conservative changes would be
less controversial than radical changes.  Also, I've always been dubious
of this approach to finding modified file id's.  The whole thing is
wrong, and changing this won't make it right.

> I don't really think of it as a codepath problem, but more of
> unsanitized data. (It isn't like we are passing a int when we expect a
> string, you are expected to pass data that you read in from a file).

There is that.  But this approach already assumes that there are no
comments or processing instructions or CDATA sections in the file
version, so assuming the data contains no entities except for the
predefined ones doesn't seem like much more of a leap.  I'd be more
concerned about mangled entities like '&amp' or '&&aammpp;;'.

>>>Strongly disagree.  Private methods need not be tested, but their
>>>clients must be tested.

> Well, that is an argument for testing 'file_ids_altered_by_revision_ids'
> not for testing 'push'.

I wasn't arguing for testing push, I was arguing for testing
Repository.fetch.

I hope the argument for testing push is self evident.  It was broken,
we're fixing it, we don't want regressions.

But I'd be happy to test file_ids_altered_by_revision_ids as well.

> I think it is fine to directly test a function so that it conforms to
> its interface. We have lots of tests that directly call a private function.

You said 'should'.  In RFC terms, I'd consider it a MAY, not a SHOULD.
We also have lots of untested private functions.

> We just have more flexibility in the future if we want to modify a
> private function. It doesn't *have* to conform to the interface we gave
> it. But test should fail so that we know we modified it somehow.

Requiring testing of private functions makes refactoring punishingly
difficult, so I don't think it should be required.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD4DBQFEso5I0F+nu1YWqI0RAm7dAJY0Md1baBxo5DS8nCFeei7QA9LsAJ46f7We
EexK64EKiySBsX80HFMkIw==
=cgv7
-----END PGP SIGNATURE-----