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.

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

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

> 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!
Take the risk of thinking for yourself.  Much more happiness,
truth, beauty and wisdom will come to you that way.

-Christopher Hitchens
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to