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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 6, 2012, 01:20 UTC
Message-ID: <7vobr53bbe.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120405232429.GA8654@hmsreliant.think-freely.org>

Neil Horman <nhorman@tuxdriver.com> writes:

Show 20 quoted lines

> On Thu, Apr 05, 2012 at 01:12:51PM -0700, Junio C Hamano wrote:
>> 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.

No. When you review and fix typo in format-patch output, you can add these to the e-mail header part and git-send-email will pick them up just fine.

Show 6 quoted lines

>> 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?

You know you have N common ones. You push some more, perhaps with a sequence of "if (condition) argv_push();" and use it. Now it is time to reprepare it for the second usage. Wouldn't it be more natural to say

	argv_array_setlen(N);
        /* start pushing the specific ones for next invocation */
        argv_array_push();
        if (condition)
        	argv_array_push();

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

Please look at the existing "for ()" loop in the same file, and then go read the CodingGuidelines.