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

[Jeff King]

On Tue, Jun 13, 2017 at 04:08:03PM -0700, Jonathan Nieder wrote:

> > +test_expect_success 'generate builtin list' '
> > +   git --list-builtins >builtins
> > +'
> > +
> > +while read builtin
> > +do
> > +   test_expect_success "$builtin can handle -h" '
> > +   test_expect_code 129 git $builtin -h >output 2>&1 &&
> > +   test_i18ngrep usage output
> > +   '
> > +done  
> I love this.  A few thoughts:
> 
> - I think the "generate builtin list" test should be marked as setup
>   so people know it can't be skipped.  I'll send a followup to do that.
> 
> - By the same token, if "generate builtin list" fails then these
>   commands afterward would fail in an unpleasant way.  Fortunately
>   that's straightforward to fix, too.

Be my guest if you want, but I don't think either of those is actually
worth spending any time on. It's a nice convenience when tests are
independent, but ultimately if any single test fails, the results of all
tests after it must be suspect. There are too many hidden dependencies
to treat it any other way.

So I have no problem with skipping tests while debugging for a quicker
turnaround, with the knowledge that what you're seeing _might_ be
slightly invalidated. But I don't think it's worth dumping developer
time into trying to make each block independent. That's what individual
scripts are for.

(I also think it's particularly worthless for this script; the whole
thing runs in ~500ms on my machine. It takes longer to type
GIT_TEST_SKIP).

> - This checks both stdout and stderr to verify that the usage appears
>   somewhere.  I would have expected commands to be consistent ---
>   should they always send usage to stdout, since the user requested it,
>   or to stderr, since that's what we have done historically?
> 
>   So I understand why this test is agnostic about that but I suspect
>   it's pointing to some inconsistency that could be fixed independently.

The difference is between parse-option's usage_with_options() and
usage(). The former sends "-h" to stdout and errors to stderr, which I
think is sensible. The latter only knows about stderr.

I think it would be reasonable to have follow the parse-options strategy
consistently. It will definitely take some tweaking, though. There are
lots of commands that respect "-h" only by hitting some default
unhandled case. So you'll not only have to have a stdout analog for
usage(), but you'll have to teach each command when to use each.

> - Do we plan to translate the "usage:" prefix some day?  I could go
>   both ways --- on one hand, having a predictable error:/usage:/fatal:
>   prefix may make life easier for e.g. GUI authors trying to show
>   different kinds of error messages in different ways, but on the other
>   hand, always using English for the prefix may be unfriendly to non
>   English speakers.
> 
>   In the past I had assumed it would never be translated but now I'm
>   not so sure.  What do you think?

I don't have an opinion either way. I know we don't translate it now,
but it has been discussed. I figured it couldn't hurt to use i18ngrep to
future-proof it.

> - Should we have something like this for non-builtins, too?

That would be nice, though you'll probably need some cooperation from
the Makefile to get the list. I was specifically worried about catching
the particular setup_git_directory() interaction with builtins here, so
it seemed like a good stopping point.

But as a general rule, making sure that everything responds sensibly to
"-h" seems like a good thing.

-Peff

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

[Jonathan Nieder]

Hi,

Jeff King wrote:

> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -49,4 +49,16 @@ test_expect_success "--help does not work for guides" "
>   test_i18ncmp expect actual
>  "
> 
> +test_expect_success 'generate builtin list' '
> + git --list-builtins >builtins
> +'
> +
> +while read builtin
> +do
> + test_expect_success "$builtin can handle -h" '
> + test_expect_code 129 git $builtin -h >output 2>&1 &&
> + test_i18ngrep usage output
> + '
> +done 

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

[Junio C Hamano]

Jeff King  writes:

> If we don't mind a one-time pain, I think we can just convert the
> existing usage() to something more like usage_with_options(). The patch
> below does that (and teaches the latter to handle a NULL options field).
>
> The diff is ugly, and the conversion is mostly mechanical. But I think
> some sites can be improved. For example, look at the change in bundle.c,
> which now hands a set of strings rather than formatting the whole "or:"
> chain manually.
> ...
> With bonus points for actually describing each option (though at that
> point it may be as much work to just convert the thing to parse-options,
> which would also be fine with me).
>
> That's tedious work which would need attention paid to each individual
> command. So I'd probably do a single mechanical patch like this one
> (that keeps the output identical), and then let people fix each one up
> on top.

