On Thu, Feb 8, 2024 at 6:44 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Christian Couder <christian.couder@xxxxxxxxx> 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.
