Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> Fix a memory leak introduced in e83e3333b57 (submodule: port submodule
> subcommand 'summary' from shell to C, 2020-08-13), to do that stop
> juggling around the "errmsg" and "struct strbuf", let's instead move
> the "struct strbuf errmsg" to the top-level.
>
> Now we don't need to strbuf_detach() it anymore, but we do need to
> ensure that we pass NULL to print_submodule_summary() when we have no
> error message.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
> builtin/submodule--helper.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a964dbeec38..a05578a7382 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -932,7 +932,8 @@ static void generate_submodule_summary(struct summary_cb *info,
> {
> char *displaypath, *src_abbrev = NULL, *dst_abbrev;
> int missing_src = 0, missing_dst = 0;
> - char *errmsg = NULL;
> + char *errmsg;
> + struct strbuf errmsg_str = STRBUF_INIT;
> int total_commits = -1;
>
> if (!info->cached && oideq(&p->oid_dst, null_oid())) {
> @@ -1032,7 +1033,6 @@ static void generate_submodule_summary(struct summary_cb *info,
> * submodule, i.e., deleted or changed to blob
> */
> if (S_ISGITLINK(p->mod_dst)) {
> - struct strbuf errmsg_str = STRBUF_INIT;
> if (missing_src && missing_dst) {
> strbuf_addf(&errmsg_str, " Warn: %s doesn't contain commits %s and %s\n",
> displaypath, oid_to_hex(&p->oid_src),
> @@ -1043,10 +1043,10 @@ static void generate_submodule_summary(struct summary_cb *info,
> oid_to_hex(&p->oid_src) :
> oid_to_hex(&p->oid_dst));
> }
> - errmsg = strbuf_detach(&errmsg_str, NULL);
> }
> }
>
> + errmsg = errmsg_str.len ? errmsg_str.buf : NULL;
> print_submodule_summary(info, errmsg, total_commits,
> displaypath, src_abbrev,
> dst_abbrev, p);
> @@ -1054,6 +1054,7 @@ static void generate_submodule_summary(struct summary_cb *info,
> free(displaypath);
> free(src_abbrev);
> free(dst_abbrev);
> + strbuf_release(&errmsg_str);
> }
>
> static void prepare_submodule_summary(struct summary_cb *info,
> --
> 2.37.0.932.g7b7031e73bc
What do you think of getting rid of "char *errmsg" altogether? We can
replace it with errmsg_str.buf and do the length check in
print_submodule_summary():
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a964dbeec3..f9f0de7e83 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -896,7 +896,7 @@ static void print_submodule_summary(struct summary_cb *info, char *errmsg,
else
printf(" (%d):\n", total_commits);
- if (errmsg) {
+ if (errmsg && *errmsg) {
printf(_("%s"), errmsg);
} else if (total_commits > 0) {
struct child_process cp_log = CHILD_PROCESS_INIT;
@@ -932,7 +932,7 @@ static void generate_submodule_summary(struct summary_cb *info,
{
char *displaypath, *src_abbrev = NULL, *dst_abbrev;
int missing_src = 0, missing_dst = 0;
- char *errmsg = NULL;
+ struct strbuf errmsg_str = STRBUF_INIT;
int total_commits = -1;
if (!info->cached && oideq(&p->oid_dst, null_oid())) {
@@ -1032,7 +1032,6 @@ static void generate_submodule_summary(struct summary_cb *info,
* submodule, i.e., deleted or changed to blob
*/
if (S_ISGITLINK(p->mod_dst)) {
- struct strbuf errmsg_str = STRBUF_INIT;
if (missing_src && missing_dst) {
strbuf_addf(&errmsg_str, " Warn: %s doesn't contain commits %s and %s\n",
displaypath, oid_to_hex(&p->oid_src),
@@ -1043,17 +1042,17 @@ static void generate_submodule_summary(struct summary_cb *info,
oid_to_hex(&p->oid_src) :
oid_to_hex(&p->oid_dst));
}
- errmsg = strbuf_detach(&errmsg_str, NULL);
}
}
- print_submodule_summary(info, errmsg, total_commits,
+ print_submodule_summary(info, errmsg_str.buf, total_commits,
displaypath, src_abbrev,
dst_abbrev, p);
free(displaypath);
free(src_abbrev);
free(dst_abbrev);
+ strbuf_release(&errmsg_str);
}
static void prepare_submodule_summary(struct summary_cb *info,
