Elijah Newren <newren@xxxxxxxxx> writes: > On Tue, Oct 5, 2021 at 6:21 AM Victoria Dye via GitGitGadget > <gitgitgadget@xxxxxxxxx> wrote: >> >> From: Victoria Dye <vdye@xxxxxxxxxx> >> >> `reset --soft` does not modify the index, so no compatibility changes are >> needed for it to function without expanding the index. For all other reset >> modes (`--mixed`, `--hard`, `--keep`, `--merge`), the full index is >> explicitly expanded with `ensure_full_index` to maintain current behavior. > > "to maintain current behavior"? You are changing code here, which > suggests some kind of behavior is changing, but that description seems > to be claiming the opposite. Is it some kind of preventative change > to add ensure_full_index calls in an additional place, with a later > patch in the series intending to remove the other one(s), so you're > making sure that later changes won't cause unwanted behavioral > changes? Or was something else meant here? > > If the above wasn't what you meant, but you're adding > ensure_full_index calls, does that suggest that we had some important > code paths that were not protected by such calls? The original called read_cache() before we know which mode we operate in, near the end of parse_args(), which resulted in an unconditional call to ensure_full_index() in repo_read_index(). This patch delays the call to read_cache(). If parse_pathspec() and everything the original called after the point where it called read_cache() needed to have a populated in-core index, the change can break things---I didn't check thoroughly, but I am guessing it is OK. >> Additionally, the `read_cache()` check verifying an uncorrupted index is >> moved after argument parsing and preparing the repo settings. The index is >> not used by the preceding argument handling, but `read_cache()` does need to >> be run after enabling sparse index for the command and before resetting. > > This seems to be discussing what code changes are being made, but not > why. I'm guessing at the reasoning, but is it something along the > lines of: > > """ > Also, make sure to read_cache() after setting > command_requires_full_index = 0, so that we don't unnecessarily expand > the index as part of our early index-corruption check. > """ I think it is more like "we used to expand very early for all modes, but with this change we move the read_cache() call to much later, and force it not to expand. The modes that call read_from_tree() needs in-core index fully expanded, so we do so there, but the soft reset does not call it and would stop expanding."
