v3, 21 partsAdd configuration options for split-index

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 merged to master (see https://github.com/git/git/commit/81358dc238372793b1590efa149cc1581d1fbd98) and is now in v2.11.

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% 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 a split index file using the shared index file is
    either created or read from. 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.

    Junio also thinks that we have to do "time-based GC" in:
 
    https://public-inbox.org/git/xmqqeg33ccjj.fsf@gitster.mtv.corp.google.com/

Note that this patch series doesn't address a leak when removing a split-index, but Duy wrote that he has a patch to fix this leak:

https://public-inbox.org/git/CACsJy8AisF2ZVs7JdnVFp5wdskkbVQQQ=DBq5UzE1MOsCfBMtQ@mail.gmail.com/

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

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

There only a few small differences between this patch series and the v2 patch series sent a few weeks ago. Only two commits have been changed a little, as suggested by Duy.

    - Patch 1/21 marks a message for translation. It was a new patch
      in v2 and it can be applied separately. (The patch 1/19 in v1,
      which was a typo fix, has been merged separately into master
      already.)

Step 1 is:

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

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

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

    The only change since v2 in this step is that an unnecessary
    variable initialization has been droped in 2/21, as suggested by
    Duy.

Step 2 is:

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

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

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

    There are no changes in this step since v2.

Step 3 is:

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

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

    - Patches 18/21 and 19/21 are new patches. They update the mtime
      of the shared index file when a split index based on the shared
      index file is read from. 18/21 is a refactoring to make the
      actual change in 19/21 easier.

    - Patches 20/21 and 21/21 add some documentation for the new
      feature.

    The only change since v2 in this step is that the full shared
    index path is passed to the can_delete_shared_index() function in
    16/21 as suggested by Duy.

Links ~~~~~

This patch series is also available here:

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

The previous versions were:

  RFC: https://github.com/chriscool/git/commits/config-split-index7
  v1:  https://github.com/chriscool/git/commits/config-split-index72
  v2:  https://github.com/chriscool/git/commits/config-split-index99

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

  RFC: https://public-inbox.org/git/20160711172254.13439-1-chriscool@tuxfamily.org/
  v1:  https://public-inbox.org/git/20161023092648.12086-1-chriscool@tuxfamily.org/
  v2:  https://public-inbox.org/git/20161217145547.11748-1-chriscool@tuxfamily.org/

Christian Couder (21):
  config: mark an error message up for translation
  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_expiry() from gc.c
  read-cache: unlink old sharedindex files
  t1700: test shared index file expiration
  read-cache: refactor read_index_from()
  read-cache: use freshen_shared_index() in read_index_from()
  Documentation/config: add splitIndex.sharedIndexExpire
  Documentation/git-update-index: explain splitIndex.*

 Documentation/config.txt           |  28 +++++++
 Documentation/git-update-index.txt |  43 +++++++++--
 builtin/gc.c                       |  15 +---
 builtin/update-index.c             |  25 +++---
 cache.h                            |   8 ++
 config.c                           |  42 +++++++++-
 read-cache.c                       | 143 ++++++++++++++++++++++++++++++++--
 sha1_file.c                        |   2 +-
 split-index.c                      |  22 ++++++
 split-index.h                      |   2 +
 t/t1700-split-index.sh             | 154 +++++++++++++++++++++++++++++++++++++
 11 files changed, 442 insertions(+), 42 deletions(-)

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

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(+)

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(-)

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(+)

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 | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

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           | 32 ++++++++++++++++++++++++++++++++
 t/t1700-split-index.sh |  1 +
 2 files changed, 33 insertions(+)

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

Just like you did in 04/21, write "break" to avoid mistakes made in the future, i.e.

	default:
		break; /* just use the configured value */

On a 32-bit arch with 2G int and more than 20 million paths in the index, multiplying by max_split that can come close to 100 can theoretically cause integer overflow, but in practice it probably does not matter. Or does it?

On Tue, Dec 27, 2016 at 8:08 PM, Junio C Hamano <gitster@pobox.com> wrote:

Ok, I will do that.

From a cursory look a "struct cache_entry" takes at least 80 bytes without counting the "char name[FLEX_ARRAY]" on a 32 bit machine, so I don't think it would be a good idea to work on a repo with 20 million paths on a 32 bit machine, but maybe theoretically it could be a problem.

To be safe I think I will use:

return (int64_t)istate->cache_nr * max_split < (int64_t)not_shared * 100;

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(-)

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

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      |  3 +++
 config.c     | 13 +++++++++++++
 3 files changed, 18 insertions(+), 13 deletions(-)

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

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

s/whole/total/ to match the last sentence of this section, perhaps? "The number of all entries" would also be OK, but "the whole number of entries" sounds a bit strange.

On Tue, Dec 27, 2016 at 8:09 PM, Junio C Hamano <gitster@pobox.com> wrote:

Yeah "total" is better than "whole" here. I will use "total".

This way a share index file will not be garbage collected if we still read from an index it is based from.

As we need to read the current index before creating a new one, the tests have to be adjusted, so that we don't expect an old shared index file to be deleted right away when we create a new one.

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

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

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

As end-user facing documentation, it would be much better if we can rephrase it for those who do not know what a 'mtime' is, and it would be even better if we can do so without losing precision.

I think "shared index files that were not modified since the time this variable specifies will be removed" would be understandable and correct enough?

To match the above suggestion, "Note that a shared index file is considered modified (for the purpose of expiration) each time a new split-index file is created based on it."?

On Tue, Dec 27, 2016 at 8:11 PM, Junio C Hamano <gitster@pobox.com> wrote:

Yeah, I agree it is better for end users. I will use what you suggest.

Yeah, I also agree it is better and will use that.

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config.txt           |  6 +++---
 Documentation/git-update-index.txt | 37 +++++++++++++++++++++++++++++--------
 2 files changed, 32 insertions(+), 11 deletions(-)

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

In the world after this series that introduced the percentage-based auto consolidation, it smells strange, or even illogical, that index is un-split after doing this:

    $ git update-index --split-index
    $ git update-index --split-index

Before this series, it may have been a quick and dirty way to force consolidating the split index without totally disabling the feature, i.e. it would have looked more like

    $ git update-index --split-index
    ... work work work to accumulate more modifications
    ... consolidate into one --- there was no other way than
    ... disabling it temporarily
    $ git update-index --no-split-index
    ... but the user likes the feature so re-enable it.
    $ git update-index --split-index

which I guess is where this strange behaviour comes from.

It may be something we need to fix to unconfuse the end-users after this series lands. Even though "first disable and then re-enable" takes two commands (as opposed to one), it is far more logical. And the percentage-based auto consolidation feature is meant to reduce the need for manual consolidation, so it probably makes sense to remove this illogical feature.

This is not a new problem, but it probably is incorrect to say "to read or write". It saves time by not rewriting the whole thing but instead write out only the updated bits. You'd still read the whole thing while populating the in-core index from the disk, and if anything, you'd probably spend _more_ cycles because you'd essentially be reading the base and then reading the delta to apply on top.

The same comment on the mention of "mtime" in another patch applies here as well.

On Tue, Dec 27, 2016 at 8:13 PM, Junio C Hamano <gitster@pobox.com> wrote:

Yeah, I tend to agree that this feature could be removed later. Though before removing it, I'd like to hear Duy's opinion on this as he created the feature in the first place.

Ok, then what about:

+This mode is designed for repositories with very large indexes, and aims +at reducing the time it takes to repeatedly write these indexes.

Ok, I will use "modification time" instead of "mtime".

Thanks.

It looks better and is simpler to review when we don't compute the same things many times in the function.

It will also help make the following commit simpler.

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

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 | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

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 | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

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

What happens when this call fails? The function returns 0 if the file did not even exist. It also returns 0 if you cannot update its timestamp.

Shouldn't the series be exposing freshen_file() instead _and_ taking its error return value seriously?

On Tue, Dec 27, 2016 at 8:10 PM, Junio C Hamano <gitster@pobox.com> wrote:

Yeah and I don't think it's a problem in either case. If we cannot update its timestamp, it's not a problem, as we could be on a read-only file system, and you said in a previous iteration that we should not even warn in this case. If the file does not exist, it could be because it has just been deleted for a good reason, and anyway, if it is a problem that the shared index file has been deleted, it is better addressed when we will actually need the shared index file to read something into from it, rather than just to update its mtime.

So what should we do if freshen_file() returns 0 which means that the freshening failed?

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

You tell me ;-)  as you are the one who is proposing this feature.

