[PATCH v1 00/19] Add configuration options for split-index

[PATCH v1 00/19] Add configuration options for split-index

[Christian Couder]

Goal
~~~~

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

This can be valuable as split-index can help significantly speed up
`git rebase` especially along with the work to libify `git apply`
that has been recently merged to master
(see https://github.com/git/git/commit/81358dc238372793b1590efa149cc1581d1fbd98).

Design
~~~~~~

The design is similar as the previous work that introduced
"core.untrackedCache". 

The new "core.splitIndex" configuration option can be either true,
false or undefined which is the default.

When it is true, the split index is created, if it does not already
exists, when the index is read. When it is false, the split index is
removed if it exists, when the index is read. Otherwise it is left as
is.

Along with this new configuration variable, the two following options
are also introduced:

    - splitIndex.maxPercentChange

    This is to avoid having too many changes accumulating in the split
    index while in split index mode. The git-update-index
    documentation says:

	If split-index mode is already enabled and `--split-index` is
	given again, all changes in $GIT_DIR/index are pushed back to
	the shared index file.

    but it is probably better to not expect the user to think about it
    and to have a mechanism that pushes back all changes to the shared
    index file automatically when some threshold is reached.

    The default threshold is when the number of entries in the split
    index file reaches 20% (by default) of the number of entries in
    the shared index file. The new "splitIndex.maxPercentChange"
    config option lets people tweak this value.

    - splitIndex.sharedIndexExpire

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

    This idea was suggested by Duy in:

    https://public-inbox.org/git/CACsJy8BqMFASHf5kJgUh+bd7XG98CafNydE964VJyPXz-emEvA@mail.gmail.com/

    and after some experiments, I agree that it is much simpler than
    what I thought could be done during our discussion.

Highlevel view of the patches in the series
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Except for patch 1/19, there are 3 big steps, one for each new
configuration variable introduced.

The main difference between this patch series and the RFC patch series
sent last July is that the Step 2 and 3 are new and have been
implemented as suggested by Duy. Thanks Duy!

    - Patch 1/19 is a typo fix in a comment that can be applied
      separately.

Step 1 is:

    - Patches 2/19 to 5/19 introduce the functions that are reading
      the "core.splitIndex" configuration variable and tweaking the
      split index depending on its value.

    - Patch 6/19 adds a few tests for the new feature.

    - Patches 7/19 and 8/19 add some documentation for the new
      feature.

Step 2 is:

    - Patches 9/19 and 10/19 introduce the functions that are reading
      the "splitIndex.maxPercentChange" configuration variable and
      regenerating a new shared index file depending on its value.

    - Patch 11/19 adds a few tests for the new feature.

    - Patch 12/19 add some documentation for the new feature.

Step 3 is:

    - Patches 13/19 to 16/19 introduce the functions that are reading
      the "splitIndex.sharedIndexExpire" configuration variable and
      expiring old shared index files depending on its value.

    - Patch 17/19 adds a few tests for the new feature.

    - Patches 18/19 and 19/19 add some documentation for the new
      feature.

Links
~~~~~

This patch series is also available here:

  https://github.com/chriscool/git/commits/config-split-index

The previous RFC version was:

  https://github.com/chriscool/git/commits/config-split-index7

On the mailing list the related patch series and discussions were:

  https://public-inbox.org/git/20160711172254.13439-1-chriscool@tuxfamily.org/

Christian Couder (19):
  split-index: s/eith/with/ typo fix
  config: add git_config_get_split_index()
  split-index: add {add,remove}_split_index() functions
  read-cache: add and then use tweak_split_index()
  update-index: warn in case of split-index incoherency
  t1700: add tests for core.splitIndex
  Documentation/config: add information for core.splitIndex
  Documentation/git-update-index: talk about core.splitIndex config var
  config: add git_config_get_max_percent_split_change()
  read-cache: regenerate shared index if necessary
  t1700: add tests for splitIndex.maxPercentChange
  Documentation/config: add splitIndex.maxPercentChange
  sha1_file: make check_and_freshen_file() non static
  read-cache: touch shared index files when used
  config: add git_config_get_date_string() from gc.c
  read-cache: unlink old sharedindex files
  t1700: test shared index file expiration
  Documentation/config: add splitIndex.sharedIndexExpire
  Documentation/git-update-index: explain splitIndex.*

 Documentation/config.txt           |  28 +++++++
 Documentation/git-update-index.txt |  39 ++++++++--
 builtin/gc.c                       |  15 +---
 builtin/update-index.c             |  25 +++---
 cache.h                            |   6 ++
 config.c                           |  39 ++++++++++
 read-cache.c                       | 119 +++++++++++++++++++++++++++-
 sha1_file.c                        |   2 +-
 split-index.c                      |  24 +++++-
 split-index.h                      |   2 +
 t/t1700-split-index.sh             | 154 +++++++++++++++++++++++++++++++++++++
 11 files changed, 417 insertions(+), 36 deletions(-)

-- 
2.10.1.462.g7e1e03a

[PATCH v1 01/19] split-index: s/eith/with/ typo fix

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 split-index.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/split-index.c b/split-index.c
index 35da553..615f4ca 100644
--- a/split-index.c
+++ b/split-index.c
@@ -187,7 +187,7 @@ void prepare_to_write_split_index(struct index_state *istate)
 		/* Go through istate->cache[] and mark CE_MATCHED to
 		 * entry with positive index. We'll go through
 		 * base->cache[] later to delete all entries in base
-		 * that are not marked eith either CE_MATCHED or
+		 * that are not marked with either CE_MATCHED or
 		 * CE_UPDATE_IN_BASE. If istate->cache[i] is a
 		 * duplicate, deduplicate it.
 		 */
-- 
2.10.1.462.g7e1e03a

[PATCH v1 02/19] config: add git_config_get_split_index()

[Christian Couder]

This new function will be used in a following commit to know
if we want to use the split index feature or not.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 cache.h  |  1 +
 config.c | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/cache.h b/cache.h
index 05ecb88..394da60 100644
--- a/cache.h
+++ b/cache.h
@@ -1809,6 +1809,7 @@ extern int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest);
 extern int git_config_get_maybe_bool(const char *key, int *dest);
 extern int git_config_get_pathname(const char *key, const char **dest);
 extern int git_config_get_untracked_cache(void);
+extern int git_config_get_split_index(void);
 
 /*
  * This is a hack for test programs like test-dump-untracked-cache to
diff --git a/config.c b/config.c
index 83fdecb..036e29b 100644
--- a/config.c
+++ b/config.c
@@ -1709,6 +1709,16 @@ int git_config_get_untracked_cache(void)
 	return -1; /* default value */
 }
 
