Re: [PATCH] fetch-pack: optionally save packs to disk

[Stefan Beller <sbeller@google.com>]

On Thu, Jun 11, 2015 at 10:44 AM, Augie Fackler <augie@google.com> wrote:
> When developing server software, it's often helpful to save a
> potentially-bogus pack for later analysis. This makes that trivial,
> instead of painful. This is made a little complicated by the fact that
> in some cases (like cloning from smart-http, but not from a local repo)
> the fetch code reads the pack header before sending the pack to
> index-pack (which then gets a --pack_header flag). The included tests
> cover both of these cases.
>
> To use the new feature, set GIT_SAVE_FETCHED_PACK_TO to a file path and
> git-fetch will do the rest. The resulting pack can be examined with
> git-index-pack or similar tools (although if it's corrupt, custom tools
> can be especially helpful.)

Please sign off your patch. (See Documentation/SubmittingPatches)

Do we want to document the config variable in Documentation/config.txt ?

Thanks,
Stefan

> ---
>  fetch-pack.c                | 44 ++++++++++++++++++++++++++++++++++++++++----
>  t/t5551-http-fetch-smart.sh | 12 ++++++++++++
>  t/t5601-clone.sh            |  9 +++++++++
>  3 files changed, 61 insertions(+), 4 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index a912935..fe6ba58 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -684,7 +684,7 @@ static int get_pack(struct fetch_pack_args *args,
>         const char *argv[22];
>         char keep_arg[256];
>         char hdr_arg[256];
> -       const char **av, *cmd_name;
> +       const char **av, *cmd_name, *savepath;
>         int do_keep = args->keep_pack;
>         struct child_process cmd = CHILD_PROCESS_INIT;
>         int ret;
> @@ -708,9 +708,8 @@ static int get_pack(struct fetch_pack_args *args,
>         cmd.argv = argv;
>         av = argv;
>         *hdr_arg = 0;
> +       struct pack_header header;
>         if (!args->keep_pack && unpack_limit) {
> -               struct pack_header header;
> -
>                 if (read_pack_header(demux.out, &header))
>                         die("protocol error: bad pack header");
>                 snprintf(hdr_arg, sizeof(hdr_arg),
> @@ -762,7 +761,44 @@ static int get_pack(struct fetch_pack_args *args,
>                 *av++ = "--strict";
>         *av++ = NULL;
>
> -       cmd.in = demux.out;
> +       savepath = getenv("GIT_SAVE_FETCHED_PACK_TO");
> +       if (savepath) {
> +               struct child_process cmd2 = CHILD_PROCESS_INIT;
> +               const char *argv2[22];
> +               int pipefds[2];
> +               int e;
> +               const char **av2;
> +               cmd2.argv = argv2;
> +               av2 = argv2;
> +               *av2++ = "tee";
> +               if (*hdr_arg) {
> +                       /* hdr_arg being nonempty means we already read the
> +                        * pack header from demux, so we need to drop a pack
> +                        * header in place for tee to append to, otherwise
> +                        * we'll end up with a broken pack on disk.
> +                        */
> +                       int fp;
> +                       struct sha1file *s;
> +                       fp = open(savepath, O_CREAT | O_TRUNC | O_WRONLY, 0666);
> +                       s = sha1fd_throughput(fp, savepath, NULL);
> +                       sha1write(s, &header, sizeof(header));
> +                       sha1flush(s);
> +                       close(fp);
> +                       /* -a is supported by both GNU and BSD tee */
> +                       *av2++ = "-a";
> +               }
> +               *av2++ = savepath;
> +               *av2++ = NULL;
> +               cmd2.in = demux.out;
> +               e = pipe(pipefds);
> +               if (e != 0)
> +                       die("couldn't make pipe to save pack");
> +               cmd2.out = pipefds[1];
> +               cmd.in = pipefds[0];
> +               if (start_command(&cmd2))
> +                       die("couldn't start tee to save a pack");
> +       } else
> +               cmd.in = demux.out;
>         cmd.git_cmd = 1;
>         if (start_command(&cmd))
>                 die("fetch-pack: unable to fork off %s", cmd_name);
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 58207d8..bf4640d 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -82,11 +82,23 @@ test_expect_success 'fetch changes via http' '
>         test_cmp file clone/file
>  '
>
> +test_expect_success 'fetch changes via http and save pack' '
> +       echo content >>file &&
> +       git commit -a -m two &&
> +       git push public &&
> +       GIT_SAVE_FETCHED_PACK_TO=saved.pack &&
> +       export GIT_SAVE_FETCHED_PACK_TO &&
> +       (cd clone && git pull) &&
> +       git index-pack clone/saved.pack
> +'
> +
>  cat >exp <<EOF
>  GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
>  POST /smart/repo.git/git-upload-pack HTTP/1.1 200
>  GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
>  POST /smart/repo.git/git-upload-pack HTTP/1.1 200
> +GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
> +POST /smart/repo.git/git-upload-pack HTTP/1.1 200
>  EOF
>  test_expect_success 'used upload-pack service' '
>         sed -e "
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index bfdaf75..73f9e1c 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -40,6 +40,15 @@ test_expect_success C_LOCALE_OUTPUT 'output from clone' '
>         test $(grep Clon output | wc -l) = 1
>  '
>
> +test_expect_success 'clone allows saving a pack' '
> +       rm -fr dst saved.pack &&
> +       GIT_SAVE_FETCHED_PACK_TO=saved.pack &&
> +       export GIT_SAVE_FETCHED_PACK_TO &&
> +       git clone -n "file://$(pwd)/src" dst >output 2>&1 &&
> +       test -e saved.pack &&
> +       git index-pack saved.pack
> +'
> +
>  test_expect_success 'clone does not keep pack' '
>
>         rm -fr dst &&
> --
> 2.4.3.369.gda395ba.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html