Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> From: Glen Choo <chooglen@xxxxxxxxxx>
>
> Refactor code added in e83e3333b57 (submodule: port submodule
> subcommand 'summary' from shell to C, 2020-08-13) so that "errmsg" and
> "errmsg_str" are folded into one. The distinction between the empty
> string and NULL is something that's tested for by
> e.g. "t/t7401-submodule-summary.sh".
>
> This is in preparation for fixing a memory leak the "struct strbuf" in
> the pre-image.
>
> Let's also pass a "const char *" to print_submodule_summary(), as it
> should not be modifying the "errmsg".
>
> Signed-off-by: Glen Choo <chooglen@xxxxxxxxxx>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
> builtin/submodule--helper.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 21b3abb7b40..f794d2b588b 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -866,7 +866,7 @@ static char *verify_submodule_committish(const char *sm_path,
> return strbuf_detach(&result, NULL);
> }
>
> -static void print_submodule_summary(struct summary_cb *info, char *errmsg,
> +static void print_submodule_summary(struct summary_cb *info, const char *errmsg,
> int total_commits, const char *displaypath,
> const char *src_abbrev, const char *dst_abbrev,
> struct module_cb *p)
> @@ -924,7 +924,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 = STRBUF_INIT;
> int total_commits = -1;
>
> if (!info->cached && oideq(&p->oid_dst, null_oid())) {
> @@ -1024,23 +1024,21 @@ 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",
> + strbuf_addf(&errmsg, " Warn: %s doesn't contain commits %s and %s\n",
> displaypath, oid_to_hex(&p->oid_src),
> oid_to_hex(&p->oid_dst));
> } else {
> - strbuf_addf(&errmsg_str, " Warn: %s doesn't contain commit %s\n",
> + strbuf_addf(&errmsg, " Warn: %s doesn't contain commit %s\n",
> displaypath, missing_src ?
> 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,
> - displaypath, src_abbrev,
> + print_submodule_summary(info, errmsg.len ? errmsg.buf : NULL,
> + total_commits, displaypath, src_abbrev,
> dst_abbrev, p);
>
> free(displaypath);
Ah, this is mostly the same as what I sent out, but with the length
check in the same function (instead of in print_submodule_summary()).
This makes it easier to tell that the caller is still doing the same
thing.
Looks good.
