[v2,12/22] tree-diff.c: move S_DIFFTREE_IFXMIN_NEQ define from cache.h

Commit Message

Elijah Newren April 22, 2023, 8:17 p.m. UTC

From: Elijah Newren <[e-mail]>

S_DIFFTREE_IFXMIN_NEQ is *only* used in tree-diff.c, so there is no
point exposing it in cache.h.  Move it to tree-diff.c.

Signed-off-by: Elijah Newren <[e-mail]>
---
 cache.h     | 15 ---------------
 tree-diff.c | 13 +++++++++++++
 2 files changed, 13 insertions(+), 15 deletions(-)

Comments

Ævar Arnfjörð Bjarmason May 1, 2023, 4:33 p.m. UTC | #1

On Sat, Apr 22 2023, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <[e-mail]>
>
> S_DIFFTREE_IFXMIN_NEQ is *only* used in tree-diff.c, so there is no
> point exposing it in cache.h.  Move it to tree-diff.c.
>
> Signed-off-by: Elijah Newren <[e-mail]>
> ---
>  cache.h     | 15 ---------------
>  tree-diff.c | 13 +++++++++++++
>  2 files changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index ad741e70bc2..7a46f300d9b 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -10,21 +10,6 @@
>  #include "object.h"
>  #include "statinfo.h"
>  
> -/*
> - * Some mode bits are also used internally for computations.
> - *
> - * They *must* not overlap with any valid modes, and they *must* not be emitted
> - * to outside world - i.e. appear on disk or network. In other words, it's just
> - * temporary fields, which we internally use, but they have to stay in-house.
> - *
> - * ( such approach is valid, as standard S_IF* fits into 16 bits, and in Git
> - *   codebase mode is `unsigned int` which is assumed to be at least 32 bits )
> - */
> -
> -/* used internally in tree-diff */
> -#define S_DIFFTREE_IFXMIN_NEQ	0x80000000
> -
> -
>  /*
>   * Basic data structures for the directory cache
>   */
> diff --git a/tree-diff.c b/tree-diff.c
> index 69031d7cbae..a76e6dae619 100644
> --- a/tree-diff.c
> +++ b/tree-diff.c
> @@ -6,6 +6,19 @@
>  #include "diffcore.h"
>  #include "tree.h"
>  
> +/*
> + * Some mode bits are also used internally for computations.
> + *
> + * They *must* not overlap with any valid modes, and they *must* not be emitted
> + * to outside world - i.e. appear on disk or network. In other words, it's just
> + * temporary fields, which we internally use, but they have to stay in-house.
> + *
> + * ( such approach is valid, as standard S_IF* fits into 16 bits, and in Git
> + *   codebase mode is `unsigned int` which is assumed to be at least 32 bits )
> + */
> +
> +#define S_DIFFTREE_IFXMIN_NEQ	0x80000000
> +
>  /*
>   * internal mode marker, saying a tree entry != entry of tp[imin]
>   * (see ll_diff_tree_paths for what it means there)

As it's only used in tree-diff.c, should this change not be instead
changing how we define S_IFXMIN_NEQ itself, and combining the two
comments seen here (the latter only partially, in the context).

Not that this makes things worse or anything...

Junio C Hamano May 1, 2023, 4:46 p.m. UTC | #2

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

>> +#define S_DIFFTREE_IFXMIN_NEQ	0x80000000
>> +
>>  /*
>>   * internal mode marker, saying a tree entry != entry of tp[imin]
>>   * (see ll_diff_tree_paths for what it means there)
>
> As it's only used in tree-diff.c, should this change not be instead
> changing how we define S_IFXMIN_NEQ itself, and combining the two
> comments seen here (the latter only partially, in the context).

Yup, the end result should look cleaner with this extra bit of
tweak.  Thanks for carefully reading the patch.

Elijah Newren May 2, 2023, 1:06 a.m. UTC | #3

On Mon, May 1, 2023 at 9:35 AM Ævar Arnfjörð Bjarmason <[e-mail]> wrote:
>
> On Sat, Apr 22 2023, Elijah Newren via GitGitGadget wrote:
[...]
> > --- a/tree-diff.c
> > +++ b/tree-diff.c
> > @@ -6,6 +6,19 @@
> >  #include "diffcore.h"
> >  #include "tree.h"
> >
> > +/*
> > + * Some mode bits are also used internally for computations.
> > + *
> > + * They *must* not overlap with any valid modes, and they *must* not be emitted
> > + * to outside world - i.e. appear on disk or network. In other words, it's just
> > + * temporary fields, which we internally use, but they have to stay in-house.
> > + *
> > + * ( such approach is valid, as standard S_IF* fits into 16 bits, and in Git
> > + *   codebase mode is `unsigned int` which is assumed to be at least 32 bits )
> > + */
> > +
> > +#define S_DIFFTREE_IFXMIN_NEQ        0x80000000
> > +
> >  /*
> >   * internal mode marker, saying a tree entry != entry of tp[imin]
> >   * (see ll_diff_tree_paths for what it means there)
>
> As it's only used in tree-diff.c, should this change not be instead
> changing how we define S_IFXMIN_NEQ itself, and combining the two
> comments seen here (the latter only partially, in the context).
>
> Not that this makes things worse or anything...

Hmm, that makes sense; I'll make the tweak.  Thanks for the suggestion.

Elijah Newren May 2, 2023, 5 a.m. UTC | #4

On Mon, May 1, 2023 at 6:06 PM Elijah Newren <[e-mail]> wrote:
>
> On Mon, May 1, 2023 at 9:35 AM Ævar Arnfjörð Bjarmason <[e-mail]> wrote:
> >
> > On Sat, Apr 22 2023, Elijah Newren via GitGitGadget wrote:
> [...]
> > > --- a/tree-diff.c
> > > +++ b/tree-diff.c
> > > @@ -6,6 +6,19 @@
> > >  #include "diffcore.h"
> > >  #include "tree.h"
> > >
> > > +/*
> > > + * Some mode bits are also used internally for computations.
> > > + *
> > > + * They *must* not overlap with any valid modes, and they *must* not be emitted
> > > + * to outside world - i.e. appear on disk or network. In other words, it's just
> > > + * temporary fields, which we internally use, but they have to stay in-house.
> > > + *
> > > + * ( such approach is valid, as standard S_IF* fits into 16 bits, and in Git
> > > + *   codebase mode is `unsigned int` which is assumed to be at least 32 bits )
> > > + */
> > > +
> > > +#define S_DIFFTREE_IFXMIN_NEQ        0x80000000
> > > +
> > >  /*
> > >   * internal mode marker, saying a tree entry != entry of tp[imin]
> > >   * (see ll_diff_tree_paths for what it means there)
> >
> > As it's only used in tree-diff.c, should this change not be instead
> > changing how we define S_IFXMIN_NEQ itself, and combining the two
> > comments seen here (the latter only partially, in the context).
> >
> > Not that this makes things worse or anything...
>
> Hmm, that makes sense; I'll make the tweak.  Thanks for the suggestion.

Although maybe I'll have to do it in a follow-on series?  Junio merged
the series to next today, so...I guess I'll just add it to my "header
cleanups" notes.

Junio C Hamano May 2, 2023, 3:56 p.m. UTC | #5

Elijah Newren <[e-mail]> writes:

> Although maybe I'll have to do it in a follow-on series?  Junio merged
> the series to next today, so...I guess I'll just add it to my "header
> cleanups" notes.

I am not Ævar, but it is so small a thing that is local to a single
file, we can wait until the dust settles to avoid distraction.

Elijah Newren May 2, 2023, 3:59 p.m. UTC | #6

On Tue, May 2, 2023 at 8:56 AM Junio C Hamano <[e-mail]> wrote:
>
> Elijah Newren <[e-mail]> writes:
>
> > Although maybe I'll have to do it in a follow-on series?  Junio merged
> > the series to next today, so...I guess I'll just add it to my "header
> > cleanups" notes.
>
> I am not Ævar, but it is so small a thing that is local to a single
> file, we can wait until the dust settles to avoid distraction.

Yup, sounds good to me.  :-)

I've got it on my todo list for continued cleanups.

Patch

diff --git a/cache.h b/cache.h
index ad741e70bc2..7a46f300d9b 100644
--- a/cache.h
+++ b/cache.h
@@ -10,21 +10,6 @@ 
 #include "object.h"
 #include "statinfo.h"
 
-/*
- * Some mode bits are also used internally for computations.
- *
- * They *must* not overlap with any valid modes, and they *must* not be emitted
- * to outside world - i.e. appear on disk or network. In other words, it's just
- * temporary fields, which we internally use, but they have to stay in-house.
- *
- * ( such approach is valid, as standard S_IF* fits into 16 bits, and in Git
- *   codebase mode is `unsigned int` which is assumed to be at least 32 bits )
- */
-
-/* used internally in tree-diff */
-#define S_DIFFTREE_IFXMIN_NEQ	0x80000000
-
-
 /*
  * Basic data structures for the directory cache
  */
diff --git a/tree-diff.c b/tree-diff.c
index 69031d7cbae..a76e6dae619 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -6,6 +6,19 @@ 
 #include "diffcore.h"
 #include "tree.h"
 
+/*
+ * Some mode bits are also used internally for computations.
+ *
+ * They *must* not overlap with any valid modes, and they *must* not be emitted
+ * to outside world - i.e. appear on disk or network. In other words, it's just
+ * temporary fields, which we internally use, but they have to stay in-house.
+ *
+ * ( such approach is valid, as standard S_IF* fits into 16 bits, and in Git
+ *   codebase mode is `unsigned int` which is assumed to be at least 32 bits )
+ */
+
+#define S_DIFFTREE_IFXMIN_NEQ	0x80000000
+
 /*
  * internal mode marker, saying a tree entry != entry of tp[imin]
  * (see ll_diff_tree_paths for what it means there)