Re: [PATCH 3/3] diff: convert flags to be stored in bitfields

[Junio C Hamano]

Junio C Hamano  writes:

> Junio C Hamano  writes:
>
>> I still haven't brought myself to like the structure being passed by
>> value and the singleton diff_flags_cleared thing, but I suspect that
>> we may get used to them once we start using these.  I dunno.
>
> Just bikeshedding, but I just had to prepare an evil merge to add a
> new use of diff_flags_cleared to a codepath that evolved in a topic
> still in flight, and realized that I really hate the name.  Perhaps
> I wouldn't have hated it so much if it were named diff_flags_none or
> diff_flags_empty, I guess.

As to the "passing by value" thing, as long as we have the _or()
helper function, no sensible person will add anything but a bitfield
into the structure, so I am fine with it.

Re: [PATCH 3/3] diff: convert flags to be stored in bitfields

[Brandon Williams]

On 10/30, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
> > I still haven't brought myself to like the structure being passed by
> > value and the singleton diff_flags_cleared thing, but I suspect that
> > we may get used to them once we start using these.  I dunno.
> 
> Just bikeshedding, but I just had to prepare an evil merge to add a
> new use of diff_flags_cleared to a codepath that evolved in a topic
> still in flight, and realized that I really hate the name.  Perhaps
> I wouldn't have hated it so much if it were named diff_flags_none or
> diff_flags_empty, I guess.

I have a new version of the series I'll send out and i ended up dropping
it entirely.  Didn't even need a clear function because I was able to
drop the touched stuff and it would have only been used inside of
builtin/log.c to clear the touched flags.

-- 
Brandon Williams

Re: [PATCH 3/3] diff: convert flags to be stored in bitfields

[Brandon Williams]

On 10/29, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > We have have reached the limit of the number of flags that can be stored
> 
> s/have have/have/; but bit #9 is unused.  
> 
> "We cannot add many more flags even if we wanted to" would be more
> flexible and does not take this change hostage to whatever topic
> that tries to claim that bit, I would think.

I'll tweak the wording a bit.

> 
> > in a single unsigned int.  In order to allow for more flags to be added
> > to the diff machinery in the future this patch converts the flags to be
> > stored in bitfields in 'struct diff_flags'.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  builtin/commit.c |  7 +++--
> >  builtin/log.c|  2 +-
> >  diff-lib.c   |  6 ++--
> >  diff.c   |  3 +-
> >  diff.h   | 96 
> > +---
> >  sequencer.c  |  5 +--
> >  6 files changed, 70 insertions(+), 49 deletions(-)
> >
> 
> > diff --git a/diff.h b/diff.h
> > index aca150ba2..d58f06106 100644
> > --- a/diff.h
> > +++ b/diff.h
> > @@ -60,42 +60,59 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct 
> > diff_options *opt, void *data)
> >  
> >  #define DIFF_FORMAT_CALLBACK   0x1000
> >  
> > -#define DIFF_OPT_RECURSIVE   (1 <<  0)
> > -#define DIFF_OPT_TREE_IN_RECURSIVE   (1 <<  1)
> > -#define DIFF_OPT_BINARY  (1 <<  2)
> > -...
> > -#define DIFF_OPT_FUNCCONTEXT (1 << 29)
> > -#define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)
> > -#define DIFF_OPT_DEFAULT_FOLLOW_RENAMES (1U << 31)
> > -
> > -#define DIFF_OPT_TST(opts, flag)((opts)->flags & DIFF_OPT_##flag)
> > -#define DIFF_OPT_TOUCHED(opts, flag)((opts)->touched_flags & 
> > DIFF_OPT_##flag)
> > -#define DIFF_OPT_SET(opts, flag)(((opts)->flags |= 
> > DIFF_OPT_##flag),((opts)->touched_flags |= DIFF_OPT_##flag))
> > -#define DIFF_OPT_CLR(opts, flag)(((opts)->flags &= 
> > ~DIFF_OPT_##flag),((opts)->touched_flags |= DIFF_OPT_##flag))
> > +#define DIFF_FLAGS_INIT { 0 }
> > +extern const struct diff_flags diff_flags_cleared;
> 
> This thing is curious.  Seeing the scary diff_flags_or(), I would
> have expected we'd have diff_flags_clear().

This seemed to make things easier in practice because I was passing some
structs by value, let me work with it a bit to see what it looks like
when they are passed by reference instead.

