* [RFH/PATCH] imap-send: support SNI (RFC4366)
From: Junio C Hamano @ 2013-02-21 0:18 UTC (permalink / raw)
To: git
To talk to a site that serves multiple names on a single IP address,
the client needs to ask for the specific hostname it wants to talk
to. Otherwise, the default certificate returned from the IP address
may not match that of the host we wanted to talk to.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* I need help from people on this patch in two areas:
(1) I only tested this patch by connecting to https://googlemail.com/
with
$ git -c imap.host=imaps://googlemail.com -c imap.port=443 imap-send <this-patch.txt
as it is the only site I knew clients needs to talk SNI to get
the right certificate to verify; of course the port does not
talk imap, and the only thing that is tested by that approach is
we successfully establish an SSL/TLS connection. Without the
patch, we fail to verify the certificate (we get a cert that is
for another hostname that is hosted at the same IP address), and
with the patch, we successfully get the right one.
I would appreciate it if somebody knows an imap server that
needs SNI and runs an end-to-end test against that server.
(2) I do not know if everybody has SSL_set_tslext_host_name() macro
defined, so this patch may be breaking build for people with
different versions of OpenSSL.
imap-send.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/imap-send.c b/imap-send.c
index 171c887..d9abd8b 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -370,6 +370,15 @@ static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int ve
return -1;
}
+ /*
+ * SNI (RFC4366)
+ * OpenSSL does not document this function, but the implementation
+ * returns 1 on success, 0 on failure after calling SSLerr().
+ */
+ ret = SSL_set_tlsext_host_name(sock->ssl, server.host);
+ if (ret != 1)
+ warning("SSL_set_tslext_host_name(%s) failed.\n", server.host);
+
ret = SSL_connect(sock->ssl);
if (ret <= 0) {
socket_perror("SSL_connect", sock, ret);
--
1.8.2.rc0.106.ga6e4a61
^ permalink raw reply related
* Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
From: Junio C Hamano @ 2013-02-20 22:19 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, git, Shawn O. Pearce
In-Reply-To: <20130220221248.GC817@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I am more concerned that the assertion is not "oops, another thread is
> doing something crazy, and it is a bug", but rather that there is some
> weird platform where SIG_DFL does not kill the program under SIGPIPE.
> That seems pretty crazy, though. I think I'd squash in something like
> this:
>
> diff --git a/write_or_die.c b/write_or_die.c
> index b50f99a..abb64db 100644
> --- a/write_or_die.c
> +++ b/write_or_die.c
> @@ -5,7 +5,9 @@ static void check_pipe(int err)
> if (err == EPIPE) {
> signal(SIGPIPE, SIG_DFL);
> raise(SIGPIPE);
> +
> /* Should never happen, but just in case... */
> + error("BUG: SIGPIPE on SIG_DFL handler did not kill us.");
> exit(141);
> }
> }
>
> which more directly reports the assertion that failed, and degrades
> reasonably gracefully. Yeah, it's probably overengineering, but it's
> easy enough to do.
Yeah, that sounds like a sensible thing to do, as it is cheap even
though we do not expect it to trigger.
^ permalink raw reply
* Re: Merge with staged and unstaged changes
From: Junio C Hamano @ 2013-02-20 22:17 UTC (permalink / raw)
To: Edward Thomson; +Cc: git@vger.kernel.org
In-Reply-To: <A54CE3E330039942B33B670D971F85740396C4F8@TK5EX14MBXC254.redmond.corp.microsoft.com>
Edward Thomson <ethomson@microsoft.com> writes:
> I also appreciate your explanation of the affect of the workdir,
> and that makes sense. I would have expected that the default was
> to presume the workdir files were existent, rather than the
> other way around, but we can agree that is an implementation detail.
>
> My biggest concern, of course, was having the unstaged files in my
> workdir overwritten or deleted.
Oh, no question about that part. You concluded your original
message with:
>> I trust the last two cases, where data is lost, are bugs to
>> report, but could I get clarification on the other situations?
and I was responding to the part after the "but could I get...".
I am fairly familiar with the "read-tree -m -u O A B" three-way
merge codepath (after all I designed that with Linus in the very
early days of Git), but I am not as familar with the merge-recursive
backend as merge-resolve, and I was hoping to see the "bug" part
triaged by other people.
^ permalink raw reply
* Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
From: Jeff King @ 2013-02-20 22:12 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Junio C Hamano, Shawn O. Pearce
In-Reply-To: <20130220220637.GC24236@google.com>
On Wed, Feb 20, 2013 at 02:06:37PM -0800, Jonathan Nieder wrote:
> > I don't mind adding a "BUG: " message like you described, but we should
> > still try to exit(141) as the backup, since that is the shell-equivalent
> > code to the SIGPIPE signal death.
>
> If you want. :)
>
> I think caring about graceful degradation of behavior in the case of
> an assertion failure is overengineering, but it's mostly harmless.
I am more concerned that the assertion is not "oops, another thread is
doing something crazy, and it is a bug", but rather that there is some
weird platform where SIG_DFL does not kill the program under SIGPIPE.
That seems pretty crazy, though. I think I'd squash in something like
this:
diff --git a/write_or_die.c b/write_or_die.c
index b50f99a..abb64db 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -5,7 +5,9 @@ static void check_pipe(int err)
if (err == EPIPE) {
signal(SIGPIPE, SIG_DFL);
raise(SIGPIPE);
+
/* Should never happen, but just in case... */
+ error("BUG: SIGPIPE on SIG_DFL handler did not kill us.");
exit(141);
}
}
which more directly reports the assertion that failed, and degrades
reasonably gracefully. Yeah, it's probably overengineering, but it's
easy enough to do.
-Peff
^ permalink raw reply related
* Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
From: Jonathan Nieder @ 2013-02-20 22:06 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Shawn O. Pearce
In-Reply-To: <20130220220359.GA1417@sigill.intra.peff.net>
Jeff King wrote:
> On Wed, Feb 20, 2013 at 02:01:14PM -0800, Jonathan Nieder wrote:
>>>> How about
>>>>
>>>> die("BUG: another thread changed SIGPIPE handling behind my back!");
>>>>
>>>> to make it easier to find and fix such problems?
>>>
>>> You mean for the "should never happen" bit, not the first part, right? I
>>> actually wonder if we should simply exit(141) in the first place. That
>>> is shell exit-code for SIGPIPE death already (so it's what our
>>> run_command would show us, and what anybody running us through shell
>>> would see).
>>
>> Yes, for the "should never happen" part.
[...]
> I don't mind adding a "BUG: " message like you described, but we should
> still try to exit(141) as the backup, since that is the shell-equivalent
> code to the SIGPIPE signal death.
If you want. :)
I think caring about graceful degradation of behavior in the case of
an assertion failure is overengineering, but it's mostly harmless.
^ permalink raw reply
* Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
From: Jeff King @ 2013-02-20 22:03 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Junio C Hamano, Shawn O. Pearce
In-Reply-To: <20130220220114.GB24236@google.com>
On Wed, Feb 20, 2013 at 02:01:14PM -0800, Jonathan Nieder wrote:
> >> How about
> >>
> >> die("BUG: another thread changed SIGPIPE handling behind my back!");
> >>
> >> to make it easier to find and fix such problems?
> >
> > You mean for the "should never happen" bit, not the first part, right? I
> > actually wonder if we should simply exit(141) in the first place. That
> > is shell exit-code for SIGPIPE death already (so it's what our
> > run_command would show us, and what anybody running us through shell
> > would see).
>
> Yes, for the "should never happen" part. Raising a signal is nice
> because it means the wait()-ing process can see what happened by
> checking WIFSIGNALED(status).
Right. My point is that only happens if there's no shell in the way. But
I guess it doesn't hurt to make the attempt to help the people using
wait() directly.
I don't mind adding a "BUG: " message like you described, but we should
still try to exit(141) as the backup, since that is the shell-equivalent
code to the SIGPIPE signal death.
-Peff
^ permalink raw reply
* Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
From: Jonathan Nieder @ 2013-02-20 22:01 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Shawn O. Pearce
In-Reply-To: <20130220215845.GB817@sigill.intra.peff.net>
Jeff King wrote:
> On Wed, Feb 20, 2013 at 01:51:11PM -0800, Jonathan Nieder wrote:
>>> + if (err == EPIPE) {
>>> + signal(SIGPIPE, SIG_DFL);
>>> + raise(SIGPIPE);
>>> + /* Should never happen, but just in case... */
>>> + exit(141);
>>
>> How about
>>
>> die("BUG: another thread changed SIGPIPE handling behind my back!");
>>
>> to make it easier to find and fix such problems?
>
> You mean for the "should never happen" bit, not the first part, right? I
> actually wonder if we should simply exit(141) in the first place. That
> is shell exit-code for SIGPIPE death already (so it's what our
> run_command would show us, and what anybody running us through shell
> would see).
Yes, for the "should never happen" part. Raising a signal is nice
because it means the wait()-ing process can see what happened by
checking WIFSIGNALED(status).
Jonathan
^ permalink raw reply
* Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
From: Jeff King @ 2013-02-20 21:58 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Junio C Hamano, Shawn O. Pearce
In-Reply-To: <20130220215043.GA24236@google.com>
On Wed, Feb 20, 2013 at 01:51:11PM -0800, Jonathan Nieder wrote:
> > This distinction doesn't typically matter in git, because we
> > do not ignore SIGPIPE in the first place. Which means that
> > we will not get EPIPE, but instead will just die when we get
> > a SIGPIPE. But it's possible for a default handler to be set
> > by a parent process,
>
> Not so much "default" as "insane inherited", as in the example
> of old versions of Python's subprocess.Popen.
It's possible that somebody could have a legitimate reason for doing so.
I just can't think of one. :)
> I suspect this used exit(0) instead of raise(SIGPIPE) in the first
> place to work around a bash bug (too much verbosity about SIGPIPE).
> If any programs still have that kind of bug, I'd rather put pressure
> on them to fix it by *not* working around it. So the basic idea here
> looks good to me.
Yeah, if you look for old discussions on SIGPIPE in the git list, it is
mostly Linus complaining about the bash behavior, and this code does
date back to that era. The bash bug is long since fixed.
> > + if (err == EPIPE) {
> > + signal(SIGPIPE, SIG_DFL);
> > + raise(SIGPIPE);
> > + /* Should never happen, but just in case... */
> > + exit(141);
>
> How about
>
> die("BUG: another thread changed SIGPIPE handling behind my back!");
>
> to make it easier to find and fix such problems?
You mean for the "should never happen" bit, not the first part, right? I
actually wonder if we should simply exit(141) in the first place. That
is shell exit-code for SIGPIPE death already (so it's what our
run_command would show us, and what anybody running us through shell
would see).
-Peff
^ permalink raw reply
* Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
From: Jonathan Nieder @ 2013-02-20 21:51 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Shawn O. Pearce
In-Reply-To: <20130220200136.GH25647@sigill.intra.peff.net>
Jeff King wrote:
> The write_or_die function will always die on an error,
> including EPIPE. However, it currently treats EPIPE
> specially by suppressing any error message, and by exiting
> with exit code 0.
>
> Suppressing the error message makes some sense; a pipe death
> may just be a sign that the other side is not interested in
> what we have to say. However, exiting with a successful
> error code is not a good idea, as write_or_die is frequently
> used in cases where we want to be careful about having
> written all of the output, and we may need to signal to our
> caller that we have done so (e.g., you would not want a push
> whose other end has hung up to report success).
>
> This distinction doesn't typically matter in git, because we
> do not ignore SIGPIPE in the first place. Which means that
> we will not get EPIPE, but instead will just die when we get
> a SIGPIPE. But it's possible for a default handler to be set
> by a parent process,
Not so much "default" as "insane inherited", as in the example
of old versions of Python's subprocess.Popen.
I suspect this used exit(0) instead of raise(SIGPIPE) in the first
place to work around a bash bug (too much verbosity about SIGPIPE).
If any programs still have that kind of bug, I'd rather put pressure
on them to fix it by *not* working around it. So the basic idea here
looks good to me.
[...]
> --- a/write_or_die.c
> +++ b/write_or_die.c
> @@ -1,5 +1,15 @@
> #include "cache.h"
>
> +static void check_pipe(int err)
> +{
> + if (err == EPIPE) {
> + signal(SIGPIPE, SIG_DFL);
> + raise(SIGPIPE);
> + /* Should never happen, but just in case... */
> + exit(141);
How about
die("BUG: another thread changed SIGPIPE handling behind my back!");
to make it easier to find and fix such problems?
Thanks,
Jonathan
^ permalink raw reply
* Re: Merge with staged and unstaged changes
From: Edward Thomson @ 2013-02-20 21:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <7vobfeybwt.fsf@alter.siamese.dyndns.org>
On 2/20/13 2:21 PM, "Junio C Hamano" <gitster@pobox.com> wrote:
>Both are very much on purpose. The integrator may have seen the
>patch on the list, ran "git apply [--index]" on it to contemplate on
>it, and before commiting the result, saw a pull request for a branch
>that contains the change. The above two allow the pull from such a
>state to succeed without losing any information.
>I think we have a similar table in Documentation/technical area that
>explains these things, by the way.
I believe you are referring to trivial-merge.txt which has been
exceptionally helpful in understanding "what" unpack trees does.
I appreciate this detailed explanation in providing the "why".
I also appreciate your explanation of the affect of the workdir,
and that makes sense. I would have expected that the default was
to presume the workdir files were existent, rather than the
other way around, but we can agree that is an implementation detail.
My biggest concern, of course, was having the unstaged files in my
workdir overwritten or deleted.
Thanks again-
-ed
^ permalink raw reply
* Re: Merge with staged and unstaged changes
From: Junio C Hamano @ 2013-02-20 20:21 UTC (permalink / raw)
To: Edward Thomson; +Cc: git@vger.kernel.org
In-Reply-To: <A54CE3E330039942B33B670D971F85740396B267@TK5EX14MBXC254.redmond.corp.microsoft.com>
Edward Thomson <ethomson@microsoft.com> writes:
> What was surprising to me was that my merge can proceed if I stage a change
> that is identical to the merge result. That is, if my merge result would
> be to take the contents from "theirs", then my merge can proceed if I've
> already staged the same contents:
>
> input result
> anc ours theirs idx wd merge result idx wd
> 4 A A B B B take B B B
> 5 A A B B C take B B C
>
> This seems unexpected - is there a use-case that this enables or is
> this accidental?
Both are very much on purpose. The integrator may have seen the
patch on the list, ran "git apply [--index]" on it to contemplate on
it, and before commiting the result, saw a pull request for a branch
that contains the change. The above two allow the pull from such a
state to succeed without losing any information.
I think we have a similar table in Documentation/technical area that
explains these things, by the way.
> Another surprising result was that if I have deleted a file (and staged
> the deletion or not) then the merge will proceed and the file in question
> will be recreated. Consider "X" to be a missing file:
>
> input result
> anc ours theirs idx wd merge result idx wd
> 6 A A B A X take B B B
> 7 A A B X X take B B B
I am not sure about #7, but #6 is done very much on purpose. The
lower level merge machinery deliberately equates "in index but not
checked out to the working tree" state and "in index and not
modified in the working tree" state; this is to support a merge done
in a temporary working area that starts out empty.
This was designed really long time ago (read: during the first two
weeks of Git development, back when there were no "stash" nor
"remote tracking branches"), with a vision to make this scenario
work nicely:
You have checked out 'master' branch and you are working on it.
You see a more urgent pull request on 'maint' branch. But your
working tree for 'master' is no shape to commit, yet.
Without disturbing the working area you are using to advance the
'master' branch, you could do the merge "only in the index" by doing:
* Fetch the requested commit:
$ git fetch $over_there refs/heads/master
* Populate a temporary index with the contents of your 'maint':
$ GIT_INDEX_FILE=,,temp-index GIT_DIR=$(pwd)/.git
$ export GIT_INDEX_FILE GIT_DIR
$ git read-tree refs/heads/maint
* Create an empty temporary working area and go there:
$ mkdir ,,temp-merge
$ cd ,,temp-merge
* Run the three-way merge between your 'maint' and the requested
commit:
$ MB=$(git merge-base FETCH_HEAD maint)
$ git read-tree -m -u $MB maint FETCH_HEAD
Notice that we start without _any_ file in that temporary working
area (,,temp-merge directory). In the last step, because the merge
machinery (read-tree -m -u) treats missing files as "they are
unmodified by us; we didn't even bother checking them out of the
index", we will see _only_ the files that are different from our
'maint' and files that needs our help to get conflicts resolved
in the temporary working area after the command finishes. You
inspect them, resolve conflicts and run "git update-index" on them
(remember, there weren't "git add" or "git commit -a"), write the
resulting index as a tree with "git write-tree" and record the tree
with "git commit-tree -p maint -p FETCH_HEAD" (actually, back then
"commit-tree -p" insisted on getting raw tree object names, so you
had to do the equivalent of "git rev-parse maint^{tree}" there) and
then update your 'maint' branch with the resulting commit.
If there is no conflict to be resolved, then you essentially can run
a script that does all of the above (and cleans up the temporary
directory ,,temp-merge and the temporary index) to implement "Can
perform a merge into a branch that is not currently checked out,
without disturbing my current work" feature. And this "missing
files are unmodified---I didn't even bother to check them out of the
index" was done as an integral part of it.
Back when we (IIRC, mostly between Linus and I) were discussing this
design, it did come up that in the normal case of "I am merging in
my working tree", this behaviour would lose information, but "I'm
planning to remove this" is only a single bit and lossage of that
was judged to be trivially small and easily recoverable compared to
the benefit of being able to do a merge in an empty working tree,
and that is where this behaviour comes from.
We could certainly revisit this design and make the behaviour
optional. When somebody wants to do the "Can perform a merge into a
branch that is not currently checked out, without disturbing my
current work" feature, its implementation needs to be able to turn
it back on, but for doing everything else we do not have to treat a
missing file as unmodified.
^ permalink raw reply
* [PATCH v3 19/19] remote-curl: always parse incoming refs
From: Jeff King @ 2013-02-20 20:07 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
When remote-curl receives a list of refs from a server, it
keeps the whole buffer intact. When we get a "list" command,
we feed the result to get_remote_heads, and when we get a
"fetch" or "push" command, we feed it to fetch-pack or
send-pack, respectively.
If the HTTP response from the server is truncated for any
reason, we will get an incomplete ref advertisement. If we
then feed this incomplete list to fetch-pack, one of a few
things may happen:
1. If the truncation is in a packet header, fetch-pack
will notice the bogus line and complain.
2. If the truncation is inside a packet, fetch-pack will
keep waiting for us to send the rest of the packet,
which we never will.
3. If the truncation is at a packet boundary, fetch-pack
will keep waiting for us to send the next packet, which
we never will.
As a result, fetch-pack hangs, waiting for input. However,
remote-curl believes it has sent all of the advertisement,
and therefore waits for fetch-pack to speak. The two
processes end up in a deadlock.
We do notice the broken ref list if we feed it to
get_remote_heads. So if git asks the helper to do a "list"
followed by a "fetch", we are safe; we'll abort during the
list operation, which parses the refs.
This patch teaches remote-curl to always parse and save the
incoming ref list when we read the ref advertisement from a
server. That means that we will always verify and abort
before even running fetch-pack (or send-pack) when reading a
corrupted list, even if we do not run the "list" command
explicitly.
Since we save the result, in the common case of running
"list" then "fetch", we do not do any extra parsing at all.
In the case of just a "fetch", we do an extra round of
parsing, but only once.
Note also that the "fetch" case will now also initialize
server_capabilities from the remote (in remote-curl; we
already would do so inside fetch-pack). Doing "list+fetch"
already does this. It doesn't actually matter now, but the
new behavior is arguably more correct, should remote-curl
ever start caring about the server's capability list.
Signed-off-by: Jeff King <peff@peff.net>
---
remote-curl.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 856decc..3d2b194 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -76,6 +76,7 @@ struct discovery {
char *buf_alloc;
char *buf;
size_t len;
+ struct ref *refs;
unsigned proto_git : 1;
};
static struct discovery *last_discovery;
@@ -145,11 +146,12 @@ static void free_discovery(struct discovery *d)
if (d == last_discovery)
last_discovery = NULL;
free(d->buf_alloc);
+ free_refs(d->refs);
free(d);
}
}
-static struct discovery* discover_refs(const char *service)
+static struct discovery* discover_refs(const char *service, int for_push)
{
struct strbuf exp = STRBUF_INIT;
struct strbuf type = STRBUF_INIT;
@@ -221,6 +223,11 @@ static struct discovery* discover_refs(const char *service)
last->proto_git = 1;
}
+ if (last->proto_git)
+ last->refs = parse_git_refs(last, for_push);
+ else
+ last->refs = parse_info_refs(last);
+
free(refs_url);
strbuf_release(&exp);
strbuf_release(&type);
@@ -234,13 +241,11 @@ static struct ref *get_refs(int for_push)
struct discovery *heads;
if (for_push)
- heads = discover_refs("git-receive-pack");
+ heads = discover_refs("git-receive-pack", for_push);
else
- heads = discover_refs("git-upload-pack");
+ heads = discover_refs("git-upload-pack", for_push);
- if (heads->proto_git)
- return parse_git_refs(heads, for_push);
- return parse_info_refs(heads);
+ return heads->refs;
}
static void output_refs(struct ref *refs)
@@ -254,7 +259,6 @@ static void output_refs(struct ref *refs)
}
printf("\n");
fflush(stdout);
- free_refs(refs);
}
struct rpc_state {
@@ -670,7 +674,7 @@ static int fetch(int nr_heads, struct ref **to_fetch)
static int fetch(int nr_heads, struct ref **to_fetch)
{
- struct discovery *d = discover_refs("git-upload-pack");
+ struct discovery *d = discover_refs("git-upload-pack", 0);
if (d->proto_git)
return fetch_git(d, nr_heads, to_fetch);
else
@@ -789,7 +793,7 @@ static int push(int nr_spec, char **specs)
static int push(int nr_spec, char **specs)
{
- struct discovery *heads = discover_refs("git-receive-pack");
+ struct discovery *heads = discover_refs("git-receive-pack", 1);
int ret;
if (heads->proto_git)
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* [PATCH v3 18/19] remote-curl: move ref-parsing code up in file
From: Jeff King @ 2013-02-20 20:07 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
The ref-parsing functions are static. Let's move them up in
the file to be available to more functions, which will help
us with later refactoring.
Signed-off-by: Jeff King <peff@peff.net>
---
remote-curl.c | 118 +++++++++++++++++++++++++++++-----------------------------
1 file changed, 59 insertions(+), 59 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index e07f654..856decc 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -80,6 +80,65 @@ static struct discovery *last_discovery;
};
static struct discovery *last_discovery;
+static struct ref *parse_git_refs(struct discovery *heads, int for_push)
+{
+ struct ref *list = NULL;
+ get_remote_heads(-1, heads->buf, heads->len, &list,
+ for_push ? REF_NORMAL : 0, NULL);
+ return list;
+}
+
+static struct ref *parse_info_refs(struct discovery *heads)
+{
+ char *data, *start, *mid;
+ char *ref_name;
+ int i = 0;
+
+ struct ref *refs = NULL;
+ struct ref *ref = NULL;
+ struct ref *last_ref = NULL;
+
+ data = heads->buf;
+ start = NULL;
+ mid = data;
+ while (i < heads->len) {
+ if (!start) {
+ start = &data[i];
+ }
+ if (data[i] == '\t')
+ mid = &data[i];
+ if (data[i] == '\n') {
+ if (mid - start != 40)
+ die("%sinfo/refs not valid: is this a git repository?", url);
+ data[i] = 0;
+ ref_name = mid + 1;
+ ref = xmalloc(sizeof(struct ref) +
+ strlen(ref_name) + 1);
+ memset(ref, 0, sizeof(struct ref));
+ strcpy(ref->name, ref_name);
+ get_sha1_hex(start, ref->old_sha1);
+ if (!refs)
+ refs = ref;
+ if (last_ref)
+ last_ref->next = ref;
+ last_ref = ref;
+ start = NULL;
+ }
+ i++;
+ }
+
+ ref = alloc_ref("HEAD");
+ if (!http_fetch_ref(url, ref) &&
+ !resolve_remote_symref(ref, refs)) {
+ ref->next = refs;
+ refs = ref;
+ } else {
+ free(ref);
+ }
+
+ return refs;
+}
+
static void free_discovery(struct discovery *d)
{
if (d) {
@@ -170,65 +229,6 @@ static struct discovery* discover_refs(const char *service)
return last;
}
-static struct ref *parse_git_refs(struct discovery *heads, int for_push)
-{
- struct ref *list = NULL;
- get_remote_heads(-1, heads->buf, heads->len, &list,
- for_push ? REF_NORMAL : 0, NULL);
- return list;
-}
-
-static struct ref *parse_info_refs(struct discovery *heads)
-{
- char *data, *start, *mid;
- char *ref_name;
- int i = 0;
-
- struct ref *refs = NULL;
- struct ref *ref = NULL;
- struct ref *last_ref = NULL;
-
- data = heads->buf;
- start = NULL;
- mid = data;
- while (i < heads->len) {
- if (!start) {
- start = &data[i];
- }
- if (data[i] == '\t')
- mid = &data[i];
- if (data[i] == '\n') {
- if (mid - start != 40)
- die("%sinfo/refs not valid: is this a git repository?", url);
- data[i] = 0;
- ref_name = mid + 1;
- ref = xmalloc(sizeof(struct ref) +
- strlen(ref_name) + 1);
- memset(ref, 0, sizeof(struct ref));
- strcpy(ref->name, ref_name);
- get_sha1_hex(start, ref->old_sha1);
- if (!refs)
- refs = ref;
- if (last_ref)
- last_ref->next = ref;
- last_ref = ref;
- start = NULL;
- }
- i++;
- }
-
- ref = alloc_ref("HEAD");
- if (!http_fetch_ref(url, ref) &&
- !resolve_remote_symref(ref, refs)) {
- ref->next = refs;
- refs = ref;
- } else {
- free(ref);
- }
-
- return refs;
-}
-
static struct ref *get_refs(int for_push)
{
struct discovery *heads;
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* [PATCH v3 17/19] remote-curl: pass buffer straight to get_remote_heads
From: Jeff King @ 2013-02-20 20:07 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
Until recently, get_remote_heads only knew how to read refs
from a file descriptor. To hack around this, we spawned a
thread (or forked a process) to write the buffer back to us.
Now that we can just pass it our buffer directly, we don't
have to use this hack anymore.
Signed-off-by: Jeff King <peff@peff.net>
---
remote-curl.c | 26 ++------------------------
1 file changed, 2 insertions(+), 24 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 3bc6cb5..e07f654 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -170,33 +170,11 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)
return last;
}
-static int write_discovery(int in, int out, void *data)
-{
- struct discovery *heads = data;
- int err = 0;
- if (write_in_full(out, heads->buf, heads->len) != heads->len)
- err = 1;
- close(out);
- return err;
-}
-
static struct ref *parse_git_refs(struct discovery *heads, int for_push)
{
struct ref *list = NULL;
- struct async async;
-
- memset(&async, 0, sizeof(async));
- async.proc = write_discovery;
- async.data = heads;
- async.out = -1;
-
- if (start_async(&async))
- die("cannot start thread to parse advertised refs");
- get_remote_heads(async.out, NULL, 0, &list,
- for_push ? REF_NORMAL : 0, NULL);
- close(async.out);
- if (finish_async(&async))
- die("ref parsing thread failed");
+ get_remote_heads(-1, heads->buf, heads->len, &list,
+ for_push ? REF_NORMAL : 0, NULL);
return list;
}
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* [PATCH v3 16/19] teach get_remote_heads to read from a memory buffer
From: Jeff King @ 2013-02-20 20:06 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
Now that we can read packet data from memory as easily as a
descriptor, get_remote_heads can take either one as a
source. This will allow further refactoring in remote-curl.
Signed-off-by: Jeff King <peff@peff.net>
---
Another "wrapper vs NULL argument" opportunity. I could go either way if
we feel strongly in one direction. A third option is:
struct packet_source {
/* Choose one. */
int fd;
char *buf;
int len;
};
but then each caller has to be bothered to define and fill in the
struct, which ends up even uglier.
builtin/fetch-pack.c | 2 +-
builtin/send-pack.c | 2 +-
cache.h | 4 +++-
connect.c | 6 +++---
remote-curl.c | 2 +-
transport.c | 6 +++---
6 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index c21cc2c..03ed2ca 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -125,7 +125,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
args.verbose ? CONNECT_VERBOSE : 0);
}
- get_remote_heads(fd[0], &ref, 0, NULL);
+ get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL);
ref = fetch_pack(&args, fd, conn, ref, dest,
&sought, pack_lockfile_ptr);
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 8778519..152c4ea 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -207,7 +207,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
memset(&extra_have, 0, sizeof(extra_have));
- get_remote_heads(fd[0], &remote_refs, REF_NORMAL, &extra_have);
+ get_remote_heads(fd[0], NULL, 0, &remote_refs, REF_NORMAL, &extra_have);
transport_verify_remote_names(nr_refspecs, refspecs);
diff --git a/cache.h b/cache.h
index e493563..db646a2 100644
--- a/cache.h
+++ b/cache.h
@@ -1049,7 +1049,9 @@ struct extra_have_objects {
int nr, alloc;
unsigned char (*array)[20];
};
-extern struct ref **get_remote_heads(int in, struct ref **list, unsigned int flags, struct extra_have_objects *);
+extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
+ struct ref **list, unsigned int flags,
+ struct extra_have_objects *);
extern int server_supports(const char *feature);
extern int parse_feature_request(const char *features, const char *feature);
extern const char *server_feature_value(const char *feature, int *len_ret);
diff --git a/connect.c b/connect.c
index 061aa5b..f57efd0 100644
--- a/connect.c
+++ b/connect.c
@@ -62,8 +62,8 @@ static void die_initial_contact(int got_at_least_one_head)
/*
* Read all the refs from the other end
*/
-struct ref **get_remote_heads(int in, struct ref **list,
- unsigned int flags,
+struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
+ struct ref **list, unsigned int flags,
struct extra_have_objects *extra_have)
{
int got_at_least_one_head = 0;
@@ -76,7 +76,7 @@ struct ref **get_remote_heads(int in, struct ref **list,
int len, name_len;
char *buffer = packet_buffer;
- len = packet_read(in, NULL, 0,
+ len = packet_read(in, &src_buf, &src_len,
packet_buffer, sizeof(packet_buffer),
PACKET_READ_GENTLE_ON_EOF |
PACKET_READ_CHOMP_NEWLINE);
diff --git a/remote-curl.c b/remote-curl.c
index c0edd4c..3bc6cb5 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -192,7 +192,7 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)
if (start_async(&async))
die("cannot start thread to parse advertised refs");
- get_remote_heads(async.out, &list,
+ get_remote_heads(async.out, NULL, 0, &list,
for_push ? REF_NORMAL : 0, NULL);
close(async.out);
if (finish_async(&async))
diff --git a/transport.c b/transport.c
index 886ffd8..62df466 100644
--- a/transport.c
+++ b/transport.c
@@ -507,7 +507,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus
struct ref *refs;
connect_setup(transport, for_push, 0);
- get_remote_heads(data->fd[0], &refs,
+ get_remote_heads(data->fd[0], NULL, 0, &refs,
for_push ? REF_NORMAL : 0, &data->extra_have);
data->got_remote_heads = 1;
@@ -541,7 +541,7 @@ static int fetch_refs_via_pack(struct transport *transport,
if (!data->got_remote_heads) {
connect_setup(transport, 0, 0);
- get_remote_heads(data->fd[0], &refs_tmp, 0, NULL);
+ get_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0, NULL);
data->got_remote_heads = 1;
}
@@ -799,7 +799,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
struct ref *tmp_refs;
connect_setup(transport, 1, 0);
- get_remote_heads(data->fd[0], &tmp_refs, REF_NORMAL, NULL);
+ get_remote_heads(data->fd[0], NULL, 0, &tmp_refs, REF_NORMAL, NULL);
data->got_remote_heads = 1;
}
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* [PATCH v3 15/19] pkt-line: share buffer/descriptor reading implementation
From: Jeff King @ 2013-02-20 20:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
The packet_read function reads from a descriptor. The
packet_get_line function is similar, but reads from an
in-memory buffer, and uses a completely separate
implementation. This patch teaches the generic packet_read
function to accept either source, and we can do away with
packet_get_line's implementation.
There are two other differences to account for between the
old and new functions. The first is that we used to read
into a strbuf, but now read into a fixed size buffer. The
only two callers are fine with that, and in fact it
simplifies their code, since they can use the same
static-buffer interface as the rest of the packet_read_line
callers (and we provide a similar convenience wrapper for
reading from a buffer rather than a descriptor).
This is technically an externally-visible behavior change in
that we used to accept arbitrary sized packets up to 65532
bytes, and now cap out at LARGE_PACKET_MAX, 65520. In
practice this doesn't matter, as we use it only for parsing
smart-http headers (of which there is exactly one defined,
and it is small and fixed-size). And any extension headers
would be breaking the protocol to go over LARGE_PACKET_MAX
anyway.
The other difference is that packet_get_line would return
on error rather than dying. However, both callers of
strbuf_get_line are actually improved by dying.
The first caller does its own error checking, but we can
drop that; as a result, we'll actually get more specific
reporting about protocol breakage when packet_read dies
internally. The only downside is that packet_read will not
print the smart-http URL that failed, but that's not a big
deal; anybody not debugging can already see the remote's URL
already, and anybody debugging would want to run with
GIT_CURL_VERBOSE anyway to see way more information.
The second caller, which is just trying to skip past any
extra smart-http headers (of which there are none defined,
but which we allow to keep room for future expansion), did
not error check at all. As a result, it would treat an error
just like a flush packet. The resulting mess would generally
cause an error later in get_remote_heads, but now we get
error reporting much closer to the source of the problem.
Signed-off-by: Jeff King <peff@peff.net>
---
This adds two options to the generic packet_read interface for which
many callers will just pass (NULL, 0). We can hide that behind a
wrapper, but I was annoyed with the proliferation of wrappers from the
last round. Pick your poison.
connect.c | 3 ++-
daemon.c | 2 +-
pkt-line.c | 77 ++++++++++++++++++++++++++++++-----------------------------
pkt-line.h | 23 +++++++++++++-----
remote-curl.c | 22 ++++++++---------
sideband.c | 2 +-
6 files changed, 70 insertions(+), 59 deletions(-)
diff --git a/connect.c b/connect.c
index 611ffb4..061aa5b 100644
--- a/connect.c
+++ b/connect.c
@@ -76,7 +76,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
int len, name_len;
char *buffer = packet_buffer;
- len = packet_read(in, packet_buffer, sizeof(packet_buffer),
+ len = packet_read(in, NULL, 0,
+ packet_buffer, sizeof(packet_buffer),
PACKET_READ_GENTLE_ON_EOF |
PACKET_READ_CHOMP_NEWLINE);
if (len < 0)
diff --git a/daemon.c b/daemon.c
index 3f70e79..9a241d9 100644
--- a/daemon.c
+++ b/daemon.c
@@ -612,7 +612,7 @@ static int execute(void)
loginfo("Connection from %s:%s", addr, port);
alarm(init_timeout ? init_timeout : timeout);
- pktlen = packet_read(0, packet_buffer, sizeof(packet_buffer), 0);
+ pktlen = packet_read(0, NULL, 0, packet_buffer, sizeof(packet_buffer), 0);
alarm(0);
len = strlen(line);
diff --git a/pkt-line.c b/pkt-line.c
index 55fb688..2c47052 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -104,12 +104,29 @@ static int safe_read(int fd, void *buffer, unsigned size, int options)
strbuf_add(buf, buffer, n);
}
-static int safe_read(int fd, void *buffer, unsigned size, int options)
+static int get_packet_data(int fd, char **src_buf, size_t *src_size,
+ void *dst, unsigned size, int options)
{
- ssize_t ret = read_in_full(fd, buffer, size);
- if (ret < 0)
- die_errno("read error");
- else if (ret < size) {
+ ssize_t ret;
+
+ if (fd >= 0 && src_buf && *src_buf)
+ die("BUG: multiple sources given to packet_read");
+
+ /* Read up to "size" bytes from our source, whatever it is. */
+ if (src_buf && *src_buf) {
+ ret = size < *src_size ? size : *src_size;
+ memcpy(dst, *src_buf, ret);
+ *src_buf += size;
+ *src_size -= size;
+ }
+ else {
+ ret = read_in_full(fd, dst, size);
+ if (ret < 0)
+ die_errno("read error");
+ }
+
+ /* And complain if we didn't get enough bytes to satisfy the read. */
+ if (ret < size) {
if (options & PACKET_READ_GENTLE_ON_EOF)
return -1;
@@ -144,12 +161,13 @@ int packet_read(int fd, char *buffer, unsigned size, int options)
return len;
}
-int packet_read(int fd, char *buffer, unsigned size, int options)
+int packet_read(int fd, char **src_buf, size_t *src_len,
+ char *buffer, unsigned size, int options)
{
int len, ret;
char linelen[4];
- ret = safe_read(fd, linelen, 4, options);
+ ret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);
if (ret < 0)
return ret;
len = packet_length(linelen);
@@ -162,7 +180,7 @@ int packet_read(int fd, char *buffer, unsigned size, int options)
len -= 4;
if (len >= size)
die("protocol error: bad line length %d", len);
- ret = safe_read(fd, buffer, len, options);
+ ret = get_packet_data(fd, src_buf, src_len, buffer, len, options);
if (ret < 0)
return ret;
@@ -175,41 +193,24 @@ int packet_get_line(struct strbuf *out,
return len;
}
-char *packet_read_line(int fd, int *len_p)
+static char *packet_read_line_generic(int fd,
+ char **src, size_t *src_len,
+ int *dst_len)
{
- int len = packet_read(fd, packet_buffer, sizeof(packet_buffer),
+ int len = packet_read(fd, src, src_len,
+ packet_buffer, sizeof(packet_buffer),
PACKET_READ_CHOMP_NEWLINE);
- if (len_p)
- *len_p = len;
+ if (dst_len)
+ *dst_len = len;
return len ? packet_buffer : NULL;
}
-int packet_get_line(struct strbuf *out,
- char **src_buf, size_t *src_len)
+char *packet_read_line(int fd, int *len_p)
{
- int len;
-
- if (*src_len < 4)
- return -1;
- len = packet_length(*src_buf);
- if (len < 0)
- return -1;
- if (!len) {
- *src_buf += 4;
- *src_len -= 4;
- packet_trace("0000", 4, 0);
- return 0;
- }
- if (*src_len < len)
- return -2;
-
- *src_buf += 4;
- *src_len -= 4;
- len -= 4;
+ return packet_read_line_generic(fd, NULL, 0, len_p);
+}
- strbuf_add(out, *src_buf, len);
- *src_buf += len;
- *src_len -= len;
- packet_trace(out->buf, out->len, 0);
- return len;
+char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)
+{
+ return packet_read_line_generic(-1, src, src_len, dst_len);
}
diff --git a/pkt-line.h b/pkt-line.h
index fa93e32..47361f5 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -25,9 +25,16 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((f
void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
/*
- * Read a packetized line from the descriptor into the buffer, which must be at
- * least size bytes long. The return value specifies the number of bytes read
- * into the buffer.
+ * Read a packetized line into the buffer, which must be at least size bytes
+ * long. The return value specifies the number of bytes read into the buffer.
+ *
+ * If src_buffer is not NULL (and nor is *src_buffer), it should point to a
+ * buffer containing the packet data to parse, of at least *src_len bytes.
+ * After the function returns, src_buf will be increments and src_len
+ * decremented by the number of bytes consumed.
+ *
+ * If src_buffer (or *src_buffer) is NULL, then data is read from the
+ * descriptor "fd".
*
* If options does not contain PACKET_READ_GENTLE_ON_EOF, we will die under any
* of the following conditions:
@@ -50,7 +57,8 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((f
*/
#define PACKET_READ_GENTLE_ON_EOF (1u<<0)
#define PACKET_READ_CHOMP_NEWLINE (1u<<1)
-int packet_read(int fd, char *buffer, unsigned size, int options);
+int packet_read(int fd, char **src_buffer, size_t *src_len, char
+ *buffer, unsigned size, int options);
/*
* Convenience wrapper for packet_read that is not gentle, and sets the
@@ -61,11 +69,14 @@ extern char packet_buffer[LARGE_PACKET_MAX];
*/
char *packet_read_line(int fd, int *size);
+/*
+ * Same as packet_read_line, but read from a buf rather than a descriptor;
+ * see packet_read for details on how src_* is used.
+ */
+char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
#define DEFAULT_PACKET_MAX 1000
#define LARGE_PACKET_MAX 65520
extern char packet_buffer[LARGE_PACKET_MAX];
-int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
-
#endif
diff --git a/remote-curl.c b/remote-curl.c
index b28f965..c0edd4c 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -138,28 +138,26 @@ static struct discovery* discover_refs(const char *service)
if (maybe_smart &&
(5 <= last->len && last->buf[4] == '#') &&
!strbuf_cmp(&exp, &type)) {
+ char *line;
+
/*
* smart HTTP response; validate that the service
* pkt-line matches our request.
*/
- if (packet_get_line(&buffer, &last->buf, &last->len) <= 0)
- die("%s has invalid packet header", refs_url);
- if (buffer.len && buffer.buf[buffer.len - 1] == '\n')
- strbuf_setlen(&buffer, buffer.len - 1);
+ line = packet_read_line_buf(&last->buf, &last->len, NULL);
strbuf_reset(&exp);
strbuf_addf(&exp, "# service=%s", service);
- if (strbuf_cmp(&exp, &buffer))
- die("invalid server response; got '%s'", buffer.buf);
+ if (strcmp(line, exp.buf))
+ die("invalid server response; got '%s'", line);
strbuf_release(&exp);
/* The header can include additional metadata lines, up
* until a packet flush marker. Ignore these now, but
* in the future we might start to scan them.
*/
- strbuf_reset(&buffer);
- while (packet_get_line(&buffer, &last->buf, &last->len) > 0)
- strbuf_reset(&buffer);
+ while (packet_read_line_buf(&last->buf, &last->len, NULL) > 0)
+ ;
last->proto_git = 1;
}
@@ -308,7 +306,7 @@ static size_t rpc_out(void *ptr, size_t eltsize,
if (!avail) {
rpc->initial_buffer = 0;
- avail = packet_read(rpc->out, rpc->buf, rpc->alloc, 0);
+ avail = packet_read(rpc->out, NULL, 0, rpc->buf, rpc->alloc, 0);
if (!avail)
return 0;
rpc->pos = 0;
@@ -425,7 +423,7 @@ static int post_rpc(struct rpc_state *rpc)
break;
}
- n = packet_read(rpc->out, buf, left, 0);
+ n = packet_read(rpc->out, 0, NULL, buf, left, 0);
if (!n)
break;
rpc->len += n;
@@ -579,7 +577,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)
rpc->hdr_accept = strbuf_detach(&buf, NULL);
while (!err) {
- int n = packet_read(rpc->out, rpc->buf, rpc->alloc, 0);
+ int n = packet_read(rpc->out, 0, NULL, rpc->buf, rpc->alloc, 0);
if (!n)
break;
rpc->pos = 0;
diff --git a/sideband.c b/sideband.c
index 15cc1ae..857954c 100644
--- a/sideband.c
+++ b/sideband.c
@@ -38,7 +38,7 @@ int recv_sideband(const char *me, int in_stream, int out)
while (1) {
int band, len;
- len = packet_read(in_stream, buf + pf, LARGE_PACKET_MAX, 0);
+ len = packet_read(in_stream, NULL, 0, buf + pf, LARGE_PACKET_MAX, 0);
if (len == 0)
break;
if (len < 1) {
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* Re: Google Summer of Code 2013 (GSoC13)
From: Michael Schubert @ 2013-02-20 19:48 UTC (permalink / raw)
To: Jeff King
Cc: Thomas Rast, git, Shawn Pearce, Jakub Narebski, Christian Couder,
Pat Thoyts, Paul Mackerras, Carlos Martín Nieto,
Thomas Gummerer, David Michael Barr, Ramkumar Ramachandra,
Jens Lehmann, Nguyen Thai Ngoc Duy, Vicent Marti
In-Reply-To: <20130218174239.GB22832@sigill.intra.peff.net>
On 02/18/2013 06:42 PM, Jeff King wrote:
>
> I will do it again, if people feel strongly about Git being a part of
> it. However, I have gotten a little soured on the GSoC experience. Not
> because of anything Google has done; it's a good idea, and I think they
> do a fine of administering the program. But I have noticed that the work
> that comes out of GSoC the last few years has quite often not been
> merged, or not made a big impact in the codebase, and nor have the
> participants necessarily stuck around.
>
> And I do not want to blame the students here (some of whom are on the cc
> list :) ). They are certainly under no obligation to stick around after
> GSoC ends, and I know they have many demands on their time. But I am
> also thinking about what Git wants to get out of GSoC (and to my mind,
> the most important thing is contributors).
Speaking of libgit2:
Git provided the libgit2 project with a slot each of the last three GSOC.
The contributions made by the former students (Disclaimer: one of them
speaking) have been quite important for libgit2 and all three students
are still involved. Each project was an important push towards building
a new, feature complete Git library.
Thank you!
http://libgit2.github.com
^ permalink raw reply
* [PATCH v3 14/19] pkt-line: provide a LARGE_PACKET_MAX static buffer
From: Jeff King @ 2013-02-20 20:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
Most of the callers of packet_read_line just read into a
static 1000-byte buffer (callers which handle arbitrary
binary data already use LARGE_PACKET_MAX). This works fine
in practice, because:
1. The only variable-sized data in these lines is a ref
name, and refs tend to be a lot shorter than 1000
characters.
2. When sending ref lines, git-core always limits itself
to 1000 byte packets.
However, the only limit given in the protocol specification
in Documentation/technical/protocol-common.txt is
LARGE_PACKET_MAX; the 1000 byte limit is mentioned only in
pack-protocol.txt, and then only describing what we write,
not as a specific limit for readers.
This patch lets us bump the 1000-byte limit to
LARGE_PACKET_MAX. Even though git-core will never write a
packet where this makes a difference, there are two good
reasons to do this:
1. Other git implementations may have followed
protocol-common.txt and used a larger maximum size. We
don't bump into it in practice because it would involve
very long ref names.
2. We may want to increase the 1000-byte limit one day.
Since packets are transferred before any capabilities,
it's difficult to do this in a backwards-compatible
way. But if we bump the size of buffer the readers can
handle, eventually older versions of git will be
obsolete enough that we can justify bumping the
writers, as well. We don't have plans to do this
anytime soon, but there is no reason not to start the
clock ticking now.
Just bumping all of the reading bufs to LARGE_PACKET_MAX
would waste memory. Instead, since most readers just read
into a temporary buffer anyway, let's provide a single
static buffer that all callers can use. We can further wrap
this detail away by having the packet_read_line wrapper just
use the buffer transparently and return a pointer to the
static storage. That covers most of the cases, and the
remaining ones already read into their own LARGE_PACKET_MAX
buffers.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/archive.c | 15 +++++++--------
builtin/fetch-pack.c | 7 +++----
builtin/receive-pack.c | 6 +++---
builtin/upload-archive.c | 7 ++-----
connect.c | 4 ++--
daemon.c | 4 ++--
fetch-pack.c | 12 ++++++------
pkt-line.c | 9 +++++++--
pkt-line.h | 9 +++++++--
send-pack.c | 7 +++----
upload-pack.c | 12 +++++-------
11 files changed, 47 insertions(+), 45 deletions(-)
diff --git a/builtin/archive.c b/builtin/archive.c
index d381ac4..49178f1 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -27,8 +27,8 @@ static int run_remote_archiver(int argc, const char **argv,
const char *remote, const char *exec,
const char *name_hint)
{
- char buf[LARGE_PACKET_MAX];
- int fd[2], i, len, rv;
+ char *buf;
+ int fd[2], i, rv;
struct transport *transport;
struct remote *_remote;
@@ -53,19 +53,18 @@ static int run_remote_archiver(int argc, const char **argv,
packet_write(fd[1], "argument %s\n", argv[i]);
packet_flush(fd[1]);
- len = packet_read_line(fd[0], buf, sizeof(buf));
- if (!len)
+ buf = packet_read_line(fd[0], NULL);
+ if (!buf)
die(_("git archive: expected ACK/NAK, got EOF"));
if (strcmp(buf, "ACK")) {
- if (len > 5 && !prefixcmp(buf, "NACK "))
+ if (!prefixcmp(buf, "NACK "))
die(_("git archive: NACK %s"), buf + 5);
- if (len > 4 && !prefixcmp(buf, "ERR "))
+ if (!prefixcmp(buf, "ERR "))
die(_("remote error: %s"), buf + 4);
die(_("git archive: protocol error"));
}
- len = packet_read_line(fd[0], buf, sizeof(buf));
- if (len)
+ if (packet_read_line(fd[0], NULL))
die(_("git archive: expected a flush"));
/* Now, start reading from fd[0] and spit it out to stdout */
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index f73664f..c21cc2c 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -100,12 +100,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
/* in stateless RPC mode we use pkt-line to read
* from stdin, until we get a flush packet
*/
- static char line[1000];
for (;;) {
- int n = packet_read_line(0, line, sizeof(line));
- if (!n)
+ char *line = packet_read_line(0, NULL);
+ if (!line)
break;
- string_list_append(&sought, xmemdupz(line, n));
+ string_list_append(&sought, xstrdup(line));
}
}
else {
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 6679e63..ccebd74 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -754,14 +754,14 @@ static struct command *read_head_info(void)
struct command *commands = NULL;
struct command **p = &commands;
for (;;) {
- static char line[1000];
+ char *line;
unsigned char old_sha1[20], new_sha1[20];
struct command *cmd;
char *refname;
int len, reflen;
- len = packet_read_line(0, line, sizeof(line));
- if (!len)
+ line = packet_read_line(0, &len);
+ if (!line)
break;
if (len < 83 ||
line[40] != ' ' ||
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 7d367b5..2a94675 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -21,8 +21,6 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
{
struct argv_array sent_argv = ARGV_ARRAY_INIT;
const char *arg_cmd = "argument ";
- char buf[4096];
- int len;
if (argc != 2)
usage(upload_archive_usage);
@@ -33,9 +31,8 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
/* put received options in sent_argv[] */
argv_array_push(&sent_argv, "git-upload-archive");
for (;;) {
- /* This will die if not enough free space in buf */
- len = packet_read_line(0, buf, sizeof(buf));
- if (len == 0)
+ char *buf = packet_read_line(0, NULL);
+ if (!buf)
break; /* got a flush */
if (sent_argv.argc > MAX_ARGS)
die("Too many options (>%d)", MAX_ARGS - 1);
diff --git a/connect.c b/connect.c
index fe8eb01..611ffb4 100644
--- a/connect.c
+++ b/connect.c
@@ -72,11 +72,11 @@ struct ref **get_remote_heads(int in, struct ref **list,
for (;;) {
struct ref *ref;
unsigned char old_sha1[20];
- static char buffer[1000];
char *name;
int len, name_len;
+ char *buffer = packet_buffer;
- len = packet_read(in, buffer, sizeof(buffer),
+ len = packet_read(in, packet_buffer, sizeof(packet_buffer),
PACKET_READ_GENTLE_ON_EOF |
PACKET_READ_CHOMP_NEWLINE);
if (len < 0)
diff --git a/daemon.c b/daemon.c
index 4f5cd61..3f70e79 100644
--- a/daemon.c
+++ b/daemon.c
@@ -604,7 +604,7 @@ static int execute(void)
static int execute(void)
{
- static char line[1000];
+ char *line = packet_buffer;
int pktlen, len, i;
char *addr = getenv("REMOTE_ADDR"), *port = getenv("REMOTE_PORT");
@@ -612,7 +612,7 @@ static int execute(void)
loginfo("Connection from %s:%s", addr, port);
alarm(init_timeout ? init_timeout : timeout);
- pktlen = packet_read(0, line, sizeof(line), 0);
+ pktlen = packet_read(0, packet_buffer, sizeof(packet_buffer), 0);
alarm(0);
len = strlen(line);
diff --git a/fetch-pack.c b/fetch-pack.c
index f830db2..66ff9ad 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -172,8 +172,8 @@ static void consume_shallow_list(struct fetch_pack_args *args, int fd)
* shallow and unshallow commands every time there
* is a block of have lines exchanged.
*/
- char line[1000];
- while (packet_read_line(fd, line, sizeof(line))) {
+ char *line;
+ while ((line = packet_read_line(fd, NULL))) {
if (!prefixcmp(line, "shallow "))
continue;
if (!prefixcmp(line, "unshallow "))
@@ -215,8 +215,8 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)
static enum ack_type get_ack(int fd, unsigned char *result_sha1)
{
- static char line[1000];
- int len = packet_read_line(fd, line, sizeof(line));
+ int len;
+ char *line = packet_read_line(fd, &len);
if (!len)
die("git fetch-pack: expected ACK/NAK, got EOF");
@@ -346,11 +346,11 @@ static int find_common(struct fetch_pack_args *args,
state_len = req_buf.len;
if (args->depth > 0) {
- char line[1024];
+ char *line;
unsigned char sha1[20];
send_request(args, fd[1], &req_buf);
- while (packet_read_line(fd[0], line, sizeof(line))) {
+ while ((line = packet_read_line(fd[0], NULL))) {
if (!prefixcmp(line, "shallow ")) {
if (get_sha1_hex(line + 8, sha1))
die("invalid shallow line: %s", line);
diff --git a/pkt-line.c b/pkt-line.c
index dc11c40..55fb688 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -1,6 +1,7 @@
#include "cache.h"
#include "pkt-line.h"
+char packet_buffer[LARGE_PACKET_MAX];
static const char *packet_trace_prefix = "git";
static const char trace_key[] = "GIT_TRACE_PACKET";
@@ -174,9 +175,13 @@ int packet_read_line(int fd, char *buffer, unsigned size)
return len;
}
-int packet_read_line(int fd, char *buffer, unsigned size)
+char *packet_read_line(int fd, int *len_p)
{
- return packet_read(fd, buffer, size, PACKET_READ_CHOMP_NEWLINE);
+ int len = packet_read(fd, packet_buffer, sizeof(packet_buffer),
+ PACKET_READ_CHOMP_NEWLINE);
+ if (len_p)
+ *len_p = len;
+ return len ? packet_buffer : NULL;
}
int packet_get_line(struct strbuf *out,
diff --git a/pkt-line.h b/pkt-line.h
index 6927ea5..fa93e32 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -54,12 +54,17 @@ int packet_read_line(int fd, char *buffer, unsigned size);
/*
* Convenience wrapper for packet_read that is not gentle, and sets the
- * CHOMP_NEWLINE option.
+ * CHOMP_NEWLINE option. The return value is NULL for a flush packet,
+ * and otherwise points to a static buffer (that may be overwritten by
+ * subsequent calls). If the size parameter is not NULL, the length of the
+ * packet is written to it.
*/
-int packet_read_line(int fd, char *buffer, unsigned size);
+char *packet_read_line(int fd, int *size);
+
#define DEFAULT_PACKET_MAX 1000
#define LARGE_PACKET_MAX 65520
+extern char packet_buffer[LARGE_PACKET_MAX];
int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
diff --git a/send-pack.c b/send-pack.c
index 8c230bf..7d172ef 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -106,9 +106,8 @@ static int receive_status(int in, struct ref *refs)
static int receive_status(int in, struct ref *refs)
{
struct ref *hint;
- char line[1000];
int ret = 0;
- int len = packet_read_line(in, line, sizeof(line));
+ char *line = packet_read_line(in, NULL);
if (prefixcmp(line, "unpack "))
return error("did not receive remote status");
if (strcmp(line, "unpack ok")) {
@@ -119,8 +118,8 @@ static int receive_status(int in, struct ref *refs)
while (1) {
char *refname;
char *msg;
- len = packet_read_line(in, line, sizeof(line));
- if (!len)
+ line = packet_read_line(in, NULL);
+ if (!line)
break;
if (prefixcmp(line, "ok ") && prefixcmp(line, "ng ")) {
error("invalid ref status from remote: %s", line);
diff --git a/upload-pack.c b/upload-pack.c
index 7446cb7..bc241ba 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -408,7 +408,6 @@ static int get_common_commits(void)
static int get_common_commits(void)
{
- static char line[1000];
unsigned char sha1[20];
char last_hex[41];
int got_common = 0;
@@ -418,10 +417,10 @@ static int get_common_commits(void)
save_commit_buffer = 0;
for (;;) {
- int len = packet_read_line(0, line, sizeof(line));
+ char *line = packet_read_line(0, NULL);
reset_timeout();
- if (!len) {
+ if (!line) {
if (multi_ack == 2 && got_common
&& !got_other && ok_to_give_up()) {
sent_ready = 1;
@@ -567,8 +566,7 @@ static void receive_needs(void)
static void receive_needs(void)
{
struct object_array shallows = OBJECT_ARRAY_INIT;
- static char line[1000];
- int len, depth = 0;
+ int depth = 0;
int has_non_tip = 0;
shallow_nr = 0;
@@ -576,9 +574,9 @@ static void receive_needs(void)
struct object *o;
const char *features;
unsigned char sha1_buf[20];
- len = packet_read_line(0, line, sizeof(line));
+ char *line = packet_read_line(0, NULL);
reset_timeout();
- if (!len)
+ if (!line)
break;
if (!prefixcmp(line, "shallow ")) {
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* [PATCH v3 13/19] pkt-line: move LARGE_PACKET_MAX definition from sideband
From: Jeff King @ 2013-02-20 20:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
Having the packet sizes defined near the packet read/write
functions makes more sense.
Signed-off-by: Jeff King <peff@peff.net>
---
http.c | 1 +
pkt-line.h | 3 +++
sideband.h | 3 ---
3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/http.c b/http.c
index d9d1aad..8803c70 100644
--- a/http.c
+++ b/http.c
@@ -5,6 +5,7 @@
#include "url.h"
#include "credential.h"
#include "version.h"
+#include "pkt-line.h"
int active_requests;
int http_is_verbose;
diff --git a/pkt-line.h b/pkt-line.h
index 5d2fb42..6927ea5 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -58,6 +58,9 @@ int packet_read_line(int fd, char *buffer, unsigned size);
*/
int packet_read_line(int fd, char *buffer, unsigned size);
+#define DEFAULT_PACKET_MAX 1000
+#define LARGE_PACKET_MAX 65520
+
int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
#endif
diff --git a/sideband.h b/sideband.h
index d72db35..e46bed0 100644
--- a/sideband.h
+++ b/sideband.h
@@ -4,9 +4,6 @@
#define SIDEBAND_PROTOCOL_ERROR -2
#define SIDEBAND_REMOTE_ERROR -1
-#define DEFAULT_PACKET_MAX 1000
-#define LARGE_PACKET_MAX 65520
-
int recv_sideband(const char *me, int in_stream, int out);
ssize_t send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* [PATCH v3 12/19] pkt-line: teach packet_read_line to chomp newlines
From: Jeff King @ 2013-02-20 20:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
The packets sent during ref negotiation are all terminated
by newline; even though the code to chomp these newlines is
short, we end up doing it in a lot of places.
This patch teaches packet_read_line to auto-chomp the
trailing newline; this lets us get rid of a lot of inline
chomping code.
As a result, some call-sites which are not reading
line-oriented data (e.g., when reading chunks of packfiles
alongside sideband) transition away from packet_read_line to
the generic packet_read interface. This patch converts all
of the existing callsites.
Since the function signature of packet_read_line does not
change (but its behavior does), there is a possibility of
new callsites being introduced in later commits, silently
introducing an incompatibility. However, since a later
patch in this series will change the signature, such a
commit would have to be merged directly into this commit,
not to the tip of the series; we can therefore ignore the
issue.
This is an internal cleanup and should produce no change of
behavior in the normal case. However, there is one corner
case to note. Callers of packet_read_line have never been
able to tell the difference between a flush packet ("0000")
and an empty packet ("0004"), as both cause packet_read_line
to return a length of 0. Readers treat them identically,
even though Documentation/technical/protocol-common.txt says
we must not; it also says that implementations should not
send an empty pkt-line.
By stripping out the newline before the result gets to the
caller, we will now treat the newline-only packet ("0005\n")
the same as an empty packet, which in turn gets treated like
a flush packet. In practice this doesn't matter, as neither
empty nor newline-only packets are part of git's protocols
(at least not for the line-oriented bits, and readers who
are not expecting line-oriented packets will be calling
packet_read directly, anyway). But even if we do decide to
care about the distinction later, it is orthogonal to this
patch. The right place to tighten would be to stop treating
empty packets as flush packets, and this change does not
make doing so any harder.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/archive.c | 2 --
builtin/fetch-pack.c | 2 --
builtin/receive-pack.c | 2 --
builtin/upload-archive.c | 4 ----
connect.c | 5 ++---
daemon.c | 2 +-
fetch-pack.c | 2 --
pkt-line.c | 7 ++++++-
pkt-line.h | 9 ++++++++-
remote-curl.c | 6 +++---
send-pack.c | 6 +-----
sideband.c | 2 +-
upload-pack.c | 8 --------
13 files changed, 22 insertions(+), 35 deletions(-)
diff --git a/builtin/archive.c b/builtin/archive.c
index 9a1cfd3..d381ac4 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -56,8 +56,6 @@ static int run_remote_archiver(int argc, const char **argv,
len = packet_read_line(fd[0], buf, sizeof(buf));
if (!len)
die(_("git archive: expected ACK/NAK, got EOF"));
- if (buf[len-1] == '\n')
- buf[--len] = 0;
if (strcmp(buf, "ACK")) {
if (len > 5 && !prefixcmp(buf, "NACK "))
die(_("git archive: NACK %s"), buf + 5);
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 940ae35..f73664f 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -105,8 +105,6 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
int n = packet_read_line(0, line, sizeof(line));
if (!n)
break;
- if (line[n-1] == '\n')
- n--;
string_list_append(&sought, xmemdupz(line, n));
}
}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9129563..6679e63 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -763,8 +763,6 @@ static struct command *read_head_info(void)
len = packet_read_line(0, line, sizeof(line));
if (!len)
break;
- if (line[len-1] == '\n')
- line[--len] = 0;
if (len < 83 ||
line[40] != ' ' ||
line[81] != ' ' ||
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 3393cef..7d367b5 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -40,10 +40,6 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
if (sent_argv.argc > MAX_ARGS)
die("Too many options (>%d)", MAX_ARGS - 1);
- if (buf[len-1] == '\n') {
- buf[--len] = 0;
- }
-
if (prefixcmp(buf, arg_cmd))
die("'argument' token or flush expected");
argv_array_push(&sent_argv, buf + strlen(arg_cmd));
diff --git a/connect.c b/connect.c
index 0aa202f..fe8eb01 100644
--- a/connect.c
+++ b/connect.c
@@ -77,14 +77,13 @@ struct ref **get_remote_heads(int in, struct ref **list,
int len, name_len;
len = packet_read(in, buffer, sizeof(buffer),
- PACKET_READ_GENTLE_ON_EOF);
+ PACKET_READ_GENTLE_ON_EOF |
+ PACKET_READ_CHOMP_NEWLINE);
if (len < 0)
die_initial_contact(got_at_least_one_head);
if (!len)
break;
- if (buffer[len-1] == '\n')
- buffer[--len] = 0;
if (len > 4 && !prefixcmp(buffer, "ERR "))
die("remote error: %s", buffer + 4);
diff --git a/daemon.c b/daemon.c
index 4602b46..4f5cd61 100644
--- a/daemon.c
+++ b/daemon.c
@@ -612,7 +612,7 @@ static int execute(void)
loginfo("Connection from %s:%s", addr, port);
alarm(init_timeout ? init_timeout : timeout);
- pktlen = packet_read_line(0, line, sizeof(line));
+ pktlen = packet_read(0, line, sizeof(line), 0);
alarm(0);
len = strlen(line);
diff --git a/fetch-pack.c b/fetch-pack.c
index b53a18f..f830db2 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -220,8 +220,6 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)
if (!len)
die("git fetch-pack: expected ACK/NAK, got EOF");
- if (line[len-1] == '\n')
- line[--len] = 0;
if (!strcmp(line, "NAK"))
return NAK;
if (!prefixcmp(line, "ACK ")) {
diff --git a/pkt-line.c b/pkt-line.c
index 8700cf8..dc11c40 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -164,6 +164,11 @@ int packet_read(int fd, char *buffer, unsigned size, int options)
ret = safe_read(fd, buffer, len, options);
if (ret < 0)
return ret;
+
+ if ((options & PACKET_READ_CHOMP_NEWLINE) &&
+ len && buffer[len-1] == '\n')
+ len--;
+
buffer[len] = 0;
packet_trace(buffer, len, 0);
return len;
@@ -171,7 +176,7 @@ int packet_read_line(int fd, char *buffer, unsigned size)
int packet_read_line(int fd, char *buffer, unsigned size)
{
- return packet_read(fd, buffer, size, 0);
+ return packet_read(fd, buffer, size, PACKET_READ_CHOMP_NEWLINE);
}
int packet_get_line(struct strbuf *out,
diff --git a/pkt-line.h b/pkt-line.h
index 8cd326c..5d2fb42 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -44,11 +44,18 @@ int packet_read(int fd, char *buffer, unsigned size, int options);
* If options does contain PACKET_READ_GENTLE_ON_EOF, we will not die on
* condition 4 (truncated input), but instead return -1. However, we will still
* die for the other 3 conditions.
+ *
+ * If options contains PACKET_READ_CHOMP_NEWLINE, a trailing newline (if
+ * present) is removed from the buffer before returning.
*/
#define PACKET_READ_GENTLE_ON_EOF (1u<<0)
+#define PACKET_READ_CHOMP_NEWLINE (1u<<1)
int packet_read(int fd, char *buffer, unsigned size, int options);
-/* Historical convenience wrapper for packet_read that sets no options */
+/*
+ * Convenience wrapper for packet_read that is not gentle, and sets the
+ * CHOMP_NEWLINE option.
+ */
int packet_read_line(int fd, char *buffer, unsigned size);
int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
diff --git a/remote-curl.c b/remote-curl.c
index 7be4b53..b28f965 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -308,7 +308,7 @@ static size_t rpc_out(void *ptr, size_t eltsize,
if (!avail) {
rpc->initial_buffer = 0;
- avail = packet_read_line(rpc->out, rpc->buf, rpc->alloc);
+ avail = packet_read(rpc->out, rpc->buf, rpc->alloc, 0);
if (!avail)
return 0;
rpc->pos = 0;
@@ -425,7 +425,7 @@ static int post_rpc(struct rpc_state *rpc)
break;
}
- n = packet_read_line(rpc->out, buf, left);
+ n = packet_read(rpc->out, buf, left, 0);
if (!n)
break;
rpc->len += n;
@@ -579,7 +579,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)
rpc->hdr_accept = strbuf_detach(&buf, NULL);
while (!err) {
- int n = packet_read_line(rpc->out, rpc->buf, rpc->alloc);
+ int n = packet_read(rpc->out, rpc->buf, rpc->alloc, 0);
if (!n)
break;
rpc->pos = 0;
diff --git a/send-pack.c b/send-pack.c
index bde796b..8c230bf 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -111,10 +111,7 @@ static int receive_status(int in, struct ref *refs)
int len = packet_read_line(in, line, sizeof(line));
if (prefixcmp(line, "unpack "))
return error("did not receive remote status");
- if (strcmp(line, "unpack ok\n")) {
- char *p = line + strlen(line) - 1;
- if (*p == '\n')
- *p = '\0';
+ if (strcmp(line, "unpack ok")) {
error("unpack failed: %s", line + 7);
ret = -1;
}
@@ -131,7 +128,6 @@ static int receive_status(int in, struct ref *refs)
break;
}
- line[strlen(line)-1] = '\0';
refname = line + 3;
msg = strchr(refname, ' ');
if (msg)
diff --git a/sideband.c b/sideband.c
index 8f7b25b..15cc1ae 100644
--- a/sideband.c
+++ b/sideband.c
@@ -38,7 +38,7 @@ int recv_sideband(const char *me, int in_stream, int out)
while (1) {
int band, len;
- len = packet_read_line(in_stream, buf + pf, LARGE_PACKET_MAX);
+ len = packet_read(in_stream, buf + pf, LARGE_PACKET_MAX, 0);
if (len == 0)
break;
if (len < 1) {
diff --git a/upload-pack.c b/upload-pack.c
index c2b2c61..7446cb7 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -50,13 +50,6 @@ static void reset_timeout(void)
alarm(timeout);
}
-static int strip(char *line, int len)
-{
- if (len && line[len-1] == '\n')
- line[--len] = 0;
- return len;
-}
-
static ssize_t send_client_data(int fd, const char *data, ssize_t sz)
{
if (use_sideband)
@@ -447,7 +440,6 @@ static int get_common_commits(void)
got_other = 0;
continue;
}
- strip(line, len);
if (!prefixcmp(line, "have ")) {
switch (got_sha1(line+5, sha1)) {
case -1: /* they have what we do not */
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* [PATCH v3 11/19] pkt-line: provide a generic reading function with options
From: Jeff King @ 2013-02-20 20:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
Originally we had a single function for reading packetized
data: packet_read_line. Commit 46284dd grew a more "gentle"
form, packet_read, that returns an error instead of dying
upon reading a truncated input stream. However, it is not
clear from the names which should be called, or what the
difference is.
Let's instead make packet_read be a generic public interface
that can take option flags, and update the single callsite
that uses it. This is less code, more clear, and paves the
way for introducing more options into the generic interface
later. The function signature is changed, so there should be
no hidden conflicts with topics in flight.
While we're at it, we'll document how error conditions are
handled based on the options, and rename the confusing
"return_line_fail" option to "gentle_on_eof". While we are
cleaning up the names, we can drop the "return_line_fail"
checks in packet_read_internal entirely. They look like
this:
ret = safe_read(..., return_line_fail);
if (return_line_fail && ret < 0)
...
The check for return_line_fail is a no-op; safe_read will
only ever return an error value if return_line_fail was true
in the first place.
Signed-off-by: Jeff King <peff@peff.net>
---
connect.c | 3 ++-
pkt-line.c | 21 ++++++++-------------
pkt-line.h | 27 ++++++++++++++++++++++++++-
3 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/connect.c b/connect.c
index 49e56ba..0aa202f 100644
--- a/connect.c
+++ b/connect.c
@@ -76,7 +76,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
char *name;
int len, name_len;
- len = packet_read(in, buffer, sizeof(buffer));
+ len = packet_read(in, buffer, sizeof(buffer),
+ PACKET_READ_GENTLE_ON_EOF);
if (len < 0)
die_initial_contact(got_at_least_one_head);
diff --git a/pkt-line.c b/pkt-line.c
index 699c2dd..8700cf8 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -103,13 +103,13 @@ static int safe_read(int fd, void *buffer, unsigned size, int return_line_fail)
strbuf_add(buf, buffer, n);
}
-static int safe_read(int fd, void *buffer, unsigned size, int return_line_fail)
+static int safe_read(int fd, void *buffer, unsigned size, int options)
{
ssize_t ret = read_in_full(fd, buffer, size);
if (ret < 0)
die_errno("read error");
else if (ret < size) {
- if (return_line_fail)
+ if (options & PACKET_READ_GENTLE_ON_EOF)
return -1;
die("The remote end hung up unexpectedly");
@@ -143,13 +143,13 @@ static int packet_read_internal(int fd, char *buffer, unsigned size, int return_
return len;
}
-static int packet_read_internal(int fd, char *buffer, unsigned size, int return_line_fail)
+int packet_read(int fd, char *buffer, unsigned size, int options)
{
int len, ret;
char linelen[4];
- ret = safe_read(fd, linelen, 4, return_line_fail);
- if (return_line_fail && ret < 0)
+ ret = safe_read(fd, linelen, 4, options);
+ if (ret < 0)
return ret;
len = packet_length(linelen);
if (len < 0)
@@ -161,22 +161,17 @@ int packet_read_line(int fd, char *buffer, unsigned size)
len -= 4;
if (len >= size)
die("protocol error: bad line length %d", len);
- ret = safe_read(fd, buffer, len, return_line_fail);
- if (return_line_fail && ret < 0)
+ ret = safe_read(fd, buffer, len, options);
+ if (ret < 0)
return ret;
buffer[len] = 0;
packet_trace(buffer, len, 0);
return len;
}
-int packet_read(int fd, char *buffer, unsigned size)
-{
- return packet_read_internal(fd, buffer, size, 1);
-}
-
int packet_read_line(int fd, char *buffer, unsigned size)
{
- return packet_read_internal(fd, buffer, size, 0);
+ return packet_read(fd, buffer, size, 0);
}
int packet_get_line(struct strbuf *out,
diff --git a/pkt-line.h b/pkt-line.h
index 3b6c19c..8cd326c 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -24,8 +24,33 @@ int packet_read_line(int fd, char *buffer, unsigned size);
void packet_buf_flush(struct strbuf *buf);
void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
+/*
+ * Read a packetized line from the descriptor into the buffer, which must be at
+ * least size bytes long. The return value specifies the number of bytes read
+ * into the buffer.
+ *
+ * If options does not contain PACKET_READ_GENTLE_ON_EOF, we will die under any
+ * of the following conditions:
+ *
+ * 1. Read error from descriptor.
+ *
+ * 2. Protocol error from the remote (e.g., bogus length characters).
+ *
+ * 3. Receiving a packet larger than "size" bytes.
+ *
+ * 4. Truncated output from the remote (e.g., we expected a packet but got
+ * EOF, or we got a partial packet followed by EOF).
+ *
+ * If options does contain PACKET_READ_GENTLE_ON_EOF, we will not die on
+ * condition 4 (truncated input), but instead return -1. However, we will still
+ * die for the other 3 conditions.
+ */
+#define PACKET_READ_GENTLE_ON_EOF (1u<<0)
+int packet_read(int fd, char *buffer, unsigned size, int options);
+
+/* Historical convenience wrapper for packet_read that sets no options */
int packet_read_line(int fd, char *buffer, unsigned size);
-int packet_read(int fd, char *buffer, unsigned size);
+
int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
#endif
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* [PATCH v3 10/19] pkt-line: drop safe_write function
From: Jeff King @ 2013-02-20 20:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
This is just write_or_die by another name. The one
distinction is that write_or_die will treat EPIPE specially
by suppressing error messages. That's fine, as we die by
SIGPIPE anyway (and in the off chance that it is disabled,
write_or_die will simulate it).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/receive-pack.c | 2 +-
builtin/send-pack.c | 2 +-
fetch-pack.c | 2 +-
http-backend.c | 8 ++++----
pkt-line.c | 21 ++-------------------
pkt-line.h | 1 -
remote-curl.c | 4 ++--
send-pack.c | 2 +-
sideband.c | 9 +++++----
upload-pack.c | 3 ++-
10 files changed, 19 insertions(+), 35 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 62ba6e7..9129563 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -932,7 +932,7 @@ static void report(struct command *commands, const char *unpack_status)
if (use_sideband)
send_sideband(1, 1, buf.buf, buf.len, use_sideband);
else
- safe_write(1, buf.buf, buf.len);
+ write_or_die(1, buf.buf, buf.len);
strbuf_release(&buf);
}
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 57a46b2..8778519 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -79,7 +79,7 @@ static void print_helper_status(struct ref *ref)
}
strbuf_addch(&buf, '\n');
- safe_write(1, buf.buf, buf.len);
+ write_or_die(1, buf.buf, buf.len);
}
strbuf_release(&buf);
}
diff --git a/fetch-pack.c b/fetch-pack.c
index 27a3e80..b53a18f 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -247,7 +247,7 @@ static void send_request(struct fetch_pack_args *args,
send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);
packet_flush(fd);
} else
- safe_write(fd, buf->buf, buf->len);
+ write_or_die(fd, buf->buf, buf->len);
}
static void insert_one_alternate_ref(const struct ref *ref, void *unused)
diff --git a/http-backend.c b/http-backend.c
index f50e77f..8144f3a 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -70,7 +70,7 @@ static void format_write(int fd, const char *fmt, ...)
if (n >= sizeof(buffer))
die("protocol error: impossibly long line");
- safe_write(fd, buffer, n);
+ write_or_die(fd, buffer, n);
}
static void http_status(unsigned code, const char *msg)
@@ -111,7 +111,7 @@ static void end_headers(void)
static void end_headers(void)
{
- safe_write(1, "\r\n", 2);
+ write_or_die(1, "\r\n", 2);
}
__attribute__((format (printf, 1, 2)))
@@ -157,7 +157,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)
hdr_int(content_length, buf->len);
hdr_str(content_type, type);
end_headers();
- safe_write(1, buf->buf, buf->len);
+ write_or_die(1, buf->buf, buf->len);
}
static void send_local_file(const char *the_type, const char *name)
@@ -185,7 +185,7 @@ static void send_local_file(const char *the_type, const char *name)
die_errno("Cannot read '%s'", p);
if (!n)
break;
- safe_write(1, buf, n);
+ write_or_die(1, buf, n);
}
close(fd);
free(buf);
diff --git a/pkt-line.c b/pkt-line.c
index 5138f47..699c2dd 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -46,23 +46,6 @@ static void packet_trace(const char *buf, unsigned int len, int write)
strbuf_release(&out);
}
-ssize_t safe_write(int fd, const void *buf, ssize_t n)
-{
- ssize_t nn = n;
- while (n) {
- int ret = xwrite(fd, buf, n);
- if (ret > 0) {
- buf = (char *) buf + ret;
- n -= ret;
- continue;
- }
- if (!ret)
- die("write error (disk full?)");
- die_errno("write error");
- }
- return nn;
-}
-
/*
* If we buffered things up above (we don't, but we should),
* we'd flush it here
@@ -70,7 +53,7 @@ void packet_flush(int fd)
void packet_flush(int fd)
{
packet_trace("0000", 4, 1);
- safe_write(fd, "0000", 4);
+ write_or_die(fd, "0000", 4);
}
void packet_buf_flush(struct strbuf *buf)
@@ -106,7 +89,7 @@ void packet_write(int fd, const char *fmt, ...)
va_start(args, fmt);
n = format_packet(fmt, args);
va_end(args);
- safe_write(fd, buffer, n);
+ write_or_die(fd, buffer, n);
}
void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
diff --git a/pkt-line.h b/pkt-line.h
index 7a67e9c..3b6c19c 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -27,6 +27,5 @@ int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
int packet_read_line(int fd, char *buffer, unsigned size);
int packet_read(int fd, char *buffer, unsigned size);
int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
-ssize_t safe_write(int, const void *, ssize_t);
#endif
diff --git a/remote-curl.c b/remote-curl.c
index 933c69a..7be4b53 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -685,7 +685,7 @@ static int fetch_git(struct discovery *heads,
err = rpc_service(&rpc, heads);
if (rpc.result.len)
- safe_write(1, rpc.result.buf, rpc.result.len);
+ write_or_die(1, rpc.result.buf, rpc.result.len);
strbuf_release(&rpc.result);
strbuf_release(&preamble);
free(depth_arg);
@@ -805,7 +805,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
err = rpc_service(&rpc, heads);
if (rpc.result.len)
- safe_write(1, rpc.result.buf, rpc.result.len);
+ write_or_die(1, rpc.result.buf, rpc.result.len);
strbuf_release(&rpc.result);
free(argv);
return err;
diff --git a/send-pack.c b/send-pack.c
index e91cbe2..bde796b 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -280,7 +280,7 @@ int send_pack(struct send_pack_args *args,
send_sideband(out, -1, req_buf.buf, req_buf.len, LARGE_PACKET_MAX);
}
} else {
- safe_write(out, req_buf.buf, req_buf.len);
+ write_or_die(out, req_buf.buf, req_buf.len);
packet_flush(out);
}
strbuf_release(&req_buf);
diff --git a/sideband.c b/sideband.c
index d5ffa1c..8f7b25b 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,3 +1,4 @@
+#include "cache.h"
#include "pkt-line.h"
#include "sideband.h"
@@ -108,7 +109,7 @@ int recv_sideband(const char *me, int in_stream, int out)
} while (len);
continue;
case 1:
- safe_write(out, buf + pf+1, len);
+ write_or_die(out, buf + pf+1, len);
continue;
default:
fprintf(stderr, "%s: protocol error: bad band #%d\n",
@@ -138,12 +139,12 @@ ssize_t send_sideband(int fd, int band, const char *data, ssize_t sz, int packet
if (0 <= band) {
sprintf(hdr, "%04x", n + 5);
hdr[4] = band;
- safe_write(fd, hdr, 5);
+ write_or_die(fd, hdr, 5);
} else {
sprintf(hdr, "%04x", n + 4);
- safe_write(fd, hdr, 4);
+ write_or_die(fd, hdr, 4);
}
- safe_write(fd, p, n);
+ write_or_die(fd, p, n);
p += n;
sz -= n;
}
diff --git a/upload-pack.c b/upload-pack.c
index 63cea91..c2b2c61 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -69,7 +69,8 @@ static ssize_t send_client_data(int fd, const char *data, ssize_t sz)
xwrite(fd, data, sz);
return sz;
}
- return safe_write(fd, data, sz);
+ write_or_die(fd, data, sz);
+ return sz;
}
static FILE *pack_pipe = NULL;
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* [PATCH v3 09/19] pkt-line: move a misplaced comment
From: Jeff King @ 2013-02-20 20:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
The comment describing the packet writing interface was
originally written above packet_write, but migrated to be
above safe_write in f3a3214, probably because it is meant to
generally describe the packet writing interface and not a
single function. Let's move it into the header file, where
users of the interface are more likely to see it.
Signed-off-by: Jeff King <peff@peff.net>
---
pkt-line.c | 15 ---------------
pkt-line.h | 14 +++++++++++++-
2 files changed, 13 insertions(+), 16 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index eaba15f..5138f47 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -46,21 +46,6 @@ static void packet_trace(const char *buf, unsigned int len, int write)
strbuf_release(&out);
}
-/*
- * Write a packetized stream, where each line is preceded by
- * its length (including the header) as a 4-byte hex number.
- * A length of 'zero' means end of stream (and a length of 1-3
- * would be an error).
- *
- * This is all pretty stupid, but we use this packetized line
- * format to make a streaming format possible without ever
- * over-running the read buffers. That way we'll never read
- * into what might be the pack data (which should go to another
- * process entirely).
- *
- * The writing side could use stdio, but since the reading
- * side can't, we stay with pure read/write interfaces.
- */
ssize_t safe_write(int fd, const void *buf, ssize_t n)
{
ssize_t nn = n;
diff --git a/pkt-line.h b/pkt-line.h
index 8cfeb0c..7a67e9c 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -5,7 +5,19 @@
#include "strbuf.h"
/*
- * Silly packetized line writing interface
+ * Write a packetized stream, where each line is preceded by
+ * its length (including the header) as a 4-byte hex number.
+ * A length of 'zero' means end of stream (and a length of 1-3
+ * would be an error).
+ *
+ * This is all pretty stupid, but we use this packetized line
+ * format to make a streaming format possible without ever
+ * over-running the read buffers. That way we'll never read
+ * into what might be the pack data (which should go to another
+ * process entirely).
+ *
+ * The writing side could use stdio, but since the reading
+ * side can't, we stay with pure read/write interfaces.
*/
void packet_flush(int fd);
void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
From: Jeff King @ 2013-02-20 20:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
The write_or_die function will always die on an error,
including EPIPE. However, it currently treats EPIPE
specially by suppressing any error message, and by exiting
with exit code 0.
Suppressing the error message makes some sense; a pipe death
may just be a sign that the other side is not interested in
what we have to say. However, exiting with a successful
error code is not a good idea, as write_or_die is frequently
used in cases where we want to be careful about having
written all of the output, and we may need to signal to our
caller that we have done so (e.g., you would not want a push
whose other end has hung up to report success).
This distinction doesn't typically matter in git, because we
do not ignore SIGPIPE in the first place. Which means that
we will not get EPIPE, but instead will just die when we get
a SIGPIPE. But it's possible for a default handler to be set
by a parent process, or for us to add a callsite inside one
of our few SIGPIPE-ignoring blocks of code.
This patch converts write_or_die to actually raise SIGPIPE
when we see EPIPE, rather than exiting with zero. This
brings the behavior in line with the "normal" case that we
die from SIGPIPE (and any callers who want to check why we
died will see the same thing). We also give the same
treatment to other related functions, including
write_or_whine_pipe and maybe_flush_or_die.
Signed-off-by: Jeff King <peff@peff.net>
---
write_or_die.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/write_or_die.c b/write_or_die.c
index 960f448..b50f99a 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -1,5 +1,15 @@
#include "cache.h"
+static void check_pipe(int err)
+{
+ if (err == EPIPE) {
+ signal(SIGPIPE, SIG_DFL);
+ raise(SIGPIPE);
+ /* Should never happen, but just in case... */
+ exit(141);
+ }
+}
+
/*
* Some cases use stdio, but want to flush after the write
* to get error handling (and to get better interactive
@@ -34,8 +44,7 @@ void maybe_flush_or_die(FILE *f, const char *desc)
return;
}
if (fflush(f)) {
- if (errno == EPIPE)
- exit(0);
+ check_pipe(errno);
die_errno("write failure on '%s'", desc);
}
}
@@ -50,8 +59,7 @@ void write_or_die(int fd, const void *buf, size_t count)
void write_or_die(int fd, const void *buf, size_t count)
{
if (write_in_full(fd, buf, count) < 0) {
- if (errno == EPIPE)
- exit(0);
+ check_pipe(errno);
die_errno("write error");
}
}
@@ -59,8 +67,7 @@ int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)
int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)
{
if (write_in_full(fd, buf, count) < 0) {
- if (errno == EPIPE)
- exit(0);
+ check_pipe(errno);
fprintf(stderr, "%s: write error (%s)\n",
msg, strerror(errno));
return 0;
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
* [PATCH v3 07/19] upload-archive: use argv_array to store client arguments
From: Jeff King @ 2013-02-20 20:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder, Shawn O. Pearce
In-Reply-To: <20130220195147.GA25332@sigill.intra.peff.net>
The current parsing scheme for upload-archive is to pack
arguments into a fixed-size buffer, separated by NULs, and
put a pointer to each argument in the buffer into a
fixed-size argv array.
This works fine, and the limits are high enough that nobody
reasonable is going to hit them, but it makes the code hard
to follow. Instead, let's just stuff the arguments into an
argv_array, which is much simpler. That lifts the "all
arguments must fit inside 4K together" limit.
We could also trivially lift the MAX_ARGS limitation (in
fact, we have to keep extra code to enforce it). But that
would mean a client could force us to allocate an arbitrary
amount of memory simply by sending us "argument" lines. By
limiting the MAX_ARGS, we limit an attacker to about 4
megabytes (64 times a maximum 64K packet buffer). That may
sound like a lot compared to the 4K limit, but it's not a
big deal compared to what git-archive will actually allocate
while working (e.g., to load blobs into memory). The
important thing is that it is bounded.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/upload-archive.c | 35 ++++++++++++++---------------------
1 file changed, 14 insertions(+), 21 deletions(-)
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index c3d134e..3393cef 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -7,6 +7,7 @@
#include "pkt-line.h"
#include "sideband.h"
#include "run-command.h"
+#include "argv-array.h"
static const char upload_archive_usage[] =
"git upload-archive <repo>";
@@ -18,10 +19,9 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
{
- const char *sent_argv[MAX_ARGS];
+ struct argv_array sent_argv = ARGV_ARRAY_INIT;
const char *arg_cmd = "argument ";
- char *p, buf[4096];
- int sent_argc;
+ char buf[4096];
int len;
if (argc != 2)
@@ -31,33 +31,26 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
die("'%s' does not appear to be a git repository", argv[1]);
/* put received options in sent_argv[] */
- sent_argc = 1;
- sent_argv[0] = "git-upload-archive";
- for (p = buf;;) {
+ argv_array_push(&sent_argv, "git-upload-archive");
+ for (;;) {
/* This will die if not enough free space in buf */
- len = packet_read_line(0, p, (buf + sizeof buf) - p);
+ len = packet_read_line(0, buf, sizeof(buf));
if (len == 0)
break; /* got a flush */
- if (sent_argc > MAX_ARGS - 2)
- die("Too many options (>%d)", MAX_ARGS - 2);
+ if (sent_argv.argc > MAX_ARGS)
+ die("Too many options (>%d)", MAX_ARGS - 1);
- if (p[len-1] == '\n') {
- p[--len] = 0;
+ if (buf[len-1] == '\n') {
+ buf[--len] = 0;
}
- if (len < strlen(arg_cmd) ||
- strncmp(arg_cmd, p, strlen(arg_cmd)))
- die("'argument' token or flush expected");
- len -= strlen(arg_cmd);
- memmove(p, p + strlen(arg_cmd), len);
- sent_argv[sent_argc++] = p;
- p += len;
- *p++ = 0;
+ if (prefixcmp(buf, arg_cmd))
+ die("'argument' token or flush expected");
+ argv_array_push(&sent_argv, buf + strlen(arg_cmd));
}
- sent_argv[sent_argc] = NULL;
/* parse all options sent by the client */
- return write_archive(sent_argc, sent_argv, prefix, 0, NULL, 1);
+ return write_archive(sent_argv.argc, sent_argv.argv, prefix, 0, NULL, 1);
}
__attribute__((format (printf, 1, 2)))
--
1.8.2.rc0.9.g352092c
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox