[PATCH] diff: report bogus input to -C/-M/-B

[PATCH] diff: report bogus input to -C/-M/-B

[Jeff King]

We already detect invalid input to these functions, but we
simply exit with an error code, never saying anything as
simple as "your input was wrong". Let's fix that.

Before:

  $ git diff -CM
  $ echo $?
  128

After:

  $ git diff -CM
  error: invalid argument to -C: M
  $ echo $?
  128

There should be no problems with having diff_opt_parse print
to stderr, as there is already precedent in complaining
about bogus --color and --output arguments.

Signed-off-by: Jeff King <peff@peff.net>
---
More old patch potpourri. Original thread:

  http://article.gmane.org/gmane.comp.version-control.git/146570

I think this one just got overlooked.

 diff.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index 71efa8e..8487643 100644
--- a/diff.c
+++ b/diff.c
@@ -3142,18 +3142,18 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	/* renames options */
 	else if (!prefixcmp(arg, "-B")) {
 		if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)
-			return -1;
+			return error("invalid argument to -B: %s", arg+2);
 	}
 	else if (!prefixcmp(arg, "-M")) {
 		if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
-			return -1;
+			return error("invalid argument to -M: %s", arg+2);
 		options->detect_rename = DIFF_DETECT_RENAME;
 	}
 	else if (!prefixcmp(arg, "-C")) {
 		if (options->detect_rename == DIFF_DETECT_COPY)
 			DIFF_OPT_SET(options, FIND_COPIES_HARDER);
 		if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
-			return -1;
+			return error("invalid argument to -C: %s", arg+2);
 		options->detect_rename = DIFF_DETECT_COPY;
 	}
 	else if (!strcmp(arg, "--no-renames"))
-- 
1.7.3.1.235.gdd6c0

Re: [PATCH] diff: report bogus input to -C/-M/-B

[Jonathan Nieder]

Jeff King wrote:

> Before:
> 
>   $ git diff -CM
>   $ echo $?
>   128
> 
> After:
> 
>   $ git diff -CM
>   error: invalid argument to -C: M
>   $ echo $?
>   128

Yes, please.

Who is it that exits(128) in this code path?  Are there other
functions it calls that might return an error without reporting it to
stderr?

Re: [PATCH] diff: report bogus input to -C/-M/-B

[Jeff King]

On Thu, Oct 21, 2010 at 11:53:40AM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > Before:
> > 
> >   $ git diff -CM
> >   $ echo $?
> >   128
> > 
> > After:
> > 
> >   $ git diff -CM
> >   error: invalid argument to -C: M
> >   $ echo $?
> >   128
> 
> Yes, please.
> 
> Who is it that exits(128) in this code path?  Are there other
> functions it calls that might return an error without reporting it to
> stderr?

It's setup_revisions(). It only happens when calling
handle_revision_opt(), which either succeeds, calls error() for its
error cases, or chains to diff_opt_parse(). The latter uses error()
appropriately except for the -B/-M/-C cases.

So I think we're good after this patch is applied.

-Peff

Re: [PATCH] diff: report bogus input to -C/-M/-B

[Kevin Ballard]

On Oct 21, 2010, at 7:49 AM, Jeff King wrote:

> We already detect invalid input to these functions, but we
> simply exit with an error code, never saying anything as
> simple as "your input was wrong". Let's fix that.
> 
> Before:
> 
>  $ git diff -CM
>  $ echo $?
>  128
> 
> After:
> 
>  $ git diff -CM
>  error: invalid argument to -C: M
>  $ echo $?
>  128
> 
> There should be no problems with having diff_opt_parse print
> to stderr, as there is already precedent in complaining
> about bogus --color and --output arguments.

Thank you. I've had a note to myself to fix this for the past month, and still haven't gotten around to it. Glad to see that procrastinating pays off! ;)

-Kevin Ballard