Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.

[Junio C Hamano <gitster@pobox.com>]

Johannes Sixt <johannes.sixt@telecom.at> writes:

> This prepares the API of git_connect() and finish_connect() to operate on
> a struct child_process. Currently, we just use that object as a placeholder
> for the pid that we used to return. A follow-up patch will change the
> implementation of git_connect() and finish_connect() to make full use
> of the object.

Good description, except removal of checks for negative return
of the calling sites raised my eyebrow and had me spend a few
more minutes to review than necessary (see below).

> diff --git a/builtin-archive.c b/builtin-archive.c
> index 04385de..76db6cf 100644
> --- a/builtin-archive.c
> +++ b/builtin-archive.c
> @@ -30,7 +30,7 @@ static int run_remote_archiver(const char *remote, int argc,
>  {
>  	char *url, buf[LARGE_PACKET_MAX];
>  	int fd[2], i, len, rv;
> -	pid_t pid;
> +	struct child_process *chld;

Is "child" a reserved keyword or something that we need to avoid
its use as an identifier?

>  	const char *exec = "git-upload-archive";
>  	int exec_at = 0;
>  
> @@ -46,9 +46,7 @@ static int run_remote_archiver(const char *remote, int argc,
>  	}
>  
>  	url = xstrdup(remote);
> -	pid = git_connect(fd, url, exec, 0);
> -	if (pid < 0)
> -		return pid;
> +	chld = git_connect(fd, url, exec, 0);
>  

Interesting.  This and other callers of git_connect() were
prepared to see git_connect() to report errors with negative PID
but the callee simply died --- so this change is not regressing
by removing an early error return.  It would be better to have
something like this in the commit log message:

	Old code had early-return-on-error checks at the calling
	sites of git_connect(), but the callee simply died on
	errors without returning negative values.  This patch
	removes such bogosity.

> diff --git a/connect.c b/connect.c
> index 3d5c4ab..f6dcab9 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -468,21 +468,22 @@ char *get_port(char *host)
>  }
>  
>  /*
> - * This returns 0 if the transport protocol does not need fork(2),
> + * This returns NULL if the transport protocol does not need fork(2),
>   * or a process id if it does.  Once done, finish the connection

It does not return a process ID anymore, so this comment needs
to be updated.  Instead, you now return a struct child_process
that is newly allocated, and needs to be deallocated somehow.
At the end of finish_connect() might be a good place to do so.

>   * with finish_connect() with the value returned from this function
> - * (it is safe to call finish_connect() with 0 to support the former
> + * (it is safe to call finish_connect() with NULL to support the former
>   * case).
>   *
> - * Does not return a negative value on error; it just dies.
> + * If it returns, the connect is successful; it just dies on errors.
>   */
> -pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
> +struct child_process *git_connect(int fd[2], char *url,
> +				  const char *prog, int flags)
>  {
> ...
> -int finish_connect(pid_t pid)
> +int finish_connect(struct child_process *chld)
>  {
> -	if (pid == 0)
> +	if (chld == NULL)
>  		return 0;
>  
> -	while (waitpid(pid, NULL, 0) < 0) {
> +	while (waitpid(chld->pid, NULL, 0) < 0) {
>  		if (errno != EINTR)
>  			return -1;
>  	}

But it seems you don't, leaking the struct.  I see this is fixed
in the next patch in the series, but it would be nicer to have
the free from the very beginning.