[RFC,v2,02/13] walken: add usage to enable -h
diff mbox series

Message ID 20190626235032.177551-3-emilyshaffer@google.com
State New
Headers show
Series
  • example implementation of revwalk tutorial
Related show

Commit Message

Emily Shaffer June 26, 2019, 11:50 p.m. UTC
It's expected that Git commands support '-h' in order to provide a
consistent user experience (and this expectation is enforced by the
test suite). '-h' is captured by parse_options() by default; in order to
support this flag, we add a short usage text to walken.c and invoke
parse_options().

With this change, we can now add cmd_walken to the builtins set and
expect tests to pass, so we'll do so - cmd_walken is now open for
business.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Change-Id: I2919dc1efadb82acb335617ea24371c84b03bbce
---
 builtin/walken.c | 25 +++++++++++++++++++++++++
 git.c            |  1 +
 2 files changed, 26 insertions(+)

Comments

Eric Sunshine June 27, 2019, 4:47 a.m. UTC | #1
On Wed, Jun 26, 2019 at 7:51 PM Emily Shaffer <emilyshaffer@google.com> wrote:
> It's expected that Git commands support '-h' in order to provide a
> consistent user experience (and this expectation is enforced by the
> test suite). '-h' is captured by parse_options() by default; in order to
> support this flag, we add a short usage text to walken.c and invoke
> parse_options().
> [...]
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> diff --git a/builtin/walken.c b/builtin/walken.c
> @@ -5,9 +5,34 @@
> +const char * const walken_usage[] = {
> +       N_("git walken"),
> +       NULL,
> +};

Unless you expect to reference this from multiple functions, there is
no need for it to reside here; instead, it can live inside
cmd_walken(). (And, if you do leave it in the global scope for some
reason, it should be 'static'.)

>  int cmd_walken(int argc, const char **argv, const char *prefix)
>  {
> +       struct option options[] = {
> +               OPT_END()
> +       };
Eric Sunshine June 27, 2019, 4:50 a.m. UTC | #2
On Wed, Jun 26, 2019 at 7:51 PM Emily Shaffer <emilyshaffer@google.com> wrote:
> It's expected that Git commands support '-h' in order to provide a
> consistent user experience (and this expectation is enforced by the
> test suite). '-h' is captured by parse_options() by default; in order to
> support this flag, we add a short usage text to walken.c and invoke
> parse_options().
> [...]
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> diff --git a/builtin/walken.c b/builtin/walken.c
> @@ -5,9 +5,34 @@
>  int cmd_walken(int argc, const char **argv, const char *prefix)
>  {
> +       [...]
> +       /*
> +        * This line is "human-readable" and we are writing a plumbing command,
> +        * so we localize it and use the trace library to print only when
> +        * the GIT_TRACE environment variable is set.
> +        */
>         trace_printf(_("cmd_walken incoming...\n"));

Also, this in-code comment should have been introduced in patch 1/13,
not here in 2/13.
Emily Shaffer June 27, 2019, 6:40 p.m. UTC | #3
On Thu, Jun 27, 2019 at 12:47:25AM -0400, Eric Sunshine wrote:
> On Wed, Jun 26, 2019 at 7:51 PM Emily Shaffer <emilyshaffer@google.com> wrote:
> > It's expected that Git commands support '-h' in order to provide a
> > consistent user experience (and this expectation is enforced by the
> > test suite). '-h' is captured by parse_options() by default; in order to
> > support this flag, we add a short usage text to walken.c and invoke
> > parse_options().
> > [...]
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> > diff --git a/builtin/walken.c b/builtin/walken.c
> > @@ -5,9 +5,34 @@
> > +const char * const walken_usage[] = {
> > +       N_("git walken"),
> > +       NULL,
> > +};
> 
> Unless you expect to reference this from multiple functions, there is
> no need for it to reside here; instead, it can live inside
> cmd_walken(). (And, if you do leave it in the global scope for some
> reason, it should be 'static'.)

Thanks, done.

> 
> >  int cmd_walken(int argc, const char **argv, const char *prefix)
> >  {
> > +       struct option options[] = {
> > +               OPT_END()
> > +       };

Fixed the comment from your other mail too.

Thanks!

Patch
diff mbox series

diff --git a/builtin/walken.c b/builtin/walken.c
index d2772a0e8f..9eea51ff70 100644
--- a/builtin/walken.c
+++ b/builtin/walken.c
@@ -5,9 +5,34 @@ 
  */
 
 #include "builtin.h"
+#include "parse-options.h"
+
+/*
+ * All builtins are expected to provide a usage to provide a consistent user
+ * experience.
+ */
+const char * const walken_usage[] = {
+	N_("git walken"),
+	NULL,
+};
 
 int cmd_walken(int argc, const char **argv, const char *prefix)
 {
+	struct option options[] = {
+		OPT_END()
+	};
+
+	/*
+	 * parse_options() handles showing usage if incorrect options are
+	 * provided, or if '-h' is passed.
+	 */
+	argc = parse_options(argc, argv, prefix, options, walken_usage, 0);
+
+	/*
+	 * This line is "human-readable" and we are writing a plumbing command,
+	 * so we localize it and use the trace library to print only when
+	 * the GIT_TRACE environment variable is set.
+	 */
 	trace_printf(_("cmd_walken incoming...\n"));
 	return 0;
 }
diff --git a/git.c b/git.c
index c2eec470c9..2a7fb9714f 100644
--- a/git.c
+++ b/git.c
@@ -601,6 +601,7 @@  static struct cmd_struct commands[] = {
 	{ "verify-pack", cmd_verify_pack },
 	{ "verify-tag", cmd_verify_tag, RUN_SETUP },
 	{ "version", cmd_version },
+	{ "walken", cmd_walken, RUN_SETUP },
 	{ "whatchanged", cmd_whatchanged, RUN_SETUP },
 	{ "worktree", cmd_worktree, RUN_SETUP | NO_PARSEOPT },
 	{ "write-tree", cmd_write_tree, RUN_SETUP },