On 10/17/2021 9:15 PM, Junio C Hamano wrote: > Derrick Stolee <stolee@xxxxxxxxx> writes: > >>> * In addition, with these patches, if index.sparse=false, a >>> sparse index will be expaned to full upon reading, and if it >>> is true, a non-sparse index will be shrunk to sparse upon >>> reading >> >> This is only half true. If "index.sparse=false", then a sparse >> index will be expanded to full upon reading. If "index.sparse=true" >> then nothing special will happen to an index on reading. > > OK. I somehow got the impression that we convert in both ways from > the patch text under discussion, specifically this part in > do_read_index(): > >> - if (istate->repo->settings.command_requires_full_index) >> + if (!istate->repo->settings.sparse_index || >> + istate->repo->settings.command_requires_full_index) >> ensure_full_index(istate); >> + else if (!istate->sparse_index) >> + convert_to_sparse(istate, 0); >> >> return istate->cache_nr; > > We used to say "when we know we are running a command that is not > sparse ready, expand it here" and nothing else. > > We now added a bit more condition for expansion, i.e. "when we are > told that the repository does not want sparse index, or when the > command is not sparse ready". > > But the patch goes one more step. "If we didn't find a reason to > expand to full, and if the in-core index we read is not sparse, then > call convert_to_sparse() on it". So if the repo settings tells us > that the repository wants a sparse index, and the index we read was > not sparse, what does convert_to_sparse() without MEM_ONLY flag bit > do? Nothing special? You are absolutely right. I've been talking about what I _thought_ the code does (and should do) but I missed this 'else if' which is in fact doing what you have been claiming the entire time. I should have done a better job double-checking the code before doubling down on my claims. I think the 'else if' should be removed, which would match my expectations. > I see many unconditional calls to convert_to_sparse(istate, 0) on > the write code paths, where I instead would expect "if the repo > wants sparse, call convert_to_sparse(), and if the repo does not > want sparse, call ensure_full_index()", before actualy writing the > entries out. These also are setting traps to confuse readers. > > Perhaps we want a helper function "ensure_right_sparsity(istate)" > that would be called where we have > > if (condition) > convert_to_sparse(); > else > ensure_full_index(); > > or an unconditonal call to convert_to_sparse() to unconfuse readers? convert_to_sparse() has several checks that prevent the conversion from happening, such as having a split index. In particular, it will skip the conversion if the_repository->settings.sparse_index is false. Thus, calling it unconditionally in the write code is correct. Doing these conditional checks within convert_to_sparse() make sense to avoid repeating the conditionals in all callers. ensure_full_index() doesn't do the same because we absolutely want a full index at the end of that method. Perhaps a rename to something like "convert_to_sparse_if_able()" would make sense? But it might be best to leave that one lie. Thanks, -Stolee