Yup.  Unlike my other hack, this does look more useful to me.  It
was kind of surprising that the change to parse-options is just a
single liner, but it's just "no input resulting in no output" ;-)

> diff --git a/git.c b/git.c
> index 1b8b7f51a..3496f8a23 100644
> --- a/git.c
> +++ b/git.c
> @@ -3,12 +3,14 @@
>  #include "help.h"
>  #include "run-command.h"
>  
> -const char git_usage_string[] =
> +const char *git_usage_string[] = {
>   "git [--version] [--help] [-C ] [-c name=value]\n"
>   "   [--exec-path[=]] [--html-path] [--man-path] 
> [--info-path]\n"
>   "   [-p | --paginate | --no-pager] [--no-replace-objects] 
> [--bare]\n"
>   "   [--git-dir=] [--work-tree=] 
> [--namespace=]\n"
> - "[]";
> + "[]",
> + NULL
> +};
>  
>  const char git_more_info_string[] =
>   N_("'git help -a' and 'git help -g' list available subcommands and 
> some\n"
> @@ -694,7 +696,7 @@ int cmd_main(int argc, const char **argv)
>   } else {
>   /* The user didn't specify a command; give them help */
>   commit_pager_choice();
> - printf("usage: %s\n\n", git_usage_string);
> + printf("usage: %s\n\n", git_usage_string[0]);
>   list_common_cmds_help();
>   printf("\n%s\n", _(git_more_info_string));
>   exit(1);


> diff --git a/parse-options.c b/parse-options.c
> index a23a1e67f..b0cc2a410 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -600,6 +600,9 @@ static int usage_with_options_internal(struct 
> parse_opt_ctx_t *ctx,
>   usagestr++;
>   }
>  
> + if (!opts)
> + return PARSE_OPT_HELP;
> +
>   if (opts->type != OPTION_GROUP)
>   fputc('\n', outfile);

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

[Jeff King]

On Thu, Jun 01, 2017 at 02:54:08PM +0900, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > For now, I will mix this in when queuing the whole thing in 'pu', as
> > I hate to push out something that does not even work for me to the
> > general public.
> > 
> > -- >8 --
> > Subject: [PATCH] diff- and log- family: handle "git cmd -h" early
> > ...
> 
> And then the check_help_option() thing may look like this.  
> 
> I am not proud of the way it "unifies" the two styles of usage
> strings, obviously.

Heh, I came up with a similar hack. It is pretty nasty.

If we don't mind a one-time pain, I think we can just convert the
existing usage() to something more like usage_with_options(). The patch
below does that (and teaches the latter to handle a NULL options field).

The diff is ugly, and the conversion is mostly mechanical. But I think
some sites can be improved. For example, look at the change in bundle.c,
which now hands a set of strings rather than formatting the whole "or:"
chain manually.

I think we could take this even further and break some of the really
long entries (e.g., index-pack) into:

  static const char *index_pack_usage[] = {
"git index-pack ",
"git index-pack --stdin",
"",
"-v",
"-o ",
etc...
NULL
  };

With bonus points for actually describing each option (though at that
point it may be as much work to just convert the thing to parse-options,
which would also be fine with me).

That's tedious work which would need attention paid to each individual
command. So I'd probably do a single mechanical patch like this one
(that keeps the output identical), and then let people fix each one up
on top.

> One benefit this patch has is that it makes it easier to highlight
> what it does *not* touch.
> 
> $ git grep -A2 -E -e 'a(rg)?c [!=]= 2 .*strcmp.*-h'
> 
> shows there are somewhat curious construct
> 
>   if (argc != 2 || !strcmp(argv[1], "-h"))
>   usage(...);
> 
> left in the code.  Upon closer inspection, they all happen to be
> doing the right thing for their current set of options and
> arguments, but they are somewhat ugly.

Yeah, I noticed those too while looking for prior art in my "-h" series.
I think they're all sane. And in fact some of them are necessary (and I
think I even added one like what you quoted above) because the arguments
are not otherwise parsed.

So for something that calls parse_options() later, just:

  if (argc == 2 && !strcmp(argv[1], "-h"))
usage(...);

is sufficient to catch the single funny case, and parse_options()
catches the rest. But for some commands, this is all the parsing they
do. We could split that in two, like:

  /* early "-h" check */
  if (argc == 2 && !strcmp(argv[1], "-h"))
usage(...);
  /* now check for argument sanity */
  if (argc != 2)
