Jeff King [off-list ref] 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...
On Tue, May 30, 2017 at 10:23:54PM +0900, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
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
[+cc Siddharth, so quoting copiously]
On Tue, May 30, 2017 at 11:27:56AM -0400, Jeff King wrote:
quoted hunk
quoted
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