git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Jakub Narebski <jnareb@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Subject: Re: [PATCHv3 11/13] strbuf: add strbuf_add*_urlencode
Date: Sat, 10 Dec 2011 15:09:09 -0500	[thread overview]
Message-ID: <20111210200909.GC17999@sigill.intra.peff.net> (raw)
In-Reply-To: <m3iplohc6s.fsf@localhost.localdomain>

On Sat, Dec 10, 2011 at 03:57:59AM -0800, Jakub Narebski wrote:

> Jeff King <peff@peff.net> writes:
> 
> > +void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len,
> > +			  int reserved)
> > +{
> > +	strbuf_grow(sb, len);
> 
> What is this `reserved` flag for, and when should one use it?
> It would be nice to have a short comment...

It indicates whether one should encode rfc3986 reserved characters. You
want to use it when encoding the host, username, and password portions
of a URL (i.e., before the "/"), but not the path (since you don't want
to encode all of the slashes). If you were breaking down the path more
(e.g., into a "query" and "fragment" portion), you would care about more
specific quoting there, but we don't; we treat everything after the
slash as an opaque blob of path.

Patch to the strbuf api documentation is below. I think it should be
squashed into patch 12/13.

> BTW. was it meant to be aligned like this?
> 
> > +void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len,
> > +			     int reserved)

It is aligned correctly. When you ident by tabs, the "+" of the diff
gets soaked in the first tabstop, so it is off-by-one with respect to
non-tabbed input (it is off even more in the quoted section above,
because "> > +" gets soaked into the first tabstop). You can see your
version above also is misaligned when I quote it. :)

If you apply the diff, the result is as you expected.

-Peff

---
diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
index afe2759..e1ab6c5 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -270,3 +270,14 @@ same behaviour as well.
 	third argument can be used to set the environment which the editor is
 	run in. If the buffer is NULL the editor is launched as usual but the
 	file's contents are not read into the buffer upon completion.
+
+`strbuf_add_urlencode`::
+
+	Copy data to the end of the buffer, percent-encoding it as per
+	rfc3986. If the reserved flag is non-zero, then characters in
+	the rfc3986 reserved list are percent-encoded; otherwise, they
+	are copied literally. Characters in the rfc3986 unreserved list
+	are always copied literally. All other characters are
+	percent-encoded. Typically, one would use the reserved flag for
+	the host and user-info sections of a URL, but leave special
+	characters in the path untouched.

  reply	other threads:[~2011-12-10 20:10 UTC|newest]

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-12-10 10:28 [PATCHv3 0/13] credential helpers Jeff King
2011-12-10 10:30 ` [PATCHv3 01/13] test-lib: add test_config_global variant Jeff King
2011-12-10 10:30 ` [PATCHv3 02/13] t5550: fix typo Jeff King
2011-12-10 10:31 ` [PATCHv3 03/13] introduce credentials API Jeff King
2011-12-10 11:43   ` Jakub Narebski
2011-12-10 19:48     ` Jeff King
2011-12-10 10:31 ` [PATCHv3 04/13] credential: add function for parsing url components Jeff King
2011-12-10 10:31 ` [PATCHv3 05/13] http: use credential API to get passwords Jeff King
2011-12-10 10:31 ` [PATCHv3 06/13] credential: apply helper config Jeff King
2011-12-10 10:31 ` [PATCHv3 07/13] credential: add credential.*.username Jeff King
2011-12-10 10:31 ` [PATCHv3 08/13] credential: make relevance of http path configurable Jeff King
2011-12-10 11:50   ` Jakub Narebski
2011-12-10 19:50     ` Jeff King
2011-12-10 10:31 ` [PATCHv3 09/13] docs: end-user documentation for the credential subsystem Jeff King
2011-12-10 10:34 ` [PATCHv3 10/13] credentials: add "cache" helper Jeff King
2012-01-10  1:50   ` Jonathan Nieder
2012-01-10  4:44     ` Jeff King
2012-01-10  4:57       ` Jeff King
2012-01-10 16:59         ` Junio C Hamano
2012-01-17  6:02         ` Jeff King
2012-01-17  6:51           ` Junio C Hamano
2012-01-10 17:44       ` Jonathan Nieder
2012-01-10 17:53         ` Jeff King
2012-01-11 23:50           ` Jonathan Nieder
2012-01-12  3:07             ` Jeff King
2011-12-10 10:34 ` [PATCHv3 11/13] strbuf: add strbuf_add*_urlencode Jeff King
2011-12-10 11:57   ` Jakub Narebski
2011-12-10 20:09     ` Jeff King [this message]
2011-12-10 10:34 ` [PATCHv3 12/13] credentials: add "store" helper Jeff King
2011-12-10 10:35 ` [PATCHv3 13/13] t: add test harness for external credential helpers Jeff King
2011-12-10 10:39 ` [PATCHv2 0/9] echo characters in username prompt Jeff King
2011-12-10 10:40   ` [PATCHv2 1/9] imap-send: avoid buffer overflow Jeff King
2011-12-10 10:40   ` [PATCHv2 2/9] imap-send: don't check return value of git_getpass Jeff King
2011-12-10 10:40   ` [PATCHv2 3/9] move git_getpass to its own source file Jeff King
2011-12-10 10:40   ` [PATCHv2 4/9] refactor git_getpass into generic prompt function Jeff King
2011-12-10 10:41   ` [PATCHv2 5/9] add generic terminal " Jeff King
2011-12-15 12:48     ` Pete Wyckoff
2011-12-15 13:39       ` Jeff King
2011-12-15 21:59         ` Pete Wyckoff
2011-12-10 10:41   ` [PATCHv2 6/9] prompt: use git_terminal_prompt Jeff King
2011-12-10 10:41   ` [PATCHv2 7/9] credential: use git_prompt instead of git_getpass Jeff King
2011-12-10 10:41   ` [PATCHv2 8/9] Makefile: linux has /dev/tty Jeff King
2011-12-10 10:41   ` [PATCHv2 9/9] Makefile: OS X " Jeff King
2011-12-12 21:10     ` [PATCH 1/2] Makefile: Windows lacks /dev/tty Johannes Sixt
2011-12-12 21:12       ` [PATCH 2/2] Makefile: optionally exclude code that needs Unix sockets Johannes Sixt
2011-12-12 21:39         ` Jeff King
2011-12-12 23:31           ` Junio C Hamano
2011-12-13  0:58             ` Jeff King
2011-12-13  0:45           ` Junio C Hamano
2011-12-13 20:00             ` Johannes Sixt
2011-12-14  0:14               ` Junio C Hamano
2011-12-12 21:18       ` [PATCH 1/2] Makefile: Windows lacks /dev/tty Jeff King
2011-12-12 21:52         ` Johannes Sixt
2011-12-10 10:53 ` [PATCH 1/1] contrib: add credential helper for OS X Keychain Jeff King

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=20111210200909.GC17999@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jnareb@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 NNTP newsgroup(s).