Re: [PATCH 1/5] argv-array: Add argv_array_pop function [v2]

> On Thu, Apr 05, 2012 at 07:24:29PM -0400, Neil Horman wrote:
> 
> > > > 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.
> 
> No, I think he means to drop them entirely; that information is already
> in the list of people you have cc'd in the email.
> 

> > > > +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.
> 
> I don't mind a "pop" call if there is a good use, but personally I find
> your use case to be hard to read. Your patch 3/5 does this:
> 
>   /* make a partial argv */
>   argv_array_init(&array);
>   argv_array_push(&array, "commit");
>   argv_array_push(&array, "-n");
> 
>   /* now do some speculative command */
>   if (some_logic) {
>           argv_array_push(&array, "--porcelain");
>           if (run_command(array.argv)) {
>               argv_array_clear(&array);
>               return 0;
>           }
>           argv_array_pop(&array, 1);
>   }
> 
>   /* and then possibly proceed to reuse part of the array */
>   argv_array_push(&array, ...);
>   argv_array_push(&array, ...);
>   run_command(array.argv);
> 
> It saves you having to repeat "commit -n", but at the expense of making
> the logic much harder to read. I think this is much easier to read:
> 
>   if (some_logic) {
>           const char *argv[] = { "commit", "-n", "--porcelain", NULL };
>           if (run_command(argv))
>                   return 0;
>   }
> 
>   argv_array_init(&array);
>   argv_array_push(&array, "commit");
>   argv_array_push(&array, "-n");
>   argv_array_push(&array, /* other options */);
>   run_command(array.argv);
> 
> You repeat "-n", but it is very clear what goes into the speculative
> command and what goes into the final command (and there is no chance of
> the "1" in your pop becoming stale and sending cruft to the real command).
> 

> > > > +	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.
> 
> I think he may have been responding to the style (lack of whitespace). I

> I think a more traditional way of writing this would be:
> 
>   while (num--) {
>           free((char **)array->argv[num);
>           array->argv[num] = NULL;
>   }
>