On Mon, Jul 18 2022, Jeff King wrote: > On Wed, Jul 13, 2022 at 03:10:33PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> diff --git a/builtin/log.c b/builtin/log.c >> index e0f40798d45..77ec256a8ae 100644 >> --- a/builtin/log.c >> +++ b/builtin/log.c >> @@ -747,6 +747,8 @@ int cmd_show(int argc, const char **argv, const char *prefix) >> memcpy(&rev.pending, &blank, sizeof(rev.pending)); >> add_object_array(o, name, &rev.pending); >> ret = cmd_log_walk_no_free(&rev); >> + object_array_clear(&rev.pending); >> + memcpy(&rev.pending, &cp, sizeof(rev.pending)); >> break; > > OK, so we clear the fake one-entry pending array we just created. That > make sense. > > And then we have to restore rev.pending, because we don't otherwise > clean up cp. That makes sense in the context of your previous patch, but > if you take my suggestion there to separate "cp" from rev.pending > entirely, and clean it up explicitly, then this second memcpy() goes > away. IMHO that leaves the resulting code much easier to follow, as the > lifetimes are clearly distinct. Thanks, I'll mull that over, and didn't have time to do it now. FWIW I was aiming to end up with the least bit of invasiveness into the private bits of revision.[ch], i.e. we know we can fiddle with rev.pending independently of other members, but it's rather nasty to assume that. So restoring the previous state and having release_revisions() handle it is a bit less nasty. Of course we assume things about its internals here anyway, but this way it's only for the narrow scope of when we're clobbering it for "show". > Two alternatives I briefly considered that don't work: > > - you can't just leave the one-item rev.pending in place to get > cleaned up by release_revisions(), because we may show multiple > commits. We have to clean each one as we go. > > - you can't just object_array_clear(&rev.pending) _before_ clearing > it, because in the initial state it's still a copy of "cp", and thus > would hose the array we're iterating over. > > This whole "reuse rev, but tweak its pending array" feels a bit sketchy > to me in general. But it has been this way since 2006, and anyway is > completely out of scope for your series, so let's hold our nose and > continue. ;) *nod* >> diff --git a/t/t7007-show.sh b/t/t7007-show.sh >> index d6cc69e0f2c..f908a4d1abc 100755 >> --- a/t/t7007-show.sh >> +++ b/t/t7007-show.sh >> @@ -2,6 +2,7 @@ >> >> test_description='git show' >> >> +TEST_PASSES_SANITIZE_LEAK=true >> . ./test-lib.sh > > Lots of stuff now passes, which is good, but.... > >> diff --git a/t/t9122-git-svn-author.sh b/t/t9122-git-svn-author.sh >> index 527ba3d2932..0fc289ae0f0 100755 >> --- a/t/t9122-git-svn-author.sh >> +++ b/t/t9122-git-svn-author.sh >> @@ -2,7 +2,6 @@ >> >> test_description='git svn authorship' >> >> -TEST_FAILS_SANITIZE_LEAK=true >> . ./lib-git-svn.sh > > ...this one (and t9162) don't anymore? Are these hunks a mistake? If > not, this feels like something that needs to go into the commit message. I can add an explanation for this, this too passes, note that it's "TEST_FAILS..." not "TEST_PASSES...". I..e. in an earlier series I whitelisted all of lib-git-svn.sh, unless that variable was specificed, which was less churn than adding "TEST_PASSES_SANITIZE_LEAK=true" to all of them (as more than 1/2 of them passed already). So there's no new leaks here, and only new passing tests.
