* [PATCH] fetch-pack: optionally save packs to disk
@ 2015-06-11 17:44 Augie Fackler
2015-06-11 18:19 ` Stefan Beller
2015-06-11 19:33 ` Junio C Hamano
0 siblings, 2 replies; 3+ messages in thread
From: Augie Fackler @ 2015-06-11 17:44 UTC (permalink / raw)
To: git; +Cc: Augie Fackler
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.)
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] fetch-pack: optionally save packs to disk
2015-06-11 17:44 [PATCH] fetch-pack: optionally save packs to disk Augie Fackler
@ 2015-06-11 18:19 ` Stefan Beller
2015-06-11 19:33 ` Junio C Hamano
1 sibling, 0 replies; 3+ messages in thread
From: Stefan Beller @ 2015-06-11 18:19 UTC (permalink / raw)
To: Augie Fackler; +Cc: git@vger.kernel.org
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] fetch-pack: optionally save packs to disk
2015-06-11 17:44 [PATCH] fetch-pack: optionally save packs to disk Augie Fackler
2015-06-11 18:19 ` Stefan Beller
@ 2015-06-11 19:33 ` Junio C Hamano
1 sibling, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2015-06-11 19:33 UTC (permalink / raw)
To: Augie Fackler; +Cc: git
Augie Fackler <augie@google.com> writes:
> @@ -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;
decl-after-stmt here...
> 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;
Have a blank after decl block before the first statement here.
> + cmd2.argv = argv2;
> + av2 = argv2;
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-06-11 19:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-11 17:44 [PATCH] fetch-pack: optionally save packs to disk Augie Fackler
2015-06-11 18:19 ` Stefan Beller
2015-06-11 19:33 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).