On Tue, Jun 04, 2019 at 02:51:08PM -0400, Jeff King wrote:
> > The purpose of has_reserved_character is to allow for future
> > extensibility if someone decides to implement a more sophisticated DSL
> > and give meaning to these characters. That may be a long-shot, but it
> > seems worth it.
>
> I think you'll find that -Wunused-function complains, though, if nobody
> is calling it. I wasn't sure if what you showed in the interdiff was
> meant to be final (I had to add a few other variable declarations to
> make it compile, too).
Sorry, my last interdiff was a mess because I made a mistake during git rebase
-i. It was missing a call to has_reserved_char. Below is another diff that
fixes the problems:
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 0f135602a7..6b206dc58b 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -110,28 +110,31 @@ static int has_reserved_character(
return 0;
}
static int parse_combine_subfilter(
struct list_objects_filter_options *filter_options,
struct strbuf *subspec,
struct strbuf *errbuf)
{
size_t new_index = filter_options->sub_nr;
+ char *decoded;
+ int result;
ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,
filter_options->sub_alloc);
decoded = url_percent_decode(subspec->buf);
- result = gently_parse_list_objects_filter(
- &filter_options->sub[new_index], decoded, errbuf);
+ result = has_reserved_character(subspec, errbuf) ||
+ gently_parse_list_objects_filter(
+ &filter_options->sub[new_index], decoded, errbuf);
free(decoded);
return result;
}
static int parse_combine_filter(
struct list_objects_filter_options *filter_options,
const char *arg,
struct strbuf *errbuf)
{
> > strbuf_addstr_urlencode will either escape or not escape all rfc3986
> > reserved characters, and that set includes both : and +. The former
> > should not require escaping since it's a common character in filter
> > specs, and I would like the hand-encoded combine specs to be relatively
> > easy to type and read. The + must be escaped since it is used as part of
> > the combine:... syntax to delimit sub filters. So
> > strbuf_addstr_url_encode would have to be more customizable to make it
> > work for this context. I'd like to add a parameterizable should_escape
> > predicate (iow function pointer) which strbuf_addstr_urlencode accepts.
> > I actually think this will be more readable than the current strbuf API.
>
> That makes some sense, and I agree that readability is a good goal. Do
> we not need to be escaping colons in other URLs? Or are the strings
> you're generating not true by-the-book URLs? I'm just wondering if we
> could take this opportunity to improve the URLs we output elsewhere,
> too.
The strings I'm generating are not URLs. Also, in http.c, we have to use : to
delimit a username and password:
strbuf_addstr_urlencode(&s, proxy_auth.username, 1);
strbuf_addch(&s, ':');
strbuf_addstr_urlencode(&s, proxy_auth.password, 1);
I think this is dictated by libcurl and is not flexible.
