Re: [PATCH v2 2/4] bundle API: change "flags" to be "extra_index_pack_args"

[Derrick Stolee <stolee@gmail.com>]

On 8/24/2021 5:41 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Aug 24 2021, Derrick Stolee wrote:
> 
>> On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:
>>> +		strvec_clear(extra_index_pack_args);
>>
>> Why is it the responsibility of this method to clear these args?
>> I suppose it is convenient. It just seems a bit wrong to me.
> 
> Because of...
> 
>>>  /**
>>>   * Unbundle after reading the header with read_bundle_header().
>>>   *
>>>   * We'll invoke "git index-pack --stdin --fix-thin" for you on the
>>>   * provided `bundle_fd` from read_bundle_header().
>>> + *
>>> + * Provide extra_index_pack_args to pass any extra arguments
>>> + * (e.g. "-v" for verbose/progress), NULL otherwise. The provided
>>> + * extra_index_pack_args (if any) will be strvec_clear()'d for you
>>> + * (like the run-command.h API itself does).
> 
> ... this, i.e. it's how the run-command.[ch] API already works for the
> same sort of thing elsewhere, I figured making them consistent was
> better than having them differ.
> 
> I think that while in general the rule of having each function allocate
> & clear its own memory is a good one, that a notable good exception in
> our codebase is various "one-shot" functions such as the run-command
> API, i.e. APIs where the vast majority of callers just want to set
> things up for a one-off run. Having those common cases not require a
> that_api_release(&ctx) afterwards seems like a good idea in general.

Makes sense to me. Thanks for explaining it.

-Stolee