Re: [RFC PATCH 00/19] object-store refactoring 3 (replace objects, main ref store)

[Stefan Beller]

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, 

Re: [RFC PATCH 00/19] object-store refactoring 3 (replace objects, main ref store)

[Stefan Beller]

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)

[Stefan Beller]

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

[Derrick Stolee]

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)

[Duy Nguyen]

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)

[René Scharfe]

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, _obj->original)) {
+   if (get_oid_hex(hash, _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(_obj->replacement, oid);
 
/* Register new object */
-   if (register_replace_object(repl_obj, 1))
+   if (oidmap_put(_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(const 

[RFC PATCH 00/19] object-store refactoring 3 (replace objects, main ref store)

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