usage(...);

That would make sense if we wanted to treat "-h" different from a normal
"huh?" response, but we don't currently. I don't know that it's worth
worrying about too much either way.

Anyway, here's the usage[] patch. I based it on the jk/consistent-h
branch, since I added a few new calls there. I won't be surprised if I
missed a NULL terminator somewhere, but it at least passes the big "-h"
loop in t0012. :)

---
diff --git a/builtin.h b/builtin.h
index 9e4a89816..c31fac64f 100644
--- a/builtin.h
+++ b/builtin.h
@@ -8,7 +8,7 @@
 
 #define DEFAULT_MERGE_LOG_LEN 20
 
-extern const char git_usage_string[];
+extern const char *git_usage_string[];
 extern const char git_more_info_string[];
 
 #define PRUNE_PACKED_DRY_RUN 01
diff --git a/builtin/blame.c b/builtin/blame.c
index 1043e5376..58cc1e8e8 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -30,10 +30,8 @@
 #include "dir.h"
 #include "progress.h"
 
-static char blame_usage[] = N_("git blame [] [] [] 
[--] ");
-
 static const char *blame_opt_usage[] = {
-   blame_usage,
+   N_("git blame [] [] [] [--] "),
"",
N_(" are documented in git-rev-list(1)"),
NULL
@@ -2865,7 +2863,7 @@ int cmd_blame(int argc, const char **argv, const char 
*prefix)
if (parse_range_arg(range_list.items[range_i].string,
nth_line_cb, , lno, anchor,
, , sb.path))
-   usage(blame_usage);
+   usage(blame_opt_usage);
if (lno < top || ((lno || bottom) && lno < bottom))
die(Q_("file %s has only %lu line",
   "file %s has only %lu lines",
diff --git a/builtin/bundle.c b/builtin/bundle.c
index d0de59b94..2284a2413 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -9,11 +9,13 @@
  * bundle supporting "fetch", "pull", and "ls-remote".
  */
 
-static const char builtin_bundle_usage[] =
-  "git bundle create  \n"
-  "   or: git bundle verify \n"
-  "   or: git bundle list-heads  [...]\n"
-  "   or: git bundle unbundle  [...]";
+static 

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

[Jeff King]

On Thu, Jun 01, 2017 at 02:42:13PM +0900, Junio C Hamano wrote:

> For now, I will mix this in when queuing the whole thing in 'pu', as
> I hate to push out something that does not even work for me to the
> general public.
> 
> -- >8 --
> Subject: [PATCH] diff- and log- family: handle "git cmd -h" early

Yeah, I think this is a good step in the interim.

-Peff

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

[Junio C Hamano]

Junio C Hamano  writes:

> For now, I will mix this in when queuing the whole thing in 'pu', as
> I hate to push out something that does not even work for me to the
> general public.
> 
> -- >8 --
> Subject: [PATCH] diff- and log- family: handle "git cmd -h" early
> ...

And then the check_help_option() thing may look like this.  

I am not proud of the way it "unifies" the two styles of usage
strings, obviously.

One benefit this patch has is that it makes it easier to highlight
what it does *not* touch.

$ git grep -A2 -E -e 'a(rg)?c [!=]= 2 .*strcmp.*-h'

shows there are somewhat curious construct

if (argc != 2 || !strcmp(argv[1], "-h"))
usage(...);

left in the code.  Upon closer inspection, they all happen to be
doing the right thing for their current set of options and
arguments, but they are somewhat ugly.


 builtin/am.c   |  3 +--
 builtin/branch.c   |  3 +--
 builtin/check-ref-format.c |  3 +--
 builtin/checkout-index.c   |  7 ---
 builtin/commit.c   |  6 ++
 builtin/diff-files.c   |  3 +--
 builtin/diff-index.c   |  3 +--
 builtin/diff-tree.c|  3 +--
 builtin/gc.c   |  3 +--
 builtin/index-pack.c   |  3 +--
 builtin/ls-files.c |  3 +--
 builtin/merge-ours.c   |  3 +--
 builtin/merge.c|  3 +--
 builtin/pack-redundant.c   |  3 +--
 builtin/rev-list.c |  3 +--
 builtin/update-index.c |  3 +--
 builtin/upload-archive.c   |  3 +--
 fast-import.c  |  3 +--
 git-compat-util.h  |  3 +++
 usage.c| 11 +++
 20 files changed, 36 insertions(+), 39 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 8881d73615..12b7298907 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2307,8 +2307,7 @@ int cmd_am(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
 
-   if (argc == 2 && !strcmp(argv[1], "-h"))
-   usage_with_options(usage, options);
+   check_help_option(argc, argv, usage, options);
 
git_config(git_am_config, NULL);
 
diff --git a/builtin/branch.c b/builtin/branch.c
index 83fcda43dc..8c4465f0e4 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -599,8 +599,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
filter.kind = FILTER_REFS_BRANCHES;
filter.abbrev = -1;
 
-   if (argc == 2 && !strcmp(argv[1], "-h"))
-   usage_with_options(builtin_branch_usage, options);
+   check_help_option(argc, argv, builtin_branch_usage, options);
 
git_config(git_branch_config, NULL);
 
diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index eac499450f..aab5776dd5 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -55,8 +55,7 @@ int cmd_check_ref_format(int argc, const char **argv, const 
char *prefix)
int flags = 0;
const char *refname;
 
-   if (argc == 2 && !strcmp(argv[1], "-h"))
-   usage(builtin_check_ref_format_usage);
+   check_help_option(argc, argv, builtin_check_ref_format_usage, NULL);
 
if (argc == 3 && !strcmp(argv[1], "--branch"))
return check_ref_format_branch(argv[2]);
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 07631d0c9c..8dd28ae8ba 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -179,9 +179,10 @@ int cmd_checkout_index(int argc, const char **argv, const 
char *prefix)
OPT_END()
};
 
-   if (argc == 2 && !strcmp(argv[1], "-h"))
-   usage_with_options(builtin_checkout_index_usage,
-  builtin_checkout_index_options);
+   check_help_option(argc, argv,
+ builtin_checkout_index_usage,
+ builtin_checkout_index_options);
+
git_config(git_default_config, NULL);
prefix_length = prefix ? strlen(prefix) : 0;
 
diff --git a/builtin/commit.c b/builtin/commit.c
index 66c9ac587b..05c2f61e33 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1376,8 +1376,7 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
OPT_END(),
};
 
-   if (argc == 2 && !strcmp(argv[1], "-h"))
-   usage_with_options(builtin_status_usage, 
builtin_status_options);
+   check_help_option(argc, argv, builtin_status_usage, 
builtin_status_options);
 
status_init_config(, git_status_config);
argc = parse_options(argc, argv, prefix,
@@ -1661,8 +1660,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
 
-   if (argc == 2 && !strcmp(argv[1], "-h"))
-   usage_with_options(builtin_commit_usage, 
builtin_commit_options);
+   check_help_option(argc, argv, builtin_commit_usage, 
builtin_commit_options);
 
status_init_config(, 

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

[Junio C Hamano]

Junio C Hamano  writes:

> Heh, I found another ;-)  
>
> 95e98cd9 ("revision.c: use refs_for_each*() instead of
> for_each_*_submodule()", 2017-04-19), which is in the middle of
> Duy's nd/prune-in-worktree series, does this:
> ...
> when jk/consistent-h is merged into it and then "git diff-files -h"
> is run.
>
> I guess anything that calls setup_revisions() from the "git cmd -h"
> bypass need to be prepared with that
>
>   check_help_option(argc, argv, usage, options);
>
> thing.  Which is a bit sad, but I tend to agree with you that
> restructuring to make usage[] of everybody available to git.c
> is probably too noisy for the benefit it would give us.

For now, I will mix this in when queuing the whole thing in 'pu', as
I hate to push out something that does not even work for me to the
general public.

-- >8 --
Subject: [PATCH] diff- and log- family: handle "git cmd -h" early

"git $builtin -h" bypasses the usual repository setup and calls the
cmd_$builtin() function, expecting it to show the help text.

Unfortunately the commands in the log- and the diff- family want to
call into the revisions machinery, which by definition needs to have
a repository already discovered, before they can parse the options.

Handle the "git $builtin -h" special case very early in these
commands to work around potential issues.

Signed-off-by: Junio C Hamano 
---
 builtin/diff-files.c | 3 +++
 builtin/diff-index.c | 3 +++
 builtin/diff-tree.c  | 3 +++
 builtin/rev-list.c   | 3 +++
 4 files changed, 12 insertions(+)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 15c61fd8d1..6be1df684a 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -20,6 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char 
*prefix)
int result;
unsigned options = 0;
 
+   if (argc == 2 && !strcmp(argv[1], "-h"))
+   usage(diff_files_usage);
+
init_revisions(, prefix);
gitmodules_config();
git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 1af373d002..02dd35ba45 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -17,6 +17,9 @@ int cmd_diff_index(int argc, const char **argv, const char 
*prefix)
int i;
int result;
 
+   if (argc == 2 && !strcmp(argv[1], "-h"))
+   usage(diff_cache_usage);
+
init_revisions(, prefix);
gitmodules_config();
git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 5ea1c14317..f633b10b08 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -105,6 +105,9 @@ int cmd_diff_tree(int argc, const char **argv, const char 
*prefix)
struct setup_revision_opt s_r_opt;
int read_stdin = 0;
 
+   if (argc == 2 && !strcmp(argv[1], "-h"))
+   usage(diff_tree_usage);
+
init_revisions(opt, prefix);
gitmodules_config();
git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 718c6059c9..b250c515b1 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -277,6 +277,9 @@ int cmd_rev_list(int argc, const char **argv, const char 
*prefix)
int use_bitmap_index = 0;
const char *show_progress = NULL;
 
+   if (argc == 2 && !strcmp(argv[1], "-h"))
+   usage(rev_list_usage);
+
git_config(git_default_config, NULL);
init_revisions(, prefix);
revs.abbrev = DEFAULT_ABBREV;
-- 
2.13.0-513-g1c0955652f

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

[Jeff King]

On Thu, Jun 01, 2017 at 01:17:55PM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > 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.
> 
> Heh, I found another ;-)  
> 
> 95e98cd9 ("revision.c: use refs_for_each*() instead of
> for_each_*_submodule()", 2017-04-19), which is in the middle of
> Duy's nd/prune-in-worktree series, does this:

Hrm, yeah. The problem is that handle_revision_pseudo_opt() initializes
the ref store at the top of the function, even if we don't see any
arguments that require us to use it (and obviously in the "-h" case, we
don't).

That's an implementation detail that we could fix, but I do think in
general that we should probably just declare it forbidden to call
setup_revisions() when the repo hasn't been discovered.

> I guess anything that calls setup_revisions() from the "git cmd -h"
> bypass need to be prepared with that
> 
>   check_help_option(argc, argv, usage, options);
> 
> thing.  Which is a bit sad, but I tend to agree with you that
> restructuring to make usage[] of everybody available to git.c
> is probably too noisy for the benefit it would give us.

The other options are:

  - reverting the "-h" magic in git.c. It really is the source of most
of this confusion, I think, because functions which assume RUN_SETUP
are having that assumption broken. But at the same time I do think
it makes "-h" a lot friendlier, and I'd prefer to keep it.

  - reverting the BUG() in setup_git_env(); this has been flushing out a
lot of bugs, and I think is worth keeping

I did look at writing something like check_help_option(). One of the
annoyances is that we have two different usage formats: one that's a
straight string for usage(), and one that's an array-of-strings for
parse_options(). We could probably unify those.

It doesn't actually save that much code, though. The real value is that
it abstracts the "did git.c decide to skip RUN_SETUP?" logic.

-Peff

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

[Junio C Hamano]

Jeff King  writes:

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

Heh, I found another ;-)  

95e98cd9 ("revision.c: use refs_for_each*() instead of
for_each_*_submodule()", 2017-04-19), which is in the middle of
Duy's nd/prune-in-worktree series, does this:

#0  die (err=0x6128f8 "BUG: setup_git_env called without repository") at 
usage.c:114
#1  0x004f9467 in setup_git_env () at environment.c:172
#2  0x004f966c in get_git_dir () at environment.c:214
#3  0x0055113b in get_main_ref_store () at refs.c:1544
#4  0x00570ee0 in handle_revision_pseudo_opt (submodule=0x0, 
revs=0x7fffd6a0, argc=1, argv=0x7fffe180, flags=0x7fffc59c)
at revision.c:2110
#5  0x005716f5 in setup_revisions (argc=2, argv=0x7fffe178, 
revs=0x7fffd6a0, opt=0x0) at revision.c:2254
#6  0x0043074a in cmd_diff_files (argc=2, argv=0x7fffe178, 
prefix=0x0)
at builtin/diff-files.c:29
#7  0x00405907 in run_builtin (p=0x87ba00 , argc=2, 
argv=0x7fffe178) at git.c:376
#8  0x00405bb5 in handle_builtin (argc=2, argv=0x7fffe178) at 
git.c:584
#9  0x00405e04 in cmd_main (argc=2, argv=0x7fffe178) at git.c:683
#10 0x004a3364 in main (argc=2, argv=0x7fffe178) at common-main.c:43

when jk/consistent-h is merged into it and then "git diff-files -h"
is run.

I guess anything that calls setup_revisions() from the "git cmd -h"
bypass need to be prepared with that

  check_help_option(argc, argv, usage, options);

thing.  Which is a bit sad, but I tend to agree with you that
restructuring to make usage[] of everybody available to git.c
is probably too noisy for the benefit it would give us.

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

[Junio C Hamano]

Jeff King  writes:

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

All correct and that is why I haven't seriously considered merging
the topic further down (yet).  Things like -.. makes readers of the
commands go "Huh?", and "git tbdiff ..- -.." does not even work ;-)

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

