Re: [Outreachy] Proposal draft v2: Improve how command

[Cynthia Uche <herroyalpriestess@gmail.com>]

Hi Christian,

Thank you for going through the candidates one by one. Here is what
I will change in v3 and what I plan to keep.

> In https://lore.kernel.org/git/xmqqmrt7c1zf.fsf@gitster.g/ Junio also
> said detection of "--" could eventually come for free as a side effect
> of using parse_options(). So not sure an early scan will be needed at
> all.

Thanks for the link. My [3] cites your answer in that thread but not
Junio's follow-up to it. I will add it, and I will change the sentence
in 3.4 so that it no longer says bisect_start() would use the early
scan. Whether it is needed at all becomes part of the question for
bisect.

> Yeah, improving the early scan can result in refactorings like the one
> in patch 2/4, which could make it easier to implement a dry run in
> parse_options().

Noted, nothing to change there.

> The "must be first" rule also looks deliberate rather than an
> oversight. "builtin/column.c" has the comment "This one is special and
> must be the first one" above the scan.

That answers the question I planned to ask the list. I had only
searched Documentation/ and t/ for the rule, so I missed the comment in
column.c. In v3 I will cite it and remove the question. Since git tag,
the only internal user, doesn't use --command, I will take column out
of my targets and keep my four runs as a note.

> It looks like bisect already uses parse_options() but only to dispatch
> sub-commands (OPT_SUBCOMMAND() with PARSE_OPT_SUBCOMMAND_OPTIONAL).
> The sub-command functions indeed don't call it

That matches what I see in cmd_bisect(). I will describe it that way:
the dispatcher uses parse_options(), and bisect_start(), bisect_run()
and bisect_terms() parse their own arguments by hand. A conversion
would give each of them its own parse_options() call, one at a time.

> I haven't taken a look at this, but I saw that Hussain Sitamouwala
> sent "check-ref-format: use parse-options API" patches.

I had not seen them. I have now read v1, Junio's review and v2. The
points that v2's commit message addresses (abbreviations such as
--norm, "--" before <refname>, options after <refname> still rejected)
are among the risks I listed for this command, so I will not start a
parallel conversion. I will leave check-ref-format to Hussain and the
reviewers, and keep my observations from master in section 4 as a
reference. If useful, I can test his next version against the cases I
observed on master and report the results in his thread.

For v3, unless you prefer something else, my plan is: the bisect
sub-commands as my main target, starting with tests for their current
behaviour, then bisect_terms() and bisect_run(), and bisect_start()
after that. If that turns out smaller than expected, I would go back
to the inventory in section 4 and pick the next candidate with the
mentors, after checking that nobody else is working on it. Is bisect
the right main target, or is there a command you would rather see
converted?

Thanks,
Cynthia