git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
NENick Edelen <sirnot@gmail.com>
Date
Jun 7, 2009, 22:04 UTC
Message-ID
<c77435a80906071504u16a46e03w6dfde9a3e694a5ec@mail.gmail.com>
In-Reply-To
<alpine.LFD.2.00.0906071637530.3906@xanadu.home>

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
>
Previous: Nicolas PitreNext: Nicolas Pitre
Message 13 of 15 in “[WIP] Shift rev-list enumeration from upload-pack to pack-objects”
  1. sam@vilain.netJun 5, 2009
  2. Sam VilainJun 5, 2009
  3. Johannes SixtJun 5, 2009
  4. fetch-pack: close output channel after sideband demultiplexer terminatesJohannes Sixt, Jun 8, 2009
  5. Nicolas PitreJun 5, 2009
  6. Nick EdelenJun 7, 2009
  7. Nick EdelenJun 7, 2009
  8. Nicolas PitreJun 7, 2009
  9. Nick EdelenJun 7, 2009
  10. Nick EdelenJun 7, 2009
  11. Sam VilainJun 7, 2009
  12. Nicolas PitreJun 7, 2009
  13. Nick EdelenJun 7, 2009
  14. Nicolas PitreJun 8, 2009
  15. Junio C HamanoJun 8, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.