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

[Junio C Hamano <gitster@pobox.com>]

Paul Tan <pyokagan@gmail.com> writes:

> On Fri, Jun 19, 2015 at 5:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Paul Tan <pyokagan@gmail.com> writes:
>>
>>> +     /* commit message and metadata */
>>> +     struct strbuf author_name;
>>> +     struct strbuf author_email;
>>> +     struct strbuf author_date;
>>> +     struct strbuf msg;
>>
>> Same comment as "dir" in the earlier patch applies to these.  If the
>> fields are read or computed and then kept constant, use a temporary
>> variable that is a strbuf to read/compute the final value, and then
>> detach to a "const char *" field.  If they are constantly changing
>> and in-place updates are vital, then they can and should be strbufs,
>> but I do not think that is the case for these.
>
> I do think it is the case here. The commit message and metadata fields
> change for every patch we process, and we could be processing a large
> volume of patches, so we must be careful to not leak any memory.

With the above fields, it is clear that the above fields are
per-message thing.  So the loop to process multiple messages is
conceptually:


	set up the entire am_state (for things like cur=1, last=N)
        for each message {
		update per-message fields like cur, author, msg, etc.
		process the single message
                clean up per-message fileds like cur++, free(msg), etc.
	}
	tear down the entire am_state

Reusing the same strbuf and rely on them to clean up after
themselves is simply a bad taste.

More importantly, we'd want to make sure that people who will be
touching the "process the single message" part in the above loop to
think twice before mucking with read-only fields like author.

If they are "char *", they would need to allocate new storage
themselves, format a new value into there, free the original, and
replace the field with the new value.  It takes a conscious effort
and very much visible code and would be clear to reviewers what is
going on, and gives them a chance to question if it is a good idea
to update things in-place in the first place.

If you left it in strbuf, that invites "let's extend it temporarily
with strbuf_add() and then return to the original once I am done
with this single step", which is an entry to a slippery slope to
"let's extend it with strbuf_add() for my purpose, and I do not even
bother to clean up because I know I am the last person to touch
this".

So, no, please don't leave a field that won't change during the bulk
of the processing in strbuf, unless you have a compelling reason to
do so (and "compelling reasons" are pretty much limited to "after
all, that field we originally thought it won't change is something
we need to change very often").

Thanks.