On Mon, Jul 18, 2022 at 05:13:05PM +0200, Ævar Arnfjörð Bjarmason wrote:
> > But if you went just a little further and made the option "rev should
> > own its own argv", then I think you can simplify life for callers even
> > more. They could construct a strvec themselves and then hand it off to
> > the rev_info, to be cleaned up when release_revisions() is called (and
> > of course freeing the "--" when we overwrite it in the interim, as you
> > do here).
> >
> > Then all of the bisect callers from the previous patch could avoid
> > having to deal with the strvec at all. They'd call bisect_rev_setup(),
> > which would internally attach the memory to rev_info.
>
> Yes, I experimented with this, and it's a solid approach.
>
> But it's a much larger change, particularly since we'd also want to
> update the API itself to take take "const" in the appropriate places to
> do it properly.
Hmm. I was thinking that we'd just have rev_info.we_own_our_argv, and
then release it in release_revisions(). But actually, rev_info does not
hold onto the argv at all! It's totally processed in setup_revisions().
And there it either:
- becomes part of prune_data (in which case a copy is made)
- is passed to handle_revisions_opt(), etc, in which case it is parsed
but not held onto (it's possible that some string option holds onto
a pointer, but the only one I found is --filter, which makes a
copy).
- is passed to handle_revision_arg(), but these days
add_rev_cmdline(), etc, make copies. As they must, otherwise --stdin
would be totally buggy.
So I actually wonder if the comment in bisect_rev_setup() is simply
wrong. It was correct in 2011 when I wrote it, but things changed in
df835d3a0c (add_rev_cmdline(): make a copy of the name argument,
2013-05-25), etc.
In that case, we could replace your patch 5 in favor of just calling
strvec_clear() at the end of bisect_rev_setup(). It's possible there's a
case I'm missing that makes this generally not-safe, but in the case of
bisect_rev_setup() there's a very limited set of items in our argv in
the first place. Doing so also passes the test suite with
SANITIZE=address, though again, this is just exercising the very limited
bisect options here.
-Peff
