Refactor prepare_body

@Lukasa Here's an alternative fix to #3066 (discussed in PR #3181), where I refactored `PreparedRequest.prepare_body` and `PreparedRequest.prepare_content_length` so that `prepare_content_length` is always called.

My only question is in the case when the body is a stream (which it is in the case that brought up this issue) that the current position will always be 0 when the length is calculated. Previously, in cases where `prepare_auth` was not called, the content length was being calculated with `super_len`, now it would be calculated using `self.headers['Content-Length'] = builtin_str(max(0, end_pos - curr_pos))`. As far as I can tell, this will give the same result (as long as the current position is 0), and this is how it's been computed in cases where authorization is being used. But I'm curious if you have thoughts about this.


So, I think in general I like this better. However, your point about `super_len` is a real one.

I think the solution here is to rewrite `prepare_content_length` to have it always call `super_len`. `super_len` does use `tell` to do a little dance here, so I think it's probably going to be good enough: @sigmavirus24, does that seem right to you?


@Lukasa in that case, could we get rid of the first elif statement and just go to the case where `body is not None`? Looking at `super_len`, I think it would calculate length in the same way. So in what I'm proposing, prepare content length would look like this:

``` python
    def prepare_content_length(self, body):
        if 'Transfer-Encoding' in self.headers:
            return
        elif body is not None:
            l = super_len(body)
            if l:
                self.headers['Content-Length'] = builtin_str(l)
        elif (self.method not in ('GET', 'HEAD')) and (self.headers.get('Content-Length') is None):
            self.headers['Content-Length'] = '0'
```

All tests pass with this change.


Ok, I think I'm happy with this, though I'd like @sigmavirus24 to review before we merge. =)


I'm not sure of the right way to do this, but this will allow someone to do:

``` py
r = requests.post(url, headers={'Transfer-Encoding': 'chunked'}, data='foo')
```

Which will generate an invalid request to the server. I would much rather forcibly strip the TE header from something where we can determine the length than ignore adding the Content-Length header if a TE is present.


@sigmavirus24 Arg, good spot. Hrm. We need to have some extra logic here to get this right.


@Lukasa @sigmavirus24 it looks to me like the try/except of using `super_len` to calculate the length of the body could be moved to `prepare_content_length` - and then both the `Content-Length` and `Transfer-Encoding` headers would be set there. If content length can be calculated, then we strip the TE header, and otherwise, we strip the CL header. I think this would be the most clear, and would ensure that the headers are mutually exclusive, regardless of what a user enters.


@davidsoncasey I think we want to keep the `try...except` in `super_len`: that method gets used outside requests quite frequently. It won't hurt to do the check twice.


@Lukasa sounds good. I'll see if I can put something together for that. Thanks for helping work through this.


@Lukasa @sigmavirus24 I updated this PR to make the Content-Length and Transfer-Encoding headers mutually exclusive, regardless of whether a user manually provides a value, as @sigmavirus24 showed in his comment. While working on this, I stumbled upon #1648. I hadn't realized that this had been a subject of so much discussion, and that people have differing opinions about how this should work. This is obviously related to that issue, so I understand if you decide that this isn't the fix that you're looking for. It could be refactored to raise an error if a user has provided a CL header when TE is set, and vice versa, instead of silently removing the unneeded header. Let me know what you think.


I'm not sure which behaviour I want (if we **must** merge something before 3.0). I think I'd rather raise an exception though.


I'd be inclined to want to raise an exception, but I also think we should consider discussing this at PyCon.


@Lukasa @sigmavirus24 I'll go ahead and alter it to raise an exception for now (perhaps `InvalidHeadersError`? Or is there an existing exception that would make sense to raise?). And then we can leave this PR open until it's decided what the best behavior is.

Also, are you planning on coming to Portland for PyCon? I didn't get a ticket in time, but I live in Portland and it would be great to meet up and discuss the project.


Yup, we'll be in Portland: we're planning to have a "state of Requests" chat at some point that'll be able to tackle things like these.


Fantastic, I'd love to participate.


@Lukasa @sigmavirus24 I made a few more changes and a bit more refactoring of `prepare_body` and `prepare_content_length`. These include:
- Moving the logic set `Transfer-Encoding` header to the `prepare_content_length` method. This makes it more explicit that the headers should be mutually exclusive, and does not rely on implicitly connected logic in two methods.
- Raises an `InvalidHeaderError` if a user manually sets either header when it should not be set (let me know if you think that there's a preexisting exception that could be raised instead).
- Added tests to check different cases of when each header should be set.

I understand we may need to wait to decide if this is the desired behavior, so no need to review or merge this right away. We can leave this PR open until you've had a chance to discuss and agree upon the desired behavior.


I've got a note inline, but I don't mind this change: it's definitely backward-incompatible though, so we'll need to release this as part of 3.0.0.


Once we've cleaned this up, can we squash some of these commits together and send this to proposed/3.0.0?


@sigmavirus24 @Lukasa sounds good. I'll make those small cleanup changes and squash extraneous commits.


I squashed some commits together and it looks like there's a conflict now. I'll get a fresh pull of the master branch and get that resolved.


@Lukasa @sigmavirus24 alright, it took me a bit to get back to this, but I've cleaned it up a bit and it should be ready to merge into the proposed branch. The conlflict is in AUTHORS.rst. Let me know if there's anything else here you'd like to see changed, otherwise I'll try to find another issue to get started on.


Thanks @davidsoncasey, I've left a few more notes.


Ok, good by me. @sigmavirus24?


So this looks great, but:
1. Should this be against proposed/3.0?
2. Can we resolve the merge conflicts?


@sigmavirus24 I'll open a new PR into proposed/3.0. As far as the conflict, that was only in the contributors.rst, and I was thinking it would be a little cleaner if one of you resolved it on merge, instead of back merging master into this branch and fixing it there. I'm happy to do that if you like though.


needs a rebase

closing due to inactivity (not sure if that's our fault or not). please re-open if you're still interested in getting this in!