From: Jeff King <peff@peff.net>
To: Calvin Wan <calvinwan@google.com>
Cc: Junio C Hamano <gitster@pobox.com>,
git@vger.kernel.org, newren@gmail.com
Subject: Re: [PATCH 2/6] credential-store: move related functions to credential-store file
Date: Wed, 3 May 2023 12:28:58 -0400 [thread overview]
Message-ID: <20230503162858.GA1789686@coredump.intra.peff.net> (raw)
In-Reply-To: <20230502211454.1673000-3-calvinwan@google.com>
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
next prev parent reply other threads:[~2023-05-03 16:29 UTC|newest]
Thread overview: 85+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-02 21:14 [PATCH 0/6] strbuf cleanups Calvin Wan
2023-05-02 21:14 ` [PATCH 1/6] abspath: move related functions to abspath Calvin Wan
2023-05-02 21:42 ` Junio C Hamano
2023-05-02 21:14 ` [PATCH 2/6] credential-store: move related functions to credential-store file Calvin Wan
2023-05-02 21:52 ` Junio C Hamano
2023-05-03 16:28 ` Jeff King [this message]
2023-05-03 16:34 ` Jeff King
2023-05-03 18:38 ` Calvin Wan
2023-05-02 21:14 ` [PATCH 3/6] object-name: move related functions to object-name Calvin Wan
2023-05-02 21:14 ` [PATCH 4/6] path: move related function to path Calvin Wan
2023-05-02 21:14 ` [PATCH 5/6] strbuf: clarify dependency Calvin Wan
2023-05-03 1:56 ` Elijah Newren
2023-05-02 21:14 ` [PATCH 6/6] strbuf: remove environment variables Calvin Wan
2023-05-03 2:15 ` Elijah Newren
2023-05-02 22:20 ` [PATCH 0/6] strbuf cleanups Junio C Hamano
2023-05-02 22:31 ` Junio C Hamano
2023-05-02 23:51 ` Felipe Contreras
2023-05-02 22:36 ` Calvin Wan
2023-05-03 2:37 ` Elijah Newren
2023-05-03 18:00 ` Calvin Wan
2023-05-07 0:14 ` Elijah Newren
2023-05-07 13:14 ` Jeff King
2023-05-03 18:48 ` [PATCH v2 0/7] " Calvin Wan
2023-05-03 18:50 ` [PATCH v2 1/7] strbuf: clarify API boundary Calvin Wan
2023-05-03 18:50 ` [PATCH v2 2/7] abspath: move related functions to abspath Calvin Wan
2023-05-03 18:50 ` [PATCH v2 3/7] credential-store: move related functions to credential-store file Calvin Wan
2023-05-03 18:50 ` [PATCH v2 4/7] object-name: move related functions to object-name Calvin Wan
2023-05-03 18:50 ` [PATCH v2 5/7] path: move related function to path Calvin Wan
2023-05-03 18:50 ` [PATCH v2 6/7] strbuf: clarify dependency Calvin Wan
2023-05-03 19:26 ` Junio C Hamano
2023-05-03 18:50 ` [PATCH v2 7/7] strbuf: remove environment variables Calvin Wan
2023-05-03 19:24 ` Junio C Hamano
2023-05-03 19:41 ` Calvin Wan
2023-05-03 19:45 ` Junio C Hamano
2023-05-03 19:42 ` [PATCH v3 7/7] strbuf: remove environment variable Calvin Wan
2023-05-05 22:33 ` [PATCH v2 0/7] strbuf cleanups Junio C Hamano
2023-05-08 16:38 ` Calvin Wan
2023-05-07 0:40 ` Elijah Newren
2023-05-07 21:47 ` Felipe Contreras
2023-05-08 16:57 ` [PATCH v4 " Calvin Wan
2023-05-08 16:59 ` [PATCH v4 1/7] strbuf: clarify API boundary Calvin Wan
2023-05-08 17:22 ` Eric Sunshine
2023-05-10 22:51 ` Junio C Hamano
2023-05-08 16:59 ` [PATCH v4 2/7] abspath: move related functions to abspath Calvin Wan
2023-05-08 16:59 ` [PATCH v4 3/7] credential-store: move related functions to credential-store file Calvin Wan
2023-05-08 16:59 ` [PATCH v4 4/7] object-name: move related functions to object-name Calvin Wan
2023-05-08 16:59 ` [PATCH v4 5/7] path: move related function to path Calvin Wan
2023-05-08 16:59 ` [PATCH v4 6/7] strbuf: clarify dependency Calvin Wan
2023-05-08 16:59 ` [PATCH v4 7/7] strbuf: remove global variable Calvin Wan
2023-05-10 8:12 ` Phillip Wood
2023-05-09 1:57 ` [PATCH v4 0/7] strbuf cleanups Elijah Newren
2023-05-09 2:13 ` Felipe Contreras
2023-05-11 19:44 ` [PATCH v5 " Calvin Wan
2023-05-11 19:48 ` [PATCH v5 1/7] strbuf: clarify API boundary Calvin Wan
2023-05-11 19:57 ` Eric Sunshine
2023-05-11 20:03 ` Calvin Wan
2023-05-11 19:48 ` [PATCH v5 2/7] abspath: move related functions to abspath Calvin Wan
2023-05-11 19:48 ` [PATCH v5 3/7] credential-store: move related functions to credential-store file Calvin Wan
2023-05-11 19:48 ` [PATCH v5 4/7] object-name: move related functions to object-name Calvin Wan
2023-05-11 19:48 ` [PATCH v5 5/7] path: move related function to path Calvin Wan
2023-05-11 19:48 ` [PATCH v5 6/7] strbuf: clarify dependency Calvin Wan
2023-05-11 19:48 ` [PATCH v5 7/7] strbuf: remove global variable Calvin Wan
2023-05-11 20:24 ` Eric Sunshine
2023-05-11 21:42 ` Junio C Hamano
2023-05-12 14:54 ` Phillip Wood
2023-05-12 14:53 ` Phillip Wood
2023-05-12 19:31 ` Junio C Hamano
2023-05-12 17:14 ` [PATCH v6 0/7] strbuf cleanups Calvin Wan
2023-05-12 17:15 ` [PATCH v6 1/7] strbuf: clarify API boundary Calvin Wan
2023-05-12 17:15 ` [PATCH v6 2/7] abspath: move related functions to abspath Calvin Wan
2023-05-12 17:15 ` [PATCH v6 3/7] credential-store: move related functions to credential-store file Calvin Wan
2023-05-12 17:15 ` [PATCH v6 4/7] object-name: move related functions to object-name Calvin Wan
2023-05-12 17:15 ` [PATCH v6 5/7] path: move related function to path Calvin Wan
2023-05-12 17:15 ` [PATCH v6 6/7] strbuf: clarify dependency Calvin Wan
2023-05-12 17:15 ` [PATCH v6 7/7] strbuf: remove global variable Calvin Wan
2023-05-12 20:24 ` [PATCH v6 0/7] strbuf cleanups Junio C Hamano
2023-05-13 5:54 ` Eric Sunshine
2023-06-06 19:47 ` [PATCH v7 " Calvin Wan
2023-06-06 19:48 ` [PATCH v7 1/7] strbuf: clarify API boundary Calvin Wan
2023-06-06 19:48 ` [PATCH v7 2/7] strbuf: clarify dependency Calvin Wan
2023-06-06 19:48 ` [PATCH v7 3/7] abspath: move related functions to abspath Calvin Wan
2023-06-06 19:48 ` [PATCH v7 4/7] credential-store: move related functions to credential-store file Calvin Wan
2023-06-06 19:48 ` [PATCH v7 5/7] object-name: move related functions to object-name Calvin Wan
2023-06-06 19:48 ` [PATCH v7 6/7] path: move related function to path Calvin Wan
2023-06-06 19:48 ` [PATCH v7 7/7] strbuf: remove global variable Calvin Wan
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230503162858.GA1789686@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=calvinwan@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=newren@gmail.com \
/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.
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).