Isn't a failure to freshen it a grave error? We are letting a base/shared index file that is known to be in-use go stale and eventually subject for garbage collection, and the user should be notified in some way before the actual GC happens that renders the index file unusable?

What is the failure mode after such a premature GC happens? What does the end-user see? Can you try to (1) split the index (2) modify bunch of entries (3) remove the base/shared index with /bin/rm and then see how various Git commands fail? Do they fail gracefully?

I am trying to gauge the seriousness of ignoring such an error here.

On Sun, Jan 8, 2017 at 4:46 AM, Junio C Hamano <gitster@pobox.com> wrote:

My answer is, we are not worse than freshening loose objects case (especially since I took the idea from there). In both cases we silently ignore utime() error (sha1_file.c does retry, but will do nothing else if the retry fails). And errors in odb are much more serious than index files. If we are to improve it, I think we should do it inside check_and_freshen_file(), maybe with an optional flag to silence it.

If we fail to refresh it and the file is old enough and gc happens, any index file referenced to it are broken. Any commands that read the index will die(). The best you could do is delete $GIT_DIR/index and read-tree HEAD.

Duy Nguyen <pclouds@gmail.com> writes:

I do not think so, unfortunately. Loose object files with stale timestamps are not removed as long as objects are still reachable. For the base/shared index file, the timestamp is the only thing that protects them from pruning, unless it is serving as the base file for the currently active $GIT_DIR/index that is split.

