Re: [PATCH] Prevent git-config from storing section keys that are too long

[Junio C Hamano <gitster@pobox.com>]

Ben Walton <bwalton@artsci.utoronto.ca> writes:

> Key names have a length limit defined by MAXNAME in config.c.  When
> reading the config file, we reserve half of this limit for the section
> identifier and the other half for the key name within that section.
>
> For example, if setting a key named url.foo.insteadOf, url.foo may use
> at most half of MANXNAME.
>
> The parser will throw an error if this condition is violated.
>
> This patch ensures that git-config enforces the same restriction
> during the creation of a section identifier so that it doesn't allow
> the generate a configuration file that cannot be re-read later.
>
> This patch also adds a test to t1303-wacky-config to catch any future
> issues with this check.
>
> Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
> ---
>
> Hi All,
>
> I happened to notice this while running the test suite in a deeply
> nested directory...
>
> The check for baselen exceeding half of MAXNAME could be done earlier
> in the function but doing it late allowed the error message to be
> clearer without extra hassle.
>
> I also wonder if MAXNAME should be increased somewhat.  Section
> identifiers generated from keys like:
>
> url./some/really/long/path.insteadOf
>
> could overrun the current limit.  It's not a common case, of course,
> or this issue would have been found sooner.  Would doubling the
> current limit be out of the question?

Is there a reason to have _any_ limitation?  It is not like we store
configuration data by allocating one file per item (in which case we
may be limited by the filesystem limit for direntry size), so if it
is not too much trouble, I think the right thing to do is to lift
the limitation altogether, possibly using strbuf instead of a
statically sized array of characters.

Of course, once you write a very long entry using a newer version of
Git, the resulting configuration file may end up unusable by older
version of Git, so a patch to implement such a change may need to be
based on older maintenance release (say maint-1.7.9) and then merged
upwards, but otherwise I do not offhand see a compatibility downside
of such a change.