patch, 19 partsAdd configuration options for split-index

> When users are using `git update-index --(no-)split-index`, they
> may expect the split-index feature to be used or not according to
> the option they just used, but this might not be the case if the
> new "core.splitIndex" config variable has been set. In this case
> let's warn about what will happen and why.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  builtin/update-index.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index b75ea03..a14dbf2 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -1098,12 +1098,21 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>         }
>
>         if (split_index > 0) {
> +               if (git_config_get_split_index() == 0)
> +                       warning("core.splitIndex is set to false; "
> +                               "remove or change it, if you really want to "
> +                               "enable split index");

>                 if (the_index.split_index)
>                         the_index.cache_changed |= SPLIT_INDEX_ORDERED;
>                 else
>                         add_split_index(&the_index);
> -       } else if (!split_index)
> +       } else if (!split_index) {
> +               if (git_config_get_split_index() == 1)
> +                       warning("core.splitIndex is set to true; "
> +                               "remove or change it, if you really want to "
> +                               "disable split index");
>                 remove_split_index(&the_index);
> +       }
>
>         switch (untracked_cache) {
>         case UC_UNSPECIFIED:
> --
> 2.10.1.462.g7e1e03a

> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> When users are using `git update-index --(no-)split-index`, they
>> may expect the split-index feature to be used or not according to
>> the option they just used, but this might not be the case if the
>> new "core.splitIndex" config variable has been set. In this case
>> let's warn about what will happen and why.
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>>  builtin/update-index.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/update-index.c b/builtin/update-index.c
>> index b75ea03..a14dbf2 100644
>> --- a/builtin/update-index.c
>> +++ b/builtin/update-index.c
>> @@ -1098,12 +1098,21 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>>         }
>>
>>         if (split_index > 0) {
>> +               if (git_config_get_split_index() == 0)
>> +                       warning("core.splitIndex is set to false; "
>> +                               "remove or change it, if you really want to "
>> +                               "enable split index");
>
> Wrap this string and the one below with _() so they can be translated.

>> diff --git a/builtin/update-index.c b/builtin/update-index.c
>> index b75ea03..a14dbf2 100644
>> --- a/builtin/update-index.c
>> +++ b/builtin/update-index.c
>> @@ -1098,12 +1098,21 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>>         }
>>
>>         if (split_index > 0) {
>> +               if (git_config_get_split_index() == 0)
>> +                       warning("core.splitIndex is set to false; "
>> +                               "remove or change it, if you really want to "
>> +                               "enable split index");
>
> Wrap this string and the one below with _() so they can be translated.

>>                 if (the_index.split_index)
>>                         the_index.cache_changed |= SPLIT_INDEX_ORDERED;
>>                 else
>>                         add_split_index(&the_index);
>> -       } else if (!split_index)
>> +       } else if (!split_index) {
>> +               if (git_config_get_split_index() == 1)
>> +                       warning("core.splitIndex is set to true; "
>> +                               "remove or change it, if you really want to "
>> +                               "disable split index");
>>                 remove_split_index(&the_index);
>> +       }
>>
>>         switch (untracked_cache) {
>>         case UC_UNSPECIFIED:
>> --
>> 2.10.1.462.g7e1e03a

> Duy Nguyen <pclouds@gmail.com> writes:
>
>>> diff --git a/builtin/update-index.c b/builtin/update-index.c
>>> index b75ea03..a14dbf2 100644
>>> --- a/builtin/update-index.c
>>> +++ b/builtin/update-index.c
>>> @@ -1098,12 +1098,21 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>>>         }
>>>
>>>         if (split_index > 0) {
>>> +               if (git_config_get_split_index() == 0)
>>> +                       warning("core.splitIndex is set to false; "
>>> +                               "remove or change it, if you really want to "
>>> +                               "enable split index");
>>
>> Wrap this string and the one below with _() so they can be translated.
>
> True.
>
> I further wonder if a natural reaction from users after seeing this
> message is "I do want to--what else would I use that option to run
> you for?  Just do as you are told, instead of telling me what to
> do!".  Is this warning really a good idea, or shouldn't these places
> be setting the configuration?

>>> Wrap this string and the one below with _() so they can be translated.
>>
>> True.
>>
>> I further wonder if a natural reaction from users after seeing this
>> message is "I do want to--what else would I use that option to run
>> you for?  Just do as you are told, instead of telling me what to
>> do!".  Is this warning really a good idea, or shouldn't these places
>> be setting the configuration?
>
> In 435ec090ec (config: add core.untrackedCache, 2016-01-27) we decided
> to just use warning() after discussing if we should instead set the
> configuration.

> +void remove_split_index(struct index_state *istate)
> +{
> +       if (istate->split_index) {
> +               /*
> +                * can't discard_split_index(&the_index); because that
> +                * will destroy split_index->base->cache[], which may
> +                * be shared with the_index.cache[]. So yeah we're
> +                * leaking a bit here.

> +                */
> +               istate->split_index = NULL;
> +               istate->cache_changed |= SOMETHING_CHANGED;
> +       }
> +}

> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> +void remove_split_index(struct index_state *istate)
>> +{
>> +       if (istate->split_index) {
>> +               /*
>> +                * can't discard_split_index(&the_index); because that
>> +                * will destroy split_index->base->cache[], which may
>> +                * be shared with the_index.cache[]. So yeah we're
>> +                * leaking a bit here.
>
> In the context of update-index, this is a one-time thing and leaking
> is tolerable. But because it becomes a library function now, this leak
> can become more serious, I think.
>
> The only other (indirect) caller is read_index_from() so probably not
> bad most of the time (we read at the beginning of a command only).
> sequencer.c may discard and re-read the index many times though,
> leaking could be visible there.

> On Tue, Oct 25, 2016 at 11:58 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>>> +void remove_split_index(struct index_state *istate)
>>> +{
>>> +       if (istate->split_index) {
>>> +               /*
>>> +                * can't discard_split_index(&the_index); because that
>>> +                * will destroy split_index->base->cache[], which may
>>> +                * be shared with the_index.cache[]. So yeah we're
>>> +                * leaking a bit here.
>>
>> In the context of update-index, this is a one-time thing and leaking
>> is tolerable. But because it becomes a library function now, this leak
>> can become more serious, I think.
>>
>> The only other (indirect) caller is read_index_from() so probably not
>> bad most of the time (we read at the beginning of a command only).
>> sequencer.c may discard and re-read the index many times though,
>> leaking could be visible there.
>
> So is it enough to check if split_index->base->cache[] is shared with
> the_index.cache[] and then decide if discard_split_index(&the_index)
> should be called?

> On Sun, Oct 30, 2016 at 5:06 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> On Tue, Oct 25, 2016 at 11:58 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
>>> <christian.couder@gmail.com> wrote:
>>>> +void remove_split_index(struct index_state *istate)
>>>> +{
>>>> +       if (istate->split_index) {
>>>> +               /*
>>>> +                * can't discard_split_index(&the_index); because that
>>>> +                * will destroy split_index->base->cache[], which may
>>>> +                * be shared with the_index.cache[]. So yeah we're
>>>> +                * leaking a bit here.
>>>
>>> In the context of update-index, this is a one-time thing and leaking
>>> is tolerable. But because it becomes a library function now, this leak
>>> can become more serious, I think.
>>>
>>> The only other (indirect) caller is read_index_from() so probably not
>>> bad most of the time (we read at the beginning of a command only).
>>> sequencer.c may discard and re-read the index many times though,
>>> leaking could be visible there.
>>
>> So is it enough to check if split_index->base->cache[] is shared with
>> the_index.cache[] and then decide if discard_split_index(&the_index)
>> should be called?
>
> It's likely shared though. We could un-share cache[] by duplicating
> index entries in the_index.cache[] if they point back to
> split_index->base

> On Mon, Nov 7, 2016 at 5:08 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Sun, Oct 30, 2016 at 5:06 AM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>>> On Tue, Oct 25, 2016 at 11:58 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>>>> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
>>>> <christian.couder@gmail.com> wrote:
>>>>> +void remove_split_index(struct index_state *istate)
>>>>> +{
>>>>> +       if (istate->split_index) {
>>>>> +               /*
>>>>> +                * can't discard_split_index(&the_index); because that
>>>>> +                * will destroy split_index->base->cache[], which may
>>>>> +                * be shared with the_index.cache[]. So yeah we're
>>>>> +                * leaking a bit here.
>>>>
>>>> In the context of update-index, this is a one-time thing and leaking
>>>> is tolerable. But because it becomes a library function now, this leak
>>>> can become more serious, I think.
>>>>
>>>> The only other (indirect) caller is read_index_from() so probably not
>>>> bad most of the time (we read at the beginning of a command only).
>>>> sequencer.c may discard and re-read the index many times though,
>>>> leaking could be visible there.
>>>
>>> So is it enough to check if split_index->base->cache[] is shared with
>>> the_index.cache[] and then decide if discard_split_index(&the_index)
>>> should be called?
>>
>> It's likely shared though. We could un-share cache[] by duplicating
>> index entries in the_index.cache[] if they point back to
>> split_index->base
>
> I have a patch for this. So don't have to do anything else in this
> area. I'll probably just pile my patch on top of your series, or post
> it once the series graduates to master.

> This new function will be used in a following commit to get the
> +int git_config_get_max_percent_split_change(void)
> +{
> +       int val = -1;
> +
> +       if (!git_config_get_int("splitindex.maxpercentchange", &val)) {
> +               if (0 <= val && val <= 100)
> +                       return val;
> +
> +               error("splitindex.maxpercentchange value '%d' "

> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> This new function will be used in a following commit to get the
>> +int git_config_get_max_percent_split_change(void)
>> +{
>> +       int val = -1;
>> +
>> +       if (!git_config_get_int("splitindex.maxpercentchange", &val)) {
>> +               if (0 <= val && val <= 100)
>> +                       return val;
>> +
>> +               error("splitindex.maxpercentchange value '%d' "
>
> We should keep camelCase form for easy reading. And wrap this string with _().

> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> This new function will be used in a following commit to get the
>> +int git_config_get_max_percent_split_change(void)
>> +{
>> +       int val = -1;
>> +
>> +       if (!git_config_get_int("splitindex.maxpercentchange", &val)) {
>> +               if (0 <= val && val <= 100)
>> +                       return val;
>> +
>> +               error("splitindex.maxpercentchange value '%d' "
>
> We should keep camelCase form for easy reading. And wrap this string with _().
>
>> +                     "should be between 0 and 100", val);
>
> I wonder if anybody would try to put 12.3 here and confused by the
> error message, because 0 <= 12.3 <= 100, but it's not an integer..
> Ah.. never mind, die_bad_number() would be called first in this case
> with a loud and clear complaint.

>
>>> +               return -1;
>
> Perhaps do the usual
>
>         return error(_("..."));
>
> here?

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  Documentation/config.txt | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 96521a4..380eeb8 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2763,6 +2763,19 @@ showbranch.default::
>  	The default set of branches for linkgit:git-show-branch[1].
>  	See linkgit:git-show-branch[1].
>  
> +splitIndex.maxPercentChange::
> +	When the split index feature is used, this specifies the
> +	percent of entries the split index can contain compared to the
> +	whole number of entries in both the split index and the shared
> +	index before a new shared index is written.
> +	The value should be between 0 and 100. If the value is 0 then
> +	a new shared index is always written, if it is 100 a new
> +	shared index is never written.

> Christian Couder <christian.couder@gmail.com> writes:
>
>> +splitIndex.maxPercentChange::
>> +     When the split index feature is used, this specifies the
>> +     percent of entries the split index can contain compared to the
>> +     whole number of entries in both the split index and the shared
>> +     index before a new shared index is written.
>> +     The value should be between 0 and 100. If the value is 0 then
>> +     a new shared index is always written, if it is 100 a new
>> +     shared index is never written.
>
> Hmph.  The early part of the description implies this will kick in
> only when some other conditions (i.e. the bit in the index or the
> other configuration) are met, but if this disables the split index
> when it is set to 0, would we even need the other configuration
> variable?  IOW, perhaps we can do without core.splitIndex?

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  t/t1700-split-index.sh | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
>
> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index 507a1dd..f03addf 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -238,4 +238,76 @@ EOF
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'set core.splitIndex config variable to true' '
> +	git config core.splitIndex true &&
> +	: >three &&
> +	git update-index --add three &&
> +	BASE=$(test-dump-split-index .git/index | grep "^base") &&
> +	test-dump-split-index .git/index | sed "/^own/d" >actual &&
> +	cat >expect <<EOF &&
> +$BASE
> +replacements:
> +deletions:
> +EOF

> When writing a new split-index and there is a big number of cache
> entries in the split-index compared to the shared index, it is a
> good idea to regenerate the shared index.
> 
> By default when the ratio reaches 20%, we will push back all
> the entries from the split-index into a new shared index file
> instead of just creating a new split-index file.
> 
> The threshold can be configured using the
> "splitIndex.maxPercentChange" config variable.
> 
> We need to adjust the existing tests in t1700 by setting
> "splitIndex.maxPercentChange" to 100 at the beginning of t1700,
> as the existing tests are assuming that the shared index is
> regenerated only when `git update-index --split-index` is used.
> 
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  read-cache.c           | 33 ++++++++++++++++++++++++++++++++-
>  t/t1700-split-index.sh |  1 +
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/read-cache.c b/read-cache.c
> index bb53823..a91fabe 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2216,6 +2216,36 @@ static int write_shared_index(struct index_state *istate,
>  	return ret;
>  }
>  
> +static const int default_max_percent_split_change = 20;
> +
> +int too_many_not_shared_entries(struct index_state *istate)

>>
>> +int too_many_not_shared_entries(struct index_state *istate)
>
> This function is a file-loacal symbol; could you please make it
> a static function.

> @@ -2233,7 +2263,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
>                 if ((v & 15) < 6)
>                         istate->cache_changed |= SPLIT_INDEX_ORDERED;
>         }
> -       if (istate->cache_changed & SPLIT_INDEX_ORDERED) {
> +       if (istate->cache_changed & SPLIT_INDEX_ORDERED ||
> +           too_many_not_shared_entries(istate)) {

>                 int ret = write_shared_index(istate, lock, flags);
>                 if (ret)
>                         return ret;
> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index db8c39f..507a1dd 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -8,6 +8,7 @@ test_description='split index mode tests'
>  sane_unset GIT_TEST_SPLIT_INDEX
>
>  test_expect_success 'enable split index' '
> +       git config splitIndex.maxPercentChange 100 &&

>         git update-index --split-index &&
>         test-dump-split-index .git/index >actual &&
>         indexversion=$(test-index-version <.git/index) &&
> --
> 2.10.1.462.g7e1e03a
>

> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> @@ -2233,7 +2263,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
>>                 if ((v & 15) < 6)
>>                         istate->cache_changed |= SPLIT_INDEX_ORDERED;
>>         }
>> -       if (istate->cache_changed & SPLIT_INDEX_ORDERED) {
>> +       if (istate->cache_changed & SPLIT_INDEX_ORDERED ||
>> +           too_many_not_shared_entries(istate)) {
>
> It's probably safer to keep this piece unchanged and add this
> somewhere before it
>
> if (too_many_not_shared_entries(istate))
>     istate->cache_changed |= SPLIT_INDEX_ORDERED;
>
> We could keep cache_changed consistent until the end this way.

>>  test_expect_success 'enable split index' '
>> +       git config splitIndex.maxPercentChange 100 &&
>
> An alternative name might be splitThreshold. I don't know, maybe
> maxPercentChange is better.

> Christian Couder <christian.couder@gmail.com> writes:
>
>> This function will be used in a following commit to get the expiration
>> time of the shared index files from the config, and it is generic
>> enough to be put in "config.c".
>
> Is it generic enough that a helper that sounds as if it can get any
> date string dies if it is given a future date?  I somehow doubt it.
>
> At the minimum, it must be made clear that there is an artificial
> limitation that the current set of callers find useful in cache.h as
> a one-liner comment next to the added declaration.  Then people with
> the same need (i.e. they want to reject future timestamps) can
> decide to use it, while others would stay away from it.
>
> If you can come up with a better word to use to encode that
> artificial limitation in its name, renaming it is even better.

> Ok it will appear like this in cache.h:
>
> /* This dies if the configured or default date is in the future */
> extern int git_config_get_expire_date_string(const char *key, const
> char **output);

> Christian Couder <christian.couder@gmail.com> writes:
>
>> Ok it will appear like this in cache.h:
>>
>> /* This dies if the configured or default date is in the future */
>> extern int git_config_get_expire_date_string(const char *key, const
>> char **output);
>
> Those who imitate existing callsites never read comments, and you
> need to spend effort to get the name right to protect the codebase
> from them.
>
> "get-expiry" may be shorter.  Neither still does not say it will
> die, though.

> What about something like this then:
>
> /* This dies if the configured or default date is in the future */
> extern int git_config_get_expiry_or_die(const char *key, const char **output);
>
> Also git_config_get_int(), git_config_get_bool() and probably other
> such functions are indirectly calling git_config_int() which die()s is
> the value is not a number, so it feels a bit strange to have only one
> function with a name that ends with "_or_die" when many other
> functions could die(). In fact it could give a false sense of security
> to those who just read cache.h.

> +static int can_delete_shared_index(const char *shared_sha1_hex)
> +{
> +       struct stat st;
> +       unsigned long expiration;
> +       const char *shared_index = git_path("sharedindex.%s", shared_sha1_hex);
> +
> +       /* Check timestamp */
> +       expiration = get_shared_index_expire_date();
> +       if (!expiration)
> +               return 0;
> +       if (stat(shared_index, &st))
> +               return error_errno("could not stat '%s", shared_index);
> +       if (st.st_mtime > expiration)

> +               return 0;
> +
> +       return 1;
> +}
> +
> +static void clean_shared_index_files(const char *current_hex)
> +{
> +       struct dirent *de;
> +       DIR *dir = opendir(get_git_dir());
> +
> +       if (!dir) {
> +               error_errno("unable to open git dir: %s", get_git_dir());

> +       }
> +
> +       while ((de = readdir(dir)) != NULL) {
> +               const char *sha1_hex;
> +               if (!skip_prefix(de->d_name, "sharedindex.", &sha1_hex))
> +                       continue;
> +               if (!strcmp(sha1_hex, current_hex))
> +                       continue;

>  static int write_shared_index(struct index_state *istate,
> @@ -2211,8 +2269,11 @@ static int write_shared_index(struct index_state *istate,
>         }
>         ret = rename_tempfile(&temporary_sharedindex,
>                               git_path("sharedindex.%s", sha1_to_hex(si->base->sha1)));
> -       if (!ret)
> +       if (!ret) {
>                 hashcpy(si->base_sha1, si->base->sha1);
> +               clean_shared_index_files(sha1_to_hex(si->base->sha1));

>>  static int write_shared_index(struct index_state *istate,
>> @@ -2211,8 +2269,11 @@ static int write_shared_index(struct index_state *istate,
>>         }
>>         ret = rename_tempfile(&temporary_sharedindex,
>>                               git_path("sharedindex.%s", sha1_to_hex(si->base->sha1)));
>> -       if (!ret)
>> +       if (!ret) {
>>                 hashcpy(si->base_sha1, si->base->sha1);
>> +               clean_shared_index_files(sha1_to_hex(si->base->sha1));
>
> This operation is technically garbage collection and should belong to
> "git gc --auto", which is already called automatically in a few
> places. Is it not called often enough that we need to do the cleaning
> up right after a new shared index is created?

> On Tue, Oct 25, 2016 at 5:43 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>>>  static int write_shared_index(struct index_state *istate,
>>> @@ -2211,8 +2269,11 @@ static int write_shared_index(struct index_state *istate,
>>>         }
>>>         ret = rename_tempfile(&temporary_sharedindex,
>>>                               git_path("sharedindex.%s", sha1_to_hex(si->base->sha1)));
>>> -       if (!ret)
>>> +       if (!ret) {
>>>                 hashcpy(si->base_sha1, si->base->sha1);
>>> +               clean_shared_index_files(sha1_to_hex(si->base->sha1));
>>
>> This operation is technically garbage collection and should belong to
>> "git gc --auto", which is already called automatically in a few
>> places. Is it not called often enough that we need to do the cleaning
>> up right after a new shared index is created?
>
> Christian, if we assume to go with Junio's suggestion to disable
> split-index on temporary files, the only files left we have to take
> care of are index and index.lock. I believe pruning here in this code
> will have an advantage over in "git gc --auto" because when this is
> executed, we know we're holding index.lock, so nobody else is updating
> the index, it's race-free. All we need to do is peek in $GIT_DIR/index
> to see what shared index file it requires and keep it alive too, the
> remaining of shared index files can be deleted safely. We don't even
> need to fall back to mtime.
>
> git-gc just can't match this because while it's running, somebody else
> may be updating $GIT_DIR/index. Handling races would be a lot harder.

> Christian, if we assume to go with Junio's suggestion to disable
> split-index on temporary files, the only files left we have to take
> care of are index and index.lock. I believe pruning here in this code
> will have an advantage over in "git gc --auto" because when this is
> executed, we know we're holding index.lock, so nobody else is updating
> the index, it's race-free.
>
> All we need to do is peek in $GIT_DIR/index
> to see what shared index file it requires and keep it alive too, the
> remaining of shared index files can be deleted safely. We don't even
> need to fall back to mtime.

>> git-gc just can't match this because while it's running, somebody else
>> may be updating $GIT_DIR/index. Handling races would be a lot harder.
>
> It could attempt to take a lock on the primary index while it runs,
> and refrain to do anything if it can't take the lock ("gc --auto"
> may want to silently retry), and then the race is no longer an
> issue, no?

>                 int ret = write_shared_index(istate, lock, flags);
>                 if (ret)
>                         return ret;
> +       } else {
> +               /* Signal that the shared index is used */
> +               const char *shared_index = git_path("sharedindex.%s",
> +                                                   sha1_to_hex(si->base_sha1));
> +               if (!check_and_freshen_file(shared_index, 1))
> +                       warning("could not freshen '%s'", shared_index);

> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> @@ -2268,6 +2268,12 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
>
> Doing this in read_index_from() would keep the shared file even more
> "fresher" since read happens a lot more often than write. But I think
> our main concern is not the temporary index files created by the user
> scripts, but $GIT_DIR/index.lock (make sure we don't accidentally
> delete its shared file before it gets renamed to $GIT_DIR/index). For
> this case, I think refreshing in write_locked_index is enough.

>
>>                 int ret = write_shared_index(istate, lock, flags);
>>                 if (ret)
>>                         return ret;
>> +       } else {
>> +               /* Signal that the shared index is used */
>> +               const char *shared_index = git_path("sharedindex.%s",
>> +                                                   sha1_to_hex(si->base_sha1));
>> +               if (!check_and_freshen_file(shared_index, 1))
>> +                       warning("could not freshen '%s'", shared_index);
>
> _()

> The design is similar as the previous work that introduced
> "core.untrackedCache". 
>
> The new "core.splitIndex" configuration option can be either true,
> false or undefined which is the default.
>
> When it is true, the split index is created, if it does not already
> exists, when the index is read. When it is false, the split index is
> removed if it exists, when the index is read. Otherwise it is left as
> is.

> Along with this new configuration variable, the two following options
> are also introduced:
>
>     - splitIndex.maxPercentChange
>
>     This is to avoid having too many changes accumulating in the split
>     index while in split index mode. The git-update-index
>     documentation says:
>
> 	If split-index mode is already enabled and `--split-index` is
> 	given again, all changes in $GIT_DIR/index are pushed back to
> 	the shared index file.
>
>     but it is probably better to not expect the user to think about it
>     and to have a mechanism that pushes back all changes to the shared
>     index file automatically when some threshold is reached.
>
>     The default threshold is when the number of entries in the split
>     index file reaches 20% (by default) of the number of entries in
>     the shared index file. The new "splitIndex.maxPercentChange"
>     config option lets people tweak this value.

>     - splitIndex.sharedIndexExpire
>
>     To make sure that old sharedindex files are eventually removed
>     when a new one has been created, we "touch" the shared index file
>     every time it is used by a new split index file. Then we can
>     delete shared indexes with an mtime older than one week (by
>     default), when we create a new shared index file. The new
>     "splitIndex.sharedIndexExpire" config option lets people tweak
>     this grace period.

>>     - splitIndex.sharedIndexExpire
>>
>>     To make sure that old sharedindex files are eventually removed
>>     when a new one has been created, we "touch" the shared index file
>>     every time it is used by a new split index file. Then we can
>>     delete shared indexes with an mtime older than one week (by
>>     default), when we create a new shared index file. The new
>>     "splitIndex.sharedIndexExpire" config option lets people tweak
>>     this grace period.
>
> I do not quite understand this justification.  Doesn't each of the
> "this hold only changes since the base index file" files have a
> backpointer that names the base index file it is a delta against?

> Is it safe to remove a base index file when there is still a split
> index file that points at it?
>
> IOW, I do not see why it can be safe for the expiration decision to
> be based on timestamp (I would understand it if it were based on a
> refcnt, though).

> Timestamps allow us to say, ok this base index file has not been read
> by anybody for N+ hours (or better, days), it's most likely not
> referenced by any temporary index files (including
> $GIT_DIR/index.lock) anymore because those files, by the definition of
> "temporary", must be gone by now....

>> Timestamps allow us to say, ok this base index file has not been read
>> by anybody for N+ hours (or better, days), it's most likely not
>> referenced by any temporary index files (including
>> $GIT_DIR/index.lock) anymore because those files, by the definition of
>> "temporary", must be gone by now....
>
> and if we guessed wrong, users will have a "temporary index" that
> they meant to keep for longer term that is now broken here.  I am
> not sure if that risk is worth taking.

> On Wed, Oct 26, 2016 at 12:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Even if we ignore user index files (by forcing them all to be stored
> in one piece), there is a problem with the special temporary file
> index.lock, which must use split-index because it will become the new
> index. Handling race conditions could be tricky with ref counting.
> Timestamps help in this regard.

> Goal
> ~~~~
>
> We want to make it possible to use the split-index feature
> automatically by just setting a new "core.splitIndex" configuration
> variable to true.

> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> Goal
>> ~~~~
>>
>> We want to make it possible to use the split-index feature
>> automatically by just setting a new "core.splitIndex" configuration
>> variable to true.
>
> Thanks. This definitely should help make split index a lot more
> convenient for day-to-day use. The series looks ok (well, except the
> pruning logic being discussed elsewhere in this thread, but I still
> think it's a good strategy).

>> This can be valuable as split-index can help significantly speed up
>> `git rebase` especially along with the work to libify `git apply`
>> that has been recently merged to master
>> (see https://github.com/git/git/commit/81358dc238372793b1590efa149cc1581d1fbd98).
>
> I remember this. Since the apply libification work has landed, we can
> now think about not writing index out after every apply step, which
> gives the same (or more) speed up without the complication of
> split-index. But if split-index is good enough for you, we can leave
> it for another time.

> Le 6 nov. 2016 09:16, "Junio C Hamano" <gitster@pobox.com> a écrit :
>>
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>> > I think it is easier for user to be able to just set core.splitIndex
>> > to true to enable split-index.
>>
>> You can have that exact benefit by making core.splitIndex to
>> bool-or-more.  If your default is 20%, take 'true' as if the user
>> specified 20% and take 'false' as if the user specified 100% (or is
>> it 0%?  I do not care about the details but you get the point).
>
> Then if we ever add 'auto' and the user wants for example 10% instead of the
> default 20%, we will have to make it accept things like "auto,10".

> On Mon, Nov 7, 2016 at 4:44 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> Le 6 nov. 2016 09:16, "Junio C Hamano" <gitster@pobox.com> a écrit :
>>>
>>> Christian Couder <christian.couder@gmail.com> writes:
>>>
>>> > I think it is easier for user to be able to just set core.splitIndex
>>> > to true to enable split-index.
>>>
>>> You can have that exact benefit by making core.splitIndex to
>>> bool-or-more.  If your default is 20%, take 'true' as if the user
>>> specified 20% and take 'false' as if the user specified 100% (or is
>>> it 0%?  I do not care about the details but you get the point).
>
>> Then if we ever add 'auto' and the user wants for example 10% instead of the
>> default 20%, we will have to make it accept things like "auto,10".

> In my opinion, "true" _is_ auto, which is a way to say "I trust you to
> do the right thing, just re-split the index when it makes sense", "no"
> is disabled of course. If the user wants to be specific, just write
> "10" or some other percentage.(and either 0 or 100 would mean enable
> split-index but do not re-split automatically, let _me_ do it when I
> want it)

> On Mon, Nov 7, 2016 at 10:38 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> (sorry I got sick in the last few weeks and could not respond to this earlier)
>
> (Yeah, I have also been sick during the last few weeks.)
>
>> On Mon, Nov 7, 2016 at 4:44 AM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>>> Le 6 nov. 2016 09:16, "Junio C Hamano" <gitster@pobox.com> a écrit :
>>>>
>>>> Christian Couder <christian.couder@gmail.com> writes:
>>>>
>>>> > I think it is easier for user to be able to just set core.splitIndex
>>>> > to true to enable split-index.
>>>>
>>>> You can have that exact benefit by making core.splitIndex to
>>>> bool-or-more.  If your default is 20%, take 'true' as if the user
>>>> specified 20% and take 'false' as if the user specified 100% (or is
>>>> it 0%?  I do not care about the details but you get the point).
>>
>>> Then if we ever add 'auto' and the user wants for example 10% instead of the
>>> default 20%, we will have to make it accept things like "auto,10".
>
> (Sorry for writing the above on my phone which added HTML, so that it
> didn't reach the list.)
>
>> In my opinion, "true" _is_ auto, which is a way to say "I trust you to
>> do the right thing, just re-split the index when it makes sense", "no"
>> is disabled of course. If the user wants to be specific, just write
>> "10" or some other percentage.(and either 0 or 100 would mean enable
>> split-index but do not re-split automatically, let _me_ do it when I
>> want it)
>
> The meaning of a future "auto" option for "core.splitIndex" could be
> "use the split-index feature only if the number of entries in whole
> index is greater than 10000 (by default)".

> If there is no difference between "true" and "auto" then, when users
> who have "core.splitIndex=true" will migrate to the git version that
> adds the "auto" feature, their repos with under 10000 entires will not
> use the split-index feature anymore. These users may then be annoyed
> that the behavior has been switched under them, and that the
> split-index feature is not always used despite having
> "core.splitIndex=true" in their config.

>>> In my opinion, "true" _is_ auto, which is a way to say "I trust you to
>>> do the right thing, just re-split the index when it makes sense", "no"
>>> is disabled of course. If the user wants to be specific, just write
>>> "10" or some other percentage.(and either 0 or 100 would mean enable
>>> split-index but do not re-split automatically, let _me_ do it when I
>>> want it)
>>
>> The meaning of a future "auto" option for "core.splitIndex" could be
>> "use the split-index feature only if the number of entries in whole
>> index is greater than 10000 (by default)".
>
> Well.. with the "just re-split the index when it makes sense" part,
> the user entrusts git to do something sensible in all cases,