On Mon, Jan 9, 2017 at 9:34 PM, Junio C Hamano <gitster@pobox.com> wrote:

But there are plenty of unreachable loose objects, added in index, then got replaced with new versions. cache-tree can create loose trees too and it's been run more often, behind user's back, to take advantage of the shortcut in unpack-trees.

Duy Nguyen <pclouds@gmail.com> writes:

I am not sure if I follow. Aren't objects reachable from the cache-tree in the index protected from gc?

Not that I think freshening would actually fail in a repository where you can actually write into to update the index or its refs to make a difference (iow, even if we make it die() loudly when shared index cannot be "touched" because we are paranoid, no real life usage will trigger that die(), and if a repository does trigger the die(), I think you would really want to know about it).

On Fri, Jan 20, 2017 at 2:00 AM, Junio C Hamano <gitster@pobox.com> wrote:

I think the problem is loose objects created then gc run just before they are referenced (e.g. index written down). But I think I may be following a wrong road. If mtime is in fact to deal with race conditions, applying the same idea here is wrong because we have a different problem here.

On Thu, Jan 19, 2017 at 8:00 PM, Junio C Hamano <gitster@pobox.com> wrote:

Before the above question I already had given my opinion about what we should do.

There are the following cases:

- Freshening failed because the shared index file does not exist
anymore. In this case it could have been removed for a good reason
(for example maybe the user wants to remove all the shared index
files), or it could be a bug somewhere else. Anyway we cannot know and
the user will get an error if the shared index file that disappeared
is read from afterwards, so I don't think we need to warn or do
anything.

