From: Stefan Beller <sbeller@google.com>
Date: Thu, 08 Sep 2016 16:28:31 GMT
Subject: Re: [PATCH] connect: tighten check for unexpected early hang up (Re: [PATCH v3 2/2] connect: advertized capability is not a ref)
Message-ID: <CAGZ79kYx_vb4_x0Bw=DUpWmb2DWO2KbfU8uCganN-0dT5CUGog@mail.gmail.com>
In-Reply-To: <20160908014555.GD25016@google.com>

On Wed, Sep 7, 2016 at 6:45 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> (+cc: Heiko)
> Jonathan Nieder wrote:
>
>> 'die_initial_contact' uses got_at_least_one_head to determine whether
>> it was on the first line but code paths added later that use
>> 'continue' don't populate it properly (see b06dcd7d, 40c155ff, and
>> 1a7141ff).  We could do
>>
>>       int first_line = 1;
>>
>>       for (;; first_line = 0) {
>>               ...
>>       }
>>
>> and use !first_line instead of got_at_least_one_head (removing
>> got_at_least_one_head in the process since it has no other purpose).
>
> I got the history wrong.  It looks like this was always confused
> by the 'continue' cases.  Unless I'm missing something subtle ---
> thoughts?

I was a bit confused by the line

    for (;; first_line = 0) {

at first, but the explanation of 'continue's make sense for this pattern.
However I'd rather prefer if we'd have

    int first_line = 1;
    for(;;) {
        ... // stuff with no continue here
        if (len < 0)
            die_initial_contact(!first_line);
        first_line = 0;
        ... // here we may have some continues, but that doesn't matter
        // w.r.t. first_line
    }