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
