{"thread":{"id":"35382","subject":"[PATCH v6 05/10] git fetch-pack: Add --diag-url","startedAt":"2013-11-21T20:40:48Z","lastAt":"2013-11-22T12:56:29Z","messageCount":3,"participants":["Torsten Bögershausen","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":6,"patchTotal":10},"messages":[{"id":"230907","messageId":"201311212140.49698.tboegi@web.de","threadId":"35382","inReplyTo":null,"subject":"[PATCH v6 05/10] git fetch-pack: Add --diag-url","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-11-21T20:40:48Z","receivedAt":"2013-11-21T20:40:48Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"The main purpose is to trace the URL parser called by git_connect() in\nconnect.c\n\nThe main features of the parser can be listed as this:\n- parse out host and path for URLs with a scheme (git:// file:// ssh://)\n- parse host names embedded by [] correctly\n- extract the port number, if present\n- seperate URLs like \"file\" (which are local)\n  from URLs like \"host:repo\" which should use ssh\n\nAdd the new parameter \"--diag-url\" to \"git fetch-pack\",\nwhich prints the value for protocol, host and path to stderr and exits.\n---\n builtin/fetch-pack.c  | 14 ++++++++++---\n connect.c             | 27 ++++++++++++++++++++++++\n connect.h             |  1 +\n fetch-pack.h          |  1 +\n t/t5500-fetch-pack.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 97 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex c8e8582..758b5ac 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -7,7 +7,7 @@\n static const char fetch_pack_usage[] =\n \"git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin] \"\n \"[--include-tag] [--upload-pack=<git-upload-pack>] [--depth=<n>] \"\n-\"[--no-progress] [-v] [<host>:]<directory> [<refs>...]\";\n+\"[--no-progress] [--diag-url] [-v] [<host>:]<directory> [<refs>...]\";\n \n static void add_sought_entry_mem(struct ref ***sought, int *nr, int *alloc,\n \t\t\t\t const char *name, int namelen)\n@@ -81,6 +81,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\targs.stdin_refs = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(\"--diag-url\", arg)) {\n+\t\t\targs.diag_url = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!strcmp(\"-v\", arg)) {\n \t\t\targs.verbose = 1;\n \t\t\tcontinue;\n@@ -146,10 +150,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\tfd[0] = 0;\n \t\tfd[1] = 1;\n \t} else {\n+\t\tint flags = args.verbose ? CONNECT_VERBOSE : 0;\n+\t\tif (args.diag_url)\n+\t\t\tflags |= CONNECT_DIAG_URL;\n \t\tconn = git_connect(fd, dest, args.uploadpack,\n-\t\t\t\t   args.verbose ? CONNECT_VERBOSE : 0);\n+\t\t\t\t   flags);\n+\t\tif (!conn)\n+\t\t\treturn args.diag_url ? 0 : 1;\n \t}\n-\n \tget_remote_heads(fd[0], NULL, 0, &ref, 0, NULL);\n \n \tref = fetch_pack(&args, fd, conn, ref, dest,\ndiff --git a/connect.c b/connect.c\nindex a6cf345..1b93b4d 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -236,6 +236,19 @@ enum protocol {\n \tPROTO_GIT\n };\n \n+static const char *prot_name(enum protocol protocol) {\n+\tswitch (protocol) {\n+\t\tcase PROTO_LOCAL:\n+\t\t\treturn \"file\";\n+\t\tcase PROTO_SSH:\n+\t\t\treturn \"ssh\";\n+\t\tcase PROTO_GIT:\n+\t\t\treturn \"git\";\n+\t\tdefault:\n+\t\t\treturn \"unkown protocol\";\n+\t}\n+}\n+\n static enum protocol get_protocol(const char *name)\n {\n \tif (!strcmp(name, \"ssh\"))\n@@ -670,6 +683,20 @@ struct child_process *git_connect(int fd[2], const char *url,\n \tsignal(SIGCHLD, SIG_DFL);\n \n \tprotocol = parse_connect_url(url, &host, &port, &path);\n+\tif (flags & CONNECT_DIAG_URL) {\n+\t\tfprintf(stderr, \"Diag: url=%s\\n\", url ? url : \"NULL\");\n+\t\tfprintf(stderr, \"Diag: protocol=%s\\n\", prot_name(protocol));\n+\t\tfprintf(stderr, \"Diag: host=%s\", host ? host : \"NULL\");\n+\t\tif (port)\n+\t\t\tfprintf(stderr, \":%s\\n\", port);\n+\t\telse\n+\t\t\tfprintf(stderr, \"\\n\");\n+\t\tfprintf(stderr, \"Diag: path=%s\\n\", path ? path : \"NULL\");\n+\t\tfree(host);\n+\t\tfree(port);\n+\t\tfree(path);\n+\t\treturn NULL;\n+\t}\n \n \tif (protocol == PROTO_GIT) {\n \t\t/* These underlying connection commands die() if they\ndiff --git a/connect.h b/connect.h\nindex 64fb7db..527d58a 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -2,6 +2,7 @@\n #define CONNECT_H\n \n #define CONNECT_VERBOSE       (1u << 0)\n+#define CONNECT_DIAG_URL      (1u << 1)\n extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);\n extern int finish_connect(struct child_process *conn);\n extern int git_connection_is_socket(struct child_process *conn);\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 461cbf3..20ccc12 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -14,6 +14,7 @@ struct fetch_pack_args {\n \t\tuse_thin_pack:1,\n \t\tfetch_all:1,\n \t\tstdin_refs:1,\n+\t\tdiag_url:1,\n \t\tverbose:1,\n \t\tno_progress:1,\n \t\tinclude_tag:1,\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex d87ddf7..9136f2a 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -531,5 +531,62 @@ test_expect_success 'shallow fetch with tags does not break the repository' '\n \t\tgit fsck\n \t)\n '\n+check_prot_path() {\n+\t> actual &&\n+\t(git fetch-pack --diag-url \"$1\" 2>&1 1>stdout) | grep -v host= >actual &&\n+\techo \"Diag: url=$1\" >expected &&\n+\techo \"Diag: protocol=$2\" >>expected &&\n+\techo \"Diag: path=$3\" >>expected &&\n+\ttest_cmp expected actual\n+}\n+\n+check_prot_host_path() {\n+\t> actual &&\n+\tgit fetch-pack --diag-url \"$1\" 2>actual &&\n+\techo \"Diag: url=$1\" >expected &&\n+\techo \"Diag: protocol=$2\" >>expected &&\n+\techo \"Diag: host=$3\" >>expected &&\n+\techo \"Diag: path=$4\" >>expected &&\n+\ttest_cmp expected actual\n+}\n+\n+for r in repo re:po re/po\n+do\n+\t# git or ssh with scheme\n+\tfor p in \"ssh+git\" \"git+ssh\" git ssh\n+\tdo\n+\t\tfor h in host host:12 [::1] [::1]:23\n+\t\tdo\n+\t\t\tif $(echo $p | grep ssh >/dev/null 2>/dev/null); then\n+\t\t\t\thh=$(echo $h | tr -d \"[]\")\n+\t\t\t\tpp=ssh\n+\t\t\telse\n+\t\t\t\thh=$h\n+\t\t\t\tpp=$p\n+\t\t\tfi\n+\t\t\ttest_expect_success \"fetch-pack $p://$h/$r\" '\n+\t\t\t\tcheck_prot_host_path $p://$h/$r $pp \"$hh\" \"/$r\"\n+\t\t\t'\n+\t\t\t# \"/~\" -> \"~\" conversion for git\n+\t\t\ttest_expect_success \"fetch-pack $p://$h/~$r\" '\n+\t\t\t\tcheck_prot_host_path $p://$h/~$r $pp \"$hh\" \"~$r\"\n+\t\t\t'\n+\t\tdone\n+\tdone\n+\t# file with scheme\n+\tfor p in file\n+\tdo\n+\t\tfor h in \"\" host host:12 [::1] [::1]:23\n+\t\tdo\n+\t\t\ttest_expect_success \"fetch-pack $p://$h/$r\" '\n+\t\t\t\tcheck_prot_path $p://$h/$r $p \"/$r\"\n+\t\t\t'\n+\t\t\t# No \"/~\" -> \"~\" conversion for file\n+\t\t\ttest_expect_success \"fetch-pack $p://$h/~$r\" '\n+\t\t\t\tcheck_prot_path $p://$h/~$r $p \"/~$r\"\n+\t\t\t'\n+\t\tdone\n+\tdone\n+done\n \n test_done\n-- \n1.8.4.457.g424cb08\n"},{"id":"230921","messageId":"xmqqob5d2x0y.fsf@gitster.dls.corp.google.com","threadId":"35382","inReplyTo":"201311212140.49698.tboegi@web.de","subject":"Re: [PATCH v6 05/10] git fetch-pack: Add --diag-url","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-21T22:46:37Z","receivedAt":"2013-11-21T22:46:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n>> Subject: Re: [PATCH v6 05/10] git fetch-pack: Add --diag-url\n\ns/Add/add/ please.\n\n> The main purpose is to trace the URL parser called by git_connect() in\n> connect.c\n>\n> The main features of the parser can be listed as this:\n> - parse out host and path for URLs with a scheme (git:// file:// ssh://)\n> - parse host names embedded by [] correctly\n> - extract the port number, if present\n> - seperate URLs like \"file\" (which are local)\n>   from URLs like \"host:repo\" which should use ssh\n>\n> Add the new parameter \"--diag-url\" to \"git fetch-pack\",\n> which prints the value for protocol, host and path to stderr and exits.\n> ---\n\nSign-off?\n\n>  builtin/fetch-pack.c  | 14 ++++++++++---\n>  connect.c             | 27 ++++++++++++++++++++++++\n>  connect.h             |  1 +\n>  fetch-pack.h          |  1 +\n>  t/t5500-fetch-pack.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++\n>  5 files changed, 97 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> index c8e8582..758b5ac 100644\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -7,7 +7,7 @@\n>  static const char fetch_pack_usage[] =\n>  \"git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin] \"\n>  \"[--include-tag] [--upload-pack=<git-upload-pack>] [--depth=<n>] \"\n> -\"[--no-progress] [-v] [<host>:]<directory> [<refs>...]\";\n> +\"[--no-progress] [--diag-url] [-v] [<host>:]<directory> [<refs>...]\";\n>  \n>  static void add_sought_entry_mem(struct ref ***sought, int *nr, int *alloc,\n>  \t\t\t\t const char *name, int namelen)\n> @@ -81,6 +81,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n>  \t\t\targs.stdin_refs = 1;\n>  \t\t\tcontinue;\n>  \t\t}\n> +\t\tif (!strcmp(\"--diag-url\", arg)) {\n> +\t\t\targs.diag_url = 1;\n> +\t\t\tcontinue;\n> +\t\t}\n>  \t\tif (!strcmp(\"-v\", arg)) {\n>  \t\t\targs.verbose = 1;\n>  \t\t\tcontinue;\n> @@ -146,10 +150,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n>  \t\tfd[0] = 0;\n>  \t\tfd[1] = 1;\n>  \t} else {\n> +\t\tint flags = args.verbose ? CONNECT_VERBOSE : 0;\n> +\t\tif (args.diag_url)\n> +\t\t\tflags |= CONNECT_DIAG_URL;\n>  \t\tconn = git_connect(fd, dest, args.uploadpack,\n> -\t\t\t\t   args.verbose ? CONNECT_VERBOSE : 0);\n> +\t\t\t\t   flags);\n> +\t\tif (!conn)\n> +\t\t\treturn args.diag_url ? 0 : 1;\n>  \t}\n> -\n>  \tget_remote_heads(fd[0], NULL, 0, &ref, 0, NULL);\n>  \n>  \tref = fetch_pack(&args, fd, conn, ref, dest,\n> diff --git a/connect.c b/connect.c\n> index a6cf345..1b93b4d 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -236,6 +236,19 @@ enum protocol {\n>  \tPROTO_GIT\n>  };\n>  \n> +static const char *prot_name(enum protocol protocol) {\n\nStyle: please move that \"{\" to the beginning of the next line (see the\nbeginning of existing functions e.g. get_protocol()).\n\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index d87ddf7..9136f2a 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -531,5 +531,62 @@ test_expect_success 'shallow fetch with tags does not break the repository' '\n>  \t\tgit fsck\n>  \t)\n>  '\n> +check_prot_path() {\n> +\t> actual &&\n\nStyle: no SP between the redirection operator and its target, i.e.\n\n\t>actual &&\n\n> +\t(git fetch-pack --diag-url \"$1\" 2>&1 1>stdout) | grep -v host= >actual &&\n\nDo we use \"stdout\" in this test?  Otherwise \"1>/dev/null\" would make\nit clearer what is going on.\n\n> +\techo \"Diag: url=$1\" >expected &&\n> +\techo \"Diag: protocol=$2\" >>expected &&\n> +\techo \"Diag: path=$3\" >>expected &&\n\nPerhaps this is a good place to use here-doc, i.e.\n\n\tcat >expected <<-EOF &&\n\tDiag: ...\n        ...\n        EOF\n\n> +\ttest_cmp expected actual\n> +}\n> +\n> +check_prot_host_path() {\n> +\t> actual &&\n> +\tgit fetch-pack --diag-url \"$1\" 2>actual &&\n> +\techo \"Diag: url=$1\" >expected &&\n> +\techo \"Diag: protocol=$2\" >>expected &&\n> +\techo \"Diag: host=$3\" >>expected &&\n> +\techo \"Diag: path=$4\" >>expected &&\n> +\ttest_cmp expected actual\n> +}\n> +\n> +for r in repo re:po re/po\n> +do\n> +\t# git or ssh with scheme\n> +\tfor p in \"ssh+git\" \"git+ssh\" git ssh\n> +\tdo\n> +\t\tfor h in host host:12 [::1] [::1]:23\n> +\t\tdo\n> +\t\t\tif $(echo $p | grep ssh >/dev/null 2>/dev/null); then\n\nStyle: \"; then\" should be spelled as \"LF\" followed by \"then\" on the\nnext line by itself.\n\nBut more ipmportantly, the above tries to do\n\n\tif \"some computed string\"; then\n\nwhich is very iffy.  I think you meant:\n\n\tcase \"$p\" in\n        *ssh*)\n        \tdo ssh thing\n                ;;\n\t*)\n        \tdo other thing\n\tesac\n"},{"id":"230941","messageId":"20131122125629.GB9273@goldbirke","threadId":"35382","inReplyTo":"201311212140.49698.tboegi@web.de","subject":"Re: [PATCH v6 05/10] git fetch-pack: Add --diag-url","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2013-11-22T12:56:29Z","receivedAt":"2013-11-22T12:56:29Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nOn Thu, Nov 21, 2013 at 09:40:48PM +0100, Torsten Bögershausen wrote:\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index d87ddf7..9136f2a 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -531,5 +531,62 @@ test_expect_success 'shallow fetch with tags does not break the repository' '\n>  \t\tgit fsck\n>  \t)\n>  '\n> +check_prot_path() {\n> +\t> actual &&\n\nThere is no need to truncate actual here, ...\n\n> +\t(git fetch-pack --diag-url \"$1\" 2>&1 1>stdout) | grep -v host= >actual &&\n\n... because it will be overwritten in this line anyway.\n\n> +\techo \"Diag: url=$1\" >expected &&\n> +\techo \"Diag: protocol=$2\" >>expected &&\n> +\techo \"Diag: path=$3\" >>expected &&\n> +\ttest_cmp expected actual\n> +}\n> +\n> +check_prot_host_path() {\n> +\t> actual &&\n> +\tgit fetch-pack --diag-url \"$1\" 2>actual &&\n\nLikewise.\n\n\nBest,\nGábor\n"}]}