Re: [PATCH v2 1/3] Documentation: remove use of whitelist

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 15, 2022, 10:47 UTC
Message-ID: <220715.86sfn2zlkm.gmgdl@evledraar.gmail.com>
In-Reply-To: <19632a2d2459114484dcdaddd4db0d22dfd64fa7.1657852722.git.gitgitgadget@gmail.com>

On Fri, Jul 15 2022, Derrick Stolee via GitGitGadget wrote:

Show 23 quoted lines

> From: Derrick Stolee <derrickstolee@github.com>
> [...]
> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
> index fdc28c041c7..7a0539cb411 100644
> --- a/Documentation/git-daemon.txt
> +++ b/Documentation/git-daemon.txt
> @@ -32,8 +32,8 @@ that service if it is enabled.
>  It verifies that the directory has the magic file "git-daemon-export-ok", and
>  it will refuse to export any Git directory that hasn't explicitly been marked
>  for export this way (unless the `--export-all` parameter is specified). If you
> -pass some directory paths as 'git daemon' arguments, you can further restrict
> -the offers to a whitelist comprising of those.
> +pass some directory paths as 'git daemon' arguments, the offers are limited to
> +repositories within those directories.
>  
>  By default, only `upload-pack` service is enabled, which serves
>  'git fetch-pack' and 'git ls-remote' clients, which are invoked
> @@ -50,7 +50,7 @@ OPTIONS
>  	Match paths exactly (i.e. don't allow "/foo/repo" when the real path is
>  	"/foo/repo.git" or "/foo/repo/.git") and don't do user-relative paths.
>  	'git daemon' will refuse to start when this option is enabled and no
> -	whitelist is specified.
> +	specific directories are specified.

Structurally this series should be changed so that like changes are coupled together, this would be much easier to review with the daemon.c changes in 3/3.

But that also shows that this change is needed, but really lacking compared to what we could do here, which is that both the the SYNOPSIS and the heading here should be, respectively:

    [--strict-paths=<path>...]

And:

    --strict-paths=<path>...:

I.e. all we're trying to get across here is "this option has a mandatory argument", so let's just say something like that explicitly? I think in this case we don't need the prose at all, the synopsis + heading + error would be enough.

More generally: As I noted on v1 I think the underlying motivation for the series is mistaken, but I'm also happy for any excuse people can find to improve our documentation.

But as I pointed out on your similar earlier series to expunge gendered pronouns from the docs I think best practices in our docs just happen to align with what you want.

I.e. in that case we prefer a style that isn't introducing "actors" anyway, so even without that motivation the prose could be improved.

And ditto here, I think the use of the term "whitelist" is fine in and of itself, but in this case we can clearly improve the docs anyway.

I just think that the origin of the change really shows in this case, I haven't yet reviewed the rest but suspect I'll find something similar. I.e. there's a clear improvement to be made, but since it was first made with s/whitelist/allowlist/g, and now an attempt to s/whitelist//g in some form we lose sight of the larger picture.

Which in thihs case is that perhaps the sentence isn't needed at all, and that the synopsis & title is the real thing worth fixing.

So, if what motivates doc improvements in your case is working on your local dictionary black^Hdenylist :): great, we can probably improve our docs in any case. But please spend a bit of time eyeballing the change without an eye to that motivation, sometimes we can improve it much more without much effert...

Show 14 quoted lines

> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 302607a4967..dd5061563eb 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -886,8 +886,7 @@ for full details.
>  	`protocol.allow` is set to `never`, and each of the listed
>  	protocols has `protocol.<name>.allow` set to `always`
>  	(overriding any existing configuration). In other words, any
> -	protocol not mentioned will be disallowed (i.e., this is a
> -	whitelist, not a blacklist). See the description of
> +	protocol not mentioned will be disallowed. See the description of
>  	`protocol.allow` in linkgit:git-config[1] for more details.
>  
>  `GIT_PROTOCOL_FROM_USER`::

I agree with Junio's earlier feedback about "in other words" being a telltale sign of prose that needs improving.

But the point of the previous prose (such as it was) was to elaborate on th existing "allow" to say "oh, allow means the same as whitelist", surely?

So I think we really could just delete this "in other words" entirely, i.e. it's basically saying "you are allowed to eat ice cream (in other words, you are not disallowed)", it's not adding anything anymore. The "(...)" can just be removed.