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

[Neil Horman <nhorman@tuxdriver.com>]

On Thu, Apr 05, 2012 at 08:19:42PM -0400, Jeff King wrote:
> 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.
> 
No, its not, thats what I'm saying.  git send-email parses the above information
to build the CC list.  I can take it out and add the cc's to the command line of
git-send-email, but if parsing the above info is incorrect, that seems like a
bug that needs fixing, one which will get resistance, because its the standard
way alot of other lists use the CC: tag.

> > > > +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).
> 
Ok, I can see the use of the argv array above being more readable, but (I think
your) comment from the first iteration of this patch, suggested re-doing this
logic to use struct argv_array.  I do like the above better.  I'll redo that.
 
> That being said, I think Junio commented on 3/5 that "git commit
> --porcelain" is not the right way of doing your speculative command
> anyway, so the two commands would not end up sharing any argv anyway.
>
I'm still not completely convinced about that, given that commit --porcelain
effectively does a git diff-index, but I'll defer to the experts on that,
diff-index works just as well for this.  And if I use your above static array
implementation instead, I can get rid of the pop api addition entirely.
 
> > > > +	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
That was really my problem with it.  I appreciate the direct comment.  A note
about lack of whitespace is something I can work with, Gaah is not :)

> also find a side-effecting initializer a little non-idiomatic. And
> indeed, I think it causes a bug in this case. "num" is an unsigned int.
> So what happens to the loop when num is 0 coming in?
> 
That should be caught by the case checking logic at the top of the function.
Although I think you're right, the 1 case I think has an OBO error.  Its moot
anyway, if I use your static array approach above, I'll remove all of this.

> I think a more traditional way of writing this would be:
> 
>   while (num--) {
>           free((char **)array->argv[num);
>           array->argv[num] = NULL;
>   }
> 
Thats certainly another way to do it, and I'm happy to do that if its the
consensus.  I just happen to prefer for loops (for no particular reason, its
just me).  But again, using your approach above, this will all get removed.

Thanks for the review.  I'll fix this up, and have a new version in a few days.


Best
Neil
> -Peff
>