On Fri, Jun 19, 2015 at 11:09 PM, Junio C Hamano <gits...@pobox.com> wrote: > You do realize that strbuf internally does alloc/free so as a solution to > fragmentation issue you are at the mercy of the same alloc/free, don't you?
Yes, of course, but it has the "alloc" variable to keep track of the size of the allocated buffer, and provided that ALLOC_GROW() uses a sensible factor to grow the buffer, it won't be calling malloc/free that much to be a problem. Of course, we could do the same and just add a "alloc" variable as well and then use it with strbuf_attach()/strbuf_detach(), but at this point why not just store it in an strbuf then and avoid the extra "alloc" struct member? > The primary thing I care about is to discourage callers of the API element > am_state from touching these fields with strbuf functions. If it is char * > then > the would think twice before modifying (which would involve allocating the > new buffer, forming the new value and assigning into it). With the fields left > as strbuf after they are read or formed by the API (by reading by the API > implementation from $GIT_DIR/rebase-apply/), it is too tempting to do > strbuf_add(), strbuf_trim(), etc., without restoring the value to the original > saying "I'm the last user of it anyway"--that is the sloppiness we can prevent > by not leaving it as strbuf. I don't think this is a good deterrent. If the code wanted to, it could just use strbuf_attach()/strbuf_detach() as well, no? I believe this kind of issue would be better solved with a big WARNING comment and code review. Also, there may be use cases where the user may wish to modify the commit message/author fields. E.g., user may wish to modify the message of the commit that results from am --continue to take into account the conflicts that caused the am to fail. Regards, Paul -- To unsubscribe from this list: send the line "unsubscribe git" in
