From: Matthew DeVore <matvore@comcast.net>
To: Jeff King <peff@peff.net>
Cc: Emily Shaffer <emilyshaffer@google.com>,
Matthew DeVore <matvore@google.com>,
jonathantanmy@google.com, jrn@google.com, git@vger.kernel.org,
dstolee@microsoft.com, jeffhost@microsoft.com,
jrnieder@gmail.com, pclouds@gmail.com
Subject: Re: [PATCH v1 3/5] list-objects-filter: implement composite filters
Date: Tue, 4 Jun 2019 15:59:21 -0700 [thread overview]
Message-ID: <20190604225921.GA43275@comcast.net> (raw)
In-Reply-To: <20190604185108.GA14738@sigill.intra.peff.net>
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.
next prev parent reply other threads:[~2019-06-04 22:59 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-22 0:21 [PATCH v1 0/5] Filter combination Matthew DeVore
2019-05-22 0:21 ` [PATCH v1 1/5] list-objects-filter: refactor into a context struct Matthew DeVore
2019-05-24 0:49 ` Emily Shaffer
2019-05-28 18:48 ` Matthew DeVore
2019-05-28 22:40 ` [PATCH] list-objects-filter: merge filter data structs Matthew DeVore
2019-05-29 19:48 ` Junio C Hamano
2019-05-29 20:57 ` Jeff Hostetler
2019-05-29 23:10 ` Matthew DeVore
2019-05-30 1:56 ` [RFC PATCH v2] " Matthew DeVore
2019-05-30 16:12 ` Junio C Hamano
2019-05-30 18:29 ` Matthew DeVore
2019-05-30 19:05 ` [PATCH] " Matthew DeVore
2019-05-22 0:21 ` [PATCH v1 2/5] list-objects-filter-options: error is localizeable Matthew DeVore
2019-05-24 0:55 ` Emily Shaffer
2019-05-28 23:01 ` Matthew DeVore
2019-05-22 0:21 ` [PATCH v1 3/5] list-objects-filter: implement composite filters Matthew DeVore
2019-05-24 21:01 ` Jeff Hostetler
2019-05-28 17:59 ` Junio C Hamano
2019-05-29 15:02 ` Matthew DeVore
2019-05-29 21:29 ` Jeff Hostetler
2019-05-29 23:27 ` Matthew DeVore
2019-05-30 14:01 ` Jeff Hostetler
2019-05-31 20:53 ` Matthew DeVore
2019-06-03 21:04 ` Jeff Hostetler
2019-06-01 0:11 ` Matthew DeVore
2019-05-28 21:53 ` Emily Shaffer
2019-05-31 20:48 ` Matthew DeVore
2019-05-31 21:10 ` Jeff King
2019-06-01 0:12 ` Matthew DeVore
2019-06-03 12:34 ` Jeff King
2019-06-03 22:22 ` Matthew DeVore
2019-06-04 16:13 ` Jeff King
2019-06-04 17:19 ` Matthew DeVore
2019-06-04 18:51 ` Jeff King
2019-06-04 22:59 ` Matthew DeVore [this message]
2019-06-04 23:14 ` Jeff King
2019-06-04 23:49 ` Matthew DeVore
2019-06-09 12:36 ` Jeff King
2019-05-22 0:21 ` [PATCH v1 4/5] list-objects-filter-options: move error check up Matthew DeVore
2019-05-22 0:21 ` [PATCH v1 5/5] list-objects-filter-options: allow mult. --filter Matthew DeVore
2019-06-06 22:44 ` [PATCH v1 0/5] Filter combination Matthew DeVore
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190604225921.GA43275@comcast.net \
--to=matvore@comcast.net \
--cc=dstolee@microsoft.com \
--cc=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=jeffhost@microsoft.com \
--cc=jonathantanmy@google.com \
--cc=jrn@google.com \
--cc=jrnieder@gmail.com \
--cc=matvore@google.com \
--cc=pclouds@gmail.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).