git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 3/8] connect: split git:// setup into a separate function

From
BWBrandon Williams <bmwill@google.com>
Date
Nov 20, 2017, 21:52 UTC
Message-ID
<20171120215218.GB92506@google.com>
In-Reply-To
<20171120212327.ssk6vmw2hd5jwbi5@aiede.mtv.corp.google.com>
On 11/20, Jonathan Nieder wrote:
Show 50 quoted lines
> The git_connect function is growing long.  Split the
> PROTO_GIT-specific portion to a separate function to make it easier to
> read.
> 
> No functional change intended.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Reviewed-by: Stefan Beller <sbeller@google.com>
> ---
> As before.
> 
>  connect.c | 103 +++++++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 59 insertions(+), 44 deletions(-)
> 
> diff --git a/connect.c b/connect.c
> index aa994d1518..9425229206 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -861,6 +861,64 @@ static enum ssh_variant determine_ssh_variant(const char *ssh_command,
>  	return ssh_variant;
>  }
>  
> +/*
> + * Open a connection using Git's native protocol.
> + *
> + * The caller is responsible for freeing hostandport, but this function may
> + * modify it (for example, to truncate it to remove the port part).
> + */
> +static struct child_process *git_connect_git(int fd[2], char *hostandport,
> +					     const char *path, const char *prog,
> +					     int flags)
> +{
> +	struct child_process *conn;
> +	struct strbuf request = STRBUF_INIT;
> +	/*
> +	 * Set up virtual host information based on where we will
> +	 * connect, unless the user has overridden us in
> +	 * the environment.
> +	 */
> +	char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
> +	if (target_host)
> +		target_host = xstrdup(target_host);
> +	else
> +		target_host = xstrdup(hostandport);
> +
> +	transport_check_allowed("git");
> +
> +	/* These underlying connection commands die() if they
> +	 * cannot connect.
> +	 */

I know this is really just code motion but maybe we can fix the style of the comment here?

Show 88 quoted lines
> +	if (git_use_proxy(hostandport))
> +		conn = git_proxy_connect(fd, hostandport);
> +	else
> +		conn = git_tcp_connect(fd, hostandport, flags);
> +	/*
> +	 * Separate original protocol components prog and path
> +	 * from extended host header with a NUL byte.
> +	 *
> +	 * Note: Do not add any other headers here!  Doing so
> +	 * will cause older git-daemon servers to crash.
> +	 */
> +	strbuf_addf(&request,
> +		    "%s %s%chost=%s%c",
> +		    prog, path, 0,
> +		    target_host, 0);
> +
> +	/* If using a new version put that stuff here after a second null byte */
> +	if (get_protocol_version_config() > 0) {
> +		strbuf_addch(&request, '\0');
> +		strbuf_addf(&request, "version=%d%c",
> +			    get_protocol_version_config(), '\0');
> +	}
> +
> +	packet_write(fd[1], request.buf, request.len);
> +
> +	free(target_host);
> +	strbuf_release(&request);
> +	return conn;
> +}
> +
>  /*
>   * This returns the dummy child_process `no_fork` if the transport protocol
>   * does not need fork(2), or a struct child_process object if it does.  Once
> @@ -892,50 +950,7 @@ struct child_process *git_connect(int fd[2], const char *url,
>  		printf("Diag: path=%s\n", path ? path : "NULL");
>  		conn = NULL;
>  	} else if (protocol == PROTO_GIT) {
> -		struct strbuf request = STRBUF_INIT;
> -		/*
> -		 * Set up virtual host information based on where we will
> -		 * connect, unless the user has overridden us in
> -		 * the environment.
> -		 */
> -		char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
> -		if (target_host)
> -			target_host = xstrdup(target_host);
> -		else
> -			target_host = xstrdup(hostandport);
> -
> -		transport_check_allowed("git");
> -
> -		/* These underlying connection commands die() if they
> -		 * cannot connect.
> -		 */
> -		if (git_use_proxy(hostandport))
> -			conn = git_proxy_connect(fd, hostandport);
> -		else
> -			conn = git_tcp_connect(fd, hostandport, flags);
> -		/*
> -		 * Separate original protocol components prog and path
> -		 * from extended host header with a NUL byte.
> -		 *
> -		 * Note: Do not add any other headers here!  Doing so
> -		 * will cause older git-daemon servers to crash.
> -		 */
> -		strbuf_addf(&request,
> -			    "%s %s%chost=%s%c",
> -			    prog, path, 0,
> -			    target_host, 0);
> -
> -		/* If using a new version put that stuff here after a second null byte */
> -		if (get_protocol_version_config() > 0) {
> -			strbuf_addch(&request, '\0');
> -			strbuf_addf(&request, "version=%d%c",
> -				    get_protocol_version_config(), '\0');
> -		}
> -
> -		packet_write(fd[1], request.buf, request.len);
> -
> -		free(target_host);
> -		strbuf_release(&request);
> +		conn = git_connect_git(fd, hostandport, path, prog, flags);
>  	} else {
>  		struct strbuf cmd = STRBUF_INIT;
>  		const char *const *var;
> -- 
> 2.15.0.448.gf294e3d99a
> 
-- 
Brandon Williams
Previous: Jonathan NiederNext: Jonathan Nieder
Message 9 of 27 in “Coping with unrecognized ssh wrapper scripts in GIT_SSH”
  1. 0/8 Coping with unrecognized ssh wrapper scripts in GIT_SSHJonathan Nieder, Nov 20, 2017
  2. 1/8 ssh test: make copy_ssh_wrapper_as clean up after itselfJonathan Nieder, Nov 20, 2017
  3. Brandon WilliamsNov 20, 2017
  4. Junio C HamanoNov 21, 2017
  5. 1/8 ssh test: make copy_ssh_wrapper_as clean up after itselfJonathan Nieder, Nov 21, 2017
  6. Stefan BellerNov 21, 2017
  7. 2/8 connect: move no_fork fallback to git_tcp_connectJonathan Nieder, Nov 20, 2017
  8. 3/8 connect: split git:// setup into a separate functionJonathan Nieder, Nov 20, 2017
  9. Brandon WilliamsNov 20, 2017
  10. Jonathan NiederNov 20, 2017
  11. Brandon WilliamsNov 20, 2017
  12. 4/8 connect: split ssh command line options into separate functionJonathan Nieder, Nov 20, 2017
  13. Brandon WilliamsNov 20, 2017
  14. Jonathan NiederNov 20, 2017
  15. Brandon WilliamsNov 20, 2017
  16. 4/8 connect: split ssh command line options into separate functionJonathan Nieder, Nov 20, 2017
  17. 5/8 connect: split ssh option computation to its own functionJonathan Nieder, Nov 20, 2017
  18. Junio C HamanoNov 21, 2017
  19. 6/8 ssh: 'auto' variant to select between 'ssh' and 'simple'Jonathan Nieder, Nov 20, 2017
  20. Brandon WilliamsNov 20, 2017
  21. Junio C HamanoNov 21, 2017
  22. Jonathan NiederNov 21, 2017
  23. 7/8 ssh: 'simple' variant does not support -4/-6Jonathan Nieder, Nov 20, 2017
  24. 8/8 ssh: 'simple' variant does not support --portJonathan Nieder, Nov 20, 2017
  25. Brandon WilliamsNov 20, 2017
  26. Stefan BellerNov 22, 2017
  27. Junio C HamanoNov 22, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.