On Thu, Dec 24, 2015 at 2:56 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Junio C Hamano <gitster@xxxxxxxxx> writes: > >> Duy Nguyen <pclouds@xxxxxxxxx> writes: >> >>> In that case we can just check config once in read_index_from and >>> destroy UNTR extension. Or the middle ground, we check config once in >>> that place, make a note in struct index_state, and make invalidate_* >>> check that note instead of config file. The middle ground has an >>> advantage over destroying UNTR: (probably) many operations will touch >>> index but do not add or remove entries. >> >> Or we can even teach read_index_from() to skip reading the extension >> without even parsing when the configuration tells it that the >> feature is force-disabled. It can also add an empty one when the >> configuration tells it that the feature is force-enabled and there >> is no UNTR extension yet. > > Thinking about this a bit more, I am starting to feel that the > config that can be used to optionally override the presence of > in-index UNTR extension does not have to be too bad a thing, > provided if it is done with a few tweaks to the design I read in > Christian & Ævar's messages. Great! > One tweak is to address the following from Ævar's message: > >>> Once this series is applied and git is shipped with it existing >>> users that have set "git update-index --untracked-cache" will have >>> their UC feature disabled. This is because we fall back to "oh no >>> config here? Must have been disabled, rm it from the index" clause >>> which keeps our UC from going stale in the face of config >>> flip-flopping. > > The above would happen only if the configuration is a boolean that > defaults to false. I'd think we can have this a tristate instead. > That is, config.untrackedCache can be explicitly set to 'true', > 'false', or 'keep'. And make a missing config.untrackedCache > default to 'keep'. Ok. The first RFC patch series about this had a tristate and I switched to a boolean as it seemed that people prefered a boolean, but you are right that a tristate is more backward compatible. > When read_index_from() reads an existing index: > > When the configuration is set to 'true': > an existing UNTR is kept, otherwise a new UNTR gets added. > When the configuration is set to 'false: > an existing UNTR is dropped. > When the configuration is set to 'keep': > an existing UNTR is kept, otherwise nothing happens. > > When write_index() writes out an index that wasn't initialized from > the filesystem, a new UNTR gets added only when the configuration is > set to 'true' and there is no in-core UNTR already. My current patch series does these changes in wt_status_collect_untracked() because currently the UNTR is mostly used only in git status, so it feels safer to me to not affect other code paths. > That way, those who use /etc/gitconfig to force the feature over > their users would be able to set it to 'true', those who have been > using the feature in some but not all of their repositories won't > see any different behaviour until they muck with the configuration > (and if they are paranoid and want to opt out of their administrator's > setting, they can set it to 'keep' in their $HOME/.gitconfig to make > sure their repositories are not affected). > > Orthogonal to the "config overrides the existing users' practice" > issue, I still think that [PATCH v2 10/10] goes too far to remove > the safety based on the working tree location. Checking kernel > version and other thing may be too much, but the check based on the > working tree location is a cheap way to make sure that those who do > unusual things (namely, use $GIT_DIR/$GIT_WORK_TREE or their > equivalents to tell Git that the working tree for this invocation is > at a place different from what UNTR data was prepared for) are not > harmed by incorrectly reusing the cached data for an unrelated > location. So another tweak I'd feel better to see is to resurrect > that safety. This has been changed in v3, see patch 09/11, and I think it should now work as you suggest. > I wouldn't have as much issue with such a scheme as I had with the > earlier description of the design in the Christian's series. Great! I am preparing a v4 that I hope to send in a few days. By the way the v3 does not pass its own tests due to a bug introduced in last minute changes. Thanks, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html