+int git_config_get_split_index(void)
+{
+	int val = -1;
+
+	if (!git_config_get_maybe_bool("core.splitindex", &val))
+		return val;
+
+	return -1; /* default value */
+}
+
 NORETURN
 void git_die_config_linenr(const char *key, const char *filename, int linenr)
 {
-- 
2.10.1.462.g7e1e03a

[PATCH v1 03/19] split-index: add {add,remove}_split_index() functions

[Christian Couder]

Also use the functions in cmd_update_index() in
builtin/update-index.c.

These functions will be used in a following commit to tweak
our use of the split-index feature depending on the setting
of a configuration variable.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/update-index.c | 18 ++++++------------
 split-index.c          | 22 ++++++++++++++++++++++
 split-index.h          |  2 ++
 3 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index f3f07e7..b75ea03 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1098,18 +1098,12 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	}
 
 	if (split_index > 0) {
-		init_split_index(&the_index);
-		the_index.cache_changed |= SPLIT_INDEX_ORDERED;
-	} else if (!split_index && the_index.split_index) {
-		/*
-		 * can't discard_split_index(&the_index); because that
-		 * will destroy split_index->base->cache[], which may
-		 * be shared with the_index.cache[]. So yeah we're
-		 * leaking a bit here.
-		 */
-		the_index.split_index = NULL;
-		the_index.cache_changed |= SOMETHING_CHANGED;
-	}
+		if (the_index.split_index)
+			the_index.cache_changed |= SPLIT_INDEX_ORDERED;
+		else
+			add_split_index(&the_index);
+	} else if (!split_index)
+		remove_split_index(&the_index);
 
 	switch (untracked_cache) {
 	case UC_UNSPECIFIED:
diff --git a/split-index.c b/split-index.c
index 615f4ca..f519e60 100644
--- a/split-index.c
+++ b/split-index.c
@@ -317,3 +317,25 @@ void replace_index_entry_in_base(struct index_state *istate,
 		istate->split_index->base->cache[new->index - 1] = new;
 	}
 }
+
+void add_split_index(struct index_state *istate)
+{
+	if (!istate->split_index) {
+		init_split_index(istate);
+		istate->cache_changed |= SPLIT_INDEX_ORDERED;
+	}
+}
+
+void remove_split_index(struct index_state *istate)
+{
+	if (istate->split_index) {
+		/*
+		 * can't discard_split_index(&the_index); because that
+		 * will destroy split_index->base->cache[], which may
+		 * be shared with the_index.cache[]. So yeah we're
+		 * leaking a bit here.
+		 */
+		istate->split_index = NULL;
+		istate->cache_changed |= SOMETHING_CHANGED;
+	}
+}
diff --git a/split-index.h b/split-index.h
index c1324f5..df91c1b 100644
--- a/split-index.h
+++ b/split-index.h
@@ -31,5 +31,7 @@ void merge_base_index(struct index_state *istate);
 void prepare_to_write_split_index(struct index_state *istate);
 void finish_writing_split_index(struct index_state *istate);
 void discard_split_index(struct index_state *istate);
+void add_split_index(struct index_state *istate);
+void remove_split_index(struct index_state *istate);
 
 #endif
-- 
2.10.1.462.g7e1e03a

[PATCH v1 04/19] read-cache: add and then use tweak_split_index()

[Christian Couder]

This will make us use the split-index feature or not depending
on the value of the "core.splitIndex" config variable.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 read-cache.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/read-cache.c b/read-cache.c
index 38d67fa..bb53823 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1562,10 +1562,27 @@ static void tweak_untracked_cache(struct index_state *istate)
 	}
 }
 
+static void tweak_split_index(struct index_state *istate)
+{
+	switch (git_config_get_split_index()) {
+	case -1: /* unset: do nothing */
+		break;
+	case 0: /* false */
+		remove_split_index(istate);
+		break;
+	case 1: /* true */
+		add_split_index(istate);
+		break;
+	default: /* unknown value: do nothing */
+		break;
+	}
+}
+
 static void post_read_index_from(struct index_state *istate)
 {
 	check_ce_order(istate);
 	tweak_untracked_cache(istate);
+	tweak_split_index(istate);
 }
 
 /* remember to discard_cache() before reading a different cache! */
-- 
2.10.1.462.g7e1e03a

[PATCH v1 05/19] update-index: warn in case of split-index incoherency

[Christian Couder]

When users are using `git update-index --(no-)split-index`, they
may expect the split-index feature to be used or not according to
the option they just used, but this might not be the case if the
new "core.splitIndex" config variable has been set. In this case
let's warn about what will happen and why.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/update-index.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index b75ea03..a14dbf2 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1098,12 +1098,21 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	}
 
 	if (split_index > 0) {
+		if (git_config_get_split_index() == 0)
+			warning("core.splitIndex is set to false; "
+				"remove or change it, if you really want to "
+				"enable split index");
 		if (the_index.split_index)
 			the_index.cache_changed |= SPLIT_INDEX_ORDERED;
 		else
 			add_split_index(&the_index);
-	} else if (!split_index)
+	} else if (!split_index) {
+		if (git_config_get_split_index() == 1)
+			warning("core.splitIndex is set to true; "
+				"remove or change it, if you really want to "
+				"disable split index");
 		remove_split_index(&the_index);
+	}
 
 	switch (untracked_cache) {
 	case UC_UNSPECIFIED:
-- 
2.10.1.462.g7e1e03a

[PATCH v1 06/19] t1700: add tests for core.splitIndex

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t1700-split-index.sh | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 292a072..db8c39f 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -200,4 +200,41 @@ EOF
 	test_cmp expect actual
 '
 
+test_expect_success 'set core.splitIndex config variable to true' '
+	git config core.splitIndex true &&
+	: >three &&
+	git update-index --add three &&
+	git ls-files --stage >ls-files.actual &&
+	cat >ls-files.expect <<EOF &&
+100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
+100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	three
+100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	two
+EOF
+	test_cmp ls-files.expect ls-files.actual &&
+	BASE=$(test-dump-split-index .git/index | grep "^base") &&
+	test-dump-split-index .git/index | sed "/^own/d" >actual &&
+	cat >expect <<EOF &&
+$BASE
+replacements:
+deletions:
+EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'set core.splitIndex config variable to false' '
+	git config core.splitIndex false &&
+	git update-index --force-remove three &&
+	git ls-files --stage >ls-files.actual &&
+	cat >ls-files.expect <<EOF &&
+100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
+100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	two
+EOF
+	test_cmp ls-files.expect ls-files.actual &&
+	test-dump-split-index .git/index | sed "/^own/d" >actual &&
+	cat >expect <<EOF &&
+not a split index
+EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.10.1.462.g7e1e03a

[PATCH v1 07/19] Documentation/config: add information for core.splitIndex

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 27069ac..96521a4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -331,6 +331,10 @@ core.trustctime::
 	crawlers and some backup systems).
 	See linkgit:git-update-index[1]. True by default.
 
+core.splitIndex::
+	If true, the split-index feature of the index will be used.
+	See linkgit:git-update-index[1]. False by default.
+
 core.untrackedCache::
 	Determines what to do about the untracked cache feature of the
 	index. It will be kept, if this variable is unset or set to
-- 
2.10.1.462.g7e1e03a

[PATCH v1 08/19] Documentation/git-update-index: talk about core.splitIndex config var

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-update-index.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
index 7386c93..e091b2a 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -171,6 +171,12 @@ may not support it yet.
 	given again, all changes in $GIT_DIR/index are pushed back to
 	the shared index file. This mode is designed for very large
 	indexes that take a significant amount of time to read or write.
++
+These options take effect whatever the value of the `core.splitIndex`
+configuration variable (see linkgit:git-config[1]). But a warning is
+emitted when the change goes against the configured value, as the
+configured value will take effect next time the index is read and this
+will remove the intended effect of the option.
 
 --untracked-cache::
 --no-untracked-cache::
-- 
2.10.1.462.g7e1e03a

[PATCH v1 09/19] config: add git_config_get_max_percent_split_change()

[Christian Couder]

This new function will be used in a following commit to get the
value of the "splitIndex.maxPercentChange" config variable.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 cache.h  |  1 +
 config.c | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/cache.h b/cache.h
index 394da60..faceceb 100644
--- a/cache.h
+++ b/cache.h
@@ -1810,6 +1810,7 @@ extern int git_config_get_maybe_bool(const char *key, int *dest);
 extern int git_config_get_pathname(const char *key, const char **dest);
 extern int git_config_get_untracked_cache(void);
 extern int git_config_get_split_index(void);
+extern int git_config_get_max_percent_split_change(void);
 
 /*
  * This is a hack for test programs like test-dump-untracked-cache to
diff --git a/config.c b/config.c
index 036e29b..5580f56 100644
--- a/config.c
+++ b/config.c
@@ -1719,6 +1719,22 @@ int git_config_get_split_index(void)
 	return -1; /* default value */
 }
 
+int git_config_get_max_percent_split_change(void)
+{
+	int val = -1;
+
+	if (!git_config_get_int("splitindex.maxpercentchange", &val)) {
+		if (0 <= val && val <= 100)
+			return val;
+
+		error("splitindex.maxpercentchange value '%d' "
+		      "should be between 0 and 100", val);
+		return -1;
+	}
+
+	return -1; /* default value */
+}
+
 NORETURN
 void git_die_config_linenr(const char *key, const char *filename, int linenr)
 {
-- 
2.10.1.462.g7e1e03a

[PATCH v1 10/19] read-cache: regenerate shared index if necessary

[Christian Couder]

When writing a new split-index and there is a big number of cache
entries in the split-index compared to the shared index, it is a
good idea to regenerate the shared index.

By default when the ratio reaches 20%, we will push back all
the entries from the split-index into a new shared index file
instead of just creating a new split-index file.

The threshold can be configured using the
"splitIndex.maxPercentChange" config variable.

We need to adjust the existing tests in t1700 by setting
"splitIndex.maxPercentChange" to 100 at the beginning of t1700,
as the existing tests are assuming that the shared index is
regenerated only when `git update-index --split-index` is used.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 read-cache.c           | 33 ++++++++++++++++++++++++++++++++-
 t/t1700-split-index.sh |  1 +
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/read-cache.c b/read-cache.c
index bb53823..a91fabe 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2216,6 +2216,36 @@ static int write_shared_index(struct index_state *istate,
 	return ret;
 }
 
+static const int default_max_percent_split_change = 20;
+
+int too_many_not_shared_entries(struct index_state *istate)
+{
+	int i, not_shared = 0;
+	int max_split = git_config_get_max_percent_split_change();
+
+	switch (max_split) {
+	case -1:
+		/* not or badly configured: use the default value */
+		max_split = default_max_percent_split_change;
+		break;
+	case 0:
+		return 1; /* 0% means always write a new shared index */
+	case 100:
+		return 0; /* 100% means never write a new shared index */
+	default:
+		; /* do nothing: just use the configured value */
+	}
+
+	/* Count not shared entries */
+	for (i = 0; i < istate->cache_nr; i++) {
+		struct cache_entry *ce = istate->cache[i];
+		if (!ce->index)
+			not_shared++;
+	}
+
+	return istate->cache_nr * max_split < not_shared * 100;
+}
+
 int write_locked_index(struct index_state *istate, struct lock_file *lock,
 		       unsigned flags)
 {
@@ -2233,7 +2263,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
 		if ((v & 15) < 6)
 			istate->cache_changed |= SPLIT_INDEX_ORDERED;
 	}
-	if (istate->cache_changed & SPLIT_INDEX_ORDERED) {
+	if (istate->cache_changed & SPLIT_INDEX_ORDERED ||
+	    too_many_not_shared_entries(istate)) {
 		int ret = write_shared_index(istate, lock, flags);
 		if (ret)
 			return ret;
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index db8c39f..507a1dd 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -8,6 +8,7 @@ test_description='split index mode tests'
 sane_unset GIT_TEST_SPLIT_INDEX
 
 test_expect_success 'enable split index' '
+	git config splitIndex.maxPercentChange 100 &&
 	git update-index --split-index &&
 	test-dump-split-index .git/index >actual &&
 	indexversion=$(test-index-version <.git/index) &&
-- 
2.10.1.462.g7e1e03a

[PATCH v1 11/19] t1700: add tests for splitIndex.maxPercentChange

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t1700-split-index.sh | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 507a1dd..f03addf 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -238,4 +238,76 @@ EOF
 	test_cmp expect actual
 '
 
+test_expect_success 'set core.splitIndex config variable to true' '
+	git config core.splitIndex true &&
+	: >three &&
+	git update-index --add three &&
+	BASE=$(test-dump-split-index .git/index | grep "^base") &&
+	test-dump-split-index .git/index | sed "/^own/d" >actual &&
+	cat >expect <<EOF &&
+$BASE
+replacements:
+deletions:
+EOF
+	test_cmp expect actual &&
+	: >four &&
+	git update-index --add four &&
+	test-dump-split-index .git/index | sed "/^own/d" >actual &&
+	cat >expect <<EOF &&
+$BASE
+100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	four
+replacements:
+deletions:
+EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'check behavior with splitIndex.maxPercentChange unset' '
+	git config --unset splitIndex.maxPercentChange &&
+	: >five &&
+	git update-index --add five &&
+	BASE=$(test-dump-split-index .git/index | grep "^base") &&
+	test-dump-split-index .git/index | sed "/^own/d" >actual &&
+	cat >expect <<EOF &&
+$BASE
+replacements:
+deletions:
+EOF
+	test_cmp expect actual &&
+	: >six &&
+	git update-index --add six &&
+	test-dump-split-index .git/index | sed "/^own/d" >actual &&
+	cat >expect <<EOF &&
+$BASE
+100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	six
+replacements:
+deletions:
+EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'check splitIndex.maxPercentChange set to 0' '
+	git config splitIndex.maxPercentChange 0 &&
+	: >seven &&
+	git update-index --add seven &&
+	BASE=$(test-dump-split-index .git/index | grep "^base") &&
+	test-dump-split-index .git/index | sed "/^own/d" >actual &&
+	cat >expect <<EOF &&
+$BASE
+replacements:
+deletions:
+EOF
+	test_cmp expect actual &&
+	: >eight &&
+	git update-index --add eight &&
+	BASE=$(test-dump-split-index .git/index | grep "^base") &&
+	test-dump-split-index .git/index | sed "/^own/d" >actual &&
+	cat >expect <<EOF &&
+$BASE
+replacements:
+deletions:
+EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.10.1.462.g7e1e03a

[PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config.txt | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 96521a4..380eeb8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2763,6 +2763,19 @@ showbranch.default::
 	The default set of branches for linkgit:git-show-branch[1].
 	See linkgit:git-show-branch[1].
 
+splitIndex.maxPercentChange::
+	When the split index feature is used, this specifies the
+	percent of entries the split index can contain compared to the
+	whole number of entries in both the split index and the shared
+	index before a new shared index is written.
+	The value should be between 0 and 100. If the value is 0 then
+	a new shared index is always written, if it is 100 a new
+	shared index is never written.
+	By default the value is 20, so a new shared index is written
+	if the number of entries in the split index would be greater
+	than 20 percent of the total number of entries.
+	See linkgit:git-update-index[1].
+
 status.relativePaths::
 	By default, linkgit:git-status[1] shows paths relative to the
 	current directory. Setting this variable to `false` shows paths
-- 
2.10.1.462.g7e1e03a

[PATCH v1 13/19] sha1_file: make check_and_freshen_file() non static

[Christian Couder]

This function will be used in a commit soon, so let's make
it available globally.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 cache.h     | 3 +++
 sha1_file.c | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index faceceb..a625b47 100644
--- a/cache.h
+++ b/cache.h
@@ -1167,6 +1167,9 @@ extern int has_pack_index(const unsigned char *sha1);
 
 extern void assert_sha1_type(const unsigned char *sha1, enum object_type expect);
 
+/* Helper to check and "touch" a file */
+extern int check_and_freshen_file(const char *fn, int freshen);
+
 extern const signed char hexval_table[256];
 static inline unsigned int hexval(unsigned char c)
 {
diff --git a/sha1_file.c b/sha1_file.c
index 266152d..29f927e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -576,7 +576,7 @@ static int freshen_file(const char *fn)
  * either does not exist on disk, or has a stale mtime and may be subject to
  * pruning).
  */
-static int check_and_freshen_file(const char *fn, int freshen)
+int check_and_freshen_file(const char *fn, int freshen)
 {
 	if (access(fn, F_OK))
 		return 0;
-- 
2.10.1.462.g7e1e03a

[PATCH v1 14/19] read-cache: touch shared index files when used

[Christian Couder]

When a split-index file is created, let's update the mtime of the
shared index file that the split-index file is referencing.

In a following commit we will make shared index file expire
depending on their mtime, so updating the mtime makes sure that
the shared index file will not be deleted soon.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 read-cache.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/read-cache.c b/read-cache.c
index a91fabe..3aeff77 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2268,6 +2268,12 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
 		int ret = write_shared_index(istate, lock, flags);
 		if (ret)
 			return ret;
+	} else {
+		/* Signal that the shared index is used */
+		const char *shared_index = git_path("sharedindex.%s",
+						    sha1_to_hex(si->base_sha1));
+		if (!check_and_freshen_file(shared_index, 1))
+			warning("could not freshen '%s'", shared_index);
 	}
 
 	return write_split_index(istate, lock, flags);
-- 
2.10.1.462.g7e1e03a

[PATCH v1 15/19] config: add git_config_get_date_string() from gc.c

[Christian Couder]

This function will be used in a following commit to get the expiration
time of the shared index files from the config, and it is generic
enough to be put in "config.c".

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/gc.c | 15 ++-------------
 cache.h      |  1 +
 config.c     | 13 +++++++++++++
 3 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 069950d..c1e9602 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -62,17 +62,6 @@ static void report_pack_garbage(unsigned seen_bits, const char *path)
 		string_list_append(&pack_garbage, path);
 }
 
-static void git_config_date_string(const char *key, const char **output)
-{
-	if (git_config_get_string_const(key, output))
-		return;
-	if (strcmp(*output, "now")) {
-		unsigned long now = approxidate("now");
-		if (approxidate(*output) >= now)
-			git_die_config(key, _("Invalid %s: '%s'"), key, *output);
-	}
-}
-
 static void process_log_file(void)
 {
 	struct stat st;
@@ -111,8 +100,8 @@ static void gc_config(void)
 	git_config_get_int("gc.auto", &gc_auto_threshold);
 	git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit);
 	git_config_get_bool("gc.autodetach", &detach_auto);
-	git_config_date_string("gc.pruneexpire", &prune_expire);
-	git_config_date_string("gc.worktreepruneexpire", &prune_worktrees_expire);
+	git_config_get_date_string("gc.pruneexpire", &prune_expire);
+	git_config_get_date_string("gc.worktreepruneexpire", &prune_worktrees_expire);
 	git_config(git_default_config, NULL);
 }
 
diff --git a/cache.h b/cache.h
index a625b47..bcfc0f1 100644
--- a/cache.h
+++ b/cache.h
@@ -1811,6 +1811,7 @@ extern int git_config_get_bool(const char *key, int *dest);
 extern int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest);
 extern int git_config_get_maybe_bool(const char *key, int *dest);
 extern int git_config_get_pathname(const char *key, const char **dest);
+extern int git_config_get_date_string(const char *key, const char **output);
 extern int git_config_get_untracked_cache(void);
 extern int git_config_get_split_index(void);
 extern int git_config_get_max_percent_split_change(void);
diff --git a/config.c b/config.c
index 5580f56..f88c61b 100644
--- a/config.c
+++ b/config.c
@@ -1685,6 +1685,19 @@ int git_config_get_pathname(const char *key, const char **dest)
 	return ret;
 }
 
+int git_config_get_date_string(const char *key, const char **output)
+{
+	int ret = git_config_get_string_const(key, output);
+	if (ret)
+		return ret;
+	if (strcmp(*output, "now")) {
+		unsigned long now = approxidate("now");
+		if (approxidate(*output) >= now)
+			git_die_config(key, _("Invalid %s: '%s'"), key, *output);
+	}
+	return ret;
+}
+
 int git_config_get_untracked_cache(void)
 {
 	int val = -1;
-- 
2.10.1.462.g7e1e03a

[PATCH v1 16/19] read-cache: unlink old sharedindex files

[Christian Couder]

Everytime split index is turned on, it creates a "sharedindex.XXXX"
file in the git directory. This change makes sure that shared index
files that haven't been used for a long time are removed when a new
shared index file is created.

The new "splitIndex.sharedIndexExpire" config variable is created
to tell the delay after which an unused shared index file can be
deleted. It defaults to "1.week.ago".

A previous commit made sure that each time a split index file is
created the mtime of the shared index file it references is updated.
This makes sure that recently used shared index file will not be
deleted.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 read-cache.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/read-cache.c b/read-cache.c
index 3aeff77..65ceb29 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2190,6 +2190,64 @@ static int write_split_index(struct index_state *istate,
 	return ret;
 }
 
+static const char *shared_index_expire = "1.week.ago";
+
+static unsigned long get_shared_index_expire_date(void)
+{
+	static unsigned long shared_index_expire_date;
+	static int shared_index_expire_date_prepared;
+
+	if (!shared_index_expire_date_prepared) {
+		git_config_get_date_string("splitindex.sharedindexexpire",
+					   &shared_index_expire);
+		shared_index_expire_date = approxidate(shared_index_expire);
+		shared_index_expire_date_prepared = 1;
+	}
+
+	return shared_index_expire_date;
+}
+
+static int can_delete_shared_index(const char *shared_sha1_hex)
+{
+	struct stat st;
+	unsigned long expiration;
+	const char *shared_index = git_path("sharedindex.%s", shared_sha1_hex);
+
+	/* Check timestamp */
+	expiration = get_shared_index_expire_date();
+	if (!expiration)
+		return 0;
+	if (stat(shared_index, &st))
+		return error_errno("could not stat '%s", shared_index);
+	if (st.st_mtime > expiration)
+		return 0;
+
+	return 1;
+}
+
+static void clean_shared_index_files(const char *current_hex)
+{
+	struct dirent *de;
+	DIR *dir = opendir(get_git_dir());
+
+	if (!dir) {
+		error_errno("unable to open git dir: %s", get_git_dir());
+		return;
+	}
+
+	while ((de = readdir(dir)) != NULL) {
+		const char *sha1_hex;
+		if (!skip_prefix(de->d_name, "sharedindex.", &sha1_hex))
+			continue;
+		if (!strcmp(sha1_hex, current_hex))
+			continue;
+		if (can_delete_shared_index(sha1_hex) > 0 &&
+		    unlink(git_path("%s", de->d_name)))
+			error_errno("unable to unlink: %s", git_path("%s", de->d_name));
+	}
+	closedir(dir);
+}
+
 static struct tempfile temporary_sharedindex;
 
 static int write_shared_index(struct index_state *istate,
@@ -2211,8 +2269,11 @@ static int write_shared_index(struct index_state *istate,
 	}
 	ret = rename_tempfile(&temporary_sharedindex,
 			      git_path("sharedindex.%s", sha1_to_hex(si->base->sha1)));
-	if (!ret)
+	if (!ret) {
 		hashcpy(si->base_sha1, si->base->sha1);
+		clean_shared_index_files(sha1_to_hex(si->base->sha1));
+	}
+
 	return ret;
 }
 
-- 
2.10.1.462.g7e1e03a

[PATCH v1 17/19] t1700: test shared index file expiration

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t1700-split-index.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index f03addf..f448fc1 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -310,4 +310,48 @@ EOF
 	test_cmp expect actual
 '
 
+test_expect_success 'shared index files expire after 7 days by default' '
+	: >ten &&
+	git update-index --add ten &&
+	test $(ls .git/sharedindex.* | wc -l) -gt 1 &&
+	just_under_7_days_ago=$((1-7*86400)) &&
+	test-chmtime =$just_under_7_days_ago .git/sharedindex.* &&
+	: >eleven &&
+	git update-index --add eleven &&
+	test $(ls .git/sharedindex.* | wc -l) -gt 1 &&
+	just_over_7_days_ago=$((-1-7*86400)) &&
+	test-chmtime =$just_over_7_days_ago .git/sharedindex.* &&
+	: >twelve &&
+	git update-index --add twelve &&
+	test $(ls .git/sharedindex.* | wc -l) = 1
+'
+
+test_expect_success 'check splitIndex.sharedIndexExpire set to 8 days' '
+	git config splitIndex.sharedIndexExpire "8.days.ago" &&
+	test-chmtime =$just_over_7_days_ago .git/sharedindex.* &&
+	: >thirteen &&
+	git update-index --add thirteen &&
+	test $(ls .git/sharedindex.* | wc -l) -gt 1 &&
+	just_over_8_days_ago=$((-1-8*86400)) &&
+	test-chmtime =$just_over_8_days_ago .git/sharedindex.* &&
+	: >fourteen &&
+	git update-index --add fourteen &&
+	test $(ls .git/sharedindex.* | wc -l) = 1
+'
+
+test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now"' '
+	git config splitIndex.sharedIndexExpire never &&
+	just_10_years_ago=$((-365*10*86400)) &&
+	test-chmtime =$just_10_years_ago .git/sharedindex.* &&
+	: >fifteen &&
+	git update-index --add fifteen &&
+	test $(ls .git/sharedindex.* | wc -l) -gt 1 &&
+	git config splitIndex.sharedIndexExpire now &&
+	just_1_second_ago=-1 &&
+	test-chmtime =$just_1_second_ago .git/sharedindex.* &&
+	: >sixteen &&
+	git update-index --add sixteen &&
+	test $(ls .git/sharedindex.* | wc -l) = 1
+'
+
 test_done
-- 
2.10.1.462.g7e1e03a

[PATCH v1 18/19] Documentation/config: add splitIndex.sharedIndexExpire

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 380eeb8..5212a97 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2776,6 +2776,17 @@ splitIndex.maxPercentChange::
 	than 20 percent of the total number of entries.
 	See linkgit:git-update-index[1].
 
+splitIndex.sharedIndexExpire::
+	When the split index feature is used, shared index files with
+	a mtime older than this time will be removed when a new shared
+	index file is created. The value "now" expires all entries
+	immediately, and "never" suppresses expiration altogether.
+	The default value is "one.week.ago".
+	Note that each time a new split-index file is created, the
+	mtime of the related shared index file is updated to the
+	current time.
+	See linkgit:git-update-index[1].
+
 status.relativePaths::
 	By default, linkgit:git-status[1] shows paths relative to the
 	current directory. Setting this variable to `false` shows paths
-- 
2.10.1.462.g7e1e03a

[PATCH v1 19/19] Documentation/git-update-index: explain splitIndex.*

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-update-index.txt | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
index e091b2a..635d157 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -163,14 +163,10 @@ may not support it yet.
 
 --split-index::
 --no-split-index::
-	Enable or disable split index mode. If enabled, the index is
-	split into two files, $GIT_DIR/index and $GIT_DIR/sharedindex.<SHA-1>.
-	Changes are accumulated in $GIT_DIR/index while the shared
-	index file contains all index entries stays unchanged. If
-	split-index mode is already enabled and `--split-index` is
-	given again, all changes in $GIT_DIR/index are pushed back to
-	the shared index file. This mode is designed for very large
-	indexes that take a significant amount of time to read or write.
+	Enable or disable split index mode. If split-index mode is
+	already enabled and `--split-index` is given again, all
+	changes in $GIT_DIR/index are pushed back to the shared index
+	file.
 +
 These options take effect whatever the value of the `core.splitIndex`
 configuration variable (see linkgit:git-config[1]). But a warning is
@@ -394,6 +390,27 @@ Although this bit looks similar to assume-unchanged bit, its goal is
 different from assume-unchanged bit's. Skip-worktree also takes
 precedence over assume-unchanged bit when both are set.
 
+Split index
+-----------
+
+This mode is designed for very large indexes that take a significant
+amount of time to read or write.
+
+In this mode, the index is split into two files, $GIT_DIR/index and
+$GIT_DIR/sharedindex.<SHA-1>. Changes are accumulated in
+$GIT_DIR/index, the split index, while the shared index file contains
+all index entries and stays unchanged.
+
+All changes in the split index are pushed back to the shared index
+file when the number of entries in the split index reaches a level
+specified by the splitIndex.maxPercentChange config variable (see
+linkgit:git-config[1]).
+
+Each time a new shared index file is created, the old shared index
+files are deleted if they are older than what is specified by the
+splitIndex.sharedIndexExpire config variable (see
+linkgit:git-config[1]).
+
 Untracked cache
 ---------------
 
-- 
2.10.1.462.g7e1e03a

Re: [PATCH v1 10/19] read-cache: regenerate shared index if necessary

[Ramsay Jones]

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

This function is a file-loacal symbol; could you please make it
a static function.

Thanks.

ATB,
Ramsay Jones

Re: [PATCH v1 00/19] Add configuration options for split-index

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

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

I admit I haven't thought it through, but this sounds OK.

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

OK.

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

I do not quite understand this justification.  Doesn't each of the
"this hold only changes since the base index file" files have a
backpointer that names the base index file it is a delta against?

Is it safe to remove a base index file when there is still a split
index file that points at it?  

IOW, I do not see why it can be safe for the expiration decision to
be based on timestamp (I would understand it if it were based on a
refcnt, though).

Re: [PATCH v1 00/19] Add configuration options for split-index

[Duy Nguyen]

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

Yes, but the shared file does not have pointers to all the files that
need it, which could be more than one. We know one of them,
$GIT_DIR/index, and possibly $GIT_DIR/index.lock too. But those files
people generate manually and refer to them with $GIT_INDEX_FILE, we
can't know where they are.

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

Problem is we can't maintain these ref counts cheap and simple. We
don't want to update sharedindex file every time somebody references
to it (or stops referencing to it) because that defeats the purpose of
splitting it out and not touching it any more. Adding a separate file
for ref count could work, but it gets complex, especially when we
think about race condition at update time.

Timestamps allow us to say, ok this base index file has not been read
by anybody for N+ hours (or better, days), it's most likely not
referenced by any temporary index files (including
$GIT_DIR/index.lock) anymore because those files, by the definition of
"temporary", must be gone by now. We should definitely check and make
sure the file $GIT_DIR/index points to still exist. I'm going to read
the series now, so I don't know if the previous sentence is true.

It will probably be harder to handle race condition at updating
$GIT_DIR/index, which could be avoided by a sufficiently long grace
period with timestamps.
-- 
Duy

Re: [PATCH v1 03/19] split-index: add {add,remove}_split_index() functions

[Duy Nguyen]

On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> +void remove_split_index(struct index_state *istate)
> +{
> +       if (istate->split_index) {
> +               /*
> +                * can't discard_split_index(&the_index); because that
> +                * will destroy split_index->base->cache[], which may
> +                * be shared with the_index.cache[]. So yeah we're
> +                * leaking a bit here.

In the context of update-index, this is a one-time thing and leaking
is tolerable. But because it becomes a library function now, this leak
can become more serious, I think.

The only other (indirect) caller is read_index_from() so probably not
bad most of the time (we read at the beginning of a command only).
sequencer.c may discard and re-read the index many times though,
leaking could be visible there.

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

Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency

[Duy Nguyen]

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

Wrap this string and the one below with _() so they can be translated.

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

Re: [PATCH v1 09/19] config: add git_config_get_max_percent_split_change()

[Duy Nguyen]

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

We should keep camelCase form for easy reading. And wrap this string with _().

> +                     "should be between 0 and 100", val);

I wonder if anybody would try to put 12.3 here and confused by the
error message, because 0 <= 12.3 <= 100, but it's not an integer..
Ah.. never mind, die_bad_number() would be called first in this case
with a loud and clear complaint.

> +               return -1;
> +       }
> +
> +       return -1; /* default value */
-- 
Duy

Re: [PATCH v1 10/19] read-cache: regenerate shared index if necessary

[Duy Nguyen]

On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> @@ -2233,7 +2263,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
>                 if ((v & 15) < 6)
>                         istate->cache_changed |= SPLIT_INDEX_ORDERED;
>         }
> -       if (istate->cache_changed & SPLIT_INDEX_ORDERED) {
> +       if (istate->cache_changed & SPLIT_INDEX_ORDERED ||
> +           too_many_not_shared_entries(istate)) {

It's probably safer to keep this piece unchanged and add this
somewhere before it

if (too_many_not_shared_entries(istate))
    istate->cache_changed |= SPLIT_INDEX_ORDERED;

We could keep cache_changed consistent until the end this way.

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

An alternative name might be splitThreshold. I don't know, maybe
maxPercentChange is better.

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



-- 
Duy

Re: [PATCH v1 14/19] read-cache: touch shared index files when used

[Duy Nguyen]

On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> @@ -2268,6 +2268,12 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,

Doing this in read_index_from() would keep the shared file even more
"fresher" since read happens a lot more often than write. But I think
our main concern is not the temporary index files created by the user
scripts, but $GIT_DIR/index.lock (make sure we don't accidentally
delete its shared file before it gets renamed to $GIT_DIR/index). For
this case, I think refreshing in write_locked_index is enough.

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

_()
-- 
Duy

Re: [PATCH v1 16/19] read-cache: unlink old sharedindex files

[Duy Nguyen]

On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> +static int can_delete_shared_index(const char *shared_sha1_hex)
> +{
> +       struct stat st;
> +       unsigned long expiration;
> +       const char *shared_index = git_path("sharedindex.%s", shared_sha1_hex);
> +
> +       /* Check timestamp */
> +       expiration = get_shared_index_expire_date();
> +       if (!expiration)
> +               return 0;
> +       if (stat(shared_index, &st))
> +               return error_errno("could not stat '%s", shared_index);
> +       if (st.st_mtime > expiration)

I wonder if we should check ctime too, in case mtime is not reliable
(and ctime is less likely to be manipulated by user), just for extra
safety. If (st.st_mtime > expiration || st.st_ctime > expiration).

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

_()

> +               return;

Or just do "return error_errno(...)". The caller can ignore the return
value for now.

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

Yeah.. make sure that the shared index linked to $GIT_DIR/index stay,
even if mtime is screwed up. I wonder if we should have the same
treatment for $GIT_DIR/index.lock though, as an extra safety measure.
If you call this function in write_locked_index, then
$GIT_DIR/index.lock is definitely there. Hmm.. maybe it _is_
current_hex, current_hex is not the value from $GIT_DIR/index...

> +               if (can_delete_shared_index(sha1_hex) > 0 &&
> +                   unlink(git_path("%s", de->d_name)))
> +                       error_errno("unable to unlink: %s", git_path("%s", de->d_name));

_()

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

This operation is technically garbage collection and should belong to
"git gc --auto", which is already called automatically in a few
places. Is it not called often enough that we need to do the cleaning
up right after a new shared index is created?
-- 
Duy

Re: [PATCH v1 00/19] Add configuration options for split-index

[Duy Nguyen]

On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> Goal
> ~~~~
>
> We want to make it possible to use the split-index feature
> automatically by just setting a new "core.splitIndex" configuration
> variable to true.

Thanks. This definitely should help make split index a lot more
convenient for day-to-day use. The series looks ok (well, except the
pruning logic being discussed elsewhere in this thread, but I still
think it's a good strategy).

> This can be valuable as split-index can help significantly speed up
> `git rebase` especially along with the work to libify `git apply`
> that has been recently merged to master
> (see https://github.com/git/git/commit/81358dc238372793b1590efa149cc1581d1fbd98).

I remember this. Since the apply libification work has landed, we can
now think about not writing index out after every apply step, which
gives the same (or more) speed up without the complication of
split-index. But if split-index is good enough for you, we can leave
it for another time.
-- 
Duy

Re: [PATCH v1 00/19] Add configuration options for split-index

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> ... But those files
> people generate manually and refer to them with $GIT_INDEX_FILE, we
> can't know where they are.

Then we probably should stop doing that, i.e. disable split-index
automatically for these temporary ones, perhaps, and even with
Christian's series which allows use of split index on the primary
one easier (which is a good idea), make sure we don't auto split the
temporary ones the users create?

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

and if we guessed wrong, users will have a "temporary index" that
they meant to keep for longer term that is now broken here.  I am
not sure if that risk is worth taking.

Re: [PATCH v1 00/19] Add configuration options for split-index

[Duy Nguyen]

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

Even if we ignore user index files (by forcing them all to be stored
in one piece), there is a problem with the special temporary file
index.lock, which must use split-index because it will become the new
index. Handling race conditions could be tricky with ref counting.
Timestamps help in this regard.
-- 
Duy

Re: [PATCH v1 00/19] Add configuration options for split-index

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

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

I actually think using the split-index only for the $GIT_DIR/index,
the primary one, and using the full index for others is a bad idea,
as we use temporary index ourselves when making partial commits,
which happens quite often.

So time-based GC it is.

I actually do not think index.lock is a problem, but is a solution,
if we only limit the split-index to the primary one (we can have at
most one, so we can GC the stale shared ones while holding the
lock).

But that is no longer important.

Re: [PATCH v1 16/19] read-cache: unlink old sharedindex files

[Duy Nguyen]

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

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

git-gc just can't match this because while it's running, somebody else
may be updating $GIT_DIR/index. Handling races would be a lot harder.
-- 
Duy

Re: [PATCH v1 16/19] read-cache: unlink old sharedindex files

[Christian Couder]

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

Yeah, I agree that if we disable split-index on temporary files and on
commands like "git read-tree --index-output=<file>" then it is the
right thing to do it in write_shared_index(). (In fact when I wrote
the previous RFC series I didn't think about those special cases, and
that was the reason why I did it like this. So I just need to go back
to the implementation that was in the previous RFC series.)

I am just still wondering if disabling split-index on temporary files
could not have a bad performance impact for some use cases, but I
guess we could always come back to problem again if that happens.

Thanks,
Christian.

Re: [PATCH v1 16/19] read-cache: unlink old sharedindex files

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

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

Yes, that exactly was why I wondered if we can afford to limit
splitting only to the primary index, because it makes things a
lot simpler.

But I suspect that temporary index is where split-index shines most,
e.g. while creating a partial commit.  The mechanism penalizes the
read performance by making the format more complex in order to favor
the write performance, which is very much suited for temporary one
that is read only once after it is written before it gets discarded
(on the other hand, splitting the primary index will penalize reads
that happen a lot more than writes).

While I still find it attractive at the conceptual level to limit
splitting only to the primary index for the resulting simplicity,
I doubt it is a good way to go, as I meant to say in
<xmqqeg33ccjj.fsf@gitster.mtv.corp.google.com>

> git-gc just can't match this because while it's running, somebody else
> may be updating $GIT_DIR/index. Handling races would be a lot harder.

It could attempt to take a lock on the primary index while it runs,
and refrain to do anything if it can't take the lock ("gc --auto"
may want to silently retry), and then the race is no longer an
issue, no?

Re: [PATCH v1 16/19] read-cache: unlink old sharedindex files

[Duy Nguyen]

On Thu, Oct 27, 2016 at 11:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> git-gc just can't match this because while it's running, somebody else
>> may be updating $GIT_DIR/index. Handling races would be a lot harder.
>
> It could attempt to take a lock on the primary index while it runs,
> and refrain to do anything if it can't take the lock ("gc --auto"
> may want to silently retry), and then the race is no longer an
> issue, no?

No, I thought of that. But if gc is holding the lock, another program
that wants to update the index may fail. And git-gc is supposed to be
non-intrusive
-- 
Duy

Re: [PATCH v1 03/19] split-index: add {add,remove}_split_index() functions

[Christian Couder]

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

So is it enough to check if split_index->base->cache[] is shared with
the_index.cache[] and then decide if discard_split_index(&the_index)
should be called?

Thanks,
Christian.

Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency

[Christian Couder]

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

Ok, it will be in the next version.

Re: [PATCH v1 09/19] config: add git_config_get_max_percent_split_change()

[Christian Couder]

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

Ok, it will be in the next version.

Thanks,
Christian.

Re: [PATCH v1 10/19] read-cache: regenerate shared index if necessary

[Christian Couder]

On Sun, Oct 23, 2016 at 6:07 PM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>>
>> +int too_many_not_shared_entries(struct index_state *istate)
>
> This function is a file-loacal symbol; could you please make it
> a static function.

Ok, it will be in the next version.

Thanks,
Christian.

Re: [PATCH v1 10/19] read-cache: regenerate shared index if necessary

[Christian Couder]

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

Ok, it will be in the next version.

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

I think it is important to say that it is a percent in the name, so I
prefer maxPercentChange.

Thanks,
Christian.

Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

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

True.  

I further wonder if a natural reaction from users after seeing this
message is "I do want to--what else would I use that option to run
you for?  Just do as you are told, instead of telling me what to
do!".  Is this warning really a good idea, or shouldn't these places
be setting the configuration?

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

Re: [PATCH v1 09/19] config: add git_config_get_max_percent_split_change()

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

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

OK.

>
>> +               return -1;

Perhaps do the usual

	return error(_("..."));

here?

>> +       }
>> +
>> +       return -1; /* default value */

Re: [PATCH v1 11/19] t1700: add tests for splitIndex.maxPercentChange

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

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

Using <<-EOF lets us indent the above four lines with a horizontal
tab to align with the remainder of this test_expect_success block,
so let's do that.

Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

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

Hmph.  The early part of the description implies this will kick in
only when some other conditions (i.e. the bit in the index or the
other configuration) are met, but if this disables the split index
when it is set to 0, would we even need the other configuration
variable?  IOW, perhaps we can do without core.splitIndex?

> +	By default the value is 20, so a new shared index is written
> +	if the number of entries in the split index would be greater
> +	than 20 percent of the total number of entries.
> +	See linkgit:git-update-index[1].

Re: [PATCH v1 14/19] read-cache: touch shared index files when used

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

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

Also warning() is unwarranted.

You may be accessing somebody else's repository to help diagnose the
issue without having any write access.  Treat the utime() like the
opportunistic index refresh done by "git status"---if we can write,
great, but it is not a problem if we can't.

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

Re: [PATCH v1 15/19] config: add git_config_get_date_string() from gc.c

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> This function will be used in a following commit to get the expiration
> time of the shared index files from the config, and it is generic
> enough to be put in "config.c".

Is it generic enough that a helper that sounds as if it can get any
date string dies if it is given a future date?  I somehow doubt it.

At the minimum, it must be made clear that there is an artificial
limitation that the current set of callers find useful in cache.h as
a one-liner comment next to the added declaration.  Then people with
the same need (i.e. they want to reject future timestamps) can
decide to use it, while others would stay away from it.  

If you can come up with a better word to use to encode that
artificial limitation in its name, renaming it is even better.

Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency

[Christian Couder]

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

In 435ec090ec (config: add core.untrackedCache, 2016-01-27) we decided
to just use warning() after discussing if we should instead set the
configuration.

Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

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

Yeah that seems to be the version that was committed.  I then wonder
if the users would naturally have a similar raction to that warning
as well.

Re: [PATCH v1 00/19] Add configuration options for split-index

[Christian Couder]

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

Thanks for your review.

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

Yeah, I think they are separate things that can be done.
When people do a lot of small rebases for example, then split-index
will speed up all the index writing, including the index writing at
the end of each rebase.

Re: [PATCH v1 09/19] config: add git_config_get_max_percent_split_change()

[Christian Couder]

On Tue, Nov 1, 2016 at 8:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>> +               return -1;
>
> Perhaps do the usual
>
>         return error(_("..."));
>
> here?

Ok, it will be in the next version.

Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange

[Christian Couder]

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

I think it is easier for user to be able to just set core.splitIndex
to true to enable split-index.
If we ask them to instead set splitIndex.maxPercentChange to a
"reasonable value like 20" to enable it, they will start to wonder why
20 and not 50 or 100 and might be unnecessarily confused.

Also it might be good to make it possible to have other values than
"true" and "false" for core.splitIndex in the future.
For example if we decide to first enable split-index only on
$GIT_DIR/index, we might later want an "everywhere" or "always" value
for core.splitIndex if we find a way to enable it on other indexes.

We might also want an "auto" mode in the future when split-index will
work really well, so that it can be turned on automatically on all the
repos where it makes sense (like when the index contains more than
10000 entries for example).

>> +     By default the value is 20, so a new shared index is written
>> +     if the number of entries in the split index would be greater
>> +     than 20 percent of the total number of entries.
>> +     See linkgit:git-update-index[1].

Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> I think it is easier for user to be able to just set core.splitIndex
> to true to enable split-index.

You can have that exact benefit by making core.splitIndex to
bool-or-more.  If your default is 20%, take 'true' as if the user
specified 20% and take 'false' as if the user specified 100% (or is
it 0%?  I do not care about the details but you get the point).

Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange

[Duy Nguyen]

(sorry I got sick in the last few weeks and could not respond to this earlier)

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

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

Re: [PATCH v1 03/19] split-index: add {add,remove}_split_index() functions

[Duy Nguyen]

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

It's likely shared though. We could un-share cache[] by duplicating
index entries in the_index.cache[] if they point back to
split_index->base (we know what entries are shared by examining the
"index" field). Once we do that, we can discard_split_index()
unconditionally. There's another place that has similar leak:
move_cache_to_base_index(), which could receive the same treatment.
-- 
Duy

Re: [PATCH v1 03/19] split-index: add {add,remove}_split_index() functions

[Duy Nguyen]

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

I have a patch for this. So don't have to do anything else in this
area. I'll probably just pile my patch on top of your series, or post
it once the series graduates to master.
-- 
Duy

Re: [PATCH v1 03/19] split-index: add {add,remove}_split_index() functions

[Christian Couder]

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

Great, thanks!

Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange

[Christian Couder]

On Mon, Nov 7, 2016 at 10:38 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> (sorry I got sick in the last few weeks and could not respond to this earlier)

(Yeah, I have also been sick during the last few weeks.)

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

(Sorry for writing the above on my phone which added HTML, so that it
didn't reach the list.)

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

The meaning of a future "auto" option for "core.splitIndex" could be
"use the split-index feature only if the number of entries in whole
index is greater than 10000 (by default)".

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

Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange

[Duy Nguyen]

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

Well.. with the "just re-split the index when it makes sense" part,
the user entrusts git to do something sensible in all cases, and going
with absolute numbers might not be the best way, I think. It's big
responsibility :)

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

Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange

[Christian Couder]

On Tue, Nov 22, 2016 at 11:35 AM, Duy Nguyen <pclouds@gmail.com> wrote:

[...]

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

That's an interpretation of what "core.splitIndex=true" could mean,
but there could be users who trust Git to re-split when it makes
sense, but who do want to use the split-index on all theirs repos even
the small ones or who just don't trust Git to choose when it might be
better to use it or not.

Yeah, a typical git user would most of the time just trust Git for all
those things, but on the other hand there are companies out there that
are willing to tweak many configuration options to get the better
possible behavior for them.

In fact I am working on this for Booking.com, and if we find out later
that we would gain something significant, like performance
improvements or configuration simplification, by adding "auto" and/or
other configuration variables to tweak more split-index related
things, we might very well post patch series to do that.

So if we now mix things up just to avoid one more configuration
option, we could very well make things harder to develop, to
configure, to parse and to understand later, so it is not a trade off
worth making.

> and going
> with absolute numbers might not be the best way, I think. It's big
> responsibility :)

About going with absolute number, yeah I am not sure at all it is the
best way, but this was just part of an example to try to explain what
I am saying above.

Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange

[Duy Nguyen]

On Tue, Nov 22, 2016 at 8:13 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> So if we now mix things up just to avoid one more configuration
> option, we could very well make things harder to develop, to
> configure, to parse and to understand later, so it is not a trade off
> worth making.

OK since we're still in the early phase of determining how to
re-split, I guess going with elaborate, precise configuration is
better, even if it's more config options. Once we know better, maybe
we can decide a good default cover-all "auto" then (or maybe not
because we find out no shoe fits all).
-- 
Duy

Re: [PATCH v1 15/19] config: add git_config_get_date_string() from gc.c

[Christian Couder]

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

Ok it will appear like this in cache.h:

/* This dies if the configured or default date is in the future */
extern int git_config_get_expire_date_string(const char *key, const
char **output);

Thanks,
Christian.

Re: [PATCH v1 15/19] config: add git_config_get_date_string() from gc.c

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

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

Those who imitate existing callsites never read comments, and you
need to spend effort to get the name right to protect the codebase
from them.

"get-expiry" may be shorter.  Neither still does not say it will
die, though.

Re: [PATCH v1 15/19] config: add git_config_get_date_string() from gc.c

[Christian Couder]

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

What about something like this then:

/* This dies if the configured or default date is in the future */
extern int git_config_get_expiry_or_die(const char *key, const char **output);

Also git_config_get_int(), git_config_get_bool() and probably other
such functions are indirectly calling git_config_int() which die()s is
the value is not a number, so it feels a bit strange to have only one
function with a name that ends with "_or_die" when many other
functions could die(). In fact it could give a false sense of security
to those who just read cache.h.

Re: [PATCH v1 15/19] config: add git_config_get_date_string() from gc.c

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

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

It is half-a-good-point that existing functions would die and I
agree with you that get_expiry_or_die can lose _or_die part.

But get_int() dying when it is fed something that is not "int" (or
something that cannot be returned in "int") is one thing; get_date()
being happy when it is fed a string that is a valid date 2015-01-01
but dying when fed another valid date 2018-01-01 is quite puzzling.

I think get_expiry() is OK.  

Q: What's expiry?  How is it different from date
A: A past timestamp that is speifies the cutoff time for the purpose
   of expiring old stuff.  You can say "now" to expire everything
   you have.

is more pleasant and understandable conversation than

Q: Why can I feed only past date to get_date()?  Otherwise it dies.
A: ...?  The people who named the function didn't know what "date" was?