On Thu, Oct 14 2021, Phillip Wood wrote:
[Changed $subject]
> On 14/10/2021 01:10, Ævar Arnfjörð Bjarmason wrote:
>> The "checkout" command is one of the main sources of leaks in the test
>> suite, let's fix the common ones by not leaking from the "struct
>> branch_info".
>> Doing this is rather straightforward, albeit verbose, we need to
>> xstrdup() constant strings going into the struct, and free() the ones
>> we clobber as we go along.
>
> It's great to see these leaks being fixed. I wonder though if it would
> be better to change the structure definition so that 'name' and 'path'
> are no longer 'const'. That would be a better reflection of the new
> regime.[...]
I think this is the right thing to do, but I'm not quite sure. There was
a thread at it here:
https://lore.kernel.org/git/YUZG0D5ayEWd7MLP@xxxxxxxxxxxxxx/
Where I chimed in and suggested exactly what you're saying here, but the
consensus seemed to go the other way, and if you grep:
git grep -F 'free((char *)'
You can see that we use this pattern pretty widely.
> It would also mean we could lose all the casts when freeing
> and there would be a compiler warning if a string literal is assigned
> to one of those fields.
What compiler/set of warnings gives you a warning when you do that? I
don't get warned on e.g.:
diff --git a/builtin/checkout.c b/builtin/checkout.c
index a32af16d5e4..d7053579bdf 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -94 +94 @@ struct branch_info {
- const char *name; /* The short name used */
+ char *name; /* The short name used */
@@ -110 +110 @@ static void branch_info_release(struct branch_info *info)
- free((char *)info->name);
+ free(info->name);
@@ -1107 +1107 @@ static int switch_branches(const struct checkout_opts *opts,
- new_branch_info->name = xstrdup("(empty)");
+ new_branch_info->name = "(empty)";
Now, what is really useful is making it a "char * const", especially
when hacking up these changes as you'll find all the assignments, but I
haven't found the general use in having that make it to a submitted
patch, since you need to assign somewhere, and those then need to be a
str[n]cpy() (except we banned.h it) or memcpy() with a cast...
