Re: [PATCH] clone: disconnect transport after fetching

From: Daniel Barkalow <barkalow@iabervon.org>
Date: Sep 2, 2009, 16:38 UTC
Message-ID: <alpine.LNX.2.00.0909021228570.28290@iabervon.org>
In-Reply-To: <20090902063647.GA29559@coredump.intra.peff.net>

On Wed, 2 Sep 2009, Jeff King wrote:

Show 35 quoted lines

> The current code just leaves the transport in whatever state
> it was in after performing the fetch.  For a non-empty clone
> over the git protocol, the transport code already
> disconnects at the end of the fetch.
> 
> But for an empty clone, we leave the connection hanging, and
> eventually close the socket when clone exits. This causes
> the remote upload-pack to complain "the remote end hung up
> unexpectedly". While this message is harmless to the clone
> itself, it is unnecessarily scary for a user to see and may
> pollute git-daemon logs.
> 
> This patch just explicitly calls disconnect after we are
> done with the remote end, which sends a flush packet to
> upload-pack and cleanly disconnects, avoiding the error
> message.
> 
> Other transports are unaffected or slightly improved:
> 
>  - for a non-empty repo over the git protocol, the second
>    disconnect is a no-op (since we are no longer connected)
> 
>  - for "walker" transports (like HTTP or FTP), we actually
>    free some used memory (which previously just sat until
>    the clone process exits)
> 
>  - for "rsync", disconnect is always a no-op anyway
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This was suggested by Daniel, so theoretically
> 
>   Acked-by: Daniel Barkalow <barkalow@iabervon.org>
> 
> :)

This is what I intended, so:

Acked-by: Daniel Barkalow <barkalow@iabervon.org>

> As you can see from the commit message, I did a little extra hunting to
> make sure we are not going to impact any other code paths, and I am
> pretty sure we are fine.

Also, builtin-fetch already does the explicit disconnect, and commonly exercises both the "we want something" and "we don't want anything" cases, so any problems would have to be surprisingly clone-specific.

Show 22 quoted lines

>  builtin-clone.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/builtin-clone.c b/builtin-clone.c
> index 991a7ae..0f231d8 100644
> --- a/builtin-clone.c
> +++ b/builtin-clone.c
> @@ -580,8 +580,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		option_no_checkout = 1;
>  	}
>  
> -	if (transport)
> +	if (transport) {
>  		transport_unlock_pack(transport);
> +		transport_disconnect(transport);
> +	}
>  
>  	if (!option_no_checkout) {
>  		struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
> -- 
> 1.6.4.2.401.ga275f.dirty
>