> 
> > +struct diff_flags {
> > +   unsigned RECURSIVE:1;
> > +   unsigned TREE_IN_RECURSIVE:1;
> > +   unsigned BINARY:1;
> > +...
> > +   unsigned FUNCCONTEXT:1;
> > +   unsigned PICKAXE_IGNORE_CASE:1;
> > +   unsigned DEFAULT_FOLLOW_RENAMES:1;
> > +};
> > +
> > +static inline struct diff_flags diff_flags_or(struct diff_flags a,
> > + struct diff_flags b)
> > +{
> > +   char *tmp_a = (char *)
> > +   char *tmp_b = (char *)
> > +   int i;
> > +
> > +   for (i = 0; i < sizeof(struct diff_flags); i++)
> > +   tmp_a[i] |= tmp_b[i];
> > +
> > +   return a;
> > +}
> 
> This is doubly scary, but let's see why we need it by looking at the
> callers.
> 
> > +#define DIFF_OPT_TST(opts, flag)   ((opts)->flags.flag)
> > +#define DIFF_OPT_TOUCHED(opts, flag)   ((opts)->touched_flags.flag)
> > +#define DIFF_OPT_SET(opts, flag)   (((opts)->flags.flag = 
> > 1),((opts)->touched_flags.flag = 1))
> > +#define DIFF_OPT_CLR(opts, flag)   (((opts)->flags.flag = 
> > 0),((opts)->touched_flags.flag = 1))
> 
> These are trivial and straight-forward.
> 
> > +
> >  #define DIFF_XDL_TST(opts, flag)((opts)->xdl_opts & XDF_##flag)
> >  #define DIFF_XDL_SET(opts, flag)((opts)->xdl_opts |= XDF_##flag)
> >  #define DIFF_XDL_CLR(opts, flag)((opts)->xdl_opts &= ~XDF_##flag)
> > @@ -122,8 +139,8 @@ struct diff_options {
> > const char *a_prefix, *b_prefix;
> > const char *line_prefix;
> > size_t line_prefix_length;
> > -   unsigned flags;
> > -   unsigned touched_flags;
> > +   struct diff_flags flags;
> > +   struct diff_flags touched_flags;
> >  
> > /* diff-filter bits */
> > unsigned int filter;
> > @@ -388,7 +405,8 @@ extern int diff_result_code(struct diff_options *, int);
> >  
> >  extern void diff_no_index(struct rev_info *, int, const char **);
> >  
> > -extern int index_differs_from(const char *def, int diff_flags, int 
> > ita_invisible_in_index);
> > +extern int index_differs_from(const char *def, struct diff_flags flags,
> > + int ita_invisible_in_index);
> 
> OK.  I tend to think twice before passing any struct by value (even
> something that starts its life as a small/single-word struct), but
> let's see how much simpler this allows callers to become.
> 
> > diff --git a/builtin/commit.c b/builtin/commit.c
> > index d75b3805e..de08c2594 100644
> > --- a/builtin/commit.c
> > +++ b/builtin/commit.c
> > @@ -912,11 +912,12 @@ static int prepare_to_commit(const char *index_file, 
> > const char *prefix,
> >  * submodules 

Re: [PATCH 3/3] diff: convert flags to be stored in bitfields

[Junio C Hamano]

Junio C Hamano  writes:

> I still haven't brought myself to like the structure being passed by
> value and the singleton diff_flags_cleared thing, but I suspect that
> we may get used to them once we start using these.  I dunno.

Just bikeshedding, but I just had to prepare an evil merge to add a
new use of diff_flags_cleared to a codepath that evolved in a topic
still in flight, and realized that I really hate the name.  Perhaps
I wouldn't have hated it so much if it were named diff_flags_none or
diff_flags_empty, I guess.

Re: [PATCH 3/3] diff: convert flags to be stored in bitfields

[Junio C Hamano]

Brandon Williams  writes:

> We have have reached the limit of the number of flags that can be stored

s/have have/have/; but bit #9 is unused.  

"We cannot add many more flags even if we wanted to" would be more
flexible and does not take this change hostage to whatever topic
that tries to claim that bit, I would think.

> in a single unsigned int.  In order to allow for more flags to be added
> to the diff machinery in the future this patch converts the flags to be
> stored in bitfields in 'struct diff_flags'.
>
> Signed-off-by: Brandon Williams 
> ---
>  builtin/commit.c |  7 +++--
>  builtin/log.c|  2 +-
>  diff-lib.c   |  6 ++--
>  diff.c   |  3 +-
>  diff.h   | 96 
> +---
>  sequencer.c  |  5 +--
>  6 files changed, 70 insertions(+), 49 deletions(-)
>

> diff --git a/diff.h b/diff.h
> index aca150ba2..d58f06106 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -60,42 +60,59 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct 
> diff_options *opt, void *data)
>  
>  #define DIFF_FORMAT_CALLBACK 0x1000
>  
> -#define DIFF_OPT_RECURSIVE   (1 <<  0)
> -#define DIFF_OPT_TREE_IN_RECURSIVE   (1 <<  1)
> -#define DIFF_OPT_BINARY  (1 <<  2)
> -...
> -#define DIFF_OPT_FUNCCONTEXT (1 << 29)
> -#define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)
> -#define DIFF_OPT_DEFAULT_FOLLOW_RENAMES (1U << 31)
> -
> -#define DIFF_OPT_TST(opts, flag)((opts)->flags & DIFF_OPT_##flag)
> -#define DIFF_OPT_TOUCHED(opts, flag)((opts)->touched_flags & 
> DIFF_OPT_##flag)
> -#define DIFF_OPT_SET(opts, flag)(((opts)->flags |= 
> DIFF_OPT_##flag),((opts)->touched_flags |= DIFF_OPT_##flag))
> -#define DIFF_OPT_CLR(opts, flag)(((opts)->flags &= 
> ~DIFF_OPT_##flag),((opts)->touched_flags |= DIFF_OPT_##flag))
> +#define DIFF_FLAGS_INIT { 0 }
> +extern const struct diff_flags diff_flags_cleared;

This thing is curious.  Seeing the scary diff_flags_or(), I would
have expected we'd have diff_flags_clear().

> +struct diff_flags {
> + unsigned RECURSIVE:1;
> + unsigned TREE_IN_RECURSIVE:1;
> + unsigned BINARY:1;
> +...
> + unsigned FUNCCONTEXT:1;
> + unsigned PICKAXE_IGNORE_CASE:1;
> + unsigned DEFAULT_FOLLOW_RENAMES:1;
> +};
> +
> +static inline struct diff_flags diff_flags_or(struct diff_flags a,
> +   struct diff_flags b)
> +{
> + char *tmp_a = (char *)
> + char *tmp_b = (char *)
> + int i;
> +
> + for (i = 0; i < sizeof(struct diff_flags); i++)
> + tmp_a[i] |= tmp_b[i];
> +
> + return a;
> +}

This is doubly scary, but let's see why we need it by looking at the
callers.

> +#define DIFF_OPT_TST(opts, flag) ((opts)->flags.flag)
> +#define DIFF_OPT_TOUCHED(opts, flag) ((opts)->touched_flags.flag)
> +#define DIFF_OPT_SET(opts, flag) (((opts)->flags.flag = 
> 1),((opts)->touched_flags.flag = 1))
> +#define DIFF_OPT_CLR(opts, flag) (((opts)->flags.flag = 
> 0),((opts)->touched_flags.flag = 1))

