| Message ID | 20190815214053.16594-24-newren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up merge API | expand |
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;
> 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);
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.
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.
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.
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;
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(-)