| Message ID | patch-v5-14.27-ddc7402b054-20220402T102002Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | revision.[ch]: add and use release_revisions() | expand |
Hi Ævar [continuing with v5 from where I left off with v4] On 02/04/2022 11:49, Ævar Arnfjörð Bjarmason wrote: > Use a release_revisions() with those "struct rev_list" users which > already "UNLEAK" the struct. It may seem odd to simultaneously attempt > to free() memory, but also to explicitly ignore whether we have memory > leaks in the same. > > As explained in preceding commits this is being done to use the > built-in commands as a guinea pig for whether the release_revisions() > function works as expected, we'd like to test e.g. whether we segfault > as we change it. In subsequent commits we'll then remove these > UNLEAK() as the function is made to free the memory that caused us to > add them in the first place. I'm a bit confused by this, the previous commit argued in favor of removing UNLEAK() so would could see the leaks and fix them, this is saying we should hide the leaks. Best Wishes Phillip > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/diff-index.c | 4 +++- > builtin/diff.c | 1 + > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/builtin/diff-index.c b/builtin/diff-index.c > index 5fd23ab5b6c..3a83183c312 100644 > --- a/builtin/diff-index.c > +++ b/builtin/diff-index.c > @@ -71,5 +71,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) > } > result = run_diff_index(&rev, option); > UNLEAK(rev); > - return diff_result_code(&rev.diffopt, result); > + result = diff_result_code(&rev.diffopt, result); > + release_revisions(&rev); > + return result; > } > diff --git a/builtin/diff.c b/builtin/diff.c > index bb7fafca618..dd48336da56 100644 > --- a/builtin/diff.c > +++ b/builtin/diff.c > @@ -595,6 +595,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) > if (1 < rev.diffopt.skip_stat_unmatch) > refresh_index_quietly(); > UNLEAK(rev); > + release_revisions(&rev); > UNLEAK(ent); > UNLEAK(blob); > return result;
On Sun, Apr 03 2022, Phillip Wood wrote: > Hi Ævar > > [continuing with v5 from where I left off with v4] > > On 02/04/2022 11:49, Ævar Arnfjörð Bjarmason wrote: >> Use a release_revisions() with those "struct rev_list" users which >> already "UNLEAK" the struct. It may seem odd to simultaneously attempt >> to free() memory, but also to explicitly ignore whether we have memory >> leaks in the same. >> As explained in preceding commits this is being done to use the >> built-in commands as a guinea pig for whether the release_revisions() >> function works as expected, we'd like to test e.g. whether we segfault >> as we change it. In subsequent commits we'll then remove these >> UNLEAK() as the function is made to free the memory that caused us to >> add them in the first place. > > I'm a bit confused by this, the previous commit argued in favor of > removing UNLEAK() so would could see the leaks and fix them, this is > saying we should hide the leaks. ..until the UNLEAK() is removed later in the sier.s All commits in this series pass with the SANITIZE=leak CI check. In this case it would result in too much churn to remove the UNLEAK() before we have this solving some of the more meaningful leaks, and if we do this later we don't have all codepaths that can use release_revisions() using it as we free things one at a time. > Best Wishes > > Phillip > >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> builtin/diff-index.c | 4 +++- >> builtin/diff.c | 1 + >> 2 files changed, 4 insertions(+), 1 deletion(-) >> diff --git a/builtin/diff-index.c b/builtin/diff-index.c >> index 5fd23ab5b6c..3a83183c312 100644 >> --- a/builtin/diff-index.c >> +++ b/builtin/diff-index.c >> @@ -71,5 +71,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) >> } >> result = run_diff_index(&rev, option); >> UNLEAK(rev); >> - return diff_result_code(&rev.diffopt, result); >> + result = diff_result_code(&rev.diffopt, result); >> + release_revisions(&rev); >> + return result; >> } >> diff --git a/builtin/diff.c b/builtin/diff.c >> index bb7fafca618..dd48336da56 100644 >> --- a/builtin/diff.c >> +++ b/builtin/diff.c >> @@ -595,6 +595,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) >> if (1 < rev.diffopt.skip_stat_unmatch) >> refresh_index_quietly(); >> UNLEAK(rev); >> + release_revisions(&rev); >> UNLEAK(ent); >> UNLEAK(blob); >> return result;
diff --git a/builtin/diff-index.c b/builtin/diff-index.c index 5fd23ab5b6c..3a83183c312 100644 --- a/builtin/diff-index.c +++ b/builtin/diff-index.c @@ -71,5 +71,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) } result = run_diff_index(&rev, option); UNLEAK(rev); - return diff_result_code(&rev.diffopt, result); + result = diff_result_code(&rev.diffopt, result); + release_revisions(&rev); + return result; } diff --git a/builtin/diff.c b/builtin/diff.c index bb7fafca618..dd48336da56 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -595,6 +595,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) if (1 < rev.diffopt.skip_stat_unmatch) refresh_index_quietly(); UNLEAK(rev); + release_revisions(&rev); UNLEAK(ent); UNLEAK(blob); return result;
Use a release_revisions() with those "struct rev_list" users which already "UNLEAK" the struct. It may seem odd to simultaneously attempt to free() memory, but also to explicitly ignore whether we have memory leaks in the same. As explained in preceding commits this is being done to use the built-in commands as a guinea pig for whether the release_revisions() function works as expected, we'd like to test e.g. whether we segfault as we change it. In subsequent commits we'll then remove these UNLEAK() as the function is made to free the memory that caused us to add them in the first place. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/diff-index.c | 4 +++- builtin/diff.c | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)