threads / patch / 25499

patchdiff: report bogus input to -C/-M/-B

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

## tl;dr

4 messages between Oct 21, 2010 and Oct 21, 2010. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Jeff King· Oct 21, 2010, 14:49 UTC · lore

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(-)

Show changes to diff.c +3 −3

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

Jonathan Nieder· Oct 21, 2010, 16:53 UTC · re: Jeff King · lore

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

Jeff King wrote:

Show 12 quoted lines

> 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?

Jeff King· Oct 21, 2010, 17:07 UTC · re: Jonathan Nieder · lore

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

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

Show 20 quoted lines

> 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

Kevin Ballard· Oct 21, 2010, 20:37 UTC · re: Jeff King · lore

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

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

Show 20 quoted lines

> 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