Re: [PATCH v6 05/10] git fetch-pack: Add --diag-url

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 21, 2013, 22:46 UTC
Message-ID: <xmqqob5d2x0y.fsf@gitster.dls.corp.google.com>
In-Reply-To: <201311212140.49698.tboegi@web.de>

Torsten Bögershausen <tboegi@web.de> writes:

>> Subject: Re: [PATCH v6 05/10] git fetch-pack: Add --diag-url

s/Add/add/ please.

Show 13 quoted lines

> The main purpose is to trace the URL parser called by git_connect() in
> connect.c
>
> The main features of the parser can be listed as this:
> - parse out host and path for URLs with a scheme (git:// file:// ssh://)
> - parse host names embedded by [] correctly
> - extract the port number, if present
> - seperate URLs like "file" (which are local)
>   from URLs like "host:repo" which should use ssh
>
> Add the new parameter "--diag-url" to "git fetch-pack",
> which prints the value for protocol, host and path to stderr and exits.
> ---

Sign-off?

Show 57 quoted lines

>  builtin/fetch-pack.c  | 14 ++++++++++---
>  connect.c             | 27 ++++++++++++++++++++++++
>  connect.h             |  1 +
>  fetch-pack.h          |  1 +
>  t/t5500-fetch-pack.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 97 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index c8e8582..758b5ac 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -7,7 +7,7 @@
>  static const char fetch_pack_usage[] =
>  "git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin] "
>  "[--include-tag] [--upload-pack=<git-upload-pack>] [--depth=<n>] "
> -"[--no-progress] [-v] [<host>:]<directory> [<refs>...]";
> +"[--no-progress] [--diag-url] [-v] [<host>:]<directory> [<refs>...]";
>  
>  static void add_sought_entry_mem(struct ref ***sought, int *nr, int *alloc,
>  				 const char *name, int namelen)
> @@ -81,6 +81,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>  			args.stdin_refs = 1;
>  			continue;
>  		}
> +		if (!strcmp("--diag-url", arg)) {
> +			args.diag_url = 1;
> +			continue;
> +		}
>  		if (!strcmp("-v", arg)) {
>  			args.verbose = 1;
>  			continue;
> @@ -146,10 +150,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>  		fd[0] = 0;
>  		fd[1] = 1;
>  	} else {
> +		int flags = args.verbose ? CONNECT_VERBOSE : 0;
> +		if (args.diag_url)
> +			flags |= CONNECT_DIAG_URL;
>  		conn = git_connect(fd, dest, args.uploadpack,
> -				   args.verbose ? CONNECT_VERBOSE : 0);
> +				   flags);
> +		if (!conn)
> +			return args.diag_url ? 0 : 1;
>  	}
> -
>  	get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL);
>  
>  	ref = fetch_pack(&args, fd, conn, ref, dest,
> diff --git a/connect.c b/connect.c
> index a6cf345..1b93b4d 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -236,6 +236,19 @@ enum protocol {
>  	PROTO_GIT
>  };
>  
> +static const char *prot_name(enum protocol protocol) {

Style: please move that "{" to the beginning of the next line (see the
beginning of existing functions e.g. get_protocol()).

Show 10 quoted lines

> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index d87ddf7..9136f2a 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -531,5 +531,62 @@ test_expect_success 'shallow fetch with tags does not break the repository' '
>  		git fsck
>  	)
>  '
> +check_prot_path() {
> +	> actual &&

Style: no SP between the redirection operator and its target, i.e.

	>actual &&

> +	(git fetch-pack --diag-url "$1" 2>&1 1>stdout) | grep -v host= >actual &&

Do we use "stdout" in this test? Otherwise "1>/dev/null" would make it clearer what is going on.

> +	echo "Diag: url=$1" >expected &&
> +	echo "Diag: protocol=$2" >>expected &&
> +	echo "Diag: path=$3" >>expected &&

Perhaps this is a good place to use here-doc, i.e.

	cat >expected <<-EOF &&
	Diag: ...
        ...
        EOF

Show 21 quoted lines

> +	test_cmp expected actual
> +}
> +
> +check_prot_host_path() {
> +	> actual &&
> +	git fetch-pack --diag-url "$1" 2>actual &&
> +	echo "Diag: url=$1" >expected &&
> +	echo "Diag: protocol=$2" >>expected &&
> +	echo "Diag: host=$3" >>expected &&
> +	echo "Diag: path=$4" >>expected &&
> +	test_cmp expected actual
> +}
> +
> +for r in repo re:po re/po
> +do
> +	# git or ssh with scheme
> +	for p in "ssh+git" "git+ssh" git ssh
> +	do
> +		for h in host host:12 [::1] [::1]:23
> +		do
> +			if $(echo $p | grep ssh >/dev/null 2>/dev/null); then

Style: "; then" should be spelled as "LF" followed by "then" on the
next line by itself.

But more ipmportantly, the above tries to do

	if "some computed string"; then

which is very iffy.  I think you meant:

	case "$p" in
        *ssh*)
        	do ssh thing
                ;;
	*)
        	do other thing
	esac