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

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

[sam, Nick Edelen]

instead of using the internal revision walker and piping object refs
to pack-objects this patch passes only the revs to pack-objects, which
in turn handles both enumeration and packing.

Signed-off-by: Sam Vilain <sam@vilain.net>
---
  Submitted on behalf of Nick in order to get wider feedback on this.
  This version passes the test suite.

 upload-pack.c |   54 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index edc7861..7eda8fd 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -155,13 +155,27 @@ 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;
-	if (start_async(&rev_list))
-		die("git upload-pack: unable to fork git-rev-list");
-
-	argv[arg++] = "pack-objects";
+	/* sending rev params to pack-objects directly is great, but unfortunately pack-objects 
+	 * has no way of turning off thin pack generation.  this would be a relatively simple 
+	 * addition, but as we also have to deal with shallow grafts and all it's simplest to 
+	 * just resort to piping object refs.
+	 */
+	if (!use_thin_pack) {
+		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;
+		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";
+		if (create_full_pack)
+			argv[arg++] = "--all";
+	}
+	
 	argv[arg++] = "--stdout";
 	if (!no_progress)
 		argv[arg++] = "--progress";
@@ -172,7 +186,7 @@ static void create_pack_file(void)
 	argv[arg++] = NULL;
 
 	memset(&pack_objects, 0, sizeof(pack_objects));
-	pack_objects.in = rev_list.out;	/* start_command closes it */
+	pack_objects.in = !use_thin_pack ? rev_list.out : -1;
 	pack_objects.out = -1;
 	pack_objects.err = -1;
 	pack_objects.git_cmd = 1;
@@ -181,6 +195,28 @@ static void create_pack_file(void)
 	if (start_command(&pack_objects))
 		die("git upload-pack: unable to fork git-pack-objects");
 
+	/* pass on revisions we (don't) want 
+	 * (do we need to check the validity of pack_objects.in?)
+	 */
+	if (use_thin_pack) {
+		FILE *pipe_fd = fdopen(pack_objects.in, "w");
+		if (!create_full_pack) {
+			int i;
+			for (i = 0; i < want_obj.nr; i++) {
+				fprintf(pipe_fd, "%s\n", sha1_to_hex(want_obj.objects[i].item->sha1));
+			}
+			fprintf(pipe_fd, "--not\n");
+			for (i = 0; i < have_obj.nr; i++) {
+				fprintf(pipe_fd, "%s\n", sha1_to_hex(have_obj.objects[i].item->sha1));
+			}
+		}
+		
+		fprintf(pipe_fd, "\n");
+		fflush(pipe_fd);
+		fclose(pipe_fd);
+	}
+
+
 	/* We read from pack_objects.err to capture stderr output for
 	 * progress bar, and pack_objects.out to capture the pack data.
 	 */
@@ -276,7 +312,7 @@ static void create_pack_file(void)
 		error("git upload-pack: git-pack-objects died with error.");
 		goto fail;
 	}
-	if (finish_async(&rev_list))
+	if (!use_thin_pack && finish_async(&rev_list))
 		goto fail;	/* error was already reported */
 
 	/* flush the data */
-- 
tg: (9d7169f..) t/revcache/upload-pack-dont-enumerate (depends on: git-daemon-caching)
tg: The patch is out-of-date wrt. the base! Run `tg update`.

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

[Sam Vilain]

Sorry, stuffed up the headers on this one, too - that should have been
me in the "From:" of the e-mail, but it is Nick's work...

Sam.

