[0/13] parseopt fixes from -Wunused-parameters

Message

Jeff King Nov. 5, 2018, 6:37 a.m. UTC

Continuing my exploration of what -Wunused-parameters can show us, here
are some bug-fixes related to parse-options callbacks.

This is the last of the actual bug-fixes I've found. After this, I have
about 60 patches worth of cleanups (i.e., dropping the unused
parameters), and then I have a series to annotate parameters that must
be unused (e.g., for functions that must conform to callback
interfaces). After we can start compiling with -Wunused-parameters,
assuming we don't find the annotations too cumbersome.

But this series fixes real bugs. These first four fix segfaults:

  [01/13]: apply: mark include/exclude options as NONEG
  [02/13]: am: handle --no-patch-format option
  [03/13]: ls-files: mark exclude options as NONEG
  [04/13]: pack-objects: mark index-version option as NONEG

And these four fix cases where we just quietly do the wrong thing:

  [05/13]: cat-file: mark batch options with NONEG
  [06/13]: status: mark --find-renames option with NONEG
  [07/13]: format-patch: mark "--no-numbered" option with NONEG
  [08/13]: show-branch: mark --reflog option as NONEG

These ones are just message improvements:

  [09/13]: tag: mark "--message" option with NONEG
  [10/13]: cat-file: report an error on multiple --batch options
  [11/13]: apply: return -1 from option callback instead of calling exit(1)

This one is a segfault, but it has no callers. ;)

  [12/13]: parse-options: drop OPT_DATE()

And then this last one is mostly about annotating the callbacks. It
doesn't strictly need to happen here, but the alternative is that I'd
eventually have to deal with it in the later series I mentioned.

  [13/13]: assert NOARG/NONEG behavior of parse-options callbacks

 apply.c                       | 24 +++++++++++++++++++++---
 builtin/am.c                  |  4 +++-
 builtin/blame.c               |  4 ++++
 builtin/cat-file.c            | 10 +++++++---
 builtin/checkout-index.c      |  2 ++
 builtin/clean.c               |  1 +
 builtin/commit.c              |  5 ++++-
 builtin/fetch.c               |  2 ++
 builtin/grep.c                | 14 +++++++++++++-
 builtin/init-db.c             |  1 +
 builtin/interpret-trailers.c  |  2 ++
 builtin/log.c                 | 12 +++++++++++-
 builtin/ls-files.c            | 14 +++++++++++---
 builtin/merge-file.c          |  2 ++
 builtin/merge.c               |  1 +
 builtin/notes.c               |  7 +++++++
 builtin/pack-objects.c        |  5 ++++-
 builtin/read-tree.c           |  3 +++
 builtin/rebase.c              |  6 ++++++
 builtin/show-branch.c         |  3 ++-
 builtin/show-ref.c            |  1 +
 builtin/tag.c                 |  6 ++++--
 builtin/update-index.c        | 21 +++++++++++++++++++--
 parse-options-cb.c            | 14 +++++++-------
 parse-options.h               | 18 ++++++++++++++----
 ref-filter.c                  |  2 ++
 t/helper/test-parse-options.c |  2 +-
 t/t0040-parse-options.sh      | 22 ----------------------
 28 files changed, 155 insertions(+), 53 deletions(-)

-Peff

Comments

Duy Nguyen Nov. 5, 2018, 4:51 p.m. UTC | #1

On Mon, Nov 5, 2018 at 7:39 AM Jeff King <[e-mail]> wrote:
>
> Continuing my exploration of what -Wunused-parameters can show us, here
> are some bug-fixes related to parse-options callbacks.
>
> This is the last of the actual bug-fixes I've found. After this, I have
> about 60 patches worth of cleanups (i.e., dropping the unused
> parameters), and then I have a series to annotate parameters that must
> be unused (e.g., for functions that must conform to callback
> interfaces). After we can start compiling with -Wunused-parameters,
> assuming we don't find the annotations too cumbersome.

Another good thing from this series is there are fewer --no-options to complete.

About the annotating unused parameters related to segfault bug fixes
in this series. Should we just add something like

 if (unset)
    BUG("This code does not support --no-stuff");

which could be done in the same patches that fix the segfault, and it
extends the diff context a bit to see what these callbacks do without
opening up the editor, and also serves as a kind of annotation?

Jeff King Nov. 5, 2018, 6:49 p.m. UTC | #2

On Mon, Nov 05, 2018 at 05:51:07PM +0100, Duy Nguyen wrote:

> On Mon, Nov 5, 2018 at 7:39 AM Jeff King <[e-mail]> wrote:
> >
> > Continuing my exploration of what -Wunused-parameters can show us, here
> > are some bug-fixes related to parse-options callbacks.
> >
> > This is the last of the actual bug-fixes I've found. After this, I have
> > about 60 patches worth of cleanups (i.e., dropping the unused
> > parameters), and then I have a series to annotate parameters that must
> > be unused (e.g., for functions that must conform to callback
> > interfaces). After we can start compiling with -Wunused-parameters,
> > assuming we don't find the annotations too cumbersome.
> 
> Another good thing from this series is there are fewer --no-options to complete.
> 
> About the annotating unused parameters related to segfault bug fixes
> in this series. Should we just add something like
> 
>  if (unset)
>     BUG("This code does not support --no-stuff");
> 
> which could be done in the same patches that fix the segfault, and it
> extends the diff context a bit to see what these callbacks do without
> opening up the editor, and also serves as a kind of annotation?

That's done in the final patch. I did originally do it alongside the
actual segfault fixes, but since it needs doing in so many other
callbacks, too, it made sense to me to do it all together.

-Peff

Duy Nguyen Nov. 5, 2018, 6:51 p.m. UTC | #3

On Mon, Nov 5, 2018 at 7:49 PM Jeff King <[e-mail]> wrote:
>
> On Mon, Nov 05, 2018 at 05:51:07PM +0100, Duy Nguyen wrote:
>
> > On Mon, Nov 5, 2018 at 7:39 AM Jeff King <[e-mail]> wrote:
> > >
> > > Continuing my exploration of what -Wunused-parameters can show us, here
> > > are some bug-fixes related to parse-options callbacks.
> > >
> > > This is the last of the actual bug-fixes I've found. After this, I have
> > > about 60 patches worth of cleanups (i.e., dropping the unused
> > > parameters), and then I have a series to annotate parameters that must
> > > be unused (e.g., for functions that must conform to callback
> > > interfaces). After we can start compiling with -Wunused-parameters,
> > > assuming we don't find the annotations too cumbersome.
> >
> > Another good thing from this series is there are fewer --no-options to complete.
> >
> > About the annotating unused parameters related to segfault bug fixes
> > in this series. Should we just add something like
> >
> >  if (unset)
> >     BUG("This code does not support --no-stuff");
> >
> > which could be done in the same patches that fix the segfault, and it
> > extends the diff context a bit to see what these callbacks do without
> > opening up the editor, and also serves as a kind of annotation?
>
> That's done in the final patch. I did originally do it alongside the
> actual segfault fixes, but since it needs doing in so many other
> callbacks, too, it made sense to me to do it all together.

Oops. I guess I stopped reading the series too early. Sorry for the noise.