Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects

From: Nicolas Pitre <nico@cam.org>
Date: Jun 7, 2009, 20:48 UTC
Message-ID: <alpine.LFD.2.00.0906071637530.3906@xanadu.home>
In-Reply-To: <c77435a80906071155g5530ccdel286907b7c6022838@mail.gmail.com>

On Sun, 7 Jun 2009, Nick Edelen wrote:

> how does this look?

Comments below.

Show 17 quoted lines

> Signed-off-by: Nick Edelen <sirnot@gmail.com>
> 
> ---
>  t/t5530-upload-pack-error.sh |    2 +-
>  upload-pack.c                |   50 +++++++++++++++++++++++++++++++++--------
>  2 files changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh
> index f5102b9..26bcd1e 100755
> --- a/t/t5530-upload-pack-error.sh
> +++ b/t/t5530-upload-pack-error.sh
> @@ -51,7 +51,7 @@ test_expect_success 'fsck fails' '
>  test_expect_success 'upload-pack fails due to error in rev-list' '
> 
>  	! echo "0032want $(git rev-parse HEAD)
> -00000009done
> +0034shallow $(git rev-parse HEAD^)00000009done

Why did you modify this?

Show 31 quoted lines

>  0000" | git upload-pack . > /dev/null 2> output.err &&
>  	grep "waitpid (async) failed" output.err
>  '
> diff --git a/upload-pack.c b/upload-pack.c
> index edc7861..c8f2dca 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -29,6 +29,7 @@ static unsigned long oldest_have;
>  static int multi_ack, nr_our_refs;
>  static int use_thin_pack, use_ofs_delta, use_include_tag;
>  static int no_progress;
> +static int shallow_nr;
>  static struct object_array have_obj;
>  static struct object_array want_obj;
>  static unsigned int timeout;
> @@ -107,8 +108,6 @@ static int do_rev_list(int fd, void *create_full_pack)
>  	struct rev_info revs;
> 
>  	pack_pipe = fdopen(fd, "w");
> -	if (create_full_pack)
> -		use_thin_pack = 0; /* no point doing it */
>  	init_revisions(&revs, NULL);
>  	revs.tag_objects = 1;
>  	revs.tree_objects = 1;
> @@ -155,13 +154,22 @@ static void create_pack_file(void)
>  	const char *argv[10];
>  	int arg = 0;
> 
> -	rev_list.proc = do_rev_list;
> -	/* .data is just a boolean: any non-NULL value will do */
> -	rev_list.data = create_full_pack ? &rev_list : NULL;

I'm glad you got rid of that.

Show 12 quoted lines

> -	if (start_async(&rev_list))
> -		die("git upload-pack: unable to fork git-rev-list");
> +	if (shallow_nr) {
> +		rev_list.proc = do_rev_list;
> +		rev_list.data = 0;
> +		if (start_async(&rev_list))
> +			die("git upload-pack: unable to fork git-rev-list");
> +		argv[arg++] = "pack-objects";
> +	} else {
> +		argv[arg++] = "pack-objects";
> +		argv[arg++] = "--revs";
> +		argv[arg++] = "--include-tag";

Why this unconditional --include-tags here? Isn't it handled already a couple lines down already?

> +		if (create_full_pack)
> +			argv[arg++] = "--all";
> +		if (use_thin_pack)
> +			argv[arg++] = "--thin";

Please turn this "if (use_thin_pack)" into an "else if (use_thin_pack)" instead. No point using --thin for a full pack.

The rest looks fine to me.

Nicolas