----- Mail original -----
> 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
>
Hello Rémi, thanks you for your input ! I'll make the appropriate changes
and send a new version as soon as i can !
--
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