Re: [RFC PATCH 00/19] object-store refactoring 3 (replace objects, main ref store)
Hi René, On Fri, Apr 6, 2018 at 9:58 PM, René Scharfe wrote: > Am 07.04.2018 um 01:21 schrieb Stefan Beller: >> This applies on top of 464416a2eaadf84d2bfdf795007863d03b222b7c >> (sb/packfiles-in-repository). >> It is also available at >> https://github.com/stefanbeller/git/tree/object-store-3 > > This series conflicts with 1731a1e239 (replace_object: convert struct > replace_object to object_id) and b383a13cc0 (Convert > lookup_replace_object to struct object_id), which are in next. ok, I'll investigate. Maybe the reroll will be on top of a merge of brians series and sb/packfiles-in-repository, both of which go to master quickly now that 2.17 is out? > >> This series will bring the replacement mechanism (git replace) >> into the object store. > > Good idea. heh, thanks. I think this is the smallest unit in which the community is willing to digest it. (The largest coherent patch series was the demo of "everything in struct repo"[1]) [1] https://public-inbox.org/git/20180205235508.216277-1-sbel...@google.com/ > >> $ git diff 464416a2eaadf84d2bfdf795007863d03b222b7c..HEAD -- >> object-store.h repository.h >> diff --git a/object-store.h b/object-store.h >> index fef33f345f..be90c02db6 100644 >> --- a/object-store.h >> +++ b/object-store.h >> @@ -93,6 +93,22 @@ struct raw_object_store { >> struct alternate_object_database *alt_odb_list; >> struct alternate_object_database **alt_odb_tail; >> >> + /* >> +* Objects that should be substituted by other objects >> +* (see git-replace(1)). >> +*/ >> + struct replace_objects { >> + /* >> +* An array of replacements. The array is kept sorted by >> the original >> +* sha1. >> +*/ >> + struct replace_object **items; >> + >> + int alloc, nr; >> + >> + unsigned prepared : 1; >> + } replacements; > > An oidmap would be a better fit -- lookups should be quicker and > memory consumption not much worse. I meant to submit something like > this eventually after Brian's series lands: > > -- >8 -- > Subject: [PATCH] replace_object: use oidmap > > Load the replace objects into an oidmap to allow for easy lookups in > constant time. > > Signed-off-by: Rene Scharfe > --- > This is on top of next. So this is on top of brians series (modulo other next-ish stuff) which this series ought to merge with? The patch looks good -- just from the diff stat alone. Thanks, Stefan > > replace_object.c | 76 ++-- > 1 file changed, 16 insertions(+), 60 deletions(-) > > diff --git a/replace_object.c b/replace_object.c > index 336357394d..a757a5ebf2 100644 > --- a/replace_object.c > +++ b/replace_object.c > @@ -1,54 +1,14 @@ > #include "cache.h" > -#include "sha1-lookup.h" > +#include "oidmap.h" > #include "refs.h" > #include "commit.h" > > -/* > - * An array of replacements. The array is kept sorted by the original > - * sha1. > - */ > -static struct replace_object { > - struct object_id original; > +struct replace_object { > + struct oidmap_entry original; > struct object_id replacement; > -} **replace_object; > - > -static int replace_object_alloc, replace_object_nr; > +}; > > -static const unsigned char *replace_sha1_access(size_t index, void *table) > -{ > - struct replace_object **replace = table; > - return replace[index]->original.hash; > -} > - > -static int replace_object_pos(const unsigned char *sha1) > -{ > - return sha1_pos(sha1, replace_object, replace_object_nr, > - replace_sha1_access); > -} > - > -static int register_replace_object(struct replace_object *replace, > - int ignore_dups) > -{ > - int pos = replace_object_pos(replace->original.hash); > - > - if (0 <= pos) { > - if (ignore_dups) > - free(replace); > - else { > - free(replace_object[pos]); > - replace_object[pos] = replace; > - } > - return 1; > - } > - pos = -pos - 1; > - ALLOC_GROW(replace_object, replace_object_nr + 1, > replace_object_alloc); > - replace_object_nr++; > - if (pos < replace_object_nr) > - MOVE_ARRAY(replace_object + pos + 1, replace_object + pos, > - replace_object_nr - pos - 1); > - replace_object[pos] = replace; > - return 0; > -} > +static struct oidmap replace_map = OIDMAP_INIT; > > static int register_replace_ref(const char *refname, > const struct object_id *oid, > @@ -59,7 +19,7 @@ static int register_replace_ref(const char *refname, > const char *hash = slash ? slash + 1 : refname; > struct replace_object *repl_obj = xmalloc(sizeof(*repl_obj)); > > - if (get_oid_hex(hash, &repl_obj->original)) { > + if (get
Re: [RFC PATCH 00/19] object-store refactoring 3 (replace objects, main ref store)
On Sat, Apr 7, 2018 at 2:50 AM, Duy Nguyen wrote: > On Sat, Apr 7, 2018 at 1:21 AM, Stefan Beller wrote: > * >> diff --git a/repository.h b/repository.h >> index 09df94a472..2922d3a28b 100644 >> --- a/repository.h >> +++ b/repository.h >> @@ -26,6 +26,11 @@ struct repository { >> */ >> struct raw_object_store *objects; >> >> + /* >> +* The store in which the refs are hold. >> +*/ >> + struct ref_store *main_ref_store; >> + > > We probably should drop the main_ prefix here because this could also > be a submodule ref store (when the repository is about a submodule). it's still the main ref store of the submodule, then. :) But yes, I agree we should rename it. Now or eventually later? > worktree ref store is a different story and I don't know how to best > present it here yet (I'm still thinking a separate struct repository). I imagine that we'd rather want to have arrays of config/worktree path/refs when needed and the worktree is just an index into all of these things? > But we can worry about that when struct repository supports multiple > worktree. ok. Thanks for bringing this to my attention. Thanks, Stefan
Re: [RFC PATCH 00/19] object-store refactoring 3 (replace objects, main ref store)
+cc list On Mon, Apr 9, 2018 at 1:39 AM, Johannes Schindelin wrote: > Hi Stefan, > > On Fri, 6 Apr 2018, Stefan Beller wrote: > >> See cfc62fc98c (sha1_file: add repository argument to link_alt_odb_entry, >> 2018-03-23) for explanation. > > "See ... for explanation." ... are you going full Russian on us? ;-) That commit is supposed to merge to master now, so we don't lose it. I just dislike to repeat myself, and there we explained that trick already. As commit messages don't link well together in email clients, I will repeat the important part. Thanks, Stefan > > Ciao, > Dscho
Re: [RFC PATCH 00/19] object-store refactoring 3 (replace objects, main ref store)
On 4/6/2018 7:21 PM, Stefan Beller wrote: This applies on top of 464416a2eaadf84d2bfdf795007863d03b222b7c (sb/packfiles-in-repository). It is also available at https://github.com/stefanbeller/git/tree/object-store-3 This series will bring the replacement mechanism (git replace) into the object store. The first patches are cleaning up a bit, and patches 6-19 are converting one function at a time using the tick-tock pattern with the #define trick. See cfc62fc98c (sha1_file: add repository argument to link_alt_odb_entry, 2018-03-23) for explanation. Thanks, Stefan I looked through these patches and only found one set of whitespace errors. Compiles and tests fine on my machine. Reviewed-by: Derrick Stolee
Re: [RFC PATCH 00/19] object-store refactoring 3 (replace objects, main ref store)
On Sat, Apr 7, 2018 at 1:21 AM, Stefan Beller wrote: * > diff --git a/repository.h b/repository.h > index 09df94a472..2922d3a28b 100644 > --- a/repository.h > +++ b/repository.h > @@ -26,6 +26,11 @@ struct repository { > */ > struct raw_object_store *objects; > > + /* > +* The store in which the refs are hold. > +*/ > + struct ref_store *main_ref_store; > + We probably should drop the main_ prefix here because this could also be a submodule ref store (when the repository is about a submodule). worktree ref store is a different story and I don't know how to best present it here yet (I'm still thinking a separate struct repository). But we can worry about that when struct repository supports multiple worktree. -- Duy
Re: [RFC PATCH 00/19] object-store refactoring 3 (replace objects, main ref store)
Am 07.04.2018 um 01:21 schrieb Stefan Beller: > This applies on top of 464416a2eaadf84d2bfdf795007863d03b222b7c > (sb/packfiles-in-repository). > It is also available at > https://github.com/stefanbeller/git/tree/object-store-3 This series conflicts with 1731a1e239 (replace_object: convert struct replace_object to object_id) and b383a13cc0 (Convert lookup_replace_object to struct object_id), which are in next. > This series will bring the replacement mechanism (git replace) > into the object store. Good idea. > $ git diff 464416a2eaadf84d2bfdf795007863d03b222b7c..HEAD -- object-store.h > repository.h > diff --git a/object-store.h b/object-store.h > index fef33f345f..be90c02db6 100644 > --- a/object-store.h > +++ b/object-store.h > @@ -93,6 +93,22 @@ struct raw_object_store { > struct alternate_object_database *alt_odb_list; > struct alternate_object_database **alt_odb_tail; > > + /* > +* Objects that should be substituted by other objects > +* (see git-replace(1)). > +*/ > + struct replace_objects { > + /* > +* An array of replacements. The array is kept sorted by the > original > +* sha1. > +*/ > + struct replace_object **items; > + > + int alloc, nr; > + > + unsigned prepared : 1; > + } replacements; An oidmap would be a better fit -- lookups should be quicker and memory consumption not much worse. I meant to submit something like this eventually after Brian's series lands: -- >8 -- Subject: [PATCH] replace_object: use oidmap Load the replace objects into an oidmap to allow for easy lookups in constant time. Signed-off-by: Rene Scharfe --- This is on top of next. replace_object.c | 76 ++-- 1 file changed, 16 insertions(+), 60 deletions(-) diff --git a/replace_object.c b/replace_object.c index 336357394d..a757a5ebf2 100644 --- a/replace_object.c +++ b/replace_object.c @@ -1,54 +1,14 @@ #include "cache.h" -#include "sha1-lookup.h" +#include "oidmap.h" #include "refs.h" #include "commit.h" -/* - * An array of replacements. The array is kept sorted by the original - * sha1. - */ -static struct replace_object { - struct object_id original; +struct replace_object { + struct oidmap_entry original; struct object_id replacement; -} **replace_object; - -static int replace_object_alloc, replace_object_nr; +}; -static const unsigned char *replace_sha1_access(size_t index, void *table) -{ - struct replace_object **replace = table; - return replace[index]->original.hash; -} - -static int replace_object_pos(const unsigned char *sha1) -{ - return sha1_pos(sha1, replace_object, replace_object_nr, - replace_sha1_access); -} - -static int register_replace_object(struct replace_object *replace, - int ignore_dups) -{ - int pos = replace_object_pos(replace->original.hash); - - if (0 <= pos) { - if (ignore_dups) - free(replace); - else { - free(replace_object[pos]); - replace_object[pos] = replace; - } - return 1; - } - pos = -pos - 1; - ALLOC_GROW(replace_object, replace_object_nr + 1, replace_object_alloc); - replace_object_nr++; - if (pos < replace_object_nr) - MOVE_ARRAY(replace_object + pos + 1, replace_object + pos, - replace_object_nr - pos - 1); - replace_object[pos] = replace; - return 0; -} +static struct oidmap replace_map = OIDMAP_INIT; static int register_replace_ref(const char *refname, const struct object_id *oid, @@ -59,7 +19,7 @@ static int register_replace_ref(const char *refname, const char *hash = slash ? slash + 1 : refname; struct replace_object *repl_obj = xmalloc(sizeof(*repl_obj)); - if (get_oid_hex(hash, &repl_obj->original)) { + if (get_oid_hex(hash, &repl_obj->original.oid)) { free(repl_obj); warning("bad replace ref name: %s", refname); return 0; @@ -69,7 +29,7 @@ static int register_replace_ref(const char *refname, oidcpy(&repl_obj->replacement, oid); /* Register new object */ - if (register_replace_object(repl_obj, 1)) + if (oidmap_put(&replace_map, repl_obj)) die("duplicate replace ref: %s", refname); return 0; @@ -84,7 +44,7 @@ static void prepare_replace_object(void) for_each_replace_ref(register_replace_ref, NULL); replace_object_prepared = 1; - if (!replace_object_nr) + if (!replace_map.map.tablesize) check_replace_refs = 0; } @@ -100,21 +60,17 @@ static void prepare_replace_object(void) */ const struct object_id *do_lookup_replace_object
[RFC PATCH 00/19] object-store refactoring 3 (replace objects, main ref store)
This applies on top of 464416a2eaadf84d2bfdf795007863d03b222b7c (sb/packfiles-in-repository). It is also available at https://github.com/stefanbeller/git/tree/object-store-3 This series will bring the replacement mechanism (git replace) into the object store. The first patches are cleaning up a bit, and patches 6-19 are converting one function at a time using the tick-tock pattern with the #define trick. See cfc62fc98c (sha1_file: add repository argument to link_alt_odb_entry, 2018-03-23) for explanation. Thanks, Stefan Stefan Beller (19): replace_object.c: rename to use dash in file name replace-object: move replace_object to object store object-store: move lookup_replace_object to replace-object.h replace-object: move replace objects prepared flag to object store replace-object: check_replace_refs is safe in multi repo environment refs: add repository argument to get_main_ref_store refs: add repository argument to for_each_replace_ref replace-object: add repository argument to replace_object_pos replace-object: add repository argument to register_replace_object replace-object: add repository argument to prepare_replace_object replace-object: add repository argument to do_lookup_replace_object replace-object: add repository argument to lookup_replace_object refs: store the main ref store inside the repository struct refs: allow for_each_replace_ref to handle arbitrary repositories replace-object: allow replace_object_pos to handle arbitrary repositories replace-object: allow register_replace_object to handle arbitrary repositories replace-object: allow prepare_replace_object to handle arbitrary repositories replace-object: allow do_lookup_replace_object to handle arbitrary repositories replace-object: allow lookup_replace_object to handle arbitrary repositories Makefile | 2 +- builtin/mktag.c | 3 +- builtin/pack-refs.c | 3 +- builtin/replace.c| 4 +- cache.h | 19 --- environment.c| 2 +- object-store.h | 16 ++ object.c | 3 +- refs.c | 80 ++-- refs.h | 4 +- replace_object.c => replace-object.c | 66 +++ replace-object.h | 35 repository.h | 5 ++ revision.c | 5 +- sha1_file.c | 7 +-- streaming.c | 3 +- t/helper/test-ref-store.c| 3 +- 17 files changed, 149 insertions(+), 111 deletions(-) rename replace_object.c => replace-object.c (56%) create mode 100644 replace-object.h $ git diff 464416a2eaadf84d2bfdf795007863d03b222b7c..HEAD -- object-store.h repository.h diff --git a/object-store.h b/object-store.h index fef33f345f..be90c02db6 100644 --- a/object-store.h +++ b/object-store.h @@ -93,6 +93,22 @@ struct raw_object_store { struct alternate_object_database *alt_odb_list; struct alternate_object_database **alt_odb_tail; + /* +* Objects that should be substituted by other objects +* (see git-replace(1)). +*/ + struct replace_objects { + /* +* An array of replacements. The array is kept sorted by the original +* sha1. +*/ + struct replace_object **items; + + int alloc, nr; + + unsigned prepared : 1; + } replacements; + /* * private data * diff --git a/repository.h b/repository.h index 09df94a472..2922d3a28b 100644 --- a/repository.h +++ b/repository.h @@ -26,6 +26,11 @@ struct repository { */ struct raw_object_store *objects; + /* +* The store in which the refs are hold. +*/ + struct ref_store *main_ref_store; + /* * Path to the repository's graft file. * Cannot be NULL after initialization. -- 2.17.0.484.g0c8726318c-goog