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