[02/11] dir: add a usage note to exclude_per_dir

Commit Message

Elijah Newren Feb. 23, 2023, 9:14 a.m. UTC

From: Elijah Newren <[e-mail]>

Signed-off-by: Elijah Newren <[e-mail]>
---
 dir.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Jonathan Tan Feb. 24, 2023, 10:31 p.m. UTC | #1

"Elijah Newren via GitGitGadget" <[e-mail]> 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.

I would be OK with excluding this from the patch set - I think the other
changes can stand independent of this.

Elijah Newren Feb. 25, 2023, 12:23 a.m. UTC | #2

On Fri, Feb 24, 2023 at 2:31 PM Jonathan Tan <[e-mail]> wrote:
>
> "Elijah Newren via GitGitGadget" <[e-mail]> 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.

Jonathan Tan Feb. 25, 2023, 1:54 a.m. UTC | #3

Elijah Newren <[e-mail]> writes:
> On Fri, Feb 24, 2023 at 2:31 PM Jonathan Tan <[e-mail]> wrote:
> >
> > "Elijah Newren via GitGitGadget" <[e-mail]> 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.

Agreed, but the comment says that ls-files is the only allowed caller,
and I would have expected that non-"allowed callers" would not need to
write to exclude_per_dir. But in unpack-trees.c:

  2346          if (o->internal.dir)                                                                                                       
  2347                  d.exclude_per_dir = o->internal.dir->exclude_per_dir;

Both "d" and "o->internal.dir" are of type "struct dir_struct" (well,
one is not a pointer and one is). I would not have expected such non-
ls-files code to read or write from this field. (But if unpack-trees
is considered part of ls-files and/or copying the same field to another
struct is not considered "calling", then this patch is fine, I guess.)

Elijah Newren Feb. 25, 2023, 3:23 a.m. UTC | #4

On Fri, Feb 24, 2023 at 5:54 PM Jonathan Tan <[e-mail]> wrote:
>
> Elijah Newren <[e-mail]> writes:
> > On Fri, Feb 24, 2023 at 2:31 PM Jonathan Tan <[e-mail]> wrote:
> > >
> > > "Elijah Newren via GitGitGadget" <[e-mail]> 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.
>
> Agreed, but the comment says that ls-files is the only allowed caller,
> and I would have expected that non-"allowed callers" would not need to
> write to exclude_per_dir. But in unpack-trees.c:
>
>   2346          if (o->internal.dir)
>   2347                  d.exclude_per_dir = o->internal.dir->exclude_per_dir;
>
> Both "d" and "o->internal.dir" are of type "struct dir_struct" (well,
> one is not a pointer and one is). I would not have expected such non-
> ls-files code to read or write from this field. (But if unpack-trees
> is considered part of ls-files and/or copying the same field to another
> struct is not considered "calling", then this patch is fine, I guess.)

Ah, gotcha, sorry for misunderstanding earlier.  And you are right;
the code in unpack_trees() is buggy and should not be using
exclude_per_dir; the fact that it is using it directly actually hints
that it has bugs when exclusions are specified in .git/info/exclude
instead of .gitignore.  I've found a testcase that demonstrates this;
I'll update the series with a new patch fixing it.

Patch

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;