Re: [PATCH 21/24] revisions API: release "reflog_info" in release revisions()
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Mar 10, 2022, 00:13 UTC
- Message-ID
- <220310.868rtilksy.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <99c429f7-af5d-78ac-9888-9f4f58d01b62@github.com>
On Wed, Mar 09 2022, Derrick Stolee wrote:
Show 30 quoted lines
> 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.
Show 6 quoted lines
>> - 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*
Show 9 quoted lines
>> +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...