Nick Edelen wrote:
> instead of using the internal revision walker and piping object refs
> to pack-objects this patch passes only the revs to pack-objects, which
> in turn handles both enumeration and packing.
>
> Signed-off-by: Sam Vilain <sam@vilain.net>
> ---
>   Submitted on behalf of Nick in order to get wider feedback on this.
>   This version passes the test suite.
>
>  upload-pack.c |   54 +++++++++++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 45 insertions(+), 9 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index edc7861..7eda8fd 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -155,13 +155,27 @@ 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;
> -	if (start_async(&rev_list))
> -		die("git upload-pack: unable to fork git-rev-list");
> -
> -	argv[arg++] = "pack-objects";
> +	/* sending rev params to pack-objects directly is great, but unfortunately pack-objects 
> +	 * has no way of turning off thin pack generation.  this would be a relatively simple 
> +	 * addition, but as we also have to deal with shallow grafts and all it's simplest to 
> +	 * just resort to piping object refs.
> +	 */
> +	if (!use_thin_pack) {
> +		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;
> +		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";
> +		if (create_full_pack)
> +			argv[arg++] = "--all";
> +	}
> +	
>  	argv[arg++] = "--stdout";
>  	if (!no_progress)
>  		argv[arg++] = "--progress";
> @@ -172,7 +186,7 @@ static void create_pack_file(void)
>  	argv[arg++] = NULL;
>  
>  	memset(&pack_objects, 0, sizeof(pack_objects));
> -	pack_objects.in = rev_list.out;	/* start_command closes it */
> +	pack_objects.in = !use_thin_pack ? rev_list.out : -1;
>  	pack_objects.out = -1;
>  	pack_objects.err = -1;
>  	pack_objects.git_cmd = 1;
> @@ -181,6 +195,28 @@ static void create_pack_file(void)
>  	if (start_command(&pack_objects))
>  		die("git upload-pack: unable to fork git-pack-objects");
>  
> +	/* pass on revisions we (don't) want 
> +	 * (do we need to check the validity of pack_objects.in?)
> +	 */
> +	if (use_thin_pack) {
> +		FILE *pipe_fd = fdopen(pack_objects.in, "w");
> +		if (!create_full_pack) {
> +			int i;
> +			for (i = 0; i < want_obj.nr; i++) {
> +				fprintf(pipe_fd, "%s\n", sha1_to_hex(want_obj.objects[i].item->sha1));
> +			}
> +			fprintf(pipe_fd, "--not\n");
> +			for (i = 0; i < have_obj.nr; i++) {
> +				fprintf(pipe_fd, "%s\n", sha1_to_hex(have_obj.objects[i].item->sha1));
> +			}
> +		}
> +		
> +		fprintf(pipe_fd, "\n");
> +		fflush(pipe_fd);
> +		fclose(pipe_fd);
> +	}
> +
> +
>  	/* We read from pack_objects.err to capture stderr output for
>  	 * progress bar, and pack_objects.out to capture the pack data.
>  	 */
> @@ -276,7 +312,7 @@ static void create_pack_file(void)
>  		error("git upload-pack: git-pack-objects died with error.");
>  		goto fail;
>  	}
> -	if (finish_async(&rev_list))
> +	if (!use_thin_pack && finish_async(&rev_list))
>  		goto fail;	/* error was already reported */
>  
>  	/* flush the data */
>   

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

[Johannes Sixt]

sam@vilain.net schrieb:
> instead of using the internal revision walker and piping object refs
> to pack-objects this patch passes only the revs to pack-objects, which
> in turn handles both enumeration and packing.

I appreciate this move. We have one test failing in MinGW git (t5530.6)
because of the rev-list that is run using start_async(). Even though this
patch doesn't change that (the test case still uses the start_async()
path), it is one step closer to the solution.

> +	/* sending rev params to pack-objects directly is great, but unfortunately pack-objects 
> +	 * has no way of turning off thin pack generation.  this would be a relatively simple 
> +	 * addition, but as we also have to deal with shallow grafts and all it's simplest to 
> +	 * just resort to piping object refs.
> +	 */

