{"thread":{"id":"44361","subject":"[PATCH v1 00/19] Add configuration options for split-index","startedAt":"2016-10-23T09:27:20Z","lastAt":"2016-11-28T16:56:15Z","messageCount":66,"participants":["Christian Couder","Ramsay Jones","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":19},"messages":[{"id":"304729","messageId":"20161023092648.12086-1-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":null,"subject":"[PATCH v1 00/19] Add configuration options for split-index","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:29Z","receivedAt":"2016-10-23T09:27:20Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Goal\n~~~~\n\nWe want to make it possible to use the split-index feature\nautomatically by just setting a new \"core.splitIndex\" configuration\nvariable to true.\n\nThis can be valuable as split-index can help significantly speed up\n`git rebase` especially along with the work to libify `git apply`\nthat has been recently merged to master\n(see https://github.com/git/git/commit/81358dc238372793b1590efa149cc1581d1fbd98).\n\nDesign\n~~~~~~\n\nThe design is similar as the previous work that introduced\n\"core.untrackedCache\". \n\nThe new \"core.splitIndex\" configuration option can be either true,\nfalse or undefined which is the default.\n\nWhen it is true, the split index is created, if it does not already\nexists, when the index is read. When it is false, the split index is\nremoved if it exists, when the index is read. Otherwise it is left as\nis.\n\nAlong with this new configuration variable, the two following options\nare also introduced:\n\n - splitIndex.maxPercentChange\n\n This is to avoid having too many changes accumulating in the split\n index while in split index mode. The git-update-index\n documentation says:\n\n\tIf split-index mode is already enabled and `--split-index` is\n\tgiven again, all changes in $GIT_DIR/index are pushed back to\n\tthe shared index file.\n\n but it is probably better to not expect the user to think about it\n and to have a mechanism that pushes back all changes to the shared\n index file automatically when some threshold is reached.\n\n The default threshold is when the number of entries in the split\n index file reaches 20% (by default) of the number of entries in\n the shared index file. The new \"splitIndex.maxPercentChange\"\n config option lets people tweak this value.\n\n - splitIndex.sharedIndexExpire\n\n To make sure that old sharedindex files are eventually removed\n when a new one has been created, we \"touch\" the shared index file\n every time it is used by a new split index file. Then we can\n delete shared indexes with an mtime older than one week (by\n default), when we create a new shared index file. The new\n \"splitIndex.sharedIndexExpire\" config option lets people tweak\n this grace period.\n\n This idea was suggested by Duy in:\n\n https://public-inbox.org/git/CACsJy8BqMFASHf5kJgUh+bd7XG98CafNydE964VJyPXz-emEvA@mail.gmail.com/\n\n and after some experiments, I agree that it is much simpler than\n what I thought could be done during our discussion.\n\nHighlevel view of the patches in the series\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\nExcept for patch 1/19, there are 3 big steps, one for each new\nconfiguration variable introduced.\n\nThe main difference between this patch series and the RFC patch series\nsent last July is that the Step 2 and 3 are new and have been\nimplemented as suggested by Duy. Thanks Duy!\n\n - Patch 1/19 is a typo fix in a comment that can be applied\n separately.\n\nStep 1 is:\n\n - Patches 2/19 to 5/19 introduce the functions that are reading\n the \"core.splitIndex\" configuration variable and tweaking the\n split index depending on its value.\n\n - Patch 6/19 adds a few tests for the new feature.\n\n - Patches 7/19 and 8/19 add some documentation for the new\n feature.\n\nStep 2 is:\n\n - Patches 9/19 and 10/19 introduce the functions that are reading\n the \"splitIndex.maxPercentChange\" configuration variable and\n regenerating a new shared index file depending on its value.\n\n - Patch 11/19 adds a few tests for the new feature.\n\n - Patch 12/19 add some documentation for the new feature.\n\nStep 3 is:\n\n - Patches 13/19 to 16/19 introduce the functions that are reading\n the \"splitIndex.sharedIndexExpire\" configuration variable and\n expiring old shared index files depending on its value.\n\n - Patch 17/19 adds a few tests for the new feature.\n\n - Patches 18/19 and 19/19 add some documentation for the new\n feature.\n\nLinks\n~~~~~\n\nThis patch series is also available here:\n\n https://github.com/chriscool/git/commits/config-split-index\n\nThe previous RFC version was:\n\n https://github.com/chriscool/git/commits/config-split-index7\n\nOn the mailing list the related patch series and discussions were:\n\n https://public-inbox.org/git/20160711172254.13439-1-chriscool@tuxfamily.org/\n\nChristian Couder (19):\n split-index: s/eith/with/ typo fix\n config: add git_config_get_split_index()\n split-index: add {add,remove}_split_index() functions\n read-cache: add and then use tweak_split_index()\n update-index: warn in case of split-index incoherency\n t1700: add tests for core.splitIndex\n Documentation/config: add information for core.splitIndex\n Documentation/git-update-index: talk about core.splitIndex config var\n config: add git_config_get_max_percent_split_change()\n read-cache: regenerate shared index if necessary\n t1700: add tests for splitIndex.maxPercentChange\n Documentation/config: add splitIndex.maxPercentChange\n sha1_file: make check_and_freshen_file() non static\n read-cache: touch shared index files when used\n config: add git_config_get_date_string() from gc.c\n read-cache: unlink old sharedindex files\n t1700: test shared index file expiration\n Documentation/config: add splitIndex.sharedIndexExpire\n Documentation/git-update-index: explain splitIndex.*\n\n Documentation/config.txt | 28 +++++++\n Documentation/git-update-index.txt | 39 ++++++++--\n builtin/gc.c | 15 +---\n builtin/update-index.c | 25 +++---\n cache.h | 6 ++\n config.c | 39 ++++++++++\n read-cache.c | 119 +++++++++++++++++++++++++++-\n sha1_file.c | 2 +-\n split-index.c | 24 +++++-\n split-index.h | 2 +\n t/t1700-split-index.sh | 154 +++++++++++++++++++++++++++++++++++++\n 11 files changed, 417 insertions(+), 36 deletions(-)\n\n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304730","messageId":"20161023092648.12086-3-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 02/19] config: add git_config_get_split_index()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:31Z","receivedAt":"2016-10-23T09:27:26Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This new function will be used in a following commit to know\nif we want to use the split index feature or not.\n\nSigned-off-by: Christian Couder \n---\n cache.h | 1 +\n config.c | 10 ++++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 05ecb88..394da60 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1809,6 +1809,7 @@ extern int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest);\n extern int git_config_get_maybe_bool(const char *key, int *dest);\n extern int git_config_get_pathname(const char *key, const char **dest);\n extern int git_config_get_untracked_cache(void);\n+extern int git_config_get_split_index(void);\n \n /*\n * This is a hack for test programs like test-dump-untracked-cache to\ndiff --git a/config.c b/config.c\nindex 83fdecb..036e29b 100644\n--- a/config.c\n+++ b/config.c\n@@ -1709,6 +1709,16 @@ int git_config_get_untracked_cache(void)\n \treturn -1; /* default value */\n }\n \n+int git_config_get_split_index(void)\n+{\n+\tint val = -1;\n+\n+\tif (!git_config_get_maybe_bool(\"core.splitindex\", &val))\n+\t\treturn val;\n+\n+\treturn -1; /* default value */\n+}\n+\n NORETURN\n void git_die_config_linenr(const char *key, const char *filename, int linenr)\n {\n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304731","messageId":"20161023092648.12086-6-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 05/19] update-index: warn in case of split-index incoherency","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:34Z","receivedAt":"2016-10-23T09:27:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"When users are using `git update-index --(no-)split-index`, they\nmay expect the split-index feature to be used or not according to\nthe option they just used, but this might not be the case if the\nnew \"core.splitIndex\" config variable has been set. In this case\nlet's warn about what will happen and why.\n\nSigned-off-by: Christian Couder \n---\n builtin/update-index.c | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex b75ea03..a14dbf2 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -1098,12 +1098,21 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (split_index > 0) {\n+\t\tif (git_config_get_split_index() == 0)\n+\t\t\twarning(\"core.splitIndex is set to false; \"\n+\t\t\t\t\"remove or change it, if you really want to \"\n+\t\t\t\t\"enable split index\");\n \t\tif (the_index.split_index)\n \t\t\tthe_index.cache_changed |= SPLIT_INDEX_ORDERED;\n \t\telse\n \t\t\tadd_split_index(&the_index);\n-\t} else if (!split_index)\n+\t} else if (!split_index) {\n+\t\tif (git_config_get_split_index() == 1)\n+\t\t\twarning(\"core.splitIndex is set to true; \"\n+\t\t\t\t\"remove or change it, if you really want to \"\n+\t\t\t\t\"disable split index\");\n \t\tremove_split_index(&the_index);\n+\t}\n \n \tswitch (untracked_cache) {\n \tcase UC_UNSPECIFIED:\n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304732","messageId":"20161023092648.12086-5-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 04/19] read-cache: add and then use tweak_split_index()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:33Z","receivedAt":"2016-10-23T09:27:30Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This will make us use the split-index feature or not depending\non the value of the \"core.splitIndex\" config variable.\n\nSigned-off-by: Christian Couder \n---\n read-cache.c | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 38d67fa..bb53823 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1562,10 +1562,27 @@ static void tweak_untracked_cache(struct index_state *istate)\n \t}\n }\n \n+static void tweak_split_index(struct index_state *istate)\n+{\n+\tswitch (git_config_get_split_index()) {\n+\tcase -1: /* unset: do nothing */\n+\t\tbreak;\n+\tcase 0: /* false */\n+\t\tremove_split_index(istate);\n+\t\tbreak;\n+\tcase 1: /* true */\n+\t\tadd_split_index(istate);\n+\t\tbreak;\n+\tdefault: /* unknown value: do nothing */\n+\t\tbreak;\n+\t}\n+}\n+\n static void post_read_index_from(struct index_state *istate)\n {\n \tcheck_ce_order(istate);\n \ttweak_untracked_cache(istate);\n+\ttweak_split_index(istate);\n }\n \n /* remember to discard_cache() before reading a different cache! */\n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304733","messageId":"20161023092648.12086-2-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 01/19] split-index: s/eith/with/ typo fix","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:30Z","receivedAt":"2016-10-23T09:27:34Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n split-index.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/split-index.c b/split-index.c\nindex 35da553..615f4ca 100644\n--- a/split-index.c\n+++ b/split-index.c\n@@ -187,7 +187,7 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t/* Go through istate->cache[] and mark CE_MATCHED to\n \t\t * entry with positive index. We'll go through\n \t\t * base->cache[] later to delete all entries in base\n-\t\t * that are not marked eith either CE_MATCHED or\n+\t\t * that are not marked with either CE_MATCHED or\n \t\t * CE_UPDATE_IN_BASE. If istate->cache[i] is a\n \t\t * duplicate, deduplicate it.\n \t\t */\n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304734","messageId":"20161023092648.12086-4-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 03/19] split-index: add {add,remove}_split_index() functions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:32Z","receivedAt":"2016-10-23T09:27:35Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Also use the functions in cmd_update_index() in\nbuiltin/update-index.c.\n\nThese functions will be used in a following commit to tweak\nour use of the split-index feature depending on the setting\nof a configuration variable.\n\nSigned-off-by: Christian Couder \n---\n builtin/update-index.c | 18 ++++++------------\n split-index.c | 22 ++++++++++++++++++++++\n split-index.h | 2 ++\n 3 files changed, 30 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex f3f07e7..b75ea03 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -1098,18 +1098,12 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (split_index > 0) {\n-\t\tinit_split_index(&the_index);\n-\t\tthe_index.cache_changed |= SPLIT_INDEX_ORDERED;\n-\t} else if (!split_index && the_index.split_index) {\n-\t\t/*\n-\t\t * can't discard_split_index(&the_index); because that\n-\t\t * will destroy split_index->base->cache[], which may\n-\t\t * be shared with the_index.cache[]. So yeah we're\n-\t\t * leaking a bit here.\n-\t\t */\n-\t\tthe_index.split_index = NULL;\n-\t\tthe_index.cache_changed |= SOMETHING_CHANGED;\n-\t}\n+\t\tif (the_index.split_index)\n+\t\t\tthe_index.cache_changed |= SPLIT_INDEX_ORDERED;\n+\t\telse\n+\t\t\tadd_split_index(&the_index);\n+\t} else if (!split_index)\n+\t\tremove_split_index(&the_index);\n \n \tswitch (untracked_cache) {\n \tcase UC_UNSPECIFIED:\ndiff --git a/split-index.c b/split-index.c\nindex 615f4ca..f519e60 100644\n--- a/split-index.c\n+++ b/split-index.c\n@@ -317,3 +317,25 @@ void replace_index_entry_in_base(struct index_state *istate,\n \t\tistate->split_index->base->cache[new->index - 1] = new;\n \t}\n }\n+\n+void add_split_index(struct index_state *istate)\n+{\n+\tif (!istate->split_index) {\n+\t\tinit_split_index(istate);\n+\t\tistate->cache_changed |= SPLIT_INDEX_ORDERED;\n+\t}\n+}\n+\n+void remove_split_index(struct index_state *istate)\n+{\n+\tif (istate->split_index) {\n+\t\t/*\n+\t\t * can't discard_split_index(&the_index); because that\n+\t\t * will destroy split_index->base->cache[], which may\n+\t\t * be shared with the_index.cache[]. So yeah we're\n+\t\t * leaking a bit here.\n+\t\t */\n+\t\tistate->split_index = NULL;\n+\t\tistate->cache_changed |= SOMETHING_CHANGED;\n+\t}\n+}\ndiff --git a/split-index.h b/split-index.h\nindex c1324f5..df91c1b 100644\n--- a/split-index.h\n+++ b/split-index.h\n@@ -31,5 +31,7 @@ void merge_base_index(struct index_state *istate);\n void prepare_to_write_split_index(struct index_state *istate);\n void finish_writing_split_index(struct index_state *istate);\n void discard_split_index(struct index_state *istate);\n+void add_split_index(struct index_state *istate);\n+void remove_split_index(struct index_state *istate);\n \n #endif\n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304735","messageId":"20161023092648.12086-8-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 07/19] Documentation/config: add information for core.splitIndex","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:36Z","receivedAt":"2016-10-23T09:27:36Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n Documentation/config.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 27069ac..96521a4 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -331,6 +331,10 @@ core.trustctime::\n \tcrawlers and some backup systems).\n \tSee linkgit:git-update-index[1]. True by default.\n \n+core.splitIndex::\n+\tIf true, the split-index feature of the index will be used.\n+\tSee linkgit:git-update-index[1]. False by default.\n+\n core.untrackedCache::\n \tDetermines what to do about the untracked cache feature of the\n \tindex. It will be kept, if this variable is unset or set to\n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304736","messageId":"20161023092648.12086-10-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 09/19] config: add git_config_get_max_percent_split_change()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:38Z","receivedAt":"2016-10-23T09:27:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This new function will be used in a following commit to get the\nvalue of the \"splitIndex.maxPercentChange\" config variable.\n\nSigned-off-by: Christian Couder \n---\n cache.h | 1 +\n config.c | 16 ++++++++++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 394da60..faceceb 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1810,6 +1810,7 @@ extern int git_config_get_maybe_bool(const char *key, int *dest);\n extern int git_config_get_pathname(const char *key, const char **dest);\n extern int git_config_get_untracked_cache(void);\n extern int git_config_get_split_index(void);\n+extern int git_config_get_max_percent_split_change(void);\n \n /*\n * This is a hack for test programs like test-dump-untracked-cache to\ndiff --git a/config.c b/config.c\nindex 036e29b..5580f56 100644\n--- a/config.c\n+++ b/config.c\n@@ -1719,6 +1719,22 @@ int git_config_get_split_index(void)\n \treturn -1; /* default value */\n }\n \n+int git_config_get_max_percent_split_change(void)\n+{\n+\tint val = -1;\n+\n+\tif (!git_config_get_int(\"splitindex.maxpercentchange\", &val)) {\n+\t\tif (0 <= val && val <= 100)\n+\t\t\treturn val;\n+\n+\t\terror(\"splitindex.maxpercentchange value '%d' \"\n+\t\t \"should be between 0 and 100\", val);\n+\t\treturn -1;\n+\t}\n+\n+\treturn -1; /* default value */\n+}\n+\n NORETURN\n void git_die_config_linenr(const char *key, const char *filename, int linenr)\n {\n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304737","messageId":"20161023092648.12086-14-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 13/19] sha1_file: make check_and_freshen_file() non static","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:42Z","receivedAt":"2016-10-23T09:27:38Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This function will be used in a commit soon, so let's make\nit available globally.\n\nSigned-off-by: Christian Couder \n---\n cache.h | 3 +++\n sha1_file.c | 2 +-\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/cache.h b/cache.h\nindex faceceb..a625b47 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1167,6 +1167,9 @@ extern int has_pack_index(const unsigned char *sha1);\n \n extern void assert_sha1_type(const unsigned char *sha1, enum object_type expect);\n \n+/* Helper to check and \"touch\" a file */\n+extern int check_and_freshen_file(const char *fn, int freshen);\n+\n extern const signed char hexval_table[256];\n static inline unsigned int hexval(unsigned char c)\n {\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 266152d..29f927e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -576,7 +576,7 @@ static int freshen_file(const char *fn)\n * either does not exist on disk, or has a stale mtime and may be subject to\n * pruning).\n */\n-static int check_and_freshen_file(const char *fn, int freshen)\n+int check_and_freshen_file(const char *fn, int freshen)\n {\n \tif (access(fn, F_OK))\n \t\treturn 0;\n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304738","messageId":"20161023092648.12086-13-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:41Z","receivedAt":"2016-10-23T09:27:41Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n Documentation/config.txt | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 96521a4..380eeb8 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2763,6 +2763,19 @@ showbranch.default::\n \tThe default set of branches for linkgit:git-show-branch[1].\n \tSee linkgit:git-show-branch[1].\n \n+splitIndex.maxPercentChange::\n+\tWhen the split index feature is used, this specifies the\n+\tpercent of entries the split index can contain compared to the\n+\twhole number of entries in both the split index and the shared\n+\tindex before a new shared index is written.\n+\tThe value should be between 0 and 100. If the value is 0 then\n+\ta new shared index is always written, if it is 100 a new\n+\tshared index is never written.\n+\tBy default the value is 20, so a new shared index is written\n+\tif the number of entries in the split index would be greater\n+\tthan 20 percent of the total number of entries.\n+\tSee linkgit:git-update-index[1].\n+\n status.relativePaths::\n \tBy default, linkgit:git-status[1] shows paths relative to the\n \tcurrent directory. Setting this variable to `false` shows paths\n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304739","messageId":"20161023092648.12086-12-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 11/19] t1700: add tests for splitIndex.maxPercentChange","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:40Z","receivedAt":"2016-10-23T09:27:42Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n t/t1700-split-index.sh | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 72 insertions(+)\n\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex 507a1dd..f03addf 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -238,4 +238,76 @@ EOF\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'set core.splitIndex config variable to true' '\n+\tgit config core.splitIndex true &&\n+\t: >three &&\n+\tgit update-index --add three &&\n+\tBASE=$(test-dump-split-index .git/index | grep \"^base\") &&\n+\ttest-dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <four &&\n+\tgit update-index --add four &&\n+\ttest-dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <five &&\n+\tgit update-index --add five &&\n+\tBASE=$(test-dump-split-index .git/index | grep \"^base\") &&\n+\ttest-dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <six &&\n+\tgit update-index --add six &&\n+\ttest-dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <seven &&\n+\tgit update-index --add seven &&\n+\tBASE=$(test-dump-split-index .git/index | grep \"^base\") &&\n+\ttest-dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <eight &&\n+\tgit update-index --add eight &&\n+\tBASE=$(test-dump-split-index .git/index | grep \"^base\") &&\n+\ttest-dump-split-index .git/index | sed \"/^own/d\" >actual &&\n+\tcat >expect <\n---\n read-cache.c | 33 ++++++++++++++++++++++++++++++++-\n t/t1700-split-index.sh | 1 +\n 2 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex bb53823..a91fabe 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2216,6 +2216,36 @@ static int write_shared_index(struct index_state *istate,\n \treturn ret;\n }\n \n+static const int default_max_percent_split_change = 20;\n+\n+int too_many_not_shared_entries(struct index_state *istate)\n+{\n+\tint i, not_shared = 0;\n+\tint max_split = git_config_get_max_percent_split_change();\n+\n+\tswitch (max_split) {\n+\tcase -1:\n+\t\t/* not or badly configured: use the default value */\n+\t\tmax_split = default_max_percent_split_change;\n+\t\tbreak;\n+\tcase 0:\n+\t\treturn 1; /* 0% means always write a new shared index */\n+\tcase 100:\n+\t\treturn 0; /* 100% means never write a new shared index */\n+\tdefault:\n+\t\t; /* do nothing: just use the configured value */\n+\t}\n+\n+\t/* Count not shared entries */\n+\tfor (i = 0; i < istate->cache_nr; i++) {\n+\t\tstruct cache_entry *ce = istate->cache[i];\n+\t\tif (!ce->index)\n+\t\t\tnot_shared++;\n+\t}\n+\n+\treturn istate->cache_nr * max_split < not_shared * 100;\n+}\n+\n int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \t\t unsigned flags)\n {\n@@ -2233,7 +2263,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \t\tif ((v & 15) < 6)\n \t\t\tistate->cache_changed |= SPLIT_INDEX_ORDERED;\n \t}\n-\tif (istate->cache_changed & SPLIT_INDEX_ORDERED) {\n+\tif (istate->cache_changed & SPLIT_INDEX_ORDERED ||\n+\t too_many_not_shared_entries(istate)) {\n \t\tint ret = write_shared_index(istate, lock, flags);\n \t\tif (ret)\n \t\t\treturn ret;\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex db8c39f..507a1dd 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -8,6 +8,7 @@ test_description='split index mode tests'\n sane_unset GIT_TEST_SPLIT_INDEX\n \n test_expect_success 'enable split index' '\n+\tgit config splitIndex.maxPercentChange 100 &&\n \tgit update-index --split-index &&\n \ttest-dump-split-index .git/index >actual &&\n \tindexversion=$(test-index-version <.git/index) &&\n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304741","messageId":"20161023092648.12086-16-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 15/19] config: add git_config_get_date_string() from gc.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:44Z","receivedAt":"2016-10-23T09:27:45Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This function will be used in a following commit to get the expiration\ntime of the shared index files from the config, and it is generic\nenough to be put in \"config.c\".\n\nSigned-off-by: Christian Couder \n---\n builtin/gc.c | 15 ++-------------\n cache.h | 1 +\n config.c | 13 +++++++++++++\n 3 files changed, 16 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 069950d..c1e9602 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -62,17 +62,6 @@ static void report_pack_garbage(unsigned seen_bits, const char *path)\n \t\tstring_list_append(&pack_garbage, path);\n }\n \n-static void git_config_date_string(const char *key, const char **output)\n-{\n-\tif (git_config_get_string_const(key, output))\n-\t\treturn;\n-\tif (strcmp(*output, \"now\")) {\n-\t\tunsigned long now = approxidate(\"now\");\n-\t\tif (approxidate(*output) >= now)\n-\t\t\tgit_die_config(key, _(\"Invalid %s: '%s'\"), key, *output);\n-\t}\n-}\n-\n static void process_log_file(void)\n {\n \tstruct stat st;\n@@ -111,8 +100,8 @@ static void gc_config(void)\n \tgit_config_get_int(\"gc.auto\", &gc_auto_threshold);\n \tgit_config_get_int(\"gc.autopacklimit\", &gc_auto_pack_limit);\n \tgit_config_get_bool(\"gc.autodetach\", &detach_auto);\n-\tgit_config_date_string(\"gc.pruneexpire\", &prune_expire);\n-\tgit_config_date_string(\"gc.worktreepruneexpire\", &prune_worktrees_expire);\n+\tgit_config_get_date_string(\"gc.pruneexpire\", &prune_expire);\n+\tgit_config_get_date_string(\"gc.worktreepruneexpire\", &prune_worktrees_expire);\n \tgit_config(git_default_config, NULL);\n }\n \ndiff --git a/cache.h b/cache.h\nindex a625b47..bcfc0f1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1811,6 +1811,7 @@ extern int git_config_get_bool(const char *key, int *dest);\n extern int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest);\n extern int git_config_get_maybe_bool(const char *key, int *dest);\n extern int git_config_get_pathname(const char *key, const char **dest);\n+extern int git_config_get_date_string(const char *key, const char **output);\n extern int git_config_get_untracked_cache(void);\n extern int git_config_get_split_index(void);\n extern int git_config_get_max_percent_split_change(void);\ndiff --git a/config.c b/config.c\nindex 5580f56..f88c61b 100644\n--- a/config.c\n+++ b/config.c\n@@ -1685,6 +1685,19 @@ int git_config_get_pathname(const char *key, const char **dest)\n \treturn ret;\n }\n \n+int git_config_get_date_string(const char *key, const char **output)\n+{\n+\tint ret = git_config_get_string_const(key, output);\n+\tif (ret)\n+\t\treturn ret;\n+\tif (strcmp(*output, \"now\")) {\n+\t\tunsigned long now = approxidate(\"now\");\n+\t\tif (approxidate(*output) >= now)\n+\t\t\tgit_die_config(key, _(\"Invalid %s: '%s'\"), key, *output);\n+\t}\n+\treturn ret;\n+}\n+\n int git_config_get_untracked_cache(void)\n {\n \tint val = -1;\n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304742","messageId":"20161023092648.12086-18-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 17/19] t1700: test shared index file expiration","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:46Z","receivedAt":"2016-10-23T09:27:46Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n t/t1700-split-index.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 44 insertions(+)\n\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex f03addf..f448fc1 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -310,4 +310,48 @@ EOF\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'shared index files expire after 7 days by default' '\n+\t: >ten &&\n+\tgit update-index --add ten &&\n+\ttest $(ls .git/sharedindex.* | wc -l) -gt 1 &&\n+\tjust_under_7_days_ago=$((1-7*86400)) &&\n+\ttest-chmtime =$just_under_7_days_ago .git/sharedindex.* &&\n+\t: >eleven &&\n+\tgit update-index --add eleven &&\n+\ttest $(ls .git/sharedindex.* | wc -l) -gt 1 &&\n+\tjust_over_7_days_ago=$((-1-7*86400)) &&\n+\ttest-chmtime =$just_over_7_days_ago .git/sharedindex.* &&\n+\t: >twelve &&\n+\tgit update-index --add twelve &&\n+\ttest $(ls .git/sharedindex.* | wc -l) = 1\n+'\n+\n+test_expect_success 'check splitIndex.sharedIndexExpire set to 8 days' '\n+\tgit config splitIndex.sharedIndexExpire \"8.days.ago\" &&\n+\ttest-chmtime =$just_over_7_days_ago .git/sharedindex.* &&\n+\t: >thirteen &&\n+\tgit update-index --add thirteen &&\n+\ttest $(ls .git/sharedindex.* | wc -l) -gt 1 &&\n+\tjust_over_8_days_ago=$((-1-8*86400)) &&\n+\ttest-chmtime =$just_over_8_days_ago .git/sharedindex.* &&\n+\t: >fourteen &&\n+\tgit update-index --add fourteen &&\n+\ttest $(ls .git/sharedindex.* | wc -l) = 1\n+'\n+\n+test_expect_success 'check splitIndex.sharedIndexExpire set to \"never\" and \"now\"' '\n+\tgit config splitIndex.sharedIndexExpire never &&\n+\tjust_10_years_ago=$((-365*10*86400)) &&\n+\ttest-chmtime =$just_10_years_ago .git/sharedindex.* &&\n+\t: >fifteen &&\n+\tgit update-index --add fifteen &&\n+\ttest $(ls .git/sharedindex.* | wc -l) -gt 1 &&\n+\tgit config splitIndex.sharedIndexExpire now &&\n+\tjust_1_second_ago=-1 &&\n+\ttest-chmtime =$just_1_second_ago .git/sharedindex.* &&\n+\t: >sixteen &&\n+\tgit update-index --add sixteen &&\n+\ttest $(ls .git/sharedindex.* | wc -l) = 1\n+'\n+\n test_done\n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304743","messageId":"20161023092648.12086-19-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 18/19] Documentation/config: add splitIndex.sharedIndexExpire","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:47Z","receivedAt":"2016-10-23T09:27:49Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n Documentation/config.txt | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 380eeb8..5212a97 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2776,6 +2776,17 @@ splitIndex.maxPercentChange::\n \tthan 20 percent of the total number of entries.\n \tSee linkgit:git-update-index[1].\n \n+splitIndex.sharedIndexExpire::\n+\tWhen the split index feature is used, shared index files with\n+\ta mtime older than this time will be removed when a new shared\n+\tindex file is created. The value \"now\" expires all entries\n+\timmediately, and \"never\" suppresses expiration altogether.\n+\tThe default value is \"one.week.ago\".\n+\tNote that each time a new split-index file is created, the\n+\tmtime of the related shared index file is updated to the\n+\tcurrent time.\n+\tSee linkgit:git-update-index[1].\n+\n status.relativePaths::\n \tBy default, linkgit:git-status[1] shows paths relative to the\n \tcurrent directory. Setting this variable to `false` shows paths\n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304744","messageId":"20161023092648.12086-20-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 19/19] Documentation/git-update-index: explain splitIndex.*","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:48Z","receivedAt":"2016-10-23T09:27:50Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n Documentation/git-update-index.txt | 33 +++++++++++++++++++++++++--------\n 1 file changed, 25 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt\nindex e091b2a..635d157 100644\n--- a/Documentation/git-update-index.txt\n+++ b/Documentation/git-update-index.txt\n@@ -163,14 +163,10 @@ may not support it yet.\n \n --split-index::\n --no-split-index::\n-\tEnable or disable split index mode. If enabled, the index is\n-\tsplit into two files, $GIT_DIR/index and $GIT_DIR/sharedindex..\n-\tChanges are accumulated in $GIT_DIR/index while the shared\n-\tindex file contains all index entries stays unchanged. If\n-\tsplit-index mode is already enabled and `--split-index` is\n-\tgiven again, all changes in $GIT_DIR/index are pushed back to\n-\tthe shared index file. This mode is designed for very large\n-\tindexes that take a significant amount of time to read or write.\n+\tEnable or disable split index mode. If split-index mode is\n+\talready enabled and `--split-index` is given again, all\n+\tchanges in $GIT_DIR/index are pushed back to the shared index\n+\tfile.\n +\n These options take effect whatever the value of the `core.splitIndex`\n configuration variable (see linkgit:git-config[1]). But a warning is\n@@ -394,6 +390,27 @@ Although this bit looks similar to assume-unchanged bit, its goal is\n different from assume-unchanged bit's. Skip-worktree also takes\n precedence over assume-unchanged bit when both are set.\n \n+Split index\n+-----------\n+\n+This mode is designed for very large indexes that take a significant\n+amount of time to read or write.\n+\n+In this mode, the index is split into two files, $GIT_DIR/index and\n+$GIT_DIR/sharedindex.. Changes are accumulated in\n+$GIT_DIR/index, the split index, while the shared index file contains\n+all index entries and stays unchanged.\n+\n+All changes in the split index are pushed back to the shared index\n+file when the number of entries in the split index reaches a level\n+specified by the splitIndex.maxPercentChange config variable (see\n+linkgit:git-config[1]).\n+\n+Each time a new shared index file is created, the old shared index\n+files are deleted if they are older than what is specified by the\n+splitIndex.sharedIndexExpire config variable (see\n+linkgit:git-config[1]).\n+\n Untracked cache\n ---------------\n \n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304745","messageId":"20161023092648.12086-17-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 16/19] read-cache: unlink old sharedindex files","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:45Z","receivedAt":"2016-10-23T09:27:53Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Everytime split index is turned on, it creates a \"sharedindex.XXXX\"\nfile in the git directory. This change makes sure that shared index\nfiles that haven't been used for a long time are removed when a new\nshared index file is created.\n\nThe new \"splitIndex.sharedIndexExpire\" config variable is created\nto tell the delay after which an unused shared index file can be\ndeleted. It defaults to \"1.week.ago\".\n\nA previous commit made sure that each time a split index file is\ncreated the mtime of the shared index file it references is updated.\nThis makes sure that recently used shared index file will not be\ndeleted.\n\nSigned-off-by: Christian Couder \n---\n read-cache.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 62 insertions(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 3aeff77..65ceb29 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2190,6 +2190,64 @@ static int write_split_index(struct index_state *istate,\n \treturn ret;\n }\n \n+static const char *shared_index_expire = \"1.week.ago\";\n+\n+static unsigned long get_shared_index_expire_date(void)\n+{\n+\tstatic unsigned long shared_index_expire_date;\n+\tstatic int shared_index_expire_date_prepared;\n+\n+\tif (!shared_index_expire_date_prepared) {\n+\t\tgit_config_get_date_string(\"splitindex.sharedindexexpire\",\n+\t\t\t\t\t &shared_index_expire);\n+\t\tshared_index_expire_date = approxidate(shared_index_expire);\n+\t\tshared_index_expire_date_prepared = 1;\n+\t}\n+\n+\treturn shared_index_expire_date;\n+}\n+\n+static int can_delete_shared_index(const char *shared_sha1_hex)\n+{\n+\tstruct stat st;\n+\tunsigned long expiration;\n+\tconst char *shared_index = git_path(\"sharedindex.%s\", shared_sha1_hex);\n+\n+\t/* Check timestamp */\n+\texpiration = get_shared_index_expire_date();\n+\tif (!expiration)\n+\t\treturn 0;\n+\tif (stat(shared_index, &st))\n+\t\treturn error_errno(\"could not stat '%s\", shared_index);\n+\tif (st.st_mtime > expiration)\n+\t\treturn 0;\n+\n+\treturn 1;\n+}\n+\n+static void clean_shared_index_files(const char *current_hex)\n+{\n+\tstruct dirent *de;\n+\tDIR *dir = opendir(get_git_dir());\n+\n+\tif (!dir) {\n+\t\terror_errno(\"unable to open git dir: %s\", get_git_dir());\n+\t\treturn;\n+\t}\n+\n+\twhile ((de = readdir(dir)) != NULL) {\n+\t\tconst char *sha1_hex;\n+\t\tif (!skip_prefix(de->d_name, \"sharedindex.\", &sha1_hex))\n+\t\t\tcontinue;\n+\t\tif (!strcmp(sha1_hex, current_hex))\n+\t\t\tcontinue;\n+\t\tif (can_delete_shared_index(sha1_hex) > 0 &&\n+\t\t unlink(git_path(\"%s\", de->d_name)))\n+\t\t\terror_errno(\"unable to unlink: %s\", git_path(\"%s\", de->d_name));\n+\t}\n+\tclosedir(dir);\n+}\n+\n static struct tempfile temporary_sharedindex;\n \n static int write_shared_index(struct index_state *istate,\n@@ -2211,8 +2269,11 @@ static int write_shared_index(struct index_state *istate,\n \t}\n \tret = rename_tempfile(&temporary_sharedindex,\n \t\t\t git_path(\"sharedindex.%s\", sha1_to_hex(si->base->sha1)));\n-\tif (!ret)\n+\tif (!ret) {\n \t\thashcpy(si->base_sha1, si->base->sha1);\n+\t\tclean_shared_index_files(sha1_to_hex(si->base->sha1));\n+\t}\n+\n \treturn ret;\n }\n \n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304746","messageId":"20161023092648.12086-9-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 08/19] Documentation/git-update-index: talk about core.splitIndex config var","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:37Z","receivedAt":"2016-10-23T09:27:57Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n Documentation/git-update-index.txt | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt\nindex 7386c93..e091b2a 100644\n--- a/Documentation/git-update-index.txt\n+++ b/Documentation/git-update-index.txt\n@@ -171,6 +171,12 @@ may not support it yet.\n \tgiven again, all changes in $GIT_DIR/index are pushed back to\n \tthe shared index file. This mode is designed for very large\n \tindexes that take a significant amount of time to read or write.\n++\n+These options take effect whatever the value of the `core.splitIndex`\n+configuration variable (see linkgit:git-config[1]). But a warning is\n+emitted when the change goes against the configured value, as the\n+configured value will take effect next time the index is read and this\n+will remove the intended effect of the option.\n \n --untracked-cache::\n --no-untracked-cache::\n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304747","messageId":"20161023092648.12086-15-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 14/19] read-cache: touch shared index files when used","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:43Z","receivedAt":"2016-10-23T09:28:01Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"When a split-index file is created, let's update the mtime of the\nshared index file that the split-index file is referencing.\n\nIn a following commit we will make shared index file expire\ndepending on their mtime, so updating the mtime makes sure that\nthe shared index file will not be deleted soon.\n\nSigned-off-by: Christian Couder \n---\n read-cache.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex a91fabe..3aeff77 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2268,6 +2268,12 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \t\tint ret = write_shared_index(istate, lock, flags);\n \t\tif (ret)\n \t\t\treturn ret;\n+\t} else {\n+\t\t/* Signal that the shared index is used */\n+\t\tconst char *shared_index = git_path(\"sharedindex.%s\",\n+\t\t\t\t\t\t sha1_to_hex(si->base_sha1));\n+\t\tif (!check_and_freshen_file(shared_index, 1))\n+\t\t\twarning(\"could not freshen '%s'\", shared_index);\n \t}\n \n \treturn write_split_index(istate, lock, flags);\n-- \n2.10.1.462.g7e1e03a\n\n"},{"id":"304748","messageId":"20161023092648.12086-7-chriscool@tuxfamily.org","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"[PATCH v1 06/19] t1700: add tests for core.splitIndex","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-23T09:26:35Z","receivedAt":"2016-10-23T09:28:05Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n t/t1700-split-index.sh | 37 +++++++++++++++++++++++++++++++++++++\n 1 file changed, 37 insertions(+)\n\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex 292a072..db8c39f 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -200,4 +200,41 @@ EOF\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'set core.splitIndex config variable to true' '\n+\tgit config core.splitIndex true &&\n+\t: >three &&\n+\tgit update-index --add three &&\n+\tgit ls-files --stage >ls-files.actual &&\n+\tcat >ls-files.expect <actual &&\n+\tcat >expect <ls-files.actual &&\n+\tcat >ls-files.expect <actual &&\n+\tcat >expect < When writing a new split-index and there is a big number of cache\n> entries in the split-index compared to the shared index, it is a\n> good idea to regenerate the shared index.\n> \n> By default when the ratio reaches 20%, we will push back all\n> the entries from the split-index into a new shared index file\n> instead of just creating a new split-index file.\n> \n> The threshold can be configured using the\n> \"splitIndex.maxPercentChange\" config variable.\n> \n> We need to adjust the existing tests in t1700 by setting\n> \"splitIndex.maxPercentChange\" to 100 at the beginning of t1700,\n> as the existing tests are assuming that the shared index is\n> regenerated only when `git update-index --split-index` is used.\n> \n> Signed-off-by: Christian Couder \n> ---\n> read-cache.c | 33 ++++++++++++++++++++++++++++++++-\n> t/t1700-split-index.sh | 1 +\n> 2 files changed, 33 insertions(+), 1 deletion(-)\n> \n> diff --git a/read-cache.c b/read-cache.c\n> index bb53823..a91fabe 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2216,6 +2216,36 @@ static int write_shared_index(struct index_state *istate,\n> \treturn ret;\n> }\n> \n> +static const int default_max_percent_split_change = 20;\n> +\n> +int too_many_not_shared_entries(struct index_state *istate)\n\nThis function is a file-loacal symbol; could you please make it\na static function.\n\nThanks.\n\nATB,\nRamsay Jones\n"},{"id":"304785","messageId":"xmqq8ttd7h8g.fsf@gitster.mtv.corp.google.com","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v1 00/19] Add configuration options for split-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-24T18:07:59Z","receivedAt":"2016-10-24T18:08:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> The design is similar as the previous work that introduced\n> \"core.untrackedCache\". \n>\n> The new \"core.splitIndex\" configuration option can be either true,\n> false or undefined which is the default.\n>\n> When it is true, the split index is created, if it does not already\n> exists, when the index is read. When it is false, the split index is\n> removed if it exists, when the index is read. Otherwise it is left as\n> is.\n\nI admit I haven't thought it through, but this sounds OK.\n\n> Along with this new configuration variable, the two following options\n> are also introduced:\n>\n> - splitIndex.maxPercentChange\n>\n> This is to avoid having too many changes accumulating in the split\n> index while in split index mode. The git-update-index\n> documentation says:\n>\n> \tIf split-index mode is already enabled and `--split-index` is\n> \tgiven again, all changes in $GIT_DIR/index are pushed back to\n> \tthe shared index file.\n>\n> but it is probably better to not expect the user to think about it\n> and to have a mechanism that pushes back all changes to the shared\n> index file automatically when some threshold is reached.\n>\n> The default threshold is when the number of entries in the split\n> index file reaches 20% (by default) of the number of entries in\n> the shared index file. The new \"splitIndex.maxPercentChange\"\n> config option lets people tweak this value.\n\nOK.\n\n> - splitIndex.sharedIndexExpire\n>\n> To make sure that old sharedindex files are eventually removed\n> when a new one has been created, we \"touch\" the shared index file\n> every time it is used by a new split index file. Then we can\n> delete shared indexes with an mtime older than one week (by\n> default), when we create a new shared index file. The new\n> \"splitIndex.sharedIndexExpire\" config option lets people tweak\n> this grace period.\n\nI do not quite understand this justification. Doesn't each of the\n\"this hold only changes since the base index file\" files have a\nbackpointer that names the base index file it is a delta against?\n\nIs it safe to remove a base index file when there is still a split\nindex file that points at it? \n\nIOW, I do not see why it can be safe for the expiration decision to\nbe based on timestamp (I would understand it if it were based on a\nrefcnt, though).\n\n\n"},{"id":"304822","messageId":"CACsJy8BBwWRGSyJDYQ7THj7quu4=T1xT_-KojQd45Vye4Kgcng@mail.gmail.com","threadId":"44361","inReplyTo":"xmqq8ttd7h8g.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 00/19] Add configuration options for split-index","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-25T09:30:25Z","receivedAt":"2016-10-25T09:31:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Oct 25, 2016 at 1:07 AM, Junio C Hamano wrote:\n>> - splitIndex.sharedIndexExpire\n>>\n>> To make sure that old sharedindex files are eventually removed\n>> when a new one has been created, we \"touch\" the shared index file\n>> every time it is used by a new split index file. Then we can\n>> delete shared indexes with an mtime older than one week (by\n>> default), when we create a new shared index file. The new\n>> \"splitIndex.sharedIndexExpire\" config option lets people tweak\n>> this grace period.\n>\n> I do not quite understand this justification. Doesn't each of the\n> \"this hold only changes since the base index file\" files have a\n> backpointer that names the base index file it is a delta against?\n\nYes, but the shared file does not have pointers to all the files that\nneed it, which could be more than one. We know one of them,\n$GIT_DIR/index, and possibly $GIT_DIR/index.lock too. But those files\npeople generate manually and refer to them with $GIT_INDEX_FILE, we\ncan't know where they are.\n\n> Is it safe to remove a base index file when there is still a split\n> index file that points at it?\n>\n> IOW, I do not see why it can be safe for the expiration decision to\n> be based on timestamp (I would understand it if it were based on a\n> refcnt, though).\n\nProblem is we can't maintain these ref counts cheap and simple. We\ndon't want to update sharedindex file every time somebody references\nto it (or stops referencing to it) because that defeats the purpose of\nsplitting it out and not touching it any more. Adding a separate file\nfor ref count could work, but it gets complex, especially when we\nthink about race condition at update time.\n\nTimestamps allow us to say, ok this base index file has not been read\nby anybody for N+ hours (or better, days), it's most likely not\nreferenced by any temporary index files (including\n$GIT_DIR/index.lock) anymore because those files, by the definition of\n\"temporary\", must be gone by now. We should definitely check and make\nsure the file $GIT_DIR/index points to still exist. I'm going to read\nthe series now, so I don't know if the previous sentence is true.\n\nIt will probably be harder to handle race condition at updating\n$GIT_DIR/index, which could be avoided by a sufficiently long grace\nperiod with timestamps.\n-- \nDuy\n"},{"id":"304824","messageId":"CACsJy8DPt3EJoSTVEZFbH6xXbh78MbLZ4h+50K4eoFxPYSaN=Q@mail.gmail.com","threadId":"44361","inReplyTo":"20161023092648.12086-4-chriscool@tuxfamily.org","subject":"Re: [PATCH v1 03/19] split-index: add {add,remove}_split_index() functions","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-25T09:58:05Z","receivedAt":"2016-10-25T09:58:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder\n wrote:\n> +void remove_split_index(struct index_state *istate)\n> +{\n> + if (istate->split_index) {\n> + /*\n> + * can't discard_split_index(&the_index); because that\n> + * will destroy split_index->base->cache[], which may\n> + * be shared with the_index.cache[]. So yeah we're\n> + * leaking a bit here.\n\nIn the context of update-index, this is a one-time thing and leaking\nis tolerable. But because it becomes a library function now, this leak\ncan become more serious, I think.\n\nThe only other (indirect) caller is read_index_from() so probably not\nbad most of the time (we read at the beginning of a command only).\nsequencer.c may discard and re-read the index many times though,\nleaking could be visible there.\n\n> + */\n> + istate->split_index = NULL;\n> + istate->cache_changed |= SOMETHING_CHANGED;\n> + }\n> +}\n-- \nDuy\n"},{"id":"304825","messageId":"CACsJy8Br2q0aadTFjkNgb=oN8nSzbkWJEK7bCCgr7v-oOZtrSA@mail.gmail.com","threadId":"44361","inReplyTo":"20161023092648.12086-6-chriscool@tuxfamily.org","subject":"Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-25T10:00:45Z","receivedAt":"2016-10-25T10:01:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder\n wrote:\n> When users are using `git update-index --(no-)split-index`, they\n> may expect the split-index feature to be used or not according to\n> the option they just used, but this might not be the case if the\n> new \"core.splitIndex\" config variable has been set. In this case\n> let's warn about what will happen and why.\n>\n> Signed-off-by: Christian Couder \n> ---\n> builtin/update-index.c | 11 ++++++++++-\n> 1 file changed, 10 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/update-index.c b/builtin/update-index.c\n> index b75ea03..a14dbf2 100644\n> --- a/builtin/update-index.c\n> +++ b/builtin/update-index.c\n> @@ -1098,12 +1098,21 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n> }\n>\n> if (split_index > 0) {\n> + if (git_config_get_split_index() == 0)\n> + warning(\"core.splitIndex is set to false; \"\n> + \"remove or change it, if you really want to \"\n> + \"enable split index\");\n\nWrap this string and the one below with _() so they can be translated.\n\n> if (the_index.split_index)\n> the_index.cache_changed |= SPLIT_INDEX_ORDERED;\n> else\n> add_split_index(&the_index);\n> - } else if (!split_index)\n> + } else if (!split_index) {\n> + if (git_config_get_split_index() == 1)\n> + warning(\"core.splitIndex is set to true; \"\n> + \"remove or change it, if you really want to \"\n> + \"disable split index\");\n> remove_split_index(&the_index);\n> + }\n>\n> switch (untracked_cache) {\n> case UC_UNSPECIFIED:\n> --\n> 2.10.1.462.g7e1e03a\n-- \nDuy\n"},{"id":"304826","messageId":"CACsJy8A0djR6=s0AY0tzVehYY5b1-o11uRsFdGtOUCeu4Z6Xjw@mail.gmail.com","threadId":"44361","inReplyTo":"20161023092648.12086-10-chriscool@tuxfamily.org","subject":"Re: [PATCH v1 09/19] config: add git_config_get_max_percent_split_change()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-25T10:06:05Z","receivedAt":"2016-10-25T10:06:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder\n wrote:\n> This new function will be used in a following commit to get the\n> +int git_config_get_max_percent_split_change(void)\n> +{\n> + int val = -1;\n> +\n> + if (!git_config_get_int(\"splitindex.maxpercentchange\", &val)) {\n> + if (0 <= val && val <= 100)\n> + return val;\n> +\n> + error(\"splitindex.maxpercentchange value '%d' \"\n\nWe should keep camelCase form for easy reading. And wrap this string with _().\n\n> + \"should be between 0 and 100\", val);\n\nI wonder if anybody would try to put 12.3 here and confused by the\nerror message, because 0 <= 12.3 <= 100, but it's not an integer..\nAh.. never mind, die_bad_number() would be called first in this case\nwith a loud and clear complaint.\n\n> + return -1;\n> + }\n> +\n> + return -1; /* default value */\n-- \nDuy\n"},{"id":"304827","messageId":"CACsJy8CfQ6d3Q74W4rm=rJD69EAzuUe7PdrW-5NDo0vHuDSNpw@mail.gmail.com","threadId":"44361","inReplyTo":"20161023092648.12086-11-chriscool@tuxfamily.org","subject":"Re: [PATCH v1 10/19] read-cache: regenerate shared index if necessary","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-25T10:16:04Z","receivedAt":"2016-10-25T10:16:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder\n wrote:\n> @@ -2233,7 +2263,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n> if ((v & 15) < 6)\n> istate->cache_changed |= SPLIT_INDEX_ORDERED;\n> }\n> - if (istate->cache_changed & SPLIT_INDEX_ORDERED) {\n> + if (istate->cache_changed & SPLIT_INDEX_ORDERED ||\n> + too_many_not_shared_entries(istate)) {\n\nIt's probably safer to keep this piece unchanged and add this\nsomewhere before it\n\nif (too_many_not_shared_entries(istate))\n istate->cache_changed |= SPLIT_INDEX_ORDERED;\n\nWe could keep cache_changed consistent until the end this way.\n\n> int ret = write_shared_index(istate, lock, flags);\n> if (ret)\n> return ret;\n> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n> index db8c39f..507a1dd 100755\n> --- a/t/t1700-split-index.sh\n> +++ b/t/t1700-split-index.sh\n> @@ -8,6 +8,7 @@ test_description='split index mode tests'\n> sane_unset GIT_TEST_SPLIT_INDEX\n>\n> test_expect_success 'enable split index' '\n> + git config splitIndex.maxPercentChange 100 &&\n\nAn alternative name might be splitThreshold. I don't know, maybe\nmaxPercentChange is better.\n\n> git update-index --split-index &&\n> test-dump-split-index .git/index >actual &&\n> indexversion=$(test-index-version <.git/index) &&\n> --\n> 2.10.1.462.g7e1e03a\n>\n\n\n\n-- \nDuy\n"},{"id":"304828","messageId":"CACsJy8As2o-ZDXMRWeebpXiWUrDMLaXC2H1R+OMbhAMmM8V_wg@mail.gmail.com","threadId":"44361","inReplyTo":"20161023092648.12086-15-chriscool@tuxfamily.org","subject":"Re: [PATCH v1 14/19] read-cache: touch shared index files when used","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-25T10:26:02Z","receivedAt":"2016-10-25T10:26:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder\n wrote:\n> @@ -2268,6 +2268,12 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n\nDoing this in read_index_from() would keep the shared file even more\n\"fresher\" since read happens a lot more often than write. But I think\nour main concern is not the temporary index files created by the user\nscripts, but $GIT_DIR/index.lock (make sure we don't accidentally\ndelete its shared file before it gets renamed to $GIT_DIR/index). For\nthis case, I think refreshing in write_locked_index is enough.\n\n> int ret = write_shared_index(istate, lock, flags);\n> if (ret)\n> return ret;\n> + } else {\n> + /* Signal that the shared index is used */\n> + const char *shared_index = git_path(\"sharedindex.%s\",\n> + sha1_to_hex(si->base_sha1));\n> + if (!check_and_freshen_file(shared_index, 1))\n> + warning(\"could not freshen '%s'\", shared_index);\n\n_()\n-- \nDuy\n"},{"id":"304831","messageId":"CACsJy8Be-_n_5Aen2-7sod=wkCbXCGxoakf_H-1Cfb4Mg=T9QA@mail.gmail.com","threadId":"44361","inReplyTo":"20161023092648.12086-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v1 00/19] Add configuration options for split-index","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-25T10:52:56Z","receivedAt":"2016-10-25T10:53:47Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder\n wrote:\n> Goal\n> ~~~~\n>\n> We want to make it possible to use the split-index feature\n> automatically by just setting a new \"core.splitIndex\" configuration\n> variable to true.\n\nThanks. This definitely should help make split index a lot more\nconvenient for day-to-day use. The series looks ok (well, except the\npruning logic being discussed elsewhere in this thread, but I still\nthink it's a good strategy).\n\n> This can be valuable as split-index can help significantly speed up\n> `git rebase` especially along with the work to libify `git apply`\n> that has been recently merged to master\n> (see https://github.com/git/git/commit/81358dc238372793b1590efa149cc1581d1fbd98).\n\nI remember this. Since the apply libification work has landed, we can\nnow think about not writing index out after every apply step, which\ngives the same (or more) speed up without the complication of\nsplit-index. But if split-index is good enough for you, we can leave\nit for another time.\n-- \nDuy\n"},{"id":"304832","messageId":"CACsJy8C4Qd76LSYxk9BJtN3n3Knv_RCDgBOO-ybkNHFZK61ahQ@mail.gmail.com","threadId":"44361","inReplyTo":"20161023092648.12086-17-chriscool@tuxfamily.org","subject":"Re: [PATCH v1 16/19] read-cache: unlink old sharedindex files","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-25T10:43:46Z","receivedAt":"2016-10-25T10:56:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder\n wrote:\n> +static int can_delete_shared_index(const char *shared_sha1_hex)\n> +{\n> + struct stat st;\n> + unsigned long expiration;\n> + const char *shared_index = git_path(\"sharedindex.%s\", shared_sha1_hex);\n> +\n> + /* Check timestamp */\n> + expiration = get_shared_index_expire_date();\n> + if (!expiration)\n> + return 0;\n> + if (stat(shared_index, &st))\n> + return error_errno(\"could not stat '%s\", shared_index);\n> + if (st.st_mtime > expiration)\n\nI wonder if we should check ctime too, in case mtime is not reliable\n(and ctime is less likely to be manipulated by user), just for extra\nsafety. If (st.st_mtime > expiration || st.st_ctime > expiration).\n\n> + return 0;\n> +\n> + return 1;\n> +}\n> +\n> +static void clean_shared_index_files(const char *current_hex)\n> +{\n> + struct dirent *de;\n> + DIR *dir = opendir(get_git_dir());\n> +\n> + if (!dir) {\n> + error_errno(\"unable to open git dir: %s\", get_git_dir());\n\n_()\n\n> + return;\n\nOr just do \"return error_errno(...)\". The caller can ignore the return\nvalue for now.\n\n> + }\n> +\n> + while ((de = readdir(dir)) != NULL) {\n> + const char *sha1_hex;\n> + if (!skip_prefix(de->d_name, \"sharedindex.\", &sha1_hex))\n> + continue;\n> + if (!strcmp(sha1_hex, current_hex))\n> + continue;\n\nYeah.. make sure that the shared index linked to $GIT_DIR/index stay,\neven if mtime is screwed up. I wonder if we should have the same\ntreatment for $GIT_DIR/index.lock though, as an extra safety measure.\nIf you call this function in write_locked_index, then\n$GIT_DIR/index.lock is definitely there. Hmm.. maybe it _is_\ncurrent_hex, current_hex is not the value from $GIT_DIR/index...\n\n> + if (can_delete_shared_index(sha1_hex) > 0 &&\n> + unlink(git_path(\"%s\", de->d_name)))\n> + error_errno(\"unable to unlink: %s\", git_path(\"%s\", de->d_name));\n\n_()\n\n> static int write_shared_index(struct index_state *istate,\n> @@ -2211,8 +2269,11 @@ static int write_shared_index(struct index_state *istate,\n> }\n> ret = rename_tempfile(&temporary_sharedindex,\n> git_path(\"sharedindex.%s\", sha1_to_hex(si->base->sha1)));\n> - if (!ret)\n> + if (!ret) {\n> hashcpy(si->base_sha1, si->base->sha1);\n> + clean_shared_index_files(sha1_to_hex(si->base->sha1));\n\nThis operation is technically garbage collection and should belong to\n\"git gc --auto\", which is already called automatically in a few\nplaces. Is it not called often enough that we need to do the cleaning\nup right after a new shared index is created?\n-- \nDuy\n"},{"id":"304853","messageId":"xmqqk2cws5t6.fsf@gitster.mtv.corp.google.com","threadId":"44361","inReplyTo":"CACsJy8BBwWRGSyJDYQ7THj7quu4=T1xT_-KojQd45Vye4Kgcng@mail.gmail.com","subject":"Re: [PATCH v1 00/19] Add configuration options for split-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-25T17:21:25Z","receivedAt":"2016-10-25T17:21:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> ... But those files\n> people generate manually and refer to them with $GIT_INDEX_FILE, we\n> can't know where they are.\n\nThen we probably should stop doing that, i.e. disable split-index\nautomatically for these temporary ones, perhaps, and even with\nChristian's series which allows use of split index on the primary\none easier (which is a good idea), make sure we don't auto split the\ntemporary ones the users create?\n\n> Timestamps allow us to say, ok this base index file has not been read\n> by anybody for N+ hours (or better, days), it's most likely not\n> referenced by any temporary index files (including\n> $GIT_DIR/index.lock) anymore because those files, by the definition of\n> \"temporary\", must be gone by now....\n\nand if we guessed wrong, users will have a \"temporary index\" that\nthey meant to keep for longer term that is now broken here. I am\nnot sure if that risk is worth taking.\n\n"},{"id":"304882","messageId":"CACsJy8Ba0BY=pZwrKf5rcD5AaZ3YyKh9=ENKkj7hHpTqh00OnA@mail.gmail.com","threadId":"44361","inReplyTo":"xmqqk2cws5t6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 00/19] Add configuration options for split-index","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-26T09:25:44Z","receivedAt":"2016-10-26T09:26:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Oct 26, 2016 at 12:21 AM, Junio C Hamano wrote:\n>> Timestamps allow us to say, ok this base index file has not been read\n>> by anybody for N+ hours (or better, days), it's most likely not\n>> referenced by any temporary index files (including\n>> $GIT_DIR/index.lock) anymore because those files, by the definition of\n>> \"temporary\", must be gone by now....\n>\n> and if we guessed wrong, users will have a \"temporary index\" that\n> they meant to keep for longer term that is now broken here. I am\n> not sure if that risk is worth taking.\n\nEven if we ignore user index files (by forcing them all to be stored\nin one piece), there is a problem with the special temporary file\nindex.lock, which must use split-index because it will become the new\nindex. Handling race conditions could be tricky with ref counting.\nTimestamps help in this regard.\n-- \nDuy\n"},{"id":"304897","messageId":"xmqqeg33ccjj.fsf@gitster.mtv.corp.google.com","threadId":"44361","inReplyTo":"CACsJy8Ba0BY=pZwrKf5rcD5AaZ3YyKh9=ENKkj7hHpTqh00OnA@mail.gmail.com","subject":"Re: [PATCH v1 00/19] Add configuration options for split-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-26T16:14:56Z","receivedAt":"2016-10-26T16:15:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Wed, Oct 26, 2016 at 12:21 AM, Junio C Hamano wrote:\n>\n> Even if we ignore user index files (by forcing them all to be stored\n> in one piece), there is a problem with the special temporary file\n> index.lock, which must use split-index because it will become the new\n> index. Handling race conditions could be tricky with ref counting.\n> Timestamps help in this regard.\n\nI actually think using the split-index only for the $GIT_DIR/index,\nthe primary one, and using the full index for others is a bad idea,\nas we use temporary index ourselves when making partial commits,\nwhich happens quite often.\n\nSo time-based GC it is.\n\nI actually do not think index.lock is a problem, but is a solution,\nif we only limit the split-index to the primary one (we can have at\nmost one, so we can GC the stale shared ones while holding the\nlock).\n\nBut that is no longer important.\n\n\n"},{"id":"304958","messageId":"CAP8UFD0uiQ5RFYPqGizUqPb-QoTHjrJJmmj4ZXLyBnpJcoyGKA@mail.gmail.com","threadId":"44361","inReplyTo":"CACsJy8BPe085Qu4GpJ3MLTPeZM_7pbVrX31enQrGQn0oyjSrdg@mail.gmail.com","subject":"Re: [PATCH v1 16/19] read-cache: unlink old sharedindex files","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-27T12:14:28Z","receivedAt":"2016-10-27T14:20:15Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Oct 27, 2016 at 12:25 PM, Duy Nguyen wrote:\n> On Tue, Oct 25, 2016 at 5:43 PM, Duy Nguyen wrote:\n>>> static int write_shared_index(struct index_state *istate,\n>>> @@ -2211,8 +2269,11 @@ static int write_shared_index(struct index_state *istate,\n>>> }\n>>> ret = rename_tempfile(&temporary_sharedindex,\n>>> git_path(\"sharedindex.%s\", sha1_to_hex(si->base->sha1)));\n>>> - if (!ret)\n>>> + if (!ret) {\n>>> hashcpy(si->base_sha1, si->base->sha1);\n>>> + clean_shared_index_files(sha1_to_hex(si->base->sha1));\n>>\n>> This operation is technically garbage collection and should belong to\n>> \"git gc --auto\", which is already called automatically in a few\n>> places. Is it not called often enough that we need to do the cleaning\n>> up right after a new shared index is created?\n>\n> Christian, if we assume to go with Junio's suggestion to disable\n> split-index on temporary files, the only files left we have to take\n> care of are index and index.lock. I believe pruning here in this code\n> will have an advantage over in \"git gc --auto\" because when this is\n> executed, we know we're holding index.lock, so nobody else is updating\n> the index, it's race-free. All we need to do is peek in $GIT_DIR/index\n> to see what shared index file it requires and keep it alive too, the\n> remaining of shared index files can be deleted safely. We don't even\n> need to fall back to mtime.\n>\n> git-gc just can't match this because while it's running, somebody else\n> may be updating $GIT_DIR/index. Handling races would be a lot harder.\n\nYeah, I agree that if we disable split-index on temporary files and on\ncommands like \"git read-tree --index-output=\" then it is the\nright thing to do it in write_shared_index(). (In fact when I wrote\nthe previous RFC series I didn't think about those special cases, and\nthat was the reason why I did it like this. So I just need to go back\nto the implementation that was in the previous RFC series.)\n\nI am just still wondering if disabling split-index on temporary files\ncould not have a bad performance impact for some use cases, but I\nguess we could always come back to problem again if that happens.\n\nThanks,\nChristian.\n"},{"id":"304962","messageId":"CACsJy8BPe085Qu4GpJ3MLTPeZM_7pbVrX31enQrGQn0oyjSrdg@mail.gmail.com","threadId":"44361","inReplyTo":"CACsJy8C4Qd76LSYxk9BJtN3n3Knv_RCDgBOO-ybkNHFZK61ahQ@mail.gmail.com","subject":"Re: [PATCH v1 16/19] read-cache: unlink old sharedindex files","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-27T10:25:38Z","receivedAt":"2016-10-27T14:44:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Oct 25, 2016 at 5:43 PM, Duy Nguyen wrote:\n>> static int write_shared_index(struct index_state *istate,\n>> @@ -2211,8 +2269,11 @@ static int write_shared_index(struct index_state *istate,\n>> }\n>> ret = rename_tempfile(&temporary_sharedindex,\n>> git_path(\"sharedindex.%s\", sha1_to_hex(si->base->sha1)));\n>> - if (!ret)\n>> + if (!ret) {\n>> hashcpy(si->base_sha1, si->base->sha1);\n>> + clean_shared_index_files(sha1_to_hex(si->base->sha1));\n>\n> This operation is technically garbage collection and should belong to\n> \"git gc --auto\", which is already called automatically in a few\n> places. Is it not called often enough that we need to do the cleaning\n> up right after a new shared index is created?\n\nChristian, if we assume to go with Junio's suggestion to disable\nsplit-index on temporary files, the only files left we have to take\ncare of are index and index.lock. I believe pruning here in this code\nwill have an advantage over in \"git gc --auto\" because when this is\nexecuted, we know we're holding index.lock, so nobody else is updating\nthe index, it's race-free. All we need to do is peek in $GIT_DIR/index\nto see what shared index file it requires and keep it alive too, the\nremaining of shared index files can be deleted safely. We don't even\nneed to fall back to mtime.\n\ngit-gc just can't match this because while it's running, somebody else\nmay be updating $GIT_DIR/index. Handling races would be a lot harder.\n-- \nDuy\n"},{"id":"304967","messageId":"xmqq4m3x93e1.fsf@gitster.mtv.corp.google.com","threadId":"44361","inReplyTo":"CACsJy8BPe085Qu4GpJ3MLTPeZM_7pbVrX31enQrGQn0oyjSrdg@mail.gmail.com","subject":"Re: [PATCH v1 16/19] read-cache: unlink old sharedindex files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-27T16:13:10Z","receivedAt":"2016-10-27T16:13:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> Christian, if we assume to go with Junio's suggestion to disable\n> split-index on temporary files, the only files left we have to take\n> care of are index and index.lock. I believe pruning here in this code\n> will have an advantage over in \"git gc --auto\" because when this is\n> executed, we know we're holding index.lock, so nobody else is updating\n> the index, it's race-free.\n>\n> All we need to do is peek in $GIT_DIR/index\n> to see what shared index file it requires and keep it alive too, the\n> remaining of shared index files can be deleted safely. We don't even\n> need to fall back to mtime.\n\nYes, that exactly was why I wondered if we can afford to limit\nsplitting only to the primary index, because it makes things a\nlot simpler.\n\nBut I suspect that temporary index is where split-index shines most,\ne.g. while creating a partial commit. The mechanism penalizes the\nread performance by making the format more complex in order to favor\nthe write performance, which is very much suited for temporary one\nthat is read only once after it is written before it gets discarded\n(on the other hand, splitting the primary index will penalize reads\nthat happen a lot more than writes).\n\nWhile I still find it attractive at the conceptual level to limit\nsplitting only to the primary index for the resulting simplicity,\nI doubt it is a good way to go, as I meant to say in\n\n\n> git-gc just can't match this because while it's running, somebody else\n> may be updating $GIT_DIR/index. Handling races would be a lot harder.\n\nIt could attempt to take a lock on the primary index while it runs,\nand refrain to do anything if it can't take the lock (\"gc --auto\"\nmay want to silently retry), and then the race is no longer an\nissue, no?\n"},{"id":"305150","messageId":"CACsJy8BU4EH=yzkEFzTPJSQ61HMJWVperjf67L9JK9bqeUEC4A@mail.gmail.com","threadId":"44361","inReplyTo":"xmqq4m3x93e1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 16/19] read-cache: unlink old sharedindex files","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-29T03:30:27Z","receivedAt":"2016-10-29T03:31:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Oct 27, 2016 at 11:13 PM, Junio C Hamano wrote:\n>> git-gc just can't match this because while it's running, somebody else\n>> may be updating $GIT_DIR/index. Handling races would be a lot harder.\n>\n> It could attempt to take a lock on the primary index while it runs,\n> and refrain to do anything if it can't take the lock (\"gc --auto\"\n> may want to silently retry), and then the race is no longer an\n> issue, no?\n\nNo, I thought of that. But if gc is holding the lock, another program\nthat wants to update the index may fail. And git-gc is supposed to be\nnon-intrusive\n-- \nDuy\n"},{"id":"305171","messageId":"CAP8UFD3hNEU_UeVizU6SVJTt4hqJPag9XWqZOM3FKCGJZXOthg@mail.gmail.com","threadId":"44361","inReplyTo":"CACsJy8DPt3EJoSTVEZFbH6xXbh78MbLZ4h+50K4eoFxPYSaN=Q@mail.gmail.com","subject":"Re: [PATCH v1 03/19] split-index: add {add,remove}_split_index() functions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-29T22:06:30Z","receivedAt":"2016-10-29T22:06:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Oct 25, 2016 at 11:58 AM, Duy Nguyen wrote:\n> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder\n> wrote:\n>> +void remove_split_index(struct index_state *istate)\n>> +{\n>> + if (istate->split_index) {\n>> + /*\n>> + * can't discard_split_index(&the_index); because that\n>> + * will destroy split_index->base->cache[], which may\n>> + * be shared with the_index.cache[]. So yeah we're\n>> + * leaking a bit here.\n>\n> In the context of update-index, this is a one-time thing and leaking\n> is tolerable. But because it becomes a library function now, this leak\n> can become more serious, I think.\n>\n> The only other (indirect) caller is read_index_from() so probably not\n> bad most of the time (we read at the beginning of a command only).\n> sequencer.c may discard and re-read the index many times though,\n> leaking could be visible there.\n\nSo is it enough to check if split_index->base->cache[] is shared with\nthe_index.cache[] and then decide if discard_split_index(&the_index)\nshould be called?\n\nThanks,\nChristian.\n"},{"id":"305172","messageId":"CAP8UFD2SX+wUg0=+cfRY5oo5SEigKU-AjrQg0_zsYsfXdb53HA@mail.gmail.com","threadId":"44361","inReplyTo":"CACsJy8Br2q0aadTFjkNgb=oN8nSzbkWJEK7bCCgr7v-oOZtrSA@mail.gmail.com","subject":"Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-29T22:19:59Z","receivedAt":"2016-10-29T22:20:08Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Oct 25, 2016 at 12:00 PM, Duy Nguyen wrote:\n> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder\n> wrote:\n>> When users are using `git update-index --(no-)split-index`, they\n>> may expect the split-index feature to be used or not according to\n>> the option they just used, but this might not be the case if the\n>> new \"core.splitIndex\" config variable has been set. In this case\n>> let's warn about what will happen and why.\n>>\n>> Signed-off-by: Christian Couder \n>> ---\n>> builtin/update-index.c | 11 ++++++++++-\n>> 1 file changed, 10 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/update-index.c b/builtin/update-index.c\n>> index b75ea03..a14dbf2 100644\n>> --- a/builtin/update-index.c\n>> +++ b/builtin/update-index.c\n>> @@ -1098,12 +1098,21 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n>> }\n>>\n>> if (split_index > 0) {\n>> + if (git_config_get_split_index() == 0)\n>> + warning(\"core.splitIndex is set to false; \"\n>> + \"remove or change it, if you really want to \"\n>> + \"enable split index\");\n>\n> Wrap this string and the one below with _() so they can be translated.\n\nOk, it will be in the next version.\n"},{"id":"305173","messageId":"CAP8UFD3kPVRdubHEsbKbUuxhMDjWNz6y_ANLBB+4Au76PJ3U6w@mail.gmail.com","threadId":"44361","inReplyTo":"CACsJy8A0djR6=s0AY0tzVehYY5b1-o11uRsFdGtOUCeu4Z6Xjw@mail.gmail.com","subject":"Re: [PATCH v1 09/19] config: add git_config_get_max_percent_split_change()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-29T22:24:54Z","receivedAt":"2016-10-29T22:25:00Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Oct 25, 2016 at 12:06 PM, Duy Nguyen wrote:\n> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder\n> wrote:\n>> This new function will be used in a following commit to get the\n>> +int git_config_get_max_percent_split_change(void)\n>> +{\n>> + int val = -1;\n>> +\n>> + if (!git_config_get_int(\"splitindex.maxpercentchange\", &val)) {\n>> + if (0 <= val && val <= 100)\n>> + return val;\n>> +\n>> + error(\"splitindex.maxpercentchange value '%d' \"\n>\n> We should keep camelCase form for easy reading. And wrap this string with _().\n\nOk, it will be in the next version.\n\nThanks,\nChristian.\n"},{"id":"305174","messageId":"CAP8UFD28w6+kzsq3wdu-iKfYsLc4w6kNVj0Fk+aB6d3g3AoUzQ@mail.gmail.com","threadId":"44361","inReplyTo":"c30212bd-c454-f1a1-d01c-d6a12d20d17d@ramsayjones.plus.com","subject":"Re: [PATCH v1 10/19] read-cache: regenerate shared index if necessary","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-29T22:40:19Z","receivedAt":"2016-10-29T22:40:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Oct 23, 2016 at 6:07 PM, Ramsay Jones\n wrote:\n>>\n>> +int too_many_not_shared_entries(struct index_state *istate)\n>\n> This function is a file-loacal symbol; could you please make it\n> a static function.\n\nOk, it will be in the next version.\n\nThanks,\nChristian.\n"},{"id":"305175","messageId":"CAP8UFD1qTeppaW2NRWhAxz0tJ7EtwmiOjKUnQt8q89FdC8kXWQ@mail.gmail.com","threadId":"44361","inReplyTo":"CACsJy8CfQ6d3Q74W4rm=rJD69EAzuUe7PdrW-5NDo0vHuDSNpw@mail.gmail.com","subject":"Re: [PATCH v1 10/19] read-cache: regenerate shared index if necessary","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-29T22:58:45Z","receivedAt":"2016-10-29T22:58:51Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Oct 25, 2016 at 12:16 PM, Duy Nguyen wrote:\n> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder\n> wrote:\n>> @@ -2233,7 +2263,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n>> if ((v & 15) < 6)\n>> istate->cache_changed |= SPLIT_INDEX_ORDERED;\n>> }\n>> - if (istate->cache_changed & SPLIT_INDEX_ORDERED) {\n>> + if (istate->cache_changed & SPLIT_INDEX_ORDERED ||\n>> + too_many_not_shared_entries(istate)) {\n>\n> It's probably safer to keep this piece unchanged and add this\n> somewhere before it\n>\n> if (too_many_not_shared_entries(istate))\n> istate->cache_changed |= SPLIT_INDEX_ORDERED;\n>\n> We could keep cache_changed consistent until the end this way.\n\nOk, it will be in the next version.\n\n>> test_expect_success 'enable split index' '\n>> + git config splitIndex.maxPercentChange 100 &&\n>\n> An alternative name might be splitThreshold. I don't know, maybe\n> maxPercentChange is better.\n\nI think it is important to say that it is a percent in the name, so I\nprefer maxPercentChange.\n\nThanks,\nChristian.\n"},{"id":"305241","messageId":"xmqqlgx3owbg.fsf@gitster.mtv.corp.google.com","threadId":"44361","inReplyTo":"CACsJy8Br2q0aadTFjkNgb=oN8nSzbkWJEK7bCCgr7v-oOZtrSA@mail.gmail.com","subject":"Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-01T19:05:07Z","receivedAt":"2016-11-01T19:05:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> diff --git a/builtin/update-index.c b/builtin/update-index.c\n>> index b75ea03..a14dbf2 100644\n>> --- a/builtin/update-index.c\n>> +++ b/builtin/update-index.c\n>> @@ -1098,12 +1098,21 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n>> }\n>>\n>> if (split_index > 0) {\n>> + if (git_config_get_split_index() == 0)\n>> + warning(\"core.splitIndex is set to false; \"\n>> + \"remove or change it, if you really want to \"\n>> + \"enable split index\");\n>\n> Wrap this string and the one below with _() so they can be translated.\n\nTrue. \n\nI further wonder if a natural reaction from users after seeing this\nmessage is \"I do want to--what else would I use that option to run\nyou for? Just do as you are told, instead of telling me what to\ndo!\". Is this warning really a good idea, or shouldn't these places\nbe setting the configuration?\n\n>> if (the_index.split_index)\n>> the_index.cache_changed |= SPLIT_INDEX_ORDERED;\n>> else\n>> add_split_index(&the_index);\n>> - } else if (!split_index)\n>> + } else if (!split_index) {\n>> + if (git_config_get_split_index() == 1)\n>> + warning(\"core.splitIndex is set to true; \"\n>> + \"remove or change it, if you really want to \"\n>> + \"disable split index\");\n>> remove_split_index(&the_index);\n>> + }\n>>\n>> switch (untracked_cache) {\n>> case UC_UNSPECIFIED:\n>> --\n>> 2.10.1.462.g7e1e03a\n"},{"id":"305242","messageId":"xmqqh97rovx9.fsf@gitster.mtv.corp.google.com","threadId":"44361","inReplyTo":"CACsJy8A0djR6=s0AY0tzVehYY5b1-o11uRsFdGtOUCeu4Z6Xjw@mail.gmail.com","subject":"Re: [PATCH v1 09/19] config: add git_config_get_max_percent_split_change()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-01T19:13:38Z","receivedAt":"2016-11-01T19:13:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder\n> wrote:\n>> This new function will be used in a following commit to get the\n>> +int git_config_get_max_percent_split_change(void)\n>> +{\n>> + int val = -1;\n>> +\n>> + if (!git_config_get_int(\"splitindex.maxpercentchange\", &val)) {\n>> + if (0 <= val && val <= 100)\n>> + return val;\n>> +\n>> + error(\"splitindex.maxpercentchange value '%d' \"\n>\n> We should keep camelCase form for easy reading. And wrap this string with _().\n>\n>> + \"should be between 0 and 100\", val);\n>\n> I wonder if anybody would try to put 12.3 here and confused by the\n> error message, because 0 <= 12.3 <= 100, but it's not an integer..\n> Ah.. never mind, die_bad_number() would be called first in this case\n> with a loud and clear complaint.\n\nOK.\n\n>\n>> + return -1;\n\nPerhaps do the usual\n\n\treturn error(_(\"...\"));\n\nhere?\n\n>> + }\n>> +\n>> + return -1; /* default value */\n"},{"id":"305243","messageId":"xmqqd1ifovtf.fsf@gitster.mtv.corp.google.com","threadId":"44361","inReplyTo":"20161023092648.12086-12-chriscool@tuxfamily.org","subject":"Re: [PATCH v1 11/19] t1700: add tests for splitIndex.maxPercentChange","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-01T19:15:56Z","receivedAt":"2016-11-01T19:16:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Signed-off-by: Christian Couder \n> ---\n> t/t1700-split-index.sh | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 72 insertions(+)\n>\n> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n> index 507a1dd..f03addf 100755\n> --- a/t/t1700-split-index.sh\n> +++ b/t/t1700-split-index.sh\n> @@ -238,4 +238,76 @@ EOF\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'set core.splitIndex config variable to true' '\n> +\tgit config core.splitIndex true &&\n> +\t: >three &&\n> +\tgit update-index --add three &&\n> +\tBASE=$(test-dump-split-index .git/index | grep \"^base\") &&\n> +\ttest-dump-split-index .git/index | sed \"/^own/d\" >actual &&\n> +\tcat >expect < +$BASE\n> +replacements:\n> +deletions:\n> +EOF\n\nUsing <<-EOF lets us indent the above four lines with a horizontal\ntab to align with the remainder of this test_expect_success block,\nso let's do that.\n\n"},{"id":"305245","messageId":"xmqq8tt3ovnp.fsf@gitster.mtv.corp.google.com","threadId":"44361","inReplyTo":"20161023092648.12086-13-chriscool@tuxfamily.org","subject":"Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-01T19:19:22Z","receivedAt":"2016-11-01T19:19:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Signed-off-by: Christian Couder \n> ---\n> Documentation/config.txt | 13 +++++++++++++\n> 1 file changed, 13 insertions(+)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 96521a4..380eeb8 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2763,6 +2763,19 @@ showbranch.default::\n> \tThe default set of branches for linkgit:git-show-branch[1].\n> \tSee linkgit:git-show-branch[1].\n> \n> +splitIndex.maxPercentChange::\n> +\tWhen the split index feature is used, this specifies the\n> +\tpercent of entries the split index can contain compared to the\n> +\twhole number of entries in both the split index and the shared\n> +\tindex before a new shared index is written.\n> +\tThe value should be between 0 and 100. If the value is 0 then\n> +\ta new shared index is always written, if it is 100 a new\n> +\tshared index is never written.\n\nHmph. The early part of the description implies this will kick in\nonly when some other conditions (i.e. the bit in the index or the\nother configuration) are met, but if this disables the split index\nwhen it is set to 0, would we even need the other configuration\nvariable? IOW, perhaps we can do without core.splitIndex?\n\n> +\tBy default the value is 20, so a new shared index is written\n> +\tif the number of entries in the split index would be greater\n> +\tthan 20 percent of the total number of entries.\n> +\tSee linkgit:git-update-index[1].\n"},{"id":"305246","messageId":"xmqq4m3rovhb.fsf@gitster.mtv.corp.google.com","threadId":"44361","inReplyTo":"CACsJy8As2o-ZDXMRWeebpXiWUrDMLaXC2H1R+OMbhAMmM8V_wg@mail.gmail.com","subject":"Re: [PATCH v1 14/19] read-cache: touch shared index files when used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-01T19:23:12Z","receivedAt":"2016-11-01T19:23:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder\n> wrote:\n>> @@ -2268,6 +2268,12 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n>\n> Doing this in read_index_from() would keep the shared file even more\n> \"fresher\" since read happens a lot more often than write. But I think\n> our main concern is not the temporary index files created by the user\n> scripts, but $GIT_DIR/index.lock (make sure we don't accidentally\n> delete its shared file before it gets renamed to $GIT_DIR/index). For\n> this case, I think refreshing in write_locked_index is enough.\n\nAlso warning() is unwarranted.\n\nYou may be accessing somebody else's repository to help diagnose the\nissue without having any write access. Treat the utime() like the\nopportunistic index refresh done by \"git status\"---if we can write,\ngreat, but it is not a problem if we can't.\n\n>\n>> int ret = write_shared_index(istate, lock, flags);\n>> if (ret)\n>> return ret;\n>> + } else {\n>> + /* Signal that the shared index is used */\n>> + const char *shared_index = git_path(\"sharedindex.%s\",\n>> + sha1_to_hex(si->base_sha1));\n>> + if (!check_and_freshen_file(shared_index, 1))\n>> + warning(\"could not freshen '%s'\", shared_index);\n>\n> _()\n"},{"id":"305247","messageId":"xmqqziljngod.fsf@gitster.mtv.corp.google.com","threadId":"44361","inReplyTo":"20161023092648.12086-16-chriscool@tuxfamily.org","subject":"Re: [PATCH v1 15/19] config: add git_config_get_date_string() from gc.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-01T19:28:18Z","receivedAt":"2016-11-01T19:28:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> This function will be used in a following commit to get the expiration\n> time of the shared index files from the config, and it is generic\n> enough to be put in \"config.c\".\n\nIs it generic enough that a helper that sounds as if it can get any\ndate string dies if it is given a future date? I somehow doubt it.\n\nAt the minimum, it must be made clear that there is an artificial\nlimitation that the current set of callers find useful in cache.h as\na one-liner comment next to the added declaration. Then people with\nthe same need (i.e. they want to reject future timestamps) can\ndecide to use it, while others would stay away from it. \n\nIf you can come up with a better word to use to encode that\nartificial limitation in its name, renaming it is even better.\n"},{"id":"305276","messageId":"CAP8UFD18gcMJY7zjXw+ry6Kq2Foug9r0e=OVgZ_hcFkEVfnChA@mail.gmail.com","threadId":"44361","inReplyTo":"xmqqlgx3owbg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-11-01T23:00:48Z","receivedAt":"2016-11-01T23:00:56Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Nov 1, 2016 at 8:05 PM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>>> diff --git a/builtin/update-index.c b/builtin/update-index.c\n>>> index b75ea03..a14dbf2 100644\n>>> --- a/builtin/update-index.c\n>>> +++ b/builtin/update-index.c\n>>> @@ -1098,12 +1098,21 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n>>> }\n>>>\n>>> if (split_index > 0) {\n>>> + if (git_config_get_split_index() == 0)\n>>> + warning(\"core.splitIndex is set to false; \"\n>>> + \"remove or change it, if you really want to \"\n>>> + \"enable split index\");\n>>\n>> Wrap this string and the one below with _() so they can be translated.\n>\n> True.\n>\n> I further wonder if a natural reaction from users after seeing this\n> message is \"I do want to--what else would I use that option to run\n> you for? Just do as you are told, instead of telling me what to\n> do!\". Is this warning really a good idea, or shouldn't these places\n> be setting the configuration?\n\nIn 435ec090ec (config: add core.untrackedCache, 2016-01-27) we decided\nto just use warning() after discussing if we should instead set the\nconfiguration.\n"},{"id":"305280","messageId":"xmqqins6mzku.fsf@gitster.mtv.corp.google.com","threadId":"44361","inReplyTo":"CAP8UFD18gcMJY7zjXw+ry6Kq2Foug9r0e=OVgZ_hcFkEVfnChA@mail.gmail.com","subject":"Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-02T01:37:37Z","receivedAt":"2016-11-02T01:37:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n>>> Wrap this string and the one below with _() so they can be translated.\n>>\n>> True.\n>>\n>> I further wonder if a natural reaction from users after seeing this\n>> message is \"I do want to--what else would I use that option to run\n>> you for? Just do as you are told, instead of telling me what to\n>> do!\". Is this warning really a good idea, or shouldn't these places\n>> be setting the configuration?\n>\n> In 435ec090ec (config: add core.untrackedCache, 2016-01-27) we decided\n> to just use warning() after discussing if we should instead set the\n> configuration.\n\nYeah that seems to be the version that was committed. I then wonder\nif the users would naturally have a similar raction to that warning\nas well.\n"},{"id":"305347","messageId":"CAP8UFD2sVHS2J6ZXshDqbV_D5Ydht5cH3egYyUN5SZSaBRD9ZQ@mail.gmail.com","threadId":"44361","inReplyTo":"CACsJy8Be-_n_5Aen2-7sod=wkCbXCGxoakf_H-1Cfb4Mg=T9QA@mail.gmail.com","subject":"Re: [PATCH v1 00/19] Add configuration options for split-index","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-11-03T14:34:36Z","receivedAt":"2016-11-03T14:34:43Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Oct 25, 2016 at 12:52 PM, Duy Nguyen wrote:\n> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder\n> wrote:\n>> Goal\n>> ~~~~\n>>\n>> We want to make it possible to use the split-index feature\n>> automatically by just setting a new \"core.splitIndex\" configuration\n>> variable to true.\n>\n> Thanks. This definitely should help make split index a lot more\n> convenient for day-to-day use. The series looks ok (well, except the\n> pruning logic being discussed elsewhere in this thread, but I still\n> think it's a good strategy).\n\nThanks for your review.\n\n>> This can be valuable as split-index can help significantly speed up\n>> `git rebase` especially along with the work to libify `git apply`\n>> that has been recently merged to master\n>> (see https://github.com/git/git/commit/81358dc238372793b1590efa149cc1581d1fbd98).\n>\n> I remember this. Since the apply libification work has landed, we can\n> now think about not writing index out after every apply step, which\n> gives the same (or more) speed up without the complication of\n> split-index. But if split-index is good enough for you, we can leave\n> it for another time.\n\nYeah, I think they are separate things that can be done.\nWhen people do a lot of small rebases for example, then split-index\nwill speed up all the index writing, including the index writing at\nthe end of each rebase.\n"},{"id":"305428","messageId":"CAP8UFD1q0vpXNLG-WS_9E0qNjPzqf7iGkH0k4qH10HMfnanwag@mail.gmail.com","threadId":"44361","inReplyTo":"xmqqh97rovx9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 09/19] config: add git_config_get_max_percent_split_change()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-11-05T00:27:14Z","receivedAt":"2016-11-05T00:27:22Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Nov 1, 2016 at 8:13 PM, Junio C Hamano wrote:\n>\n>>> + return -1;\n>\n> Perhaps do the usual\n>\n> return error(_(\"...\"));\n>\n> here?\n\nOk, it will be in the next version.\n"},{"id":"305429","messageId":"CAP8UFD2H6rZNQqg1VGNkes65KAk-4P160rjQSjFY3NjgZydZmA@mail.gmail.com","threadId":"44361","inReplyTo":"xmqq8tt3ovnp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-11-05T00:45:04Z","receivedAt":"2016-11-05T00:45:11Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Nov 1, 2016 at 8:19 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> +splitIndex.maxPercentChange::\n>> + When the split index feature is used, this specifies the\n>> + percent of entries the split index can contain compared to the\n>> + whole number of entries in both the split index and the shared\n>> + index before a new shared index is written.\n>> + The value should be between 0 and 100. If the value is 0 then\n>> + a new shared index is always written, if it is 100 a new\n>> + shared index is never written.\n>\n> Hmph. The early part of the description implies this will kick in\n> only when some other conditions (i.e. the bit in the index or the\n> other configuration) are met, but if this disables the split index\n> when it is set to 0, would we even need the other configuration\n> variable? IOW, perhaps we can do without core.splitIndex?\n\nI think it is easier for user to be able to just set core.splitIndex\nto true to enable split-index.\nIf we ask them to instead set splitIndex.maxPercentChange to a\n\"reasonable value like 20\" to enable it, they will start to wonder why\n20 and not 50 or 100 and might be unnecessarily confused.\n\nAlso it might be good to make it possible to have other values than\n\"true\" and \"false\" for core.splitIndex in the future.\nFor example if we decide to first enable split-index only on\n$GIT_DIR/index, we might later want an \"everywhere\" or \"always\" value\nfor core.splitIndex if we find a way to enable it on other indexes.\n\nWe might also want an \"auto\" mode in the future when split-index will\nwork really well, so that it can be turned on automatically on all the\nrepos where it makes sense (like when the index contains more than\n10000 entries for example).\n\n>> + By default the value is 20, so a new shared index is written\n>> + if the number of entries in the split index would be greater\n>> + than 20 percent of the total number of entries.\n>> + See linkgit:git-update-index[1].\n"},{"id":"305464","messageId":"xmqqoa1scyuy.fsf@gitster.mtv.corp.google.com","threadId":"44361","inReplyTo":"CAP8UFD2H6rZNQqg1VGNkes65KAk-4P160rjQSjFY3NjgZydZmA@mail.gmail.com","subject":"Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-06T17:16:53Z","receivedAt":"2016-11-06T17:17:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> I think it is easier for user to be able to just set core.splitIndex\n> to true to enable split-index.\n\nYou can have that exact benefit by making core.splitIndex to\nbool-or-more. If your default is 20%, take 'true' as if the user\nspecified 20% and take 'false' as if the user specified 100% (or is\nit 0%? I do not care about the details but you get the point).\n\n"},{"id":"305479","messageId":"CACsJy8BZNfESmFv=V89Cq-b+aMJWLH=qhXHNE8inZZRjvXB33Q@mail.gmail.com","threadId":"44361","inReplyTo":"CAP8UFD1YL+RgdqbV0V1OnC=sJHJFc_an02Q9JeDNapW+u1CZcA@mail.gmail.com","subject":"Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-11-07T09:38:24Z","receivedAt":"2016-11-07T09:40:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"(sorry I got sick in the last few weeks and could not respond to this earlier)\n\nOn Mon, Nov 7, 2016 at 4:44 AM, Christian Couder\n wrote:\n> Le 6 nov. 2016 09:16, \"Junio C Hamano\" a écrit :\n>>\n>> Christian Couder writes:\n>>\n>> > I think it is easier for user to be able to just set core.splitIndex\n>> > to true to enable split-index.\n>>\n>> You can have that exact benefit by making core.splitIndex to\n>> bool-or-more. If your default is 20%, take 'true' as if the user\n>> specified 20% and take 'false' as if the user specified 100% (or is\n>> it 0%? I do not care about the details but you get the point).\n>\n> Then if we ever add 'auto' and the user wants for example 10% instead of the\n> default 20%, we will have to make it accept things like \"auto,10\".\n\nIn my opinion, \"true\" _is_ auto, which is a way to say \"I trust you to\ndo the right thing, just re-split the index when it makes sense\", \"no\"\nis disabled of course. If the user wants to be specific, just write\n\"10\" or some other percentage.(and either 0 or 100 would mean enable\nsplit-index but do not re-split automatically, let _me_ do it when I\nwant it)\n-- \nDuy\n"},{"id":"305482","messageId":"CACsJy8DjXrOgB-_-t47uSdCQFg9s_o+Oj9NBmAhDFZ3aYvjBgg@mail.gmail.com","threadId":"44361","inReplyTo":"CAP8UFD3hNEU_UeVizU6SVJTt4hqJPag9XWqZOM3FKCGJZXOthg@mail.gmail.com","subject":"Re: [PATCH v1 03/19] split-index: add {add,remove}_split_index() functions","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-11-07T10:08:45Z","receivedAt":"2016-11-07T10:09:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Oct 30, 2016 at 5:06 AM, Christian Couder\n wrote:\n> On Tue, Oct 25, 2016 at 11:58 AM, Duy Nguyen wrote:\n>> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder\n>> wrote:\n>>> +void remove_split_index(struct index_state *istate)\n>>> +{\n>>> + if (istate->split_index) {\n>>> + /*\n>>> + * can't discard_split_index(&the_index); because that\n>>> + * will destroy split_index->base->cache[], which may\n>>> + * be shared with the_index.cache[]. So yeah we're\n>>> + * leaking a bit here.\n>>\n>> In the context of update-index, this is a one-time thing and leaking\n>> is tolerable. But because it becomes a library function now, this leak\n>> can become more serious, I think.\n>>\n>> The only other (indirect) caller is read_index_from() so probably not\n>> bad most of the time (we read at the beginning of a command only).\n>> sequencer.c may discard and re-read the index many times though,\n>> leaking could be visible there.\n>\n> So is it enough to check if split_index->base->cache[] is shared with\n> the_index.cache[] and then decide if discard_split_index(&the_index)\n> should be called?\n\nIt's likely shared though. We could un-share cache[] by duplicating\nindex entries in the_index.cache[] if they point back to\nsplit_index->base (we know what entries are shared by examining the\n\"index\" field). Once we do that, we can discard_split_index()\nunconditionally. There's another place that has similar leak:\nmove_cache_to_base_index(), which could receive the same treatment.\n-- \nDuy\n"},{"id":"305621","messageId":"CACsJy8AisF2ZVs7JdnVFp5wdskkbVQQQ=DBq5UzE1MOsCfBMtQ@mail.gmail.com","threadId":"44361","inReplyTo":"CACsJy8DjXrOgB-_-t47uSdCQFg9s_o+Oj9NBmAhDFZ3aYvjBgg@mail.gmail.com","subject":"Re: [PATCH v1 03/19] split-index: add {add,remove}_split_index() functions","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-11-09T09:24:16Z","receivedAt":"2016-11-09T09:24:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Nov 7, 2016 at 5:08 PM, Duy Nguyen wrote:\n> On Sun, Oct 30, 2016 at 5:06 AM, Christian Couder\n> wrote:\n>> On Tue, Oct 25, 2016 at 11:58 AM, Duy Nguyen wrote:\n>>> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder\n>>> wrote:\n>>>> +void remove_split_index(struct index_state *istate)\n>>>> +{\n>>>> + if (istate->split_index) {\n>>>> + /*\n>>>> + * can't discard_split_index(&the_index); because that\n>>>> + * will destroy split_index->base->cache[], which may\n>>>> + * be shared with the_index.cache[]. So yeah we're\n>>>> + * leaking a bit here.\n>>>\n>>> In the context of update-index, this is a one-time thing and leaking\n>>> is tolerable. But because it becomes a library function now, this leak\n>>> can become more serious, I think.\n>>>\n>>> The only other (indirect) caller is read_index_from() so probably not\n>>> bad most of the time (we read at the beginning of a command only).\n>>> sequencer.c may discard and re-read the index many times though,\n>>> leaking could be visible there.\n>>\n>> So is it enough to check if split_index->base->cache[] is shared with\n>> the_index.cache[] and then decide if discard_split_index(&the_index)\n>> should be called?\n>\n> It's likely shared though. We could un-share cache[] by duplicating\n> index entries in the_index.cache[] if they point back to\n> split_index->base\n\nI have a patch for this. So don't have to do anything else in this\narea. I'll probably just pile my patch on top of your series, or post\nit once the series graduates to master.\n-- \nDuy\n"},{"id":"305634","messageId":"CAP8UFD1M3juG2Dz8caFqexNC=7_730-w+y8miVmDPKPF0th80g@mail.gmail.com","threadId":"44361","inReplyTo":"CACsJy8AisF2ZVs7JdnVFp5wdskkbVQQQ=DBq5UzE1MOsCfBMtQ@mail.gmail.com","subject":"Re: [PATCH v1 03/19] split-index: add {add,remove}_split_index() functions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-11-09T14:47:30Z","receivedAt":"2016-11-09T14:47:39Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Nov 9, 2016 at 10:24 AM, Duy Nguyen wrote:\n> On Mon, Nov 7, 2016 at 5:08 PM, Duy Nguyen wrote:\n>> On Sun, Oct 30, 2016 at 5:06 AM, Christian Couder\n>> wrote:\n>>> On Tue, Oct 25, 2016 at 11:58 AM, Duy Nguyen wrote:\n>>>> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder\n>>>> wrote:\n>>>>> +void remove_split_index(struct index_state *istate)\n>>>>> +{\n>>>>> + if (istate->split_index) {\n>>>>> + /*\n>>>>> + * can't discard_split_index(&the_index); because that\n>>>>> + * will destroy split_index->base->cache[], which may\n>>>>> + * be shared with the_index.cache[]. So yeah we're\n>>>>> + * leaking a bit here.\n>>>>\n>>>> In the context of update-index, this is a one-time thing and leaking\n>>>> is tolerable. But because it becomes a library function now, this leak\n>>>> can become more serious, I think.\n>>>>\n>>>> The only other (indirect) caller is read_index_from() so probably not\n>>>> bad most of the time (we read at the beginning of a command only).\n>>>> sequencer.c may discard and re-read the index many times though,\n>>>> leaking could be visible there.\n>>>\n>>> So is it enough to check if split_index->base->cache[] is shared with\n>>> the_index.cache[] and then decide if discard_split_index(&the_index)\n>>> should be called?\n>>\n>> It's likely shared though. We could un-share cache[] by duplicating\n>> index entries in the_index.cache[] if they point back to\n>> split_index->base\n>\n> I have a patch for this. So don't have to do anything else in this\n> area. I'll probably just pile my patch on top of your series, or post\n> it once the series graduates to master.\n\nGreat, thanks!\n"},{"id":"306141","messageId":"CAP8UFD1mun7wz2WqV8GCj6MODVjP2mPEBRGbJTf_ypiGCgtb9Q@mail.gmail.com","threadId":"44361","inReplyTo":"CACsJy8BZNfESmFv=V89Cq-b+aMJWLH=qhXHNE8inZZRjvXB33Q@mail.gmail.com","subject":"Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-11-18T14:34:10Z","receivedAt":"2016-11-18T14:34:17Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Nov 7, 2016 at 10:38 AM, Duy Nguyen wrote:\n> (sorry I got sick in the last few weeks and could not respond to this earlier)\n\n(Yeah, I have also been sick during the last few weeks.)\n\n> On Mon, Nov 7, 2016 at 4:44 AM, Christian Couder\n> wrote:\n>> Le 6 nov. 2016 09:16, \"Junio C Hamano\" a écrit :\n>>>\n>>> Christian Couder writes:\n>>>\n>>> > I think it is easier for user to be able to just set core.splitIndex\n>>> > to true to enable split-index.\n>>>\n>>> You can have that exact benefit by making core.splitIndex to\n>>> bool-or-more. If your default is 20%, take 'true' as if the user\n>>> specified 20% and take 'false' as if the user specified 100% (or is\n>>> it 0%? I do not care about the details but you get the point).\n>\n>> Then if we ever add 'auto' and the user wants for example 10% instead of the\n>> default 20%, we will have to make it accept things like \"auto,10\".\n\n(Sorry for writing the above on my phone which added HTML, so that it\ndidn't reach the list.)\n\n> In my opinion, \"true\" _is_ auto, which is a way to say \"I trust you to\n> do the right thing, just re-split the index when it makes sense\", \"no\"\n> is disabled of course. If the user wants to be specific, just write\n> \"10\" or some other percentage.(and either 0 or 100 would mean enable\n> split-index but do not re-split automatically, let _me_ do it when I\n> want it)\n\nThe meaning of a future \"auto\" option for \"core.splitIndex\" could be\n\"use the split-index feature only if the number of entries in whole\nindex is greater than 10000 (by default)\".\n\nIf there is no difference between \"true\" and \"auto\" then, when users\nwho have \"core.splitIndex=true\" will migrate to the git version that\nadds the \"auto\" feature, their repos with under 10000 entires will not\nuse the split-index feature anymore. These users may then be annoyed\nthat the behavior has been switched under them, and that the\nsplit-index feature is not always used despite having\n\"core.splitIndex=true\" in their config.\n"},{"id":"306295","messageId":"CACsJy8BazeR=4tz3q2f35x=fCfp-Ld9LJz0mQh_CZoR_iXKEEQ@mail.gmail.com","threadId":"44361","inReplyTo":"CAP8UFD1mun7wz2WqV8GCj6MODVjP2mPEBRGbJTf_ypiGCgtb9Q@mail.gmail.com","subject":"Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-11-22T10:35:17Z","receivedAt":"2016-11-22T10:36:00Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Nov 18, 2016 at 9:34 PM, Christian Couder\n wrote:\n> On Mon, Nov 7, 2016 at 10:38 AM, Duy Nguyen wrote:\n>> (sorry I got sick in the last few weeks and could not respond to this earlier)\n>\n> (Yeah, I have also been sick during the last few weeks.)\n>\n>> On Mon, Nov 7, 2016 at 4:44 AM, Christian Couder\n>> wrote:\n>>> Le 6 nov. 2016 09:16, \"Junio C Hamano\" a écrit :\n>>>>\n>>>> Christian Couder writes:\n>>>>\n>>>> > I think it is easier for user to be able to just set core.splitIndex\n>>>> > to true to enable split-index.\n>>>>\n>>>> You can have that exact benefit by making core.splitIndex to\n>>>> bool-or-more. If your default is 20%, take 'true' as if the user\n>>>> specified 20% and take 'false' as if the user specified 100% (or is\n>>>> it 0%? I do not care about the details but you get the point).\n>>\n>>> Then if we ever add 'auto' and the user wants for example 10% instead of the\n>>> default 20%, we will have to make it accept things like \"auto,10\".\n>\n> (Sorry for writing the above on my phone which added HTML, so that it\n> didn't reach the list.)\n>\n>> In my opinion, \"true\" _is_ auto, which is a way to say \"I trust you to\n>> do the right thing, just re-split the index when it makes sense\", \"no\"\n>> is disabled of course. If the user wants to be specific, just write\n>> \"10\" or some other percentage.(and either 0 or 100 would mean enable\n>> split-index but do not re-split automatically, let _me_ do it when I\n>> want it)\n>\n> The meaning of a future \"auto\" option for \"core.splitIndex\" could be\n> \"use the split-index feature only if the number of entries in whole\n> index is greater than 10000 (by default)\".\n\nWell.. with the \"just re-split the index when it makes sense\" part,\nthe user entrusts git to do something sensible in all cases, and going\nwith absolute numbers might not be the best way, I think. It's big\nresponsibility :)\n\n> If there is no difference between \"true\" and \"auto\" then, when users\n> who have \"core.splitIndex=true\" will migrate to the git version that\n> adds the \"auto\" feature, their repos with under 10000 entires will not\n> use the split-index feature anymore. These users may then be annoyed\n> that the behavior has been switched under them, and that the\n> split-index feature is not always used despite having\n> \"core.splitIndex=true\" in their config.\n-- \nDuy\n"},{"id":"306298","messageId":"CAP8UFD2VFQs2X14omWEy-iJzEeTEeEOa+fxMEq3HTQK3rFyuCg@mail.gmail.com","threadId":"44361","inReplyTo":"CACsJy8BazeR=4tz3q2f35x=fCfp-Ld9LJz0mQh_CZoR_iXKEEQ@mail.gmail.com","subject":"Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-11-22T13:13:03Z","receivedAt":"2016-11-22T13:13:42Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Nov 22, 2016 at 11:35 AM, Duy Nguyen wrote:\n\n[...]\n\n>>> In my opinion, \"true\" _is_ auto, which is a way to say \"I trust you to\n>>> do the right thing, just re-split the index when it makes sense\", \"no\"\n>>> is disabled of course. If the user wants to be specific, just write\n>>> \"10\" or some other percentage.(and either 0 or 100 would mean enable\n>>> split-index but do not re-split automatically, let _me_ do it when I\n>>> want it)\n>>\n>> The meaning of a future \"auto\" option for \"core.splitIndex\" could be\n>> \"use the split-index feature only if the number of entries in whole\n>> index is greater than 10000 (by default)\".\n>\n> Well.. with the \"just re-split the index when it makes sense\" part,\n> the user entrusts git to do something sensible in all cases,\n\nThat's an interpretation of what \"core.splitIndex=true\" could mean,\nbut there could be users who trust Git to re-split when it makes\nsense, but who do want to use the split-index on all theirs repos even\nthe small ones or who just don't trust Git to choose when it might be\nbetter to use it or not.\n\nYeah, a typical git user would most of the time just trust Git for all\nthose things, but on the other hand there are companies out there that\nare willing to tweak many configuration options to get the better\npossible behavior for them.\n\nIn fact I am working on this for Booking.com, and if we find out later\nthat we would gain something significant, like performance\nimprovements or configuration simplification, by adding \"auto\" and/or\nother configuration variables to tweak more split-index related\nthings, we might very well post patch series to do that.\n\nSo if we now mix things up just to avoid one more configuration\noption, we could very well make things harder to develop, to\nconfigure, to parse and to understand later, so it is not a trade off\nworth making.\n\n> and going\n> with absolute numbers might not be the best way, I think. It's big\n> responsibility :)\n\nAbout going with absolute number, yeah I am not sure at all it is the\nbest way, but this was just part of an example to try to explain what\nI am saying above.\n"},{"id":"306299","messageId":"CACsJy8AV7mYDsod=BedAsy9W=tNpZg2kpzQ5HBTAmE-WcdroDA@mail.gmail.com","threadId":"44361","inReplyTo":"CAP8UFD2VFQs2X14omWEy-iJzEeTEeEOa+fxMEq3HTQK3rFyuCg@mail.gmail.com","subject":"Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-11-22T13:20:07Z","receivedAt":"2016-11-22T13:20:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Nov 22, 2016 at 8:13 PM, Christian Couder\n wrote:\n> So if we now mix things up just to avoid one more configuration\n> option, we could very well make things harder to develop, to\n> configure, to parse and to understand later, so it is not a trade off\n> worth making.\n\nOK since we're still in the early phase of determining how to\nre-split, I guess going with elaborate, precise configuration is\nbetter, even if it's more config options. Once we know better, maybe\nwe can decide a good default cover-all \"auto\" then (or maybe not\nbecause we find out no shoe fits all).\n-- \nDuy\n"},{"id":"306378","messageId":"CAP8UFD0iToVxU+maNL9BFXacp3sER+AfrqAnQXWf7EAwURKmdQ@mail.gmail.com","threadId":"44361","inReplyTo":"xmqqziljngod.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 15/19] config: add git_config_get_date_string() from gc.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-11-23T15:04:57Z","receivedAt":"2016-11-23T15:05:08Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Nov 1, 2016 at 8:28 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> This function will be used in a following commit to get the expiration\n>> time of the shared index files from the config, and it is generic\n>> enough to be put in \"config.c\".\n>\n> Is it generic enough that a helper that sounds as if it can get any\n> date string dies if it is given a future date? I somehow doubt it.\n>\n> At the minimum, it must be made clear that there is an artificial\n> limitation that the current set of callers find useful in cache.h as\n> a one-liner comment next to the added declaration. Then people with\n> the same need (i.e. they want to reject future timestamps) can\n> decide to use it, while others would stay away from it.\n>\n> If you can come up with a better word to use to encode that\n> artificial limitation in its name, renaming it is even better.\n\nOk it will appear like this in cache.h:\n\n/* This dies if the configured or default date is in the future */\nextern int git_config_get_expire_date_string(const char *key, const\nchar **output);\n\nThanks,\nChristian.\n"},{"id":"306386","messageId":"xmqqlgwam76c.fsf@gitster.mtv.corp.google.com","threadId":"44361","inReplyTo":"CAP8UFD0iToVxU+maNL9BFXacp3sER+AfrqAnQXWf7EAwURKmdQ@mail.gmail.com","subject":"Re: [PATCH v1 15/19] config: add git_config_get_date_string() from gc.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-23T17:34:35Z","receivedAt":"2016-11-23T17:34:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Ok it will appear like this in cache.h:\n>\n> /* This dies if the configured or default date is in the future */\n> extern int git_config_get_expire_date_string(const char *key, const\n> char **output);\n\nThose who imitate existing callsites never read comments, and you\nneed to spend effort to get the name right to protect the codebase\nfrom them.\n\n\"get-expiry\" may be shorter. Neither still does not say it will\ndie, though.\n\n"},{"id":"306496","messageId":"CAP8UFD0Q4yTfnhLW6XbfqbxBvMc_QmZTEn4XJb-9fj6Uvq6hkw@mail.gmail.com","threadId":"44361","inReplyTo":"xmqqlgwam76c.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 15/19] config: add git_config_get_date_string() from gc.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-11-28T16:19:17Z","receivedAt":"2016-11-28T16:19:24Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Nov 23, 2016 at 6:34 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> Ok it will appear like this in cache.h:\n>>\n>> /* This dies if the configured or default date is in the future */\n>> extern int git_config_get_expire_date_string(const char *key, const\n>> char **output);\n>\n> Those who imitate existing callsites never read comments, and you\n> need to spend effort to get the name right to protect the codebase\n> from them.\n>\n> \"get-expiry\" may be shorter. Neither still does not say it will\n> die, though.\n\nWhat about something like this then:\n\n/* This dies if the configured or default date is in the future */\nextern int git_config_get_expiry_or_die(const char *key, const char **output);\n\nAlso git_config_get_int(), git_config_get_bool() and probably other\nsuch functions are indirectly calling git_config_int() which die()s is\nthe value is not a number, so it feels a bit strange to have only one\nfunction with a name that ends with \"_or_die\" when many other\nfunctions could die(). In fact it could give a false sense of security\nto those who just read cache.h.\n"},{"id":"306497","messageId":"xmqqeg1vlf15.fsf@gitster.mtv.corp.google.com","threadId":"44361","inReplyTo":"CAP8UFD0Q4yTfnhLW6XbfqbxBvMc_QmZTEn4XJb-9fj6Uvq6hkw@mail.gmail.com","subject":"Re: [PATCH v1 15/19] config: add git_config_get_date_string() from gc.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-28T16:56:06Z","receivedAt":"2016-11-28T16:56:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> What about something like this then:\n>\n> /* This dies if the configured or default date is in the future */\n> extern int git_config_get_expiry_or_die(const char *key, const char **output);\n>\n> Also git_config_get_int(), git_config_get_bool() and probably other\n> such functions are indirectly calling git_config_int() which die()s is\n> the value is not a number, so it feels a bit strange to have only one\n> function with a name that ends with \"_or_die\" when many other\n> functions could die(). In fact it could give a false sense of security\n> to those who just read cache.h.\n\nIt is half-a-good-point that existing functions would die and I\nagree with you that get_expiry_or_die can lose _or_die part.\n\nBut get_int() dying when it is fed something that is not \"int\" (or\nsomething that cannot be returned in \"int\") is one thing; get_date()\nbeing happy when it is fed a string that is a valid date 2015-01-01\nbut dying when fed another valid date 2018-01-01 is quite puzzling.\n\nI think get_expiry() is OK. \n\nQ: What's expiry? How is it different from date\nA: A past timestamp that is speifies the cutoff time for the purpose\n of expiring old stuff. You can say \"now\" to expire everything\n you have.\n\nis more pleasant and understandable conversation than\n\nQ: Why can I feed only past date to get_date()? Otherwise it dies.\nA: ...? The people who named the function didn't know what \"date\" was?\n\n\n\n\n\n\n"}]}