- Freshening failed because the mtime of the shared index cannot be
changed. You already in a previous email wrote that we shoudn't warn
if the file system is read only, and I agree with that, as anyway if
the file system is read only, the shared index file cannot be deleted,
so there is no risk from the current user. Also the split index mode
is useful to speed up index writing at the cost of making index
reading a little slower, so its use in a read only mode should not be
the primary way it is used.

So my opinion is that there are good reasons not to do anything if freshening fails.

As the current index is read, which freshens its shared index file, before a new index is created, the number of shared index files doesn't go below 2. This can be seen in the tests changed in patch 19/21. So the risk of deleting interesting shared index files is quite low in my opinion.

Also in general the split-index mode is useful when you often write new indexes, and in this case shared index files that are used will often be freshened, so the risk of deleting interesting shared index files should be low.

As I wrote above, I think if we can actually write the shared index file but its freshening fails, it probably means that the shared index file has been removed behind us, and this case is equivalent as when loose files have been removed behind us.

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

OK, so it is unlikely to happen, and when it happens it leads to a catastrophic failure---do we just ignore or do we report an error?

On Mon, Jan 23, 2017 at 7:53 PM, Junio C Hamano <gitster@pobox.com> wrote:

Well, when we cannot freshen a loose file (with freshen_loose_object()), we don't warn or die, we just write the loose file. But here we cannot write the shared index file.

And this doesn't lead to a catastrophic failure right away. There could be a catastrophic failure if the shared index file is needed again later, but we are not sure that it's going to be needed later. In fact it may have just been removed because it won't be needed later.

So I am not sure it's a good idea to anticipate a catastrophic failure that may not happen. Perhaps we could just warn, but I am not sure it will help the user. If the catastrophic failure doesn't happen because the shared index file is not needed, I can't see how the warning could help. And if there is a catastrophic failure, the following will be displayed:

fatal: .git/sharedindex.cbfe41352a4623d4d3e9757861fed53f3094e0ac: index file open failed: No such file or directory

and I don't see how the warning could help on top of that. It could at most repeat the same thing.

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

I think that is an excellent point. Let me make sure I got you right. For loose object files, we may attempt to freshen and when it fails we stay silent and do not talk about the failure. Instead, we write the same file again. That will have two potential outcomes:

 1. write fails and we fail the whole thing.  It is very clear to
    the user that there is something wrong going on.

 2. write succeeds, and because we just wrote it, we know that the
    file is fresh and is protected from gc.

So the "freshen and if fails just write" is sufficient. It is absolutely the right thing to do for loose object files.

When we are forking off a new split index file based on an old shared index file, we may attempt to "touch" the old shared one, but we cannot write the old shared one again, because other split index may be based on that, and we do not have enough information to recreate the old one [*1*]. The fallback safety is not available.

Exactly.

You are listing only the irrelevant cases. The shared one may be used immediately, and the user can keep using it for a while without "touching". Perhaps the shared one was chowned to "root" while the user is looking the other way, and its timestamp is now frozen to the time of chown. It is a ticking time-bomb that will go off when your expiry logic kicks in.

That "fatal" is given _ONLY_ after time passes and our failure to "touch" the file that is still-in-use left it subject to "gc". Of course, when "fatal" is given, it is too late to warn about ticking time bomb.

At the time we notice a ticking time bomb is the only sensible time to warn. Or better yet take a corrective action.

As I expect (and you seem to agree) that a failure to "touch" would be a very rare event (like, sysadmin chowning it to "root" by mistake), I do not mind if the "corrective action" were "immediately unsplit the index, so that at least the current and the latest index contents will be written out safely to a new single unsplit index file". That won't help _other_ split index files that are based on the same "untouchable" shared index, but I do not think that is a problem we need to solve---if they are still in use, the code that use them will notice it, and otherwise they are not in use and can be aged away safely.

[Footnote]

