On Thu, May 21, 2009 at 11:58 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Nguyen Thai Ngoc Duy <pclouds@xxxxxxxxx> writes: > >> 2009/5/20 Johannes Sixt <j.sixt@xxxxxxxxxxxxx>: >>> Nguyễn Thái Ngọc Duy schrieb: >>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> >>>> --- >>>> revision.c | 4 ++-- >>>> 1 files changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/revision.c b/revision.c >>>> index 18b7ebb..be1e307 100644 >>>> --- a/revision.c >>>> +++ b/revision.c >>>> @@ -1241,9 +1241,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch >>>> if (strcmp(arg, "--")) >>>> continue; >>>> argv[i] = NULL; >>>> - argc = i; >>>> - if (argv[i + 1]) >>>> + if (i + 1 < argc && argv[i + 1]) >>>> revs->prune_data = get_pathspec(revs->prefix, argv + i + 1); >>>> + argc = i; >>>> seen_dashdash = 1; >>>> break; >>>> } >>> >>> Why is this necessary? I'd expect that argv arrays have NULL at the end. >> >> I have no idea. I hit this "bug" in my builtin-rebase.c and had that >> question too. But I grepped through and saw that >> at least verify_bundle() does not terminate argv with NULL. So I >> assume that setup_revisions() does not expect NULL at the end. > > If a function takes (int ac, char **av), then people should be able to > depend on the usual convention of > > (1) for any i < ac, av[i] is not NULL; and > (2) av[ac] is NULL. > > With your patch, a broken caller's wish is simply discarded and nobody > will notice. Without your patch, at least you will know that the caller > passed an inconsistent pair of ac and av to this function by seeing a > coalmine canary segfault. OK. I will send another patch for verify_bundle() then :-) just to make sure no one goes down the same way. -- Duy -- 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