These are trivial and straight-forward.

> +
>  #define DIFF_XDL_TST(opts, flag)((opts)->xdl_opts & XDF_##flag)
>  #define DIFF_XDL_SET(opts, flag)((opts)->xdl_opts |= XDF_##flag)
>  #define DIFF_XDL_CLR(opts, flag)((opts)->xdl_opts &= ~XDF_##flag)
> @@ -122,8 +139,8 @@ struct diff_options {
>   const char *a_prefix, *b_prefix;
>   const char *line_prefix;
>   size_t line_prefix_length;
> - unsigned flags;
> - unsigned touched_flags;
> + struct diff_flags flags;
> + struct diff_flags touched_flags;
>  
>   /* diff-filter bits */
>   unsigned int filter;
> @@ -388,7 +405,8 @@ extern int diff_result_code(struct diff_options *, int);
>  
>  extern void diff_no_index(struct rev_info *, int, const char **);
>  
> -extern int index_differs_from(const char *def, int diff_flags, int 
> ita_invisible_in_index);
> +extern int index_differs_from(const char *def, struct diff_flags flags,
> +   int ita_invisible_in_index);

OK.  I tend to think twice before passing any struct by value (even
something that starts its life as a small/single-word struct), but
let's see how much simpler this allows callers to become.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index d75b3805e..de08c2594 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -912,11 +912,12 @@ static int prepare_to_commit(const char *index_file, 
> const char *prefix,
>* submodules which were manually staged, which would
>* be really confusing.
>*/
> - int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
> + struct diff_flags flags = DIFF_FLAGS_INIT;
> + flags.OVERRIDE_SUBMODULE_CONFIG = 1;
>   if (ignore_submodule_arg &&
>   !strcmp(ignore_submodule_arg, "all"))
> -  

[PATCH 3/3] diff: convert flags to be stored in bitfields

[Brandon Williams]

We have have reached the limit of the number of flags that can be stored
in a single unsigned int.  In order to allow for more flags to be added
to the diff machinery in the future this patch converts the flags to be
stored in bitfields in 'struct diff_flags'.

Signed-off-by: Brandon Williams 
---
 builtin/commit.c |  7 +++--
 builtin/log.c|  2 +-
 diff-lib.c   |  6 ++--
 diff.c   |  3 +-
 diff.h   | 96 +---
 sequencer.c  |  5 +--
 6 files changed, 70 insertions(+), 49 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d75b3805e..de08c2594 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -912,11 +912,12 @@ static int prepare_to_commit(const char *index_file, 
const char *prefix,
 * submodules which were manually staged, which would
 * be really confusing.
 */
-   int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
+   struct diff_flags flags = DIFF_FLAGS_INIT;
+   flags.OVERRIDE_SUBMODULE_CONFIG = 1;
if (ignore_submodule_arg &&
!strcmp(ignore_submodule_arg, "all"))
-   diff_flags |= DIFF_OPT_IGNORE_SUBMODULES;
-   commitable = index_differs_from(parent, diff_flags, 1);
+   flags.IGNORE_SUBMODULES = 1;
+   commitable = index_differs_from(parent, flags, 1);
}
}
strbuf_release(_ident);
diff --git a/builtin/log.c b/builtin/log.c
index d81a09051..780975ed4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -134,7 +134,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)
 