*1* My understanding is that we lose information on stale entries in
    the shared file that are covered by the split index overlay
    after read_index() returns, so we _might_ be able to write the
    "old" one that is sufficient for _our_ split index, but we do
    not have good enough information to recreate "old" one usable by
    other split index files.

On Wed, Jan 25, 2017 at 9:52 PM, Junio C Hamano <gitster@pobox.com> wrote:

Yeah I agree. But note that I was talking about the case where the shared index file does not exist anymore. I was just saying that when the shared index file has been removed behind us, it is equivalent as when loose files have been removed behind us, except that we cannot write the removed file to disk.

Now you are talking about a case where the shared index file can be used immediately and the user can keep using it. This is a different case than the case I was talking about (which is the case when the shared index file does not exist anymore).

In the case I was talking about, the shared index file doesn't exist anymore. So there is no time ticking bomb, because what could happen, if we cannot freshen the shared index file, is that the shared index file could be removed, which wouldn't make things worse as anyway it does not exist anymore.

So the case when the shared index file doesn't exist is different than other cases.

Now if you want to talk about the case when the shared index file has not been removed, but just chowned to "root", then I wonder how is it different from the case when the file system is read only. Perhaps the admin just chowned everything to make sure that the repo could not be changed anymore by the usual processes and users.

In a previous email you wrote that if the file system is read only, it is a bad thing if we warn. So I wonder why it would be a good thing to warn or try to do something if the admin chowned everything to prevent usual processes and users to change anything.

Yeah, I agree that a failure to "touch" would be a rare event. But the thing is we often don't know if admins or users do things by mistake or not. If they rm -rf everything forcefully, for example, they might not like it, if we start to rewrite stuff to try to "correct" things.

If the admins or users chowned a shared index file precisely because they want to save it from ever being garbage collected, I am not sure that they would be happy if we decide to "correct" things. For example they might not like it if we change the current mode to unsplit index.

So in my opinion, the safest thing, if we really want to do something, is to just warn, perhaps once, and I am ok to do that. If we really wanted to do our best, perhaps we could warn once only in the case when the shared index file still exists (because otherwise there nothing to lose anymore).

I am not sure that I understand you correctly on this. The current code does not age away the split index files. It only ages away shared index files. I think aging away the split index file is a different issue, because when we are not in split index mode, the index files are not aged anyway.

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

Yes, as I said, you are listing irrelevant and uninteresting one. If the shared index is already gone, reporting failure to touch it is of no use---an attempt to read and use the split index that depends on the shared index that is gone will fail anyway.

Yes, but you need to realize that "it is better not to bother users with a report of failure to touch in read-only repository" and "we ignore all failures".

IIUC, you attempt to touch the shared index even when you are only reading the index, because you want to mark the fact that the shared index is still being depended upon. And I tend to agree that it is OK not to report a failure for that case. It is very similar to a situation where you are asked to peek into your coworker's repository, which you do not have write access to, and run "status". The command first runs the equivalent of "update-index --refresh" only in-core, and it attempts to write the updated index because (1) it paid the cost of doing the refreshing already, and (2) if it can write into the index, it will help future operations in the repository. But it does not report a failure to write the index exactly because it is merely doing an opportunistic write.

And in the "we read from the split index, and we attempt to touch the underlying shared one to update its timestamp" case, it is OK not to report if we failed to touch.

But you also attempt to touch the shared index when you are actually writing out a new split index file based on it, no? The "you created a ticking bomb" situation is where you fail to touch the shared index for whatever reason, even when you managed to write the new split index file.

We agreed that read-only operation should not nag, so it won't trigger when you are peeking somebody else's repository to help him. As I said, it is an uninteresting and irrelevant case---when your read-only peeking did not add new information to be preserved, it is less severe problem that you fail to reset the expiration. On the other hand, if you added new information, i.e. wrote the split index based on it, it is a good indication that the <split,shared> index pair has information that is more valuable. We must warn in that case.