You certainly will reformat comments like this to shorter lines, proper
capitalization, without trailing spaces, and adjust the style (initial /*
is on its own line)?

> @@ -181,6 +195,28 @@ static void create_pack_file(void)
>  	if (start_command(&pack_objects))
>  		die("git upload-pack: unable to fork git-pack-objects");
>  
> +	/* pass on revisions we (don't) want 
> +	 * (do we need to check the validity of pack_objects.in?)

No, you don't need to check. It's valid, or you would have died above.

FYI, with this patch MinGW git hangs in t5530.8; the test-case exercises
the new code path.

-- Hannes

[PATCH] fetch-pack: close output channel after sideband demultiplexer terminates

[Johannes Sixt]

From: Johannes Sixt <j6t@kdbg.org>

fetch-pack runs the sideband demultiplexer using start_async(). This
facility requires that the asynchronously executed function closes the
output file descriptor (see Documentation/technical/api-run-command.txt).
But the sideband demultiplexer did not do that. This fixes it.

In certain error situations this could lock up a fetch operation on
Windows because the asynchronous function is run in a thread; by not
closing the output fd the reading end never got EOF and waited for more
data indefinitely. On Unix this is not a problem because the asynchronous
function is run in a separate process, which exits after the function ends
and so implicitly closes the output.

Since the pack that is sent over the wire encodes the number of objects in
the stream, during normal operation the reading end knows when the stream
ends and terminates by itself, and does not lock up.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---

 Johannes Sixt schrieb:
 > FYI, with this patch MinGW git hangs in t5530.8; the test-case exercises
 > the new code path.

 This fixes the lockup.

 -- Hannes

 builtin-fetch-pack.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 2eb7a22..b11d799 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -487,7 +487,9 @@ static int sideband_demux(int fd, void *data)
 {
 	int *xd = data;

-	return recv_sideband("fetch-pack", xd[0], fd);
+	int ret = recv_sideband("fetch-pack", xd[0], fd);
+	close(fd);
+	return ret;
 }

 static int get_pack(int xd[2], char **pack_lockfile)
-- 
1.6.3.1.1141.gcf6b0.dirty

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

[Nicolas Pitre]

On Fri, 5 Jun 2009, sam@vilain.net wrote:

> instead of using the internal revision walker and piping object refs
> to pack-objects this patch passes only the revs to pack-objects, which
> in turn handles both enumeration and packing.
> 
> Signed-off-by: Sam Vilain <sam@vilain.net>
> ---
>   Submitted on behalf of Nick in order to get wider feedback on this.
>   This version passes the test suite.
> 
>  upload-pack.c |   54 +++++++++++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 45 insertions(+), 9 deletions(-)
> 
> diff --git a/upload-pack.c b/upload-pack.c
> index edc7861..7eda8fd 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -155,13 +155,27 @@ 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;
> -	if (start_async(&rev_list))
> -		die("git upload-pack: unable to fork git-rev-list");
> -
> -	argv[arg++] = "pack-objects";
> +	/* sending rev params to pack-objects directly is great, but unfortunately pack-objects 
> +	 * has no way of turning off thin pack generation.  this would be a relatively simple 
> +	 * addition, but as we also have to deal with shallow grafts and all it's simplest to 
> +	 * just resort to piping object refs.
> +	 */

What's that?  Where did you get that?

The way to not generate a thin pack is to not specify --thin to 
pack-objects.  If you get a thin pack without specifying --thin then 
this is a bug that needs to be fixed first.

> +	if (!use_thin_pack) {
> +		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;
> +		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";

Shouldn't this be specified only if corresponding capability was 
provided by the client?


Nicolas

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

[Nick Edelen]

hi guys,

I wasn't aware that Sam was going to upload this, otherwise I would've
cleaned it up a bit.  not a problem though; thanks for taking the time
to do this Sam.

@Johannes:
the real reason I'm using start_async at all is because of shallow
commit grafts.  we could potentially add an interface to pack-objects
to allow it to accept those types of commit grafts if you wanted.
like, in addition to the --not flag we could add a --shallow flag or
something...  it'd actually be pretty easy to implement, but I don't
know if you guys'd want that in pack-objects.

@Nicolas:
I'm using the --revs flag in pack-objects, which causes it to use
get_object_list().  you'll notice, regardless of whether --thin is
set, this function still calls
	mark_edges_uninteresting(revs.commits, &revs, show_edge);
which sets uninteresting objects as preferred bases, which I'd think
would create a thin pack.  I could be wrong though...

as I mentioned in the comment and above, it's an easy fix, but even
then I wasn't sure what to do with commit grafts.  as use_thin_pack
seemed to be predominantly set on shallow interactions, I just didn't
bother seperating the cases 'normal but thick pack' and 'shallow
stuff'.

(btw, I have a really cool idea for shallow/narrow/lazy interaction if
you have the time.  it basically uses 'fantom' placeholder objects to
cover for missing blobs, so a clone/fetch would get all commits/trees
but only retrieve blobs when a user specifies.  I'll get a proof of
concept done after this rev-cache project).

thank you both for looking over this though.

 - Nick

On Fri, Jun 5, 2009 at 6:51 PM, Nicolas Pitre<nico@cam.org> wrote:
> On Fri, 5 Jun 2009, sam@vilain.net wrote:
>
>> instead of using the internal revision walker and piping object refs
>> to pack-objects this patch passes only the revs to pack-objects, which
>> in turn handles both enumeration and packing.
>>
>> Signed-off-by: Sam Vilain <sam@vilain.net>
>> ---
>>   Submitted on behalf of Nick in order to get wider feedback on this.
>>   This version passes the test suite.
>>
>>  upload-pack.c |   54 +++++++++++++++++++++++++++++++++++++++++++++---------
>>  1 files changed, 45 insertions(+), 9 deletions(-)
>>
>> diff --git a/upload-pack.c b/upload-pack.c
>> index edc7861..7eda8fd 100644
>> --- a/upload-pack.c
>> +++ b/upload-pack.c
>> @@ -155,13 +155,27 @@ 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;
>> -     if (start_async(&rev_list))
>> -             die("git upload-pack: unable to fork git-rev-list");
>> -
>> -     argv[arg++] = "pack-objects";
>> +     /* sending rev params to pack-objects directly is great, but unfortunately pack-objects
>> +      * has no way of turning off thin pack generation.  this would be a relatively simple
>> +      * addition, but as we also have to deal with shallow grafts and all it's simplest to
>> +      * just resort to piping object refs.
>> +      */
>
> What's that?  Where did you get that?
>
> The way to not generate a thin pack is to not specify --thin to
> pack-objects.  If you get a thin pack without specifying --thin then
> this is a bug that needs to be fixed first.
>
>> +     if (!use_thin_pack) {
>> +             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;
>> +             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";
>
> Shouldn't this be specified only if corresponding capability was
> provided by the client?
>
>
> Nicolas
>

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

[Nick Edelen]

by the way, since I'm on the list now anyway:

is there any reason why you guys are calling
mark_parents_uninteresting() after add_parents_to_list() in
limit_list() (revision.c), considering the latter does everything the
former does?  sorta trivial, but it's been bugging me...

On Sun, Jun 7, 2009 at 3:25 PM, Nick Edelen<sirnot@gmail.com> wrote:
> hi guys,
>
> I wasn't aware that Sam was going to upload this, otherwise I would've
> cleaned it up a bit.  not a problem though; thanks for taking the time
> to do this Sam.
>
> @Johannes:
> the real reason I'm using start_async at all is because of shallow
> commit grafts.  we could potentially add an interface to pack-objects
> to allow it to accept those types of commit grafts if you wanted.
> like, in addition to the --not flag we could add a --shallow flag or
> something...  it'd actually be pretty easy to implement, but I don't
> know if you guys'd want that in pack-objects.
>
> @Nicolas:
> I'm using the --revs flag in pack-objects, which causes it to use
> get_object_list().  you'll notice, regardless of whether --thin is
> set, this function still calls
>        mark_edges_uninteresting(revs.commits, &revs, show_edge);
> which sets uninteresting objects as preferred bases, which I'd think
> would create a thin pack.  I could be wrong though...
>
> as I mentioned in the comment and above, it's an easy fix, but even
> then I wasn't sure what to do with commit grafts.  as use_thin_pack
> seemed to be predominantly set on shallow interactions, I just didn't
> bother seperating the cases 'normal but thick pack' and 'shallow
> stuff'.
>
> (btw, I have a really cool idea for shallow/narrow/lazy interaction if
> you have the time.  it basically uses 'fantom' placeholder objects to
> cover for missing blobs, so a clone/fetch would get all commits/trees
> but only retrieve blobs when a user specifies.  I'll get a proof of
> concept done after this rev-cache project).
>
> thank you both for looking over this though.
>
>  - Nick
>
> On Fri, Jun 5, 2009 at 6:51 PM, Nicolas Pitre<nico@cam.org> wrote:
>> On Fri, 5 Jun 2009, sam@vilain.net wrote:
>>
>>> instead of using the internal revision walker and piping object refs
>>> to pack-objects this patch passes only the revs to pack-objects, which
>>> in turn handles both enumeration and packing.
>>>
>>> Signed-off-by: Sam Vilain <sam@vilain.net>
>>> ---
>>>   Submitted on behalf of Nick in order to get wider feedback on this.
>>>   This version passes the test suite.
>>>
>>>  upload-pack.c |   54 +++++++++++++++++++++++++++++++++++++++++++++---------
>>>  1 files changed, 45 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/upload-pack.c b/upload-pack.c
>>> index edc7861..7eda8fd 100644
>>> --- a/upload-pack.c
>>> +++ b/upload-pack.c
>>> @@ -155,13 +155,27 @@ 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;
>>> -     if (start_async(&rev_list))
>>> -             die("git upload-pack: unable to fork git-rev-list");
>>> -
>>> -     argv[arg++] = "pack-objects";
>>> +     /* sending rev params to pack-objects directly is great, but unfortunately pack-objects
>>> +      * has no way of turning off thin pack generation.  this would be a relatively simple
>>> +      * addition, but as we also have to deal with shallow grafts and all it's simplest to
>>> +      * just resort to piping object refs.
>>> +      */
>>
>> What's that?  Where did you get that?
>>
>> The way to not generate a thin pack is to not specify --thin to
>> pack-objects.  If you get a thin pack without specifying --thin then
>> this is a bug that needs to be fixed first.
>>
>>> +     if (!use_thin_pack) {
>>> +             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;
>>> +             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";
>>
>> Shouldn't this be specified only if corresponding capability was
>> provided by the client?
>>
>>
>> Nicolas
>>
>

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

[Nicolas Pitre]

On Sun, 7 Jun 2009, Nick Edelen wrote:

> I'm using the --revs flag in pack-objects, which causes it to use
> get_object_list().  you'll notice, regardless of whether --thin is
> set, this function still calls
> 	mark_edges_uninteresting(revs.commits, &revs, show_edge);
> which sets uninteresting objects as preferred bases, which I'd think
> would create a thin pack.  I could be wrong though...

Look at the arguments passed to setup_revisions().
When --thin is set, the --objects-edge flag is passed instead of 
--objects.  Now see what effect this has on the third argument of 
mark_edges_uninteresting().

> as I mentioned in the comment and above, it's an easy fix, but even 
> then I wasn't sure what to do with commit grafts.  as use_thin_pack 
> seemed to be predominantly set on shallow interactions, I just didn't 
> bother seperating the cases 'normal but thick pack' and 'shallow 
> stuff'.

Please do separate them.  In theory you could use thin packs with a 
relative deepening of a shallow clone.  In other words, !thin and 
shallow is a wrong association to make.


Nicolas

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

[Nick Edelen]

crap, your right.  somehow I managed to miss that...

I'll go ahead and seperate them then...

On Sun, Jun 7, 2009 at 6:41 PM, Nicolas Pitre<nico@cam.org> wrote:
> On Sun, 7 Jun 2009, Nick Edelen wrote:
>
>> I'm using the --revs flag in pack-objects, which causes it to use
>> get_object_list().  you'll notice, regardless of whether --thin is
>> set, this function still calls
>>       mark_edges_uninteresting(revs.commits, &revs, show_edge);
>> which sets uninteresting objects as preferred bases, which I'd think
>> would create a thin pack.  I could be wrong though...
>
> Look at the arguments passed to setup_revisions().
> When --thin is set, the --objects-edge flag is passed instead of
> --objects.  Now see what effect this has on the third argument of
> mark_edges_uninteresting().
>
>> as I mentioned in the comment and above, it's an easy fix, but even
>> then I wasn't sure what to do with commit grafts.  as use_thin_pack
>> seemed to be predominantly set on shallow interactions, I just didn't
>> bother seperating the cases 'normal but thick pack' and 'shallow
>> stuff'.
>
> Please do separate them.  In theory you could use thin packs with a
> relative deepening of a shallow clone.  In other words, !thin and
> shallow is a wrong association to make.
>
>
> Nicolas
>

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

[Nick Edelen]

how does this look?

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
 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;
-	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";
+		if (create_full_pack)
+			argv[arg++] = "--all";
+		if (use_thin_pack)
+			argv[arg++] = "--thin";
+	}

-	argv[arg++] = "pack-objects";
 	argv[arg++] = "--stdout";
 	if (!no_progress)
 		argv[arg++] = "--progress";
@@ -172,7 +180,7 @@ static void create_pack_file(void)
 	argv[arg++] = NULL;

 	memset(&pack_objects, 0, sizeof(pack_objects));
-	pack_objects.in = rev_list.out;	/* start_command closes it */
+	pack_objects.in = shallow_nr ? rev_list.out : -1;
 	pack_objects.out = -1;
 	pack_objects.err = -1;
 	pack_objects.git_cmd = 1;
@@ -181,6 +189,24 @@ static void create_pack_file(void)
 	if (start_command(&pack_objects))
 		die("git upload-pack: unable to fork git-pack-objects");

+	/* pass on revisions we (don't) want */
+	if (!shallow_nr) {
+		FILE *pipe_fd = fdopen(pack_objects.in, "w");
+		if (!create_full_pack) {
+			int i;
+			for (i = 0; i < want_obj.nr; i++)
+				fprintf(pipe_fd, "%s\n", sha1_to_hex(want_obj.objects[i].item->sha1));
+			fprintf(pipe_fd, "--not\n");
+			for (i = 0; i < have_obj.nr; i++)
+				fprintf(pipe_fd, "%s\n", sha1_to_hex(have_obj.objects[i].item->sha1));
+		}
+
+		fprintf(pipe_fd, "\n");
+		fflush(pipe_fd);
+		fclose(pipe_fd);
+	}
+
+
 	/* We read from pack_objects.err to capture stderr output for
 	 * progress bar, and pack_objects.out to capture the pack data.
 	 */
@@ -276,7 +302,7 @@ static void create_pack_file(void)
 		error("git upload-pack: git-pack-objects died with error.");
 		goto fail;
 	}
-	if (finish_async(&rev_list))
+	if (shallow_nr && finish_async(&rev_list))
 		goto fail;	/* error was already reported */

 	/* flush the data */
@@ -451,6 +477,7 @@ static void receive_needs(void)
 	static char line[1000];
 	int len, depth = 0;

+	shallow_nr = 0;
 	if (debug_fd)
 		write_in_full(debug_fd, "#S\n", 3);
 	for (;;) {
@@ -534,6 +561,7 @@ static void receive_needs(void)
 				packet_write(1, "shallow %s",
 						sha1_to_hex(object->sha1));
 				register_shallow(object->sha1);
+				shallow_nr++;
 			}
 			result = result->next;
 		}
@@ -567,6 +595,8 @@ static void receive_needs(void)
 			for (i = 0; i < shallows.nr; i++)
 				register_shallow(shallows.objects[i].item->sha1);
 		}
