[+cc Siddharth, so quoting copiously] On Tue, May 30, 2017 at 11:27:56AM -0400, Jeff King wrote:
> > 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. I think this line of reasoning comes from http://public-inbox.org/git/20170206181026.GA4010@ubuntu-512mb-blr1-01.localdomain/ And the idea is that ranges like "-.." should work. TBH, I'm not sure how I feel about that, for exactly the reason that came up here: it makes it hard to syntactically differentiate the "-" shorthand from actual options. We do have @{-1} already for this purpose. I don't mind "-" as a shortcut for things like "git checkout -" or "git show -", but it feels like most of the benefit is lost when you're combining it with other operators. -Peff