[v3,23/24] merge-recursive: add sanity checks for relevant merge_options
diff mbox series

Message ID 20190815214053.16594-24-newren@gmail.com
State New
Headers show
Series
  • Clean up merge API
Related show

Commit Message

Elijah Newren Aug. 15, 2019, 9:40 p.m. UTC
There are lots of options that callers can set, yet most have a limited
range of valid values, some options are meant for output (e.g.
opt->obuf, which is expected to start empty), and callers are expected
to not set opt->priv.  Add several sanity checks to ensure callers
provide sane values.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 23 +++++++++++++++++++++++
 merge-recursive.h |  2 +-
 2 files changed, 24 insertions(+), 1 deletion(-)

Comments

Junio C Hamano Aug. 16, 2019, 7:52 p.m. UTC | #1
Elijah Newren <newren@gmail.com> writes:

> There are lots of options that callers can set, yet most have a limited
> range of valid values, some options are meant for output (e.g.
> opt->obuf, which is expected to start empty), and callers are expected
> to not set opt->priv.  Add several sanity checks to ensure callers
> provide sane values.
> ...

The change to the struct does not seem to have much with the above
rationale.

> diff --git a/merge-recursive.h b/merge-recursive.h
> index 978847e672..d201ee80fb 100644
> --- a/merge-recursive.h
> +++ b/merge-recursive.h
> @@ -27,7 +27,7 @@ struct merge_options {
>  	} detect_directory_renames;
>  	int rename_limit;
>  	int rename_score;
> -	int show_rename_progress;
> +	int show_rename_progress : 1;

And a one-bit wide bitfield that is signed is always an iffy idea.

>  
>  	/* xdiff-related options (patience, ignore whitespace, ours/theirs) */
>  	long xdl_opts;
Junio C Hamano Aug. 16, 2019, 7:59 p.m. UTC | #2
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 647b1f25c3..bc0da608c4 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -3620,6 +3620,29 @@ static int merge_start(struct merge_options *opt, struct tree *head)
>  ...
> +	assert(opt->buffer_output >= 0 && opt->buffer_output <= 2);

The field is unsigned, so >=0 side triggers "-Werror=type-limits" warning.

Material for squashing I have collected so far...

 cache-tree.c      | 2 +-
 merge-recursive.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index 00eda3e537..ef8c9f5e04 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -608,7 +608,7 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat
 	return it;
 }
 
-int write_index_as_tree_internal(struct object_id *oid, struct index_state *index_state, int cache_tree_valid, int flags, const char *prefix)
+static int write_index_as_tree_internal(struct object_id *oid, struct index_state *index_state, int cache_tree_valid, int flags, const char *prefix)
 {
 	if (flags & WRITE_TREE_IGNORE_CACHE_TREE) {
 		cache_tree_free(&index_state->cache_tree);
diff --git a/merge-recursive.c b/merge-recursive.c
index d3dc3d8a49..3d126dcc48 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3638,7 +3638,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)
 	       opt->recursive_variant <= MERGE_VARIANT_THEIRS);
 
 	assert(opt->verbosity >= 0 && opt->verbosity <= 5);
-	assert(opt->buffer_output >= 0 && opt->buffer_output <= 2);
+	assert(opt->buffer_output <= 2);
 	assert(opt->obuf.len == 0);
 
 	assert(opt->priv == NULL);
Elijah Newren Aug. 16, 2019, 10:08 p.m. UTC | #3
On Fri, Aug 16, 2019 at 12:52 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > There are lots of options that callers can set, yet most have a limited
> > range of valid values, some options are meant for output (e.g.
> > opt->obuf, which is expected to start empty), and callers are expected
> > to not set opt->priv.  Add several sanity checks to ensure callers
> > provide sane values.
> > ...
>
> The change to the struct does not seem to have much with the above
> rationale.

If the only possible values are 0 and 1, I can either add assertions
to check that at run time, or make the compiler check it for us by
confining its value to a single bit.  A compile-time check seems more
robust...

