[MERGE] Updated sftp_readv

Vincent Ladeuil v.ladeuil+lp at free.fr
Thu Dec 20 16:27:13 GMT 2007


>>>>> "john" == John Arbash Meinel <john at arbash-meinel.com> writes:

    john> Vincent Ladeuil wrote:
    john> ...

    >> I think we should take a decision on that point and also
    >> includes/excludes the ability to specify duplicate offsets to
    >> readv.
    >> 
    >> I can't imagine use cases for neither functionality, so I'd vote
    >> to get rid of them.I f needed, the caller can handle such cases
    >> but at the transport level and given the complexity of the
    >> actuals implementationS, I'd prefer that we simply exclude them.

    john> Well, I did set the default to allow_overlap=False. And
    john> it turns out that the overlap check will also disallow
    john> duplicate ranges.

True. So what about getting rid of that, if the need arise,
either the caller may find a simple way to do it or we'll have a
cleaner base to implement it.

    john> ...

    john> last_end = None
    john> cur = _CoalescedOffset(None, None, [])
    john> +        coalesced_offsets = []
    >> 
    >> Why did you delete the iterator behavior ? You still iterate the
    >> result in the caller.

    john> Because all callers actually use a list. If you look at
    john> the sftp code, it actually casts it into a list, and
    john> wraps a different iterator around it. We need the data
    john> 2 times, so we have to use a list. The second iterator
    john> is just rather than keeping an integer of which offset
    john> we are on.

Ok. I was just surprised, no problem with that.

<snip/>

    john> Are you talking about _coalesce_offsets or _sftp_readv?

_sftp_readv.

_coalesce_offsets is a bit surprising at first read, but after
that, there is one big condition which is easy to read.

<snip/>

    >> Your complexity daemon warns you too I see :)

    john> Actually, I had started to try and implement it that
    john> way. It was going to take me too long to get all the
    john> bits right, and doing "data = buffer[start:end]" is a
    john> whole lot simpler than trying to do the same thing from
    john> a list.

Sure. But l.append() / s = ''.join(l) is mentioned in several
places as the most optimal way to handle such things, but until
the method become far simpler I will not require that change.

<snip/>

    john> It shouldn't be possible from the outside. So it isn't something we can
    john> actually test.

    john> I could take the if statement out completely, though it
    john> did help before I got the algorithm correct.

My thought.

If the method was split it may become possible to test from
outside, that's what I was trying to express (poorly).

<snip/>


    john> For HTTP you pushed the complexity down into a helper (response file). Which
    john> might be a reasonable way to handle it here.

Exactly.

<snip/>

    john> +                buffered_data = ''
    john> +                buffered_start = cur_coalesced.start
    john> +            elif buffered_end >= cur_coalesced.start + cur_coalesced.length:
    john> +                raise AssertionError('Somehow we read too much data: %s > %s'
    john> +                    % (buffered_end, cur_coalesced.start + cur_coalesced.length))
    >> 
    >> Same here, if that could be documented and exercised in a test,
    >> the resulting code would be clearer.

    john> Well, the test was already present, and it is something that depends on
    john> "fp.readv()" returning more data than we requested. Which would be the fault of
    john> Paramiko.

Ok.

    john> I'm not really sure how you would document and exercise
    john> it in a test. But if you really did want to test it,
    john> then you need code to handle it. Which means that you
    john> still have this elif and raise an exception here.

    john> So I'm not sure how it would simplify the cdoe.

Forget it, I misunderstood. A comment mentioning your explanation
above (even shorter) will be enough.

<snip/>

    >> In summary, on three main criteria:
    >> - Doesn't reduce test coverage
    >> - Doesn't reduce design clarity
    >> - Improves bugs, features, speed, or code simplicity.
    >> 
    >> Err, I think you succeed only on part of the third: improve bugs
    >> ;-)

    john> Well, I would argue it doesn't improve a bug, but features/speed. :)

Well I copy/pasted from HACKING and was a bit puzzled by the
'improve bug'.

If I would argue I'd say your patch improve the robustness by
putting less stress on memory, so yes improving features/speed by
not trashing/crashing ;-)

    >> 
    >> Hope that helps,
    >> 
    >> Vincent
    >> 
    >> P.S.: Since this is my first real review, don't hesitate to
    >> comment the review itself.
    >> 

    john> I think you did very well. You also remembered to think
    john> outside of just what was written, which is what I find
    john> the hardest to do when reviewing.

Thanks,

        Vincent



More information about the bazaar mailing list