* [PATCHv2 09/10] remote-curl: move ref-parsing code up in file
From: Jeff King @ 2013-02-18 9:29 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130218091203.GB17003@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>
---
Just a cleanup for the next patch. We could also just do extra
declarations at the top.
remote-curl.c | 117 +++++++++++++++++++++++++++++-----------------------------
1 file changed, 59 insertions(+), 58 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index f049da2..62f82d1 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) {
@@ -173,64 +232,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)
{
--
1.8.1.20.g7078b03
^ permalink raw reply related
* [PATCHv2 10/10] remote-curl: always parse incoming refs
From: Jeff King @ 2013-02-18 9:30 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130218091203.GB17003@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>
---
And this does the equivalent of patch 3/3 from the first series, but I
think this is much more robust (certainly it solves the ERR problem, but
more importantly, it uses the exact same function that other code paths
do, so we do not have to worry about it diverging).
remote-curl.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 62f82d1..33af198 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;
@@ -224,6 +226,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);
@@ -232,19 +239,16 @@ static struct ref *get_refs(int for_push)
return last;
}
-
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)
@@ -258,7 +262,6 @@ static void output_refs(struct ref *refs)
}
printf("\n");
fflush(stdout);
- free_refs(refs);
}
struct rpc_state {
@@ -674,7 +677,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
@@ -793,7 +796,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.1.20.g7078b03
^ permalink raw reply related
* Re: [PATCHv2 0/10] pkt-line and remote-curl cleanups server
From: Jeff King @ 2013-02-18 9:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git, Shawn O. Pearce
In-Reply-To: <7vhalaas2b.fsf@alter.siamese.dyndns.org>
On Mon, Feb 18, 2013 at 01:29:16AM -0800, Junio C Hamano wrote:
> > I just checked, and GitHub also does not send flush packets after ERR.
> > Which makes sense; ERR is supposed to end the conversation.
>
> Hmph. A flush packet was supposed to be a mark to say "all the
> packets before this one can be buffered and kept without getting
> passed to write(2), but this and all such buffered data _must_ go on
> the wire _now_". So in the sense, ERR not followed by a flush may
> not even have a chance to be seen on the other end, no? That is
> what the comment before the implementation of packet_flush() is all
> about.
Despite the name, I always thought of packet_flush as more of a signal
for the _receiver_, who then knows that they have gotten all of a
particular list. In other words, we seem to use it as a sequence point
as much as anything (mostly because we immediately write() out any other
packets anyway, so there is no flushing to be done; but perhaps there
were originally thoughts that we could do more buffering on the writing
side).
-Peff
^ permalink raw reply
* Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet
From: Junio C Hamano @ 2013-02-18 9:40 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, git, Shawn O. Pearce
In-Reply-To: <20130218092221.GD5096@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I'm really tempted to bump all of our 1000-byte buffers to just use
> LARGE_PACKET_MAX. If we provided a packet_read variant that used a
> static buffer (which is fine for all but one or two callers), then it
> would not take much memory...
I thought that 1000-byte limit was kept when we introduced the 64k
interface to interoperate with other side that does not yet support
the 64k interface. Is your justification that such an old version of
Git no longer matters in the real world (which is true, I think), or
we use 1000-byte limit in some codepaths even when we know that we
are talking with a 64k-capable version of Git on the other side?
^ permalink raw reply
* Re: [PATCHv2 0/10] pkt-line and remote-curl cleanups server
From: Junio C Hamano @ 2013-02-18 9:49 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, git, Shawn O. Pearce
In-Reply-To: <20130218093335.GK5096@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Feb 18, 2013 at 01:29:16AM -0800, Junio C Hamano wrote:
>
>> > I just checked, and GitHub also does not send flush packets after ERR.
>> > Which makes sense; ERR is supposed to end the conversation.
>>
>> Hmph. A flush packet was supposed to be a mark to say "all the
>> packets before this one can be buffered and kept without getting
>> passed to write(2), but this and all such buffered data _must_ go on
>> the wire _now_". So in the sense, ERR not followed by a flush may
>> not even have a chance to be seen on the other end, no? That is
>> what the comment before the implementation of packet_flush() is all
>> about.
>
> Despite the name, I always thought of packet_flush as more of a signal
> for the _receiver_, who then knows that they have gotten all of a
> particular list. In other words, we seem to use it as a sequence point
> as much as anything (mostly because we immediately write() out any other
> packets anyway, so there is no flushing to be done; but perhaps there
> were originally thoughts that we could do more buffering on the writing
> side).
Exactly.
The logic behind the comment "we do not buffer, but we should" is
that flush tells the receiver that the sending end is "done" feeding
it a class of data, and the data before flush do not have to reach
the receiver immediately, hence we can afford to buffer on the
sending end if we can measure that doing so would give us better
performance. We haven't measure and turned buffering on yet.
But when dying, we may of course have data before flushing. We may
disconnect (by dying) without showing flush (or preceding ERR) to
the other side, and for that reason, not relying on the flush packet
on the receiving end is a good change. Once we turn buffering on, we
probably need to be careful when sending an ERR indicator by making
it always drain any buffered data and show the ERR indicator without
buffering, or something.
^ permalink raw reply
* Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet
From: Jeff King @ 2013-02-18 9:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git, Shawn O. Pearce
In-Reply-To: <7vd2vyarjy.fsf@alter.siamese.dyndns.org>
On Mon, Feb 18, 2013 at 01:40:17AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I'm really tempted to bump all of our 1000-byte buffers to just use
> > LARGE_PACKET_MAX. If we provided a packet_read variant that used a
> > static buffer (which is fine for all but one or two callers), then it
> > would not take much memory...
>
> I thought that 1000-byte limit was kept when we introduced the 64k
> interface to interoperate with other side that does not yet support
> the 64k interface. Is your justification that such an old version of
> Git no longer matters in the real world (which is true, I think), or
> we use 1000-byte limit in some codepaths even when we know that we
> are talking with a 64k-capable version of Git on the other side?
I should have been more clear that I want to bump only the _reading_
side, not the writing.
The sideband-64k capability bumps the sideband chunk size. But the size
for packetized ref advertisement lines has remained at 1000. Which it
must, since we start outputting them before doing capability
negotiation. The sender will die before writing a longer ref line (see
pkg-line.c:format_packet), and most of the reading callsites feed a
1000-byte buffer to packet_read_line (which will die if we get a larger
packet). So the upgrade path for that would be:
1. Git bumps up all reading buffers to LARGE_PACKET_MAX, just in case.
This immediately covers us for any alternate implementations that
send larger ref packets (I do not know if any exist, but the
documentation does not mention this limitation at all, so I would
not be surprised if other implementations just use LARGE_PACKET_MAX
as a maximum).
2. Many years pass. We decide that Git v1.8.2 and older are ancient
history and not worth caring about.
3. We bump the 1000-byte limit for format_packet to LARGE_PACKET_MAX.
This is not pressing at all; I wouldn't have even noticed it if I hadn't
been wondering how large to make the new buffer I was adding in a later
patch. I have not heard of anyone running up against this limit in
practice. But it's easy to do (1), and it starts the clock ticking for
the 1000-byte readers to become obsolete.
-Peff
^ permalink raw reply
* Re: [PATCHv2 0/10] pkt-line and remote-curl cleanups server
From: Jeff King @ 2013-02-18 9:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git, Shawn O. Pearce
In-Reply-To: <7v8v6mar4e.fsf@alter.siamese.dyndns.org>
On Mon, Feb 18, 2013 at 01:49:37AM -0800, Junio C Hamano wrote:
> The logic behind the comment "we do not buffer, but we should" is
> that flush tells the receiver that the sending end is "done" feeding
> it a class of data, and the data before flush do not have to reach
> the receiver immediately, hence we can afford to buffer on the
> sending end if we can measure that doing so would give us better
> performance. We haven't measure and turned buffering on yet.
>
> But when dying, we may of course have data before flushing. We may
> disconnect (by dying) without showing flush (or preceding ERR) to
> the other side, and for that reason, not relying on the flush packet
> on the receiving end is a good change. Once we turn buffering on, we
> probably need to be careful when sending an ERR indicator by making
> it always drain any buffered data and show the ERR indicator without
> buffering, or something.
Yeah, I'd agree. An ERR packet should always be accompanied by a flush
(whether an actual flush packet or not doesn't really matter, but
definitely a buffer flush on the sender). But I think that is really the
sender's problem, and we can deal with it there if and when we buffer.
>From the receiver's end, they simply want to be liberal in interpreting
an ERR as the end of the data stream. They do not have to be picky about
whether it is followed by more data, or by a flush packet, or whatever.
But that is not a change I am introducing; that is how get_remote_heads
has always worked. I am merely correcting the proposed change from v1 of
the series that did not correctly take that into account (and therefore
regressed the error message in the ERR case).
-Peff
^ permalink raw reply
* Re: [PATCHv2 02/10] pkt-line: drop safe_write function
From: Jonathan Nieder @ 2013-02-18 9:56 UTC (permalink / raw)
To: Jeff King; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130218091519.GB5096@sigill.intra.peff.net>
Jeff King wrote:
> This is just write_or_die by another name.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Actually, they are not quite the same. write_or_die will exit(0) when it
> sees EPIPE.
That information definitely belongs in the commit message.
> Which makes me a little nervous.
Me, too. Let's see:
[...]
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -908,7 +908,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);
If the connection to send-pack is lost and stdout becomes a broken
pipe and I am updating enough refs to overflow the pipe buffer,
receive-pack will die with SIGPIPE. So unless the sadistic caller has
set the inherited SIGPIPE action to SIG_IGN (for example by wrapping
git with an uncautious Python wrapper that uses subprocess.Popen), the
change to EPIPE handling is not a behavior change.
Since the pipe is closed, presumably the calling send-pack has hung up
and won't notice the exit status, so this should be safe.
Arguably it would be more friendly to stay alive to run the
post-receive and post-update hooks, though, given that a ref update
has occurred. Maybe transport commands like this one should always
set the disposition of SIGPIPE to SIG_IGN.
[...]
> --- 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);
A signal will kill send-pack before write_or_die has a chance to
intervene so this change is a no-op unless the caller is sadistic
(as in the [1] case). In the signal(SIGPIPE, SIG_IGN) case, it might
be a regression, since "git push" should not declare success when its
connection to receive-pack closes early.
[1] http://www.chiark.greenend.org.uk/ucgi/~cjwatson/blosxom/2009-07-02-python-sigpipe.html
[...]
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -245,7 +245,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);
Also a no-op except when the parent process is insane enough to let us
inherit signal(SIGPIPE, SIG_IGN).
In that case, if triggerable this looks like a bad change: if
upload-pack has gone missing, the fetch should not be considered a
success.
[...]
> --- 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);
Etc. I'm stopping here.
I'm thinking before a patch like this we should make the following
change:
1. at startup, set the signal action of SIGPIPE to SIG_DFL, to make
the behavior a little more predictable.
Perhaps the following as well:
2. in write_or_die(), when encountering EPIPE, set the signal action
of SIGPIPE to SIG_DFL and raise(SIGPIPE), ensuring the exit status
reflects the broken pipe. If the parent process is unnecessarily
noisy about that, that's a bug in the parent process (hopefully
uncommon).
Or alternatively:
2b. never set SIGPIPE to SIG_IGN except in short blocks of code that
do not call write_or_die()
What do you think?
Jonathan
^ permalink raw reply
* Re: git clone tag shallow
From: Thibault Kruse @ 2013-02-18 10:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, git
In-Reply-To: <7vliamascv.fsf@alter.siamese.dyndns.org>
Hi Junio,
On Mon, Feb 18, 2013 at 10:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Thibault Kruse <tibokruse@googlemail.com> writes:
>
>> Whenever a command description involves "<branch>" this can, depending
>> on the command, refer to
>> 1) a name that, when prepended with "refs/heads/", is a valid ref,
>> 2) a name that, when prepended with "refs/heads/" or "refs/tags", is a
>> valid ref,
>> 3) a name that, when prepended with "refs/[heads|tags]/", or unique in
>> "refs/remotes/*/" is a valid ref
>>
>> Now in the docu I don't see a nice distinction between 1), 2) and 3).
>> I could work on a patch if someone
>> tells me how to clearly distinguish those cases.
>
> It is _very_ true that we do not give strict distinction in many
> cases in the SYNOPSIS section.
>
> It is clear that (1) should use <branch> or even <branch-name>.
> "git checkout master" and "git checkout head/master" mean very
> different things. The former is the "git checkout <branch-name>"
> case---checkout the named branch and prepare to grow the history of
> that branch. The latter is "git checkout <committish>"---detach the
> HEAD at that commit, and even when the committish was named using
> the name of an existing branch (e.g. "master^0" or "heads/master"),
> prevent future commits made in that state from affecting the branch.
>
> I am not sure why you meant to treat (2) and (3) differently,
> though. Care to elaborate?
As in my example, git clone --branch <branch> does not accept all of (3).
I now see that indeed the options section for git clone --branch has
been changed to inlude the information that tags are also allowed, so that's
in order.
> Outside "git checkout", we historically deliberately stayed loose in
> an attempt to help beginners by avoiding <committish> or <ref>, when
> most people are expected to feed branch names to the command and
> used <branch>. I am not sure if it is a good idea to break such a
> white lie just to be technically more correct in the first place.
That's fair enough, I guess, I am not sure either. If I understand you
right, the Synopsis and
description are supposed to explain the non-hackish usage of commands,
whereas documentation after the OPTIONS headline is supposed to be
more of a complete description. Hence e.g. the synopsis of git-checkout
does not mention the --t,--track,--no-track options, and takes a liberal
approach to option syntaxes (listing '[-p|--patch]', but only '-m',
but not '[-m|--merge]'),
similar git-clone help does not mention the '--branch' option in the synopsis
for that reason, I guess. Do I get this right?
Does this also extend to the (bash) tab completion?
E.g. hitting tab after "git clone --", offers me (Ubuntu precise, git 1.7.9.5):
--bare --local --no-checkout --origin
--reference --template=
--depth --mirror --no-hardlinks --quiet
--shared --upload-pack
missing:
---recursive --recurse-submodules (--[no-]single-branch)
--separate-git-dir --verbose --progress --branch
Is this also intentional?
cheers,
Thibault
^ permalink raw reply
* Re: [PATCHv2 03/10] pkt-line: clean up "gentle" reading function
From: Jonathan Nieder @ 2013-02-18 10:12 UTC (permalink / raw)
To: Jeff King; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130218091619.GC5096@sigill.intra.peff.net>
Jeff King wrote:
> Originally we had a single function for reading packetized
> data: packet_read_line. Commit 46284dd grew a more "gentle"
> form that would return an error instead of dying upon
> reading a truncated input stream. However:
In other words:
Based on the names of two functions "packet_read" and
"packet_read_line", it is not obvious which to use and what the
ramifications of that choice are.
Rename packet_read to packet_read_line_gently and add a comment
explaining that the latter is a "gentler" form that returns an
error instead of dying upon reading a truncated input stream.
While at it:
* Rename the internal argument triggering the gentle mode to
"gentle" instead of "return_line_fail".
* Drop the redundant "return_line_fail &&" in checks like
"if (return_line_fail && ret < 0)". safe_read() never
returns an error when !gentle.
No functional change intended.
FWIW, the patch itself is
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet
From: Jonathan Nieder @ 2013-02-18 10:15 UTC (permalink / raw)
To: Jeff King; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130218092221.GD5096@sigill.intra.peff.net>
Jeff King wrote:
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -160,7 +160,8 @@ static int packet_read_internal(int fd, char *buffer, unsigned size, int gently)
> }
> len -= 4;
> if (len >= size)
> - die("protocol error: bad line length %d", len);
> + die("protocol error: line too large: (expected %u, got %d)",
> + size, len);
Makes sense. I think this should say "expected < %u, got %d", since we
don't actually expect most lines to be 1004 bytes in practice.
With or without such a change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCHv2 05/10] pkt-line: rename s/packet_read_line/packet_read/
From: Jonathan Nieder @ 2013-02-18 10:19 UTC (permalink / raw)
To: Jeff King; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130218092252.GE5096@sigill.intra.peff.net>
Jeff King wrote:
> Originally packets were used just for the line-oriented ref
> advertisement and negotiation. These days, we also stuff
> packfiles and sidebands into them, and they do not
> necessarily represent a line. Drop the "_line" suffix, as it
> is not informative and makes the function names quite long
> (especially as we add "_gently" and other variants).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Again, this is a taste issue. Can be optional.
In combination with patch 3, this changes the meaning of packet_read()
without changing its signature, which could make other patches
cherry-picked on top change behavior in unpredictable ways. :(
So I'd be all for this if the signature changes (for example to put
the fd at the end or something), but not so if not.
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCHv2 02/10] pkt-line: drop safe_write function
From: Jeff King @ 2013-02-18 10:24 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130218095644.GA7049@elie.Belkin>
On Mon, Feb 18, 2013 at 01:56:45AM -0800, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > This is just write_or_die by another name.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > Actually, they are not quite the same. write_or_die will exit(0) when it
> > sees EPIPE.
>
> That information definitely belongs in the commit message.
Yeah, this one was more RFC; I was hoping for some input on the EPIPE
thing, so I could know _what_ to put in the commit message. Is it safe,
and if so, why? And if not, we should drop the patch.
> If the connection to send-pack is lost and stdout becomes a broken
> pipe and I am updating enough refs to overflow the pipe buffer,
> receive-pack will die with SIGPIPE. So unless the sadistic caller has
> set the inherited SIGPIPE action to SIG_IGN (for example by wrapping
> git with an uncautious Python wrapper that uses subprocess.Popen), the
> change to EPIPE handling is not a behavior change.
Yeah, but I don't want to count on always catching SIGPIPE. There's the
inherited signal handler thing, but there's also the fact that we may
end up ignoring SIGPIPE from backend programs like upload-pack and
receive-pack; they check their writes anyway, and we have already run
into issues with getting SIGPIPE when we don't necessarily expect or
care about it.
The nice thing about write_or_die is that it still _exits_ on EPIPE.
It's just that it doesn't print an error (which is really not a big
deal) and exit with a 0 return code. I really wonder if we should just
change the latter. For programs which are creating copious output (e.g.,
"git log"), the return value is not important anyway. For backend
programs, an unexpected EPIPE from something like write_or_die should
probably involve a non-successful return code.
> Arguably it would be more friendly to stay alive to run the
> post-receive and post-update hooks, though, given that a ref update
> has occurred. Maybe transport commands like this one should always
> set the disposition of SIGPIPE to SIG_IGN.
Yeah, I've suggested that in the past. And I do think it's sane, because
if you took a ref update, you almost certainly want to run the
post-receive, even if the client is no longer around (e.g., if it is
going to email out the changeset).
> [...]
> > --- 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);
>
> A signal will kill send-pack before write_or_die has a chance to
> intervene so this change is a no-op unless the caller is sadistic
> (as in the [1] case). In the signal(SIGPIPE, SIG_IGN) case, it might
> be a regression, since "git push" should not declare success when its
> connection to receive-pack closes early.
But that isn't going to receive-pack, is it? Send-pack's stdout is
really just going to the user (or wherever). So it would have an effect
more for:
(git push && echo >&2 OK) | grep -m1 foo
which might print "OK" even if we failed. That's quite contrived, but it
is at least a measurable change. And anyway...
> In that case, if triggerable this looks like a bad change: if
> upload-pack has gone missing, the fetch should not be considered a
> success.
> [...]
> Etc. I'm stopping here.
Yeah, there are definitely some bad ones.
> I'm thinking before a patch like this we should make the following
> change:
>
> 1. at startup, set the signal action of SIGPIPE to SIG_DFL, to make
> the behavior a little more predictable.
I'm lukewarm on that, just because we may want to ignore SIGPIPE
ourselves at some point.
> Perhaps the following as well:
>
> 2. in write_or_die(), when encountering EPIPE, set the signal action
> of SIGPIPE to SIG_DFL and raise(SIGPIPE), ensuring the exit status
> reflects the broken pipe. If the parent process is unnecessarily
> noisy about that, that's a bug in the parent process (hopefully
> uncommon).
I like this. My suggestion would be to just exit(1) instead of exit(0).
But really, raising SIGPIPE makes the most sense, because it
communicates to the parent what happened (and the shell will wisely not
print a message, but careful parents like "git fetch" and "git push"
will check it properly and notice that the child did not succeed).
> Or alternatively:
>
> 2b. never set SIGPIPE to SIG_IGN except in short blocks of code that
> do not call write_or_die()
Yuck. :)
> What do you think?
I really like option 2. That exit(0) when we see SIGPIPE bugs me.
Because we _are_ dying due to our write failing, and I think the only
reason to exit(0) was to avoid unnecessary complaints from parents. But
raising SIGPIPE seems like the best of both worlds to me.
-Peff
^ permalink raw reply
* Re: [PATCHv2 03/10] pkt-line: clean up "gentle" reading function
From: Jeff King @ 2013-02-18 10:25 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130218101209.GC7049@elie.Belkin>
On Mon, Feb 18, 2013 at 02:12:09AM -0800, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > Originally we had a single function for reading packetized
> > data: packet_read_line. Commit 46284dd grew a more "gentle"
> > form that would return an error instead of dying upon
> > reading a truncated input stream. However:
>
> In other words:
Hmph. I had originally written a commit message organized more like
yours, then I changed it to try to be more clear. I guess that didn't
work.
But yes, you get the intent exactly.
-Peff
^ permalink raw reply
* Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet
From: Jeff King @ 2013-02-18 10:26 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130218101523.GD7049@elie.Belkin>
On Mon, Feb 18, 2013 at 02:15:23AM -0800, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > --- a/pkt-line.c
> > +++ b/pkt-line.c
> > @@ -160,7 +160,8 @@ static int packet_read_internal(int fd, char *buffer, unsigned size, int gently)
> > }
> > len -= 4;
> > if (len >= size)
> > - die("protocol error: bad line length %d", len);
> > + die("protocol error: line too large: (expected %u, got %d)",
> > + size, len);
>
> Makes sense. I think this should say "expected < %u, got %d", since we
> don't actually expect most lines to be 1004 bytes in practice.
Yeah, I had toyed with writing "expected max %u" for the same reason.
I'll tweak it in the re-roll.
-Peff
^ permalink raw reply
* Re: [PATCHv2 05/10] pkt-line: rename s/packet_read_line/packet_read/
From: Jeff King @ 2013-02-18 10:29 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130218101915.GE7049@elie.Belkin>
On Mon, Feb 18, 2013 at 02:19:15AM -0800, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > Originally packets were used just for the line-oriented ref
> > advertisement and negotiation. These days, we also stuff
> > packfiles and sidebands into them, and they do not
> > necessarily represent a line. Drop the "_line" suffix, as it
> > is not informative and makes the function names quite long
> > (especially as we add "_gently" and other variants).
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > Again, this is a taste issue. Can be optional.
>
> In combination with patch 3, this changes the meaning of packet_read()
> without changing its signature, which could make other patches
> cherry-picked on top change behavior in unpredictable ways. :(
>
> So I'd be all for this if the signature changes (for example to put
> the fd at the end or something), but not so if not.
True. Though packet_read has only existed since last June, only had one
callsite (which would now conflict, since I'm touching it in this
series), and has no new calls in origin..origin/pu. So it's relatively
low risk for such a problem. I don't know how careful we want to be.
-Peff
^ permalink raw reply
* Re: [PATCHv2 06/10] pkt-line: share buffer/descriptor reading implementation
From: Jonathan Nieder @ 2013-02-18 10:43 UTC (permalink / raw)
To: Jeff King; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130218092612.GF5096@sigill.intra.peff.net>
Jeff King wrote:
> The packet_read function reads from a descriptor.
Ah, so this introduces a new analagous helper that reads from
a strbuf, to avoid the copy-from-async-procedure hack?
[...]
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -103,12 +103,26 @@ static int safe_read(int fd, void *buffer, unsigned size, int gently)
> strbuf_add(buf, buffer, n);
> }
>
> -static int safe_read(int fd, void *buffer, unsigned size, int gently)
> +static int get_packet_data(int fd, char **src_buf, size_t *src_size,
> + void *dst, unsigned size, int gently)
> {
> - ssize_t ret = read_in_full(fd, buffer, size);
> - if (ret < 0)
> - die_errno("read error");
> - else if (ret < size) {
> + ssize_t ret;
> +
> + /* Read up to "size" bytes from our source, whatever it is. */
> + if (src_buf) {
> + ret = size < *src_size ? size : *src_size;
> + memcpy(dst, *src_buf, ret);
> + *src_buf += size;
> + *src_size -= size;
> + }
> + else {
Style: git cuddles its "else"s.
assert(src_buf ? fd < 0 : fd >= 0);
if (src_buf) {
...
} else {
...
}
> + ret = read_in_full(fd, dst, size);
> + if (ret < 0)
> + die_errno("read error");
This is noisy about upstream pipe gone missing, which makes sense
since this is transport-related. Maybe that deserves a comment.
[...]
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -138,28 +138,29 @@ static struct discovery* discover_refs(const char *service)
> if (maybe_smart &&
> (5 <= last->len && last->buf[4] == '#') &&
> !strbuf_cmp(&exp, &type)) {
> + char line[1000];
> + int len;
> +
> /*
> * 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);
> + len = packet_read_from_buf(line, sizeof(line), &last->buf, &last->len);
> + if (len && line[len - 1] == '\n')
> + len--;
Was anything guaranteeing that buffer.len < 1000 before this change?
The rest looks good from a quick glance.
Jonathan
^ permalink raw reply
* Re: [PATCHv2 08/10] remote-curl: pass buffer straight to get_remote_heads
From: Jonathan Nieder @ 2013-02-18 10:47 UTC (permalink / raw)
To: Jeff King; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130218092905.GH5096@sigill.intra.peff.net>
Jeff King wrote:
> I don't know that this code was hurting anything, but it has always
> struck me as ugly and a possible source of error. And now it's gone.
Heh. Belongs in the commit message, presumably.
I don't think the async procedure was very harmful, but it's nice to
avoid the cost of a new thread and some copying.
> remote-curl.c | 26 ++------------------------
> 1 file changed, 2 insertions(+), 24 deletions(-)
Nice. The patch looks sane.
^ permalink raw reply
* Re: [PATCHv2 06/10] pkt-line: share buffer/descriptor reading implementation
From: Jeff King @ 2013-02-18 10:48 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130218104350.GF7049@elie.Belkin>
On Mon, Feb 18, 2013 at 02:43:50AM -0800, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > The packet_read function reads from a descriptor.
>
> Ah, so this introduces a new analagous helper that reads from
> a strbuf, to avoid the copy-from-async-procedure hack?
Not from a strbuf, but basically, yes.
> > + ret = read_in_full(fd, dst, size);
> > + if (ret < 0)
> > + die_errno("read error");
>
> This is noisy about upstream pipe gone missing, which makes sense
> since this is transport-related. Maybe that deserves a comment.
That is not new code; it is just reindented from the original safe_read.
But is it noisy about a missing pipe? We do not get EPIPE for reading.
We should just get a short read or EOF, both of which is handled later.
> > + len = packet_read_from_buf(line, sizeof(line), &last->buf, &last->len);
> > + if (len && line[len - 1] == '\n')
> > + len--;
>
> Was anything guaranteeing that buffer.len < 1000 before this change?
No. That's discussed in point (3) of the "implications" in the commit
message.
-Peff
^ permalink raw reply
* Re: [PATCHv2 10/10] remote-curl: always parse incoming refs
From: Jonathan Nieder @ 2013-02-18 10:50 UTC (permalink / raw)
To: Jeff King; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130218093056.GJ5096@sigill.intra.peff.net>
Jeff King wrote:
> remote-curl.c | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
I like.
^ permalink raw reply
* Re: [PATCHv2 06/10] pkt-line: share buffer/descriptor reading implementation
From: Jonathan Nieder @ 2013-02-18 10:54 UTC (permalink / raw)
To: Jeff King; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130218104804.GB16408@sigill.intra.peff.net>
Jeff King wrote:
> But is it noisy about a missing pipe? We do not get EPIPE for reading.
Ah, that makes more sense.
[...]
>>> + len = packet_read_from_buf(line, sizeof(line), &last->buf, &last->len);
>>> + if (len && line[len - 1] == '\n')
>>> + len--;
>>
>> Was anything guaranteeing that buffer.len < 1000 before this change?
>
> No. That's discussed in point (3) of the "implications" in the commit
> message.
Thanks. Sorry I missed it the first time.
^ permalink raw reply
* Re: [PATCHv2 05/10] pkt-line: rename s/packet_read_line/packet_read/
From: Jonathan Nieder @ 2013-02-18 11:05 UTC (permalink / raw)
To: Jeff King; +Cc: git, Shawn O. Pearce
In-Reply-To: <20130218102931.GP5096@sigill.intra.peff.net>
Jeff King wrote:
> On Mon, Feb 18, 2013 at 02:19:15AM -0800, Jonathan Nieder wrote:
>> In combination with patch 3, this changes the meaning of packet_read()
>> without changing its signature, which could make other patches
>> cherry-picked on top change behavior in unpredictable ways. :(
>>
>> So I'd be all for this if the signature changes (for example to put
>> the fd at the end or something), but not so if not.
>
> True. Though packet_read has only existed since last June, only had one
> callsite (which would now conflict, since I'm touching it in this
> series), and has no new calls in origin..origin/pu. So it's relatively
> low risk for such a problem. I don't know how careful we want to be.
I was unclear. What I am worried about is that someone using a
version of git without this patch will try some yet-to-be-written
patch using packet_read from the mailing list and not notice that they
are using the wrong function. For example, if someone is using
1.7.12.y or 1.8.1.y and wants to try a patch from after the above,
they would get subtly different and wrong results.
The rule "change the name or signature when breaking the ABI of a
global function" is easy to remember and follow. I think we want not
to have to be careful at all, and such rules can help with that. :)
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH v3 6/9] user-manual: Use 'git config --global user.*' for setup
From: W. Trevor King @ 2013-02-18 12:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git, Jonathan Nieder
In-Reply-To: <7vobficp8y.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 357 bytes --]
On Sun, Feb 17, 2013 at 06:47:09PM -0800, Junio C Hamano wrote:
> > +Which will adds the following to a file named `.gitconfig` in your
>
> s/adds/add/;
Oops again :p. This change is SOB me.
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v3 9/9] user-manual: Use -o latest.tar.gz to create a gzipped tarball
From: W. Trevor King @ 2013-02-18 12:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git, Jonathan Nieder
In-Reply-To: <7vbobicop9.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 572 bytes --]
On Sun, Feb 17, 2013 at 06:58:58PM -0800, Junio C Hamano wrote:
> It is easy for me to deal with conflicts in this particular case,
> but in general it would have been more straightforward to manage if
> these more localized phrasing fixes came earlier and a larger
> file-wide cosmetic change was done after all the others have
> settled.
Ok. I'll shift the backtick fix to the back of the stack for v4.
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v3 0/9] User manual updates
From: W. Trevor King @ 2013-02-18 12:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git, Jonathan Nieder
In-Reply-To: <7vr4keatlk.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 506 bytes --]
On Mon, Feb 18, 2013 at 12:56:07AM -0800, Junio C Hamano wrote:
> I've taken the following to 'maint'…
Should I rebase v4 onto maint so I don't accidentally collide with any
of the previous patches which have already been merged there? It
doesn't look like you've pushed the last round of maint additions to
your public repository yet…
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
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