Re: [PATCH v2 4/4] rev-list: allow missing tips with --missing=[print|allow*]

[Christian Couder <christian.couder@gmail.com>]

On Thu, Feb 8, 2024 at 6:44 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:

> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> > index b3f4783858..ec9556f135 100644
> > --- a/builtin/rev-list.c
> > +++ b/builtin/rev-list.c
> > @@ -545,6 +545,15 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
> >        *
> >        * Let "--missing" to conditionally set fetch_if_missing.
> >        */
> > +     /*
> > +      * NEEDSWORK: These dump loops to look for some options early
> > +      * are ugly. We really need setup_revisions() to have a
>
> I would drop the "dump" and "ugly" from here.  It OK to make value
> judgement with such words in casual conversations on a proposed
> patch, but when we make a request to future developers to fix our
> mess, we should be more objective and stick to the techincal facts,
> so that they have better understanding on why we think this area
> needs more work.
>
> Perhaps something like:
>
>     These loops that attempt to find presence of options without
>     understanding what the options they are skipping are broken
>     (e.g., it would not know "--grep --exclude-promisor-objects" is
>     not triggering "--exclude-promisor-objects" option).

I have used what you suggest in V3, except for s/what/that/.

> Everything after "We really need" is good (modulo possible grammos),
> I think.  Thanks for writing it.
>
> > +      * mechanism to allow and disallow some sets of options for
> > +      * different commands (like rev-list, replay, etc). Such
> > +      * mechanism should do an early parsing of option and be able
> > +      * to manage the `--exclude-promisor-objects` and `--missing=...`
> > +      * options below.
> > +      */
> >       for (i = 1; i < argc; i++) {
> >               const char *arg = argv[i];
> >               if (!strcmp(arg, "--exclude-promisor-objects")) {
> > @@ -753,8 +762,12 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
> >
> >       if (arg_print_omitted)
> >               oidset_init(&omitted_objects, DEFAULT_OIDSET_SIZE);
> > -     if (arg_missing_action == MA_PRINT)
> > +     if (arg_missing_action == MA_PRINT) {
> >               oidset_init(&missing_objects, DEFAULT_OIDSET_SIZE);
> > +             /* Already add missing tips */
> > +             oidset_insert_from_set(&missing_objects, &revs.missing_commits);
> > +             oidset_clear(&revs.missing_commits);
> > +     }
>
> It is unclear what "already" here refers to, at least to me.

I removed it and changed the comment to just "/* Add missing tips */"
in the V3 I just sent.

Thanks.