{"thread":{"id":"39601","subject":"[PATCH] fetch-pack: optionally save packs to disk","startedAt":"2015-06-11T17:44:36Z","lastAt":"2015-06-11T19:33:17Z","messageCount":3,"participants":["Augie Fackler","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"263621","messageId":"1434044676-2942-1-git-send-email-augie@google.com","threadId":"39601","inReplyTo":null,"subject":"[PATCH] fetch-pack: optionally save packs to disk","fromName":"Augie Fackler","fromEmail":"augie@google.com","sentAt":"2015-06-11T17:44:36Z","receivedAt":"2015-06-11T17:44:36Z","isPatch":true,"sender":{"key":"augie@google.com","avatar":null},"body":"When developing server software, it's often helpful to save a\npotentially-bogus pack for later analysis. This makes that trivial,\ninstead of painful. This is made a little complicated by the fact that\nin some cases (like cloning from smart-http, but not from a local repo)\nthe fetch code reads the pack header before sending the pack to\nindex-pack (which then gets a --pack_header flag). The included tests\ncover both of these cases.\n\nTo use the new feature, set GIT_SAVE_FETCHED_PACK_TO to a file path and\ngit-fetch will do the rest. The resulting pack can be examined with\ngit-index-pack or similar tools (although if it's corrupt, custom tools\ncan be especially helpful.)\n---\n fetch-pack.c                | 44 ++++++++++++++++++++++++++++++++++++++++----\n t/t5551-http-fetch-smart.sh | 12 ++++++++++++\n t/t5601-clone.sh            |  9 +++++++++\n 3 files changed, 61 insertions(+), 4 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a912935..fe6ba58 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -684,7 +684,7 @@ static int get_pack(struct fetch_pack_args *args,\n \tconst char *argv[22];\n \tchar keep_arg[256];\n \tchar hdr_arg[256];\n-\tconst char **av, *cmd_name;\n+\tconst char **av, *cmd_name, *savepath;\n \tint do_keep = args->keep_pack;\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tint ret;\n@@ -708,9 +708,8 @@ static int get_pack(struct fetch_pack_args *args,\n \tcmd.argv = argv;\n \tav = argv;\n \t*hdr_arg = 0;\n+\tstruct pack_header header;\n \tif (!args->keep_pack && unpack_limit) {\n-\t\tstruct pack_header header;\n-\n \t\tif (read_pack_header(demux.out, &header))\n \t\t\tdie(\"protocol error: bad pack header\");\n \t\tsnprintf(hdr_arg, sizeof(hdr_arg),\n@@ -762,7 +761,44 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t*av++ = \"--strict\";\n \t*av++ = NULL;\n \n-\tcmd.in = demux.out;\n+\tsavepath = getenv(\"GIT_SAVE_FETCHED_PACK_TO\");\n+\tif (savepath) {\n+\t\tstruct child_process cmd2 = CHILD_PROCESS_INIT;\n+\t\tconst char *argv2[22];\n+\t\tint pipefds[2];\n+\t\tint e;\n+\t\tconst char **av2;\n+\t\tcmd2.argv = argv2;\n+\t\tav2 = argv2;\n+\t\t*av2++ = \"tee\";\n+\t\tif (*hdr_arg) {\n+\t\t\t/* hdr_arg being nonempty means we already read the\n+\t\t\t * pack header from demux, so we need to drop a pack\n+\t\t\t * header in place for tee to append to, otherwise\n+\t\t\t * we'll end up with a broken pack on disk.\n+\t\t\t */\n+\t\t\tint fp;\n+\t\t\tstruct sha1file *s;\n+\t\t\tfp = open(savepath, O_CREAT | O_TRUNC | O_WRONLY, 0666);\n+\t\t\ts = sha1fd_throughput(fp, savepath, NULL);\n+\t\t\tsha1write(s, &header, sizeof(header));\n+\t\t\tsha1flush(s);\n+\t\t\tclose(fp);\n+\t\t\t/* -a is supported by both GNU and BSD tee */\n+\t\t\t*av2++ = \"-a\";\n+\t\t}\n+\t\t*av2++ = savepath;\n+\t\t*av2++ = NULL;\n+\t\tcmd2.in = demux.out;\n+\t\te = pipe(pipefds);\n+\t\tif (e != 0)\n+\t\t\tdie(\"couldn't make pipe to save pack\");\n+\t\tcmd2.out = pipefds[1];\n+\t\tcmd.in = pipefds[0];\n+\t\tif (start_command(&cmd2))\n+\t\t\tdie(\"couldn't start tee to save a pack\");\n+\t} else\n+\t\tcmd.in = demux.out;\n \tcmd.git_cmd = 1;\n \tif (start_command(&cmd))\n \t\tdie(\"fetch-pack: unable to fork off %s\", cmd_name);\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 58207d8..bf4640d 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -82,11 +82,23 @@ test_expect_success 'fetch changes via http' '\n \ttest_cmp file clone/file\n '\n \n+test_expect_success 'fetch changes via http and save pack' '\n+\techo content >>file &&\n+\tgit commit -a -m two &&\n+\tgit push public &&\n+\tGIT_SAVE_FETCHED_PACK_TO=saved.pack &&\n+\texport GIT_SAVE_FETCHED_PACK_TO &&\n+\t(cd clone && git pull) &&\n+\tgit index-pack clone/saved.pack\n+'\n+\n cat >exp <<EOF\n GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200\n POST /smart/repo.git/git-upload-pack HTTP/1.1 200\n GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200\n POST /smart/repo.git/git-upload-pack HTTP/1.1 200\n+GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200\n+POST /smart/repo.git/git-upload-pack HTTP/1.1 200\n EOF\n test_expect_success 'used upload-pack service' '\n \tsed -e \"\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex bfdaf75..73f9e1c 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -40,6 +40,15 @@ test_expect_success C_LOCALE_OUTPUT 'output from clone' '\n \ttest $(grep Clon output | wc -l) = 1\n '\n \n+test_expect_success 'clone allows saving a pack' '\n+\trm -fr dst saved.pack &&\n+\tGIT_SAVE_FETCHED_PACK_TO=saved.pack &&\n+\texport GIT_SAVE_FETCHED_PACK_TO &&\n+\tgit clone -n \"file://$(pwd)/src\" dst >output 2>&1 &&\n+\ttest -e saved.pack &&\n+\tgit index-pack saved.pack\n+'\n+\n test_expect_success 'clone does not keep pack' '\n \n \trm -fr dst &&\n-- \n2.4.3.369.gda395ba.dirty\n"},{"id":"263625","messageId":"CAGZ79kZo7ZCu8c=w6fxtccahX7vf1oo2ud8M2-q2AL_FuAyjsA@mail.gmail.com","threadId":"39601","inReplyTo":"1434044676-2942-1-git-send-email-augie@google.com","subject":"Re: [PATCH] fetch-pack: optionally save packs to disk","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-06-11T18:19:21Z","receivedAt":"2015-06-11T18:19:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jun 11, 2015 at 10:44 AM, Augie Fackler <augie@google.com> wrote:\n> When developing server software, it's often helpful to save a\n> potentially-bogus pack for later analysis. This makes that trivial,\n> instead of painful. This is made a little complicated by the fact that\n> in some cases (like cloning from smart-http, but not from a local repo)\n> the fetch code reads the pack header before sending the pack to\n> index-pack (which then gets a --pack_header flag). The included tests\n> cover both of these cases.\n>\n> To use the new feature, set GIT_SAVE_FETCHED_PACK_TO to a file path and\n> git-fetch will do the rest. The resulting pack can be examined with\n> git-index-pack or similar tools (although if it's corrupt, custom tools\n> can be especially helpful.)\n\nPlease sign off your patch. (See Documentation/SubmittingPatches)\n\nDo we want to document the config variable in Documentation/config.txt ?\n\nThanks,\nStefan\n\n> ---\n>  fetch-pack.c                | 44 ++++++++++++++++++++++++++++++++++++++++----\n>  t/t5551-http-fetch-smart.sh | 12 ++++++++++++\n>  t/t5601-clone.sh            |  9 +++++++++\n>  3 files changed, 61 insertions(+), 4 deletions(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index a912935..fe6ba58 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -684,7 +684,7 @@ static int get_pack(struct fetch_pack_args *args,\n>         const char *argv[22];\n>         char keep_arg[256];\n>         char hdr_arg[256];\n> -       const char **av, *cmd_name;\n> +       const char **av, *cmd_name, *savepath;\n>         int do_keep = args->keep_pack;\n>         struct child_process cmd = CHILD_PROCESS_INIT;\n>         int ret;\n> @@ -708,9 +708,8 @@ static int get_pack(struct fetch_pack_args *args,\n>         cmd.argv = argv;\n>         av = argv;\n>         *hdr_arg = 0;\n> +       struct pack_header header;\n>         if (!args->keep_pack && unpack_limit) {\n> -               struct pack_header header;\n> -\n>                 if (read_pack_header(demux.out, &header))\n>                         die(\"protocol error: bad pack header\");\n>                 snprintf(hdr_arg, sizeof(hdr_arg),\n> @@ -762,7 +761,44 @@ static int get_pack(struct fetch_pack_args *args,\n>                 *av++ = \"--strict\";\n>         *av++ = NULL;\n>\n> -       cmd.in = demux.out;\n> +       savepath = getenv(\"GIT_SAVE_FETCHED_PACK_TO\");\n> +       if (savepath) {\n> +               struct child_process cmd2 = CHILD_PROCESS_INIT;\n> +               const char *argv2[22];\n> +               int pipefds[2];\n> +               int e;\n> +               const char **av2;\n> +               cmd2.argv = argv2;\n> +               av2 = argv2;\n> +               *av2++ = \"tee\";\n> +               if (*hdr_arg) {\n> +                       /* hdr_arg being nonempty means we already read the\n> +                        * pack header from demux, so we need to drop a pack\n> +                        * header in place for tee to append to, otherwise\n> +                        * we'll end up with a broken pack on disk.\n> +                        */\n> +                       int fp;\n> +                       struct sha1file *s;\n> +                       fp = open(savepath, O_CREAT | O_TRUNC | O_WRONLY, 0666);\n> +                       s = sha1fd_throughput(fp, savepath, NULL);\n> +                       sha1write(s, &header, sizeof(header));\n> +                       sha1flush(s);\n> +                       close(fp);\n> +                       /* -a is supported by both GNU and BSD tee */\n> +                       *av2++ = \"-a\";\n> +               }\n> +               *av2++ = savepath;\n> +               *av2++ = NULL;\n> +               cmd2.in = demux.out;\n> +               e = pipe(pipefds);\n> +               if (e != 0)\n> +                       die(\"couldn't make pipe to save pack\");\n> +               cmd2.out = pipefds[1];\n> +               cmd.in = pipefds[0];\n> +               if (start_command(&cmd2))\n> +                       die(\"couldn't start tee to save a pack\");\n> +       } else\n> +               cmd.in = demux.out;\n>         cmd.git_cmd = 1;\n>         if (start_command(&cmd))\n>                 die(\"fetch-pack: unable to fork off %s\", cmd_name);\n> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> index 58207d8..bf4640d 100755\n> --- a/t/t5551-http-fetch-smart.sh\n> +++ b/t/t5551-http-fetch-smart.sh\n> @@ -82,11 +82,23 @@ test_expect_success 'fetch changes via http' '\n>         test_cmp file clone/file\n>  '\n>\n> +test_expect_success 'fetch changes via http and save pack' '\n> +       echo content >>file &&\n> +       git commit -a -m two &&\n> +       git push public &&\n> +       GIT_SAVE_FETCHED_PACK_TO=saved.pack &&\n> +       export GIT_SAVE_FETCHED_PACK_TO &&\n> +       (cd clone && git pull) &&\n> +       git index-pack clone/saved.pack\n> +'\n> +\n>  cat >exp <<EOF\n>  GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200\n>  POST /smart/repo.git/git-upload-pack HTTP/1.1 200\n>  GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200\n>  POST /smart/repo.git/git-upload-pack HTTP/1.1 200\n> +GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200\n> +POST /smart/repo.git/git-upload-pack HTTP/1.1 200\n>  EOF\n>  test_expect_success 'used upload-pack service' '\n>         sed -e \"\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index bfdaf75..73f9e1c 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -40,6 +40,15 @@ test_expect_success C_LOCALE_OUTPUT 'output from clone' '\n>         test $(grep Clon output | wc -l) = 1\n>  '\n>\n> +test_expect_success 'clone allows saving a pack' '\n> +       rm -fr dst saved.pack &&\n> +       GIT_SAVE_FETCHED_PACK_TO=saved.pack &&\n> +       export GIT_SAVE_FETCHED_PACK_TO &&\n> +       git clone -n \"file://$(pwd)/src\" dst >output 2>&1 &&\n> +       test -e saved.pack &&\n> +       git index-pack saved.pack\n> +'\n> +\n>  test_expect_success 'clone does not keep pack' '\n>\n>         rm -fr dst &&\n> --\n> 2.4.3.369.gda395ba.dirty\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"263633","messageId":"xmqq8ubqf3b6.fsf@gitster.dls.corp.google.com","threadId":"39601","inReplyTo":"1434044676-2942-1-git-send-email-augie@google.com","subject":"Re: [PATCH] fetch-pack: optionally save packs to disk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-11T19:33:17Z","receivedAt":"2015-06-11T19:33:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Augie Fackler <augie@google.com> writes:\n\n> @@ -708,9 +708,8 @@ static int get_pack(struct fetch_pack_args *args,\n>  \tcmd.argv = argv;\n>  \tav = argv;\n>  \t*hdr_arg = 0;\n> +\tstruct pack_header header;\n\ndecl-after-stmt here...\n\n>  \tif (!args->keep_pack && unpack_limit) {\n> -\t\tstruct pack_header header;\n> -\n>  \t\tif (read_pack_header(demux.out, &header))\n>  \t\t\tdie(\"protocol error: bad pack header\");\n>  \t\tsnprintf(hdr_arg, sizeof(hdr_arg),\n> @@ -762,7 +761,44 @@ static int get_pack(struct fetch_pack_args *args,\n>  \t\t*av++ = \"--strict\";\n>  \t*av++ = NULL;\n>  \n> -\tcmd.in = demux.out;\n> +\tsavepath = getenv(\"GIT_SAVE_FETCHED_PACK_TO\");\n> +\tif (savepath) {\n> +\t\tstruct child_process cmd2 = CHILD_PROCESS_INIT;\n> +\t\tconst char *argv2[22];\n> +\t\tint pipefds[2];\n> +\t\tint e;\n> +\t\tconst char **av2;\n\nHave a blank after decl block before the first statement here.\n\n> +\t\tcmd2.argv = argv2;\n> +\t\tav2 = argv2;\n"}]}