[Jeff King]

[+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

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

[Jeff King]

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

> Jeff King  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

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

[Junio C Hamano]

Jeff King  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...

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

[Jeff King]

On Tue, May 30, 2017 at 03:08:25PM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > On Tue, May 30, 2017 at 03:03:18PM +0900, Junio C Hamano wrote:
> >
> >> > +test_expect_success 'generate builtin list' '
> >> > +git --list-builtins >builtins
> >> > +'
> >> > +
> >> > +while read builtin
> >> > +do
> >> > +test_expect_success "$builtin can handle -h" '
> >> > +test_expect_code 129 git $builtin -h >output 2>&1 &&
> >> > +test_i18ngrep usage output
> >> > +'
> >> > +done  >> > +
> >> 
> >> These still seem to need further tweaks?
> >> 
> >> diff-files
> >> diff-index
> >> diff-tree
> >> rev-list
> >
> > How so? They pass the test for me, and the output for a manual run looks
> > sane.
> 
> Am I missing some patches (I have these 8) outside the series,
> perhaps?

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?

-Peff

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

[Junio C Hamano]

Jeff King  writes:

> On Tue, May 30, 2017 at 03:03:18PM +0900, Junio C Hamano wrote:
>
>> > +test_expect_success 'generate builtin list' '
>> > +  git --list-builtins >builtins
>> > +'
>> > +
>> > +while read builtin
>> > +do
>> > +  test_expect_success "$builtin can handle -h" '
>> > +  test_expect_code 129 git $builtin -h >output 2>&1 &&
>> > +  test_i18ngrep usage output
>> > +  '
>> > +done > > +
>> 
>> These still seem to need further tweaks?
>> 
>> diff-files
>> diff-index
>> diff-tree
>> rev-list
>
> How so? They pass the test for me, and the output for a manual run looks
> sane.

Am I missing some patches (I have these 8) outside the series,
perhaps?

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

[Jeff King]

On Tue, May 30, 2017 at 03:03:18PM +0900, Junio C Hamano wrote:

> > +test_expect_success 'generate builtin list' '
> > +   git --list-builtins >builtins
> > +'
> > +
> > +while read builtin
> > +do
> > +   test_expect_success "$builtin can handle -h" '
> > +   test_expect_code 129 git $builtin -h >output 2>&1 &&
> > +   test_i18ngrep usage output
> > +   '
> > +done  > +
> 
> These still seem to need further tweaks?
> 
> diff-files
> diff-index
> diff-tree
> rev-list

How so? They pass the test for me, and the output for a manual run looks
sane.

-Peff

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

[Junio C Hamano]

Jeff King  writes:

> This patch just tests that "git foo -h" works for every
> builtin, where we see a 129 exit code (the normal code for
> our usage() helper), and that the word "usage" appears in
> the output.
>
> Signed-off-by: Jeff King 
> ---
>  t/t0012-help.sh | 12 
>  1 file changed, 12 insertions(+)
>
> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> index 8faba2e8b..487b92a5d 100755
> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -49,4 +49,16 @@ test_expect_success "--help does not work for guides" "
>   test_i18ncmp expect actual
>  "
>  
> +test_expect_success 'generate builtin list' '
> + git --list-builtins >builtins
> +'
> +
> +while read builtin
> +do
> + test_expect_success "$builtin can handle -h" '
> + test_expect_code 129 git $builtin -h >output 2>&1 &&
> + test_i18ngrep usage output
> + '
> +done  +

These still seem to need further tweaks?

diff-files
diff-index
diff-tree
rev-list