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

From: Paul Tan <pyokagan@gmail.com>
Date: Jun 19, 2015, 16:15 UTC
Message-ID: <CACRoPnQdR7qC6gH7j9_8gQUYKShLRQCr6GNtmn+XCpe9P4P-nA@mail.gmail.com>
In-Reply-To: <CAPc5daVbpB_T4cY1xvLrBKPUZw0JNMXqNAOsKE-R7NPO2nrnZA@mail.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?

Show 9 quoted lines

> 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