| Message ID | 20190626235032.177551-3-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | example implementation of revwalk tutorial | expand |
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() > + };
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.
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!
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 },
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(+)