On Thu, Apr 05, 2012 at 07:24:29PM -0400, Neil Horman wrote:
> > > CC: Jeff King <peff@xxxxxxxx>
> > > CC: Phil Hord <phil.hord@xxxxxxxxx>
> > > CC: Junio C Hamano <gitster@xxxxxxxxx>
> > > ---
> >
> > 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).
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.
> > > + 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
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?
I think a more traditional way of writing this would be:
while (num--) {
free((char **)array->argv[num);
array->argv[num] = NULL;
}
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html