Besides, this hunk is totally unwarranted.
Here are the relevant parts (some of your H_IS_NOT_HELP are not visible
because you needlessly wrapped the lines):
Except for "grep" and "show-ref", none of these have a valid -h option
that means something else.
Considering that this niggle is strictly about "git cmd -h", and not about
"git cmd --otheropt -h somearg", we can even say that "git grep -h" is
asking for help, and not "do not show filenames from match", as there is
no pattern specified.
So I think the right approach is something like how you handled http-push;
namely, check if the sole argument is "-h", and if so show help and exit.
Clarification. the following description only talks about "cmd -h"
without any other options and arguments.
Such a change cannot be breaking backward compatibility for...
* "cherry -h" could be asking to compare histories that leads to our HEAD
and a commit that can be named as "-h". Strictly speaking, that may be
a valid refname, but the user would have to say something like
"tags/-h" to name such a pathological ref already, so I do not think it
is such a big deal.
* "commit-tree -h" is to make a root commit that records a tree-ish
pointed by a tag whose name is "-h". Same as above.
* The first word to "fetch--tool" is a subcommand name, so "fetch--tool -h"
is an error and there cannot be any existing callers. Besides, is it
still being used?
* "grep -h" cannot be asking for suppressing filenames as there is no
match pattern specified.
* "merge-*" strategy backends take the merge base (or "--") as the first
parameter; it cannot sanely be "-h". The callers are supposed to run
rev-parse to make it 40-hexdigit and the command won't see a refname
anyway.
That leaves "show-ref -h". It shows all the refs/* and HEAD, as opposed
to "show-ref" that shows all the refs/* and not HEAD.
Does anybody use "show-ref -h"? It was in Linus's original, and I suspect
it was done only because he thought "it might be handy", not because "the
command should not show the HEAD by default for such and such reasons".
So I think it actually is Ok if "show-ref -h" (but not "show-ref --head")
gave help and exit.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:47:41
Junio C Hamano wrote:
Jonathan Nieder [off-list ref] writes:
quoted
+/*
+ * Let RUN_SETUP, USE_PAGER, and NEED_WORK_TREE take effect even if
+ * passed the -h option.
+ */
+#define H_IS_NOT_HELP (1<<3)
Yuck. Let's think of a way to avoid this ugliness.
Thank you. :)
So I think the right approach is something like how you handled http-push;
namely, check if the sole argument is "-h", and if so show help and exit.
Clarification. the following description only talks about "cmd -h"
without any other options and arguments.
Such a change cannot be breaking backward compatibility for...
[...]
* "grep -h" cannot be asking for suppressing filenames as there is no
match pattern specified.
Okay, here’s a start.
-- %< --
Subject: Show usage string for 'git grep -h'
Clarification: the following description only talks about "git
grep -h" without any other options and arguments.
Such a change cannot be breaking backward compatibility. "grep
-h" cannot be asking for suppressing filenames, as there is no
match pattern specified.
Signed-off-by: Jonathan Nieder <redacted>
---
Is the long usage information really what is wanted here? (I would
think yes, since there is no other way to get that, but sometimes all
I want is a reminder of the non-optional arguments.)
Without something like the previous patch, the usage information is
captured by a pager. I know this is an accidental thing (not all
commands send their -h output through a pager), but it is very
convenient and mitigates the first effect somewhat. Should
whatever -h always use with a pager?
The -h output is very long, and since it goes to standard error,
"git grep -h | head" does not succeed in capturing the best of it.
Usage errors caught in the same function die() currently. I was going
to switch them to usage_msg_opt(), but because of the long usage
message, that would cause the error message to scroll off the
screen...
So I am not totally happy with this. But it is certainly an
improvement over the output from before:
$ git grep -h
fatal: no pattern given.
I’ll sleep on it. Thank you for the advice.
Good night,
Jonathan
builtin-grep.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:47:41
Junio C Hamano wrote:
So I think the right approach is something like how you handled http-push;
namely, check if the sole argument is "-h", and if so show help and exit.
I took a more aggressive approach with some commands. As long as it
was obviously safe and not too complicated to implement, I let -h work
wherever other options work.
Tested with
git_home=$(pwd)
while read a b
do
cmd=${a#git-}
test "$cmd" = "$a" && continue
echo "$cmd"
( cd /tmp && "$git_home/git" "$cmd" -h 2>&1 | head -1 )
done |
less
and the test suite.
Thoughts?
Jonathan Nieder (24):
Retire fetch--tool helper to contrib/examples
Show usage string for 'git grep -h'
Show usage string for 'git cherry -h'
Show usage string for 'git commit-tree -h'
Show usage string for 'git merge-ours -h'
Show usage string for 'git show-ref -h'
check-ref-format: update usage string
Show usage string for 'git check-ref-format -h'
Show usage string for 'git fast-import -h'
Show usage string for 'git get-tar-commit-id -h'
Show usage string for 'git imap-send -h'
Show usage string for 'git mailsplit -h'
Show usage string for 'git merge-one-file -h'
Show usage string for 'git rev-parse -h'
Show usage string for 'git show-index -h'
Show usage string for 'git unpack-file -h'
Show usage string for 'git stripspace -h'
merge: do not setup worktree twice
Let 'git http-fetch -h' show usage outside any git repository
http-fetch: add missing initialization of argv0_path
Let 'git <command> -h' show usage without a git dir
Let usage() take a printf-style format
merge-{recursive,subtree}: use usage() to print usage
diff --no-index: make the usage string less scary
Documentation/git-show-ref.txt | 3 +-
Makefile | 1 -
builtin-check-ref-format.c | 9 +++++++-
builtin-commit-tree.c | 2 +-
builtin-grep.c | 7 ++++++
builtin-log.c | 9 ++++++++
builtin-mailsplit.c | 2 +
builtin-merge-ours.c | 6 +++++
builtin-merge-recursive.c | 2 +-
builtin-merge.c | 1 -
builtin-mv.c | 8 +++---
builtin-read-tree.c | 4 +-
builtin-reflog.c | 3 ++
builtin-rerere.c | 3 ++
builtin-rev-parse.c | 10 +++++++++
builtin-show-ref.c | 10 +++++++-
builtin-stripspace.c | 4 ++-
builtin-tar-tree.c | 6 +++++
builtin.h | 1 -
.../examples/builtin-fetch--tool.c | 0
diff-no-index.c | 2 +-
fast-import.c | 3 ++
git-compat-util.h | 2 +-
git-merge-one-file.sh | 12 ++++++++++
git.c | 22 ++++++++++---------
http-fetch.c | 21 ++++++++++++------
imap-send.c | 5 ++++
index-pack.c | 3 ++
pack-redundant.c | 3 ++
show-index.c | 5 ++++
unpack-file.c | 2 +-
usage.c | 14 ++++++++----
32 files changed, 143 insertions(+), 42 deletions(-)
rename builtin-fetch--tool.c => contrib/examples/builtin-fetch--tool.c (100%)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:47:41
When git-fetch was builtin-ized, the previous script was moved to
contrib/examples. Now, it is the sole remaining user for
'git fetch--tool'.
The fetch--tool code is still worth keeping around so people can
try out the old git-fetch.sh, for example when investigating
regressions from the builtinifaction.
Signed-off-by: Jonathan Nieder <redacted>
---
See also <http://thread.gmane.org/gmane.comp.version-control.git/65122/focus=65191>.
Makefile | 1 -
builtin.h | 1 -
.../examples/builtin-fetch--tool.c | 0
git.c | 1 -
4 files changed, 0 insertions(+), 3 deletions(-)
rename builtin-fetch--tool.c => contrib/examples/builtin-fetch--tool.c (100%)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:47:41
Clarification: the following description only talks about "git
grep -h" without any other options and arguments.
Such a change cannot be breaking backward compatibility. "grep
-h" cannot be asking for suppressing filenames, as there is no
match pattern specified.
Signed-off-by: Jonathan Nieder <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
builtin-grep.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:47:41
Let "git fast-import -h" (with no other arguments) print usage
before exiting, even when run outside any repository.
Cc: Shawn O. Pearce <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
fast-import.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:47:41
This change is strictly about 'git merge-ours -h' without
any other options and arguments.
This change cannot break compatibility since merge drivers are
always passed '--', among other arguments.
Any usage string for this command is a lie, since it ignored its
arguments until now. Still, it makes sense to let the user know
the expected usage when asked.
Signed-off-by: Jonathan Nieder <redacted>
---
builtin-merge-ours.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:47:41
Treat an "-h" option as a request for help, rather than a "Not a
valid object name" error.
"commit-tree -h" could be asking to create a new commit from a
treeish named "-h". Strictly speaking, such a pathological ref
name is possible, but the user would have to had said something
like "tags/-h" to name such a pathological already. commit-tree
is usually used in scripts with raw object ids, anyway.
For consistency, the "-h" option uses its new meaning even if
followed by other arguments.
Signed-off-by: Jonathan Nieder <redacted>
---
builtin-commit-tree.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
@@ -16,6 +16,18 @@# been handled already by git read-tree, but that one doesn't# do any merges that might change the tree layout.+USAGE='<orig blob> <our blob> <their blob> <path>'+USAGE="$USAGE <orig mode> <our mode> <their mode>"+LONG_USAGE="Usage: git merge-one-file $USAGE++Blobidsandmodesshouldbeemptyformissingfiles."++if!test"$#"-eq7+then+echo"$LONG_USAGE"+exit1+fi+case"${1:-.}${2:-.}${3:-.}"in## Deleted in both or deleted in one and unchanged in the other
@@ -1491,6 +1493,9 @@ int main(int argc, char **argv)git_extract_argv0_path(argv[0]);+if(argc!=1)+usage(imap_send_usage);+/* init the random number generator */arc4_init();
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:47:41
"unpack-file -h" could be asking to save the contents of a blob
named "-h". Strictly speaking, such a pathological ref name is
possible, but the user would have to had said something like
"tags/-h" to name such a pathological ref already. When used in
scripts, unpack-file is typically not passed a user-supplied tag
name directly.
Signed-off-by: Jonathan Nieder <redacted>
---
unpack-file.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
@@ -426,6 +426,13 @@ static void die_no_single_rev(int quiet)die("Needed a single revision");}+staticconstcharbuiltin_rev_parse_usage[]=+"git rev-parse --parseopt [options] -- [<args>...]\n"+" or: git rev-parse --sq-quote [<arg>...]\n"+" or: git rev-parse [options] [<arg>...]\n"+"\n"+"Run \"git rev-parse --parseopt -h\" for more information on the first usage.";+intcmd_rev_parse(intargc,constchar**argv,constchar*prefix){inti,as_is=0,verify=0,quiet=0,revs_count=0,type=0;
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:47:41
Builtins do not need to run setup_worktree() for themselves, since
the builtin machinery runs it for them.
Signed-off-by: Jonathan Nieder <redacted>
---
This matter since '-h' cannot suppress _this_ setup_work_tree()
through the builtin machinery.
builtin-merge.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
@@ -843,7 +843,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)constchar*best_strategy=NULL,*wt_strategy=NULL;structcommit_list**remotes=&remoteheads;-setup_work_tree();if(file_exists(git_path("MERGE_HEAD")))die("You have not concluded your merge. (MERGE_HEAD exists)");if(read_cache_unmerged())
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:47:41
Delay search for a git directory until option parsing has finished.
None of the functions used in option parsing look for or read any
files other than stdin, so this is safe.
Signed-off-by: Jonathan Nieder <redacted>
---
http-fetch.c | 21 +++++++++++++--------
1 files changed, 13 insertions(+), 8 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:47:41
Usage messages (for example, from "git merge-recursive -h") are
friendlier when not preceded by "fatal".
Signed-off-by: Jonathan Nieder <redacted>
---
builtin-merge-recursive.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:47:41
merge-recursive and diff --no-index are not able to use usage()
because their usage strings depend on the circumstances in which
they are called.
Signed-off-by: Jonathan Nieder <redacted>
---
git-compat-util.h | 2 +-
usage.c | 14 +++++++++-----
2 files changed, 10 insertions(+), 6 deletions(-)
@@ -36,7 +36,7 @@ static void warn_builtin(const char *warn, va_list params)/* If we are in a dlopen()ed .so write to a global variable would segfault*(ugh),sokeepthingsstatic.*/-staticNORETURN_PTRvoid(*usage_routine)(constchar*err)=usage_builtin;+staticNORETURN_PTRvoid(*usage_routine)(constchar*err,va_listparams)=usage_builtin;staticNORETURN_PTRvoid(*die_routine)(constchar*err,va_listparams)=die_builtin;staticvoid(*error_routine)(constchar*err,va_listparams)=error_builtin;staticvoid(*warn_routine)(constchar*err,va_listparams)=warn_builtin;
@@ -698,6 +698,9 @@ static const char reflog_usage[] =intcmd_reflog(intargc,constchar**argv,constchar*prefix){+if(argc>1&&!strcmp(argv[1],"-h"))+usage(reflog_usage);+/* With no command, we default to showing it. */if(argc<2||*argv[1]=='-')returncmd_log_reflog(argc,argv,prefix);
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:47:41
'git check-ref-format' has learned --branch and --print options
since the usage string was last updated.
Signed-off-by: Jonathan Nieder <redacted>
---
builtin-check-ref-format.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:47:41
Treat an "-h" option as a request for help, rather than an
"Unknown commit -h" error.
"cherry -h" could be asking to compare histories that leads to
our HEAD and a commit that can be named as "-h". Strictly
speaking, that may be a valid refname, but the user would have to
say something like "tags/-h" to name such a pathological ref
already, so it is not such a big deal.
The "-h" option keeps its meaning even if preceded by other
options or followed by other arguments. This keeps the
command-line syntax closer to what parse_options would give and
supports shell aliases like 'alias cherry="git cherry -v"' a
little better.
Signed-off-by: Jonathan Nieder <redacted>
---
builtin-log.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:47:41
This only changes the behavior of "git check-ref-format -h"
without any other options and arguments.
This change cannot be breaking backward compatibility, since any
valid refname must contain a /. Most existing scripts use
arguments such as "heads/$foo". If some script checks the
refname "-h" alone, git check-ref-format will still exit with
nonzero status, and the only detrimental side-effect will be a
usage string sent to stderr.
Signed-off-by: Jonathan Nieder <redacted>
---
builtin-check-ref-format.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:47:41
This only changes the behavior of "git show-ref -h" without any
other options and arguments.
"show-ref -h" currently is short for "show-ref --head", which
shows all the refs/* and HEAD, as opposed to "show-ref" that
shows all the refs/* and not HEAD.
Does anybody use "show-ref -h"? It was in Linus's original, most
likely only because "it might be handy", not because "the command
should not show the HEAD by default for such and such reasons".
So I think it is okay if "show-ref -h" (but not "show-ref
--head") gives help and exits.
If a current script uses "git show-ref -h" without any other
arguments, it would have to be adapted by changing "-h" to
"--head".
Signed-off-by: Jonathan Nieder <redacted>
---
Documentation/git-show-ref.txt | 3 +--
builtin-show-ref.c | 10 ++++++++--
2 files changed, 9 insertions(+), 4 deletions(-)
@@ -183,7 +183,10 @@ static const struct option show_ref_options[] = {OPT_BOOLEAN(0,"heads",&heads_only,"only show heads (can be combined with tags)"),OPT_BOOLEAN(0,"verify",&verify,"stricter reference checking, ""requires exact ref path"),-OPT_BOOLEAN('h',"head",&show_head,"show the HEAD reference"),+{OPTION_BOOLEAN,'h',NULL,&show_head,NULL,+"show the HEAD reference",+PARSE_OPT_NOARG|PARSE_OPT_HIDDEN},+OPT_BOOLEAN(0,"head",&show_head,"show the HEAD reference"),OPT_BOOLEAN('d',"dereference",&deref_tags,"dereference tags into object IDs"),{OPTION_CALLBACK,'s',"hash",&abbrev,"n",
From: Junio C Hamano <hidden> Date: 2016-06-15 22:47:42
Jonathan Nieder [off-list ref] writes:
Builtins do not need to run setup_worktree() for themselves, since
the builtin machinery runs it for them.
Signed-off-by: Jonathan Nieder <redacted>
---
This matter since '-h' cannot suppress _this_ setup_work_tree()
through the builtin machinery.
I think this should directly go to 'maint'. I ejected it from the
series.
Thanks.
From: Junio C Hamano <hidden> Date: 2016-06-15 22:47:42
Jonathan Nieder [off-list ref] writes:
Signed-off-by: Jonathan Nieder <redacted>
---
Why do you have inclusion of "exec_cmd.h" in [19/24]? As far as I can
tell, nothing you do in that patch depends on it.
According to c6dfb39 (remote-curl: add missing initialization of
argv0_path, 2009-10-13), this patch is necessary (and you must include
"exec_cmd.h") on MinGW, regardless of the "give help upon -h" topic.
I think this should be ejected from your series go directly to 'maint', or
am I mistaken?
http-fetch.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
From: Junio C Hamano <hidden> Date: 2016-06-15 22:47:42
Jonathan Nieder [off-list ref] writes:
merge-recursive and diff --no-index are not able to use usage()
because their usage strings depend on the circumstances in which
they are called.
Since die() and warn() are already printf-like, it may be tempting
to do this, but this is wrong.
I do not want to vet all the existing call sites to usage() of make sure
that all of them _happen_ to pass constant strings that do not have any
'%' in them.
Much more importantly, without a patch to future-proof all existing
callsites to modify from
usage(blame_usage);
to
usage("%s", blame_usage);
everybody needs to remember that some *_usage strings are special and have
to double % in it forever, which is a maintenance nightmare.
Besides, the majority of usage strings are _expected_ to be constant.
That is an important difference from die/warn whose purpose is to diagnose
and give appropriate message to the situation (hence they benefit from
formatting).
I've renamed this to usagef() and updated your two callers to use it in
the version I queued to 'pu'.
Thanks.
From: Johannes Sixt <hidden> Date: 2016-06-15 22:47:42
On Dienstag, 10. November 2009, Junio C Hamano wrote:
Jonathan Nieder [off-list ref] writes:
quoted
Signed-off-by: Jonathan Nieder <redacted>
---
Why do you have inclusion of "exec_cmd.h" in [19/24]? As far as I can
tell, nothing you do in that patch depends on it.
According to c6dfb39 (remote-curl: add missing initialization of
argv0_path, 2009-10-13), this patch is necessary (and you must include
"exec_cmd.h") on MinGW, regardless of the "give help upon -h" topic.
I think this should be ejected from your series go directly to 'maint', or
am I mistaken?
You are right.
This command (in bash):
comm <(git grepc -l main\( *.c) <(git grepc -l extract_argv0 *.c)
shows programs in the 1st column that have main(), but do not call
git_extract_argv0_path. One remaining candidate is show-index.c, but its only
call-out is sha1_to_hex(), which doesn't use any other services.
http-fetch.c is the only file that needs this patch.
-- Hannes
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:47:42
Junio C Hamano wrote:
Jonathan Nieder [off-list ref] writes:
quoted
Builtins do not need to run setup_worktree() for themselves, since
the builtin machinery runs it for them.
Signed-off-by: Jonathan Nieder <redacted>
---
This matter since '-h' cannot suppress _this_ setup_work_tree()
through the builtin machinery.
I think this should directly go to 'maint'. I ejected it from the
series.
Thanks. I think something like this should go on top on maint, then
reverted in master.
Sorry for the trouble,
Jonathan
-- %< --
Subject: check-ref-format does not know --print yet
Don’t advertise the --print option of the future in the current
usage string.
Signed-off-by: Jonathan Nieder <redacted>
---
builtin-check-ref-format.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)