+
+	shallow_nr += shallows.nr;
 	free(shallows.objects);
 }

-- 
tg: (503f464..) t/revcache/upload-pack-dont-enumerate (depends on: master)


On Sun, Jun 7, 2009 at 6:47 PM, Nick Edelen<sirnot@gmail.com> wrote:
> crap, your right.  somehow I managed to miss that...
>
> I'll go ahead and seperate them then...
>
> On Sun, Jun 7, 2009 at 6:41 PM, Nicolas Pitre<nico@cam.org> wrote:
>> On Sun, 7 Jun 2009, Nick Edelen wrote:
>>
>>> I'm using the --revs flag in pack-objects, which causes it to use
>>> get_object_list().  you'll notice, regardless of whether --thin is
>>> set, this function still calls
>>>       mark_edges_uninteresting(revs.commits, &revs, show_edge);
>>> which sets uninteresting objects as preferred bases, which I'd think
>>> would create a thin pack.  I could be wrong though...
>>
>> Look at the arguments passed to setup_revisions().
>> When --thin is set, the --objects-edge flag is passed instead of
>> --objects.  Now see what effect this has on the third argument of
>> mark_edges_uninteresting().
>>
>>> as I mentioned in the comment and above, it's an easy fix, but even
>>> then I wasn't sure what to do with commit grafts.  as use_thin_pack
>>> seemed to be predominantly set on shallow interactions, I just didn't
>>> bother seperating the cases 'normal but thick pack' and 'shallow
>>> stuff'.
>>
>> Please do separate them.  In theory you could use thin packs with a
>> relative deepening of a shallow clone.  In other words, !thin and
>> shallow is a wrong association to make.
>>
>>
>> Nicolas
>>
>

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

