Re: [PATCH 10/23] builtin/pull: fix leaking "ff" option

[Patrick Steinhardt <ps@pks.im>]

On Fri, Sep 20, 2024 at 10:00:04AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > The `opt_ff` field gets populated either via `OPT_PASSTHRU` via
> > `config_get_ff()` or when `--rebase` is passed. So we sometimes end up
> > overriding the value in `opt_ff` with another value, but we do not free
> > the old value, causing a memory leak.
> 
> Good eyes.
> 
> I have to wonder if it will come back and bite us again that
> OPT_PASSTHRU does not pass through but creates a new copy, while
> OPT_STRING borrows from the argv[].  I guess that we could check in
> parse_options_check() if the address of the same variable is passed
> to both OPT_PASSTHRU (giving it a piece of memory we are responsible
> for freeing) and OPT_STRING (giving it a piece of memory borrowed
> from argv[], and would trigger a failure if given to free()).
> 
> In an extremely longer run, I wonder if it becomes necessary for us
> to also make OPT_STRING to make a copy (which means strings that
> come from the configuration and from parsing the command line
> arguments are all allocated ones that we are responsible to free).

For now it seems like we are fine with `OPT_STRING` not copying the
value, and I haven't yet found a situation where we cannot work around
the alloc/don't alloc split. But it certainly does lead to an awkward
calling convention in many places that makes the resulting code way
harder to read than necessary if it did allocate.

I also doubt that avoiding an allocation for these gives us a noticeable
speedup. After all we're bounded by the number of arguments passed by
the user, so it's not like we would end up dupliciating thousands of
strings in the general case.

So yes, I think that it would simplify things quite a bit when all three
of them (OPT_PASSTHRU, OPT_STRING, config parsing) were duplicating the
values.

Patrick