Hi Ævar
On 14/10/2021 20:54, Ævar Arnfjörð Bjarmason wrote:
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.:
Oh, I think I was thinking of -Wwrite-strings but we don't have that
warning on and turning it on causes a bunch of -Wdiscarded-qualifier
warnings.
Best Wishes
Phillip
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...