[Sam Vilain]

On Sun, 2009-06-07 at 20:55 +0200, Nick Edelen wrote:
> how does this look?
> 
> Signed-off-by: Nick Edelen <sirnot@gmail.com>
> 
> ---

Comments like that belong after the '---'; the change description is
part of the work that you are developing, too.

Sam.

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

[Nicolas Pitre]

On Sun, 7 Jun 2009, Nick Edelen wrote:

> how does this look?

Comments below.

> 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?

>  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.

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

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

[Nick Edelen]

man I can sure be blind at times...  alright fixed the latter two
comments.  I changed the test file because that particular test wanted
upload-pack to fail through the revision walker, which it could only
now do if shallow objects were involved.

@sam: oops, sorry about that.

From: Nick Edelen <sirnot@gmail.com>
Subject: [PATCH] Shift object enumeration out of upload-pack

Offload object enumeration in upload-pack to pack-objects, but fall
back on internal revision walker for shallow interaction.  Test t5530
updated to reflect mechanism change.

Signed-off-by: Nick Edelen <sirnot@gmail.com>

---
 t/t5530-upload-pack-error.sh |    2 +-
 upload-pack.c                |   49 +++++++++++++++++++++++++++++++++--------
 2 files changed, 40 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
 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..397cada 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,21 @@ 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;
