Re: [PATCH] Remove the hard coded length limit on variable names in config files

From: Ben Walton <bdwalton@gmail.com>
Date: Sep 30, 2012, 18:20 UTC
Message-ID: <CAP30j15wKa7wbLyaLo8omHcAh5u7O=Yt8Tjy4bD3V_0nT1zJPQ@mail.gmail.com>
In-Reply-To: <5067C51F.6020908@alum.mit.edu>

Hi Michael,

> The patch doesn't apply to the current master; it appears to have been
> built against master 883a2a3504 (2012-02-23) or older.  It will have to
> be rebased to the current master.

Junio had asked that it be based on maint so that's what I (thought I?) did. I'm happy to redo it against master if that's better though.

Show 6 quoted lines

> The preferred format for multiline comments in the git project is
>
>     /*
>      * Truncate the var name back to the section header prior to
>      * grabbing the suffix part of the name and the value.
>      */

Oops; Will fix.

Show 13 quoted lines

> In the old code, get_base_var() read the string into var and returned
> var's length (or -1 on error).  The fact that the length of var was
> first "reset" to zero is somewhat implicit in the fact that no length
> parameter is being passed to get_base_var().
>
> But in the new version, get_base_var() is passed a strbuf.  Often,
> operations with strbufs append to the strbuf, and this is what I first
> assumed.  It took me a while to realize that get_base_var() calls
> strbuf_reset() before getting to work.  Moreover, get_base_var() still
> returns the length of what it found, which is redundant with a strbuf
> and therefore unexpected.  So when the return value of get_base_var() is
> stored into baselen, it is not really obvious that it is the string's
> length.

Ok, that's a fair criticism. When I was creating the patch, I thought that placing the strbuf_reset in get_base_var() seemed nicer as it matched the baselen = 0 which effectively reset the old character array. Your point is well taken though and I think it makes sense to switch things around the way you've suggested.

Show 8 quoted lines

> Finally, I realize that the MAXNAME constant was not exported and I
> can't find the old length limits documented anywhere, but I nevertheless
> worry a little bit that one of the users of the config API has a
> built-in assumption that names can never be longer than 256 characters
> (for example, a config_fn_t function might try to store the name into a
> fixed-length buffer).  Hopefully such code would never have been written
> or accepted, but...?  If you have thought about this or audited the
> callers, please mention that in your commit message.

I did look through the code to see if anything was relying on fixed size buffers and I didn't see anything. I'll update the commit message with this info too.

I'll send a modified patch shortly.

Thanks for the review! -Ben

-- 
---------------------------------------------------------------------------------------------------------------------------
Take the risk of thinking for yourself.  Much more happiness,
truth, beauty and wisdom will come to you that way.

-Christopher Hitchens
---------------------------------------------------------------------------------------------------------------------------