Re: [PATCH 2/4] do not send client agent unless server does first

[Jeff King <peff@peff.net>]

On Fri, Aug 10, 2012 at 12:45:19PM -0700, Junio C Hamano wrote:

> > diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> > index fe56596..bc7a0f9 100644
> > --- a/builtin/fetch-pack.c
> > +++ b/builtin/fetch-pack.c
> > @@ -19,6 +19,7 @@ static int prefer_ofs_delta = 1;
> >  static int no_done;
> >  static int fetch_fsck_objects = -1;
> >  static int transfer_fsck_objects = -1;
> > +static int agent_supported;
> >  static struct fetch_pack_args args = {
> >  	/* .uploadpack = */ "git-upload-pack",
> >  };
> 
> This is only set to false once per process invocation.  We do not
> currently talk with more than one remote from the same process in
> the "fetch" codepath, and we must maintain that. This fix will be
> broken otherwise ("recursive submodule fetch" comes to mind; one
> more reason it should do its submodule business in a separate
> process).

Right; I followed the existing options in both fetch-pack and send-pack
(use_sideband, no_done, and others here have the same issue), with the
assumption that the current code was not broken (which may not be true,
if it is simply masked by the fact that in practice everybody happens to
support those older features).

I don't know off-hand whether that is actually a trigger-able bug in the
current code or not. It's probably a good topic for a follow-on series.

> > @@ -328,7 +329,8 @@ static int find_common(int fd[2], unsigned char *result_sha1,
> >  			if (args.no_progress)   strbuf_addstr(&c, " no-progress");
> >  			if (args.include_tag)   strbuf_addstr(&c, " include-tag");
> 
> This codepath still forgets to check if the other side advertised
> "thin-pack", "no-progress", and "include-tag", no?

Yes. I didn't realize it until Dave mentioned the "innocuous" list. I
think cleaning them up is reasonable, but probably a separate topic.

> > diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> > index 5c69995..7d05064 100644
> > --- a/builtin/send-pack.c
> > +++ b/builtin/send-pack.c
> > @@ -252,6 +252,7 @@ int send_pack(struct send_pack_args *args,
> >  	int status_report = 0;
> >  	int use_sideband = 0;
> >  	int quiet_supported = 0;
> > +	int agent_supported = 0;
> >  	unsigned cmds_sent = 0;
> >  	int ret;
> >  	struct async demux;
> 
> This is initialied to 0 per communication, so having multiple
> remote.$there.pushURL configuration variables will work correctly.

Right. Not through me having thought about it, though, but by following
the existing convention.

-Peff