Re: [PATCH 3/7] check-docs: do not pretend that commands are listed which are excluded

[Junio C Hamano <gitster@pobox.com>]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> Subject: Re: [PATCH 3/7] check-docs: do not pretend that commands are listed which are excluded

Sorry, but I cannot quite parse the title.  I am guessing that you
meant "do not pretend that commands, which are excluded, are
listed", which is a mouthful but at least can be parseable X-<.

> In the previous commit, we taught `git help -a` to stop listing commands
> that are excluded from the build.
>
> In this commit, we stop `check-docs` from claiming that those commands
> are listed.

I think the result would become more readable (and I suspect that it
would at the same time make the title parseable, but I am not sure
as I do not quite know what the title wants to say in the first
place) if you clarified the verb "list" here perhaps with "listed in
command-list.txt".

The output from the sed script that filters the contents of the
command-list.txt file is compared with the ALL_COMMANDS list, and
we complain if an entry in command-list.txt is no longer in the
ALL_COMMANDS list (i.e. a developer removed a stale command but
forgot to remove it from the command-list.txt) [*1*].  

The step 2/7 marked the subcommands that ought to be on ALL_COMMANDS
for the purpose of this check but that are excluded from the build,
as a preparation for this step, and this step 3/7 uses the list of
excluded ones to avoid complaining them being in the categorized
list of subcommands (i.e. command-list.txt).

Makes sense.

	Side note *1*.  Another thing we would want to catch is a
	developer adding a new subcommand to ALL_COMMANDS while
	forgetting to give it a category in the command-list.txt
	file.  That is done in the "other" loop before this part,
	and the patch is correct that it does not touch that loop to
	filter the command-list using the EXCLUDED_PROGRAMS list.

We can read that "stop listing them" is done as means to some end,
but it is unclear what the end-user/developer visible effect this
step intends to achieve.  After thinking about what the patch does a
bit more, here is what I came up with.

    check-docs: allow excluded subcommands to be in the command-list file

    One of the things this build target makes sure is that all
    entries in the command-list.txt are about subcommands that still
    exist in the system (i.e. if a developer removes a subcommand
    and forgets to remove it from the command list file, it needs to
    be flagged as an incomplete change), and it is done by comparing
    the entries in the file against the list of subprograms in
    $(ALL_COMMANDS) variable.

    However, the latter list is dynamic---not all build contains all
    the Git subcommands categorized in the command-list.txt file.
    And such a partial build would falsely trigger the check,
    complaining that some subcommands are removed but still are
    listed in the command list file.

    Fix this by teaching the logic to use the EXCLUDED_PROGRAMS list
    prepared in the previous step.

or something like that, perhaps?

The same logic to warn about "removed but listed" commands that are
not built (hence missing from ALL_COMMANDS list) is also fed the
list of all documented subcommands by looking at "make print-man1"
in the Documentation directory, so that it can issue "removed but
documented" when a developer removes a subcommand but forgets to
remove its documentation.  Don't we need to teach a similar
treatment on that side of the coin?

I am wondering if it makes that easier to instead add EXCLUDED ones
back to ALL_COMMANDS on the receiving end of the pipe, rather than
filtering them out in the sed script that reads from command-list,
i.e.

	# revert what this patch did to the upstream of the pipe
	(
		sed ... filters comments ... \
			-e 's/^/listed /' command-list.txt;
		$(MAKE) -C Documentation print-man1 |
		sed ... -e 's/^/documented /'
	) | \
	while read how cmd; \
	do
		# instead add them back here
		case " $(patsubst %$X,%,$(ALL_COMMANDS) $(EXCLUDED)) " in
		*" $$cmd "*) : ok ;;
		...

That way, the documentation side can be fixed at the same time as
the command-list side of the check that incorrectly reports "removed
but listed".

If we go that route, the earlier rewrite of the proposed log message
I suggested may want to be further updated.  Perhaps by replacing
"use the EXCLUDED_PROGRAMS list prepared in the previous step" with
"add the EXCLUDED_PROGRAMS list prepared in the previous step back
to ALL_COMMANDS list when checking entries from the list of
documented and categorized subcommands." or something like that.

Thanks.