Antoine Queru [off-list ref] 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 ;-).
quoted hunk
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/
----- Mail original -----
Antoine Queru [off-list ref] writes:
quoted
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 ;-).
Yes indeed it was hand wrapped, it will be changed in the next version,
thanks for the hint!
quoted
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.
Ok it makes sense, we will change for "pushBlacklist".
quoted
+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 -----).
Indeed the rendered doc is broken, it will be fixed in the next version.
quoted
+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.
I understand the confusion, it's not very clear. What we wanted to say
was that:
pushBlacklist = example.com/
pushWhitelist = example.com/
would raise an error, because the "priority rule" can't be applied.
We will change the sentence.
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
As of right now the protocol is not handled, we just ignore it.
We already thought about handling it the way you described but
we found out some problems:
pushBlacklist = example.com
pushWhitelist = https://example.com
-> push example.com forbidden EXCEPT with https
pushBlacklist = example.com/example2
pushWhitelist = https://example.com
-> push https://example.com/example2
-> Allow or deny?
To sum up the problem, what should be the priority between a
protocol-matching rule and a sub-folder-matching rule?
Perhaps adding a configuration variable?
pushPriority = "protocol"/"sub-folder" (needs renaming)
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).
Interesting idea, perhaps a simplified version could be:
pushDenyProtocolMessage = "This protocol is not allowed for this remote"
pushDenyRemoteMessage = "This remote is not authorized"
(as above v ery roughnaming)
quoted
+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.
Ok we will change it and use the skip_prefix() function.
quoted
+ 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 ... */.
This code removes all "unnecessary" parts of the url (protocol, user...).
Seeing the comments above, this code will probably be changed but we will
comment tricky code more often.
quoted
+test_expect_success 'unsetup' '
"cleanup" ?
Ok that's better
(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)
I agree
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/