Re: [PATCH 8/8] t0012: test "-h" with builtins

[Jeff King <peff@peff.net>]

On Tue, May 30, 2017 at 10:23:54PM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Nope, I have those patches directly on your e83352ef23, and it passes. I
> > wonder if there's something funny between our environments. What does
> > the failure look like for you?
> 
> Travis seems to be seeing the same failure.  Curiously, the topic by
> itself passes for me; iow, pu fails, pu^2 doesn't fail.
> 
>     git.git/pu$ ./git rev-list -h
>     BUG: environment.c:173: setup_git_env called without repository
>     Aborted (core dumped)
> 
> Hmph...

Ah, OK, I can reproduce when merged with pu. Bisecting it was tricky.
To see the problem, you need both my new test _and_ b1ef400ee
(setup_git_env: avoid blind fall-back to ".git", 2016-10-20). The latter
is only in v2.13, so topics forked from v2.12 need that commit applied.

Anyway, the problem is sk/dash-is-previous, specifically fc5684b47
(revision.c: args starting with "-" might be a revision, 2017-02-25). It
looks like the revision parser used to just bail on "-h", because
revision.c would say "I don't recognize this" and then cmd_rev_list()
would similarly say "I don't recognize this" and call usage(). But now
we actually try to read it as a ref, which obviously requires being
inside a repository.

Normally that's OK, but because of the "-h doesn't set up the repo"
thing from 99caeed05, we may not have setup the repo, and so looking up
refs is forbidden. The fix is probably to have revision.c explicitly
recognize "-h" and bail on it as an unknown option (it can't handle
the flag itself because only the caller knows the full usage()).

I do wonder, though, if there's any other downside to trying to look up
other options as revisions (at least it wastes time doing nonsense
revision lookups on options known only to cmd_rev_list()).  I'm not sure
why that commit passes everything starting with a dash as a possible
revision, rather than just "-".

I.e.:

diff --git a/revision.c b/revision.c
index 5470c33ac..1e26c3951 100644
--- a/revision.c
+++ b/revision.c
@@ -2233,7 +2233,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			}
 			if (opts < 0)
 				exit(128);
-			maybe_opt = 1;
+			if (arg[1]) {
+				/* arg is an unknown option */
+				argv[left++] = arg;
+				continue;
+			} else {
+				/* special token "-" */
+				maybe_opt = 1;
+			}
 		}
 
 

I don't see anything in the commit message, but I didn't dig in the
mailing list. 


-Peff