-	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";
+		if (create_full_pack)
+			argv[arg++] = "--all";
+		else if (use_thin_pack)
+			argv[arg++] = "--thin";
+	}

-	argv[arg++] = "pack-objects";
 	argv[arg++] = "--stdout";
 	if (!no_progress)
 		argv[arg++] = "--progress";
@@ -172,7 +179,7 @@ static void create_pack_file(void)
 	argv[arg++] = NULL;

 	memset(&pack_objects, 0, sizeof(pack_objects));
-	pack_objects.in = rev_list.out;	/* start_command closes it */
+	pack_objects.in = shallow_nr ? rev_list.out : -1;
 	pack_objects.out = -1;
 	pack_objects.err = -1;
 	pack_objects.git_cmd = 1;
@@ -181,6 +188,24 @@ static void create_pack_file(void)
 	if (start_command(&pack_objects))
 		die("git upload-pack: unable to fork git-pack-objects");

+	/* pass on revisions we (don't) want */
+	if (!shallow_nr) {
+		FILE *pipe_fd = fdopen(pack_objects.in, "w");
+		if (!create_full_pack) {
+			int i;
+			for (i = 0; i < want_obj.nr; i++)
+				fprintf(pipe_fd, "%s\n", sha1_to_hex(want_obj.objects[i].item->sha1));
+			fprintf(pipe_fd, "--not\n");
+			for (i = 0; i < have_obj.nr; i++)
+				fprintf(pipe_fd, "%s\n", sha1_to_hex(have_obj.objects[i].item->sha1));
+		}
+
+		fprintf(pipe_fd, "\n");
+		fflush(pipe_fd);
+		fclose(pipe_fd);
+	}
+
+
 	/* We read from pack_objects.err to capture stderr output for
 	 * progress bar, and pack_objects.out to capture the pack data.
 	 */
