Re: [PATCH 0/5] Refactor excludes library

From: Elijah Newren <newren@gmail.com>
Date: Sep 4, 2019, 20:28 UTC
Message-ID: <CABPp-BH2LoP=f0ZD1uegP3-CKTgZ9MExamyYQw8YLkdmtTVpkg@mail.gmail.com>
In-Reply-To: <pull.329.git.gitgitgadget@gmail.com>

On Tue, Sep 3, 2019 at 11:04 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 15 quoted lines

>
> 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.

Show 6 quoted lines

> 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.

Show 18 quoted lines

> 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.