Re: [PATCH/WIP v3 07/31] am: extract patch, message and authorship with git-mailinfo

[Paul Tan <pyokagan@gmail.com>]

On Fri, Jun 19, 2015 at 11:09 PM, Junio C Hamano <gitster@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