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

From: Derrick Stolee <derrickstolee@github.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com,
	johannes.schindelin@gmx.de, Jeff King <peff@peff.net>
Subject: Re: [PATCH v2 1/3] Documentation: remove use of whitelist
Date: Tue, 19 Jul 2022 10:21:04 -0400	[thread overview]
Message-ID: <0fe031b8-1eec-7407-4e5c-cae298ddeb8e@github.com> (raw)
In-Reply-To: <220715.86sfn2zlkm.gmgdl@evledraar.gmail.com>

On 7/15/2022 6:47 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Jul 15 2022, Derrick Stolee via GitGitGadget wrote:
> 
>> 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.

Sure. That makes sense. The point here is that git-daemon's documentation
and error messages currently make the word "whitelist" a critical point to
understanding how the feature works. Instead, we can explain it more
clearly using other language. Since this is the biggest place where such
important is placed on the word, then making the changes isolated to this
command makes sense.

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

This example is misunderstanding that --strict-paths is a boolean
option and changes how the list of "undecorated" arguments at the end
is interpreted.

The point is that there is an optional list of directories given as
arguments, and the --strict-paths mode changes those directories to
not include recursive subdirectories as repo roots.
>>  	`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.

I guess I stopped at the first level of "in other words", that being the
"i.e." parenthetical. I didn't realize that this was already nested inside
an aside that was unnecessary.

Thanks,
-Stolee