Re: [PATCHv2 4/4] unpack-trees: support super-prefix option

From: Stefan Beller <sbeller@google.com>
Date: Jan 12, 2017, 22:19 UTC
Message-ID: <CAGZ79kZWYytJbXJZ=qeStuSe3tZ3qiCZZ-01Y0Ud9TcqsorjMw@mail.gmail.com>
In-Reply-To: <xmqqtw94hs8f.fsf@gitster.mtv.corp.google.com>

On Thu, Jan 12, 2017 at 1:40 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 24 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> This is only patchv4 that is rerolled, patches 1-3 remain as is.
>
> Good timing, as I was about to send a reminder to suggest rerolling
> 4/4 alone ;-)
>
>> +static const char *super_prefixed(const char *path)
>> +{
>
> There used to be a comment that explains why we keep two static
> buffers around here.  Even though it is in the log message, the
> in-code comment would save people trouble of having to go through
> "git blame" output.
>
> I'd say something like
>
>         /*
>          * It is necessary and sufficient to have two static buffers
>          * as the return value of this function is fed to error()
>          * using the unpack_*_errors[] templates we can see above.
>          */
>
> perhaps.

If you think it helps, I can reroll with such a comment. At the time of fixing up for v4 I felt like it is overly verbose. Such a comment is only useful in understanding the choice of 2. I thought it was sufficient in the log as once you're interested in that function you'd read the output of blame anyway.

On the other hand having statically allocated arrays of fixed size is dangerous in terms of maintainability, i.e. down the road someone thinks this is a good function to reuse and then they may run into subtle bugs as they will not be aware of the internally static buffer that is overwritten after a certain time.

Show 9 quoted lines

>
>> +     static struct strbuf buf[2] = {STRBUF_INIT, STRBUF_INIT};
>> +     static int super_prefix_len = -1;
>> +     static unsigned idx = 0;
>> +
>
> If we initialize this to 1 (or even better, "ARRAY_SIZE(buf) - 1"),
> then we would use buf[0] first and then buf[1], which feels more
> natural to me.

Yes I agree, though I overcomplicating things just so that they feel more natural seems wrong as well.

At the time I assumed that having a static variable initialized to 0 was slightly cheaper, as the init code just memsets all of .bss to 0 unlike the .data segment that has to be crafted manually. But to get that variable into the .bss section reliably we'd have to omit the "=0". (My compiler recognized that it can be put into .bss because it is 0)

>
> Other than that, this looks OK.  Will queue.
>
> Thanks.

Thanks, Stefan