Re: [PATCH] create_delta_index: simplify condition always evaluating to true

From: Nicolas Pitre <nico@fluxnic.net>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Stefan Beller <stefanbeller@googlemail.com>,
	Git List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] create_delta_index: simplify condition always evaluating to true
Date: Thu, 15 Aug 2013 22:38:52 -0400 (EDT)	[thread overview]
Message-ID: <alpine.LFD.2.03.1308152236250.14472@syhkavp.arg> (raw)
In-Reply-To: <CAPig+cQ5Y9irLk=9Bhz09c=5yzZEcyMKn2kbhcrO_zDpgmkhGw@mail.gmail.com>

On Thu, 15 Aug 2013, Eric Sunshine wrote:

> On Thu, Aug 15, 2013 at 5:34 PM, Stefan Beller
> <stefanbeller@googlemail.com> wrote:
> > When checking the previous lines in that function, we can deduce that
> > hsize must always be smaller than (1u<<31), since 506049c7df2c6
> > (fix >4GiB source delta assertion failure), because entries is
> > capped at an upper bound of 0xfffffffeU, so hsize contains a maximum
> > value of 0x3fffffff, which is smaller than (1u<<31), so the value of
> > 'i' will never be larger than 31.
> >
> > Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
> > ---
> >
> > Eric, thanks for reviewing my patch.
> >
> > I applied the first 2 proposals (deduce, entries), but I disagree on
> > the third, so I reformulated the sentence, as I really meant the variable
> > i and not it as a pronoun.
> 
> Thanks. Adding the quotes around 'i' makes your meaning clear. Without
> the quotes, apparently it was ambiguous, and my brain read it as a
> misspelling of 'it'.
> 
> > Do I understand right, you're suggesting to remove the
> > source code comment? I did this now, but I have a bad feeling with it.
> >
> > The change of this patch surely removes dead code as of now and makes it
> > more readable. But also it could become alive again, once somebody
> > changes things nearby and forgets about the assumption, hsize not
> > exceeding a certain size. That's why I put a comment in there, so
> > the future changes nearby may be more careful.
> 
> Indeed, I feel uncomfortable with the patch in general for the very
> reason that you state: it might become live again. Without the patch,
> the code remains safe without any extra effort. With this patch, even
> with the in-code comment, someone making changes needs to take special
> care. Sometimes it makes sense to leave safeties in place, even if
> they can't be triggered _today_; and safeties (such as i < 31) also
> serve as documentation.

That's also a valid argument.  I don't think this loop is going to 
appear on any trace profile either.

I personally have no strong opinion here.

Nicolas