Antoine Queru <antoine.queru@xxxxxxxxxxxxxxxxxxxxxxx> writes:
> Currently, a user wanting to prevent accidental pushes to the wrong remote
> has to create a pre-push hook.
> The feature offers a configuration to allow users to prevent accidental pushes
> to the wrong remote. The user may define a list of whitelisted remotes, a list
> of blacklisted remotes and a default policy ("allow" or "deny"). A push
> is denied if the remote is explicitely blacklisted or if it isn't
> whitelisted and the default policy is "deny".
Not really serious, but the text above is weirdly wrapped, probably by
hand. I'm sure your text editor can do better and quicker ;-).
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 53f00db..1478ce3 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2517,6 +2517,23 @@ remote.pushDefault::
> `branch.<name>.remote` for all branches, and is overridden by
> `branch.<name>.pushRemote` for specific branches.
>
> +remote.pushBlacklisted::
> + The list of remotes the user is forbidden to push to.
> + See linkgit:git-push[1]
I'd have spelled that "pushBlacklist" (no 'ed'). I think variable names
usually do not use verbs. But I'm fine with your version too.
> +For example, if we set up the configuration variables like this:
> + git config --add remote.pushBlacklisted repository.com
> + git config --add remote.pushWhitelisted repository.com/Special_Folder
> +Any push of this form will be accepted:
> + git push repository.com/Special_Folder/foo
> +While those ones for example will be denied:
> + git push repository.com/Normal_Folder/bar
Please, look at the rendered output of your documentation, and notice
it's broken. We'd typically use the asciidoc syntax for inline code here
(between -----).
> +An error will be raised if the url is blacklisted and whitelisted at the same.
"at the same time"?
But as-is, this sentence conflicts with the previous "the more the url
in the config prefixes the asked url the more priority it has."
statement.
The documentation doesn't talk about the URL-normalization the code is
doing. I think a reasonable behavior would be:
pushBlacklisted = example.com/ => deny all accesses to example.com
pushBlacklisted = http://example.com/ => deny HTTP accesses to
example.com
The second is a valid use-case IMHO, some people may want to forbid some
protocols. Actually, one may even want to whilelist only one protocol
and write stg like this to force HTTPS on host example.com:
pushBlacklisted = example.com
pushWhitelisted = https://example.com
BTW, these use-cases could motivate some per-blacklist deny message
like
[remote "example.com"]
pushDenyMessage = "Please use HTTPS when you push to example.com"
I don't think it has to be implemented now though (better have users get
used to the basic feature and see if more is needed later).
> +static const char *string_url_normalize(const char *repo_name)
> +{
> + if (starts_with(repo_name, "file://"))
> + return repo_name + 7;
There are many instances of this in Git's codebase, but we now try to
avoid magic numbers like this, and would use strlen("file://") instead.
Actually, we even have skip_prefix() precisely for this use-case.
> + if (is_url(repo_name)) {
> + struct url_info url_infos;
> + url_normalize(repo_name, &url_infos);
> + return url_infos.url + url_infos.host_off;
I think this would deserve a comment to explain why and what this is
doing, like /* turn ... into ... to ... */.
> +test_expect_success 'unsetup' '
"cleanup" ?
(I just did a very quick look at the code, I think we need an agreement
on the details of specification before a more detailed review)
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html