Hi Antoine,
Antoine Queru <Antoine.Queru@xxxxxxxxxxxxxxxx> writes:
> [...]
> +For example, if we set up the configuration variables like this:
> +
> +-------------------------------
> +git config --add remote.pushBlacklist repository.com
> +git config --add remote.pushWhitelist repository.com/Special_Folder
> +-------------------------------
> +
> +Pushes like this will be accepted:
> +-------------------------------
> +git push repository.com/Special_Path/*
> +-------------------------------
According to your previous `git config`, it should be:
git push repository.com/Special_Folder/*
> +
> +While this one for example will be denied:
> +-------------------------------
> +git push repository.com/Other_Path/
> +-------------------------------
> +
> [...]
> +/*
> + *NEEDSWORK: Ugly because file://... is recognized as an url, and we
> + *may want to compare it to local path without this scheme. Forcing
> + *the user to put file:// before every local path would make the code
> + *easier and avoid confusion with a distant repo like 'github.com'
> + *which is not an url.
> + */
Style: space after '*' (when there is text after), meaning:
/*
* NEEDSWORK: Ugly because file://... is recognized [...]
* [...]
*/
> +static int longest_prefix_size(const char* target_str,
> + const struct string_list *list)
That might just be my mailer but this line is not properly lined up
with the previous one (one space too much).
It should be:
static int longest_prefix_size(const char* target_str,
const struct string_list *list)
> [...]
> + for_each_string_list_item(curr_item, list) {
> + struct url_info curr_url;
> + const char *curr_str = curr_item->string;
> + skip_prefix(curr_str, "file://", &curr_str);
> + url_normalize(curr_str, &curr_url);
> + if (target_url.url &&
> + curr_url.url &&
You can put target_url.url and curr_url.url on the same line.
> + target_url.scheme_len == curr_url.scheme_len &&
> + !strncmp(target_url.url,curr_url.url,curr_url.scheme_len))
Style: space after ','.
With those two things, the condition would look like this:
if (target_url.url && curr_url.url &&
target_url.scheme_len == curr_url.scheme_len &&
!strncmp(target_url.url, curr_url.url, curr_url.scheme_len))
> [...]
> + whitelist_size = longest_prefix_size(repo, whitelist);
> + blacklist_size = longest_prefix_size(repo, blacklist);
> +
> + check_length_prefix(whitelist_size, blacklist_size, repo, deny_message, default_policy);
This line is above 80 characters, so:
check_length_prefix(whitelist_size, blacklist_size, repo, deny_message,
default_policy);
> [...]
> +test_expect_success 'setup' '
> + git init --bare blacklist/ &&
> + git init --bare whitelist/ &&
> + git init --bare blacklist/allow &&
> + test_commit commit &&
> + echo "fatal: Pushing to this remote using this protocol is forbidden" > forbidden
> +'
> +
> +test_expect_success 'basic case' '
> + git config --add remote.pushBlacklist http://blacklist.com &&
You use `git config` instead of `test_config`, meaning that the
configuration you introduce will persist after the test.
It is not really a problem here since for the other tests you don't
use `git config --add` so the configuration will be
overwritten. However I still think you should use `test_config` to
avoid causing trouble to potential future tests that would use
`--add` and expect a clean state.
> [...]
> +test_expect_success 'local path with file://' '
> + git config remote.pushBlacklist file://blacklist &&
> + test_must_fail git push blacklist HEAD 2> result &&
> + test_cmp result forbidden
> +'
(you forgot a new line here)
> +test_expect_success 'only one scheme allowed' '
> + git config remote.pushDefaultPolicy deny &&
> + git config remote.pushWhitelist http://blacklist.com &&
> + test_must_fail git push https://blacklist.com HEAD 2> result &&
> + test_cmp result forbidden
> +'
> +
> +test_expect_success 'denied repo in allowed repo' '
'allowed repo in denied remote'? In any case the current title is
misleading for me.
> + git config remote.pushBlacklist blacklist &&
> + git config --add remote.pushWhitelist blacklist/allow &&
> + git push blacklist/allow HEAD
> +'
Thanks,
Rémi
--
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