> > diff --git a/merge-recursive.h b/merge-recursive.h
> > index 978847e672..d201ee80fb 100644
> > --- a/merge-recursive.h
> > +++ b/merge-recursive.h
> > @@ -27,7 +27,7 @@ struct merge_options {
> >       } detect_directory_renames;
> >       int rename_limit;
> >       int rename_score;
> > -     int show_rename_progress;
> > +     int show_rename_progress : 1;
>
> And a one-bit wide bitfield that is signed is always an iffy idea.

...assuming of course I don't mess it up like this, not noticing that
it's an int that needs to be changed to an unsigned.  I'll fix this
up.

But the fact that you flagged the struct change -- would you prefer
some commit message explanation of how it's related, or was it more
the case that you felt it was a different kind of change and wanted it
split out into a separate patch?  I'm suspecting the former but am not
quite sure.
Elijah Newren Aug. 16, 2019, 10:09 p.m. UTC | #4
On Fri, Aug 16, 2019 at 12:59 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> > diff --git a/merge-recursive.c b/merge-recursive.c
> > index 647b1f25c3..bc0da608c4 100644
> > --- a/merge-recursive.c
> > +++ b/merge-recursive.c
> > @@ -3620,6 +3620,29 @@ static int merge_start(struct merge_options *opt, struct tree *head)
> >  ...
> > +     assert(opt->buffer_output >= 0 && opt->buffer_output <= 2);
>
> The field is unsigned, so >=0 side triggers "-Werror=type-limits" warning.
>
> Material for squashing I have collected so far...
<snip>

Thanks, will include all of this in my next round.
Junio C Hamano Aug. 16, 2019, 11:15 p.m. UTC | #5
Elijah Newren <newren@gmail.com> writes:

> If the only possible values are 0 and 1, I can either add assertions
> to check that at run time, or make the compiler check it for us by
> confining its value to a single bit.  A compile-time check seems more
> robust...

Sure, as long as they can catch assignments (e.g. ".field = 2",
or more interestingly ".field = .field + 1" in a loop, etc.) and
increments or decrements and flag them.

> But the fact that you flagged the struct change -- would you prefer
> some commit message explanation of how it's related, or was it more
> the case that you felt it was a different kind of change and wanted it
> split out into a separate patch?  I'm suspecting the former but am not
> quite sure.

I do not see it as related at all, so either split it out into a
separate patch, or just drop it (and have a runtime check as
everybody else in this step), would be the sensible alternatives.  I
think the latter is easier to reason about but it may be just me.

Patch
diff mbox series

diff --git a/merge-recursive.c b/merge-recursive.c
index 647b1f25c3..bc0da608c4 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3620,6 +3620,29 @@  static int merge_start(struct merge_options *opt, struct tree *head)
 {
 	struct strbuf sb = STRBUF_INIT;
 
+	/* Sanity checks on opt */
+	assert(opt->repo);
+
+	assert(opt->branch1 && opt->branch2);
+
+	assert(opt->detect_renames >= -1 &&
+	       opt->detect_renames <= DIFF_DETECT_COPY);
+	assert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE &&
+	       opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE);
+	assert(opt->rename_limit >= -1);
+	assert(opt->rename_score >= 0 && opt->rename_score <= MAX_SCORE);
+
+	assert(opt->xdl_opts >= 0);
+	assert(opt->recursive_variant >= MERGE_VARIANT_NORMAL &&
+	       opt->recursive_variant <= MERGE_VARIANT_THEIRS);
+
+	assert(opt->verbosity >= 0 && opt->verbosity <= 5);
+	assert(opt->buffer_output >= 0 && opt->buffer_output <= 2);
+	assert(opt->obuf.len == 0);
+
+	assert(opt->priv == NULL);
+
+	/* Sanity check on repo state; index must match head */
 	if (repo_index_has_changes(opt->repo, head, &sb)) {
 		err(opt, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
 		    sb.buf);
diff --git a/merge-recursive.h b/merge-recursive.h
index 978847e672..d201ee80fb 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -27,7 +27,7 @@  struct merge_options {
 	} detect_directory_renames;
 	int rename_limit;
 	int rename_score;
-	int show_rename_progress;
+	int show_rename_progress : 1;
 
 	/* xdiff-related options (patience, ignore whitespace, ours/theirs) */
 	long xdl_opts;