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! -Ben -- --------------------------------------------------------------------------------------------------------------------------- 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html