| Message ID | df5f8305-79d5-2c12-bdf0-961428c0bdd1@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] diff-no-index: release strbuf on queue error | expand |
Hi René, On Tue, 6 Sep 2022, René Scharfe wrote: > diff --git a/diff-no-index.c b/diff-no-index.c > index a3683d8a04..35809f26d7 100644 > --- a/diff-no-index.c > +++ b/diff-no-index.c > @@ -245,6 +245,7 @@ int diff_no_index(struct rev_info *revs, > int i, no_index; > int ret = 1; > const char *paths[2]; > + char *to_free[2] = { 0 }; > struct strbuf replacement = STRBUF_INIT; > const char *prefix = revs->prefix; > struct option no_index_options[] = { > @@ -274,7 +275,7 @@ int diff_no_index(struct rev_info *revs, > */ > p = file_from_standard_input; > else if (prefix) > - p = prefix_filename(prefix, p); > + p = to_free[i] = prefix_filename(prefix, p); > paths[i] = p; > } > > @@ -308,6 +309,8 @@ int diff_no_index(struct rev_info *revs, > ret = diff_result_code(&revs->diffopt, 0); > > out: > + for (i = 0; i < 2; i++) > + free(to_free[i]); Heh. That's long-hand for free(to_free[0]); free(to_free[1]); If you do want to have that loop, please replace the hard-coded 2 by `ARRAY_SIZE(to_free)`. Otherwise, both patches look fine to me. Thanks! Dscho > strbuf_release(&replacement); > return ret; > } > -- > 2.37.2 >
Am 07.09.22 um 12:03 schrieb Johannes Schindelin: > Hi René, > > On Tue, 6 Sep 2022, René Scharfe wrote: > >> diff --git a/diff-no-index.c b/diff-no-index.c >> index a3683d8a04..35809f26d7 100644 >> --- a/diff-no-index.c >> +++ b/diff-no-index.c >> @@ -245,6 +245,7 @@ int diff_no_index(struct rev_info *revs, >> int i, no_index; >> int ret = 1; >> const char *paths[2]; >> + char *to_free[2] = { 0 }; >> struct strbuf replacement = STRBUF_INIT; >> const char *prefix = revs->prefix; >> struct option no_index_options[] = { >> @@ -274,7 +275,7 @@ int diff_no_index(struct rev_info *revs, >> */ >> p = file_from_standard_input; >> else if (prefix) >> - p = prefix_filename(prefix, p); >> + p = to_free[i] = prefix_filename(prefix, p); >> paths[i] = p; >> } >> >> @@ -308,6 +309,8 @@ int diff_no_index(struct rev_info *revs, >> ret = diff_result_code(&revs->diffopt, 0); >> >> out: >> + for (i = 0; i < 2; i++) >> + free(to_free[i]); > > Heh. That's long-hand for > > free(to_free[0]); > free(to_free[1]); Had that before, but it's repetitive and more importantly this loop matches the first one. > If you do want to have that loop, please replace the hard-coded 2 by > `ARRAY_SIZE(to_free)`. The two is hard-coded in other places explicitly as well and implied in fixup_paths(). The root cause is not any array size but the design decision to require exactly two things to compare. A reader would need to know that. We could sure use ARRAY_SIZE(paths) in the declaration of to_free and ARRAY_SIZE(to_free) in the loop to at least not add more instances of that magic number and make the code understandable without seeing the bigger picture. > Otherwise, both patches look fine to me. > > Thanks! > Dscho > >> strbuf_release(&replacement); >> return ret; >> } >> -- >> 2.37.2 >>
diff --git a/diff-no-index.c b/diff-no-index.c index a3683d8a04..35809f26d7 100644 --- a/diff-no-index.c +++ b/diff-no-index.c @@ -245,6 +245,7 @@ int diff_no_index(struct rev_info *revs, int i, no_index; int ret = 1; const char *paths[2]; + char *to_free[2] = { 0 }; struct strbuf replacement = STRBUF_INIT; const char *prefix = revs->prefix; struct option no_index_options[] = { @@ -274,7 +275,7 @@ int diff_no_index(struct rev_info *revs, */ p = file_from_standard_input; else if (prefix) - p = prefix_filename(prefix, p); + p = to_free[i] = prefix_filename(prefix, p); paths[i] = p; } @@ -308,6 +309,8 @@ int diff_no_index(struct rev_info *revs, ret = diff_result_code(&revs->diffopt, 0); out: + for (i = 0; i < 2; i++) + free(to_free[i]); strbuf_release(&replacement); return ret; }
Callers of prefix_filename() are responsible for freeing its result. Remember them and release them to appease leak checkers. Signed-off-by: René Scharfe <l.s.r@web.de> --- diff-no-index.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) -- 2.37.2