On 27 October 2017 at 17:06, Pranit Bauva [off-list ref] wrote:

+static void free_terms(struct bisect_terms *terms)
+{
+       if (!terms->term_good)
+               free((void *) terms->term_good);
+       if (!terms->term_bad)
+               free((void *) terms->term_bad);
+}

These look like no-ops. Remove `!` for correctness, or `if (...)` for
simplicity, since `free()` can handle NULL.

You leave the pointers dangling, but you're ok for now since this is the
last thing that happens in `cmd_bisect__helper()`. Your later patches
add more users, but they're also ok, since they immediately assign new
values.

In case you (and others) find it useful, the below is a patch I've been
sitting on for a while as part of a series to plug various memory-leaks.
`FREE_AND_NULL_CONST()` would be useful in precisely these situations.

-- >8 --
Subject: git-compat-util: add FREE_AND_NULL_CONST() wrapper

Commit 481df65 ("git-compat-util: add a FREE_AND_NULL() wrapper around
free(ptr); ptr = NULL", 2017-06-15) added `FREE_AND_NULL()`. One
advantage of this macro is that it reduces risk of copy-paste errors and
reviewer-fatigue, especially when cleaning up more than just a single
pointer.

However, `FREE_AND_NULL()` can not be used with a const-pointer:

  struct foo { const char *bar; }
  ...
  FREE_AND_NULL(baz.bar);

This causes the compiler to barf as `free()` is called: "error: passing
argument 1 of ‘free’ discards ‘const’ qualifier from pointer target
type". A naive attempt to remedy this might look like this:

  FREE_AND_NULL((void *)baz.bar);

Now the assignment is problematic: "error: lvalue required as left
operand of assignment".

Add a `FREE_AND_NULL_CONST()` wrapper macro which acts as
`FREE_AND_NULL()`, except it casts to `(void *)` when freeing. As a
demonstration, use this in two existing code paths that were identified
by some grepping. Future patches will use it to clean up struct-fields:

  FREE_AND_NULL_CONST(baz.bar);

This macro is a slightly bigger hammer than `FREE_AND_NULL` and has a
slightly larger potential for misuse. Document that `FREE_AND_NULL`
should be preferred.

Signed-off-by: Martin Ågren <redacted>
---
 argv-array.c      | 3 +--
 git-compat-util.h | 8 ++++++++
 submodule.c       | 3 +--
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/argv-array.c b/argv-array.c
index 5d370fa33..433a5997a 100644
--- a/argv-array.c
+++ b/argv-array.c

@@ -59,8 +59,7 @@ void argv_array_pop(struct argv_array *array)
 {
 	if (!array->argc)
 		return;
-	free((char *)array->argv[array->argc - 1]);
-	array->argv[array->argc - 1] = NULL;
+	FREE_AND_NULL_CONST(array->argv[array->argc - 1]);
 	array->argc--;
 }
 

diff --git a/git-compat-util.h b/git-compat-util.h
index cedad4d58..ca3dcbc58 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -815,6 +815,14 @@ extern FILE *fopen_or_warn(const char *path, const char *mode);
  */
 #define FREE_AND_NULL(p) do { free(p); (p) = NULL; } while (0)
 
+/*
+ * FREE_AND_NULL_CONST(ptr) is like FREE_AND_NULL(ptr) except it casts
+ * to (void *) when calling free. This is useful when handling, e.g., a
+ * `const char *`, but it can also be misused. Prefer FREE_AND_NULL, and
+ * use this only when you need to and it is safe to do so.
+ */
+#define FREE_AND_NULL_CONST(p) do { free((void *)(p)); (p) = NULL; } while (0)
+
 #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))
 

diff --git a/submodule.c b/submodule.c
index 63e7094e1..7759f9050 100644
--- a/submodule.c
+++ b/submodule.c

@@ -364,8 +364,7 @@ int parse_submodule_update_strategy(const char *value,
 {
 	enum submodule_update_type type;
 
-	free((void*)dst->command);
-	dst->command = NULL;
+	FREE_AND_NULL_CONST(dst->command);
 
 	type = parse_submodule_update_type(value);
 	if (type == SM_UPDATE_UNSPECIFIED)

-- 
2.15.0.rc2.5.g97dd00bb7