Re: [PATCH v4 6/6] fetch: delay user information post committing of transaction

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

Karthik Nayak <karthik.188@gmail.com> writes:

>>> +static void ref_update_display_info_free(struct ref_update_display_info *info)
>>> +{
>>> +	free((char *)info->summary);
>>> +	free((char *)info->success_detail);
>>> +	free((char *)info->fail_detail);
>>> +	free((char *)info->remote);
>>> +	free((char *)info->ref);
>>> +}
>>
>> This answers "no" to my previous question.  These are not borrowed,
>> but are owned by this structure.
>>
>
> Yup, cannot be borrowed, since those go out of scope much earlier.

And the reason why they are marked "const char *" which typically
signals that they are borrowed is?  After all, that is where these
casts inside free() comes from.

There are two schools of thought.  One (which I originally was in)
marks resources we own with "const", if these members will not
change once we initialize them and we want to avoid accidentally
muck with the contents of these pieces of memory during the course
of the program.  Those of us in the school often have to cast away
constness in their calls to free() like the above.

But I saw many of our developers squarely fall into the other camp,
where they always use a non-const pointer to point at the resource
the structure owns.

The latter school of thought opens us up to bugs caused by mistaken
code that modifies these memory regions that those of us in the
former school would use "const" to avoid, but it makes it easier to
reason about memory ownership models by signalling if the enclosing
structure owns or borrows the resources.

I'd say the latter school are majority of our developer base, and a
lot of existing structures follow that rule.  I was hinting that we
may want to follow suit in this new structure.

Thanks.