Re: [PATCH 21/24] revisions API: release "reflog_info" in release revisions()

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Mar 09 2022, Derrick Stolee wrote:

> On 3/9/2022 8:16 AM, Ævar Arnfjörð Bjarmason wrote:
>> Add a missing reflog_walk_info_release() to "reflog-walk.c" and use it
>> in release_revisions().
>> 
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  reflog-walk.c            | 26 ++++++++++++++++++++++++--
>>  reflog-walk.h            |  1 +
>>  revision.c               |  1 +
>>  t/t0100-previous.sh      |  1 +
>>  t/t1401-symbolic-ref.sh  |  2 ++
>>  t/t1411-reflog-show.sh   |  1 +
>>  t/t1412-reflog-loop.sh   |  2 ++
>>  t/t1415-worktree-refs.sh |  1 +
>>  8 files changed, 33 insertions(+), 2 deletions(-)
>> 
>> diff --git a/reflog-walk.c b/reflog-walk.c
>> index 8ac4b284b6b..4322228d122 100644
>> --- a/reflog-walk.c
>> +++ b/reflog-walk.c
>> @@ -7,7 +7,7 @@
>>  #include "reflog-walk.h"
>>  
>>  struct complete_reflogs {
>> -	char *ref;
>> +	const char *ref;
>>  	const char *short_ref;
>
> This seems like the opposite change from what I would
> expect, because the 'const' implies non-ownership.

Yes, I'll change it.

FWIW we've had recent discussions on this point & it's a bit
context-dependant. See
https://lore.kernel.org/git/patch-v2-1.1-e2a166a9733-20211021T201541Z-avarab@gmail.com/
and https://lore.kernel.org/git/xmqqlf2vbbl8.fsf@gitster.g/

I.e. the preferred pattern is:

 * Make it "char *" if it's your own private struct, because it's non-const
 * If it's a "public API" and it's not really "const char *", but we
   don't want the API user to think they can fiddle with it, make it
   "const char *" and cast it in the free().

In this case I think I just mixed those two up, or maybe I initially
wrote it before that was clarified. In this case it's our own private
struct within this file.

>> -	free(array->ref);
>> +	free((char *)array->ref);
>> +	free((char *)array->short_ref);
>
> This further makes the point that we should be keeping
> non-const versions so we can clearly document ownership.

*nod*

>> +static void complete_reflogs_clear(void *util, const char *str)
>> +{
>> +	struct complete_reflogs *array = util;
>> +	free_complete_reflog(array);
>> +}
>
> Is there a reason we don't do the cast inside?
>
> 	free_complete_reflog((struct complete_reflogs *)util);

I think we generally do that more often that not (although I should add
an extra \n) there. I.e. to immediately cast the void* into a variable.

I also find it handy when e.g. stepping through in a debugger, because
you'll have a variable you can inspect without de-referencing it every
time, and if the function uses "array" for something else in the future
it'll be less verbosity on the second use...