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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxx/
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...
