On Tue, Sep 3, 2019 at 11:04 AM Derrick Stolee via GitGitGadget <gitgitgadget@xxxxxxxxx> wrote: > > The exclude library defined in dir.h was originally written for the > .gitignore feature, but has since been used for .gitattributes and > sparse-checkout. In the later applications, these patterns are used for > inclusion rather than exclusion, so the name is confusing. This gets > particularly bad when looking at how the sparse-checkout feature uses > is_excluded_from_list() to really mean "should be included in the working > directory". > > This series performs several renames of structs and methods to generalize > the exclude library to be a "pattern matching" library. Instead of a list of > excludes, we have a list of path patterns. It is up to the consumer to > decide what to do with a match. The .gitignore logic will still treat the > patterns as a list of exclusions, the sparse-checkout logic treats the > patterns as a list of inclusions. Wahoo! Thanks for going through and cleaning these up. The possibility of negated patterns (or negated logic or both) on top of using "exclude" when "include" was _sometimes_ meant was just too much for me to keep track of. This series will make it much easier. > For this reason, some methods and structs in dir.h retain "exclude" in their > name. These are limited to things consumed only by the .gitignore feature > (as far as I can tell). > > Most of these changes are mechanical find-and-replaces, with the exception > of some variable names and the last patch. I looked through the first four patches with --color-words=. to spot check them; as you say, looks like straightforward mechanical changes. I did notice that the first two paragraphs were shared between the commit messages; it does make sense to help provide the context, but it seemed a little duplicative. I'm not sure what to suggest; it may be fine as it is. Just thought I'd flag it. > The last patch, "unpack-trees: rename 'is_excluded_from_list()'", performs a > more meaningful refactor. The method is_excluded_from_list() was only used > by sparse-checkout (inside the clear_ce_flags() methods) to see if a path > should be included in the working directory. The return value of "1 for > excluded" was confusing. Instead, use a new enum value. This required > changing several method prototypes inside unpack-trees.c. > > This refactor was inspired by Elijah Newren's feedback [1] on my > sparse-checkout builtin RFC. I am working on a few other orthogonal changes > to make to the existing sparse-checkout behavior before I resubmit that RFC. > > I had started working on v2.23.0, but found adjacent-diff conflicts with > md/list-objects-filter-combo [2] and js/partial-clone-sparse-blob [3]. Those > branches are independent, but the conflicts with > md/list-objects-filter-combo were more severe (and that branch seems closer > to merging) so this is based on md/list-object-filter-combo. Hopefully the > conflicts with js/partial-clone-sparse-blob are clear enough to resolve > easily. I posted some comments on patch 5; otherwise, the series looks good to me.
