On 8/19/2021 4:11 AM, Johannes Schindelin wrote: > On Tue, 17 Aug 2021, Derrick Stolee via GitGitGadget wrote: >> + /* >> + * In the case of cone-mode sparse-checkout, getting the >> + * .gitattributes file from a directory is meaningless: all >> + * contained paths will be sparse if the .gitattributes is also >> + * sparse. In the case of a sparse index, it is critical that we >> + * don't go looking for one as it will expand the index. >> + */ >> + init_sparse_checkout_patterns(istate); > At first I thought that `init_sparse_checkout_patterns()` is called by > `path_in_sparse_checkout()` below, and therefore would not be necessary. > > But it is! Without it, we have no way to test whether `use_cone_patterns` > is set, because, well, it gets set by `init_sparse_checkout_patterns()`. > > Would it therefore make sense to refactor the code to have a > `path_in_sparse_checkout_cone()` function? Or add a flag > `only_in_cone_mode` as function parameter to `path_in_sparse_checkout()`? One way to clean this up as-is would be to replace >> + if (istate->sparse_checkout_patterns && >> + istate->sparse_checkout_patterns->use_cone_patterns && >> + path_in_sparse_checkout(path, istate) == NOT_MATCHED) with if (!path_in_sparse_checkout(path, istate) && istate->sparse_checkout_patterns->use_cone_patterns) to get a similar effect. However, it creates wasted pattern-match checks when not in cone-mode, so you are probably right that a path_in_cone_mode_sparse_checkout() would be best to short-circuit in the non-cone-mode case. This special casing should be rare, so I don't think it is worth adding a flag to path_in_sparse_checkout() and making those call sites more complicated. Thanks, -Stolee
