Re: [PATCH 02/14] packed-graph: add core.graph setting

From: Derrick Stolee <stolee@gmail.com>
Date: Jan 25, 2018, 20:40 UTC
Message-ID: <d69b7eab-0aa2-6fed-7750-2fdf1ac8269f@gmail.com>
In-Reply-To: <CAGZ79kbUr-iQghL8qXxCq7noiYR9-MrxmTB4_v3D7WyXLPwUjA@mail.gmail.com>

On 1/25/2018 3:17 PM, Stefan Beller wrote:

Show 22 quoted lines

> On Thu, Jan 25, 2018 at 6:02 AM, Derrick Stolee <stolee@gmail.com> wrote:
>> The packed graph feature is controlled by the new core.graph config
>> setting. This defaults to 0, so the feature is opt-in.
>>
>> The intention of core.graph is that a user can always stop checking
>> for or parsing packed graph files if core.graph=0.
>>
>> @@ -825,6 +825,7 @@ extern char *git_replace_ref_base;
>>   extern int fsync_object_files;
>>   extern int core_preload_index;
>>   extern int core_apply_sparse_checkout;
>> +extern int core_graph;
> Putting it here instead of say the_repository makes sense as you'd want
> to use this feature globally. However you can still have the config
> different per repository  (e.g. version number of the graph setting,
> as one might be optimized for speed and the other for file size of
> the .graph file or such).
>
> So not sure if we'd rather want to put this into the repository struct.
> But then again the other core settings aren't there either and this
> feature sounds like it is repository specific only in the experimental
> phase; later it is expected to be on everywhere?

I do think that more things should go in the repository struct. Unfortunately, that is not the world we live in.

However, to make things clearer I'm following the pattern currently in master. You'll see the same with the global 'packed_graph' pointer, similar to 'packed_git'. I think these should be paired together until the repository absorbs them.

If other 'core_*' variables move to the repository, I'm happy to move core_graph. If 'packed_git' moves to the repository, I'm happy to move 'packed_git'.

However, if there is significant interest in moving all new state to the repository, then I'll move these values there. Let's have that discussion here instead of spread around the rest of the patch.

Thanks, -Stolee