if (default_date_mode)
parse_date_format(default_date_mode, >date_mode);
-   rev->diffopt.touched_flags = 0;
+   rev->diffopt.touched_flags = diff_flags_cleared;
 }
 
 static void cmd_log_init_finish(int argc, const char **argv, const char 
*prefix,
diff --git a/diff-lib.c b/diff-lib.c
index 4e0980caa..7375ef71d 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -71,7 +71,7 @@ static int match_stat_with_submodule(struct diff_options 
*diffopt,
 {
int changed = ce_match_stat(ce, st, ce_option);
if (S_ISGITLINK(ce->ce_mode)) {
-   unsigned orig_flags = diffopt->flags;
+   struct diff_flags orig_flags = diffopt->flags;
if (!DIFF_OPT_TST(diffopt, OVERRIDE_SUBMODULE_CONFIG))
set_diffopt_flags_from_submodule_config(diffopt, 
ce->name);
if (DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES))
@@ -534,7 +534,7 @@ int do_diff_cache(const struct object_id *tree_oid, struct 
diff_options *opt)
return 0;
 }
 
-int index_differs_from(const char *def, int diff_flags,
+int index_differs_from(const char *def, struct diff_flags flags,
   int ita_invisible_in_index)
 {
struct rev_info rev;
@@ -546,7 +546,7 @@ int index_differs_from(const char *def, int diff_flags,
setup_revisions(0, NULL, , );
DIFF_OPT_SET(, QUICK);
DIFF_OPT_SET(, EXIT_WITH_STATUS);
-   rev.diffopt.flags |= diff_flags;
+   rev.diffopt.flags = diff_flags_or(rev.diffopt.flags, flags);
rev.diffopt.ita_invisible_in_index = ita_invisible_in_index;
run_diff_index(, 1);
object_array_clear();
diff --git a/diff.c b/diff.c
index 6fd288420..6f1050d42 100644
--- a/diff.c
+++ b/diff.c
@@ -46,6 +46,7 @@ static int diff_no_prefix;
 static int diff_stat_graph_width;
 static int diff_dirstat_permille_default = 30;
 static struct diff_options default_diff_options;
+const struct diff_flags diff_flags_cleared = DIFF_FLAGS_INIT;
 static long diff_algorithm;
 static unsigned ws_error_highlight_default = WSEH_NEW;
 
@@ -5899,7 +5900,7 @@ int diff_can_quit_early(struct diff_options *opt)
 static int is_submodule_ignored(const char *path, struct diff_options *options)
 {
int ignored = 0;
-   unsigned orig_flags = options->flags;
+   struct diff_flags orig_flags = options->flags;
if (!DIFF_OPT_TST(options, OVERRIDE_SUBMODULE_CONFIG))
set_diffopt_flags_from_submodule_config(options, path);
if (DIFF_OPT_TST(options, IGNORE_SUBMODULES))
diff --git a/diff.h b/diff.h
index aca150ba2..d58f06106 100644
--- a/diff.h
+++ b/diff.h
@@ -60,42 +60,59 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct 
diff_options *opt, void *data)
 
 #define DIFF_FORMAT_CALLBACK   0x1000
 
-#define DIFF_OPT_RECURSIVE   (1 <<  0)
-#define DIFF_OPT_TREE_IN_RECURSIVE   (1 <<  1)
-#define DIFF_OPT_BINARY  (1 <<  2)
-#define DIFF_OPT_TEXT(1 <<  3)
-#define DIFF_OPT_FULL_INDEX  (1 <<  4)
-#define DIFF_OPT_SILENT_ON_REMOVE(1 <<  5)
-#define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)
-#define DIFF_OPT_FOLLOW_RENAMES  (1 <<