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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 7, 2012, 01:33 UTC
Message-ID: <7vpq5yzkf3.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1346978829-4486-1-git-send-email-bwalton@artsci.utoronto.ca>

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

Show 36 quoted lines

> 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.