The difference is that your code has enough information to notice the case where you know your touch failed, you know that you wanted to write (and the write succeeded), and yet you do NOT know why your touch failed. That is the ticking bomb we need to warn the user about.

Junio C Hamano <gitster@pobox.com> writes:

Sorry about an unfinished sentence here. "need to realize that ... and ... are different things."

This reminds us of a third case. What should happen if we are doing the "opportunistic writing of the index" in "git status", managed to write the split index, but failed to touch the shared one?

In the ideal world, I think we should do the following sequence:

 - "status" tries to write cache to the file.

 - we try to write and on any error, we return error to the caller,
   who is already prepared to ignore it and stay silent.

    - as the first step of writing the index, we first try to touch
      the shared one.  If it fails, we return an error here without
      writing the split one out.

    - then we try to write the split one out.  If this fails, we
      also return an error.

    - otherwise, both touching of the shared one and writing of the
      split one are successful.  

 - "status" finishes the opportunistic refreshing of the index, by
   either ignoring an error silently (if either touching of shared
   one or writing of split one fails) or writing the refreshed index
   out successfully.

It is OK to swap the order of touching the shared one and writing of the split one in the above sequence, as long as an error in either step signals a failure to the opportunistic caller.

I do not offhand know if the split-index code is structured in such a way to allow the above sequence easily, or it needs refactoring.

If such a restructuring is required, it might not be within the scope of the series and I am OK if you just left the NEEDSWORK comment that describes the above (i.e. what we should be doing) as an in-code comment so that we can pick it up later. The whole point of the step 14/21 on the other hand is to make sure that a shared index that is still in active use will not go stale, and from that point of view, such a "punting" may not be a good idea---it deliberately finishes the series knowing that it does not adequately do what it promises to do.

So, ... I dunno.

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(-)

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

See comment on 14/21; I am not convinced that this is the function you would want to borrow.

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

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

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

Doesn't the "whatever..." clause in this sentence lack its verb (presumably, "is", right after "variable")?

It feels somewhat strange to see a warning in this case. 

If the user configures the system to usually do X, and issues a command that explicitly does Y (or "not-X"), we do not warn for the command invocation if it is a single-shot operation. That's the usual "command line overrides configuration" an end-user expects.

I think you made this deviate from the usual end-user expectation because it is unpredictable when the configuration kicks in the next time to undo the effect of this command line request. And I agree that it is a very un-nice thing to do to the users if we did not warn here, if we are to keep that unpredictableness.

But stepping back a bit, we know the user's intention is that with "--split-index" the user wants to use the split index, even though the user may have configuration not to use the split index. Don't we want to honor that intention? In order to honor that intention without getting confused by the configured variable to tell us not to split, another way is to destroy that unpredictableness. For that, shouldn't we automatically remove the configuration that says "do not split" (by perhaps set it to "do split")? Doing so still may need a warning that says "we disabled your previously configured setting while at it", but it would be much better than a warning message that essentially says "we do it once, but we will disobey you at an unpredictable time", I would think.

On Tue, Dec 27, 2016 at 8:07 PM, Junio C Hamano <gitster@pobox.com> wrote:

