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

On Wed, Mar 09 2022, Derrick Stolee wrote:

On 3/9/2022 8:16 AM, Ævar Arnfjörð Bjarmason wrote:

quoted

Add a missing reflog_walk_info_release() to "reflog-walk.c" and use it
in release_revisions().

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 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/ (local)
and https://lore.kernel.org/git/xmqqlf2vbbl8.fsf@gitster.g/ (local)

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.

quoted

-	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*

quoted

+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...