Re: [PATCH 02/11] dir: add a usage note to exclude_per_dir
- From
Elijah Newren <newren@gmail.com>
- Date
- Feb 25, 2023, 00:23 UTC
- Message-ID
- <CABPp-BG4KChJa0Kj8n84QBUt5Rc6_rX2Fus3R4MbmUuF6RNuOQ@mail.gmail.com>
- In-Reply-To
- <20230224223116.1508846-1-jonathantanmy@google.com>
On Fri, Feb 24, 2023 at 2:31 PM Jonathan Tan <jonathantanmy@google.com> wrote:
Show 24 quoted lines
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > diff --git a/dir.h b/dir.h
> > index 33fd848fc8d..2196e12630c 100644
> > --- a/dir.h
> > +++ b/dir.h
> > @@ -295,8 +295,12 @@ struct dir_struct {
> > struct untracked_cache *untracked;
> >
> > /**
> > - * The name of the file to be read in each directory for excluded files
> > - * (typically `.gitignore`).
> > + * Deprecated: ls-files is the only allowed caller; all other callers
> > + * should leave this as NULL; it pre-dated the
> > + * setup_standard_excludes() mechanism that replaces this.
> > + *
> > + * This field tracks the name of the file to be read in each directory
> > + * for excluded files (typically `.gitignore`).
> > */
> > const char *exclude_per_dir;
>
> I'm not sure what is meant by "allowed caller", but I wouldn't have
> expected this to also mean that unpack-trees would need to know to
> propagate this from o->internal.dir to d in verify_clean_subdirectory.Are you confusing fields that are internal to dir, with fields that are internal to unpack-trees?
This series does not make exclude_per_dir an internal field within dir_struct.
Later in this series, we do make the embedded dir_struct within unpack_trees_options as internal. Thus every access of any field of dir within unpack_trees will have an "internal." prefix, but that has nothing to do with this patch and would still be true even if this patch were dropped.
> I would be OK with excluding this from the patch set - I think the other > changes can stand independent of this.
Trying to get a consistent look and feel between commands is important. The "setup_standard_excludes()" is one small area where we've achieved that, and it's shared by _almost_ all commands (builtin/ls-files being the only exception and it persists for backward compatibility reasons). So I definitely want to keep the deprecation notice and warn people away from using this field. That's what this patch is for.