I think that it is ok to have no verb here. My preferred online dictionary (http://www.wordreference.com/enfr/whatever) gives "We'll try to free the hostage whatever the cost." as an example with no verb, though I am not a native speaker.

Also note that I mostly copied what I had already written for --(no-)untracked-cache:

--untracked-cache::
--no-untracked-cache::
    Enable or disable untracked cache feature. Please use
    `--test-untracked-cache` before enabling it.
+
These options take effect whatever the value of the `core.untrackedCache`
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.

We already discussed the warning issue for --(no-)untracked-cache, and I am just following the conclusions of that previous discussion about --(no-)untracked-cache. See the end of this message where you suggested the warning:

https://public-inbox.org/git/xmqqlh8cg9y2.fsf@gitster.mtv.corp.google.com/

I also think it would be strange and user unfriendly for --untracked-cache and --split-index to behave differently, and I am reluctant at this point to rework the way it works for --untracked-cache.

I wanted to do that for --untracked-cache around one year ago but in the following message:

https://public-inbox.org/git/xmqqsi3ckadi.fsf@gitster.mtv.corp.google.com/

you wrote the following:

"Why is it a good idea to allow an explicit operation from the command line to muck with the configuration? If the user wants to change the configuration permanently, "git config" has been there for the purpose for all the configuration variables to do so for almost forever. Why is it a good idea to make this variable so special that the user does not have to use "git config" but disable or enable it in some other way?"

It feels strange that when I do things one way, you suggest another way, and the next time in a similar situation when I do things the way you suggested previously, then you suggest the way I did it initially the first time...

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

Perhaps because neither is quite satisfactory and I am being forced to choose between the two unsatifactory designs? In any case, I mostly am and was pointing out the issues and not saying the other one is the most preferred solution in these threads.

I think there should just be one authoritative source of the truth, and I have always thought it should be the bit set in the index file when the command line option is used, because that was the way the feature was introduced first and I am superstitious about end-user inertia. And from that point of view, no matter how you make this new "config" thing interact with it, it would always give a strange and unsatifactory end-user experience, at least to me.

Perhaps we should declare that config will be the one and only way in the future and start deprecating the command line option way. That will remove the need for two to interact with each other.

I dunno.

On Sun, Jan 8, 2017 at 4:38 AM, Junio C Hamano <gitster@pobox.com> wrote:

Either that, or we make sure all sources of truth are consistent. In this case, 'update --split-index' could update core.splitIndex if it finds that the config tells a different story. As a plumbing though, I rather leave update-index do simple things, even if it means the user has to clean up after it (or before it) with "git config -unset core.splitIndex". Another option is refuse to execute --split-index in the presence of (conflicting) core.splitIndex. We kind of force the user to keep all sources of truth consistent this way while leaving a back door ("git -c core.splitIndex= update-index") for those who need tools to recover from a bad case.

On Mon, Jan 9, 2017 at 12:18 PM, Duy Nguyen <pclouds@gmail.com> wrote:

That would be my preferred solution as I think it is the simplest in the end for users. Also, as Duy wrote above, one can always use something like "git -c core.splitIndex= ...", which by the way can work for any command, not just "update-index".

Anyway we would have to introduce core.splitIndex first, and it wouldn't make sense to have a different behavior between --[no-]split-index and --[no-]untracked-cache in the meantime before they are deprecated and eventually removed.

So let's just go with the implementation in this series, which is similar to --[no-]untracked-cache, and let's plan to deprecate --[no-]split-index and --[no-]untracked-cache in a later patch series.

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

Sounds like we have a plan.  Thanks.

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

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

It does not add much value to follow the "existing" outdated style like this when you are only adding new tests. Write these like

        cat >ls-files.expect <<-\EOF &&
	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	three
	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	two
	EOF

which would give incentive to others (or yourself) to update the style of the existing mess ;-).

On Tue, Dec 27, 2016 at 8:04 PM, Junio C Hamano <gitster@pobox.com> wrote:

Ok, I will add a patch to update the style of the existing tests at the beginning of the series and then use the same new style in the tests I add in later patches.

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

That's not what I meant---I was expecting and was willing to accept a corrected patch that leaves existing old-fashioned ones as they are while making sure that added ones are modern, to reduce the cost of finishing this series, leaving the style fixes of existing ones for future clean-up task that can be done by anybody after the dust from this series settles.

A preparatory clean-up patch before the series like you plan is fine (eh, rather, "extra nice"), so... thanks.

On Mon, Dec 26, 2016 at 11:22 AM, Christian Couder <christian.couder@gmail.com> wrote:

Here is the diff compared to v2: