Re: [PATCH 1/5] argv-array: Add argv_array_pop function [v2]
- From
Neil Horman <nhorman@tuxdriver.com>
- Date
- Apr 5, 2012, 23:24 UTC
- Message-ID
- <20120405232429.GA8654@hmsreliant.think-freely.org>
- In-Reply-To
- <7vd37m5458.fsf@alter.siamese.dyndns.org>
On Thu, Apr 05, 2012 at 01:12:51PM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> Neil Horman <nhorman@tuxdriver.com> writes: > > > As a convienience, it would be nice if we could pop entries off the argv_array > > structs so that if they had multiple uses in a function, we wouldn't have to > > clear them and repopulate common entries. This patch adds the argv_array_pop > > function to do just that. Common entries can be added to an argv_array first, > > then useage specific ones can be added on the end and removed later on. > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > > > > CC: Jeff King <peff@peff.net> > > CC: Phil Hord <phil.hord@gmail.com> > > CC: Junio C Hamano <gitster@pobox.com> > > --- > > Please don't do "Cc:" here; they belong to your e-mail header. >
You mean place them below the snip line? I can do that.
Show 18 quoted lines
> > diff --git a/argv-array.c b/argv-array.c
> > index a4e0420..ce24a48 100644
> > --- a/argv-array.c
> > +++ b/argv-array.c
> > @@ -39,6 +39,18 @@ void argv_array_pushf(struct argv_array *array, const char *fmt, ...)
> > argv_array_push_nodup(array, strbuf_detach(&v, NULL));
> > }
> >
> > +int argv_array_pop(struct argv_array *array, unsigned int num)
> > +{
> > + if (num > array->argc)
> > + return -1;
>
> If your use case is "After using an argv_array for the first invocation,
> truncate it while keeping the common ones that appear early, so that ones
> that are specific to the second invocation can be pushed", it strikes me
> somewhat odd why you would want to specify "how many to pop".
> Why? It seems perfectly logical to me to be able to, as a convienience, specify how many items to pop, and the api call seems pleasantly symmetric to the push[f] calls.
> Wouldn't argv_array_truncate() or argv_array_setlen() make more sense? >
No, truncate is vague about its meaning. Truncate to zero would be useless, as its equivalent to the clear call at that point, and truncating to a specific value is exactly the same as what I have currently, minus the symmetry to the push[f] calls. Likewise setlen doesn't seem to fit in properly, plus theres the possibility there of believing the api call might be able to set a length longer than what exists in the array already, which, while silly, requires additional error checking. Popping a fixed number of elements off an argv_array that have previously been pushed makes perfectly good sense to me.
Show 5 quoted lines
>
> > + for(num--; num>0; num--) {
>
> Gaah.
> Eeek :). If you want something else equally....equal here, please ask for it. I prefer for loops, but if you would rather have a while loop here, I'm fine with that.
Regards Neil