Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.

> Hi Antoine,
> 
> Antoine Queru <Antoine.Queru@grenoble-inp.org> 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
>