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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 18, 2019, 12:41 UTC
Message-ID: <nycvar.QRO.7.76.6.1904181424330.46@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqq4l6zsplq.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Mon, 15 Apr 2019, Junio C Hamano wrote:

Show 9 quoted lines

> "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-<.

Well, what can I say, you're right.

Show 11 quoted lines

> > 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".

I was actually coming from the `help -a` side. But that is not obvious from the commit message, nor does that Makefile target handle that `help -a` output in the first place.

Show 11 quoted lines

> 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).

Right.

Show 25 quoted lines

> 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?

I rephrased this yet again (to reflect the way I speak):

	check-docs: allow command-list.txt to contain excluded commands

	Among other things, the `check-docs` target ensures that
	`command-list.txt` no longer contains commands that were dropped (or
	that were never added in the first place).

	To do so, it compares the list of commands from that file to the
	commands listed in `$(ALL_COMMANDS)`.

	However, some build options exclude commands from the latter. Fix the
	target to handle this situation correctly by taking the just-introduced
	`$(EXCLUDED_PROGRAMS)` into account.

Show 7 quoted lines

> 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?

Yep, that's what the later patch "docs: exclude documentation for commands that have been excluded" is all about.

Show 22 quoted lines

> 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".

I did exactly that, thank you for that suggestion.

Thanks, Dscho