| Message ID | 20190826214737.164132-1-jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fetch-pack: write fetched refs to .promisor | expand |
Jonathan Tan <jonathantanmy@google.com> writes: > As written in the NEEDSWORK comment, repack does not preserve the > contents of .promisor files, but I thought I'd send this out anyway as > this change is already useful for users who don't run repack much. What do you exactly mean by "much" here? The comment sounds like it is saying "running this code once and you'd make the commits and objects that were depending on the existing promisor invalid", in which case it would be more like "it is already useful for users until they run their first repack that destroyes their repository", but certainly that is not what we want to do, so... > +test_expect_success 'verify that .promisor file contains refs fetched' ' > + ls pc1/.git/objects/pack/pack-*.promisor >promisorlist && > + test_line_count = 1 promisorlist && > + git -C srv.bare rev-list HEAD >headhash && > + grep "$(cat headhash) HEAD" $(cat promisorlist) && > + grep "$(cat headhash) refs/heads/master" $(cat promisorlist) > +' > + > # checkout master to force dynamic object fetch of blobs at HEAD. > test_expect_success 'verify checkout with dynamic object fetch' ' > git -C pc1 rev-list --quiet --objects --missing=print HEAD >observed &&
> Jonathan Tan <jonathantanmy@google.com> writes: > > > As written in the NEEDSWORK comment, repack does not preserve the > > contents of .promisor files, but I thought I'd send this out anyway as > > this change is already useful for users who don't run repack much. > > What do you exactly mean by "much" here? For diagnostic information to be preserved, the user must not have run repack between the fetch and the discovery of a problem. Admittedly, this is probablistic, but if the user never GCs (for example), this would work. > The comment sounds like it > is saying "running this code once and you'd make the commits and > objects that were depending on the existing promisor invalid", in > which case it would be more like "it is already useful for users > until they run their first repack that destroyes their repository", > but certainly that is not what we want to do, so... To be clear, repacks will not destroy their repository, whether before or after this change. Before and after this change, a repack will just collect all promisor objects from all promisor packs (that is, the ones with .promisor) into one single pack, and then generate an empty .promisor file to indicate that the new single pack is a promisor pack. The difference is that before this change, Git does not write anything into the .promisor file (at least for fetches), so nothing is lost. With this change, we now write something for fetches, so something is lost (since we delete all the old packs, including the .promisor files). But the only thing lost is diagnostic information (for humans - to diagnose, the user will need to open the .promisor file in a text editor) - commits/objects are still valid, and the repository is not destroyed.
On Mon, Aug 26, 2019 at 02:47:37PM -0700, Jonathan Tan wrote: > The specification of promisor packfiles (in partial-clone.txt) states > that the .promisor files that accompany packfiles do not matter (just > like .keep files), so whenever a packfile is fetched from the promisor > remote, Git has been writing empty .promisor files. But these files > could contain more useful information. > > So instead of writing empty files, write the refs fetched to these > files. This makes it easier to debug issues with partial clones, as we > can identify what refs (and their associated hashes) were fetched at the > time the packfile was downloaded, and if necessary, compare those hashes > against what the promisor remote reports now. I'm not really opposed to what you're doing here, but I did recently think of another possible use for .promisor files. So it seems like a good time to bring it up, since presumably we'd have to choose one or the other. I noticed when playing with partial clones that the client may sometimes pause for a while, chewing CPU. The culprit is is_promisor_object(), which generates the list of known promisor objects by opening every object we _do_ have to find out which ones they mention. I know one of the original design features of the promisor pack was that the client would _not_ keep a list of all of the objects it didn't have. But I wonder if it would make sense to keep a cache of these "cut points" in the partial clone. That's potentially smaller than the complete set of objects (especially for tree-based partial cloning), and it seems clear we're willing to store it in memory anyway. And if we do that, would the .promisor file for a pack be a good place to store it? -Peff
Jeff King <peff@peff.net> writes: > But I wonder if it would make sense to keep a cache of these "cut > points" in the partial clone. That's potentially smaller than the > complete set of objects (especially for tree-based partial cloning), and > it seems clear we're willing to store it in memory anyway. That sounds in line with how "shallow" gives us cut points in the history, but then would we end up listing a handful of cut-point objects for each and every commit in the history? That still may be a lot cheaper than computing the same set of cut-point objects every time, though. > And if we do that, would the .promisor file for a pack be a good place > to store it? > > -Peff
On Thu, Sep 05, 2019 at 10:13:24AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > But I wonder if it would make sense to keep a cache of these "cut > > points" in the partial clone. That's potentially smaller than the > > complete set of objects (especially for tree-based partial cloning), and > > it seems clear we're willing to store it in memory anyway. > > That sounds in line with how "shallow" gives us cut points in the > history, but then would we end up listing a handful of cut-point > objects for each and every commit in the history? That still may be > a lot cheaper than computing the same set of cut-point objects every > time, though. I think it would be one set of cut points for the whole pack. There would potentially be a lot of overlap between commits (e.g., if you omit blob X, then every commit after it was added, even if it doesn't touch it, mentions X in its tree). It would also make sense to omit objects from the list that are actually in the pack (because we can trivially already know they're promisor objects by finding them in the pack's .idx). Which means that in the case of blob filters, the set of cut points is identical to the set of omitted objects (because blobs can't reference other objects). For a sparse-path filter that omits a whole tree like "!/foo", we'd end up with a list of all of the oids in history that were ever at the "foo" entry, but nothing below that. -Peff
> I'm not really opposed to what you're doing here, but I did recently > think of another possible use for .promisor files. So it seems like a > good time to bring it up, since presumably we'd have to choose one or > the other. Thanks for bringing it up - yes, we should discuss this. > I noticed when playing with partial clones that the client may sometimes > pause for a while, chewing CPU. The culprit is is_promisor_object(), > which generates the list of known promisor objects by opening every > object we _do_ have to find out which ones they mention. > > I know one of the original design features of the promisor pack was that > the client would _not_ keep a list of all of the objects it didn't have. > But I wonder if it would make sense to keep a cache of these "cut > points" in the partial clone. That's potentially smaller than the > complete set of objects (especially for tree-based partial cloning), and > it seems clear we're willing to store it in memory anyway. Well, before the current design was implemented, I had a design that had such a list of missing objects. :-) I couldn't find a writeup, but here is some preliminary code [1]. In that code, as far as I can tell, the server gives us the list directly during fetch and the client merges it with a repository-wide file called $GIT_DIR/objects/promisedblob, but we don't have to follow the design (we could lazily generate the file, have per-packfile promisedblob files, etc.). [1] https://public-inbox.org/git/cover.1499800530.git.jonathantanmy@google.com/ > And if we do that, would the .promisor file for a pack be a good place > to store it? After looking at [1], it might be better in another place. If we want to preserve fast fetches, we still need another file to indicate that the pack is a promisor, so ".promisor" seems good for that. The presence or absence of the cutoff points is a separate issue and could go into a separate file, and it might be worth putting all cutoff points into a single per-repository file too.
On Thu, Sep 05, 2019 at 11:39:26AM -0700, Jonathan Tan wrote: > > I'm not really opposed to what you're doing here, but I did recently > > think of another possible use for .promisor files. So it seems like a > > good time to bring it up, since presumably we'd have to choose one or > > the other. > > Thanks for bringing it up - yes, we should discuss this. Sorry for starting a discussion and then abandoning it. :) > > I know one of the original design features of the promisor pack was that > > the client would _not_ keep a list of all of the objects it didn't have. > > But I wonder if it would make sense to keep a cache of these "cut > > points" in the partial clone. That's potentially smaller than the > > complete set of objects (especially for tree-based partial cloning), and > > it seems clear we're willing to store it in memory anyway. > > Well, before the current design was implemented, I had a design that had > such a list of missing objects. :-) I couldn't find a writeup, but here > is some preliminary code [1]. In that code, as far as I can tell, the > server gives us the list directly during fetch and the client merges it > with a repository-wide file called $GIT_DIR/objects/promisedblob, but we > don't have to follow the design (we could lazily generate the file, have > per-packfile promisedblob files, etc.). > > [1] https://public-inbox.org/git/cover.1499800530.git.jonathantanmy@google.com/ This was also a feature of my very old "external odb" patches. In fact, there the server told the client the type and size, which let us easily know that many objects weren't worth fetching (e.g., a large blob would be marked as binary and skipped for diffing, without the diff code even having to care about "is it a promisor object that we don't have?"). For just omitting some blobs, I think that carrying extra information (either just the set of blobs, or even some basic meta-information) is valuable. But for "narrow" or "cone" clones, the current system of implied promisors is pretty nice, because then the client effort literally does (or could) scale with the size of their cone, independent of the number of objects outside their cone. > > And if we do that, would the .promisor file for a pack be a good place > > to store it? > > After looking at [1], it might be better in another place. If we want to > preserve fast fetches, we still need another file to indicate that the > pack is a promisor, so ".promisor" seems good for that. The presence or > absence of the cutoff points is a separate issue and could go into a > separate file, and it might be worth putting all cutoff points into a > single per-repository file too. OK, that makes sense. Thanks for giving it some thought. -Peff
I have a few questions below, but they're probably due to lack of a full understanding on my part of how packfiles are managed. On 2019.08.26 14:47, Jonathan Tan wrote: > The specification of promisor packfiles (in partial-clone.txt) states > that the .promisor files that accompany packfiles do not matter (just > like .keep files), so whenever a packfile is fetched from the promisor > remote, Git has been writing empty .promisor files. But these files > could contain more useful information. > > So instead of writing empty files, write the refs fetched to these > files. This makes it easier to debug issues with partial clones, as we > can identify what refs (and their associated hashes) were fetched at the > time the packfile was downloaded, and if necessary, compare those hashes > against what the promisor remote reports now. > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > As written in the NEEDSWORK comment, repack does not preserve the > contents of .promisor files, but I thought I'd send this out anyway as > this change is already useful for users who don't run repack much. > --- > builtin/repack.c | 5 +++++ > fetch-pack.c | 41 ++++++++++++++++++++++++++++++++++++---- > t/t5616-partial-clone.sh | 8 ++++++++ > 3 files changed, 50 insertions(+), 4 deletions(-) > > diff --git a/builtin/repack.c b/builtin/repack.c > index 632c0c0a79..8c1621d414 100644 > --- a/builtin/repack.c > +++ b/builtin/repack.c > @@ -232,6 +232,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args, > /* > * pack-objects creates the .pack and .idx files, but not the > * .promisor file. Create the .promisor file, which is empty. > + * > + * NEEDSWORK: fetch-pack generates non-empty .promisor files, > + * but this would not preserve their contents. Maybe > + * concatenate the contents of all .promisor files instead of > + * just creating a new empty file. > */ > promisor_name = mkpathdup("%s-%s.promisor", packtmp, > line.buf); Since this is just diagnostic information, it seems fine. Maybe explicitly note in the comment what information is being lost? > diff --git a/fetch-pack.c b/fetch-pack.c > index 65be043f2a..07029e1bbf 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -758,8 +758,33 @@ static int sideband_demux(int in, int out, void *data) > return ret; > } > > +static void write_promisor_file(const char *keep_name, > + struct ref **sought, int nr_sought) > +{ > + struct strbuf promisor_name = STRBUF_INIT; > + int suffix_stripped; > + FILE *output; > + int i; > + > + strbuf_addstr(&promisor_name, keep_name); > + suffix_stripped = strbuf_strip_suffix(&promisor_name, ".keep"); > + if (!suffix_stripped) > + BUG("name of pack lockfile should end with .keep (was '%s')", > + keep_name); > + strbuf_addstr(&promisor_name, ".promisor"); > + > + output = xfopen(promisor_name.buf, "w"); > + for (i = 0; i < nr_sought; i++) > + fprintf(output, "%s %s\n", oid_to_hex(&sought[i]->old_oid), > + sought[i]->name); > + fclose(output); > + > + strbuf_release(&promisor_name); > +} > + I am not sure why we want to tie creating the .promisor to creating the lockfile. I'll keep reading and see if it becomes clear later. Other than that, the logic here seems clear. > static int get_pack(struct fetch_pack_args *args, > - int xd[2], char **pack_lockfile) > + int xd[2], char **pack_lockfile, > + struct ref **sought, int nr_sought) > { > struct async demux; > int do_keep = args->keep_pack; > @@ -821,7 +846,13 @@ static int get_pack(struct fetch_pack_args *args, > } > if (args->check_self_contained_and_connected) > argv_array_push(&cmd.args, "--check-self-contained-and-connected"); > - if (args->from_promisor) > + /* > + * If we're obtaining the filename of a lockfile, we'll use > + * that filename to write a .promisor file with more > + * information below. If not, we need index-pack to do it for > + * us. > + */ > + if (!(do_keep && pack_lockfile) && args->from_promisor) > argv_array_push(&cmd.args, "--promisor"); > } > else { This makes me wonder why we don't also change index-pack to write a similar message to the .promisor. I guess there's potentially too much information to shove all the refs on the command-line? > @@ -859,6 +890,8 @@ static int get_pack(struct fetch_pack_args *args, > die(_("fetch-pack: unable to fork off %s"), cmd_name); > if (do_keep && pack_lockfile) { > *pack_lockfile = index_pack_lockfile(cmd.out); > + if (args->from_promisor) > + write_promisor_file(*pack_lockfile, sought, nr_sought); > close(cmd.out); > } > Apart from using the lockfile name as the base for the .promisor filename, I'm still not seeing why we need to tie this to the fact that we're creating a lockfile. Could we instead just unconditionally create the .promisor when args->from_promisor is set, and then remove the logic in the previous chunk that adds the "--promisor" flag to the index-pack call? > @@ -1009,7 +1042,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args, > alternate_shallow_file = setup_temporary_shallow(si->shallow); > else > alternate_shallow_file = NULL; > - if (get_pack(args, fd, pack_lockfile)) > + if (get_pack(args, fd, pack_lockfile, sought, nr_sought)) > die(_("git fetch-pack: fetch failed.")); > > all_done: > @@ -1458,7 +1491,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, > > /* get the pack */ > process_section_header(&reader, "packfile", 0); > - if (get_pack(args, fd, pack_lockfile)) > + if (get_pack(args, fd, pack_lockfile, sought, nr_sought)) > die(_("git fetch-pack: fetch failed.")); > > state = FETCH_DONE; > diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh > index 565254558f..486db27ee0 100755 > --- a/t/t5616-partial-clone.sh > +++ b/t/t5616-partial-clone.sh > @@ -46,6 +46,14 @@ test_expect_success 'do partial clone 1' ' > test "$(git -C pc1 config --local core.partialclonefilter)" = "blob:none" > ' > > +test_expect_success 'verify that .promisor file contains refs fetched' ' > + ls pc1/.git/objects/pack/pack-*.promisor >promisorlist && > + test_line_count = 1 promisorlist && > + git -C srv.bare rev-list HEAD >headhash && > + grep "$(cat headhash) HEAD" $(cat promisorlist) && > + grep "$(cat headhash) refs/heads/master" $(cat promisorlist) > +' > + > # checkout master to force dynamic object fetch of blobs at HEAD. > test_expect_success 'verify checkout with dynamic object fetch' ' > git -C pc1 rev-list --quiet --objects --missing=print HEAD >observed && > -- > 2.23.0.187.g17f5b7556c-goog >
Thanks for your comments. Rearranging them: > This makes me wonder why we don't also change index-pack to write a > similar message to the .promisor. I guess there's potentially too much > information to shove all the refs on the command-line? index-pack already is capable of writing messages to .promisor using the "--promisor" argument. You're right that I'm not using that because I don't want to run into argument length limits. > I am not sure why we want to tie creating the .promisor to creating the > lockfile. I'll keep reading and see if it becomes clear later. Other > than that, the logic here seems clear. [snip] > Apart from using the lockfile name as the base for the .promisor > filename, I'm still not seeing why we need to tie this to the fact that > we're creating a lockfile. Could we instead just unconditionally create > the .promisor when args->from_promisor is set, and then remove the logic > in the previous chunk that adds the "--promisor" flag to the index-pack > call? I'm tying the promisor to the lockfile to avoid overcomplicating things: fetch-pack currently reads filename information from index-pack only when there is a lockfile. (It could do so even when there is no lockfile, but it currently does not.) We need this filename to know what to call the ".promisor" file. And the situation that I'm interested in - when the user fetches with "git fetch" from http or ssh - always uses a lockfile (see fetch_refs_via_pack() in transport.c). So I'm writing additional data in this case, and falling back on the "--promisor" flag otherwise. I'll elaborate in the commit message.
diff --git a/builtin/repack.c b/builtin/repack.c index 632c0c0a79..8c1621d414 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -232,6 +232,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args, /* * pack-objects creates the .pack and .idx files, but not the * .promisor file. Create the .promisor file, which is empty. + * + * NEEDSWORK: fetch-pack generates non-empty .promisor files, + * but this would not preserve their contents. Maybe + * concatenate the contents of all .promisor files instead of + * just creating a new empty file. */ promisor_name = mkpathdup("%s-%s.promisor", packtmp, line.buf); diff --git a/fetch-pack.c b/fetch-pack.c index 65be043f2a..07029e1bbf 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -758,8 +758,33 @@ static int sideband_demux(int in, int out, void *data) return ret; } +static void write_promisor_file(const char *keep_name, + struct ref **sought, int nr_sought) +{ + struct strbuf promisor_name = STRBUF_INIT; + int suffix_stripped; + FILE *output; + int i; + + strbuf_addstr(&promisor_name, keep_name); + suffix_stripped = strbuf_strip_suffix(&promisor_name, ".keep"); + if (!suffix_stripped) + BUG("name of pack lockfile should end with .keep (was '%s')", + keep_name); + strbuf_addstr(&promisor_name, ".promisor"); + + output = xfopen(promisor_name.buf, "w"); + for (i = 0; i < nr_sought; i++) + fprintf(output, "%s %s\n", oid_to_hex(&sought[i]->old_oid), + sought[i]->name); + fclose(output); + + strbuf_release(&promisor_name); +} + static int get_pack(struct fetch_pack_args *args, - int xd[2], char **pack_lockfile) + int xd[2], char **pack_lockfile, + struct ref **sought, int nr_sought) { struct async demux; int do_keep = args->keep_pack; @@ -821,7 +846,13 @@ static int get_pack(struct fetch_pack_args *args, } if (args->check_self_contained_and_connected) argv_array_push(&cmd.args, "--check-self-contained-and-connected"); - if (args->from_promisor) + /* + * If we're obtaining the filename of a lockfile, we'll use + * that filename to write a .promisor file with more + * information below. If not, we need index-pack to do it for + * us. + */ + if (!(do_keep && pack_lockfile) && args->from_promisor) argv_array_push(&cmd.args, "--promisor"); } else { @@ -859,6 +890,8 @@ static int get_pack(struct fetch_pack_args *args, die(_("fetch-pack: unable to fork off %s"), cmd_name); if (do_keep && pack_lockfile) { *pack_lockfile = index_pack_lockfile(cmd.out); + if (args->from_promisor) + write_promisor_file(*pack_lockfile, sought, nr_sought); close(cmd.out); } @@ -1009,7 +1042,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args, alternate_shallow_file = setup_temporary_shallow(si->shallow); else alternate_shallow_file = NULL; - if (get_pack(args, fd, pack_lockfile)) + if (get_pack(args, fd, pack_lockfile, sought, nr_sought)) die(_("git fetch-pack: fetch failed.")); all_done: @@ -1458,7 +1491,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, /* get the pack */ process_section_header(&reader, "packfile", 0); - if (get_pack(args, fd, pack_lockfile)) + if (get_pack(args, fd, pack_lockfile, sought, nr_sought)) die(_("git fetch-pack: fetch failed.")); state = FETCH_DONE; diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh index 565254558f..486db27ee0 100755 --- a/t/t5616-partial-clone.sh +++ b/t/t5616-partial-clone.sh @@ -46,6 +46,14 @@ test_expect_success 'do partial clone 1' ' test "$(git -C pc1 config --local core.partialclonefilter)" = "blob:none" ' +test_expect_success 'verify that .promisor file contains refs fetched' ' + ls pc1/.git/objects/pack/pack-*.promisor >promisorlist && + test_line_count = 1 promisorlist && + git -C srv.bare rev-list HEAD >headhash && + grep "$(cat headhash) HEAD" $(cat promisorlist) && + grep "$(cat headhash) refs/heads/master" $(cat promisorlist) +' + # checkout master to force dynamic object fetch of blobs at HEAD. test_expect_success 'verify checkout with dynamic object fetch' ' git -C pc1 rev-list --quiet --objects --missing=print HEAD >observed &&
The specification of promisor packfiles (in partial-clone.txt) states that the .promisor files that accompany packfiles do not matter (just like .keep files), so whenever a packfile is fetched from the promisor remote, Git has been writing empty .promisor files. But these files could contain more useful information. So instead of writing empty files, write the refs fetched to these files. This makes it easier to debug issues with partial clones, as we can identify what refs (and their associated hashes) were fetched at the time the packfile was downloaded, and if necessary, compare those hashes against what the promisor remote reports now. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- As written in the NEEDSWORK comment, repack does not preserve the contents of .promisor files, but I thought I'd send this out anyway as this change is already useful for users who don't run repack much. --- builtin/repack.c | 5 +++++ fetch-pack.c | 41 ++++++++++++++++++++++++++++++++++++---- t/t5616-partial-clone.sh | 8 ++++++++ 3 files changed, 50 insertions(+), 4 deletions(-)