@@ -276,7 +301,7 @@ static void create_pack_file(void)
 		error("git upload-pack: git-pack-objects died with error.");
 		goto fail;
 	}
-	if (finish_async(&rev_list))
+	if (shallow_nr && finish_async(&rev_list))
 		goto fail;	/* error was already reported */

 	/* flush the data */
@@ -451,6 +476,7 @@ static void receive_needs(void)
 	static char line[1000];
 	int len, depth = 0;

+	shallow_nr = 0;
 	if (debug_fd)
 		write_in_full(debug_fd, "#S\n", 3);
 	for (;;) {
@@ -534,6 +560,7 @@ static void receive_needs(void)
 				packet_write(1, "shallow %s",
 						sha1_to_hex(object->sha1));
 				register_shallow(object->sha1);
+				shallow_nr++;
 			}
 			result = result->next;
 		}
@@ -567,6 +594,8 @@ static void receive_needs(void)
 			for (i = 0; i < shallows.nr; i++)
 				register_shallow(shallows.objects[i].item->sha1);
 		}
+
+	shallow_nr += shallows.nr;
 	free(shallows.objects);
 }

-- 
tg: (503f464..) t/revcache/upload-pack-dont-enumerate (depends on: master)


On Sun, Jun 7, 2009 at 10:48 PM, Nicolas Pitre<nico@cam.org> wrote:
> On Sun, 7 Jun 2009, Nick Edelen wrote:
>
>> how does this look?
>
> Comments below.
>
>> 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?
>
>>  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.
>
>> -     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
>

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

