| Message ID | 20230502211454.1673000-3-calvinwan@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | strbuf cleanups | expand |
Calvin Wan <calvinwan@google.com> writes: > is_rfc3986_unreserved() and is_rfc3986_reserved_or_unreserved() are only > called from builtin/credential-store.c and they are only relevant to that > file so move those functions and make them static. > > Signed-off-by: Calvin Wan <calvinwan@google.com> > --- > builtin/credential-store.c | 19 +++++++++++++++++++ > strbuf.c | 19 ------------------- > strbuf.h | 3 --- > 3 files changed, 19 insertions(+), 22 deletions(-) > > diff --git a/builtin/credential-store.c b/builtin/credential-store.c > index 8977604eb9..4776118331 100644 > --- a/builtin/credential-store.c > +++ b/builtin/credential-store.c > @@ -73,6 +73,25 @@ static void rewrite_credential_file(const char *fn, struct credential *c, > die_errno("unable to write credential store"); > } > > +static int is_rfc3986_unreserved(char ch) > +{ > + return isalnum(ch) || > + ch == '-' || ch == '_' || ch == '.' || ch == '~'; > +} > + > +static int is_rfc3986_reserved_or_unreserved(char ch) > +{ > + if (is_rfc3986_unreserved(ch)) > + return 1; > + switch (ch) { > + case '!': case '*': case '\'': case '(': case ')': case ';': > + case ':': case '@': case '&': case '=': case '+': case '$': > + case ',': case '/': case '?': case '#': case '[': case ']': > + return 1; > + } > + return 0; > +} Both the moved ones being static means we even lose the declarations in the header file. Very nice.
On Tue, May 02, 2023 at 09:14:50PM +0000, Calvin Wan wrote: > is_rfc3986_unreserved() and is_rfc3986_reserved_or_unreserved() are only > called from builtin/credential-store.c and they are only relevant to that > file so move those functions and make them static. This is probably OK to do, though I think "and they are only relevant to that file" is overstating things a bit. They are callbacks to be used with strbuf_add_urlencode(). Originally they were static-locals next to that function, with a flag bit to trigger which was used. Later, c2694952e3 (strbuf: give URL-encoding API a char predicate fn, 2019-06-27) replaced the flag with a callback mechanism, which meant the functions had to be public. But all callers except the new one added in that series still needed them (that other caller is not encoding a URL, so it wants different rules). And then finally, 644de29e22 (http: drop support for curl < 7.19.4, 2021-07-30) dropped the http.c callers, leaving only the ones in credential-store. So yes, it's true that only credential-store.c needs them right now. But separating them from strbuf_add_urlencode() is making that function next to useless for any future callers, as they'd have to reimplement the logic about which characters are reserved. I say "next to useless", because that one "not a URL" caller doesn't need that logic. So I dunno. It's largely academic until somebody else needs to encode bits of a URL. But when they do, the first thing they'd need to do is make these functions public again. I think the main reason we do not have other callers is that urlmatch.c implements its own percent-encoding code, and that's where we do most of our URL handling. It does make me wonder if credential-store could simply switch to using that, but that is probably out of scope for your series. -Peff
On Wed, May 03, 2023 at 12:28:58PM -0400, Jeff King wrote: > I think the main reason we do not have other callers is that urlmatch.c > implements its own percent-encoding code, and that's where we do most of > our URL handling. It does make me wonder if credential-store could > simply switch to using that, but that is probably out of scope for your > series. Poking at it a bit, I think the answer is "not easily". credential-store has broken-out elements of a URL, and wants to turn them back into one. And urlmatch.c's url_normalize() always takes a URL string as input. Even though it finds the broken-out fields as part of its work, the parsing and reconstruction of the URL are inter-mingled in a complex way (i.e., it's not just "parse it, then turn the fields back into a normalized string). I'm sure it could be refactored, but it's probably not worth it (and again, this tangent is somewhat orthogonal to your series, except that it would make these is_rfc3986 functions go away entirely). -Peff
Thanks for digging up the history and context for these functions. I agree that refactoring these functions right now is not worth it and can be saved for when there would be new callers to them. On Wed, May 3, 2023 at 9:34 AM Jeff King <peff@peff.net> wrote: > > On Wed, May 03, 2023 at 12:28:58PM -0400, Jeff King wrote: > > > I think the main reason we do not have other callers is that urlmatch.c > > implements its own percent-encoding code, and that's where we do most of > > our URL handling. It does make me wonder if credential-store could > > simply switch to using that, but that is probably out of scope for your > > series. > > Poking at it a bit, I think the answer is "not easily". credential-store > has broken-out elements of a URL, and wants to turn them back into one. > And urlmatch.c's url_normalize() always takes a URL string as input. > Even though it finds the broken-out fields as part of its work, the > parsing and reconstruction of the URL are inter-mingled in a complex > way (i.e., it's not just "parse it, then turn the fields back into a > normalized string). > > I'm sure it could be refactored, but it's probably not worth it (and > again, this tangent is somewhat orthogonal to your series, except that > it would make these is_rfc3986 functions go away entirely). > > -Peff
diff --git a/builtin/credential-store.c b/builtin/credential-store.c index 8977604eb9..4776118331 100644 --- a/builtin/credential-store.c +++ b/builtin/credential-store.c @@ -73,6 +73,25 @@ static void rewrite_credential_file(const char *fn, struct credential *c, die_errno("unable to write credential store"); } +static int is_rfc3986_unreserved(char ch) +{ + return isalnum(ch) || + ch == '-' || ch == '_' || ch == '.' || ch == '~'; +} + +static int is_rfc3986_reserved_or_unreserved(char ch) +{ + if (is_rfc3986_unreserved(ch)) + return 1; + switch (ch) { + case '!': case '*': case '\'': case '(': case ')': case ';': + case ':': case '@': case '&': case '=': case '+': case '$': + case ',': case '/': case '?': case '#': case '[': case ']': + return 1; + } + return 0; +} + static void store_credential_file(const char *fn, struct credential *c) { struct strbuf buf = STRBUF_INIT; diff --git a/strbuf.c b/strbuf.c index c3b6d48797..da2693b21f 100644 --- a/strbuf.c +++ b/strbuf.c @@ -809,25 +809,6 @@ void strbuf_addstr_xml_quoted(struct strbuf *buf, const char *s) } } -int is_rfc3986_reserved_or_unreserved(char ch) -{ - if (is_rfc3986_unreserved(ch)) - return 1; - switch (ch) { - case '!': case '*': case '\'': case '(': case ')': case ';': - case ':': case '@': case '&': case '=': case '+': case '$': - case ',': case '/': case '?': case '#': case '[': case ']': - return 1; - } - return 0; -} - -int is_rfc3986_unreserved(char ch) -{ - return isalnum(ch) || - ch == '-' || ch == '_' || ch == '.' || ch == '~'; -} - static void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len, char_predicate allow_unencoded_fn) { diff --git a/strbuf.h b/strbuf.h index 6b4ad63266..1a4c07a053 100644 --- a/strbuf.h +++ b/strbuf.h @@ -682,9 +682,6 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name); typedef int (*char_predicate)(char ch); -int is_rfc3986_unreserved(char ch); -int is_rfc3986_reserved_or_unreserved(char ch); - void strbuf_addstr_urlencode(struct strbuf *sb, const char *name, char_predicate allow_unencoded_fn);
is_rfc3986_unreserved() and is_rfc3986_reserved_or_unreserved() are only called from builtin/credential-store.c and they are only relevant to that file so move those functions and make them static. Signed-off-by: Calvin Wan <calvinwan@google.com> --- builtin/credential-store.c | 19 +++++++++++++++++++ strbuf.c | 19 ------------------- strbuf.h | 3 --- 3 files changed, 19 insertions(+), 22 deletions(-)