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

[Johannes Sixt <j6t@kdbg.org>]

Am 11.06.2015 um 20:59 schrieb 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.

When you develop server software, shouldn't you test drive the server 
via the bare metal protocol anyway? That *is* painful, but unavoidable 
because you must harden the server against any garbage that a 
potentially malicous client could throw at it. Restricting yourself to a 
well-behaved client such as fetch-pack is only half the deal.

That said, I do think that fetch-pack could learn a mode that makes it 
easier to debug the normal behavior of a server (if such a mode is 
missing currently).

What is the problem with the current fetch-pack implementation? Does it 
remove a bogus packfile after download? Does it abort during download 
when it detects a broken packfile? Does --keep not do what you need?

Instead of your approach (which forks off tee to dump a copy of the 
packfile), would it not be simpler to add an option --debug-pack 
(probably not the best name) that skips the cleanup step when a broken 
packfile is detected and prints the name of the downloaded packfile?

> 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.
> +			 */

			/*
			 * Write multi-line comments
			 * like this (/* on its own line)
			 */

> +			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);

Are you abusing sha1write() and sha1flush() to write a byte sequence to 
a file? Is write_in_full() not sufficient?

> +			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");

start_command() can create the pipe for you. Just say cmd2.out = -1.

> +		cmd2.out = pipefds[1];
> +		cmd.in = pipefds[0];
> +		if (start_command(&cmd2))
> +			die("couldn't start tee to save a pack");

When you call start_command(), you must also call finish_command(). 
start_command() prints an error message for you; you don't have to do 
that (the start_command() in the context below is a bad example).

> +	} 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) &&

This can be written as

	(
		cd clone &&
		GIT_SAVE_FETCHED_PACK_TO=../saved.pack git pull
	) &&

without 'export'.

> +	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 &&

Same here.

> +	test -e saved.pack &&
> +	git index-pack saved.pack
> +'
> +
>   test_expect_success 'clone does not keep pack' '
>
>   	rm -fr dst &&
>

-- Hannes