On Sun, Feb 17, 2013 at 05:41:13PM -0800, Jonathan Nieder wrote:

> > I don't think so. Don't ERR lines appear inside their own packets?
> Yes, I misread get_remote_heads for some reason.  Thanks for checking.

Thanks for bringing it up. I had not even thought about ERR at all. So
it was luck rather than skill that I was right. :)

> I'm not sure whether servers are expected to send a flush after an
> ERR packet.  The only codepath I know of in git itself that sends
> such packets is git-daemon, which does not flush after the error (but
> is not used in the stateless-rpc case).  http-backend uses HTTP error
> codes for its errors.

I just checked, and GitHub also does not send flush packets after ERR.
Which makes sense; ERR is supposed to end the conversation. I can change
GitHub, of course, but who knows what other implementations exist (e.g.,
I do not know off-hand whether gitolite has custom ERR responses). So it
seems pretty clear that just checking for a flush packet is not the
right thing, and we need to actually parse the packet contents (at least
to some degree).

> If I am reading get_remote_heads correctly, calling it with the
> following tweak should work ok.  The extra thread is just to feed a
> string into a fd-based interface and could be avoided for "list", too,
> if it costs too much.

Yeah, your patch does work, though we miss out on some of the refname
checks.  I think what I'd rather do is just teach get_remote_heads to
read from a buffer (to avoid the extra thread and pipe), and then just
run (and cache) the ref parsing unconditionally once we've read from the
server.  It shouldn't make a difference in the normal case, as we would
usually do a "list" anyway (and by caching, "list" can just feed out the
cached copy).

While looking into this, I noticed a bunch of other possible cleanups.
Patches to follow:

  [01/10]: pkt-line: move a misplaced comment
  [02/10]: pkt-line: drop safe_write function
  [03/10]: pkt-line: clean up "gentle" reading function
  [04/10]: pkt-line: change error message for oversized packet
  [05/10]: pkt-line: rename s/packet_read_line/packet_read/

These are all just cleanups I noticed while looking at pkt-line. Any of
them can be dropped, though there would be some textual conflicts for
the later patches.

  [06/10]: pkt-line: share buffer/descriptor reading implementation
  [07/10]: teach get_remote_heads to read from a memory buffer
  [08/10]: remote-curl: pass buffer straight to get_remote_heads

These all build on each other to get rid of the extra thread/pipe, which
I think is worth doing even without the rest of the series.

  [09/10]: remote-curl: move ref-parsing code up in file
  [10/10]: remote-curl: always parse incoming refs

And these ones actually fix the problem I noticed.

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to