Re: [PATCH 042/194] object-store: move alternates API to new alternates.h

[Junio C Hamano]

"brian m. carlson"  writes:

>> +#include "strbuf.h"
>> +#include "sha1-array.h"
>> +
>> +struct alternates {
>> +struct alternate_object_database *list;
>> +struct alternate_object_database **tail;
>> +};
>> +#define ALTERNATES_INIT { NULL, NULL }
>
> I was surprised to find that this patch not only moves the alternates
> API to a new location, but introduces this struct.  I certainly think
> the struct is a good idea, but it should probably go in a separate
> patch, or at least get a mention in the commit message.

Yeah, I tend to agree that splitting it into two patches may make
more sense, especially given that this is already close to 200 patch
series ;-)

Re: [PATCH 042/194] object-store: move alternates API to new alternates.h

[Stefan Beller]

On Mon, Feb 5, 2018 at 5:44 PM, brian m. carlson
 wrote:
> On Mon, Feb 05, 2018 at 03:55:03PM -0800, Stefan Beller wrote:
>> From: Jonathan Nieder 
>>
>> This should make these functions easier to find and object-store.h
>> less overwhelming to read.
>>
>> Signed-off-by: Stefan Beller 
>> Signed-off-by: Jonathan Nieder 
>> ---
>>  alternates.h| 68 
>> +
>>  builtin/clone.c |  1 +
>>  builtin/count-objects.c |  1 +
>>  builtin/fsck.c  |  3 +-
>>  builtin/grep.c  |  1 +
>>  builtin/submodule--helper.c |  1 +
>>  cache.h | 52 --
>>  object-store.h  | 16 +--
>>  packfile.c  |  3 +-
>>  sha1_file.c | 23 +++
>>  sha1_name.c |  3 +-
>>  submodule.c |  1 +
>>  t/helper/test-ref-store.c   |  1 +
>>  tmp-objdir.c|  1 +
>>  transport.c |  1 +
>>  15 files changed, 102 insertions(+), 74 deletions(-)
>>  create mode 100644 alternates.h
>>
>> diff --git a/alternates.h b/alternates.h
>> new file mode 100644
>> index 00..df5dc67e2e
>> --- /dev/null
>> +++ b/alternates.h
>> @@ -0,0 +1,68 @@
>> +#ifndef ALTERNATES_H
>> +#define ALTERNATES_H
>> +
>> +#include "strbuf.h"
>> +#include "sha1-array.h"
>> +
>> +struct alternates {
>> + struct alternate_object_database *list;
>> + struct alternate_object_database **tail;
>> +};
>> +#define ALTERNATES_INIT { NULL, NULL }
>
> I was surprised to find that this patch not only moves the alternates
> API to a new location, but introduces this struct.  I certainly think
> the struct is a good idea, but it should probably go in a separate
> patch, or at least get a mention in the commit message.

ok, I'll fix the commit message.

Thanks,
Stefan

Re: [PATCH 042/194] object-store: move alternates API to new alternates.h

[Stefan Beller]

On Mon, Feb 5, 2018 at 8:52 PM, Eric Sunshine  wrote:
> On Mon, Feb 5, 2018 at 6:55 PM, Stefan Beller  wrote:
>> This should make these functions easier to find and object-store.h
>> less overwhelming to read.
>
> I think you mean: s/object-store.h/cache.h/

Probably both.

At the end of the series the object-store.h has grown a bit, such that
we'd want to have specific things outside of it. Alternates are a good choice
as they are coherent on their own.

I have given up on cache.h and its readability, but moving things out of there
helps of course. And that is what the patch does.
So I'll change that.

Thanks,
Stefan

Re: [PATCH 042/194] object-store: move alternates API to new alternates.h

[Eric Sunshine]

On Mon, Feb 5, 2018 at 6:55 PM, Stefan Beller  wrote:
> This should make these functions easier to find and object-store.h
> less overwhelming to read.

I think you mean: s/object-store.h/cache.h/

> Signed-off-by: Stefan Beller 
> Signed-off-by: Jonathan Nieder 
> ---
>  cache.h | 52 --
>  object-store.h  | 16 +--

Re: [PATCH 042/194] object-store: move alternates API to new alternates.h

[brian m. carlson]

On Mon, Feb 05, 2018 at 03:55:03PM -0800, Stefan Beller wrote:
> From: Jonathan Nieder 
> 
> This should make these functions easier to find and object-store.h
> less overwhelming to read.
> 
> Signed-off-by: Stefan Beller 
> Signed-off-by: Jonathan Nieder 
> ---
>  alternates.h| 68 
> +
>  builtin/clone.c |  1 +
>  builtin/count-objects.c |  1 +
>  builtin/fsck.c  |  3 +-
>  builtin/grep.c  |  1 +
>  builtin/submodule--helper.c |  1 +
>  cache.h | 52 --
>  object-store.h  | 16 +--
>  packfile.c  |  3 +-
>  sha1_file.c | 23 +++
>  sha1_name.c |  3 +-
>  submodule.c |  1 +
>  t/helper/test-ref-store.c   |  1 +
>  tmp-objdir.c|  1 +
>  transport.c |  1 +
>  15 files changed, 102 insertions(+), 74 deletions(-)
>  create mode 100644 alternates.h
> 
> diff --git a/alternates.h b/alternates.h
> new file mode 100644
> index 00..df5dc67e2e
> --- /dev/null
> +++ b/alternates.h
> @@ -0,0 +1,68 @@
> +#ifndef ALTERNATES_H
> +#define ALTERNATES_H
> +
> +#include "strbuf.h"
> +#include "sha1-array.h"
> +
> +struct alternates {
> + struct alternate_object_database *list;
> + struct alternate_object_database **tail;
> +};
> +#define ALTERNATES_INIT { NULL, NULL }

