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

On 8/24/2021 5:41 PM, Ævar Arnfjörð Bjarmason wrote:

On Tue, Aug 24 2021, Derrick Stolee wrote:

quoted

On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:

quoted

+		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...

quoted

quoted

 /**
  * 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