# [WIP] Shift rev-list enumeration from upload-pack to pack-objects 15 messages from 2009-06-05 to 2009-06-08. Participants: sam@vilain.net, Sam Vilain, Johannes Sixt, Nicolas Pitre, Nick Edelen, Junio C Hamano. Thread: https://gitlist.dev/t/19672 ## sam@vilain.net, 2009-06-05 05:45 Subject: [WIP] Shift rev-list enumeration from upload-pack to pack-objects Message-ID: <20090605054500.06A9D21C3F4@mail.utsl.gen.nz> URL: https://gitlist.dev/e/20090605054500.06A9D21C3F4%40mail.utsl.gen.nz ``` 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 --- 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`. ``` ## Sam Vilain, 2009-06-05 05:46 Subject: Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects Message-ID: <4A28B128.6040709@vilain.net> URL: https://gitlist.dev/e/4A28B128.6040709%40vilain.net In-Reply-To: <20090605054500.06A9D21C3F4@mail.utsl.gen.nz> ``` 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 > --- > 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 */ > ``` ## Johannes Sixt, 2009-06-05 08:10 Subject: Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects Message-ID: <4A28D2EF.8040704@viscovery.net> URL: https://gitlist.dev/e/4A28D2EF.8040704%40viscovery.net In-Reply-To: <20090605054500.06A9D21C3F4@mail.utsl.gen.nz> ``` 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 ``` ## Nicolas Pitre, 2009-06-05 16:51 Subject: Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.0906051224510.3906%40xanadu.home In-Reply-To: <20090605054500.06A9D21C3F4@mail.utsl.gen.nz> ``` 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 > --- > 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 ``` ## Nick Edelen, 2009-06-07 13:25 Subject: Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects Message-ID: URL: https://gitlist.dev/e/c77435a80906070625i4daaa69bi134df765d7a77cdf%40mail.gmail.com In-Reply-To: ``` 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 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 >> --- >>   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 > ``` ## Nick Edelen, 2009-06-07 13:31 Subject: Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects Message-ID: URL: https://gitlist.dev/e/c77435a80906070631x5c030b9cve4c8963c0d33e71b%40mail.gmail.com In-Reply-To: ``` 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 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 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 >>> --- >>>   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 >> > ``` ## Nicolas Pitre, 2009-06-07 16:41 Subject: Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.0906071225060.3906%40xanadu.home In-Reply-To: ``` 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 ``` ## Nick Edelen, 2009-06-07 16:47 Subject: Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects Message-ID: URL: https://gitlist.dev/e/c77435a80906070947u9bf8ce9m9d59f86e5a5f18ab%40mail.gmail.com In-Reply-To: ``` 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 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 > ``` ## Nick Edelen, 2009-06-07 18:55 Subject: Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects Message-ID: URL: https://gitlist.dev/e/c77435a80906071155g5530ccdel286907b7c6022838%40mail.gmail.com In-Reply-To: ``` how does this look? Signed-off-by: Nick Edelen --- 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 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 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 >> > ``` ## Sam Vilain, 2009-06-07 20:48 Subject: Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects Message-ID: <1244407713.5870.7.camel@maia.lan> URL: https://gitlist.dev/e/1244407713.5870.7.camel%40maia.lan In-Reply-To: ``` On Sun, 2009-06-07 at 20:55 +0200, Nick Edelen wrote: > how does this look? > > Signed-off-by: Nick Edelen > > --- Comments like that belong after the '---'; the change description is part of the work that you are developing, too. Sam. ``` ## Nicolas Pitre, 2009-06-07 20:48 Subject: Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.0906071637530.3906%40xanadu.home In-Reply-To: ``` On Sun, 7 Jun 2009, Nick Edelen wrote: > how does this look? Comments below. > Signed-off-by: Nick Edelen > > --- > 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 ``` ## Nick Edelen, 2009-06-07 22:04 Subject: Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects Message-ID: URL: https://gitlist.dev/e/c77435a80906071504u16a46e03w6dfde9a3e694a5ec%40mail.gmail.com In-Reply-To: ``` 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 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 --- 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 wrote: > On Sun, 7 Jun 2009, Nick Edelen wrote: > >> how does this look? > > Comments below. > >> Signed-off-by: Nick Edelen >> >> --- >>  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 > ``` ## Nicolas Pitre, 2009-06-08 00:50 Subject: Re: [WIP] Shift rev-list enumeration from upload-pack to pack-objects Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.0906072043400.3906%40xanadu.home In-Reply-To: ``` 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 URL: https://gitlist.dev/e/7v1vpva2a0.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Nicolas Pitre 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 > 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. ``` ## Johannes Sixt, 2009-06-08 08:51 Subject: [PATCH] fetch-pack: close output channel after sideband demultiplexer terminates Message-ID: <4A2CD10A.3000703@viscovery.net> URL: https://gitlist.dev/e/4A2CD10A.3000703%40viscovery.net In-Reply-To: <4A28D2EF.8040704@viscovery.net> ``` From: Johannes Sixt 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 --- 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 ```