| Message ID | 877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Coverity fixes | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > Reported by Coverity. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > builtin/submodule--helper.c | 2 ++ > 1 file changed, 2 insertions(+) > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 5c77dfcffee..d7b8004b933 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2512,6 +2512,8 @@ static int update_submodule(struct update_data *update_data) > > next.recursive_prefix = get_submodule_displaypath(prefixed_path, > update_data->prefix); > + free(prefixed_path); > + This function has two very similar code block that computes prefixed_path depending on the same condition, and one frees the variable correctly while the other one (i.e. this one) forgets to do so, which is irritating to see. Perhaps the whole "we have update_data structure, in which recursive_prefix, sm_path and prefix members in it; please set the displaypath member based on these values" should become a helper function, e.g. static const char *displaypath_from_update_data(struct update_data *u) { char *pp, *ret; if (u->recursive_prefix) pp = xstrfmt("%s%s", u->recursive_prefix, u->sm_path); else pp = xstrdup(u->sm_path); ret = get_submodule_displaypath(pp, u->prefix); free(pp); return ret; } to avoid duplicated computation. But the whole thing may become moot, as there seems to be a move to get rid of submodule--helper.c altogether? I'll refrain from touching this patch and instead redirect it to Glen; perhaps removal of submodule--helper.c involves moving the code here to another file or something, in which case it is far easier if I outsource that to somebody who is actually working on the file ;-) Thanks. > next.prefix = NULL; > oidcpy(&next.oid, null_oid()); > oidcpy(&next.suboid, null_oid());
Junio C Hamano <gitster@pobox.com> writes: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > >> From: Johannes Schindelin <johannes.schindelin@gmx.de> >> >> Reported by Coverity. >> >> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> >> --- >> builtin/submodule--helper.c | 2 ++ >> 1 file changed, 2 insertions(+) > > >> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c >> index 5c77dfcffee..d7b8004b933 100644 >> --- a/builtin/submodule--helper.c >> +++ b/builtin/submodule--helper.c >> @@ -2512,6 +2512,8 @@ static int update_submodule(struct update_data *update_data) >> >> next.recursive_prefix = get_submodule_displaypath(prefixed_path, >> update_data->prefix); >> + free(prefixed_path); >> + > > This function has two very similar code block that computes > prefixed_path depending on the same condition, and one frees the > variable correctly while the other one (i.e. this one) forgets to do > so, which is irritating to see. Good eye, I hadn't noticed this. In this case, we're computing the same thing twice in the function - first unconditionally, then conditionally. So we can actually just reuse the first result by doing something like next.recursive_prefix = xstrfmt("%s/", update_data->displaypath); (note that we need to append a "/" at the end) > Perhaps the whole "we have update_data structure, in which > recursive_prefix, sm_path and prefix members in it; please set the > displaypath member based on these values" should become a helper > function, e.g. > > static const char *displaypath_from_update_data(struct update_data *u) > { > char *pp, *ret; > > if (u->recursive_prefix) > pp = xstrfmt("%s%s", u->recursive_prefix, u->sm_path); > else > pp = xstrdup(u->sm_path); > > ret = get_submodule_displaypath(pp, u->prefix); > free(pp); > return ret; > } > > to avoid duplicated computation. Even better, I recently noticed that .recursive_prefix can probably just be replaced with "--super-prefix", and that will let us dispatch to get_submodule_displaypath() without this extra dance here. I'll flesh that out into a series and send it soon. > But the whole thing may become moot, as there seems to be a move to > get rid of submodule--helper.c altogether? > > > I'll refrain from touching this patch and instead redirect it to > Glen; perhaps removal of submodule--helper.c involves moving the > code here to another file or something, in which case it is far > easier if I outsource that to somebody who is actually working on > the file ;-) cc-ed Ævar, who knows even more than I do :) git-submodule.sh is definitely going away soon \o/ but I don't think we have plans to get rid of submodule--helper.c just yet, so I think we should probably still keep this patch. > > Thanks. > >> next.prefix = NULL; >> oidcpy(&next.oid, null_oid()); >> oidcpy(&next.suboid, null_oid());
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 5c77dfcffee..d7b8004b933 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2512,6 +2512,8 @@ static int update_submodule(struct update_data *update_data) next.recursive_prefix = get_submodule_displaypath(prefixed_path, update_data->prefix); + free(prefixed_path); + next.prefix = NULL; oidcpy(&next.oid, null_oid()); oidcpy(&next.suboid, null_oid());