[Nicolas Pitre]

On Mon, 8 Jun 2009, Nick Edelen wrote:

> man I can sure be blind at times...  alright fixed the latter two
> comments.  I changed the test file because that particular test wanted
> upload-pack to fail through the revision walker, which it could only
> now do if shallow objects were involved.

OK.  However, since now we have 2 different paths for creating object 
list instead of only one (the existing one in upload-pack and the one in 
pack-objects), I'd prefer if you could add another test case to make 
sure the pack-objects path also fails appropriately.

The rest looks fine to me, and with the test issue fixed you could add 
my:

  Acked-by: Nicolas Pitre <nico@cam.org

to your next submission.  Oh and you might change the subject prefix to 
[PATCH] as well since I think this won't be WIP anymore.


Nicolas

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

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

> On Mon, 8 Jun 2009, Nick Edelen wrote:
>
>> man I can sure be blind at times...  alright fixed the latter two
>> comments.  I changed the test file because that particular test wanted
>> upload-pack to fail through the revision walker, which it could only
>> now do if shallow objects were involved.
>
> OK.  However, since now we have 2 different paths for creating object 
> list instead of only one (the existing one in upload-pack and the one in 
> pack-objects), I'd prefer if you could add another test case to make 
> sure the pack-objects path also fails appropriately.
>
> The rest looks fine to me, and with the test issue fixed you could add 
> my:
>
>   Acked-by: Nicolas Pitre <nico@cam.org
>
> to your next submission.  Oh and you might change the subject prefix to 
> [PATCH] as well since I think this won't be WIP anymore.

True and true.  Thanks.