I was surprised to find that this patch not only moves the alternates
API to a new location, but introduces this struct.  I certainly think
the struct is a good idea, but it should probably go in a separate
patch, or at least get a mention in the commit message.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

[PATCH 042/194] object-store: move alternates API to new alternates.h

[Stefan Beller]

From: Jonathan Nieder 

This should make these functions easier to find and object-store.h
less overwhelming to read.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 alternates.h| 68 +
 builtin/clone.c |  1 +
 builtin/count-objects.c |  1 +
 builtin/fsck.c  |  3 +-
 builtin/grep.c  |  1 +
 builtin/submodule--helper.c |  1 +
 cache.h | 52 --
 object-store.h  | 16 +--
 packfile.c  |  3 +-
 sha1_file.c | 23 +++
 sha1_name.c |  3 +-
 submodule.c |  1 +
 t/helper/test-ref-store.c   |  1 +
 tmp-objdir.c|  1 +
 transport.c |  1 +
 15 files changed, 102 insertions(+), 74 deletions(-)
 create mode 100644 alternates.h

diff --git a/alternates.h b/alternates.h
new file mode 100644
index 00..df5dc67e2e
--- /dev/null
+++ b/alternates.h
@@ -0,0 +1,68 @@
+#ifndef ALTERNATES_H
+#define ALTERNATES_H
+
+#include "strbuf.h"
+#include "sha1-array.h"
+
+struct alternates {
+   struct alternate_object_database *list;
+   struct alternate_object_database **tail;
+};
+#define ALTERNATES_INIT { NULL, NULL }
+
+struct alternate_object_database {
+   struct alternate_object_database *next;
+
+   /* see alt_scratch_buf() */
+   struct strbuf scratch;
+   size_t base_len;
+
+   /*
+* Used to store the results of readdir(3) calls when searching
+* for unique abbreviated hashes.  This cache is never
+* invalidated, thus it's racy and not necessarily accurate.
+* That's fine for its purpose; don't use it for tasks requiring
+* greater accuracy!
+*/
+   char loose_objects_subdir_seen[256];
+   struct oid_array loose_objects_cache;
+
+   /*
+* Path to the alternate object database, relative to the
+* current working directory.
+*/
+   char path[FLEX_ARRAY];
+};
+extern void prepare_alt_odb(struct repository *r);
+extern char *compute_alternate_path(const char *path, struct strbuf *err);
+typedef int alt_odb_fn(struct alternate_object_database *, void *);
+extern int foreach_alt_odb(struct repository *r, alt_odb_fn, void*);
+
+/*
+ * Allocate a "struct alternate_object_database" but do _not_ actually
+ * add it to the list of alternates.
+ */
+struct alternate_object_database *alloc_alt_odb(const char *dir);
+
+/*
+ * Add the directory to the on-disk alternates file; the new entry will also
+ * take effect in the current process.
+ */
+extern void add_to_alternates_file(const char *dir);
+
+/*
+ * Add the directory to the in-memory list of alternates (along with any
+ * recursive alternates it points to), but do not modify the on-disk alternates
+ * file.
+ */
+extern void add_to_alternates_memory(const char *dir);
+
+/*
+ * Returns a scratch strbuf pre-filled with the alternate object directory,
+ * including a trailing slash, which can be used to access paths in the
+ * alternate. Always use this over direct access to alt->scratch, as it
+ * cleans up any previous use of the scratch buffer.
+ */
+extern struct strbuf *alt_scratch_buf(struct alternate_object_database *alt);
+
+#endif /* ALTERNATES_H */
diff --git a/builtin/clone.c b/builtin/clone.c
index 2da71db107..27463c8fc5 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -11,6 +11,7 @@
 #include "builtin.h"
 #include "config.h"
 #include "lockfile.h"
+#include "alternates.h"
 #include "parse-options.h"
 #include "fetch-pack.h"
 #include "refs.h"
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index e340b3e3c3..805803fedd 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -8,6 +8,7 @@
 #include "config.h"
 #include "dir.h"
 #include "repository.h"
+#include "alternates.h"
 #include "object-store.h"
 #include "builtin.h"
 #include "parse-options.h"
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 0e78f4ba72..571aa51579 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -2,6 +2,7 @@
 #include "cache.h"
 #include "repository.h"
 #include "config.h"
+#include "alternates.h"
 #include "object-store.h"
 #include "commit.h"
 #include "tree.h"
@@ -696,7 +697,7 @@ int cmd_fsck(int argc, const char **argv, const char 
*prefix)
fsck_object_dir(get_object_directory());
 
prepare_alt_odb(the_repository);
-   for (alt = the_repository->objects.alt_odb_list;
+   for (alt = the_repository->objects.alt_odb.list;
alt; alt = alt->next)
fsck_object_dir(alt->path);
 
diff --git a/builtin/grep.c b/builtin/grep.c
index 3ca4ac80d8..1c71dff341 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -6,6 +6,7 @@
 #include "cache.h"
 #include "repository.h"
 #include "config.h"
+#include