Subject: Re: [PATCH 1/9] git_config_set: fix off-by-two

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Fri, 30 Mar 2018 16:15:34 +0200

On Fri, Mar 30 2018, Johannes Schindelin wrote:

> On Thu, 29 Mar 2018, Jeff King wrote:
>
>> On Thu, Mar 29, 2018 at 11:15:33AM -0700, Stefan Beller wrote:
>>
>> > > When calling `git config --unset abc.a` on this file, it leaves this
>> > > (invalid) config behind:
>> > >
>> > >         [
>> > >         [xyz]
>> > >                 key = value
>> > >
>> > > The reason is that we try to search for the beginning of the line (or
>> > > for the end of the preceding section header on the same line) that
>> > > defines abc.a, but as an optimization, we subtract 2 from the offset
>> > > pointing just after the definition before we call
>> > > find_beginning_of_line(). That function, however, *also* performs that
>> > > optimization and promptly fails to find the section header correctly.
>> >
>> > This commit message would be more convincing if we had it in test form.
>>
>> I agree a test might be nice. But I don't find the commit message
>> unconvincing at all. It explains pretty clearly why the bug occurs, and
>> you can verify it by looking at find_beginning_of_line.
>>
>> >     [abc]a
>> >
>> > is not written by Git, but would be written from an outside tool or person
>> > and we barely cope with it?
>>
>> Yes, I don't think git would ever write onto the same line. But clearly
>> we should handle anything that's syntactically valid.
>
> I was tempted to add the test case, because it is easy to test it.
>
> But I then decided *not* to add it. Why? Testing is a balance between "can
> do" and "need to do".
>
> Can you imagine that I did *not* run the entire test suite before
> submitting this patch series, because it takes an incredible *90 minutes*
> to run *on a fast Windows machine*?

I think if it's worth fixing it's worth testing for, a future change to
the config code could easily introduce a regression for this, and
particularly in this type of code obscure edge cases like this can point
to bugs elsewhere.

We have the EXPENSIVE_ON_WINDOWS prerequisite already in master from an
earlier series of mine, maybe we could use that here, or add some other
prereq like OVERLY_EXHAUSTIVE which by default could depend on
EXPENSIVE_ON_WINDOWS, i.e. we'd have a set of overly pedantic tests that
we skip on Windows by default, as there's no reason to suspect they're
platform-dependent, but we'd like to know if they regress.